[Adhoc Filters] integrating dashboard filters with adhoc filters #5056

gabe-lyons · 2018-05-22T21:19:46Z

Thanks to @michellethomas for reporting. Filters on dashboards were not compatible with adhoc filters. This PR essentially factors out adhoc_filter => legacy filter conversion into a util function and then calls it both places merge_extra_filters is called.

relevant issue: #5049

test plan:

created a dashboard locally with a filter
added slices with no filters, with adhoc_filters and with legacy filters
applied the filter to dashboard
verified all the slices incorporated the filter when rendering
clicked on various slices
verified they all had the dashboard's filter applied in the explore view

reviewers:
@michellethomas @john-bodley @graceguo-supercat @williaster @mistercrunch

codecov-io · 2018-05-22T21:55:49Z

Codecov Report

Merging #5056 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5056      +/-   ##
==========================================
+ Coverage   77.52%   77.53%   +<.01%     
==========================================
  Files          44       44              
  Lines        8707     8710       +3     
==========================================
+ Hits         6750     6753       +3     
  Misses       1957     1957

Impacted Files	Coverage Δ
superset/viz.py	`81.42% <100%> (-0.29%)`	⬇️
superset/utils.py	`88.52% <100%> (+0.61%)`	⬆️
superset/views/core.py	`74.66% <100%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c44223...eadb9c5. Read the comment docs.

michellethomas · 2018-05-22T22:17:43Z

superset/utils.py

@@ -841,3 +841,39 @@ def ensure_path_exists(path):
    except OSError as exc:
        if not (os.path.isdir(path) and exc.errno == errno.EEXIST):
            raise
+
+
+def split_adhoc_filters_into_legacy_filters(fd):


Can you add a docstring here?

Calling it "legacy" filters sounds a little to me like this is legacy code that will be cleaned up eventually, but this translation will be sticking around correct? I wonder if there's a word other than "legacy" that would describe this. I'm fine if not though.

@michellethomas done!

john-bodley · 2018-05-22T22:40:21Z

superset/utils.py

@@ -841,3 +841,45 @@ def ensure_path_exists(path):
    except OSError as exc:
        if not (os.path.isdir(path) and exc.errno == errno.EEXIST):
            raise
+
+
+def split_adhoc_filters_into_base_filters(fd):


Typically we use the form_data variable to represent form data. It's probably best to rename fd to form_data for consistency.

@john-bodley fd is used throughout the codebase already

john-bodley · 2018-05-22T22:42:17Z

superset/views/core.py

@@ -1279,6 +1279,7 @@ def explore(self, datasource_type=None, datasource_id=None):
        form_data['datasource'] = str(datasource_id) + '__' + datasource_type

        # On explore, merge extra filters into the form data
+        form_data = utils.split_adhoc_filters_into_base_filters(form_data)


Your function mutates the form data so there's no need to re-assign. You can either make split_adhoc_filters_into_base_filters create a copy and return the mutated copy or simply mutate the input which seems to be the pattern as per line #243.

john-bodley · 2018-05-22T22:42:25Z

superset/viz.py

+        # extras are used to query elements specific to a datasource type
+        # for instance the extra where clause that applies only to Tables
+
+        form_data = utils.split_adhoc_filters_into_base_filters(form_data)


john-bodley

@GabeLoins a couple of small nits, but otherwise LGTM.

john-bodley · 2018-05-23T18:36:10Z

superset/viz.py

-        # Add extra filters into the query form data
+        # extras are used to query elements specific to a datasource type
+        # for instance the extra where clause that applies only to Tables
+


Nit. Remove blank line.

john-bodley · 2018-05-23T18:36:16Z

superset/viz.py

+        # for instance the extra where clause that applies only to Tables
+
+        utils.split_adhoc_filters_into_base_filters(form_data)
+


Nit. Remove blank line.

(cherry picked from commit fa3e4e2)

This reverts commit fa3e4e2.

gabe-lyons force-pushed the gabe_fixing_dashboard_filters branch 2 times, most recently from f5ed925 to b3c92ff Compare May 22, 2018 21:38

gabe-lyons changed the title ~~[WIP] integrating dashboard filters with adhoc filters~~ [Adhoc Filters] integrating dashboard filters with adhoc filters May 22, 2018

michellethomas reviewed May 22, 2018

View reviewed changes

gabe-lyons force-pushed the gabe_fixing_dashboard_filters branch from b3c92ff to 999f7a4 Compare May 22, 2018 22:38

john-bodley reviewed May 22, 2018

View reviewed changes

integrating dashboard filters with adhoc filters

eadb9c5

gabe-lyons force-pushed the gabe_fixing_dashboard_filters branch from 999f7a4 to eadb9c5 Compare May 22, 2018 22:56

john-bodley reviewed May 23, 2018

View reviewed changes

john-bodley approved these changes May 23, 2018

View reviewed changes

john-bodley merged commit fa3e4e2 into apache:master May 23, 2018

michellethomas mentioned this pull request May 23, 2018

Slices with new Adhoc Filters break dashboards #5049

Closed

3 tasks

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

integrating dashboard filters with adhoc filters (apache#5056)

c31b21b

(cherry picked from commit fa3e4e2)

john-bodley added a commit that referenced this pull request May 24, 2018

Revert "integrating dashboard filters with adhoc filters (#5056)"

2d8bd63

This reverts commit fa3e4e2.

timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018

integrating dashboard filters with adhoc filters (apache#5056)

b1c14dd

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

integrating dashboard filters with adhoc filters (apache#5056)

8a61993

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Adhoc Filters] integrating dashboard filters with adhoc filters #5056

[Adhoc Filters] integrating dashboard filters with adhoc filters #5056

gabe-lyons commented May 22, 2018 •

edited

Loading

codecov-io commented May 22, 2018 •

edited

Loading

michellethomas May 22, 2018 •

edited

Loading

michellethomas May 22, 2018

gabe-lyons May 22, 2018

john-bodley May 22, 2018

gabe-lyons May 22, 2018

john-bodley May 22, 2018

john-bodley May 22, 2018

john-bodley left a comment

john-bodley May 23, 2018

john-bodley May 23, 2018

		# for instance the extra where clause that applies only to Tables

		utils.split_adhoc_filters_into_base_filters(form_data)

[Adhoc Filters] integrating dashboard filters with adhoc filters #5056

[Adhoc Filters] integrating dashboard filters with adhoc filters #5056

Conversation

gabe-lyons commented May 22, 2018 • edited Loading

codecov-io commented May 22, 2018 • edited Loading

Codecov Report

michellethomas May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabe-lyons commented May 22, 2018 •

edited

Loading

codecov-io commented May 22, 2018 •

edited

Loading

michellethomas May 22, 2018 •

edited

Loading