[GitHub] [superset] codyml commented on a diff in pull request #21497: fix(explore): fix chart save when dashboard deleted

[GitBox <gi...@apache.org>]

codyml commented on code in PR #21497:
URL: https://github.com/apache/superset/pull/21497#discussion_r974199540


##########
superset-frontend/src/explore/actions/saveModalActions.js:
##########
@@ -215,3 +218,23 @@ export const getDashboard = dashboardId => async dispatch => {
     throw error;
   }
 };
+
+//  Get dashboards the slice is added to
+export const getSliceDashboards = slice => async dispatch => {
+  if (slice) {

Review Comment:
   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.



##########
superset-frontend/src/explore/actions/saveModalActions.js:
##########
@@ -215,3 +218,23 @@ export const getDashboard = dashboardId => async dispatch => {
     throw error;
   }
 };
+
+//  Get dashboards the slice is added to
+export const getSliceDashboards = slice => async dispatch => {
+  if (slice) {
+    try {
+      const response = await SupersetClient.get({
+        endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({

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



##########
superset-frontend/src/explore/actions/saveModalActions.js:
##########
@@ -215,3 +218,23 @@ export const getDashboard = dashboardId => async dispatch => {
     throw error;
   }
 };
+
+//  Get dashboards the slice is added to
+export const getSliceDashboards = slice => async dispatch => {
+  if (slice) {
+    try {
+      const response = await SupersetClient.get({
+        endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({
+          columns: ['dashboards.id'],
+        })}`,
+      });
+
+      return response.json.result.dashboards.map(({ id }) => id);
+    } catch (error) {
+      dispatch(saveSliceFailed());
+      throw error;
+    }
+  }
+
+  return [];

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



##########
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);

Review Comment:
   Much cleaner, thanks.



##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -183,6 +182,14 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
       }
     }
 
+    //  Get chart dashboards
+    let sliceDashboards: number[];
+    promise = promise
+      .then(() => this.props.actions.getSliceDashboards(this.props.slice))
+      .then(dashboards => {
+        sliceDashboards = dashboards;
+      });
+

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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org