[openstreetmap/openstreetmap-website] Before/after id pagination for user diary comments (PR #4239)

[Tom Hughes]

@tomhughes commented on this pull request.



> +  def get_page_items(items, includes)
+    id_column = "#{items.table_name}.id"
+    page_items = if params[:before]
+                   items.where("#{id_column} < ?", params[:before]).order(:id => :desc)
+                 elsif params[:after]
+                   items.where("#{id_column} > ?", params[:after]).order(:id => :asc)
+                 else
+                   items.order(:id => :desc)
+                 end
+
+    page_items = page_items.limit(20)
+    page_items = page_items.includes(includes)
+    page_items = page_items.sort.reverse
+
+    newer_items_id = page_items.count.positive? && items.exists?(["#{id_column} > ?", page_items.first.id]) && page_items.first.id
+    older_items_id = page_items.count.positive? && items.exists?(["#{id_column} < ?", page_items.last.id]) && page_items.last.id

I think it would be better to write these as guarded assignments - it took me quite a while to figure out what you were doing. I suggest doing:

```ruby
newer_items_id = page_items.first.id if page_items.count.positive? && items.exists?(["#{id_column} > ?", page_items.first.id])
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4239#pullrequestreview-1617922205
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4239/review/1617922205 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230908/9d749f64/attachment-0001.htm>