chore: Restructure explore redux state by kgabryje · Pull Request #20448 · apache/superset

kgabryje · 2022-06-21T07:56:04Z

SUMMARY

This PR restructures Explore redux state, so that it's more consistent with redux state of Dashboard view. This is a prerequisite for upcoming Explore SPA project - we need the redux states to be consistent so that we can smoothly navigate between dashboards and charts.

Now the following structure is used:

{
  user: User;
  common: {
    flash_messages: string[];
    conf: JsonObject;
  };
  charts: { [key: number]: ChartState };
  datasources: { [key: string]: Dataset };
  explore: {
    can_add: boolean;
    can_download: boolean;
    can_overwrite: boolean;
    isDatasourceMetaLoading: boolean;
    isStarred: boolean;
    triggerRender: boolean;
    // duplicate datasource in exploreState - it's needed by getControlsState
    datasource: Dataset,
    controls: ControlStateMapping;
    form_data: JsonObject;
    slice: Slice;
    controlsTransferred: string[],
    standalone: boolean;
    force: boolean;
  }
}

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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

kgabryje · 2022-06-21T15:06:00Z

CC @eric-briscoe since you were interested in this project 🙂

codecov · 2022-06-21T17:59:37Z

Codecov Report

Merging #20448 (76e0b46) into master (44f0b51) will increase coverage by 0.01%.
The diff coverage is 64.44%.

@@            Coverage Diff             @@
##           master   #20448      +/-   ##
==========================================
+ Coverage   66.75%   66.76%   +0.01%     
==========================================
  Files        1740     1744       +4     
  Lines       65172    65172              
  Branches     6900     6901       +1     
==========================================
+ Hits        43505    43514       +9     
+ Misses      19918    19908      -10     
- Partials     1749     1750       +1

