Fixes issue apache#5804 - SQLLab - Saving a query with the same label…

… results in a new query being created
fabiomendescom · Feb 19, 2019 · b55fa87 · b55fa87
1 parent 0b1fbf8
commit b55fa87
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 15 deletions.
diff --git a/superset/assets/spec/javascripts/sqllab/actions/sqlLab_spec.js b/superset/assets/spec/javascripts/sqllab/actions/sqlLab_spec.js
@@ -21,7 +21,7 @@ import sinon from 'sinon';
 import fetchMock from 'fetch-mock';
 
 import * as actions from '../../../../src/SqlLab/actions/sqlLab';
-import { query } from '../fixtures';
+import { query, queryforupdate, queryforcreate } from '../fixtures';
 
 describe('async actions', () => {
   const mockBigNumber = '9223372036854775807';
@@ -34,32 +34,56 @@ describe('async actions', () => {
 
   afterEach(fetchMock.resetHistory);
 
-  describe('saveQuery', () => {
+  describe('saveQuery CREATE', () => {
     const saveQueryEndpoint = 'glob:*/savedqueryviewapi/api/create';
     fetchMock.post(saveQueryEndpoint, 'ok');
 
     it('posts to the correct url', () => {
       expect.assertions(1);
-      const thunk = actions.saveQuery(query);
+      const thunk = actions.saveQuery(queryforcreate);
 
       return thunk((/* mockDispatch */) => ({})).then(() => {
         expect(fetchMock.calls(saveQueryEndpoint)).toHaveLength(1);
       });
     });
 
     it('posts the correct query object', () => {
-      const thunk = actions.saveQuery(query);
+      const thunk = actions.saveQuery(queryforcreate);
 
       return thunk((/* mockDispatch */) => ({})).then(() => {
         const call = fetchMock.calls(saveQueryEndpoint)[0];
         const formData = call[1].body;
-        Object.keys(query).forEach((key) => {
+        Object.keys(queryforcreate).forEach((key) => {
           expect(formData.get(key)).toBeDefined();
         });
       });
     });
   });
 
+  describe('saveQuery UPDATE', () => {
+    const saveQueryEndpoint = 'glob:*/savedqueryviewapi/api/update/42';
+    fetchMock.put(saveQueryEndpoint, 'ok');
+
+    it('posts to the correct url', () => {
+      expect.assertions(1);
+      const thunk = actions.saveQuery(queryforupdate);
+
+      return thunk((/* mockDispatch */) => ({})).then(() => {
+        expect(fetchMock.calls(saveQueryEndpoint)).toHaveLength(1);
+      });
+    });
+
+    it('posts the correct query object', () => {
+      const thunk = actions.saveQuery(queryforupdate);
+
+      return thunk((/* mockDispatch */) => ({})).then(() => {
+        const call = fetchMock.calls(saveQueryEndpoint)[0];
+        const formData = call[1].body;
+        expect(formData).toBeUndefined();
+      });
+    });
+  });
+
   describe('fetchQueryResults', () => {
     const fetchQueryEndpoint = 'glob:*/superset/results/*';
     fetchMock.get(fetchQueryEndpoint, '{ "data": ' + mockBigNumber + ' }');

diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js
@@ -401,3 +401,27 @@ export const query = {
   ctas: false,
   cached: false,
 };
+
+export const queryforupdate = {
+  id: '42',
+  dbId: 1,
+  sql: 'SELECT * FROM something',
+  sqlEditorId: defaultQueryEditor.id,
+  tab: 'unimportant',
+  tempTableName: null,
+  runAsync: false,
+  ctas: false,
+  cached: false,
+};
+
+export const queryforcreate = {
+  id: 'VVVVVVVV',
+  dbId: 1,
+  sql: 'SELECT * FROM something',
+  sqlEditorId: defaultQueryEditor.id,
+  tab: 'unimportant',
+  tempTableName: null,
+  runAsync: false,
+  ctas: false,
+  cached: false,
+};
diff --git a/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js b/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js
@@ -57,6 +57,8 @@ describe('sqlLabReducer', () => {
       newState = { ...initialState };
       defaultQueryEditor = newState.queryEditors[0];
       qe = Object.assign({}, defaultQueryEditor);
+      // add a suffix so the two ids are not the same
+      qe.id += '2';
       newState = sqlLabReducer(newState, actions.addQueryEditor(qe));
       qe = newState.queryEditors[newState.queryEditors.length - 1];
     });

diff --git a/superset/assets/src/SqlLab/actions/sqlLab.js b/superset/assets/src/SqlLab/actions/sqlLab.js
@@ -77,14 +77,27 @@ export function resetState() {
 }
 
 export function saveQuery(query) {
-  return dispatch =>
-    SupersetClient.post({
-      endpoint: '/savedqueryviewapi/api/create',
-      postPayload: query,
-      stringify: false,
-    })
-      .then(() => dispatch(addSuccessToast(t('Your query was saved'))))
-      .catch(() => dispatch(addDangerToast(t('Your query could not be saved'))));
+     let ret;
+     if (isNaN((query.id))) {
+        ret = dispatch =>
+        SupersetClient.post({
+            endpoint: '/savedqueryviewapi/api/create',
+            postPayload: query,
+            stringify: false,
+        })
+            .then(() => dispatch(addSuccessToast(t('Your query was saved'))))
+            .catch(() => dispatch(addDangerToast(t('Your query could not be saved'))));
+     } else {
+        ret = dispatch =>
+        SupersetClient.put({
+            endpoint: '/savedqueryviewapi/api/update/' + query.id,
+            postPayload: query,
+            stringify: false,
+        })
+            .then(() => dispatch(addSuccessToast(t('Your query was saved'))))
+            .catch(() => dispatch(addDangerToast(t('Your query could not be saved'))));
+     }
+    return ret;
 }
 
 export function startQuery(query) {
@@ -202,7 +215,7 @@ export function setDatabases(databases) {
 export function addQueryEditor(queryEditor) {
   const newQueryEditor = {
     ...queryEditor,
-    id: shortid.generate(),
+    id: queryEditor.id ? queryEditor.id : shortid.generate(),
   };
   return { type: ADD_QUERY_EDITOR, queryEditor: newQueryEditor };
 }
@@ -400,6 +413,8 @@ export function popSavedQuery(saveQueryId) {
       .then(({ json }) => {
         const { result } = json;
         const queryEditorProps = {
+          id: saveQueryId,
+          description: result.description,
           title: result.label,
           dbId: result.db_id ? parseInt(result.db_id, 10) : null,
           schema: result.schema,

diff --git a/superset/assets/src/SqlLab/components/SaveQuery.jsx b/superset/assets/src/SqlLab/components/SaveQuery.jsx
@@ -26,6 +26,8 @@ import ModalTrigger from '../../components/ModalTrigger';
 
 const propTypes = {
   defaultLabel: PropTypes.string,
+  id: PropTypes.number,
+  description: PropTypes.string,
   sql: PropTypes.string,
   schema: PropTypes.string,
   dbId: PropTypes.number,
@@ -42,7 +44,8 @@ class SaveQuery extends React.PureComponent {
   constructor(props) {
     super(props);
     this.state = {
-      description: '',
+      id: props.id,
+      description: props.description,
       label: props.defaultLabel,
       showSave: false,
     };
@@ -54,6 +57,7 @@ class SaveQuery extends React.PureComponent {
   }
   onSave() {
     const query = {
+      id: this.state.id,
       label: this.state.label,
       description: this.state.description,
       db_id: this.props.dbId,

diff --git a/superset/assets/src/SqlLab/components/SqlEditor.jsx b/superset/assets/src/SqlLab/components/SqlEditor.jsx
@@ -252,6 +252,8 @@ class SqlEditor extends React.PureComponent {
             </span>
             <span className="m-r-5">
               <SaveQuery
+                id={qe.id}
+                description={qe.description}
                 defaultLabel={qe.title}
                 sql={qe.sql}
                 className="m-r-5"