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

feat(native-filters): Adhoc dashboard native filters #22168

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

cccs-RyanK
Copy link
Contributor

SUMMARY

Our users really like the functionality of the adhoc filters in the explore view and were interested in something similar for for dashboard filtering. Anything that can be done with an adhoc filter native filter can be done with the other filter types and vice versa, but in order to filter by any given column the user has to either:

  • create a new filter (which can be a hassle to do on an adhoc basis),
  • or have a value filter for every column in the dashboard which is not feasible in every dashboard.

In this PR we added a new native filter type that reuses the adhoc filter component from the explore view to allow for more complex filtering to easily be done in a dashboard.

This solution provides the user with a lot of power and flexibility in dashboards, which is why it is behind a feature flag for now.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

adhoc-native-filters-demo_p1.mp4
adhoc-native-filters-demo_p2.mp4

TESTING INSTRUCTIONS

  • Enable Feature Flag: ADHOC_DASHBOARD_NATIVE_FILTERS
  • Navigate to dashboard
  • Add/Edit Filters on the sidebar
  • A new filter type should appear called "Adhoc"

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: ADHOC_DASHBOARD_NATIVE_FILTERS
  • 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
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Attention: Patch coverage is 6.45161% with 87 lines in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (76d897e) to head (3c8c73c).
Report is 305 commits behind head on master.

Files Patch % Lines
...src/filters/components/Adhoc/AdhocFilterPlugin.tsx 0.00% 59 Missing ⚠️
...end/src/filters/components/Adhoc/transformProps.ts 0.00% 12 Missing ⚠️
superset-frontend/src/filters/utils.ts 12.50% 7 Missing ⚠️
...rontend/src/filters/components/Adhoc/buildQuery.ts 16.66% 5 Missing ⚠️
...set-frontend/src/filters/components/Adhoc/index.ts 0.00% 3 Missing ⚠️
...ontrols/FilterControl/AdhocFilterControl/index.jsx 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #22168      +/-   ##
==========================================
+ Coverage   60.48%   70.35%   +9.86%     
==========================================
  Files        1931     1958      +27     
  Lines       76236    78450    +2214     
  Branches     8568     8783     +215     
==========================================
+ Hits        46114    55192    +9078     
+ Misses      28017    21126    -6891     
- Partials     2105     2132      +27     
Flag Coverage Δ
hive 48.62% <ø> (-0.54%) ⬇️
javascript 57.67% <6.45%> (-0.04%) ⬇️
mysql 77.31% <ø> (?)
postgres 77.43% <ø> (?)
presto 53.67% <ø> (-0.13%) ⬇️
python 83.70% <ø> (+20.21%) ⬆️
sqlite 76.87% <ø> (?)
unit 59.22% <ø> (+1.59%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@villebro villebro changed the title Adhoc dashboard native filters feat(native-filters): Adhoc dashboard native filters Nov 22, 2022
@villebro villebro self-requested a review November 22, 2022 07:00
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

A few first pass comments. I love this, this really improves adhoc analysis in the dashboard view ❤️

@hushaoqing
Copy link
Contributor

looking forward to it being released.

@rusackas
Copy link
Member

Pinging @kasiazjc @yousoph to make sure we're ok with the popover approach and other components being recycled in the filters area.

@yousoph
Copy link
Member

yousoph commented Apr 18, 2023

Cool! Thanks for this feature! Two comments:

  1. The + button feels out of place, I think we can hide it like it's hidden from the chart builder:
    image
  2. I don't think we use the terminology "ad hoc filters" in any user facing copy right now, are there any other names that might make it clear to the user about what the filter does? I'm having trouble naming it, would love any ideas!

@michael-s-molina
Copy link
Member

Thank you for the PR @cccs-RyanK!

How will this filter behave considering inter filter features such as Values depend on other filters and Filter scopes? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.

Is the popup arrow pointing to the correct direction when the filter bar is horizontal? We need to consider the scenario where the filter is displayed in the bar in which case the arrow should point up and the case where the filter is displayed in the dropdown in which case the arrow should point to the right.

I agree with @yousoph's points.

@cccs-RyanK
Copy link
Contributor Author

Cool! Thanks for this feature! Two comments:

1. The + button feels out of place, I think we can hide it like it's hidden from the chart builder:
   ![image](https://user-images.githubusercontent.com/10627051/232914816-77cf6196-ee53-4f10-b6a6-2485608fa63b.png)

2. I don't think we use the terminology "ad hoc filters" in any user facing copy right now, are there any other names that might make it clear to the user about what the filter does? I'm having trouble naming it, would love any ideas!

Thank you for the feedback @yousoph! I agree with the first point I can remove the plus button for sure. For the name maybe "advanced", "extra", or "custom" filters? Not sure if any of those fit.

@michael-s-molina
Copy link
Member

How will this filter behave considering inter filter features such as Values depend on other filters and Filter scopes? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.

@cccs-RyanK Just to be more clear about the above point. There's a feature called Values depend on other filters (check this PR's video) which allows you to establish an hierarchy between filters. One example is when you have a State and City filters and you want to change the City's values depending on the selected value in State. This all works because there's a clear definition of the dataset and column when creating a filter. When you introduce a SQL filter, we don't know what's the column (or even if the SQL returns a column), so I'm wondering how all the features that depend on this definition will work. I think we need to at least disable these features when working with a SQL filter. If I defined the State filter as a SQL filter, then it shouldn't appear as one of the options in Values depend on other filters when configuring the City filter.

@cccs-RyanK
Copy link
Contributor Author

How will this filter behave considering inter filter features such as Values depend on other filters and Filter scopes? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.

@cccs-RyanK Just to be more clear about the above point. There's a feature called Values depend on other filters (check this PR's video) which allows you to establish an hierarchy between filters. One example is when you have a State and City filters and you want to change the City's values depending on the selected value in State. This all works because there's a clear definition of the dataset and column when creating a filter. When you introduce a SQL filter, we don't know what's the column (or even if the SQL returns a column), so I'm wondering how all the features that depend on this definition will work. I think we need to at least disable these features when working with a SQL filter. If I defined the State filter as a SQL filter, then it shouldn't appear as one of the options in Values depend on other filters when configuring the City filter.

