From e7ce38b486feddf144a3e1e25fe505f4e043f87e Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Tue, 2 Aug 2016 08:06:19 +0200 Subject: [PATCH] Smarter redirect on slice creation (#691) After ea8a7ec1ba26393047f70bcacd335b9e22fc63b8 creating a slice started redirecting to druid datasource from sqlalchemy tables. That's quite painful for sqlalchemy tables users. Instead of hardcoding a choice just query the db, if we don't have any druid datasource fallback to sqlalchemy tables. Bonus points we remove hacky javascript and make the message translatable. While at it fix druid client test to not hardcode datasource id. --- caravel/templates/caravel/add_slice.html | 6 ------ caravel/views.py | 23 +++++++++++++++++++++- tests/core_tests.py | 25 ++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 9 deletions(-) delete mode 100644 caravel/templates/caravel/add_slice.html diff --git a/caravel/templates/caravel/add_slice.html b/caravel/templates/caravel/add_slice.html deleted file mode 100644 index 6d74fca5bcb..00000000000 --- a/caravel/templates/caravel/add_slice.html +++ /dev/null @@ -1,6 +0,0 @@ - diff --git a/caravel/views.py b/caravel/views.py index fe1984254d1..9851ba8fbcf 100755 --- a/caravel/views.py +++ b/caravel/views.py @@ -520,7 +520,6 @@ if config['DRUID_IS_ACTIVE']: class SliceModelView(CaravelModelView, DeleteMixin): # noqa datamodel = SQLAInterface(models.Slice) - add_template = "caravel/add_slice.html" can_add = False label_columns = { 'datasource_link': 'Datasource', @@ -568,6 +567,28 @@ class SliceModelView(CaravelModelView, DeleteMixin): # noqa def pre_delete(self, obj): check_ownership(obj) + @expose('/add', methods=['GET', 'POST']) + @has_access + def add(self): + widget = self._add() + if not widget: + return redirect(self.get_redirect()) + + a_druid_datasource = db.session.query(models.DruidDatasource).first() + if a_druid_datasource is not None: + url = "/druiddatasourcemodelview/list/" + msg = _( + "Click on a datasource link to create a Slice, " + "or click on a table link here " + "to create a Slice for a table" + ) + else: + url = "/tablemodelview/list/" + msg = _("Click on a table link to create a Slice") + + redirect_url = "/r/msg/?url={}&msg={}".format(url, msg) + return redirect(redirect_url) + appbuilder.add_view( SliceModelView, "Slices", diff --git a/tests/core_tests.py b/tests/core_tests.py index b6f9154b388..40e206c2379 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -17,7 +17,7 @@ from flask_appbuilder.security.sqla import models as ab_models import caravel from caravel import app, db, models, utils, appbuilder -from caravel.models import DruidCluster +from caravel.models import DruidCluster, DruidDatasource os.environ['CARAVEL_CONFIG'] = 'tests.caravel_test_config' @@ -218,6 +218,27 @@ class CoreTests(CaravelTestCase): assert new_slice in dash.slices assert len(set(dash.slices)) == len(dash.slices) + def test_add_slice_redirect_to_sqla(self, username='admin'): + self.login(username=username) + url = '/slicemodelview/add' + resp = self.client.get(url, follow_redirects=True) + assert "Click on a table link to create a Slice" in resp.data.decode('utf-8') + + def test_add_slice_redirect_to_druid(self, username='admin'): + datasource = DruidDatasource( + datasource_name="datasource_name", + ) + db.session.add(datasource) + db.session.commit() + + self.login(username=username) + url = '/slicemodelview/add' + resp = self.client.get(url, follow_redirects=True) + assert "Click on a datasource link to create a Slice" in resp.data.decode('utf-8') + + db.session.delete(datasource) + db.session.commit() + def test_gamma(self): self.login(username='gamma') resp = self.client.get('/slicemodelview/list/') @@ -409,7 +430,7 @@ class DruidTests(CaravelTestCase): instance.export_pandas.return_value = df instance.query_dict = {} instance.query_builder.last_query.query_dict = {} - resp = self.client.get('/caravel/explore/druid/1/?viz_type=table&granularity=one+day&druid_time_origin=&since=7+days+ago&until=now&row_limit=5000&include_search=false&metrics=count&groupby=name&flt_col_0=dim1&flt_op_0=in&flt_eq_0=&slice_id=&slice_name=&collapsed_fieldsets=&action=&datasource_name=test_datasource&datasource_id=1&datasource_type=druid&previous_viz_type=table&json=true&force=true') + resp = self.client.get('/caravel/explore/druid/{}/?viz_type=table&granularity=one+day&druid_time_origin=&since=7+days+ago&until=now&row_limit=5000&include_search=false&metrics=count&groupby=name&flt_col_0=dim1&flt_op_0=in&flt_eq_0=&slice_id=&slice_name=&collapsed_fieldsets=&action=&datasource_name=test_datasource&datasource_id={}&datasource_type=druid&previous_viz_type=table&json=true&force=true'.format(datasource_id, datasource_id)) assert "Canada" in resp.data.decode('utf-8')