Merge pull request apache#91 from michellethomas/cherry_pick_form_dat…

…a_fix

Fix form data issue switching viz types (apache#5100)

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

@@ -292,12 +292,6 @@ ExploreViewContainer.propTypes = propTypes;
 
 function mapStateToProps({ explore, charts, impressionId }) {
   const form_data = getFormDataFromControls(explore.controls);
-  // fill in additional params stored in form_data but not used by control
-  Object.keys(explore.rawFormData).forEach((key) => {
-    if (form_data[key] === undefined) {
-      form_data[key] = explore.rawFormData[key];
-    }
-  });
   const chartKey = Object.keys(charts)[0];
   const chart = charts[chartKey];
   return {

superset/assets/src/explore/controls.jsx

@@ -1841,6 +1841,13 @@ export const controls = {
     description: t('The number of seconds before expiring the cache'),
   },
 
+  url_params: {
+    type: 'HiddenControl',
+    label: t('URL Parameters'),
+    hidden: true,
+    description: t('Extra parameters for use in jinja templated queries'),
+  },
+
   order_by_entity: {
     type: 'CheckboxControl',
     label: t('Order by entity id'),

superset/assets/src/explore/store.js

@@ -115,13 +115,6 @@ export function applyDefaultFormData(form_data) {
       formData[k] = form_data[k];
     }
   });
-  // fill in additional params stored in form_data but not used by control
-  Object.keys(form_data)
-    .forEach((key) => {
-      if (formData[key] === undefined) {
-        formData[key] = form_data[key];
-      }
-    });
   return formData;
 }
 

superset/assets/src/explore/visTypes.jsx

@@ -23,7 +23,7 @@ export const sections = {
     controlSetRows: [
       ['datasource'],
       ['viz_type'],
-      ['slice_id', 'cache_timeout'],
+      ['slice_id', 'cache_timeout', 'url_params'],
     ],
   },
   colorScheme: {

superset/views/core.py

@@ -962,7 +962,7 @@ def clean_fulfilled_requests(session):
         session.commit()
         return redirect('/accessrequestsmodelview/list/')
 
-    def get_form_data(self, slice_id=None):
+    def get_form_data(self, slice_id=None, use_slice_data=False):
         form_data = {}
         post_data = request.form.get('form_data')
         request_args_data = request.args.get('form_data')
@@ -999,7 +999,12 @@ def get_form_data(self, slice_id=None):
         slice_id = form_data.get('slice_id') or slice_id
         slc = None
 
-        if slice_id:
+        # Check if form data only contains slice_id
+        contains_only_slc_id = not any(key != 'slice_id' for key in form_data)
+
+        # Include the slice_form_data if request from explore or slice calls
+        # or if form_data only contains slice_id
+        if slice_id and (use_slice_data or contains_only_slc_id):
             slc = db.session.query(models.Slice).filter_by(id=slice_id).first()
             slice_form_data = slc.form_data.copy()
             # allow form_data in request override slice from_data
@@ -1037,7 +1042,7 @@ def get_viz(
     @has_access
     @expose('/slice/<slice_id>/')
     def slice(self, slice_id):
-        form_data, slc = self.get_form_data(slice_id)
+        form_data, slc = self.get_form_data(slice_id, use_slice_data=True)
         endpoint = '/superset/explore/?form_data={}'.format(
             parse.quote(json.dumps(form_data)),
         )
@@ -1237,7 +1242,7 @@ def datasource_info(datasource_id, datasource_type, form_data):
     @expose('/explore/', methods=['GET', 'POST'])
     def explore(self, datasource_type=None, datasource_id=None):
         user_id = g.user.get_id() if g.user else None
-        form_data, slc = self.get_form_data()
+        form_data, slc = self.get_form_data(use_slice_data=True)
 
         datasource_id, datasource_type = self.datasource_info(
             datasource_id, datasource_type, form_data)

tests/core_tests.py

@@ -195,10 +195,11 @@ def test_save_slice(self):
 
         form_data = {
             'viz_type': 'sankey',
-            'groupby': 'target',
+            'groupby': 'source',
             'metric': 'sum__value',
             'row_limit': 5000,
             'slice_id': new_slice_id,
+            'time_range': 'now',
         }
         # Setting the name back to its original name by overwriting new slice
         self.get_resp(
@@ -211,6 +212,7 @@ def test_save_slice(self):
         )
         slc = db.session.query(models.Slice).filter_by(id=new_slice_id).first()
         assert slc.slice_name == new_slice_name
+        assert slc.viz.form_data == form_data
         db.session.delete(slc)
 
     def test_filter_endpoint(self):
@@ -561,7 +563,7 @@ def test_slice_id_is_always_logged_correctly_on_web_request(self):
         # superset/explore case
         slc = db.session.query(models.Slice).filter_by(slice_name='Girls').one()
         qry = db.session.query(models.Log).filter_by(slice_id=slc.id)
-        self.get_resp(slc.slice_url, {'form_data': json.dumps(slc.viz.form_data)})
+        self.get_resp(slc.slice_url, {'form_data': json.dumps(slc.form_data)})
         self.assertEqual(1, qry.count())
 
     def test_slice_id_is_always_logged_correctly_on_ajax_request(self):
@@ -570,7 +572,7 @@ def test_slice_id_is_always_logged_correctly_on_ajax_request(self):
         slc = db.session.query(models.Slice).filter_by(slice_name='Girls').one()
         qry = db.session.query(models.Log).filter_by(slice_id=slc.id)
         slc_url = slc.slice_url.replace('explore', 'explore_json')
-        self.get_json_resp(slc_url, {'form_data': json.dumps(slc.viz.form_data)})
+        self.get_json_resp(slc_url, {'form_data': json.dumps(slc.form_data)})
         self.assertEqual(1, qry.count())
 
     def test_slice_query_endpoint(self):
@@ -659,46 +661,34 @@ def test_comments_in_sqlatable_query(self):
         rendered_query = text_type(table.get_from_clause())
         self.assertEqual(clean_query, rendered_query)
 
-    def test_slice_url_overrides(self):
-        # No override
-        self.login(username='admin')
-        slice_name = 'Girls'
-        slc = self.get_slice(slice_name, db.session)
-        resp = self.get_resp(slc.explore_json_url)
-        assert '"Jennifer"' in resp
-
-        # Overriding groupby
-        url = slc.get_explore_url(
-            base_url='/superset/explore_json',
-            overrides={'groupby': ['state']})
-        resp = self.get_resp(url)
-        assert '"CA"' in resp
-
     def test_slice_payload_no_data(self):
         self.login(username='admin')
         slc = self.get_slice('Girls', db.session)
+        json_endpoint = '/superset/explore_json/'
+        form_data = slc.form_data
+        form_data.update({
+            'filters': [{'col': 'state', 'op': 'in', 'val': ['N/A']}],
+        })
 
-        url = slc.get_explore_url(
-            base_url='/superset/explore_json',
-            overrides={
-                'filters': [{'col': 'state', 'op': 'in', 'val': ['N/A']}],
-            },
+        data = self.get_json_resp(
+            json_endpoint,
+            {'form_data': json.dumps(form_data)},
         )
-
-        data = self.get_json_resp(url)
         self.assertEqual(data['status'], utils.QueryStatus.SUCCESS)
         self.assertEqual(data['error'], 'No data')
 
     def test_slice_payload_invalid_query(self):
         self.login(username='admin')
         slc = self.get_slice('Girls', db.session)
+        form_data = slc.form_data
+        form_data.update({
+            'groupby': ['N/A'],
+        })
 
-        url = slc.get_explore_url(
-            base_url='/superset/explore_json',
-            overrides={'groupby': ['N/A']},
+        data = self.get_json_resp(
+            '/superset/explore_json/',
+            {'form_data': json.dumps(form_data)},
         )
-
-        data = self.get_json_resp(url)
         self.assertEqual(data['status'], utils.QueryStatus.FAILED)
         assert 'KeyError' in data['stacktrace']