Rails 5.2 upgrade

[Souravirus]

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
• code is in uniquely-named feature branch and has no merge conflicts
• PR is descriptively titled
• PR body includes fixes #0000-style reference to original issue #
• ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

[jywarren]

It keeps on coming! 😄

[Souravirus]

Yeah 😄

[plotsbot]

	2 Messages
📖	@Souravirus Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖	It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

[jywarren]

Oh, wow, passing already? Is this ready?

[Souravirus]

No, some deprecation warning to be cleared then it will be ready

[jywarren]

Cool, how about now? and... WHATS AFTER 5.2??? 😆

[Souravirus]

Yeah only one deprecation warning is left. I am behind it and after that we will be ready to merge. Actually, we should stop at rails 5.2. what do you think? I guess we should do rails 6.0 update later some time. And I don't know what to do after rails 5.2 upgrade. Is there any project which I could work. I would be really happy working at it. Thank you!!

[Souravirus]

its ready to merge @jywarren. That deprecation warning was due to sqlite3 gem which will removed by them later.

[jywarren]

Whoa, what's this? How does it work?

[Souravirus]

It's basically removing the deprecation warning :
DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "max(node_revisions.timestamp) DESC, node.nid". 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 places at /app/app/controllers/notes_controller.rb:25)

and as they have written in the warning itself that Known-safe values can be passed by wrapping them in Arel.sql(). So, I followed them.

[jywarren]

OK, awesome - maybe we should insert a comment by this usage referencing this OK usage?

…

On Mon, Jul 2, 2018 at 5:24 PM Sourav Sahoo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/controllers/notes_controller.rb <#2927 (comment)>: > @@ -22,7 +22,7 @@ def places .references(:term_data) .where('term_data.name = ?', 'chapter') .group('node.nid') - .order('max(node_revisions.timestamp) DESC, node.nid') + .order(Arel.sql('max(node_revisions.timestamp) DESC, node.nid')) It's basically removing the deprecation warning : DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "max(node_revisions.timestamp) DESC, node.nid". 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 places at /app/app/controllers/notes_controller.rb:25) and as they have written in the warning itself that Known-safe values can be passed by wrapping them in Arel.sql(). So, I followed them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2927 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJx4xHUCACfMxiEx6uxiHXAPJJNXBks5uCo9mgaJpZM4U4wPO> .

[Souravirus]

Ok about this arel usage right? I will insert a comment. Thank you!!

[Souravirus]

@jywarren, Check the comments I have added. Thank you!!

[jywarren]

Looks great! Ill merge now. 🎉

[Souravirus]

Thanks @jywarren