[WIP][dashboard scoped filter] part 3: merge filter scope settings into dashboard redux state by graceguo-supercat · Pull Request #8522 · apache/superset

graceguo-supercat · 2019-11-06T22:28:27Z

SUMMARY

For each filter, add a scopes section into dashboard redux state:

{
  dashboardFilters: {
    '107': {
      chartId: 107,
      componentId: 'CHART-cb4a4fRA4_',
      filterName: 'Geo Filters',
      directPathToFilter: [
        'ROOT_ID',
        'TABS-zu0FcvKA_T',
        'TAB-BvXQux8j3p',
        'ROW-kAjhE4lwEU',
        'CHART-cb4a4fRA4_'
      ],
      isDateFilter: false,
      isInstantFilter: false,
      columns: {
        region: [
          'East Asia & Pacific'
        ]
      },
      labels: {
        country_name: 'country name'
      },
      scopes: {
        region: {
          scope: [
            'TAB-USeZvBtrV4'
          ],
          immune: []
        },
        country_name: {
          scope: [
            'TAB-hH18HAsBc'
          ],
          immune: []
        }
      }
    },
    '108': {
      chartId: 108,
      componentId: 'CHART-sC2TxRWldr',
      filterName: 'Time Filter',
      directPathToFilter: [
        'ROOT_ID',
        'TABS-zu0FcvKA_T',
        'TAB-6qz58YwIk',
        'ROW-9NyODqb45',
        'CHART-sC2TxRWldr'
      ],
      isDateFilter: true,
      isInstantFilter: true,
      columns: {},
      labels: {
        __time_range: 'Time range'
      },
      scopes: {
        __time_range: {
          scope: [
            'TAB-6qz58YwIk'
          ],
          immune: []
        }
      }
    }
  }
}

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

etr2460

first pass, i think i need to go through this again though

etr2460 · 2019-11-08T00:22:22Z

superset/assets/src/dashboard/components/Dashboard.jsx

+        const idSet = new Set(affectedChartIds);
+        this.refreshCharts([...idSet]);


why build a set then make it an array again? just to dedupe? why not make affectedChartIds a Set from the getgo?

yes. convert to Set is to de-dupe. In the following loop, i need to keep adding array of values into affectedChartIds. I am not sure there is an easier way to add an array of values into Set?

etr2460 · 2019-11-08T00:29:21Z

superset/assets/src/dashboard/components/filterscope/FilterScopeSelector.jsx

    );

+    this.props.updateDashboardFiltersScope(allFilterFieldScopes);
+    this.props.setUnsavedChanges(true);


What does passing true here mean? seems like setUnsavedChanges wouldn't require an argument

this is signature ofsetUnsavedChanges function:

export function setUnsavedChanges(hasUnsavedChanges) { return { type: SET_UNSAVED_CHANGES, payload: { hasUnsavedChanges } }; }

i need to notify dashboard state that there is unsaved changes, since dashboardFilters state is updated.

etr2460 · 2019-11-08T00:30:53Z

superset/assets/src/dashboard/containers/Dashboard.jsx

    slices: sliceEntities.slices,
    layout: dashboardLayout.present,
    impressionId,
