feat(native-filters): Set default scope by filters' and charts' datasets #15302

kgabryje · 2021-06-22T18:02:55Z

SUMMARY

When new native filter was created, "Apply to all panels" scope was always selected by default. In the case of dashboards that have charts built on multiple datasets (e.g. Slack Dashboard from test data), filter that uses given dataset usually is incompatible and does not have any effect on charts that use a different dataset. This PR implements "smarter" selection of default scope - when user created a new filter, charts that use different dataset than the filter will be unselected. Changing dataset updates the default scoping tree, but only if the user didn't make any changes there - if the user changes selection in the scoping tree and then changes dataset, we don't update the tree automatically.
The feature takes effect only on new filters. Once a filter is saved, we don't apply default selection to the scopong tree upon editing the filter and its dataset.
Next to "incompatible" charts I added an icon with a tooltip that explains why those charts were not selected by default. I used Info icon for that purpose. Other icons that I considered were Alert and Warning , but since potentially a lot of those icons can appear next to each other, I didn't want to use something too "aggressive". I'd greatly appreciate design input/approval! CC @mihir174.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-06-22.at.19.34.14.mov

TESTING INSTRUCTIONS

Enable DASHBOARD_NATIVE_FILTERS feature flag
Create a dashboard with charts that use the same dataset.
Create a native filter and verify that "Apply to all panels" scope is selected by default
Create a dashboard with charts that use multiple datasets
Create a native filter, choose a dataset and verify that only the charts that use that dataset are in scope by default.
Change dataset, verify that scope tree got updated so that charts that use the new dataset are now in scope.
Modify the scope, change dataset and verify that the scope didn't change.
Save the filter, edit it, change its dataset, verify that the scoe didn't change.

ADDITIONAL INFORMATION

Has associated issue: closes [native-filters]upscope chart if the filter uses a different dataset? #14977
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

CC: @villebro @michael-s-molina @rusackas

villebro

LGTM - a few minor readability improvement ideas. Also, should we reword this, especially if it will end up applying to charts referencing a different dataset, in which case the info icon could be helpful (maybe even stating how many charts are refefrencing a different dataset)?

villebro · 2021-06-23T10:24:15Z

...rd/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx

-  const initialScope = scope || getDefaultScopeValue(chartId);
-  const initialScoping = isScopingAll(initialScope, chartId)
-    ? Scoping.all
-    : Scoping.specific;
+  const [initialScope] = useState(
+    scope || getDefaultScopeValue(chartId, initiallyExcludedCharts),
+  );
+  const [initialScoping] = useState(
+    isScopingAll(initialScope, chartId) ? Scoping.all : Scoping.specific,
+  );


This isn't directly related to this PR, but it confused me while reading this code: having two props called initialScope and initialScoping right next to each other feels very ambiguous. If changing this to something slightly more specific doesn't lead down a huge rabbit hole, I'd opt for renaming Scoping to ScopingType and consequently initialScoping to initialScopingType, perhaps even initialScope to initialChartScope.

Totally agree, I feel like I spend more time trying to wrap my head around difference between those 2 than actual developing. I'll change that in this PR

villebro · 2021-06-23T10:27:42Z

...ashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

@@ -580,6 +592,28 @@ const FiltersConfigForm = (
    setActiveFilterPanelKey(activeFilterPanelKey);
  }, [hasCheckedAdvancedControl]);

