[explore view] fix: Inline edit chart title cause unintended overwrite original query parameter (#8835)

* [explore view] fix: Inline edit chart title cause unintended overwrite original query parameter

* add more unit tests

* handle new slice case

superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx

@@ -23,14 +23,25 @@ import ExploreChartHeader from '../../../../src/explore/components/ExploreChartH
 import ExploreActionButtons from '../../../../src/explore/components/ExploreActionButtons';
 import EditableTitle from '../../../../src/components/EditableTitle';
 
+const stub = jest.fn(() => ({
+  then: () => {},
+}));
 const mockProps = {
-  actions: {},
+  actions: {
+    saveSlice: stub,
+  },
   can_overwrite: true,
   can_download: true,
   isStarred: true,
-  slice: {},
+  slice: {
+    form_data: {
+      viz_type: 'line',
+    },
+  },
   table_name: 'foo',
-  form_data: {},
+  form_data: {
+    viz_type: 'table',
+  },
   timeout: 1000,
   chart: {
     queryResponse: {},
@@ -53,4 +64,25 @@ describe('ExploreChartHeader', () => {
     expect(wrapper.find(EditableTitle)).toHaveLength(1);
     expect(wrapper.find(ExploreActionButtons)).toHaveLength(1);
   });
+
+  it('should updateChartTitleOrSaveSlice for existed slice', () => {
+    const newTitle = 'New Chart Title';
+    wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
+    expect(stub.call.length).toEqual(1);
+    expect(stub).toHaveBeenCalledWith(mockProps.slice.form_data, {
+      action: 'overwrite',
+      slice_name: newTitle,
+    });
+  });
+
+  it('should updateChartTitleOrSaveSlice for new slice', () => {
+    const newTitle = 'New Chart Title';
+    wrapper.setProps({ slice: undefined });
+    wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
+    expect(stub.call.length).toEqual(1);
+    expect(stub).toHaveBeenCalledWith(mockProps.form_data, {
+      action: 'saveas',
+      slice_name: newTitle,
+    });
+  });
 });

superset/assets/spec/javascripts/explore/components/SaveModal_spec.jsx

@@ -146,7 +146,10 @@ describe('SaveModal', () => {
 
       sinon.stub(defaultProps.actions, 'saveSlice').callsFake(() =>
         Promise.resolve({
-          data: { dashboard: '/mock/', slice: { slice_url: '/mock/' } },
+          data: {
+            dashboard: '/mock_dashboard/',
+            slice: { slice_url: '/mock_slice/' },
+          },
         }),
       );
     });
@@ -190,6 +193,52 @@ describe('SaveModal', () => {
       const args = defaultProps.actions.saveSlice.getCall(0).args;
       expect(args[1].new_dashboard_name).toBe(newDashboardName);
     });
+
+    describe('should always reload or redirect', () => {
+      let wrapper;
+      beforeEach(() => {
+        wrapper = getWrapper();
+        sinon.stub(window.location, 'assign');
+      });
+      afterEach(() => {
+        window.location.assign.restore();
+      });
+
+      it('Save & go to dashboard', done => {
+        wrapper.instance().saveOrOverwrite(true);
+        defaultProps.actions.saveSlice().then(() => {
+          expect(window.location.assign.callCount).toEqual(1);
+          expect(window.location.assign.getCall(0).args[0]).toEqual(
+            'http://localhost/mock_dashboard/',
+          );
+          done();
+        });
+      });
+
+      it('saveas new slice', done => {
+        wrapper.setState({ action: 'saveas', newSliceName: 'new slice name' });
+        wrapper.instance().saveOrOverwrite(false);
+        defaultProps.actions.saveSlice().then(() => {
+          expect(window.location.assign.callCount).toEqual(1);
+          expect(window.location.assign.getCall(0).args[0]).toEqual(
+            '/mock_slice/',
+          );
+          done();
+        });
+      });
+
+      it('overwrite original slice', done => {
+        wrapper.setState({ action: 'overwrite' });
+        wrapper.instance().saveOrOverwrite(false);
+        defaultProps.actions.saveSlice().then(() => {
+          expect(window.location.assign.callCount).toEqual(1);
+          expect(window.location.assign.getCall(0).args[0]).toEqual(
+            '/mock_slice/',
+          );
+          done();
+        });
+      });
+    });
   });
 
   describe('fetchDashboards', () => {

superset/assets/src/explore/components/ExploreChartHeader.jsx

@@ -61,11 +61,18 @@ class ExploreChartHeader extends React.PureComponent {
 
   updateChartTitleOrSaveSlice(newTitle) {
     const isNewSlice = !this.props.slice;
+    const currentFormData = isNewSlice
+      ? this.props.form_data
+      : this.props.slice.form_data;
+
     const params = {
       slice_name: newTitle,
       action: isNewSlice ? 'saveas' : 'overwrite',
     };
-    this.props.actions.saveSlice(this.props.form_data, params).then(json => {
+    // this.props.slice hold the original slice params stored in slices table
+    // when chart is saved or overwritten, the explore view will reload page
+    // to make sure sync with updated query params
+    this.props.actions.saveSlice(currentFormData, params).then(json => {
       const { data } = json;
       if (isNewSlice) {
         this.props.actions.updateChartId(data.slice.slice_id, 0);

superset/assets/src/explore/components/SaveModal.jsx

@@ -154,9 +154,9 @@ class SaveModal extends React.Component {
         }
         // Go to new slice url or dashboard url
         if (gotodash) {
-          window.location = supersetURL(data.dashboard);
+          window.location.assign(supersetURL(data.dashboard));
         } else {
-          window.location = data.slice.slice_url;
+          window.location.assign(data.slice.slice_url);
         }
       });
     this.props.onHide();