[WIP][dashboard scoped filter] part 1: scope selector modal by graceguo-supercat · Pull Request #8404 · apache/superset

graceguo-supercat · 2019-10-17T00:26:16Z

SUMMARY

This is part 1 for Dashboard Scoped Filter project. This PR is for a new UI module: filter scope selector modal. It covers the UI/UX that display/select scope (tabs and charts) for a filter field. Update dashboard redux state and persist filter scope settings to backend, will be covered in another PR.

Superset dashboard stores layout data in a tree structure. This layout structure always contains a root component. A root component will have a grid component (for dashboard without tabs) or a root-level tabs components as its children. In the dashboard, every UI component (tabs, row, column, and chart, etc) is a tree node. For example:

We can think A is root component, leaves(K,L, F, G, M, I, J) are chart components, and the rest can be container (like row, column or tabs) components.

With the growing size of the dashboard, user may want to apply one dashboard filter to some of charts in dashboard, and one filter apply to another set of charts. A very common use case is user groups similar charts in different tabs, and make a filter only applicable for this group. Currently the way to set filter's scope is not very easy to use:

This PR introduced a new UI component: filter scope selector.

left panel: All filter_box in dashboard and their filter fields. There is also a toggle link to switch between single-edit mode or multi-edit mode.
right panel: show dashboard layout in the tree structure. But we hide row/column container, and group charts into its tab parent. If user have nested tabs containers, we will show all the nested tabs structure from chart to dashboard root component. We show a checkbox for each of chart and tab, user can click to select or deselect single or multiple charts.

When user clicks the Save button at the bottom of the modal, we will aggregate the checked chart ids into scope object and update dashboard filters state. The algorithm for this aggregation will be covered in another PR.

Features:

Start filter scope selector:
User can edit a single filter, or edit multiple filter fields all together. We also allow user toggle back to a single filter field in the multi-edit mode:
Search chart name:
When new chart is added to dashboard, based on its layout position, it will get filters that are applicable to its tab(s):
When user move charts between tabs, this may cause chart to be applicable to different filter(s). We will show a warning message when chart is moved to a different filter scope.

Implementation

Filter scope selector has following UI components:

Container component: src/dashboard/containers/FilterScope.jsx
modal trigger: src/dashboard/components/filterscope/FilterScopeModal.jsx
core function UI component: src/dashboard/components/filterscope/FilterScopeSelector.jsx
checkbox tree component in left pane: FilterFieldTree.jsx
recursive traversal renderer for each filter field tree node: renderFilterFieldTreeNodes.jsx
simple UI component for filter filed item: FilterFieldItem.jsx
checkbox tree component in right pane: FilterScopeTree.jsx
recursive traversal renderer for each filter scope tree node: renderFilterScopeTreeNodes.jsx
util functions:

getFilterFieldNodesTree: build filter field tree from dashboard filters redux state.
getFilterScopeNodesTree: build filter scope tree from dashboard layout.

Tree and tree nodes

We used a 3rd party component to render left panel filter field selector and right panel scope selector. It best matched our layout data structure, and offers very natural and convenient user experience like single selection, group selection, expand-on-click, etc.

According to this API, aCheckboxTree component needs following props:

// the original data to be rendered in `CheckboxTree`. The data should be in tree structure. Since js doesn't have build-in Tree data structure, we use array of node object.
// for filter scope tree, it is full list of the dashboard layout (only have tabs and chart components), 
// for filter fields tree, it is all dashboard filter box and its fields.
nodes: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired,

// when user typed keywords in search box, right pane will only show matched charts and all their parent tabs. Only used by filter scope tree component.
nodesFiltered: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired,

// An array of  node values. `CheckboxTree` component will show these node as checked.
checked: PropTypes.arrayOf(
    PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
  ).isRequired, 

// An array of node values. `CheckboxTree` component will show these node as expanded tree nodes. 
expanded: PropTypes.arrayOf(
    PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
  ).isRequired

For filter field tree, the tree node data is:

{
  value: filter key.
  label: customized column label or column name for the chosen filter field.
  children: array of children node
}

For filter scope tree, the tree node data is:

{
  value: chart id or tab component id
  label: chart name or tab title
  children: array of children node
}

Local state in the filter scope selector modal

For each filter filed, we construct a unique filter key which is composed of filter_box's chart id and filter's column name: chartId_columnName

FilterScopeSelector component uses following data to maintain the current state in the modal:

showSelector: as long as dashboard has one or more filter, we show filter scope selector UI. otherwise, show message says no filter in the dashboard.
filterFieldNodes: filter fields tree object.
filterScopeMap: a map for filter scope selections. in each entry, key is filter key, value is filter scope tree object for a filter field.
checkedFilterFields: array of all chosen filter key(s) in the left pane.

Based on the chosen filter keys from left pane, we show the filter scope tree in the right pane, which memorize the expanded tabs and selected charts. If all charts under the tab are selected, tab will show checked state in its checkbox. If some of the charts under the tab are selected, tab will show half checked state. And if none of the charts are selected, tab will show a empty state for its checkbox.

TEST PLAN

Manual test.

ADDITIONAL INFORMATION

REVIEWERS

@etr2460 @williaster @mistercrunch @kenchendesign

* [docs] New, document need for PG to use autocommit for CTAS

@villebro

* [ci] Deprecate flake8 * Addressing @villebro's comments

@dpgaspar

…ompatibility. (#8353) * removing deprecated function for future Python release compatibility. * also removed assertNotEquals per @dpgaspar

etr2460

first pass!

etr2460 · 2019-10-21T21:28:24Z

superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js


-describe('dashboardFilters reducer', () => {
+// disable broken unit tests by now, will fix it in another PR
+xdescribe('dashboardFilters reducer', () => {


can we fix the unit tests in this pr since the behavior is being changed (and hopefully the unit tests confirm the proper change)?

fixed. this is dashboardFilters reducer unit test. I have a lot more changes for dashboardFilters reducer in next PR, to merge filter scope logic into redux state.
I want to keep minimum change for redux change in this PR to avoid merge conflicts.

superset/assets/src/components/CheckboxChecked.jsx

superset/assets/src/components/CheckboxHalfchecked.jsx

etr2460 · 2019-10-21T21:31:39Z

superset/assets/src/components/CheckboxUnchecked.jsx

+ */
+import React from 'react';
+
+export default function CheckboxUnchecked() {


same comment from CheckboxChecked

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

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

etr2460 · 2019-10-21T23:57:28Z

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

+  dashboardFilters = {},
+  isSingleEditMode = true,
+}) {
+  if (Object.keys(dashboardFilters).length === 0) {


this is unneeded, Object.values({}).map(...) returns []

cool. fixed.

etr2460 · 2019-10-22T01:08:44Z

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

+    let children = [];
+    if (currentNode.children && currentNode.children.length) {
+      currentNode.children.forEach(child => {
+        const cNode = traverse(components[child]);


what's a cNode? full variable names please

etr2460 · 2019-10-22T01:09:21Z

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

+    }
+  };
+
+  if (nodes && nodes.length) {


prefer nodes.length > 0

Also fix isort issue on master

kristw

I wonder if in the future we can break a PR like this into smaller chunks? It is difficult to review 1600 lines of code.
Perhaps small PRs for the individual UI components and util functions, before another PR for the integration.

kristw · 2019-10-23T00:07:33Z

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

+
+    this.modal = null;
+    this.close = this.close.bind(this);
+    this.setModalRef = this.setModalRef.bind(this);


can use React.createRef() instead of manually setting ref

kristw · 2019-10-23T00:08:18Z

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

+      expanded={expanded}
+      onCheck={onCheck}
+      onExpand={onExpand}
+      onClick={() => {}}


same with icons

williaster

left some comments, will do another pass later

williaster · 2019-10-22T01:24:23Z

superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js

        componentId: component.id,
        directPathToFilter,
-        scope: DASHBOARD_ROOT_ID,
+        scope: 'ROOT_ID',


why no constant anymore?

fixed the unit test.

superset/assets/src/components/CheckboxHalfchecked.jsx

williaster · 2019-10-22T01:25:48Z

superset/assets/src/components/CheckboxUnchecked.jsx

+ */
+import React from 'react';
+
+export default function CheckboxUnchecked() {


superset/assets/src/components/CheckboxChecked.jsx

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

williaster · 2019-10-23T02:15:46Z

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

+  return `${chartId}_${column}`;
+}
+
+export function getDashboardFilterByKey(key) {


I don't understand why this function is called getDashboardFilterByKey, maybe getChartIdAndColumnFromFilterId?

renamed to getChartIdAndColumnFromFilterKey

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

williaster · 2019-10-23T02:18:04Z

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

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export function getDashboardFilterKey(chartId, column) {


can you change this to named arguments for better type safety + readability? (you use named arguments in many other places)

fixed as much as i found.

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

williaster · 2019-10-23T02:19:19Z

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

+
+const FILTER_SCOPE_CONTAINER_TYPES = [TAB_TYPE, DASHBOARD_ROOT_TYPE];
+
+export default function getFilterScopeNodesTree({


can you please add a description for what this function does?

bumping this comment!

please see the Implementation section. This tree structure is used by react-checkbox-tree framework.

codecov-io · 2019-10-23T05:14:54Z

Codecov Report

Merging #8404 into feature--dashboard-scoped-filter will decrease coverage by 1.69%.
The diff coverage is 53.41%.

@@                         Coverage Diff                         @@
##           feature--dashboard-scoped-filter    #8404     +/-   ##
===================================================================
- Coverage                             67.65%   65.95%   -1.7%     
===================================================================
  Files                                   448      469     +21     
  Lines                                 22498    23091    +593     
  Branches                               2364     2468    +104     
===================================================================
+ Hits                                  15220    15230     +10     
- Misses                                 7140     7700    +560     
- Partials                                138      161     +23

Impacted Files	Coverage Δ
...erset/assets/src/SqlLab/components/QuerySearch.jsx	`58.65% <ø> (ø)`	⬆️
...uperset/assets/src/SqlLab/components/SouthPane.jsx	`91.42% <ø> (ø)`	⬆️
superset/examples/birth_names.py	`100% <ø> (ø)`	⬆️
...rset/assets/src/SqlLab/components/QueryHistory.jsx	`83.33% <ø> (ø)`	⬆️
superset/assets/src/components/ModalTrigger.jsx	`90% <ø> (ø)`	⬆️
superset/examples/sf_population_polygons.py	`29.16% <ø> (ø)`	⬆️
superset/examples/unicode_test_data.py	`100% <ø> (ø)`	⬆️
superset/examples/deck.py	`11.11% <ø> (ø)`	⬆️
...rc/explore/components/controls/CheckboxControl.jsx	`85.71% <ø> (+14.28%)`	⬆️
superset/examples/tabbed_dashboard.py	`22.22% <ø> (ø)`	⬆️
... and 159 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 c422b49...4456fa3. Read the comment docs.

* Separate RC from VERSION * Fix pypi_push.sh and refine instructions * Add SUPERSET_ prefix to env variables * Finalize release instructions * Change one-off to First Time Only * Add tagging of final version * Convert some remaining env variables and add a check step to pypi deploy

see https://docs.celeryproject.org/en/latest/userguide/optimizing.html

* added user tutorial and rearranged docs hierarchy * added license header, renamed admin tutorial file * fixed image url issue * Fix spelling * Tweaks to text based on feedback * guilabel and menu test * added guilabel and upload csv section * tidy up of rst formatting

* [cli] Fix, import datasources exported by UI

* Update CONTRIBUTING.md * Update CONTRIBUTING.md

* Allow customization of documentation icon and text * Set icon width to 100% * Use double quotes for strings

* Add support for database engine SAP Hana * Support hana services Increase time, minute, and second * Fix hana return string * Fix formatting errors

* [docs] Update CHANGELOG with 0.35.0 * [docs] Fix, github md problem

* passing onClick prop to header with the existing onChange method. * basic test checking that label click fires the onChange method. * cleaning up stuff caught by linting.

* Encode feature flags to JSON pessimistically * Add unit test * Remove old imports

willbarrett · 2019-11-13T15:55:42Z

Hey @graceguo-supercat - this PR is getting impossible to review due to the size of the delta. Should we close this one and open something fresh that's rebased off of latest master?

graceguo-supercat · 2019-11-13T17:38:05Z

Please comment in #8553. Please note I plan to work on feature branch feature--dashboard-scoped-filter, then merge whole feature branch to master.

graceguo-supercat · 2019-11-13T18:14:41Z

@willbarrett @etr2460 review it for a long time, he prefer to use this old PR.

…-selector-modal

filter scope selector modal

1609403

pull-request-size bot added the size/XXL label Oct 17, 2019

graceguo-supercat mentioned this pull request Oct 17, 2019

[WIP][feature] Dashboard filter scope selector modal #8403

Closed

12 tasks

graceguo-supercat changed the title ~~filter scope selector modal~~ [WIP][feature]filter scope selector modal Oct 17, 2019

graceguo-supercat changed the title ~~[WIP][feature]filter scope selector modal~~ [WIP][feature]Dashboard filter scope selector modal Oct 17, 2019

graceguo-supercat requested review from etr2460, khtruong and williaster October 17, 2019 00:27

dpgaspar and others added 7 commits October 17, 2019 14:54

[docs] CTAS on PostgreSQL needs commit to apply (#8367)

c62b2f4

* [docs] New, document need for PG to use autocommit for CTAS

[Config] Cache static resources (#8370)

d3406e0

[sqla] Enforcing ISO 8601 date/timestamp formats (#7702)

a199901

[ci] Deprecate flake8 (#8409)

9fc37ea

* [ci] Deprecate flake8 * Addressing @villebro's comments

fixed typo in installation instructions (#8413)

1e533db

[tests] Fix, removing deprecated function for future Python release c…

4cfd6b3

…ompatibility. (#8353) * removing deprecated function for future Python release compatibility. * also removed assertNotEquals per @dpgaspar

Add improved typed casting to BigQuery (#8331)

cca689b

etr2460 requested changes Oct 22, 2019

View reviewed changes

mistercrunch and others added 2 commits October 22, 2019 11:50

chore: disable another flaky cypress test (#8423)

8f246ce

Also fix isort issue on master

[pydruid] Bumping the pydruid version (#8430)

b91205d

kristw reviewed Oct 23, 2019

View reviewed changes

williaster reviewed Oct 23, 2019

View reviewed changes

mistercrunch and others added 4 commits October 22, 2019 21:42

docs: add a note to RELEASING about Slack Channel (#8419)

a0db22b

docs: Add an 'Orientation' section to CONTRIBUTING.md (#8422)

cde17a8

convert ids to scope object

9d2a391

remove changes that should be another PR

970e636

villebro and others added 5 commits October 23, 2019 16:43

[fix] Updating parse_human_timedelta typing (#8436)

786d770

spelling fix (#8442)

19d2ea2

explain the need to enable async queries (#8444)

a39c24e

introduce a space in command line option (#8438)

1d5718a

see https://docs.celeryproject.org/en/latest/userguide/optimizing.html

francishemingway and others added 12 commits November 12, 2019 22:00

[cli] Fix, import datasources exported by UI (#8477)

18046c2

* [cli] Fix, import datasources exported by UI

[docs] Updating slice parameter documentation. (#8535)

249947a

* Update CONTRIBUTING.md * Update CONTRIBUTING.md

fab 2.2.0 (#8517)

630360f

Allow customization of documentation icon and text (#8507)

5091270

* Allow customization of documentation icon and text * Set icon width to 100% * Use double quotes for strings

Add support for database engine SAP Hana (#8411)

a548c63

* Add support for database engine SAP Hana * Support hana services Increase time, minute, and second * Fix hana return string * Fix formatting errors

[docs] 0.35.0 CHANGELOG (#8547)

085ccbc

* [docs] Update CHANGELOG with 0.35.0 * [docs] Fix, github md problem

Upgrade superset-ui/time-format (#8541)

b9f5678

Clickable checkbox labels (#8534)

1fd36bb

* passing onClick prop to header with the existing onChange method. * basic test checking that label click fires the onChange method. * cleaning up stuff caught by linting.

Encode feature flags to JSON pessimistically (#8529)

9a29367

* Encode feature flags to JSON pessimistically * Add unit test * Remove old imports

[fix] Re-cleanup legacy filters (#8523)

4456fa3

Add convert_dttm method to SnowflakeEngineSpec (#8551)

90275fe

graceguo-supercat mentioned this pull request Nov 13, 2019

[WIP][dashboard scoped filter] part 1: scope selector modal #8557

Merged

graceguo-supercat closed this Nov 13, 2019

graceguo-supercat reopened this Nov 13, 2019

Grace added 8 commits November 13, 2019 10:55

filter scope selector modal

051d311

convert ids to scope object

29adb4c

remove changes that should be another PR

dd5c47f

add single-field-edit in multi-edit mode switch

4bcc579

fix code review comments (round 1)

b0549a2

refactory after design review

7f41641

fix a few props initial value

292570e

Merge remote-tracking branch 'origin/scope-selector-modal' into scope…

d3bc346

…-selector-modal

graceguo-supercat closed this Nov 13, 2019

graceguo-supercat deleted the scope-selector-modal branch November 25, 2019 23:31

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

graceguo-supercat mentioned this pull request Apr 22, 2020

fix: re-enable table filter #9593

Closed

3 tasks


		const FILTER_SCOPE_CONTAINER_TYPES = [TAB_TYPE, DASHBOARD_ROOT_TYPE];

		export default function getFilterScopeNodesTree({

Comments

Conversation

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

Uh oh!

CATEGORY

SUMMARY

Features:

Implementation

Tree and tree nodes

Local state in the filter scope selector modal

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 Oct 29, 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!

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!

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!

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!

kristw left a comment

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!

Choose a reason for hiding this comment

Uh oh!

williaster left a comment

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!

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

graceguo-supercat commented Oct 17, 2019 •

edited

Loading

graceguo-supercat Oct 29, 2019 •

edited

Loading

codecov-io commented Oct 23, 2019 •

edited

Loading

graceguo-supercat commented Nov 13, 2019 •

edited

Loading