Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(native-filters): apply cascading without instant filtering #14966

Merged
merged 1 commit into from Jun 3, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented Jun 3, 2021

SUMMARY

Currently enabling cascading in native filters requires instantly applying filters, causing all charts in scope to rerender and retrigger queries. This PR changes child filters to apply the currently selected data mask of the parent filter instead of the currently applied one, removing the need to instantly apply filters.

While making the changes, I considered moving dataMaskSelected into Redux instead of adding it as a prop on the chain of components. However, I noticed that many of the intermediate components between the filter tab and FilterValue were already using dataMaskSelected, which made the benefits of moving it to Redux less appealing. In addition, adding it to Redux would have required either

  • introducing a new dataMaskSelected property to complement the dataMask property (which contains the applied data mask) in the state tree

or

  • refactoring dataMask so that it would be split into dataMask.applied and dataMask.selected

Since both of these would have required fairly significant refactoring, it seems better to perform this refactoring later when the full scope of refactoring needs is clear (e.g. will some of this state be moved into Context or be kept in Redux).

BFEORE

Currently each time a parent filter is changed, it triggers the filter automatically:
https://user-images.githubusercontent.com/33317356/120623645-962b6000-c468-11eb-86b5-db95319691f4.mp4

AFTER

Now the parent filter only retriggers the child filters (applying works like before):
https://user-images.githubusercontent.com/33317356/120628894-e2c56a00-c46d-11eb-8200-51d2c73ea10d.mp4

TESTING INSTRUCTIONS

  1. Create a dashboard with cascading filters
  2. Make sure the parent filter is not set to "Apply changes instantly"
  3. Change the selection in the parent filter

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #14966 (bf656b5) into master (f94c6d2) will decrease coverage by 0.00%.
The diff coverage is 70.00%.

❗ Current head bf656b5 differs from pull request most recent head 1ee5360. Consider uploading reports for the commit 1ee5360 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14966      +/-   ##
==========================================
- Coverage   77.62%   77.62%   -0.01%     
==========================================
  Files         965      965              
  Lines       49521    49510      -11     
  Branches     6263     6259       -4     
==========================================
- Hits        38442    38430      -12     
- Misses      10878    10880       +2     
+ Partials      201      200       -1     
Flag Coverage Δ
javascript 72.48% <70.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rBar/CascadeFilters/CascadeFilterControl/index.tsx 92.30% <ø> (ø)
...Filters/FilterBar/FilterControls/FilterControl.tsx 100.00% <ø> (ø)
...ilters/FilterBar/FilterControls/FilterControls.tsx 77.58% <ø> (ø)
...ts/nativeFilters/FilterBar/FilterControls/state.ts 78.57% <40.00%> (-1.43%) ⬇️
.../FilterBar/CascadeFilters/CascadePopover/index.tsx 63.23% <100.00%> (-1.56%) ⬇️
...veFilters/FilterBar/FilterControls/FilterValue.tsx 67.44% <100.00%> (ø)
...board/components/nativeFilters/FilterBar/index.tsx 100.00% <100.00%> (ø)
superset-frontend/src/dataMask/actions.ts 71.42% <0.00%> (-7.15%) ⬇️

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 f94c6d2...1ee5360. Read the comment docs.

@villebro villebro force-pushed the villebro/cascade-non-instant branch from 627b7ae to 1ee5360 Compare June 3, 2021 10:17
@villebro villebro requested a review from simcha90 June 3, 2021 10:29
dataMaskSelected: DataMaskState;
dataMaskSelected: DataMaskStateWithId;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bycatch: incorrect datatype

@michael-s-molina
Copy link
Member

michael-s-molina commented Jun 3, 2021

Two issues:
1 - If I select a parent filter then it filters the child filters. However, if I select the child filters first, and select the parent filter after, the values are inconsistent as shown in the video. Is this a problem?
2 - There's an error happening when trying to access the filter after updating the filters. This error is not happening on master.

20210603085803494_OpHTRWqp.mov

@zhaoyongjie
Copy link
Member

can't remove selected item

can.t.remove.selected.item.mp4

@villebro
Copy link
Member Author

villebro commented Jun 3, 2021

can't remove selected item

Thanks for flagging this! It seems to be happening on master, too, so isn't directly related to this change.

@villebro
Copy link
Member Author

villebro commented Jun 3, 2021

Two issues:
1 - If I select a parent filter then it filters the child filters. However, if I select the child filters first, and select the parent filter after, the values are inconsistent as shown in the video. Is this a problem?
2 - There's an error happening when trying to access the filter after updating the filters. This error is not happening on master.

  1. I don't think this is a problem, as the value filter supports creating values that don't exist in the query result. But we can potentially consider resetting child values that aren't present when updating the parent. Let's discuss this later.
  2. I was able to reproduce this problem on master. Therefore I propose merging this now and following up with a fix for this bug and the bug @zhaoyongjie pointed out (also present on master)

@villebro villebro merged commit 765e9dd into apache:master Jun 3, 2021
@villebro villebro deleted the villebro/cascade-non-instant branch June 3, 2021 14:49
@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jun 4, 2021
amitmiran137 added a commit to nielsen-oss/superset that referenced this pull request Jun 6, 2021
* upstream/master: (68 commits)
  fix typos (apache#14950)
  docs: fix custom oauth config (apache#14997)
  fix: apply template_params on external_metadata (apache#14996)
  fix: toggle fullscreen on the dashboard (apache#14979)
  feat(native-filters): add markers and number formatter + simple tests (apache#14981)
  fix(native-filters): Fix "undefined" error after editing a filter (apache#14984)
  fix(native-filters): remove implied fetch predicate (apache#14982)
  fix(native-filters): update cascaded filter state on change (apache#14980)
  fix(filter box): replace freeform where clause with ilike (apache#14900)
  refactor: Convert TableElement.jsx component from class to functional with hooks (apache#14830)
  fix: renamed sqllab filters to _filters (apache#14971)
  feat(native-filters): apply cascading without instant filtering (apache#14966)
  chore: bump superset-ui to 0.17.53 (apache#14968)
  fix(native-filters): cascading filters not rendering in tab (apache#14964)
  feat: add type_generic and is_dttm to table metadata (apache#14863)
  additional safeguard (apache#14953)
  feat: Adding FORCE_SSL as feature flag in config.py (apache#14934)
  feat(dashboard/native-filters): Hide filters out of scope of current tab (apache#14933)
  fix: time parser truncate to first day of year/month (apache#14945)
  fix: is_temporal should overwrite is_dttm (apache#14894)
  ...
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels dashboard:native-filters Related to the native filters of the Dashboard preset-io size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants