-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS #24400
chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS #24400
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24400 +/- ##
==========================================
- Coverage 68.95% 68.91% -0.05%
==========================================
Files 1903 1904 +1
Lines 74070 73913 -157
Branches 8110 8118 +8
==========================================
- Hits 51077 50939 -138
+ Misses 20881 20864 -17
+ Partials 2112 2110 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 cleanup. I left a couple of comments, but otherwise LGTM.
superset/views/core.py
Outdated
payload = { | ||
"user": bootstrap_user_data(user, include_perms=True), | ||
"user": bootstrap_user_data(g.user, include_perms=True), |
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.
@dpgaspar what happens if the user is a guest user? Is g.user
guaranteed to exist and be non-None
.
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.
Good point, it should be AnonymousUser
from flask-login or GuestUser
, but added some safe guard, and we're returning 404 now
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.
LGTM
@expose("/profile/<username>/") | ||
def profile(self, username: str) -> FlaskResponse: | ||
@expose("/profile/") | ||
def profile(self) -> FlaskResponse: |
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.
❤️
if error_obj := self.get_user_activity_access_error(user_id): | ||
return error_obj | ||
|
||
user = g.user if hasattr(g, "user") and g.user else None |
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 think this snippet or a variation of it appears in LOTS of places. We should replace all of these with a single util to DRY this up and add tests for all expected cases. I can do it if needed (I think this PR is big enough already, so it should not be obfuscated with more chores).
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.
totally agree! would be great also to make anonymous users and guest user's more clear to check, use and test
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.
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.
LGTM
if error_obj := self.get_user_activity_access_error(user_id): | ||
return error_obj | ||
|
||
user = g.user if hasattr(g, "user") and g.user else None |
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.
SUMMARY
Removes old deprecated APIs:
/superset/recent_activity/...
/superset/fave_dashboards_by_username/...
/superset/fave_dashboards/...
/superset/created_dashboards/...
/superset/user_slices/...
/superset/created_slices/...
/superset/fave_slices/...
/superset/favstar/...
This PR also removes
ENABLE_BROAD_ACTIVITY_ACCESS
feature flag (was disabled by default), since by removing the above mentioned deprecated API's only one final API (/api/v1/log/recent_activity/
) was using it.Also removes
changed_by_url
andcreated_by_url
REST API response fields from datasets, dashboards and charts, since these fields function was to provide a link to other user's profiles.Effort on removing deprecated APIs from
/superset
: #24332BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION