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: Visualization settings were lost when editing a datasource from Explore #10092

Merged
merged 6 commits into from Jun 19, 2020

Conversation

willbarrett
Copy link
Member

@willbarrett willbarrett commented Jun 17, 2020

SUMMARY

When working on a visualization in Explore, if there were changes to the visualization that had not been "run" via the "Run Query" button, they would be lost when editing the related datasource via the modal on the page.

Why? The reducer that updated the form_data when a form component changed was broken in such a way that it would update the control settings to make the change visible, but not the form_data settings to make the change durable.

This PR retains the behavior of not updating the encoded data in the URL until the "Run Query" button is pressed.

Question for the community: are there any tests for any of the reducers in Superset? Can someone point me to an example? I'd love to write some tests for this change to avoid a regression.

Also, big props to @nytai for walking me through the Explore front-end code and helping me track this one down.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

  • Open the Explore view by creating an arbitrary query in SQL Lab and clicking Explore
  • Enter some chart options
  • Edit the dataset by using the dropdown
  • Click Save in the modal
  • Click Confirm
  • The chart options you had selected before editing the dataset should still be present.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@willbarrett willbarrett changed the title bugfix: Visualization settings were lost when editing a datasource from Explore fix: Visualization settings were lost when editing a datasource from Explore Jun 17, 2020
@nytai
Copy link
Member

nytai commented Jun 17, 2020

@willbarrett there are a few tests for the exploreReducer here

@willbarrett
Copy link
Member Author

Thanks @nytai - I'll add a few more.

@ktmud
Copy link
Member

ktmud commented Jun 18, 2020

I also noticed this yesterday and was surprised. Thanks for the fix!

@willbarrett
Copy link
Member Author

@nytai @ktmud @etr2460 I believe this is ready for review, when you have a moment.

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #10092 into master will decrease coverage by 4.83%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10092      +/-   ##
==========================================
- Coverage   70.63%   65.80%   -4.84%     
==========================================
  Files         592      590       -2     
  Lines       31191    31139      -52     
  Branches     3191     3163      -28     
==========================================
- Hits        22032    20490    -1542     
- Misses       9050    10471    +1421     
- Partials      109      178      +69     
Flag Coverage Δ
#cypress ?
#javascript 59.55% <100.00%> (-0.19%) ⬇️
#python 70.21% <ø> (+0.02%) ⬆️
Impacted Files Coverage Δ
...et-frontend/src/explore/reducers/exploreReducer.js 26.41% <100.00%> (-4.50%) ⬇️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
... and 147 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 be6b9b8...fa0034a. Read the comment docs.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Just a note that this will also keep the settings when changing to a different datasource, which leaves invalid selected columns sometimes (probably doesn't matter though).

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm, assuming the tests failed before making this change. and thanks so much for tracking this bug down!

also, could you add a test plan to the summary prior to merging? Thanks!

@nytai nytai merged commit 961b55c into apache:master Jun 19, 2020
@nytai nytai deleted the wbarrett/fix-explore-redux-state branch June 19, 2020 19:55
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 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/S 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants