Uysim / pagy-cursor

Cursor pagination with pagy for Ruby On Rails
MIT License
126 stars 23 forks source link

Ordering bug #38

Closed AlexMooney closed 1 year ago

AlexMooney commented 2 years ago

https://github.com/Uysim/pagy-cursor/pull/14 introduced the ability to add ordering by other fields, but there's a bug where records can appear multiple times. This happens because the objects are being compared by created_at and id but not with the other fields in the order.

This spec passes, but post79 should not occur twice in the paging ```ruby context 'with ordered records' do before do Post.destroy_all 1.upto(100) do |i| Post.create!(title: "post#{i}", created_at: (100-i).minutes.ago) end post = Post.find_by(title: "post79") post.update(title: "This is post79") end it "paginates with an order specified" do pagy, records = backend.send( :pagy_uuid_cursor, Post.all, order: { updated_at: :desc } ) expect(records.map(&:title)).to eq( ["This is post79", "post100", "post99", "post98", "post97", "post96", "post95", "post94", "post93", "post92", "post91", "post90", "post89", "post88", "post87", "post86", "post85", "post84", "post83", "post82"]) expect(pagy.has_more?).to eq(true) expect(pagy.order).to eq(created_at: :desc, "id"=>:desc, updated_at: :desc) pagy2, records2 = backend.send( :pagy_uuid_cursor, Post.all, order: { updated_at: :desc }, before: records.last.id ) expect(records2.map(&:title)).to eq( ["This is post79", "post81", "post80", "post78", "post77", # 🐞 post 79 is on page 1 and 2! "post76", "post75", "post74", "post73", "post72", "post71", "post70", "post69", "post68", "post67", "post66", "post65", "post64", "post63", "post62"]) expect(pagy2.has_more?).to eq(true) expect(pagy2.order[:updated_at]).to eq(:desc) pagy3, records3 = backend.send( :pagy_uuid_cursor, Post.all, order: { updated_at: :desc }, before: records2.last.id ) expect(records3.map(&:title)).to eq( ["post61", "post60", "post59", "post58", "post57", "post56", "post55", "post54", "post53", "post52", "post51", "post50", "post49", "post48", "post47", "post46", "post45", "post44", "post43", "post42"]) expect(pagy3.has_more?).to eq(true) expect(pagy3.order[:updated_at]).to eq(:desc) end ```
Uysim commented 1 year ago

@AlexMooney fixed in version 0.6.1. Credit @h-michael