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

Multiple filters on the frontend #4174

Merged
merged 16 commits into from
Jun 11, 2024
Merged

Multiple filters on the frontend #4174

merged 16 commits into from
Jun 11, 2024

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jun 3, 2024

Changes

This PR does a few things:

  • It adds a new multiple_filters feature flag which allows adding multiple filters on the frontend (except for goals).
  • Updates filter serialization on the frontend
  • Adds new filter (and query) parsing methods (for a follow-up PR)
  • Reduces number of internal filter operators we support, collapsing list and single-item operators

image

Blocked on #4118 - will update filtering within after that PR lands.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@macobo macobo requested a review from ukutaht June 3, 2024 06:21
@macobo macobo force-pushed the qa2-2 branch 4 times, most recently from 5f87125 to 89e3e63 Compare June 5, 2024 06:48
@macobo macobo marked this pull request as ready for review June 5, 2024 06:52
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Approved but I would like to discuss which filter types will be available to the client

}
if (operation == FILTER_OPERATIONS.contains) {
clauses = clauses.map((value) => value.includes('*') ? value : `**${value}**`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to go in the direction that matches is a first-class operation that would also be available in the API. Am I reading it right?

My thinking on what filters should be available to client:

  • is - can deal with lists like you've already done in this PR. Can also take wildcards.
  • contains - equivalent to is where value is wrapped with *${value}*

I'm not sure about exposing matches for simple wildcards. It would fit better for a full regex match filter operation in the future.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I completely understood. Created https://3.basecamp.com/5308029/buckets/37310803/card_tables/cards/7494308360 for this discussion so it doesn't get dropped.

lib/plausible/stats/aggregate.ex Outdated Show resolved Hide resolved
assets/js/dashboard/stats/modals/filter-modal-group.js Outdated Show resolved Hide resolved
@macobo macobo merged commit e1f0002 into master Jun 11, 2024
10 checks passed
@macobo macobo deleted the qa2-2 branch June 11, 2024 11:23
aerosol added a commit that referenced this pull request Jun 11, 2024
macobo pushed a commit that referenced this pull request Jun 11, 2024
macobo added a commit that referenced this pull request Jun 11, 2024
macobo added a commit that referenced this pull request Jun 13, 2024
* Revert "Revert "Multiple filters on the frontend (#4174)" (#4218)"

This reverts commit 2f7dcae.

* Make city links work again

By enforcing everything sent to the BE is stringified. We do this at serialization-time to ensure old dashboard links still work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants