chore: Preserve native filters selection after refresh #15583

michael-s-molina · 2021-07-08T12:51:58Z

SUMMARY

Preserve native filters selection after refresh.

The user selection is stored in the URL
User selections have higher priority over the default values
Fixes [native-filters]filter scoping select don't stick after refresh #15535

@jinghua-qa @junlincc @rosemarie-chiu @rusackas

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

screen-recording-2021-07-08-at-74912-am_jwOvpeQb.mp4

TESTING INSTRUCTIONS

Add some native filters with different types and default values
Change the filter values in the filter bar
Refresh
The user selection should be preserved

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 · 2021-07-08T13:14:18Z

Codecov Report

Merging #15583 (b0da1cc) into master (f0c0d74) will increase coverage by 0.02%.
The diff coverage is 80.85%.

❗ Current head b0da1cc differs from pull request most recent head b654aee. Consider uploading reports for the commit b654aee to get more accurate results

@@            Coverage Diff             @@
##           master   #15583      +/-   ##
==========================================
+ Coverage   76.99%   77.02%   +0.02%     
==========================================
  Files         978      979       +1     
  Lines       51493    51506      +13     
  Branches     6951     6946       -5     
==========================================
+ Hits        39649    39671      +22     
+ Misses      11620    11611       -9     
  Partials      224      224

Flag	Coverage Δ
javascript	`71.57% <80.85%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
...s/components/TimeColumn/TimeColumnFilterPlugin.tsx	`0.00% <0.00%> (ø)`
...ers/components/TimeGrain/TimeGrainFilterPlugin.tsx	`0.00% <0.00%> (ø)`
...board/components/nativeFilters/FilterBar/index.tsx	`86.32% <81.48%> (+3.14%)`	⬆️
...omponents/nativeFilters/FilterBar/Header/index.tsx	`100.00% <100.00%> (+10.25%)`	⬆️
...set-frontend/src/dashboard/util/getDashboardUrl.ts	`100.00% <100.00%> (ø)`
...ntend/src/dashboard/util/replaceUndefinedByNull.ts	`100.00% <100.00%> (ø)`
superset-frontend/src/dataMask/reducer.ts	`67.30% <100.00%> (+2.00%)`	⬆️
...et-frontend/src/filters/components/Select/index.ts	`90.00% <0.00%> (-10.00%)`	⬇️
... and 1 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 f0c0d74...b654aee. Read the comment docs.

kgabryje · 2021-07-12T09:50:58Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

github-actions · 2021-07-12T09:52:54Z

@kgabryje Ephemeral environment spinning up at http://52.27.157.190:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2021-07-12T10:11:52Z

superset-frontend/src/dashboard/util/replaceUndefinedByNull.ts

+ */
+import { cloneDeep } from 'lodash';
+
+export default function replaceUndefinedByNull(object: Object) {


We run cloneDeep on every level of recursion - I think doing it once, in the first level of recursion, would be enough. However, I'd like to propose a different solution:

const replacer = (key, val) => { if (val === undefined) return null; return val; } const replaceUndefinedByNull = (object) => JSON.parse(JSON.stringify(object, replacer));

I'm not sure if it would be faster than your solution (we could time it to verify), but I think it's easier to understand. WDYT?

I totally agree with the unnecessary cloneDeep. Will fix that.

About the JSON serialization/deserialization I prefer the cloneDeep because it's faster. We already have benchmarks on this topic:
https://stackoverflow.com/questions/122102/what-is-the-most-efficient-way-to-deep-clone-an-object-in-javascript
https://jsben.ch/bWfk9

kgabryje · 2021-07-12T10:13:50Z

superset-frontend/src/dataMask/reducer.ts

@@ -128,6 +131,7 @@ const dataMaskReducer = produce(
            [],
          cleanState,
          draft,
+          undefined,


Why do we need pass undefined here explicitely?

This was a result of a previous refactor where I had an additional parameter but ended up reverting. Will remove it. Nice catch!

kgabryje · 2021-07-12T10:27:44Z

2 issues that I noticed:

When we open a dashboard with native filter preselected in the URL, the chart requests are done twice - first time without the native filter and 2nd time with the native filter.
When we open a dashboard with native filter preselected in the URL, any change to the native filter is applied instantly instead of waiting until user clicks apply

Screen.Recording.2021-07-12.at.12.21.20.mov

michael-s-molina · 2021-07-12T19:22:39Z

@kgabryje Fixed the issues.

michael-s-molina · 2021-07-12T19:55:34Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

github-actions · 2021-07-12T19:59:54Z

@michael-s-molina Ephemeral environment spinning up at http://34.215.175.94:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

michael-s-molina · 2021-07-13T13:38:38Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

github-actions · 2021-07-13T13:46:23Z

@michael-s-molina Ephemeral environment spinning up at http://34.214.105.118:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

suddjian

Approved with non-blocking test request, thanks!

suddjian · 2021-07-13T17:48:51Z

superset-frontend/src/dashboard/util/replaceUndefinedByNull.ts

+  return result;
+}
+
+export default function replaceUndefinedByNull(object: Object) {


Tests for this would be nice

Yes! Will do it in a follow-up PR. Thanks for the review!

github-actions · 2021-07-13T18:14:41Z

Ephemeral environment shutdown and build artifacts deleted.

superset-github-bot bot added the Turing label Jul 8, 2021

pull-request-size bot added the size/L label Jul 8, 2021

michael-s-molina requested review from villebro, kgabryje and rusackas July 8, 2021 12:52

michael-s-molina marked this pull request as draft July 8, 2021 18:57

amitmiran137 added the v1.3 label Jul 9, 2021

michael-s-molina force-pushed the preserve-filters-on-refresh branch from ee923db to 68ace90 Compare July 9, 2021 15:01

michael-s-molina marked this pull request as ready for review July 9, 2021 15:02

michael-s-molina force-pushed the preserve-filters-on-refresh branch from 68ace90 to e2575fe Compare July 9, 2021 16:04

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jul 9, 2021

kgabryje reviewed Jul 12, 2021

View reviewed changes

michael-s-molina added this to In progress in Native dashboard filters via automation Jul 12, 2021

michael-s-molina force-pushed the preserve-filters-on-refresh branch from e2575fe to 9a7650a Compare July 12, 2021 19:16

michael-s-molina requested a review from kgabryje July 12, 2021 19:23

chore: Preserve native filters selection after refresh

b654aee

michael-s-molina force-pushed the preserve-filters-on-refresh branch from 9a7650a to b654aee Compare July 13, 2021 12:34

rusackas requested a review from suddjian July 13, 2021 17:26

suddjian approved these changes Jul 13, 2021

View reviewed changes

michael-s-molina merged commit e6bbca3 into apache:master Jul 13, 2021

Native dashboard filters automation moved this from In progress to Done Jul 13, 2021

michael-s-molina mentioned this pull request Jul 14, 2021

chore: Removes unnecessary uses of preselectNativeFilters #15688

Merged

8 tasks

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021

chore: Preserve native filters selection after refresh (apache#15583)

9e4b730

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

chore: Preserve native filters selection after refresh (apache#15583)

530712a

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

chore: Preserve native filters selection after refresh (apache#15583)

169274b

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Preserve native filters selection after refresh #15583

chore: Preserve native filters selection after refresh #15583

michael-s-molina commented Jul 8, 2021

codecov bot commented Jul 8, 2021 •

edited

Loading

kgabryje commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

kgabryje Jul 12, 2021

michael-s-molina Jul 12, 2021 •

edited

Loading

kgabryje Jul 12, 2021 •

edited

Loading

michael-s-molina Jul 12, 2021

kgabryje commented Jul 12, 2021

michael-s-molina commented Jul 12, 2021

michael-s-molina commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

michael-s-molina commented Jul 13, 2021

github-actions bot commented Jul 13, 2021

suddjian left a comment

suddjian Jul 13, 2021 •

edited

Loading

michael-s-molina Jul 13, 2021

github-actions bot commented Jul 13, 2021

chore: Preserve native filters selection after refresh #15583

chore: Preserve native filters selection after refresh #15583

Conversation

michael-s-molina commented Jul 8, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

kgabryje commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

kgabryje Jul 12, 2021

Choose a reason for hiding this comment

michael-s-molina Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

kgabryje Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jul 12, 2021

Choose a reason for hiding this comment

kgabryje commented Jul 12, 2021

michael-s-molina commented Jul 12, 2021

michael-s-molina commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

michael-s-molina commented Jul 13, 2021

github-actions bot commented Jul 13, 2021

suddjian left a comment

Choose a reason for hiding this comment

suddjian Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jul 13, 2021

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2021

codecov bot commented Jul 8, 2021 •

edited

Loading

michael-s-molina Jul 12, 2021 •

edited

Loading

kgabryje Jul 12, 2021 •

edited

Loading

suddjian Jul 13, 2021 •

edited

Loading