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

Implement filtering for imported data #4118

Merged
merged 41 commits into from
Jun 3, 2024
Merged

Conversation

RobertJoonas
Copy link
Contributor

@RobertJoonas RobertJoonas commented May 21, 2024

Changes

This PR adds support for filtering by imported data. What we can or cannot filter by is limited by the imported schemas. Usually that means that the relation between different properties does not exist - e.g. we cannot filter by page and break down by source. That information doesn't exist. However, in some tables, there are several properties - e.g. imported_locations contains country, region and city propertis. That means that filtering by country and breaking down by city is possible.

Whenever filters are applied and imported data is included in the currently viewed period, we will display warning bubbles if a certain report is not able to include imported data:

image

Note that this does not mean that native data is excluded - the dashboard in the screenshot simply doesn't have any.

Tests

  • Automated tests have been added

Changelog

  • Entry has been added to changelog

Documentation

  • Docs have been updated

Dark mode

  • The UI has been tested both in dark and light mode

@RobertJoonas RobertJoonas marked this pull request as draft May 21, 2024 13:34
@zoldar zoldar force-pushed the imported-data-filters-cont branch 6 times, most recently from c4ac50a to 806c390 Compare May 29, 2024 07:06
@zoldar zoldar added deploy-to-staging Special label that triggers a deploy to a staging environment preview labels May 29, 2024
@zoldar zoldar force-pushed the imported-data-filters-cont branch 2 times, most recently from c9273ea to 849315a Compare May 29, 2024 13:21
@RobertJoonas RobertJoonas marked this pull request as ready for review May 30, 2024 11:24
|> do_decide_table()
end

defp transform_filters(query) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Can you explain the business logic here?

I'm planning on moving business logic soonish so would need to understand this better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transform_filters function is used for:

  1. dropping the redundant "is name pageview" filter which doesn't make sense it imported data
  2. mapping an event:goal filter into either event:name or event:page

In the imported querying logic we need to map goals to name/page in two places:

  1. deciding whether imported data can be included or not (i.e. schema_supports_query)
  2. actually applying the filter

The general business logic with imported data is that we can query different properties in relation to each other (breakdown prop / filters) as long as they belong to the same imported_* table.

For example, when filtering by event:page==/blog and event:goal==Visit /blog, the query is supported and the data should be fetched from the imported_pages table.

Happy to clarify it more in depth if you have any more specific questions :)

@macobo
Copy link
Contributor

macobo commented May 30, 2024

Request: If we can merge this faster by splitting this into separate FE and BE PRs I'd appreciate that. Blocked on a large chunk of query/APIv2 work on these changes.

@RobertJoonas
Copy link
Contributor Author

Request: If we can merge this faster by splitting this into separate FE and BE PRs I'd appreciate that. Blocked on a large chunk of query/APIv2 work on these changes.

I think we don't need to split it and can go ahead and merge it on Monday.

@macobo macobo mentioned this pull request Jun 3, 2024
8 tasks
@zoldar zoldar force-pushed the imported-data-filters-cont branch from 69dfcaa to efcb7ea Compare June 3, 2024 09:00
zoldar and others added 25 commits June 3, 2024 11:14
@zoldar zoldar force-pushed the imported-data-filters-cont branch from efcb7ea to 4e932dd Compare June 3, 2024 09:15
@zoldar zoldar merged commit 1d3b068 into master Jun 3, 2024
10 checks passed
@zoldar zoldar deleted the imported-data-filters-cont branch June 3, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-to-staging Special label that triggers a deploy to a staging environment preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants