Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve order when generating CSV #3575

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pcai
Copy link
Contributor

@pcai pcai commented Nov 26, 2022

Resolves #2580

Generate CSVs using kaminari pagination when available (e.g., when @objects is an ActiveRecord_Relation) instead of using the native rails find_each.

This allows us to preserve the activerecord ORDER BY clause defined when browsing records before attempting to export them; find_each overrides the ORDER BY to use the primary key id.

One potential downside to this is that it will make certain large exports much less efficient, as paginating large tables ordered by a column without the correct database indexes will run into algorithmic problems.

Issue reproduction:

  • Navigate to a records index page and change the sort ordering (e.g., sort by "Name descending"):
    image
  • Click "export found _____"
  • generate the CSV

EXPECTED:

  • the generated CSV has the sort ordering from the previous step

ACTUAL:

  • the generated CSV is in primary key id order

@coveralls
Copy link

coveralls commented Nov 26, 2022

Coverage Status

coverage: 96.044% (+0.08%) from 95.965% when pulling d90183f on pcai:order-csv-exports into e158beb on railsadminteam:master.

end
end

context 'when objects are ordered' do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is okay to you, please also consider matching whole records. This will make sure of order even for other code changes.

context 'when objects are ordered' do
  # or use before block as before
  let!(players) do
    FactoryBot.create_list :player, 2 do |player, index|
      player.name = "Player #{index}"
    end
  end

  let(:objects) { Player.all.order(name: :desc) }
  let(:options) { {} }

  # modify the subject block to return the array of objects
  it 'preserves the ordering' do
    expect(subject[2].split("\n")[1]).to eq('Player 1', 'Player 2')
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I incorporated some of your suggestions, but matching the whole record is a bit nontrivial because the pk ids are not predictable when randomizing test seed

@pcai pcai force-pushed the order-csv-exports branch 3 times, most recently from 8f5bf70 to 7dfb730 Compare July 12, 2023 16:42
@pcai
Copy link
Contributor Author

pcai commented Jul 1, 2024

I'm still looking for feedback on this. I don't mind keeping it up to date and passing CI, but I would appreciate some guidance on whether the change would be accepted at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV export doesn't keep order
3 participants