fix(explore): fix chart save when dashboard deleted #21497

codyml · 2022-09-16T21:11:54Z

SUMMARY

As part of moving Explore to the SPA, #20498 replaced the previous single POST /superset/explore request that was used to save charts with calls to the v1 API. The PUT /api/v1/chart/{id} endpoint responsible for updating the chart required sending the complete list of IDs of dashboards that the chart should be added to in the request body, and this list was previously sourced from the form_data.dashboards array returned by the GET /api/v1/explore endpoint. However, form_data.dashboards is not updated when dashboards are deleted from Superset. As a result, saving a chart that was added to a dashboard when that dashboard was deleted would result in the frontend submitting that dashboard in the PUT request, which would cause the request to fail. This PR fixes this by getting the chart dashboards with a GET /api/v1/chart/{id} request during the explore save process, the results of which are correctly updated when dashboards are deleted.

Additionally, the Save Chart modal was configured to close immediately upon clicking the "Save chart" button, rather than waiting for requests to complete/fail and showing error messages on fail. This PR also fixes this so errors are displayed correctly. This PR now just fixes the broken saving, as the fix for the Save Chart modal closing before error message shows turned out to trigger other Explore display bugs that couldn't be quickly solved for this PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Saving a chart w/ dashboard deleted before:

Before.mov

Saving a chart w/ dashboard deleted after:

Screen.Recording.2022-09-19.at.9.56.50.PM.mov

TESTING INSTRUCTIONS

Create a chart, save it to a dashboard, delete the dashboard, then make a change to the chart and attempt to save again. The chart should save correctly and opening the chart from the chart list again should show the saved version. Additionally, verify in the inspector that the PUT request was successful, with no 422 error.

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

codecov · 2022-09-16T23:36:35Z

Codecov Report

Merging #21497 (cb1f6bd) into master (f27e20e) will increase coverage by 0.00%.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##           master   #21497   +/-   ##
=======================================
  Coverage   66.66%   66.67%           
=======================================
  Files        1793     1793           
  Lines       68499    68531   +32     
  Branches     7278     7282    +4     
=======================================
+ Hits        45666    45694   +28     
- Misses      20969    20974    +5     
+ Partials     1864     1863    -1

Flag	Coverage Δ
javascript	`52.85% <60.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
...set-frontend/src/explore/actions/hydrateExplore.ts	`42.10% <ø> (ø)`
.../explore/components/ExploreViewContainer/index.jsx	`52.74% <ø> (+0.28%)`	⬆️
...rset-frontend/src/explore/components/SaveModal.tsx	`38.53% <14.28%> (-0.52%)`	⬇️
...t-frontend/src/explore/actions/saveModalActions.js	`98.59% <100.00%> (+0.13%)`	⬆️
...src/dashboard/components/PropertiesModal/index.tsx	`61.07% <0.00%> (-0.75%)`	⬇️
superset-frontend/src/SqlLab/constants.ts	`100.00% <0.00%> (ø)`
...frontend/src/SqlLab/components/SqlEditor/index.jsx	`52.74% <0.00%> (+1.33%)`	⬆️
...tend/plugins/plugin-chart-table/src/TableChart.tsx	`42.07% <0.00%> (+1.61%)`	⬆️
...et-frontend/src/components/TableSelector/index.tsx	`80.00% <0.00%> (+4.00%)`	⬆️

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

zhaoyongjie · 2022-09-19T04:47:42Z

superset-frontend/src/explore/actions/saveModalActions.js

+
+//  Get dashboards the slice is added to
+export const getSliceDashboards = slice => async dispatch => {
+  if (slice) {


the if clause is redundant because the try...cache handled exception

My intention was for the try...catch clause to just handle request errors or other unexpected errors. The if clause is there because this function might be called with no slice param, in the case of a chart that hasn't yet been saved, and I wanted it to return [] in that case as part of normal, non-exception behavior.

if there is no slice param, the request will also raise an exception, so the catch statement alway catches an exception although an invalid input param.

But, I don't want to send the request at all if there's no slice param, I just want to return an empty array.

if you don't want to send a request, you shouldn't call this action. e.g.:

const originalDashboards = this.props.slice ? await this.props.actions.getSliceDashboards( this.props.slice, ) : [];

just a personal suggestion, optimize it for the future. thanks for the fixing.

Ok, that makes sense, thanks.

zhaoyongjie · 2022-09-19T04:49:01Z

superset-frontend/src/explore/actions/saveModalActions.js

+  if (slice) {
+    try {
+      const response = await SupersetClient.get({
+        endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({


nits:

Suggested change

endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({

endpoint: `/api/v1/chart/${slice?.slice_id}?q=${rison.encode({

I check above that slice is defined, so I don't think this check is necessary.

zhaoyongjie · 2022-09-19T04:52:10Z

superset-frontend/src/explore/actions/saveModalActions.js

+    }
+  }
+
+  return [];


return [] should be removed if remove the if clause

I don't want to remove the if clause; returning [] isn't exception behavior (see above).

zhaoyongjie · 2022-09-19T05:31:48Z

superset-frontend/src/explore/components/SaveModal.tsx

+    //  Get chart dashboards
+    let sliceDashboards: number[];
+    promise = promise
+      .then(() => this.props.actions.getSliceDashboards(this.props.slice))
+      .then(dashboards => {
+        sliceDashboards = dashboards;
+      });
+


nits:

Suggested change

// Get chart dashboards

let sliceDashboards: number[];

promise = promise

.then(() => this.props.actions.getSliceDashboards(this.props.slice))

.then(dashboards => {

sliceDashboards = dashboards;

});

const originalDashboards = await this.props.actions.getSliceDashboards(

this.props.slice,

);

Oops, I didn't realize this saveOrOverwrite was now an async function! I can clean up a lot of these promises now, thanks.

I'm actually going to leave out turning the promises into awaits because my full PR that included that fix triggered an Explore refresh bug that I think we're going to work on separately.

zhaoyongjie

Thanks for the fixing, left a few comments.

zhaoyongjie · 2022-09-19T05:43:14Z

superset-frontend/src/explore/components/SaveModal.tsx

@@ -200,12 +207,14 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
        .then(() => this.props.actions.getDashboard(saveToDashboardId))
        .then((response: { result: DashboardGetResponse }) => {
          dashboard = response.result;
-          const dashboards = new Set<number>(this.props.sliceDashboards);


nits:

sliceDashboards = originalDashboards.includes(dashboard.id) ? originalDashboards : [...originalDashboards, dashboard.id],

Much cleaner, thanks.

geido · 2022-09-19T11:45:31Z

superset-frontend/src/explore/components/SaveModal.tsx

-                />
-              </>
-            }
+            message={this.state.alert ? this.state.alert : this.props.alert}


Suggested change

message={this.state.alert ? this.state.alert : this.props.alert}

message={this.state.alert || this.props.alert}

superset-frontend/src/explore/components/SaveModal.tsx

rusackas · 2022-09-19T16:25:33Z

/testenv up

github-actions · 2022-09-19T16:27:21Z

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

rusackas · 2022-09-19T17:33:36Z

There is currently a bug seen on this PR - if you create a new chart from scratch, and then save it without going to a dashboard, the chart vanishes from view, rather than displaying the chart.

rusackas · 2022-09-19T22:44:42Z

/testenv up

github-actions · 2022-09-19T22:48:49Z

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

jinghua-qa · 2022-09-20T02:31:13Z

/testenv up

github-actions · 2022-09-20T02:36:20Z

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

jinghua-qa · 2022-09-20T04:32:51Z

tested in ephemeral env, found 1 issues
1, chart can not save to dashboard
steps:
1, create a chart
2, save and add to a new dashboard
3, go to dashboard

expect:
chart will be save to new dashboard

actual:
chart did not save to new dashboard

can.not.save.chart.to.dashboard.mov

michael-s-molina · 2022-09-20T11:47:26Z

/testenv up

github-actions · 2022-09-20T11:52:12Z

@michael-s-molina Ephemeral environment spinning up at http://35.91.186.119:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

codyml · 2022-09-20T12:10:05Z

tested in ephemeral env, found 1 issues 1, chart can not save to dashboard steps: 1, create a chart 2, save and add to a new dashboard 3, go to dashboard

expect: chart will be save to new dashboard

actual: chart did not save to new dashboard

can.not.save.chart.to.dashboard.mov

@jinghua-qa This was the issue that my latest update was hoping to fix, and I could reproduce it in the previous ephemeral environment you created but it looked to be fixed in the latest one @michael-s-molina just spun up. Could you check again in the latest ephemeral env? Maybe the previous one was using an outdated image?

michael-s-molina · 2022-09-20T12:10:29Z

tested in ephemeral env, found 1 issues 1, chart can not save to dashboard steps: 1, create a chart 2, save and add to a new dashboard 3, go to dashboard

expect: chart will be save to new dashboard

actual: chart did not save to new dashboard

@jinghua-qa @codyml

I first tried to reproduce this locally but I wasn't able to reproduce the problem.
Then I tested on the ephemeral environment that was running and I was able to reproduce it. I also noticed that the time the environment was run coincided with a commit from Cody so I wasn't sure if the environment was updated.
I spun a new ephemeral environment and I wasn't able to reproduce the problem anymore.

Screen.Recording.2022-09-20.at.9.03.01.AM.mov

michael-s-molina · 2022-09-20T12:13:05Z

I also tested:

Saving a chart that referenced a deleted dashboard
Save a new chart and check that the chart does not disappear

michael-s-molina

LGTM. Given the priority, I vote for any non-blocking improvements to be made in a follow-up.

michael-s-molina · 2022-09-20T12:16:40Z

@jinghua-qa Can you give a last round of testing before we merge it?

jinghua-qa

LGTM

jinghua-qa · 2022-09-20T19:13:49Z

/testenv up

github-actions · 2022-09-20T19:18:07Z

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

jinghua-qa · 2022-09-20T19:44:00Z

LGTM

github-actions · 2022-09-20T19:49:50Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit 6644a84)

…1497)" This reverts commit 6644a84.

pull-request-size bot added the size/M label Sep 16, 2022

codyml changed the title ~~Request slice dashboards instead of using formData's.~~ fix(explore): Request slice dashboards instead of using form data Sep 16, 2022

codyml changed the title ~~fix(explore): Request slice dashboards instead of using form data~~ fix(explore): Fix chart save when previous dashboard deleted Sep 16, 2022

codyml changed the title ~~fix(explore): Fix chart save when previous dashboard deleted~~ fix(explore): Fix chart save when dashboard deleted Sep 16, 2022

codyml changed the title ~~fix(explore): Fix chart save when dashboard deleted~~ fix(explore): fix chart save when dashboard deleted Sep 16, 2022

rusackas requested review from villebro, michael-s-molina, geido, kgabryje, zhaoyongjie and stephenLYZ September 18, 2022 05:00

zhaoyongjie reviewed Sep 19, 2022

View reviewed changes

geido reviewed Sep 19, 2022

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Sep 19, 2022

codyml force-pushed the fix/explore-save-deleted-dashboard branch from a7a2c90 to 2f39ba4 Compare September 19, 2022 21:53

pull-request-size bot added size/M and removed size/L labels Sep 19, 2022

codyml force-pushed the fix/explore-save-deleted-dashboard branch from 2f39ba4 to 6003d73 Compare September 20, 2022 02:00

pull-request-size bot removed the size/M label Sep 20, 2022

codyml force-pushed the fix/explore-save-deleted-dashboard branch from 6003d73 to 4686dce Compare September 20, 2022 02:19

michael-s-molina approved these changes Sep 20, 2022

View reviewed changes

jinghua-qa approved these changes Sep 20, 2022

View reviewed changes

Fix adding charts to extra dashboards on Save As.

cb1f6bd

rusackas approved these changes Sep 20, 2022

View reviewed changes

rusackas merged commit 6644a84 into apache:master Sep 20, 2022

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Sep 20, 2022

fix(explore): fix chart save when dashboard deleted (apache#21497)

4679dfc

(cherry picked from commit 6644a84)

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Sep 21, 2022

fix(explore): fix chart save when dashboard deleted (apache#21497)

b6d887c

(cherry picked from commit 6644a84)

jinghua-qa added the preset:2022.37 label Sep 21, 2022

mayurnewase added a commit to mayurnewase/incubator-superset that referenced this pull request Sep 22, 2022

Revert "fix(explore): fix chart save when dashboard deleted (apache#2…

9f592a5

…1497)" This reverts commit 6644a84.

codyml mentioned this pull request Oct 6, 2022

Charts reinserted into dashboards they were deleted from after save(overwrite) #21434

Closed

justinpark mentioned this pull request Jan 28, 2023

fix(explore): unable to update linked charts #22896

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

	endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({
	endpoint: `/api/v1/chart/${slice?.slice_id}?q=${rison.encode({

	message={this.state.alert ? this.state.alert : this.props.alert}
	message={this.state.alert \|\| this.props.alert}

fix(explore): fix chart save when dashboard deleted #21497

fix(explore): fix chart save when dashboard deleted #21497

Conversation

codyml commented Sep 16, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas commented Sep 19, 2022

github-actions bot commented Sep 19, 2022

rusackas commented Sep 19, 2022

rusackas commented Sep 19, 2022

github-actions bot commented Sep 19, 2022

jinghua-qa commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

jinghua-qa commented Sep 20, 2022 • edited Loading

michael-s-molina commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

codyml commented Sep 20, 2022

michael-s-molina commented Sep 20, 2022

michael-s-molina commented Sep 20, 2022

michael-s-molina left a comment • edited Loading

Choose a reason for hiding this comment

michael-s-molina commented Sep 20, 2022

jinghua-qa left a comment

Choose a reason for hiding this comment

jinghua-qa commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

jinghua-qa commented Sep 20, 2022

github-actions bot commented Sep 20, 2022

codyml commented Sep 16, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading

zhaoyongjie Sep 19, 2022 •

edited

Loading

jinghua-qa commented Sep 20, 2022 •

edited

Loading

michael-s-molina left a comment •

edited

Loading