-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(temporary-cache): when user is anonymous #20181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20181 +/- ##
==========================================
- Coverage 66.44% 66.41% -0.04%
==========================================
Files 1721 1724 +3
Lines 64548 64629 +81
Branches 6811 6811
==========================================
+ Hits 42890 42921 +31
- Misses 19925 19975 +50
Partials 1733 1733
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 love the changes! Really improved the code's readability and fixed an important bug.
* fix(temporary-cache): fail on anonymous user * make exceptions generic * fix test * remove redundant bool return * fix unit tests (cherry picked from commit 64c4226)
* fix(temporary-cache): fail on anonymous user * make exceptions generic * fix test * remove redundant bool return * fix unit tests
SUMMARY
While testing Explore with an anonymous user, I noticed that the explore form data endpoint errored out due to calling
get_user_id()
on the anonymous user object:This PR introduces a new util function
get_owner
that returns the id if the user is not anonymous, otherwiseNone
, and replaces all direct calls toactor.get_user_id()
withget_owner(actor)
.In addition, I noticed that
TemporaryCache
had a lot of references to Chart, Dashboard and Dataset Exceptions, particularly(((Chart))|(Dataset)|(Dashboard))((NotFound)|(AccessDenied))Error
. To make it more generic, we introduce a new exceptionTemporaryCacheResourceNotFoundError
and reraise all occurrences of the resource-specific exceptions with the new generic one. Since the integration tests already check for the outcome (400, 403, 404 etc), the tests don't need to be updated.The return type for the Explore util
check_access
is also removed, as it isn't needed. The method is expected to raise an exception if access is denied, hence noOptional[bool]
is needed. The unit tests are updated accordingly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION