-
-
Notifications
You must be signed in to change notification settings - Fork 701
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
[14.0] [FIX] website_require_login: hidden exception #1036
[14.0] [FIX] website_require_login: hidden exception #1036
Conversation
36a0ddc
to
f3249ff
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.
Functional ok!
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.
Code review, LGTM
@pedrobaeza good for you? |
# it means that an exception has been | ||
# raised and the cursor is currently closed | ||
try: | ||
user_id = website.user_id |
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.
Incorrect variable name:
user_id = website.user_id | |
user = website.user_id |
# raised and the cursor is currently closed | ||
try: | ||
user_id = website.user_id | ||
except Exception: |
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.
Too broad exception. Put a stricter restriction for the use case.
user_id = website.user_id | ||
except Exception: | ||
return res | ||
|
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's better to not add empty lines inside a method, as its value is very limited and makes you scroll more.
f3249ff
to
1963e8c
Compare
Thanks for the feedback @pedrobaeza, I've addressed all of the issues, could you please give it another look? |
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.
/ocabot merge patch
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 399ca0d. Thanks a lot for contributing to OCA. ❤️ |
Fixed an issue where if an exception has been raised, it would try to access a closed cursor, causing the disconnection and re-connection from the website, totally hiding the exception message.
Traceback (partial):
pre-commit is fixed in: #1037