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

Allow overwriting a SQLLab query that has previously been saved #8298

Merged
merged 4 commits into from
Oct 1, 2019

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented Sep 26, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

When saving a query in SQLLab, currently there is no good way to update a query you were previously working on. You have to save a new one, then go to the saved queries list and delete the old one. This change lets you choose to overwrite the previous entry or save a new copy.

A speed bump I ran into was that the frontend is using several field names that are different from the names the backend uses. My first instinct was to change that so that a field means the same thing everywhere, but that gets pretty complicated pretty fast, due to the way the redux state is being persisted locally - how does the Superset frontend do migrations? #7748 will probably allow solving this problem.

So instead I grouped the existing differences (plus my addition of a remoteId field) into a queryClientMapping object, with the goal of isolating the differences to one layer of the system - where the client actually makes API calls. That way UI components like SaveQuery don't have to concern themselves with those differences. This hasn't been propagated to all parts of the code that use saved queries, yet.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Screen Shot 2019-09-25 at 7 04 01 PM
After:
Screen Shot 2019-09-25 at 7 03 30 PM

TEST PLAN

I've already tested it pretty well, I think. This is my first contribution to Superset so it's entirely possible that I missed something. It would be nice to get some additional verification that nothing I've done will mess up anyone's local state.

ADDITIONAL INFORMATION

REVIEWERS

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Minor comments and some red on the builds, otherwise LGTM

@suddjian
Copy link
Member Author

There's one cypress test that failed in that last build, but I can't reproduce it on my machine. It's probably flaky. If someone with the necessary auth could re-run that job it'd be much appreciated.

@villebro
Copy link
Member

I restarted the build, I believe this is due to flakiness.

@codecov-io
Copy link

codecov-io commented Sep 26, 2019

Codecov Report

Merging #8298 into master will decrease coverage by 5.77%.
The diff coverage is 63.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8298      +/-   ##
==========================================
- Coverage   73.53%   67.76%   -5.78%     
==========================================
  Files         115      451     +336     
  Lines       12396    22730   +10334     
  Branches        0     2377    +2377     
==========================================
+ Hits         9116    15402    +6286     
- Misses       3280     7190    +3910     
- Partials        0      138     +138
Impacted Files Coverage Δ
...src/SqlLab/utils/reduxStateToLocalStorageHelper.js 100% <ø> (ø)
superset/views/core.py 70.06% <ø> (ø) ⬆️
superset/assets/src/SqlLab/reducers/sqlLab.js 43.68% <0%> (ø)
superset/views/sql_lab.py 86.11% <100%> (ø) ⬆️
...uperset/assets/src/SqlLab/components/SqlEditor.jsx 52.81% <100%> (ø)
superset/assets/src/SqlLab/actions/sqlLab.js 60.76% <46.66%> (ø)
...uperset/assets/src/SqlLab/components/SaveQuery.jsx 77.27% <93.75%> (ø)
...sets/src/explore/components/ExploreChartHeader.jsx 50% <0%> (ø)
superset/assets/src/components/Checkbox.jsx 100% <0%> (ø)
...perset/assets/src/dashboard/util/getEmptyLayout.js 0% <0%> (ø)
... and 333 more

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 0a8f3eb...ee32e70. Read the comment docs.

@graceguo-supercat graceguo-supercat merged commit fbbc5f0 into apache:master Oct 1, 2019
@suddjian suddjian deleted the query-overwrite branch October 1, 2019 16:35
@graceguo-supercat
Copy link

@suddjian this PR broke existed save query feature. when i try to save a query from sql lab, i got server-side error message:

Screen Shot 2019-10-03 at 9 33 08 PM

@suddjian
Copy link
Member Author

suddjian commented Oct 4, 2019

Thanks for the report @graceguo-supercat I could have sworn I saw this working! I am able to repro it though, so hopefully I'll have a fix up today.

etr2460 pushed a commit that referenced this pull request Oct 4, 2019
etr2460 pushed a commit to airbnb/superset-fork that referenced this pull request Oct 4, 2019
@suddjian suddjian mentioned this pull request Oct 4, 2019
12 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 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 size/L 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing a saved query in the sql_lab generate the new one instead of overwrite it!
6 participants