add fixes for code review

apache · Jun 13, 2017 · 2a42cee · 2a42cee
1 parent d52273e
commit 2a42cee
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 51 deletions.
diff --git a/superset/assets/javascripts/components/EditableTitle.jsx b/superset/assets/javascripts/components/EditableTitle.jsx
@@ -2,6 +2,16 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import TooltipWrapper from './TooltipWrapper';
 
+const propTypes = {
+  title: PropTypes.string,
+  canEdit: PropTypes.bool,
+  onSaveTitle: PropTypes.func.isRequired,
+};
+const defaultProps = {
+  title: 'Title',
+  canEdit: false,
+};
+
 class EditableTitle extends React.PureComponent {
   constructor(props) {
     super(props);
@@ -68,14 +78,7 @@ class EditableTitle extends React.PureComponent {
     );
   }
 }
-EditableTitle.propTypes = {
-  title: PropTypes.string,
-  canEdit: PropTypes.bool,
-  onSaveTitle: PropTypes.func.isRequired,
-};
-EditableTitle.defaultProps = {
-  title: 'Title',
-  canEdit: false,
-};
+EditableTitle.propTypes = propTypes;
+EditableTitle.defaultProps = defaultProps;
 
 export default EditableTitle;
diff --git a/superset/assets/javascripts/explore/components/ChartContainer.jsx b/superset/assets/javascripts/explore/components/ChartContainer.jsx
@@ -13,7 +13,6 @@ import TooltipWrapper from '../../components/TooltipWrapper';
 import Timer from '../../components/Timer';
 import { getExploreUrl } from '../exploreUtils';
 import { getFormDataFromControls } from '../stores/store';
-import { serialize } from '../../../utils/common';
 import CachedLabel from '../../components/CachedLabel';
 
 const CHART_STATUS_MAP = {
@@ -151,12 +150,11 @@ class ChartContainer extends React.PureComponent {
   }
 
   updateChartTitle(newTitle) {
-    const params = {};
-    params.slice_name = newTitle;
-    params.action = 'overwrite';
-    params.form_data = this.props.formData;
-    const saveUrl = '/superset/explore/' +
-      `${this.props.datasourceType}/${this.props.datasourceId}/?${serialize(params)}`;
+    const params = {
+      slice_name: newTitle,
+      action: 'overwrite',
+    };
+    const saveUrl = getExploreUrl(this.props.formData, 'base', false, null, params);
     this.props.actions.saveSlice(saveUrl)
       .then(() => {
         this.props.actions.updateChartTitle(newTitle);

diff --git a/superset/assets/javascripts/explore/exploreUtils.js b/superset/assets/javascripts/explore/exploreUtils.js
@@ -1,7 +1,8 @@
 /* eslint camelcase: 0 */
 import URI from 'urijs';
 
-export function getExploreUrl(form_data, endpointType = 'base', force = false, curUrl = null) {
+export function getExploreUrl(form_data, endpointType = 'base', force = false,
+  curUrl = null, requestParams = {}) {
   if (!form_data.datasource) {
     return null;
   }
@@ -38,6 +39,14 @@ export function getExploreUrl(form_data, endpointType = 'base', force = false, c
   if (endpointType === 'query') {
     search.query = 'true';
   }
+  const paramNames = Object.keys(requestParams);
+  if (paramNames.length) {
+    paramNames.forEach((name) => {
+      if (requestParams.hasOwnProperty(name)) {
+        search[name] = requestParams[name];
+      }
+    });
+  }
   uri = uri.search(search).directory(directory);
   return uri.toString();
 }
diff --git a/superset/assets/utils/common.js b/superset/assets/utils/common.js
@@ -86,33 +86,3 @@ export function getShortUrl(longUrl, callback) {
     },
   });
 }
-
-export function serialize(obj) {
-  const parts = [];
-  const addParamParts = (propName, propValue) => {
-    const type = typeof propValue;
-    switch (type) {
-      case 'object':
-        parts.push(propName + '=' + encodeURIComponent(JSON.stringify(propValue)));
-        break;
-      case 'boolean':
-      case 'number':
-      case 'string':
-        parts.push(propName + '=' + encodeURIComponent(propValue));
-        break;
-      default:
-        parts.push(propName + '=');
-    }
-  };
-
-  Object.keys(obj)
-    .filter(key => obj.hasOwnProperty(key))
-    .forEach((prop) => {
-      if (Array.isArray(obj[prop])) {
-        obj[prop].forEach(value => addParamParts(prop, value));
-      } else {
-        addParamParts(prop, obj[prop]);
-      }
-    });
-  return parts.join('&');
-}
diff --git a/tests/core_tests.py b/tests/core_tests.py
@@ -380,8 +380,11 @@ def test_save_dash(self, username='admin'):
 
     def test_save_dash_with_dashboard_title(self, username='admin'):
         self.login(username=username)
-        dash = db.session.query(models.Dashboard).filter_by(
-            slug="births").first()
+        dash = (
+            db.session.query(models.Dashboard)
+                .filter_by(slug="births")
+                .first()
+        )
         origin_title = dash.dashboard_title
         positions = []
         for i, slc in enumerate(dash.slices):
@@ -400,8 +403,11 @@ def test_save_dash_with_dashboard_title(self, username='admin'):
         }
         url = '/superset/save_dash/{}/'.format(dash.id)
         resp = self.get_resp(url, data=dict(data=json.dumps(data)))
-        updatedDash = db.session.query(models.Dashboard).filter_by(
-            slug="births").first()
+        updatedDash = (
+            db.session.query(models.Dashboard)
+                .filter_by(slug="births")
+                .first()
+        )
         self.assertEqual(updatedDash.dashboard_title, 'new title')
         # # bring back dashboard original title
         data['dashboard_title'] = origin_title