-
Notifications
You must be signed in to change notification settings - Fork 110
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
Issue #561 - Fix for ensuring Json format rendering errors no longer #3015
Issue #561 - Fix for ensuring Json format rendering errors no longer #3015
Conversation
This PR was created to start a conversation on changes I made based on @briri's comments https://github.com/DigitalCurationCentre/DMPonline-Service/issues/561#issuecomment-872306836 Attached is image of an error with Json that previously returned html |
effff20
to
9c3ded6
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.
Thanks, this looks good @johnpinto1
I just question the use of 404 in the user_not_authorized section. @raycarrick-ed what do you think?
It also looks like a test broke due to the change. It may be that the test just needs to be updated to accommodate this change in error handling rather than there being something wrong with your changes.
redirect_to plans_url, alert: _("You are not authorized to perform this action.") | ||
# redirect_to plans_url, alert: _("You are not authorized to perform this action.") | ||
msg = _("You are not authorized to perform this action.") | ||
render_respond_to_format_with_error_message(msg, plans_url, 404, nil) |
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.
Should this be a 403 - forbidden?
redirect_to root_url, alert: _("You need to sign in or sign up before continuing.") | ||
# redirect_to root_url, alert: _("You need to sign in or sign up before continuing.") | ||
msg = "You need to sign in or sign up before continuing." | ||
render_respond_to_format_with_error_message(msg, root_url, 404, nil) |
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.
Should this be a 401 - unauthorized?
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.
Agreed.
404 = resource not found which isn't the case here.
if repeating the request after authenticating would work then should be a 401, I think.
if even after authenticating you still wouldn't be allowed the it's 403
That's my understanding anyway.
21429fc
to
65034f4
Compare
@briri @raycarrick-ed Updated code as suggested. Thanks for feedback. |
@johnpinto1 thanks for making those changes. It looks like the test is failing because it was expecting to be redirected to the "My dashboard" page when the user is unauthorized. I think you can just update the test itself to expect |
Thanks @briri my bad. As usual I never looked at tests, assumed error were db related. Will update tomorrow. |
65034f4
to
3c9c1a4
Compare
@briri & @raycarrick-ed To fix tests I had to make the following changes (not sure about whether removing "format: :js" is acceptable:
|
return the standard html error page. Changes: In ApplicationController: - so that rescue_from errors are rendered appropriately formatted messages for html and json with a private method render_respond_to_format_with_error_message() - added a rescue_from StandardError with a method handle_server_method() - updated existing user_not_authorized - updated rescue_from ActiveRecord::RecordNotFound by removing if Production condition - added missing method render_not_found() In spec/controllers/super_admin/orgs_controller_spec.rb failing tests (1) merge_analyze: test "fails if user is not a super admin" (2) merge_commit: test "fails if user is not a super admin" - changed redirect_to(plans_path) -> redirect_to(root_path) - removed "format: :js" because of ActionController::UnknownFormatError post :merge_analyze, params: @params. format: :js and post :merge_commit, params: @params. format: :js
3c9c1a4
to
e770d28
Compare
looks good. Thanks @johnpinto1! |
…should_return_json_not_html_page Issue #561 - Fix for ensuring Json format rendering errors no longer
return the standard html error page.
Fixes #561