fix(generic-chart-axes): set x-axis if unset and ff is enabled #20107

villebro · 2022-05-18T09:48:33Z

SUMMARY

After enabling the GENERIC_CHART_AXES feature flag, timeseries charts that were created before enabling the flag won't render in Explore due to the required x-axis control being unset. This PR checks if the feature flag is enabled, and sets the default value of the x-axis control to the value of the granularity_sqla flag if the value of the x-axis control is unset.

Due to mixing of JS and TS and inconsistent function calls (e.g. calling a function with null when it was expecting ControlPanelState, all while having logic for when the value was in fact null), some of this code was slightly difficult to follow. I tried to refactor it to be more intuitive, but even minor refactoring seemed to cause weird regressions, so I decided to postpone the refactor to a follow-up PR.

AFTER

Now the timeseries chart that was saved before the feature flag was enabled opens up correctly:

default-after.mp4

BEFORE

Previously, the chart would not render, as the required x-axis control was unset in the chart form data:

default-before.mp4

TESTING INSTRUCTIONS

Create a Chart using the Time - Series Bar Chart V2.
Add it to a Dashboard.
Enable the GENERIC_CHART_AXES FF.
Access the Dashboard - note that the Chart is still working.
Click on "Edit Chart" and notice how the chart no longer renders in Explore

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

villebro · 2022-05-18T09:50:37Z

superset-frontend/src/explore/store.js

@@ -64,8 +64,14 @@ export function getControlsState(state, inputFormData) {
 export function applyDefaultFormData(inputFormData) {
  const datasourceType = inputFormData.datasource.split('__')[1];
  const vizType = inputFormData.viz_type;
-  const controlsState = getAllControlsState(vizType, datasourceType, null, {


This would previously have caused a type error if this file were TypeScript: the signature of getAllControlsState requires the third parameter to be ControlPanelState (here null).

villebro · 2022-05-18T09:52:12Z

superset-frontend/src/explore/store.js

-    ...inputFormData,
+  let controlsState = getAllControlsState(vizType, datasourceType, {
+    form_data: inputFormData,
+  });


Here I'm removing the shallow copy of inputFormData and placing it in the Partial<ControlPanelState> object, as I didn't see any downstream code mutating the object.

villebro · 2022-05-18T10:45:50Z

superset-frontend/src/explore/controlUtils/getControlState.ts

@@ -90,7 +85,7 @@ export function applyMapStateToPropsToControl<T = ControlType>(
  const { mapStateToProps } = controlState;
  let state = { ...controlState };
  let { value } = state; // value is current user-input value
-  if (mapStateToProps && controlPanelState) {


we seem to be checking for truthiness of controlPanelState here and in a few other places, despite the signature stating it will always be truthy.

codecov · 2022-05-18T18:27:59Z

Codecov Report

Merging #20107 (c9bb5b4) into master (660af40) will decrease coverage by 0.00%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master   #20107      +/-   ##
==========================================
- Coverage   66.47%   66.46%   -0.01%     
==========================================
  Files        1721     1721              
  Lines       64477    64483       +6     
  Branches     6795     6796       +1     
==========================================
- Hits        42858    42857       -1     
- Misses      19891    19898       +7     
  Partials     1728     1728

Flag	Coverage Δ
javascript	`51.34% <20.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...egacy-plugin-chart-event-flow/src/controlPanel.tsx	`14.28% <0.00%> (ø)`
...tend/plugins/plugin-chart-echarts/src/controls.tsx	`56.66% <0.00%> (-17.25%)`	⬇️
...ontend/src/explore/controlUtils/getControlState.ts	`85.18% <100.00%> (-0.27%)`	⬇️
superset-frontend/src/explore/store.js	`58.33% <100.00%> (ø)`

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 660af40...c9bb5b4. Read the comment docs.

kgabryje · 2022-05-19T10:12:17Z

superset-frontend/plugins/legacy-plugin-chart-event-flow/src/controlPanel.tsx

-                  ? control.choices[0][0]
-                  : null,
+              default: (control: ControlState) =>
+                control.choices?.[0]?.[0] || null,


How about ensureIsArray(control.choices)[0][0]? I'm actually not sure which one is easier to read, up to you 🙂

Wouldn't it need to be ensureIsArray(control.choices)[0]?.[0] ? If so, I think the current one is probably ok (mind you, I agree it's pretty ugly 😄 )

Yep! Let's keep it as it is 👍

kgabryje

LGTM!

…e#20107) * fix(generic-chart-axes): set x-axis if unset and ff is enabled * simplify * simplify * continue cleanup * yet more cleanup

superset-github-bot bot added the preset-io label May 18, 2022

pull-request-size bot added the size/M label May 18, 2022

villebro commented May 18, 2022

View reviewed changes

villebro requested a review from kgabryje May 18, 2022 09:53

villebro force-pushed the villebro/default-x-axis branch 2 times, most recently from 9f3f14b to 011190e Compare May 18, 2022 10:42

villebro commented May 18, 2022

View reviewed changes

villebro force-pushed the villebro/default-x-axis branch 3 times, most recently from ece8446 to 1594576 Compare May 18, 2022 16:27

villebro added 4 commits May 19, 2022 09:28

fix(generic-chart-axes): set x-axis if unset and ff is enabled

f79d4ae

simplify

7587101

simplify

465b869

continue cleanup

93bc1ee

villebro force-pushed the villebro/default-x-axis branch from 1594576 to 769f9d0 Compare May 19, 2022 06:28

yet more cleanup

c9bb5b4

villebro force-pushed the villebro/default-x-axis branch from 769f9d0 to c9bb5b4 Compare May 19, 2022 06:50

kgabryje reviewed May 19, 2022

View reviewed changes

kgabryje approved these changes May 19, 2022

View reviewed changes

villebro merged commit 0b3d3dd into apache:master May 19, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(generic-chart-axes): set x-axis if unset and ff is enabled #20107

fix(generic-chart-axes): set x-axis if unset and ff is enabled #20107

villebro commented May 18, 2022 •

edited

Loading

villebro May 18, 2022 •

edited

Loading

villebro May 18, 2022

villebro May 18, 2022

codecov bot commented May 18, 2022 •

edited

Loading

kgabryje May 19, 2022

villebro May 19, 2022

kgabryje May 19, 2022

kgabryje left a comment

fix(generic-chart-axes): set x-axis if unset and ff is enabled #20107

fix(generic-chart-axes): set x-axis if unset and ff is enabled #20107

Conversation

villebro commented May 18, 2022 • edited Loading

SUMMARY

AFTER

BEFORE

TESTING INSTRUCTIONS

villebro May 18, 2022 • edited Loading

Choose a reason for hiding this comment

villebro May 18, 2022

Choose a reason for hiding this comment

villebro May 18, 2022

Choose a reason for hiding this comment

codecov bot commented May 18, 2022 • edited Loading

Codecov Report

kgabryje May 19, 2022

Choose a reason for hiding this comment

villebro May 19, 2022

Choose a reason for hiding this comment

kgabryje May 19, 2022

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

villebro commented May 18, 2022 •

edited

Loading

villebro May 18, 2022 •

edited

Loading

codecov bot commented May 18, 2022 •

edited

Loading