@michael-s-molina thank you for the clarification! Yes I think that feature will need to be disabled on the SQL filter based on the reasons you mentioned. As for scoping, the SQL filter should work the same as other filters such as Time column filter which only specifies the dataset and not a specific column.

Lastly, I also agree about the horizontal filter bar I will test and adjust the popover so that it pops below instead of to the right in that case. Aiming to work on this soon.

@rusackas rusackas requested a review from kasiazjc July 27, 2023 03:42
@michaelkovatt
Copy link

michaelkovatt commented Nov 21, 2023

@cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.

@cccs-RyanK
Copy link
Contributor Author

@cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.

Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?

@michaelkovatt
Copy link

michaelkovatt commented Nov 21, 2023

@cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.

Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?

@cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.

Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?

@cccs-RyanK , yes you are right. Only need to enable this for some roles. Also, wanted to know if there is any timeline for the feature to go as part of next release?

@cccs-RyanK
Copy link
Contributor Author

@cccs-RyanK , yes you are right. Only need to enable this for some roles. Also, wanted to know if there is any timeline for the feature to go as part of next release?

I have no timeline on finishing it at the moment since I have been using this feature in my fork of superset for quite a while. Your comment the other day brought my attention back to this and I would like to finally push to include it in a future release. One major wall that I ran into was the design of the component when in horizontal view. In the screenshot I have included below you can see that adding a few filters expands the height too much. If you added enough filters you could eventually expand the filterbar to take up the whole screen. I am looking for input/ideas on adjusting it
image

@rusackas rusackas marked this pull request as draft February 1, 2024 20:36
@rusackas
Copy link
Member

rusackas commented Feb 1, 2024

Setting this to draft, but please make it ready for review when it's... ready for review. :)

@cccs-rc cccs-rc force-pushed the Adhoc-Dashboard-Native-Filters branch from be2e686 to db8b382 Compare March 6, 2024 19:08
@github-actions github-actions bot added doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages dependencies:python labels Mar 6, 2024
@villebro
Copy link
Member

@cccs-RyanK is CCCS still interested in reopening this PR? This request comes up from time to time, and this would cover a bunch of use cases that are impossible with the current set of filters..

@cccs-RyanK cccs-RyanK force-pushed the Adhoc-Dashboard-Native-Filters branch from db8b382 to 71ef25e Compare June 4, 2024 13:30
@github-actions github-actions bot removed doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code dependencies:python labels Jun 4, 2024
@villebro villebro marked this pull request as ready for review June 7, 2024 16:01
@dosubot dosubot bot added change:frontend Requires changing the frontend dashboard:native-filters Related to the native filters of the Dashboard labels Jun 7, 2024
@john-bodley john-bodley added hold! On hold and removed review:draft labels Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend dashboard:native-filters Related to the native filters of the Dashboard hold! On hold packages size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants