-
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
Fix superset error message flow #5540
Conversation
e435057
to
c27bb41
Compare
33d443c
to
be605b7
Compare
Codecov Report
@@ Coverage Diff @@
## master #5540 +/- ##
==========================================
+ Coverage 63.29% 63.29% +<.01%
==========================================
Files 349 349
Lines 22188 22194 +6
Branches 2467 2467
==========================================
+ Hits 14043 14047 +4
- Misses 8131 8133 +2
Partials 14 14
Continue to review full report at Codecov.
|
superset/security.py
Outdated
t for t in superset_query.tables if not | ||
self.datasource_access_by_fullname(database, t, schema)] | ||
if rejected: | ||
return [':'.join(self.get_schema_and_table(i, schema)) for i in rejected] |
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.
This should be removed (discussed offline). Returning a list of tables (w/ schema) seems suffice.
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.
Agree. Done.
be605b7
to
6d4a18a
Compare
Don't you still want to return the full table name in |
It's legacy so it's okay if we leave it the way it is and leave it to each deployment to override as they see fit. |
(cherry picked from commit 4bf69a7)
This PR fixes several minor issues in the access request flow and the process of allowing the security manager provide the information for error messages.
I will follow up with a PR that allows administrators fully customize the links added beside error message. This will allow them control both the url and the text to be clicked.
@graceguo-supercat @john-bodley @michellethomas