Flag	Coverage Δ
javascript	`51.83% <64.44%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...perset-frontend/src/addSlice/AddSliceContainer.tsx	`65.85% <ø> (ø)`
superset-frontend/src/dashboard/actions/hydrate.js	`2.04% <ø> (ø)`
...frontend/src/dashboard/components/Header/index.jsx	`59.39% <ø> (ø)`
...rontend/src/dashboard/containers/DashboardPage.tsx	`5.31% <ø> (ø)`
...set-frontend/src/dashboard/util/permissionUtils.ts	`90.00% <ø> (ø)`
...rset-frontend/src/explore/components/SaveModal.tsx	`66.19% <ø> (ø)`
...re/components/controls/DatasourceControl/index.jsx	`74.73% <0.00%> (ø)`
...et-frontend/src/explore/reducers/exploreReducer.js	`40.67% <0.00%> (+5.89%)`	⬆️
...t-frontend/src/explore/reducers/getInitialState.ts	`0.00% <0.00%> (ø)`
... and 19 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 44f0b51...76e0b46. Read the comment docs.

michael-s-molina

Thanks for the PR @kgabryje! I left some first-pass comments.

kgabryje · 2022-06-22T08:25:53Z

Thanks for comments Michael, they all make sense to me. Will fix

zhaoyongjie · 2022-06-22T12:39:12Z

no blocking suggestion, define a constant for the unsaved chart, default value is 0, and this function might be splited into getChartKeyFromSlice and getChartKeyFromFromData.

+1 for creating a constant for default slice id, but is there a use case for splitting this function in 2?

eschutho · 2022-06-22T18:02:31Z

This type will probably change in the future to include other datasource types.

Should I change it now? Or just add a todo when those new datasource types are introduced?

Either way would work. Using the intersection type may cause some typescript errors because you'll need to resolve the difference in the schema of the datasources, which is something that we're working on now in a feature branch. It may be easiest to leave it as a todo, honestly.

Added a todo

jinghua-qa · 2022-06-22T21:54:22Z

/testenv up

github-actions · 2022-06-22T21:56:06Z

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

stephenLYZ · 2022-06-23T10:25:56Z

Can we confirm the exact type of getState return here?

Good point... which resulted in finding a bug. I thought that getState returns the state of reducer, but it actually returns global redux state.
Added a unit test for that

jinghua-qa · 2022-06-23T13:49:25Z

Explore regression is tested in ephemeral env , did not find major issue related to this PR, LGTM

kgabryje · 2022-06-24T10:58:44Z

/testenv up

github-actions · 2022-06-24T11:03:05Z

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

geido · 2022-06-24T11:15:42Z

-    datasource_type: explore.datasource.type,
-    datasourceId: explore.datasource_id,
+    datasource,
+    datasource_type: datasource.type,


I think this might be a good time to make also the property names consistent, some of them are camel case, some others use hyphens currently

Good call. I'll fix it in a follow up PR

kgabryje · 2022-06-24T11:37:41Z

/testenv up

github-actions · 2022-06-24T11:42:14Z

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

michael-s-molina

LGTM

stephenLYZ

Nice Work!

github-actions · 2022-06-24T13:26:36Z

Ephemeral environment shutdown and build artifacts deleted.

* chore: Restructure explore redux state * fixes * fix tests * add new tests * Fix type * Address comments * Fix bug * Fix import * Add new test * Move unsaved chart id to a constant * Add todo

pull-request-size Bot added the size/L label Jun 21, 2022

superset-github-bot Bot added the preset-io label Jun 21, 2022

pull-request-size Bot added size/XL and removed size/L labels Jun 21, 2022

kgabryje requested review from geido, michael-s-molina and rusackas June 21, 2022 15:04

michael-s-molina requested a review from zhaoyongjie June 21, 2022 18:14

michael-s-molina reviewed Jun 21, 2022

View reviewed changes

zhaoyongjie reviewed Jun 22, 2022

View reviewed changes

eschutho reviewed Jun 22, 2022

View reviewed changes

Comment thread superset-frontend/src/explore/reducers/datasourcesReducer.ts Outdated

eschutho reviewed Jun 22, 2022

View reviewed changes

stephenLYZ reviewed Jun 23, 2022

View reviewed changes

kgabryje force-pushed the chore/explore-redux branch from 32a2b4b to e5a3228 Compare June 23, 2022 12:10

kgabryje added 10 commits June 24, 2022 12:30

chore: Restructure explore redux state

2aba68a

fixes

fde3787

fix tests

17a60ed

add new tests

a7b1dac

Fix type

b065a5d

Address comments

14f7804

Fix bug

824236f

Fix import

4171dcd

Add new test

adbe5a5

Move unsaved chart id to a constant

eade95e

kgabryje force-pushed the chore/explore-redux branch from b5c5591 to eade95e Compare June 24, 2022 10:34

Add todo

76e0b46

geido reviewed Jun 24, 2022

View reviewed changes

michael-s-molina approved these changes Jun 24, 2022

View reviewed changes

zhaoyongjie approved these changes Jun 24, 2022

View reviewed changes

stephenLYZ approved these changes Jun 24, 2022

View reviewed changes

kgabryje merged commit c7f1c7d into apache:master Jun 24, 2022

zhaorui2022 mentioned this pull request Mar 8, 2023

Can't change chart data source if the new source has same schema as the old one #23289

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 First shipped in 2.1.0 labels Mar 13, 2024

Uh oh!

Conversation

kgabryje commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

kgabryje commented Jun 21, 2022

Uh oh!

codecov Bot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michael-s-molina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kgabryje commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinghua-qa commented Jun 22, 2022

Uh oh!

github-actions Bot commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgabryje Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinghua-qa commented Jun 23, 2022

Uh oh!

kgabryje commented Jun 24, 2022

Uh oh!

github-actions Bot commented Jun 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgabryje commented Jun 24, 2022

Uh oh!

github-actions Bot commented Jun 24, 2022

Uh oh!

michael-s-molina left a comment

Choose a reason for hiding this comment

Uh oh!

stephenLYZ left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

kgabryje commented Jun 21, 2022 •

edited

Loading

codecov Bot commented Jun 21, 2022 •

edited

Loading

kgabryje Jun 23, 2022 •

edited

Loading