[explore] Fix and test slice id logging issue (apache#3339)

* [explore] fixed padding bug on filter section

* fix slice_id logging issue

* [superset-sqllab] fix slice_id population in appropriate column

* [explore-logging] test the slice_id logging fix

* fix travis errors

* fix nits pointed out in PR comments

* cleanup tests

* made python more beautiful

* made python even more beautiful

* made python even more more beautiful

* made python even more more more beautiful

* fix lint error

* make exception handling more specific

* fixed silly error

* fixed argument indentation

superset/models/core.py

@@ -86,8 +86,7 @@ class CssTemplate(Model, AuditMixinNullable):
 slice_user = Table('slice_user', metadata,
                    Column('id', Integer, primary_key=True),
                    Column('user_id', Integer, ForeignKey('ab_user.id')),
-                   Column('slice_id', Integer, ForeignKey('slices.id'))
-                   )
+                   Column('slice_id', Integer, ForeignKey('slices.id')))
 
 
 class Slice(Model, AuditMixinNullable, ImportMixin):
@@ -435,7 +434,7 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
                 new_filter_immune_slices.append(new_slc_id_str)
             if ('timed_refresh_immune_slices' in i_params_dict and
                     old_slc_id_str in
-                        i_params_dict['timed_refresh_immune_slices']):
+                    i_params_dict['timed_refresh_immune_slices']):
                 new_timed_refresh_immune_slices.append(new_slc_id_str)
             if ('expanded_slices' in i_params_dict and
                     old_slc_id_str in i_params_dict['expanded_slices']):
@@ -761,24 +760,26 @@ def wrapper(*args, **kwargs):
             post_data = request.form or {}
             d.update(post_data)
             d.update(kwargs)
-            slice_id = d.get('slice_id', 0)
+            slice_id = d.get('slice_id')
+
             try:
-                slice_id = int(slice_id) if slice_id else 0
-            except ValueError:
+                slice_id = int(
+                    slice_id or json.loads(d.get('form_data')).get('slice_id'))
+            except (ValueError, TypeError):
                 slice_id = 0
+
             params = ""
             try:
                 params = json.dumps(d)
             except:
                 pass
             stats_logger.incr(f.__name__)
             value = f(*args, **kwargs)
-
             sesh = db.session()
             log = cls(
                 action=f.__name__,
                 json=params,
-                dashboard_id=d.get('dashboard_id') or None,
+                dashboard_id=d.get('dashboard_id'),
                 slice_id=slice_id,
                 duration_ms=(
                     datetime.now() - start_dttm).total_seconds() * 1000,

tests/core_tests.py

@@ -41,6 +41,7 @@ def setUpClass(cls):
     def setUp(self):
         db.session.query(Query).delete()
         db.session.query(models.DatasourceAccessRequest).delete()
+        db.session.query(models.Log).delete()
 
     def tearDown(self):
         db.session.query(Query).delete()
@@ -726,7 +727,22 @@ def test_user_profile(self, username='admin'):
         data = self.get_json_resp('/superset/fave_dashboards_by_username/{}/'.format(username))
         self.assertNotIn('message', data)
 
+    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)
+        self.assertEqual(1, qry.count())
+
+    def test_slice_id_is_always_logged_correctly_on_ajax_request(self):
+        # superset/explore_json case
+        self.login(username="admin")
+        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)
+        self.assertEqual(1, qry.count())
+
 
 if __name__ == '__main__':
     unittest.main()
-