Skip to content
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

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Aug 1, 2018

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.

  1. Changes the link on sqllab errors to be access request as opposed to 'common error messages'.
  2. Fixes a bug with passing the link to the frontend.

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

@timifasubaa timifasubaa force-pushed the fix_sqllab_error_flow branch 5 times, most recently from e435057 to c27bb41 Compare August 1, 2018 19:01
@timifasubaa timifasubaa changed the title [WIP] fix superset error message flow Fix superset error message flow Aug 1, 2018
@timifasubaa timifasubaa force-pushed the fix_sqllab_error_flow branch 2 times, most recently from 33d443c to be605b7 Compare August 1, 2018 20:34
@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

Merging #5540 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
superset/views/core.py 74.17% <ø> (ø) ⬆️
...uperset/assets/src/SqlLab/components/ResultSet.jsx 84.84% <0%> (ø) ⬆️
superset/assets/src/SqlLab/actions.js 71.25% <100%> (ø) ⬆️
superset/views/base.py 67.56% <50%> (ø) ⬆️
superset/security.py 72.9% <75%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9831730...6d4a18a. Read the comment docs.

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]
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Done.

@john-bodley
Copy link
Member

Don't you still want to return the full table name in rejected_datasources as the table may not contain the schema when parsing the SQL? Also it's probably worth double checking the function naming is consistent. There's references to datasource, table, etc.

@timifasubaa
Copy link
Contributor Author

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.

@timifasubaa timifasubaa merged commit 4bf69a7 into apache:master Aug 1, 2018
timifasubaa added a commit to airbnb/superset-fork that referenced this pull request Aug 2, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants