-
-
Notifications
You must be signed in to change notification settings - Fork 729
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.
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
fix rubocop Rails/ResponseParseBody #13112
fix rubocop Rails/ResponseParseBody #13112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Yassir4 🙏 !
I disabled the RuboCop check on the spec/controllers/user_registrations_controller_spec.rb because the response is returned using format.js not format.json which doesn't work well with the parsed_body method.
It looks like something that should be using format.json
and that we should fix, Would you want to give this a go ?
@@ -2,6 +2,7 @@ | |||
|
|||
require 'spec_helper' | |||
|
|||
# rubocop:disable Rails/ResponseParsedBody | |||
RSpec.describe UserRegistrationsController, type: :controller do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like something that should be using
format.json
and that we should fix
Agreed, this should be fixed if possible.
Alternatively, we shouldn't disable the rubocop rule here. It's a violation that is still "to do"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will tackle it.
The UserRegistrationsController#create is not yet used for signup, I saw an open issue for merging Spree::UserRegistrationsController into UserRegistrationsController
, so changing the format won't break anything
14737a9
to
8f0c3c9
Compare
8f0c3c9
to
69f3f9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome ! thank you @Yassir4 🙏
FYI for next time, we tend to prefer rebasing the branch on master, rather than merging master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've added a testing note to check the user registration.
Hey @Yassir4, Thanks for this PR as well. (Thank you @dacook for the notes on what to test). Before this PR After this PR All working as before. Merging. |
What? Why?
Note
I disabled the RuboCop check on the
spec/controllers/user_registrations_controller_spec.rb
because the response is returned using format.js not format.json which doesn't work well with the parsed_body method.What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):