-
-
Notifications
You must be signed in to change notification settings - Fork 730
[16.0][FIX] website_require_login: Error response #1099
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
base: 16.0
Are you sure you want to change the base?
Conversation
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
|
@pedrobaeza could you remove the stale tag please? |
|
OK, but please enroll people to review it. |
|
@yvaucher @NICO-SOLUTIONS could you review please? |
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.
Havent seen this effect up to now but it makes sense!
in generel i would place the check for Content-Type == "application/json" before assigning the redirect_path. but thats academic.
LGTM!
c5c4a3e to
58037bf
Compare
| ) | ||
|
|
||
| def test_dispatch_json_no_redirect(self): | ||
| """Con Content-Type application/json no debe haber redirección.""" |
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.
Please use English
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.
@tarteo done!
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.
@tarteo could you check again? thanks!
58037bf to
fb35027
Compare
SirPyTech
left a comment
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 for the PR!
I only reviewed the code: I just suggested an optimization.
Please have a look at #1160 that is quite similar but for backend requests 😁
| path = request.httprequest.path | ||
| for auth_path in auth_paths: | ||
| if auth_path == path or Path(auth_path) in Path(path).parents: | ||
| if request.httprequest.content_type == "application/json": |
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.
suggestion: I'd place this close to the checks at the beginning of the method so that if this fails then we can skip everything else, like the search method.
| "Expected the response status code to be 200 which means no redirection", | ||
| ) | ||
|
|
||
| def test_dispatch_json_no_redirect(self): |
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.
praise: Thanks for adding the test!
Error in response in first load:
