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

feat(explore): Keep or reset chart config after datasource change #18215

Merged
merged 8 commits into from
Feb 2, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jan 28, 2022

SUMMARY

Currently, when user switches datasource, controls such as metrics, group by, filters etc. are being reset. This PR changes that behaviour - after switching datasource, controls that use columns or saved metrics that are present in both previous and current datasource are preserved. After that, we display an alert that let's user decide if they want to reset the chart config or keep the changes. If there were no controls using columns nor metrics that matched the new dataset, we display a "warning" alert that informs user that chart config was reset.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-01-28.at.13.45.07.mov
Screen.Recording.2022-01-28.at.13.46.17.mov

EDIT: new copy
image
image

TESTING INSTRUCTIONS

  1. Turn on drag and drop feature flag
  2. Open a chart
  3. Add some columns and metrics to controls
  4. Change dataset
  5. If some of the columns or metrics that you used in controls are present in the new dataset, those controls should keep their state and an info alert should appear with 2 button - "Clear form" and "Continue". Clicking "Continue" should close the alert, clicking "Clear form" should reset the controls and close the alert.
  6. If none of the columns or metrics that you used in controls are present in the new dataset, a warning alert should appear with button "Continue", that closes the alert.

ADDITIONAL INFORMATION

  • Has associated issue: Chart config is reset when changing the datasource #17010
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

CC @kasiazjc

https://app.shortcut.com/preset/story/34087/gh-17010-prod-95-keep-chart-config-when-changing-data-source

@rumbin
Copy link
Contributor

rumbin commented Jan 30, 2022

Would this logic also work case-insensitive?

We have use cases where we migrate tables from Postgres to Snowflake, which potentially results in a change in case from lowercase to UPPERCASE. However, the model's general schema is still the same.
So it would be really helpful if the case would not have to match in order to keep the chart config after changing the datasource.

@kgabryje
Copy link
Member Author

Would this logic also work case-insensitive?

We have use cases where we migrate tables from Postgres to Snowflake, which potentially results in a change in case from lowercase to UPPERCASE. However, the model's general schema is still the same. So it would be really helpful if the case would not have to match in order to keep the chart config after changing the datasource.

Hey @rumbin, I discussed your suggestion with @villebro and we agree that it makes sense to have that logic case insensitive. However, that change should be made on a higher level, in the whole Superset. I'd suggest that we merge this PR as-is and work on case insensitivity as a separate project.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #18215 (8349ccd) into master (c40b337) will decrease coverage by 0.02%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18215      +/-   ##
==========================================
- Coverage   66.30%   66.28%   -0.03%     
==========================================
  Files        1592     1594       +2     
  Lines       62437    62491      +54     
  Branches     6292     6311      +19     
==========================================
+ Hits        41401    41420      +19     
- Misses      19383    19421      +38     
+ Partials     1653     1650       -3     
Flag Coverage Δ
javascript 51.34% <42.85%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../explore/components/ExploreViewContainer/index.jsx 57.22% <ø> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js 33.33% <0.00%> (-1.97%) ⬇️
...t-frontend/src/explore/reducers/getInitialState.ts 0.00% <ø> (ø)
...lUtils/getControlValuesCompatibleWithDatasource.ts 8.00% <8.00%> (ø)
...ntend/src/explore/components/ControlPanelAlert.tsx 22.22% <22.22%> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 77.65% <75.00%> (+1.52%) ⬆️
...-frontend/src/explore/components/ControlHeader.jsx
...-frontend/src/explore/components/ControlHeader.tsx 82.60% <0.00%> (ø)
.../src/explore/components/controls/SelectControl.jsx 62.06% <0.00%> (+1.72%) ⬆️
...plore/components/controls/VizTypeControl/index.tsx 80.00% <0.00%> (+3.33%) ⬆️
... and 4 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 c40b337...8349ccd. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I love this, best of both worlds! One minor bug that causes Explore not to load when entering via the dataset list.

import { styled } from '@superset-ui/core';
import Button from 'src/components/Button';

interface ControlPanelAlertI {
Copy link
Member

Choose a reason for hiding this comment

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

The I here gave me a horrible C# flashback 👻 https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/interface (granted, it's trailing, not leading). I did a quick search of the codebase, and it seems like we usually suffix these with Interface when there would otherwise be overlap with a component name. At the risk of being awarded the nitpicker of the year award, maybe we could change this to ControlPanelAlertInterface to keep with conventions..? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right! And I think that we use a Props suffix more often, so I'm going to change the name to ControlPanelAlertProps

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

renderControl({ name, config }: CustomControlItem) {
const { actions, controls, chart, exploreState } = this.props;
const renderControl = ({ name, config }: CustomControlItem) => {
const { controls, chart } = props;
Copy link
Member

Choose a reason for hiding this comment

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

It seems we're missing exploreState here (needed down on line 257):

Suggested change
const { controls, chart } = props;
const { controls, chart, exploreState } = props;

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 I moved exploreState into ControlPanelsContainer, but then I changed my mind and moved it back to props. For some reason, linter didn't mark this line with "variable not found" error. Thanks for finding that!

@@ -260,7 +260,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
// re-render, we only run this when the chart plugin explicitly ask for this.
...(config.mapStateToProps?.length === 3
? // @ts-ignore /* The typing accuses of having an extra parameter. I didn't remove it because I believe it could be an error in the types and not in the code */
config.mapStateToProps(exploreState, controls[name], chart)
config.mapStateToProps(props.exploreState, controls[name], chart)
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we move this to line 249?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM!

@kgabryje
Copy link
Member Author

/testenv up FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true

@github-actions
Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://54.201.119.118:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa jinghua-qa added the need:qa-review Requires QA review label Jan 31, 2022
@jinghua-qa
Copy link
Member

I have found 3 issues in the PR when i testing in the ephemeral env
1, On the dataset name filed, the dataset name showed without the schema (master has .<dataset_name>, is that expected?
Screen Shot 2022-01-31 at 11 21 57 AM
2, seems like when the warning msg is shown, it is not auto pop to the msg and if user is at the bottom of the panel, user may not able to see the warning msg until user scroll up

not.auto.scroll.to.warning.msg.mov

3, After the i change dataset, if i also change the viz type, the explore page will become blank

Screen.Recording.2022-01-31.at.11.16.26.AM.mov

@kgabryje
Copy link
Member Author

kgabryje commented Feb 1, 2022

Thanks so much for spotting those @jinghua-qa!

  1. No changes here - it might be related to sqlite used in eph environments.
  2. Good idea, I'll add scrolling to the top when datasource changes.
  3. 😱 Will fix!

@kgabryje
Copy link
Member Author

kgabryje commented Feb 1, 2022

@jinghua-qa Both issues should be fixed now! Can you retest please?

@jinghua-qa
Copy link
Member

/testenv up FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2022

@jinghua-qa Ephemeral environment spinning up at http://34.212.59.37:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@jinghua-qa jinghua-qa left a comment

Choose a reason for hiding this comment

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

LGTM

@kgabryje kgabryje merged commit 7096982 into apache:master Feb 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2022

Ephemeral environment shutdown and build artifacts deleted.

shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
…ache#18215)

* feat(explore): Keep or reset chart config after datasource change

* Update copy

* Remove useDispatch

* fix test

* Fix bugs

* Remove ts ignore

* Scroll top when datasource changes

* Fix crashing when switching viz type
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
…ache#18215)

* feat(explore): Keep or reset chart config after datasource change

* Update copy

* Remove useDispatch

* fix test

* Fix bugs

* Remove ts ignore

* Scroll top when datasource changes

* Fix crashing when switching viz type
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
…ache#18215)

* feat(explore): Keep or reset chart config after datasource change

* Update copy

* Remove useDispatch

* fix test

* Fix bugs

* Remove ts ignore

* Scroll top when datasource changes

* Fix crashing when switching viz type
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 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 lts-v1 need:qa-review Requires QA review size/XL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants