This is extracted from 
https://github.com/openstreetmap/openstreetmap-website/pull/7030, with view to 
reducing the size of that PR as well as clarify something.

Currently, `get_page_items` simply uses `sort` to order the items. This appears 
to work. However it doesn't work when the items are of different classes, even 
if they share an ActiveRecord superclass and DB table. This is going to be a 
problem when paginating notifications, which are of different classes that 
inherit from `Noticed::Notification` and share the `noticed_notifications` 
table.

This is because ActiveRecord defines `<=>` as follows:

https://github.com/rails/rails/blob/9ecc4a5ca6fb9a93a1f243e8f23d8ba592f41600/activerecord/lib/active_record/core.rb#L674-L680

So if I'm understanding correctly, it allows comparing a record class and a 
subclass, but not two subclasses of the same abstract superclass.

Anyway, not a big deal if we just are explicit and order by `cursor_column`, as 
introduced in this PR...

...which while looking into this, I realised surely we should have been doing 
in the first place? From what I can see, the argument `cursor_column` was not 
being used correctly and needed this change I'm making here anyway. This led me 
to add a second test which is relevant without the context of the upcoming 
notifications page.
You can view, comment on, or merge this pull request online at:

  https://github.com/openstreetmap/openstreetmap-website/pull/7190

-- Commit Summary --

  * Make PaginationMethods work with subclasses of a common AR model

-- File Changes --

    M app/controllers/concerns/pagination_methods.rb (2)
    A test/controllers/concerns/pagination_methods_test.rb (43)
    A test/factories/notifications.rb (19)

-- Patch Links --

https://github.com/openstreetmap/openstreetmap-website/pull/7190.patch
https://github.com/openstreetmap/openstreetmap-website/pull/7190.diff

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

Message ID: <openstreetmap/openstreetmap-website/pull/[email protected]>
_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev

Reply via email to