-    loadStats: getLoadStatsPerTopLevelComponent({


why are we removing this?

this is old dashboard perf logging. this prop loadStats is not used anywhere. getLoadStatsPerTopLevelComponent is kind of expensive and frequently being called.

etr2460 · 2019-11-08T00:39:52Z

superset/views/core.py

+            md.pop("filter_immune_slice_fields", None)
+            md["filter_scopes"] = json.loads(data.get("filter_scopes", "{}"))
+        else:
+            if "filter_immune_slices" not in md:


do we have a migration plan for migrating filters over? when will this happen? will we migrate all the form data even if dashboards don't get visited?

yeah, i think migration from backend is good idea. Users will update/add filter scopes for the active dashboards, but old dashboard will never got updated. I can make a migration once this feature got released and stabled.
For migration, the logic could be very simple: just convert all existed dashboard filters as global scope, and add immune charts if they had immune settings in dashboard metadata.

codecov-io · 2019-11-11T10:20:52Z

Codecov Report

Merging #8522 into feature--dashboard-scoped-filter will decrease coverage by <.01%.
The diff coverage is 69.01%.

@@                         Coverage Diff                         @@
##           feature--dashboard-scoped-filter   #8522      +/-   ##
===================================================================
- Coverage                                66%     66%   -0.01%     
===================================================================
  Files                                   470     474       +4     
  Lines                                 23127   23197      +70     
  Branches                               2485    2484       -1     
===================================================================
+ Hits                                  15265   15311      +46     
- Misses                                 7701    7728      +27     
+ Partials                                161     158       -3

Impacted Files	Coverage Δ
...shboard/util/charts/getFormDataWithExtraFilters.js	`89.47% <ø> (-1.44%)`	⬇️
...et/assets/src/dashboard/components/SliceHeader.jsx	`24% <ø> (ø)`	⬆️
...src/dashboard/components/HeaderActionsDropdown.jsx	`69.56% <ø> (ø)`	⬆️
...uperset/assets/src/dashboard/components/Header.jsx	`43.24% <ø> (ø)`	⬆️
...shboard/components/filterscope/FilterScopeTree.jsx	`50% <ø> (-4.55%)`	⬇️
...ssets/src/dashboard/containers/DashboardHeader.jsx	`100% <ø> (ø)`	⬆️
...sets/src/dashboard/util/getFilterFieldNodesTree.js	`0% <ø> (ø)`	⬆️
...shboard/components/filterscope/FilterFieldTree.jsx	`44.44% <ø> (-5.56%)`	⬇️
...s/src/dashboard/components/SliceHeaderControls.jsx	`11.62% <ø> (ø)`	⬆️
...rset/assets/src/dashboard/components/SaveModal.jsx	`43.9% <ø> (ø)`	⬆️
... and 34 more

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 b1aaa43...5f42413. Read the comment docs.

graceguo-supercat · 2019-11-13T05:20:40Z

A few updates in the last commits, based on users and designer's feedack:

Replace Select/deselect all filters to All filters. Replace Select/deselect all charts to All charts
Click on Save button should update filter scopes setting to redux state, and close modal.
Fix the bug: filter scopes settings are not saved to backend.

superset/assets/src/dashboard/components/FilterIndicatorsContainer.jsx

superset/assets/src/dashboard/components/filterscope/FilterScopeModal.jsx

kristw · 2019-11-13T19:42:17Z

superset/assets/src/dashboard/components/filterscope/FilterScopeTree.jsx

+
+const NOOP = () => {};
+
+const FILTER_SCOPE_CHECKBOX_TREE_ICONS = {


Is this the same with FILTER_FIELD_CHECKBOX_TREE_ICONS?

yes, they were not same at the very first version. now they are same. so i created a shared config.

superset/assets/src/dashboard/reducers/getInitialState.js

superset/assets/src/dashboard/util/activeDashboardFilters.js

superset/assets/src/dashboard/util/getKeyForFilterScopeTree.js

kristw · 2019-11-15T00:45:27Z

superset/assets/src/dashboard/reducers/getInitialState.js

+            ...DASHBOARD_FILTER_SCOPE_GLOBAL,
+            ...scopeSettings[column],
+          };
+          const immuneChartIds = new Set(filterImmuneSlices.slice());


If you pass the array to Set, probably not need to make a copy.

kristw · 2019-11-15T00:48:07Z

superset/assets/src/dashboard/util/charts/getEffectiveExtraFilters.js

-
-  // Build a list of fields the slice is immune to filters on
+export default function getEffectiveExtraFilters(filters) {
  const effectiveFilters = [];


export default function getEffectiveExtraFilters(filters) { return Object.entries(filters).map([column, values]) => ({ col: column, op: 'in', val: values, }); }

kristw

a few minor, non-blocking comments.

* [WIP][dashboard scoped filter] part 1: scope selector modal (#8557) * filter scope selector modal * add single-field-edit in multi-edit mode switch * fix code review comments (round 1) * refactory after design review * fix a few props initial value * [WIP][dashboard scoped filter] part 2: add algorithm to convert checked ids to scope object (#8564) * convert ids to scope object * use lodash helpers to make code readable * [WIP][dashboard scoped filter] part 3: merge filter scope settings into dashboard redux state (#8522) * merge filter scope settings into dashboard redux state * fix/add unit tests * minor bug fixes * fix save filter Scopes behavior * resolve review comments * fix save filter scope settings * minor comments * [dashboard scoped filter] Improve scrollbar inside modal (#8553) * improve scroll inside modal * make left pane and right pane scroll separately * fix review comments * force show filter_box as unchecked (#8587)

pull-request-size bot added the size/XL label Nov 6, 2019

graceguo-supercat mentioned this pull request Nov 6, 2019

[WIP][dashboard scoped filter] part 2: add algorithm to convert checked ids to scope object #8521

Closed

12 tasks

graceguo-supercat force-pushed the scoped-filter-redux-v2 branch from af17619 to c9f3de9 Compare November 7, 2019 07:01

etr2460 reviewed Nov 8, 2019

View reviewed changes

graceguo-supercat force-pushed the scoped-filter-redux-v2 branch from c9f3de9 to 039d819 Compare November 9, 2019 07:17

graceguo-supercat force-pushed the convert-ids-to-tabs-v2 branch 2 times, most recently from 6f0d13b to 03b722e Compare November 11, 2019 08:49

graceguo-supercat force-pushed the scoped-filter-redux-v2 branch 2 times, most recently from 3ee5354 to aa679e8 Compare November 11, 2019 10:07

pull-request-size bot added size/XXL and removed size/XL labels Nov 13, 2019

graceguo-supercat force-pushed the convert-ids-to-tabs-v2 branch 2 times, most recently from eecf04f to 3e83142 Compare November 13, 2019 17:47

kristw reviewed Nov 13, 2019

View reviewed changes

graceguo-supercat force-pushed the scoped-filter-redux-v2 branch from 2bae290 to 015a390 Compare November 13, 2019 21:40

graceguo-supercat changed the base branch from convert-ids-to-tabs-v2 to feature--dashboard-scoped-filter November 13, 2019 22:23

Grace added 4 commits November 13, 2019 14:30

merge filter scope settings into dashboard redux state

d67e404

fix/add unit tests

5025b24

minor bug fixes

edd4b28

fix save filter Scopes behavior

83269e9

graceguo-supercat force-pushed the scoped-filter-redux-v2 branch from 015a390 to 83269e9 Compare November 13, 2019 22:35

Grace added 2 commits November 13, 2019 21:42

resolve review comments

43e1715

fix save filter scope settings

5f42413

graceguo-supercat requested review from etr2460 and kristw November 14, 2019 08:56

kristw reviewed Nov 15, 2019

View reviewed changes

kristw approved these changes Nov 15, 2019

View reviewed changes

minor comments

320bb3c

graceguo-supercat merged commit f869e19 into feature--dashboard-scoped-filter Nov 15, 2019

graceguo-supercat deleted the scoped-filter-redux-v2 branch November 25, 2019 23:31

graceguo-supercat added the dashboard-scoped-filters label Jan 22, 2020

		const idSet = new Set(affectedChartIds);
		this.refreshCharts([...idSet]);


		const NOOP = () => {};

		const FILTER_SCOPE_CHECKBOX_TREE_ICONS = {

Comments

Conversation

graceguo-supercat commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

graceguo-supercat commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

graceguo-supercat commented Nov 6, 2019 •

edited

Loading

graceguo-supercat Nov 11, 2019 •

edited

Loading

graceguo-supercat Nov 11, 2019 •

edited

Loading

graceguo-supercat Nov 11, 2019 •

edited

Loading

codecov-io commented Nov 11, 2019 •

edited

Loading

graceguo-supercat commented Nov 13, 2019 •

edited

Loading