From 1ee87cc4d1810163bd00788636291588ecbfcbdf Mon Sep 17 00:00:00 2001 From: John Bodley <4567245+john-bodley@users.noreply.github.com> Date: Wed, 2 Sep 2020 16:03:25 -0700 Subject: [PATCH] fix: dashboard extra filters (#10692) Co-authored-by: John Bodley --- superset/examples/world_bank.py | 2 +- superset/views/utils.py | 37 ++++- tests/strategy_tests.py | 2 +- tests/utils_tests.py | 279 ++++---------------------------- 4 files changed, 65 insertions(+), 255 deletions(-) diff --git a/superset/examples/world_bank.py b/superset/examples/world_bank.py index 8a696e7539ba6..3f7509bb824b2 100644 --- a/superset/examples/world_bank.py +++ b/superset/examples/world_bank.py @@ -153,7 +153,7 @@ def load_world_bank_health_n_pop( # pylint: disable=too-many-locals, too-many-s "column": "region", "key": "2s98dfu", "metric": "sum__SP_POP_TOTL", - "multiple": True, + "multiple": False, }, { "asc": False, diff --git a/superset/views/utils.py b/superset/views/utils.py index 7e50800207361..eaecc5fe87031 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -336,7 +336,7 @@ def get_dashboard_extra_filters( return [] -def build_extra_filters( +def build_extra_filters( # pylint: disable=too-many-locals,too-many-nested-blocks layout: Dict[str, Dict[str, Any]], filter_scopes: Dict[str, Dict[str, Any]], default_filters: Dict[str, Dict[str, List[Any]]], @@ -344,11 +344,22 @@ def build_extra_filters( ) -> List[Dict[str, Any]]: extra_filters = [] - # do not apply filters if chart is not in filter's scope or - # chart is immune to the filter + # do not apply filters if chart is not in filter's scope or chart is immune to the + # filter. for filter_id, columns in default_filters.items(): + filter_slice = db.session.query(Slice).filter_by(id=filter_id).one_or_none() + + filter_configs = ( + json.loads(filter_slice.params or "{}").get("filter_configs") or [] + if filter_slice + else [] + ) + scopes_by_filter_field = filter_scopes.get(filter_id, {}) for col, val in columns.items(): + if not val: + continue + current_field_scopes = scopes_by_filter_field.get(col, {}) scoped_container_ids = current_field_scopes.get("scope", ["ROOT_ID"]) immune_slice_ids = current_field_scopes.get("immune", []) @@ -357,7 +368,25 @@ def build_extra_filters( if slice_id not in immune_slice_ids and is_slice_in_container( layout, container_id, slice_id ): - extra_filters.append({"col": col, "op": "in", "val": val}) + # Ensure that the filter value encoding adheres to the filter select + # type. + for filter_config in filter_configs: + if filter_config["column"] == col: + is_multiple = filter_config["multiple"] + + if not is_multiple and isinstance(val, list): + val = val[0] + elif is_multiple and not isinstance(val, list): + val = [val] + break + + extra_filters.append( + { + "col": col, + "op": "in" if isinstance(val, list) else "==", + "val": val, + } + ) return extra_filters diff --git a/tests/strategy_tests.py b/tests/strategy_tests.py index c4f0019c6dd9c..f8cea8be31a8e 100644 --- a/tests/strategy_tests.py +++ b/tests/strategy_tests.py @@ -168,7 +168,7 @@ def test_get_form_data(self): "slice_id": chart_id, "extra_filters": [ {"col": "name", "op": "in", "val": ["Alice", "Bob"]}, - {"col": "__time_range", "op": "in", "val": "100 years ago : today"}, + {"col": "__time_range", "op": "==", "val": "100 years ago : today"}, ], } self.assertEqual(result, expected) diff --git a/tests/utils_tests.py b/tests/utils_tests.py index 4d7806448af16..91d1ad39d5a55 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -35,6 +35,8 @@ from superset import app, db, security_manager from superset.exceptions import CertificateException, SupersetException from superset.models.core import Database, Log +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice from superset.utils.cache_manager import CacheManager from superset.utils.core import ( base_json_conv, @@ -965,268 +967,47 @@ def test_get_iterable(self): self.assertListEqual(get_iterable("foo"), ["foo"]) def test_build_extra_filters(self): - layout = { - "CHART-2ee52f30": { - "children": [], - "id": "CHART-2ee52f30", - "meta": { - "chartId": 1020, - "height": 38, - "sliceName": "Chart 927", - "width": 6, - }, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - "TAB-asWdJzKmTN", - "ROW-i_sG4ccXE", - ], - "type": "CHART", - }, - "CHART-36bfc934": { - "children": [], - "id": "CHART-36bfc934", - "meta": { - "chartId": 1018, - "height": 26, - "sliceName": "Region Filter", - "width": 2, - }, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-W62P60D88", - "ROW-1e064e3c", - "COLUMN-fe3914b8", - ], - "type": "CHART", - }, - "CHART-E_y2cuNHTv": { - "children": [], - "id": "CHART-E_y2cuNHTv", - "meta": {"chartId": 998, "height": 55, "sliceName": "MAP", "width": 6}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-W62P60D88", - "ROW-1e064e3c", - ], - "type": "CHART", - }, - "CHART-JNxDOsAfEb": { - "children": [], - "id": "CHART-JNxDOsAfEb", - "meta": { - "chartId": 1015, - "height": 27, - "sliceName": "Population", - "width": 4, - }, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-W62P60D88", - "ROW-1e064e3c", - "COLUMN-fe3914b8", - ], - "type": "CHART", - }, - "CHART-KoOwqalV80": { - "children": [], - "id": "CHART-KoOwqalV80", - "meta": { - "chartId": 927, - "height": 20, - "sliceName": "Chart 927", - "width": 4, - }, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - "TAB-cHNWcBZC9", - "ROW-9b9vrWKPY", - ], - "type": "CHART", - }, - "CHART-YCQAPVK7mQ": { - "children": [], - "id": "CHART-YCQAPVK7mQ", - "meta": { - "chartId": 1023, - "height": 38, - "sliceName": "World's Population", - "width": 4, - }, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "ROW-UfxFT36oV5", - ], - "type": "CHART", - }, - "COLUMN-fe3914b8": { - "children": ["CHART-36bfc934", "CHART-JNxDOsAfEb"], - "id": "COLUMN-fe3914b8", - "meta": {"background": "BACKGROUND_TRANSPARENT", "width": 6}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-W62P60D88", - "ROW-1e064e3c", - ], - "type": "COLUMN", - }, - "DASHBOARD_VERSION_KEY": "v2", - "GRID_ID": { - "children": [], - "id": "GRID_ID", - "parents": ["ROOT_ID"], - "type": "GRID", - }, - "HEADER_ID": { - "id": "HEADER_ID", - "meta": {"text": "Test warmup 1023"}, - "type": "HEADER", - }, - "ROOT_ID": { - "children": ["TABS-Qq4sdkANSY"], - "id": "ROOT_ID", - "type": "ROOT", - }, - "ROW-1e064e3c": { - "children": ["COLUMN-fe3914b8", "CHART-E_y2cuNHTv"], - "id": "ROW-1e064e3c", - "meta": {"background": "BACKGROUND_TRANSPARENT"}, - "parents": ["ROOT_ID", "TABS-Qq4sdkANSY", "TAB-W62P60D88"], - "type": "ROW", - }, - "ROW-9b9vrWKPY": { - "children": ["CHART-KoOwqalV80"], - "id": "ROW-9b9vrWKPY", - "meta": {"background": "BACKGROUND_TRANSPARENT"}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - "TAB-cHNWcBZC9", - ], - "type": "ROW", - }, - "ROW-UfxFT36oV5": { - "children": ["CHART-YCQAPVK7mQ"], - "id": "ROW-UfxFT36oV5", - "meta": {"background": "BACKGROUND_TRANSPARENT"}, - "parents": ["ROOT_ID", "TABS-Qq4sdkANSY", "TAB-VrhTX2WUlO"], - "type": "ROW", - }, - "ROW-i_sG4ccXE": { - "children": ["CHART-2ee52f30"], - "id": "ROW-i_sG4ccXE", - "meta": {"background": "BACKGROUND_TRANSPARENT"}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - "TAB-asWdJzKmTN", - ], - "type": "ROW", - }, - "TAB-VrhTX2WUlO": { - "children": ["ROW-UfxFT36oV5", "TABS-N1zN4CIZP0"], - "id": "TAB-VrhTX2WUlO", - "meta": {"text": "New Tab"}, - "parents": ["ROOT_ID", "TABS-Qq4sdkANSY"], - "type": "TAB", - }, - "TAB-W62P60D88": { - "children": ["ROW-1e064e3c"], - "id": "TAB-W62P60D88", - "meta": {"text": "Tab 2"}, - "parents": ["ROOT_ID", "TABS-Qq4sdkANSY"], - "type": "TAB", - }, - "TAB-asWdJzKmTN": { - "children": ["ROW-i_sG4ccXE"], - "id": "TAB-asWdJzKmTN", - "meta": {"text": "nested tab 1"}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - ], - "type": "TAB", - }, - "TAB-cHNWcBZC9": { - "children": ["ROW-9b9vrWKPY"], - "id": "TAB-cHNWcBZC9", - "meta": {"text": "test2d tab 2"}, - "parents": [ - "ROOT_ID", - "TABS-Qq4sdkANSY", - "TAB-VrhTX2WUlO", - "TABS-N1zN4CIZP0", - ], - "type": "TAB", - }, - "TABS-N1zN4CIZP0": { - "children": ["TAB-asWdJzKmTN", "TAB-cHNWcBZC9"], - "id": "TABS-N1zN4CIZP0", - "meta": {}, - "parents": ["ROOT_ID", "TABS-Qq4sdkANSY", "TAB-VrhTX2WUlO"], - "type": "TABS", - }, - "TABS-Qq4sdkANSY": { - "children": ["TAB-VrhTX2WUlO", "TAB-W62P60D88"], - "id": "TABS-Qq4sdkANSY", - "meta": {}, - "parents": ["ROOT_ID"], - "type": "TABS", - }, - } + world_health = db.session.query(Dashboard).filter_by(slug="world_health").one() + layout = json.loads(world_health.position_json) + filter_ = db.session.query(Slice).filter_by(slice_name="Region Filter").one() + world = db.session.query(Slice).filter_by(slice_name="World's Population").one() + box_plot = db.session.query(Slice).filter_by(slice_name="Box plot").one() + treemap = db.session.query(Slice).filter_by(slice_name="Treemap").one() + filter_scopes = { - "1018": { - "region": {"scope": ["TAB-W62P60D88"], "immune": [998]}, - "country_name": {"scope": ["ROOT_ID"], "immune": [927, 998]}, + str(filter_.id): { + "region": {"scope": ["ROOT_ID"], "immune": [treemap.id]}, + "country_name": { + "scope": ["ROOT_ID"], + "immune": [treemap.id, box_plot.id], + }, } } + default_filters = { - "1018": {"region": ["North America"], "country_name": ["United States"]} + str(filter_.id): { + "region": ["North America"], + "country_name": ["United States"], + } } # immune to all filters - slice_id = 998 - extra_filters = build_extra_filters( - layout, filter_scopes, default_filters, slice_id + assert ( + build_extra_filters(layout, filter_scopes, default_filters, treemap.id) + == [] ) - expected = [] - self.assertEqual(extra_filters, expected) # in scope - slice_id = 1015 - extra_filters = build_extra_filters( - layout, filter_scopes, default_filters, slice_id - ) - expected = [ - {"col": "region", "op": "in", "val": ["North America"]}, + assert build_extra_filters( + layout, filter_scopes, default_filters, world.id + ) == [ + {"col": "region", "op": "==", "val": "North America"}, {"col": "country_name", "op": "in", "val": ["United States"]}, ] - self.assertEqual(extra_filters, expected) - # not in scope - slice_id = 927 - extra_filters = build_extra_filters( - layout, filter_scopes, default_filters, slice_id - ) - expected = [] - self.assertEqual(extra_filters, expected) + assert build_extra_filters( + layout, filter_scopes, default_filters, box_plot.id + ) == [{"col": "region", "op": "==", "val": "North America"}] def test_ssl_certificate_parse(self): parsed_certificate = parse_ssl_cert(ssl_certificate)