fix(native-filters): Setting default value #12465

agatapst · 2021-01-12T18:00:57Z

SUMMARY

The aim of this PR was to improve setting default value for Native Filters.
The new Default Value Select component was created, which enables to choose default value based on dataset and field.
Now it is possible to set default value for a given filter.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:
for native filter without children (including editing)

for native filters child

TEST PLAN

Go to config.py and set "DASHBOARD_NATIVE_FILTERS": True
Go to dashboards, choose one and create filter or use existing one.
Set default value.

ADDITIONAL INFORMATION

Has associated issue: Fixes one of the issues mentioned in [🎉New Feature for Testing]Dashboard native filter and cascading filter are HERE! #12148
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

cc @junlincc @villebro @rusackas
@adam-stasiak could you test it?

adam-stasiak · 2021-01-12T19:10:40Z

I have a bug. Scenario:
Go to dashboard and add native filter. Set default value.
Export this dashboard
Import this dashboard
Open imported dashboard
Check default value imported - > should be the same as the original one
Open filter settings and change default filter value
You can see that default filter value will remain the same.

import.mov

codecov-io · 2021-01-12T20:29:43Z

Codecov Report

Merging #12465 (5bc267f) into master (017f11f) will increase coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12465      +/-   ##
==========================================
+ Coverage   63.13%   63.40%   +0.27%     
==========================================
  Files        1022      488     -534     
  Lines       50032    30115   -19917     
  Branches     4915        0    -4915     
==========================================
- Hits        31587    19095   -12492     
+ Misses      18245    11020    -7225     
+ Partials      200        0     -200