+  const initiallyExcluded = useMemo(() => {


nit: initiallyExcludedCharts

villebro

Very nice! Extra points for the useComponentDidUpdate hook :D

kgabryje · 2021-06-23T13:03:02Z

@graceguo-supercat Would you like to verify that this PR works properly on Airbnb environments before it's merged?

codecov · 2021-06-23T13:20:46Z

Codecov Report

Merging #15302 (cd6232a) into master (cef3dc0) will decrease coverage by 0.01%.
The diff coverage is 87.17%.

@@            Coverage Diff             @@
##           master   #15302      +/-   ##
==========================================
- Coverage   77.23%   77.22%   -0.02%     
==========================================
  Files         973      975       +2     
  Lines       50438    50525      +87     
  Branches     6175     6201      +26     
==========================================
+ Hits        38957    39017      +60     
- Misses      11276    11301      +25     
- Partials      205      207       +2

Flag	Coverage Δ
javascript	`71.79% <87.17%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...nd/src/common/hooks/useComponentDidUpdate/index.ts	`0.00% <0.00%> (ø)`
...ilterScopingModal/CrossFilterScopingForm/index.tsx	`84.61% <ø> (ø)`
...ConfigModal/FiltersConfigForm/FilterScope/state.ts	`95.65% <66.66%> (-4.35%)`	⬇️
...odal/FiltersConfigForm/FilterScope/ScopingTree.tsx	`90.62% <80.00%> (-5.03%)`	⬇️
...ConfigModal/FiltersConfigForm/FilterScope/utils.ts	`95.89% <81.81%> (-2.62%)`	⬇️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`69.97% <90.00%> (-0.03%)`	⬇️
...odal/FiltersConfigForm/FilterScope/FilterScope.tsx	`88.88% <92.00%> (+2.68%)`	⬆️
...oks/useComponentDidUpdate/useComponentDidUpdate.ts	`100.00% <100.00%> (ø)`
...ConfigModal/FiltersConfigForm/FilterScope/types.ts	`100.00% <100.00%> (ø)`
...veFilters/FilterBar/FilterControls/FilterValue.tsx	`66.99% <0.00%> (-4.15%)`	⬇️
... and 11 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 cef3dc0...cd6232a. Read the comment docs.

michael-s-molina · 2021-06-23T16:50:32Z

...mponents/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx

@@ -115,6 +115,7 @@ describe('FilterScope', () => {
    fireEvent.click(screen.getByText('Scoping'));
    fireEvent.click(screen.getByLabelText('Apply to specific panels'));

+    screen.debug(undefined, 30000);


Suggested change

screen.debug(undefined, 30000);

michael-s-molina · 2021-06-23T16:59:03Z

superset-frontend/src/common/hooks/useComponentDidUpdate/useComponentDidUpdate.ts

+    }
+  }, [effect]);
+
+  useEffect(() => {


Do we need this second useEffect? https://stackoverflow.com/questions/53179075/with-useeffect-how-can-i-skip-applying-an-effect-upon-the-initial-render

Good point, thanks!

…ets (apache#15302) * feat(native-filters): Set default scope by filter's and charts datasets * Fix undefined error * Use JSON.stringify in dependency array * Fix lint issue * Lock scope after switching radio buttons * Fix weird eslint issue * Change prop names * Implement useComponentDidUpdate * Fix lint * Refactor useComponentDidUpdate * Remove screen.debug()

kgabryje added 5 commits June 22, 2021 19:18

feat(native-filters): Set default scope by filter's and charts datasets

7a74d29

Fix undefined error

123f80f

Use JSON.stringify in dependency array

e357130

Fix lint issue

ff04af9

Lock scope after switching radio buttons

39508e2

kgabryje requested review from rusackas and villebro June 22, 2021 18:02

pull-request-size bot added the size/L label Jun 22, 2021

Fix weird eslint issue

c95a4ba

villebro reviewed Jun 23, 2021

View reviewed changes

kgabryje added 2 commits June 23, 2021 12:58

Change prop names

6400b5c

Implement useComponentDidUpdate

3839dda

villebro approved these changes Jun 23, 2021

View reviewed changes

Fix lint

54121df

michael-s-molina reviewed Jun 23, 2021

View reviewed changes

kgabryje added 2 commits June 23, 2021 19:12

Refactor useComponentDidUpdate

3c4ce80

Remove screen.debug()

cd6232a

michael-s-molina approved these changes Jun 23, 2021

View reviewed changes

kgabryje merged commit f0b6419 into apache:master Jun 24, 2021

kgabryje mentioned this pull request Jul 12, 2021

feat(native-filters): Filter charts by dataSet #15630

Closed

8 tasks

junlincc added dashboard:native-filters Related to the native filters of the Dashboard need:qa-review Requires QA review labels Jul 13, 2021

junlincc added this to In progress in Native dashboard filters via automation Jul 13, 2021

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

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 12, 2024

mistercrunch added the 🚢 1.3.0 label Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Set default scope by filters' and charts' datasets #15302

feat(native-filters): Set default scope by filters' and charts' datasets #15302

kgabryje commented Jun 22, 2021

villebro left a comment

villebro Jun 23, 2021

kgabryje Jun 23, 2021

villebro Jun 23, 2021

villebro left a comment

kgabryje commented Jun 23, 2021

codecov bot commented Jun 23, 2021 •

edited

Loading

michael-s-molina Jun 23, 2021

kgabryje Jun 23, 2021

michael-s-molina Jun 23, 2021

kgabryje Jun 23, 2021

feat(native-filters): Set default scope by filters' and charts' datasets #15302

feat(native-filters): Set default scope by filters' and charts' datasets #15302

Conversation

kgabryje commented Jun 22, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

villebro Jun 23, 2021

Choose a reason for hiding this comment

kgabryje Jun 23, 2021

Choose a reason for hiding this comment

villebro Jun 23, 2021

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

kgabryje commented Jun 23, 2021

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

michael-s-molina Jun 23, 2021

Choose a reason for hiding this comment

kgabryje Jun 23, 2021

Choose a reason for hiding this comment

michael-s-molina Jun 23, 2021

Choose a reason for hiding this comment

kgabryje Jun 23, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 23, 2021 •

edited

Loading