Resolve deprecation warning in preparation for Rails 6 (eventually)

[jywarren]

Travis reports this warning several times:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "COUNT(term_data.tid) 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 block in related at /app/app/models/tag.rb:328)

I believe we should wrap term_data.tid as recommended, like Arel.sql(term_data.tid) but am not 100% sure. This should make our test reports much more readable!

https://travis-ci.org/publiclab/plots2/builds/479677570#L3230

The line change should be here, i guess:

plots2/app/models/tag.rb

Line 328 in 3e6ce6b

.order('COUNT(term_data.tid) DESC')

[jywarren]

This has been marked as a good candidate for becoming a first-timers-only issue like these, meaning that it's simple, self-contained, and with some extra formatting, could be a great entry point for a new contributor. If you're familiar enough with this code, please consider reformatting or reposting it as a first-timers-only issue, and then ping @publiclab/reviewers to get it labelled. Or, if this is not your first time, try to solve it yourself!

[IshaGupta18]

Okay @jywarren so the deprecation warning has changed I think (atleast at my local machine) to this:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "created ". 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 tagNearbyNodes at /home/isha/plots2/app/services/search_service.rb:136)

Also, the line 328 has now been changes to this:

plots2/app/models/tag.rb

Line 328 in 08d0f7d

.order(count: :desc)

So I think we should probably address this new warning now, if I am correct. What do you think?

[IshaGupta18]

Okay so changing this line:

plots2/app/services/search_service.rb

Line 136 in 08d0f7d

items.order("created #{order_direction}")

to this:
items.order("created #{Arel.sql(order_direction)}")
gives these errors:

ERROR["test_running_search_taglocations_with_valid_params", #<Minitest::Reporters::Suite:0x0000561e3d5ab9d0 @name="SearchServiceTest">, 71.05786943600106]
 test_running_search_taglocations_with_valid_params#SearchServiceTest (71.06s)
TypeError:         TypeError: no implicit conversion of nil into String
            app/services/search_service.rb:136:in `tagNearbyNodes'
            test/unit/api/search_service_test.rb:67:in `block in <class:SearchServiceTest>'

ERROR["test_running_search_taglocations_with_valid_params_and_specific_period", #<Minitest::Reporters::Suite:0x0000561e3d529980 @name="SearchServiceTest">, 71.07244423100201]
 test_running_search_taglocations_with_valid_params_and_specific_period#SearchServiceTest (71.07s)
TypeError:         TypeError: no implicit conversion of nil into String
            app/services/search_service.rb:136:in `tagNearbyNodes'
            test/unit/api/search_service_test.rb:75:in `block in <class:SearchServiceTest>'

What do you think should be done?

[snpd25]

@IshaGupta18 What if we change the line 328 in tag.rb from:
.order(count: :desc)
to:
.order(Arel.sql('COUNT(term_data.tid) DESC'))

[IshaGupta18]

I am so sorry @snpd25 I missed your comment, let me have a look at it ASAP.

[IshaGupta18]

I think that issue was probably resolved, could you have a look the Deprecation warning I have posted in #4624 (comment) ? Thanks a lot!