Prevent dangerous query method on #order_by_id

[sejinkim1904]

What does this PR do?

• Adds Arel.sql() to #order_by_id in /lib/order.rb to prevent deprecation warning for dangerous query method.
• Adds test for sorting by has_many association

Where should the reviewer start?

/lib/order.rb

How should this be manually tested?

• Temporarily remove Arel.sql() from #order_by_id
• Run bundle exec appraisal rails52 rspec spec/features/index_page_spec.rb

What are the relevant tickets?

#1758

Questions:

• Do Migrations Need to be ran? NO
• Do Environment Variables need to be set? NO
• Any other deploy steps? NO

[nickcharlton]

Thanks!

Can you think of a way we can test this change? Running the specs currently doesn't throw this warning and ideally I'd like to be able to see the warning, apply the (tiny!) fix and then see it go away again.

[sejinkim1904]

I will try to recreate the warning and get back to you

[sejinkim1904]

@nickcharlton I spent the last couple days trying to recreate the warning in my forked repo but haven't had any luck. However, we recently migrated the app where I originally ran into this deprecation warning to rails 6 and we're not seeing the warning anymore.

I did some digging into the rails code base and found this but still unsure why I am not seeing the deprecation warning anymore.

[pablobm]

Had a look at what's possible. How about adding the following to spec/features/index_page_spec.rb?

  it "sorts by count on a has_many association" do
    create_list(:order, 2, customer: create(:customer, name: "Ade"))
    create_list(:order, 3, customer: create(:customer, name: "Ben"))
    create_list(:order, 1, customer: create(:customer, name: "Cam"))

    visit admin_customers_path

    click_on "Orders"
    expect(page).to have_content(/Cam.*1 order.*Ade.*2 orders.*Ben.*3 orders/)

    click_on "Orders"
    expect(page).to have_content(/Ben.*3 orders.*Ade.*2 orders.*Cam.*1 order/)
  end

Running this with Appraisal gemfile for Rails 5.2 will trigger the warning on master:

$ bundle exec appraisal rails52 rspec spec/features/index_page_spec.rb
(...)
DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "COUNT(orders.id) asc". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from order_by_count at /home/pablobm/Documents/projects/administrate/gem/lib/administrate/order.rb:65)
DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "COUNT(orders.id) desc". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from order_by_count at /home/pablobm/Documents/projects/administrate/gem/lib/administrate/order.rb:65)

On your branch, the warning will not appear.

[sejinkim1904]

@pablobm Thanks for this, I was actually able to recreate the deprecation warning on my branch by adding this test, temporarily removing Arel.sql, and running bundle exec appraisal rails52 rspec spec/features/index_page_spec.rb. I guess I should have checked to see if any of the feature tests were checking sorting on the dashboards first.

[sejinkim1904]

Updated PR message with new changes

[pablobm]

This looks good to me 👍