From 253c7d99f0a2035ed27bc6f05d9eda48e22d5911 Mon Sep 17 00:00:00 2001 From: Grace Date: Fri, 13 Dec 2019 20:53:09 -0800 Subject: [PATCH 1/3] [explore view] fix: Inline edit chart title cause unintended overwrite original query parameter --- .../components/ExploreChartHeader_spec.jsx | 27 ++++++++++-- .../explore/components/ExploreChartHeader.jsx | 41 ++++++++++--------- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx b/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx index ddc107ab5ecfc..50537565d18e4 100644 --- a/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx +++ b/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,14 @@ describe('ExploreChartHeader', () => { expect(wrapper.find(EditableTitle)).toHaveLength(1); expect(wrapper.find(ExploreActionButtons)).toHaveLength(1); }); + + it('updateChartTitleOrSaveSlice', () => { + 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, + }); + }); }); diff --git a/superset/assets/src/explore/components/ExploreChartHeader.jsx b/superset/assets/src/explore/components/ExploreChartHeader.jsx index d4d5967c0e6b0..afed2f9cc6388 100644 --- a/superset/assets/src/explore/components/ExploreChartHeader.jsx +++ b/superset/assets/src/explore/components/ExploreChartHeader.jsx @@ -65,25 +65,28 @@ class ExploreChartHeader extends React.PureComponent { slice_name: newTitle, action: isNewSlice ? 'saveas' : 'overwrite', }; - this.props.actions.saveSlice(this.props.form_data, params).then(json => { - const { data } = json; - if (isNewSlice) { - this.props.actions.updateChartId(data.slice.slice_id, 0); - this.props.actions.createNewSlice( - data.can_add, - data.can_download, - data.can_overwrite, - data.slice, - data.form_data, - ); - this.props.addHistory({ - isReplace: true, - title: `[chart] ${data.slice.slice_name}`, - }); - } else { - this.props.actions.updateChartTitle(newTitle); - } - }); + // this.props.slice hold the original slice params stored in slices table + this.props.actions + .saveSlice(this.props.slice.form_data, params) + .then(json => { + const { data } = json; + if (isNewSlice) { + this.props.actions.updateChartId(data.slice.slice_id, 0); + this.props.actions.createNewSlice( + data.can_add, + data.can_download, + data.can_overwrite, + data.slice, + data.form_data, + ); + this.props.addHistory({ + isReplace: true, + title: `[chart] ${data.slice.slice_name}`, + }); + } else { + this.props.actions.updateChartTitle(newTitle); + } + }); } renderChartTitle() { From d89e3d0f9eed3cfa1f2d22276e6a7fb74f2004e4 Mon Sep 17 00:00:00 2001 From: Grace Date: Wed, 18 Dec 2019 09:39:37 -0800 Subject: [PATCH 2/3] add more unit tests --- .../explore/components/SaveModal_spec.jsx | 51 ++++++++++++++++++- .../src/explore/components/SaveModal.jsx | 4 +- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/superset/assets/spec/javascripts/explore/components/SaveModal_spec.jsx b/superset/assets/spec/javascripts/explore/components/SaveModal_spec.jsx index 1353e0e938efb..316c76e0ef690 100644 --- a/superset/assets/spec/javascripts/explore/components/SaveModal_spec.jsx +++ b/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', () => { diff --git a/superset/assets/src/explore/components/SaveModal.jsx b/superset/assets/src/explore/components/SaveModal.jsx index 609b448e4110e..0cd8bad5e8f22 100644 --- a/superset/assets/src/explore/components/SaveModal.jsx +++ b/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(); From 58900bb0ff331e0754819c985b8d0c23181486f2 Mon Sep 17 00:00:00 2001 From: Grace Date: Wed, 18 Dec 2019 13:56:09 -0800 Subject: [PATCH 3/3] handle new slice case --- .../components/ExploreChartHeader_spec.jsx | 13 +++++- .../explore/components/ExploreChartHeader.jsx | 46 ++++++++++--------- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx b/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx index 50537565d18e4..ec7278ffd8e4e 100644 --- a/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx @@ -65,7 +65,7 @@ describe('ExploreChartHeader', () => { expect(wrapper.find(ExploreActionButtons)).toHaveLength(1); }); - it('updateChartTitleOrSaveSlice', () => { + it('should updateChartTitleOrSaveSlice for existed slice', () => { const newTitle = 'New Chart Title'; wrapper.instance().updateChartTitleOrSaveSlice(newTitle); expect(stub.call.length).toEqual(1); @@ -74,4 +74,15 @@ describe('ExploreChartHeader', () => { 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, + }); + }); }); diff --git a/superset/assets/src/explore/components/ExploreChartHeader.jsx b/superset/assets/src/explore/components/ExploreChartHeader.jsx index afed2f9cc6388..11989c6e397f5 100644 --- a/superset/assets/src/explore/components/ExploreChartHeader.jsx +++ b/superset/assets/src/explore/components/ExploreChartHeader.jsx @@ -61,32 +61,36 @@ 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.slice hold the original slice params stored in slices table - this.props.actions - .saveSlice(this.props.slice.form_data, params) - .then(json => { - const { data } = json; - if (isNewSlice) { - this.props.actions.updateChartId(data.slice.slice_id, 0); - this.props.actions.createNewSlice( - data.can_add, - data.can_download, - data.can_overwrite, - data.slice, - data.form_data, - ); - this.props.addHistory({ - isReplace: true, - title: `[chart] ${data.slice.slice_name}`, - }); - } else { - this.props.actions.updateChartTitle(newTitle); - } - }); + // 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); + this.props.actions.createNewSlice( + data.can_add, + data.can_download, + data.can_overwrite, + data.slice, + data.form_data, + ); + this.props.addHistory({ + isReplace: true, + title: `[chart] ${data.slice.slice_name}`, + }); + } else { + this.props.actions.updateChartTitle(newTitle); + } + }); } renderChartTitle() {