feat(native-filters): Show/Hide filter bar by metdata ff #14261

simcha90 · 2021-04-21T06:29:32Z

SUMMARY

As we are slowly releasing native filters we would like to have the ability per dashboard to choose if to hide/show the filter.
some of dashboard owners still not ready to just convert their filter boxes into native filters and although they could both live together having an empty filter bar might be confusing for end clients.
therefore we are adding the ability to
Show/Hide filter bar by using show_native_filters as another property within the dashboard metadata in addition to the Global feature flag

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

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

� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts

…rs_ff

codecov · 2021-04-21T07:36:00Z

Codecov Report

Merging #14261 (b03b909) into master (22f9e12) will increase coverage by 0.07%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master   #14261      +/-   ##
==========================================
+ Coverage   77.05%   77.13%   +0.07%     
==========================================
  Files         954      954              
  Lines       48140    48149       +9     
  Branches     5986     5991       +5     
==========================================
+ Hits        37096    37140      +44     
+ Misses      10847    10812      -35     
  Partials      197      197

Flag	Coverage Δ
hive	`80.77% <100.00%> (+<0.01%)`	⬆️
javascript	`72.02% <50.00%> (-0.01%)`	⬇️
mysql	`81.03% <100.00%> (+<0.01%)`	⬆️
postgres	`81.07% <100.00%> (+<0.01%)`	⬆️
presto	`80.76% <100.00%> (?)`
python	`81.59% <100.00%> (+0.15%)`	⬆️
sqlite	`80.67% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js	`3.30% <0.00%> (-0.06%)`	⬇️
...d/components/DashboardBuilder/DashboardBuilder.tsx	`86.95% <71.42%> (-1.42%)`	⬇️
...omponents/nativeFilters/FilterBar/Header/index.tsx	`90.90% <100.00%> (ø)`
superset/dashboards/schemas.py	`99.37% <100.00%> (+<0.01%)`	⬆️
superset/models/core.py	`89.13% <0.00%> (+0.27%)`	⬆️
superset/connectors/sqla/models.py	`90.07% <0.00%> (+1.45%)`	⬆️
superset/db_engine_specs/presto.py	`89.89% <0.00%> (+5.47%)`	⬆️

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 22f9e12...b03b909. Read the comment docs.

vonsamnang01041994

ok

amitmiran137

LGTM!

tested that migration works
when key doesn't exist it default to true as if only the DASHBOARD_NATIVE_FILTERS FF is calling the shots
when key is false no filter bar
when key is true , filter bar is shown

villebro

I think this is a good feature and would probably be helpful in the migration effort leaving old dashboards as-is until a conscious decision to migrate has been made. However, for now I think we should only disable native filters for dashboards that have filter boxes added to them. In addition, I'd add a comment to UPDATING.md to let users know that this option now exists and has been set to false for all charts that have filter boxes, otherwise end users might not know why certain dashboards are missing the feature. Also calling on @graceguo-supercat and @junlincc for feeback.

superset/migrations/versions/4fad8b8b566d_show_native_filters_dashboard_metadata.py

suddjian

I would like to avoid adding a database migration for this.

If some dashboard owners would like to remove the filter bar from their dashboards, it makes sense to add a metadata json option that they can set, but setting it as the default seems like overkill. Especially so when the native filters are already behind a feature flag in the first place.

Side note: There are also efforts underway to build an option for dashboard owners to auto-migrate from filter box to native filters, so the issue of having filter boxes hanging around will hopefully be short lived.

Side note 2: In the case where a non-editor is viewing a dashboard that has no native filters, it might be better to not show the filter bar since it will just be empty.

suddjian · 2021-04-26T21:24:00Z

superset/migrations/versions/4fad8b8b566d_show_native_filters_dashboard_metadata.py

+        .all()
+    )
+
+    changed_filters = 0


Is this variable used for something?

graceguo-supercat · 2021-04-27T02:05:49Z

Currently dashboard has no automatic logic to convert filter_box into filter bar.

if dashboard already had filter component and no filter_box, Dashboard should show filter bar.
if dashboard already had filter component and some old filter_box, we should show both filter bar and filter_box. This means user started manually convert filter_box to new filter components.
if dashboard had no filter component but had many old had filter_box, I guess this type of users are not ready to manually convert their filter_box to filter component yet, then we should hide filter bar when user open dashboard.

In short, we should show filter bar if dashboard has any native filter metadata, otherwise, hide filter bar by default.
What is new feature flag supposed to do?

amitmiran137 · 2021-04-27T04:55:40Z

Currently dashboard has no automatic logic to convert filter_box into filter bar.

if dashboard already had filter component and no filter_box, Dashboard should show filter bar.

if dashboard already had filter component and some old filter_box, we should show both filter bar and filter_box. This means user started manually convert filter_box to new filter components.

if dashboard had no filter component but had many old had filter_box, I guess this type of users are not ready to manually convert their filter_box to filter component yet, then we should hide filter bar when user open dashboard.

In short, we should show filter bar if dashboard has any native filter metadata, otherwise, hide filter bar by default.
What is new feature flag supposed to do?

It enables you to hide the filter component in a specific Dashboard

amitmiran137 · 2021-04-27T05:05:00Z

I would like to avoid adding a database migration for this.

If some dashboard owners would like to remove the filter bar from their dashboards, it makes sense to add a metadata json option that they can set, but setting it as the default seems like overkill. Especially so when the native filters are already behind a feature flag in the first place.

Side note: There are also efforts underway to build an option for dashboard owners to auto-migrate from filter box to native filters, so the issue of having filter boxes hanging around will hopefully be short lived.

Side note 2: In the case where a non-editor is viewing a dashboard that has no native filters, it might be better to not show the filter bar since it will just be empty.

I think I'm going to replace the migration with side note 2
That would accomplish exactly the use of showing the filter bar when it is not relevant
Thanks for that !

amitmiran137

We should also hide filter bar if no native filters exists
@simcha90

…rs_ff

simcha90 · 2021-04-27T07:00:48Z

@suddjian added proposal:

Side note 2: In the case where a non-editor is viewing a dashboard that has no native filters, it might be better to not show the filter bar since it will just be empty.

suddjian

Thanks for making that change, this LGTM

* fix:fix get permission function * feat: hide native filters by metadata * fix: fix show_native_filters name * fix: metadata * chore: add migration to hide native_filters in all existing dashboards * chore: fix pre-commit * fix: make migration migration dashboard with filter_box only * fix: pre-commit * feat: hide filter bar if no permission edit and no filters * fix: remove migration Co-authored-by: amitmiran137 <amit.miran@nielsen.com> (cherry picked from commit 2486fd4)

john-bodley · 2021-07-01T17:01:24Z

superset-frontend/src/dashboard/actions/hydrate.js

+    metadata = {};
+  }
+
+  metadata.show_native_filters =


@simcha90 @amitmiran137 @villebro should this use the DASHBOARD_NATIVE_FILTERS feature flag for defining the fallback rather than defaulting to true? We've seen instances at Airbnb where dashboard native filters are showing up in the dashboard metadata even though the feature is disabled.

* fix:fix get permission function * feat: hide native filters by metadata * fix: fix show_native_filters name * fix: metadata * chore: add migration to hide native_filters in all existing dashboards * chore: fix pre-commit * fix: make migration migration dashboard with filter_box only * fix: pre-commit * feat: hide filter bar if no permission edit and no filters * fix: remove migration Co-authored-by: amitmiran137 <amit.miran@nielsen.com>

simcha90 added 5 commits April 13, 2021 16:04

fix:fix get permission function

6eb04b1

Merge branch 'master' of github.com:apache/superset

5e9a1d8

� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts

Merge branch 'master' of github.com:apache/superset

2c5800a

Merge branch 'master' of github.com:apache/superset into native_filte…

18ea534

…rs_ff

feat: hide native filters by metadata

0587d54

pull-request-size bot added the size/S label Apr 21, 2021

amitmiran137 added the v1.2 label Apr 21, 2021

amitmiran137 changed the title ~~feat(native-filters): Hide filter bar by matdata~~ feat(native-filters): Show/Hide filter bar by metdata ff Apr 21, 2021

amitmiran137 added v1.2 and removed v1.2 labels Apr 22, 2021

simcha90 and others added 3 commits April 22, 2021 12:52

fix: fix show_native_filters name

6dc4250

fix: metadata

ff40666

chore: add migration to hide native_filters in all existing dashboards

4e1beec

simcha90 requested a review from a team as a code owner April 25, 2021 10:27

pull-request-size bot added size/L and removed size/S labels Apr 25, 2021

chore: fix pre-commit

682698f

vonsamnang01041994 approved these changes Apr 25, 2021

View reviewed changes

amitmiran137 requested a review from villebro April 25, 2021 16:49

amitmiran137 approved these changes Apr 25, 2021

View reviewed changes

villebro reviewed Apr 26, 2021

View reviewed changes

superset/migrations/versions/4fad8b8b566d_show_native_filters_dashboard_metadata.py Outdated Show resolved Hide resolved

villebro requested review from graceguo-supercat and junlincc April 26, 2021 04:14

amitmiran137 added 2 commits April 26, 2021 15:54

fix: make migration migration dashboard with filter_box only

d435241

fix: pre-commit

8c29d23

suddjian reviewed Apr 26, 2021

View reviewed changes

amitmiran137 requested changes Apr 27, 2021

View reviewed changes

simcha90 added 2 commits April 27, 2021 09:53

Merge branch 'master' of github.com:apache/superset into native_filte…

452ba8f

…rs_ff

feat: hide filter bar if no permission edit and no filters

2f0643b

fix: remove migration

b03b909

pull-request-size bot added size/M and removed size/L labels Apr 27, 2021

suddjian approved these changes Apr 27, 2021

View reviewed changes

amitmiran137 approved these changes Apr 28, 2021

View reviewed changes

amitmiran137 merged commit 2486fd4 into apache:master Apr 28, 2021

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Apr 28, 2021

junlincc added this to Done in Native dashboard filters Apr 30, 2021

john-bodley reviewed Jul 1, 2021

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Show/Hide filter bar by metdata ff #14261

feat(native-filters): Show/Hide filter bar by metdata ff #14261

simcha90 commented Apr 21, 2021 •

edited by amitmiran137

codecov bot commented Apr 21, 2021 •

edited

vonsamnang01041994 left a comment

amitmiran137 left a comment

villebro left a comment

suddjian left a comment •

edited

suddjian Apr 26, 2021

graceguo-supercat commented Apr 27, 2021 •

edited

amitmiran137 commented Apr 27, 2021

amitmiran137 commented Apr 27, 2021

amitmiran137 left a comment

simcha90 commented Apr 27, 2021 •

edited

suddjian left a comment

john-bodley Jul 1, 2021

feat(native-filters): Show/Hide filter bar by metdata ff #14261

feat(native-filters): Show/Hide filter bar by metdata ff #14261

Conversation

simcha90 commented Apr 21, 2021 • edited by amitmiran137

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Apr 21, 2021 • edited

Codecov Report

vonsamnang01041994 left a comment

Choose a reason for hiding this comment

amitmiran137 left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

suddjian left a comment • edited

Choose a reason for hiding this comment

suddjian Apr 26, 2021

Choose a reason for hiding this comment

graceguo-supercat commented Apr 27, 2021 • edited

amitmiran137 commented Apr 27, 2021

amitmiran137 commented Apr 27, 2021

amitmiran137 left a comment

Choose a reason for hiding this comment

simcha90 commented Apr 27, 2021 • edited

suddjian left a comment

Choose a reason for hiding this comment

john-bodley Jul 1, 2021

Choose a reason for hiding this comment

simcha90 commented Apr 21, 2021 •

edited by amitmiran137

codecov bot commented Apr 21, 2021 •

edited

suddjian left a comment •

edited

graceguo-supercat commented Apr 27, 2021 •

edited

simcha90 commented Apr 27, 2021 •

edited