Flag	Coverage Δ
javascript	`?`
python	`63.40% <ø> (-0.69%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/db_engine_specs/hive.py	`73.84% <0.00%> (-17.31%)`	⬇️
superset/databases/commands/create.py	`83.67% <0.00%> (-8.17%)`	⬇️
superset/databases/commands/update.py	`85.71% <0.00%> (-8.17%)`	⬇️
superset/connectors/sqla/models.py	`84.31% <0.00%> (-6.28%)`	⬇️
superset/db_engine_specs/sqlite.py	`90.62% <0.00%> (-6.25%)`	⬇️
superset/databases/commands/test_connection.py	`84.78% <0.00%> (-4.35%)`	⬇️
superset/utils/celery.py	`96.42% <0.00%> (-3.58%)`	⬇️
superset/models/core.py	`85.59% <0.00%> (-3.27%)`	⬇️
... and 540 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 017f11f...5bc267f. Read the comment docs.

rusackas · 2021-01-13T07:38:11Z

I have a bug. Scenario:
Go to dashboard and add native filter. Set default value.
Export this dashboard
Import this dashboard
Open imported dashboard
Check default value imported - > should be the same as the original one
Open filter settings and change default filter value
You can see that default filter value will remain the same.

import.mov

Whoa... that's some impressive testing!

agatapst · 2021-01-13T12:42:24Z

@adam-stasiak thanks for thorough testing - as always! 🚀
I think there is a bigger bug with imported dashboards - it is not possible to edit them on master as well. I suggest to separate it - I have created issue for it #12495 that should solve the bug you have described.

There is a gif presenting that if problem with editing imported dashboard is fixed, we won't have that bug.

suddjian

awesome work!

junlincc

Great work!
Next step, let's make it possible to set multiple default values in the modal, reflect the selection in the filter bar dropdown.

@agatapst 🙏

this one is good to go. ✅

adam-stasiak · 2021-01-26T22:08:15Z

Double checked this - is ok 🟢

junlincc · 2021-01-27T05:14:13Z

@agatapst could we resolve conflicts? we probably should have merge this before the other ones🤦🏾‍♀️
thanks! 🙏

This reverts commit bd07e169f009862647327bf3d15e02f98ea8be9b.

agatapst · 2021-01-27T11:02:09Z

Conflicts solved, but there were some significant changes to adjust this PR to current master, so please wait with merging - I'll check it one more time. 🟡

Update: works as before 🟢

simcha90 · 2021-01-27T11:31:55Z

superset-frontend/src/dashboard/components/nativeFilters/DefaultValueSelect.tsx

+  }, [form, filterId]);
+
+  useChangeEffect(datasetId, previous => {
+    if (previous != null) {


may better use strict comparison like: === to process correctly 0 or empty string?

simcha90 · 2021-01-27T11:32:10Z

superset-frontend/src/dashboard/components/nativeFilters/DefaultValueSelect.tsx

+  });
+
+  useChangeEffect(column, previous => {
+    if (previous != null) {


and same here

simcha90 · 2021-01-27T11:44:46Z

Hi it looks good work 👍
I have some question about implementation. I see you use Select to choose defaultValue but we have different native filters chart types, for example for now we have already implemented Range native filter, so I think in this case we can't use Select to select appropriate value.

So I think it can be better use SuperChart here to inject appropriate value selection for default value, I had some PR for it but closed it in favour of yours, because you also implemented it in different way... but I still think that it can be good to use SuperChart here to select default value:
#12199
cc @villebro thoughts?

simcha90

@junlincc @suddjian @rusackas just wanted to ask what other devs think about SuperChart proposal?
Like here: #12199

agatapst · 2021-01-27T12:24:14Z

@simchanielsen Hi, thanks for your suggestions! Yes, you are right - we use Select component for setting default values in all cases. So for range filters it may not work properly. Tbh creating this PR I didn't know that type should be supported, so thanks for checking that.

Your solution looks nice - I don't mind closing this PR in favor of yours. My PR is completely based on DefaultValueSelect component, which in case of using SuperChart, would require to change the whole implementation here and create it from scratch.

@villebro @junlincc @rusackas @suddjian I am waiting for your opinion as well.

villebro · 2021-01-27T15:15:46Z

I agree with @simchanielsen - we should decouple the filters as much as possible from the filter config modal, so it would be preferable to use SuperChart there similarly to how it is being used in the filter tab.

junlincc · 2021-01-28T08:10:00Z

@simchanielsen thanks for the solution, I do agree we should go this route ♥️. I wonder why you closed it earlier though?

@agatapst thanks for your great work still:) 🙏

suddjian · 2021-02-01T21:19:28Z

Agreed that the other approach makes more sense. Closing this for now.

superset-github-bot bot added the Polidea label Jan 12, 2021

pull-request-size bot added the size/L label Jan 12, 2021

rusackas requested a review from suddjian January 13, 2021 07:34

agatapst force-pushed the fix/native-filters-default-value branch from fc469dd to bd07e16 Compare January 14, 2021 11:48

junlincc requested review from junlincc and rusackas January 16, 2021 20:01

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jan 16, 2021

agatapst force-pushed the fix/native-filters-default-value branch from e4e3f71 to 7945d7c Compare January 18, 2021 09:15

junlincc added the hold:testing! On hold for testing label Jan 19, 2021

agatapst force-pushed the fix/native-filters-default-value branch from 7945d7c to a59a2e2 Compare January 20, 2021 17:37

suddjian approved these changes Jan 22, 2021

View reviewed changes

junlincc reviewed Jan 23, 2021

View reviewed changes

junlincc removed the hold:testing! On hold for testing label Jan 24, 2021

agatapst force-pushed the fix/native-filters-default-value branch from a59a2e2 to 3ac260e Compare January 25, 2021 08:54

agatapst added 7 commits January 27, 2021 11:45

Enable setting default value for native filters

8a6a177

Fix setting initial filter state

97cdfd6

Add Default Value Select component

a609dd8

Fix linter errors

b6008e1

Enable clear value for Default Value Select

8e38852

Reset default value select for removed dataset

e43102c

Revert "Reset default value select for removed dataset"

2d72446

This reverts commit bd07e169f009862647327bf3d15e02f98ea8be9b.

agatapst force-pushed the fix/native-filters-default-value branch from 3ac260e to 2d72446 Compare January 27, 2021 10:54

simcha90 reviewed Jan 27, 2021

View reviewed changes

simcha90 requested changes Jan 27, 2021

View reviewed changes

Add prev state to getInitialExtraFormData

5bc267f

simcha90 mentioned this pull request Jan 28, 2021

feat(native-filters): Add defaultValue for Native filters modal #12199

Merged

6 tasks

suddjian closed this Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(native-filters): Setting default value #12465

fix(native-filters): Setting default value #12465

agatapst commented Jan 12, 2021

adam-stasiak commented Jan 12, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

rusackas commented Jan 13, 2021

agatapst commented Jan 13, 2021

suddjian left a comment

junlincc left a comment •

edited

Loading

adam-stasiak commented Jan 26, 2021

junlincc commented Jan 27, 2021

agatapst commented Jan 27, 2021 •

edited

Loading

simcha90 Jan 27, 2021

simcha90 Jan 27, 2021

simcha90 commented Jan 27, 2021

simcha90 left a comment •

edited

Loading

agatapst commented Jan 27, 2021 •

edited

Loading

villebro commented Jan 27, 2021

junlincc commented Jan 28, 2021

suddjian commented Feb 1, 2021

fix(native-filters): Setting default value #12465

fix(native-filters): Setting default value #12465

Conversation

agatapst commented Jan 12, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

adam-stasiak commented Jan 12, 2021

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

rusackas commented Jan 13, 2021

agatapst commented Jan 13, 2021

suddjian left a comment

Choose a reason for hiding this comment

junlincc left a comment • edited Loading

Choose a reason for hiding this comment

adam-stasiak commented Jan 26, 2021

junlincc commented Jan 27, 2021

agatapst commented Jan 27, 2021 • edited Loading

simcha90 Jan 27, 2021

Choose a reason for hiding this comment

simcha90 Jan 27, 2021

Choose a reason for hiding this comment

simcha90 commented Jan 27, 2021

simcha90 left a comment • edited Loading

Choose a reason for hiding this comment

agatapst commented Jan 27, 2021 • edited Loading

villebro commented Jan 27, 2021

junlincc commented Jan 28, 2021

suddjian commented Feb 1, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

junlincc left a comment •

edited

Loading

agatapst commented Jan 27, 2021 •

edited

Loading

simcha90 left a comment •

edited

Loading

agatapst commented Jan 27, 2021 •

edited

Loading