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

fix(native-filters): Show incompatible native filters indicator #12687

Merged

Conversation

agatapst
Copy link
Contributor

@agatapst agatapst commented Jan 22, 2021

SUMMARY

  • The aim of this PR was to add Incompatible section to filter indicator. ⚠Incompatible filters are created using datasets & fields which are incompatible with a given chart.
  • I have also added simple mocks for native filters: mockStore and mockNativeFilters to test displaying filter indicators for native filters.
  • I have also slightly changed structure of Filter Badge tests in FiltersBadge_spec.tsx - I have divided them into 'for dashboard filters' and 'for native filters'. Section 'for dashboard filters' is the same as on master, I have only added analogical tests for native filters.

Fixes #12568
Closes #12568

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before incompatible section for native filters was missing.
Now different configurations:
With some correct native filters applied and one incompatible:
Zrzut ekranu 2021-01-22 o 15 02 38

With trying to apply two incompatible native filters from two different datasets:
Zrzut ekranu 2021-01-22 o 15 09 53

With one dashboard filter applied and one incompatible native filter:
Zrzut ekranu 2021-01-22 o 15 12 35

With incompatible dashboard filters (without native filters):
Zrzut ekranu 2021-01-22 o 15 23 49

TEST PLAN

Enable native filters: go to config.py and set "DASHBOARD_NATIVE_FILTERS": True,
Go to dashboards and create native filters. One one them should be created with incompatible dataset, which cannot be applied to this dashboard.

ADDITIONAL INFORMATION

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

@agatapst agatapst changed the title fix(native-filter): Show incompatible native filters indicator fix(native-filters): Show incompatible native filters indicator Jan 22, 2021
@codecov-io
Copy link

Codecov Report

Merging #12687 (bfdd97b) into master (734fb75) will decrease coverage by 3.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12687      +/-   ##
==========================================
- Coverage   66.85%   63.32%   -3.54%     
==========================================
  Files        1018      486     -532     
  Lines       49776    29970   -19806     
  Branches     4869        0    -4869     
==========================================
- Hits        33278    18978   -14300     
+ Misses      16375    10992    -5383     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress ?
javascript ?
python 63.32% <ø> (-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/databases/commands/create.py 32.65% <0.00%> (-59.19%) ⬇️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/db_engine_specs/hive.py 54.61% <0.00%> (-29.24%) ⬇️
superset/views/database/mixins.py 59.64% <0.00%> (-22.81%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/databases/schemas.py 93.40% <0.00%> (-6.05%) ⬇️
superset/databases/dao.py 94.11% <0.00%> (-5.89%) ⬇️
superset/databases/api.py 86.55% <0.00%> (-5.47%) ⬇️
superset/views/database/validators.py 78.94% <0.00%> (-5.27%) ⬇️
... and 552 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 734fb75...bfdd97b. Read the comment docs.

@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jan 22, 2021
@junlincc junlincc requested review from junlincc and rusackas January 22, 2021 16:02
@junlincc junlincc added the hold:testing! On hold for testing label Jan 22, 2021
@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 22, 2021

@agatapst could you check this scenario? @junlincc Could you confirm that my expected behavior is valid?
Scenario:
Set 3 native filters (1 valid for this dashboard)
Enable all filters
Apply change
Check chart filters
Disable one of incorrect native filters and apply change

Observed: I see disabled filter in section Incompatible Filters
Expected: I would see this filter under Unset section.

unset.mov

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

Code LGTM, nice

@adam-stasiak
Copy link
Contributor

Retested again and consulted with Agata. It looks like my issue was caused by performance on my machine (on video you can see how long everything takes).
I double checked that after computer restart and saw that it was for a moment in yellow zone without value but after a second it was moved to unset 🆗 .

So no issue there.

@suddjian
Copy link
Member

Oh apologies Adam, I didn't see your comment when I left my review. Glad it's working!

@junlincc junlincc removed the hold:testing! On hold for testing label Jan 22, 2021
@junlincc
Copy link
Member

junlincc commented Jan 22, 2021

LGTM, thank you for fixing it! ♥️ @agatapst we are very close to demo-able stage.
Screen Shot 2021-01-22 at 1 51 41 PM

next step is to get the magnifier work

@junlincc junlincc added the need:merge The PR is ready to be merged label Jan 23, 2021
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@zhaoyongjie zhaoyongjie merged commit 9c53ad4 into apache:master Jan 23, 2021
@junlincc junlincc removed the need:merge The PR is ready to be merged label Jan 23, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard]native filter indicator incompatible section is missing
8 participants