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.

Sign up for GitHub

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

Jump to bottom

Rails 5.1 upgrade #2798

Merged
merged 9 commits into from
Jun 21, 2018
Merged

Rails 5.1 upgrade #2798

merged 9 commits into from
Jun 21, 2018

Conversation

Souravirus
Copy link
Member

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!

@ghost ghost assigned Souravirus Jun 8, 2018
@ghost ghost added the in progress label Jun 8, 2018
@plotsbot
Copy link
Collaborator

plotsbot commented Jun 13, 2018 •
edited
Loading

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

@Souravirus Souravirus closed this Jun 18, 2018
@ghost ghost removed the in progress label Jun 18, 2018
@Souravirus Souravirus reopened this Jun 18, 2018
@ghost ghost added the in progress label Jun 18, 2018
@Souravirus Souravirus mentioned this pull request Jun 18, 2018
Closed
@Souravirus
Copy link
Member Author

Souravirus commented Jun 18, 2018 •
edited
Loading

Hi @jywarren, I am currently stuck on this error with OpenID:
NoMethodError: NoMethodError: undefined method 'length' for #<ActionController::Parameters:0x0000000ec966d0> app/controllers/openid_controller.rb:19:in 'index' test/integration/openid_test.rb:58:in block in <class:LoginFlowTest>'
I cannot trace out where is the location of the length method. It would be great if you help me out with this error. As I don't have much knowledge about OpenID.

@jywarren
Copy link
Member

Hmm, the error leads to decode_request:

plots2/app/controllers/openid_controller.rb

Line 19 in caf18c8

oidreq = server.decode_request(params)

Maybe a clue here: http://edgeapi.rubyonrails.org/classes/ActionController/Parameters.html

I might try to look for errors with decode_request on stack overflow, and esp. if things have changed in Rails 5 - i see here, for example: https://stackoverflow.com/questions/17240106/what-is-the-best-way-to-convert-all-controller-params-from-camelcase-to-snake-ca

and in general it looks like it's expecting an array or a string, but getting a Parameters object, which has no length property. If you are still stuck i can try digging deeper!

@Souravirus
Copy link
Member Author

Souravirus commented Jun 19, 2018 •
edited
Loading

Thanks for the hints @jywarren, I am now trying to remove this error.

@Souravirus
Copy link
Member Author

Open_id authentication is now working fine.

@jywarren
Copy link
Member

jywarren commented Jun 20, 2018 via email

@Souravirus Souravirus requested review from a team, jywarren and siaw23-retired June 20, 2018 19:37
@Souravirus Souravirus requested a review from icarito June 20, 2018 19:37
@jywarren
Copy link
Member

Ooh wow! A bit easier than last one, huh? 😄

@Souravirus
Copy link
Member Author

Yeah this was quite easy 😄

@jywarren
Copy link
Member

Shall we merge it then? :-)

@jywarren
Copy link
Member

@publiclab/reviewers can someone else look through this? I'd like to merge and publish tomorrow but want to be sure. Thanks!

@jywarren
Copy link
Member

Were you able to test it with the docs in https://github.com/publiclab/plots2/blob/master/doc/OPENID.md ?

@Souravirus
Copy link
Member Author

Yeah we can merge it. Yeah I tested the open id authentication as is written in this link https://github.com/publiclab/plots2/blob/master/doc/OPENID.md and it's working fine. Thank you !!

sagarpreet-chadha
sagarpreet-chadha approved these changes Jun 21, 2018
View reviewed changes
Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Great work 👍 !

@jywarren jywarren merged commit 860d341 into master Jun 21, 2018
@ghost ghost removed the ready label Jun 21, 2018
@jywarren
Copy link
Member

THANK YOU @Souravirus and @sagarpreet-chadha !!!

@jywarren
Copy link
Member

This is now live!

@Souravirus Souravirus deleted the rails5.1_upgrade branch June 21, 2018 19:16
siaw23-retired
siaw23-retired reviewed Jun 22, 2018
View reviewed changes
app/controllers/openid_controller.rb
'openid.trust_root',
'openid.id_select',
'openid.immediate',
'openid.cancel_url').to_h
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw i think you should put this in a before_action, and make a method to return permitted_params but make it private. @Souravirus

PS: sorry for late review. busy.

@jywarren jywarren mentioned this pull request Feb 26, 2019
Open
20 tasks
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@Souravirus @jywarren
Rails 5.1 upgrade (publiclab#2798)
5e1a2ff 
* Gemfile change for upgrade to rails 5.1

* Downgraded rails to 5.1.5

* Removed errors related to passing parameters

* converted the ActionController::Parameters into hash in openid_controller

* Added some more openid params

* modified all render text to render plain

* Added identation in openid_controller

* Fixed codeclimate issues

* Removed all deprecation warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siaw23-retired siaw23-retired siaw23-retired left review comments

@sagarpreet-chadha sagarpreet-chadha sagarpreet-chadha approved these changes

@jywarren jywarren Awaiting requested review from jywarren

@icarito icarito Awaiting requested review from icarito

Assignees

@Souravirus Souravirus

Labels
summer-of-code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Souravirus @plotsbot @jywarren @siaw23-retired @sagarpreet-chadha