Remove to_s from Arel.sql statements

[microstudi]

🎩 What? Why?

As far as I know, the Arel.sql method is use to inject safe SQL inside queries. by using to_s after it, it becomes unsafe SQL again. Until now it didn't seem to be a problem but we found that is affecting the Proposal's scope sort_by_valuation_assignments_count_asc/desc.

This PR, removes all to_s statements (note that the other might be irrelevant as Rails might not find it dangerous), but still I think we should not use it.

Looks like this is from Decidim 0.28 onwards.

📌 Related Issues

Link your PR to an issue

• Related to #?
• Fixes #?

Testing

1. Enter the console of any Decidim (bin/rails c)
2. Run the following:
   Decidim::Proposals::Proposal.sort_by_valuation_assignments_count_asc
3. See this error:

lib/active_record/sanitization.rb:145:in `disallow_raw_sql!': Query method called with non-attribute argument(s): "( SELECT COUNT(decidim_proposals_valuation_assignments.id) FROM decidim_proposals_valuation_assignments WHERE decidim_proposals_valuation_assignments.decidim_proposal_id = decidim_proposals_proposals.id GROUP BY decidim_proposals_valuation_assignments.decidim_proposal_id ) ASC NULLS FIRST" (ActiveRecord::UnknownAttributeReference)

4. Apply this fix
5. See it working normally

📷 Screenshots

Please add screenshots of the changes you are proposing

♥️ Thank you!

[request-info]

It seems like you did not give us much information about what you are trying to do here. We would appreciate it if you could provide us with more info about this issue/PR!

[github-actions]

This pull request does not contain a valid label. Please add one of the following labels: ['type: feature', 'type: change', 'type: fix', 'type: removal', 'target: developer-experience', 'type: internal']

[github-actions]

This pull request contains invalid labels. Please remove all of the following labels: ['type: bug']

[alecslupu]

@microstudi, thanks for the PR. I was looking at sort_by_valuation_assignments_count_asc implementation and it really seems this very poor ideea to do a sort. I just wonder if we could rewrite those methods, so that we use a simple counter cache, and avoid that additional subquery.

[microstudi]

You're right @alecslupu, this is just a quick fix as it affects the valuator's part in external extensions we have. Maybe a maintenance task should look for subotimal sql queries and do this kind of refactoring.

[alecslupu]

@microstudi , let me know if you want to continue fixing this PR, or you can work with the fix i have opened in #12669

[andreslucena]

@microstudi have you seen the last comment from Alex? You'd need to fix the git conflicts too, and fix the specs that are failing.

[microstudi]

@andreslucena is #12669 going to be backported?
I'll finish this anyway yes

[alecslupu]

@andreslucena is #12669 going to be backported? I'll finish this anyway yes

@microstudi, yes. #12669 is labeled as fix, so it will be backported.

[microstudi]

Let's discard this, we should probably in the future asses those complicate sql statements but my case should be solve with #12669. Thanks Alex!