fix(chart): chart updates are not retained #23627

justinpark · 2023-04-07T22:13:31Z

SUMMARY

When a table chart saved on a dashboard and edited from the dashboard, the table reloads in the edit view and the updated option returns to the previous setting.
This bug is a regression of #20498, which reloads the fetchExploreData but always overrides it with the dashboardContext (which retains the original settings).

This hotfix adds the condition of overwrite saving to retrieve the changes from updated exploreFormData.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

after--retained-hotfix.mov

Before:

before--retained-hotfix.mov

TESTING INSTRUCTIONS

Create a new Table chart
Save and publish the chart to a dashboard
Open the dashboard, and click "Edit" on the chart from the dashboard
Go to "Customize" and uncheck "Cell Bars"
Click save. The Cell Bars option returns to checked after saving.
Exit and go to the "Charts" list in Superset
Open your saved chart from the Charts list directly
Go to "Customize" and uncheck "Cell Bars"
Click save. The Cell Bars option retains the unchecked state

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

cc: @michael-s-molina @ktmud cc: @john-bodley

codecov · 2023-04-07T23:08:50Z

Codecov Report

Merging #23627 (278ea1b) into master (0bf8907) will increase coverage by 14.37%.
The diff coverage is 33.33%.

❗ Current head 278ea1b differs from pull request most recent head 93a9b4c. Consider uploading reports for the commit 93a9b4c to get more accurate results

@@             Coverage Diff             @@
##           master   #23627       +/-   ##
===========================================
+ Coverage   53.58%   67.96%   +14.37%     
===========================================
  Files        1938     1925       -13     
  Lines       74957    74390      -567     
  Branches     8140     8109       -31     
===========================================
+ Hits        40167    50556    +10389     
+ Misses      32706    21762    -10944     
+ Partials     2084     2072       -12

Flag	Coverage Δ
javascript	`54.26% <33.33%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...rset-frontend/src/explore/components/SaveModal.tsx	`34.78% <0.00%> (-0.31%)`	⬇️
superset-frontend/src/pages/Chart/index.tsx	`74.00% <100.00%> (+74.00%)`	⬆️

... and 381 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

michael-s-molina · 2023-04-11T14:43:24Z

superset-frontend/src/pages/Chart/Chart.test.tsx

+      expect(fetchMock.calls(exploreApiRoute).length).toBe(1),
+    );
+    expect(getByTestId('mock-explore-view-container')).toBeInTheDocument();
+    expect(store.getActions()).toContainEqual({


@justinpark React Testing Library has a different view on how to write tests. One of its main concerns is accessibility. It differs from Enzyme in the sense that it's more focused on how the user interacts with the app rather then implementation details. Check this quote from their site:

You want to write maintainable tests that give you high confidence that your components are working for your users. As a part of this goal, you want your tests to avoid including implementation details so refactors of your components (changes to implementation but not functionality) don't break your tests and slow you and your team down.

In your case, you could write tests that check if the "Show cell bars" is checked rather then test if an specific Redux action is fired. This will make your tests more durable in a scenario where the implementation details change, such as the redux action name, or the action structure, but the interface, and the way users interact with it, remain the same.

@michael-s-molina I updated the test code as you suggested. (but using partial rendering since full ChartContainer requires a lot of dependancies)

michael-s-molina · 2023-04-11T14:44:48Z

superset-frontend/spec/fixtures/mockExploreFormData.ts

+
+import { JsonObject } from '@superset-ui/core';
+
+export const getExploreFormData = (overrides: JsonObject = {}) => ({


Really nice improvement to extract the mock.

michael-s-molina · 2023-04-11T14:45:36Z

superset-frontend/spec/fixtures/mockExploreFormData.ts

+  ...overrides,
+});
+
+export const getDashboardFormData = (overrides: JsonObject = {}) => ({


Should this be in its own file? Could this be used by another test that is not inside Explore?

Good point. I will change this mock function location

@michael-s-molina could you review the PR again?

michael-s-molina

LGTM

…action payload

justinpark · 2023-04-27T21:57:41Z

@rusackas could you help to stamp this PR to release the blocker?

rusackas

Approving cypress mods as code owner

(cherry picked from commit f5b1711)

pull-request-size bot added the size/XL label Apr 7, 2023

justinpark force-pushed the fix--chart-settings-not-retained branch from 3db7ce8 to 334c84e Compare April 10, 2023 17:11

justinpark requested review from jinghua-qa, geido, eschutho, rusackas and betodealmeida as code owners April 10, 2023 17:54

justinpark force-pushed the fix--chart-settings-not-retained branch 2 times, most recently from c1d6ede to 66537cb Compare April 10, 2023 21:08

justinpark requested a review from michael-s-molina April 10, 2023 22:03

michael-s-molina reviewed Apr 11, 2023

View reviewed changes

justinpark force-pushed the fix--chart-settings-not-retained branch from 66537cb to 2c12c6c Compare April 11, 2023 21:59

justinpark force-pushed the fix--chart-settings-not-retained branch from 2c12c6c to e1f439d Compare April 21, 2023 19:12

michael-s-molina approved these changes Apr 26, 2023

View reviewed changes

justinpark added 4 commits April 27, 2023 11:10

fix(chart): chart updates are not retained

6268c64

update getExplore route to support overwrite with form_data_key

a11f31f

move getDashboardFormData to new path

80b73f6

replace test code to verify by component rendering rather than redux …

93a9b4c

…action payload

justinpark force-pushed the fix--chart-settings-not-retained branch from e1f439d to 93a9b4c Compare April 27, 2023 18:11

john-bodley approved these changes Apr 27, 2023

View reviewed changes

justinpark requested a review from john-bodley April 27, 2023 21:56

rusackas approved these changes Apr 27, 2023

View reviewed changes

john-bodley merged commit f5b1711 into apache:master Apr 27, 2023

john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Apr 27, 2023

fix(chart): chart updates are not retained (apache#23627)

b1cac84

(cherry picked from commit f5b1711)

sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023

fix(chart): chart updates are not retained (apache#23627)

b9ec104

john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request May 10, 2023

fix(chart): chart updates are not retained (apache#23627)

f056b25

(cherry picked from commit f5b1711)

hughhhh mentioned this pull request Aug 29, 2023

fix: permalink save/overwrites in explore #25112

Merged

9 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(chart): chart updates are not retained #23627

fix(chart): chart updates are not retained #23627

justinpark commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

michael-s-molina Apr 11, 2023

justinpark Apr 21, 2023

michael-s-molina Apr 11, 2023

michael-s-molina Apr 11, 2023

justinpark Apr 11, 2023

justinpark Apr 14, 2023

michael-s-molina left a comment

justinpark commented Apr 27, 2023

rusackas left a comment


		import { JsonObject } from '@superset-ui/core';

		export const getExploreFormData = (overrides: JsonObject = {}) => ({

fix(chart): chart updates are not retained #23627

fix(chart): chart updates are not retained #23627

Conversation

justinpark commented Apr 7, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Apr 7, 2023 • edited Loading

Codecov Report

michael-s-molina Apr 11, 2023

Choose a reason for hiding this comment

justinpark Apr 21, 2023

Choose a reason for hiding this comment

michael-s-molina Apr 11, 2023

Choose a reason for hiding this comment

michael-s-molina Apr 11, 2023

Choose a reason for hiding this comment

justinpark Apr 11, 2023

Choose a reason for hiding this comment

justinpark Apr 14, 2023

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

justinpark commented Apr 27, 2023

rusackas left a comment

Choose a reason for hiding this comment

justinpark commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading