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

Sessions filtering system #2912

Merged
merged 45 commits into from
Jan 13, 2021
Merged

Sessions filtering system #2912

merged 45 commits into from
Jan 13, 2021

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jan 11, 2021

Changes

This PR:

  1. Refactors SelectBox + PropertyFilters logic to be able to be reused in sessions filtering
  2. Introduces sessions filtering logic
  3. Introduces a way to save sessions filters
  4. Refactors the backend to filter data based on parameters
  5. Removes previous action filtering/recording duration filtering logic, replacing it with new system (closes Filter sessions by recording available #2281)

What this does not (yet) do:

  1. Support multiple action filters on clickhouse
  2. Supporting filtering via action filters on postgres
  3. Nice UX
  4. Making sessions page support viewing multiple dates at once
  5. Cypress tests

These will be resolved in follow-up PRs. Paolo was promising to help with the design-side things.

2021-01-11_15-34
2021-01-11_14-34

Peek.2021-01-08.16-13.mp4

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-insane-filters-henjhqd January 11, 2021 13:37 Inactive
@macobo macobo temporarily deployed to posthog-insane-filters-henjhqd January 11, 2021 14:22 Inactive
@macobo macobo temporarily deployed to posthog-insane-filters-henjhqd January 11, 2021 14:37 Inactive
@macobo macobo marked this pull request as ready for review January 11, 2021 15:39
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

QA'd and it seems to work as expected. As mentioned in the general PR description, will skip any comments on UX for a separate improvement.

A few additional comments:

  1. Unsure if this should be considered part of this PR or not, but the filters no longer have the same styling (the same height issue is something I believe @EDsCODE was working on)
    image

  2. Again not sure if you want to leave for later, but after you save a filter, the filter dropdown still says "Custom filter" instead of the name of the newly saved filter.

Btw great job on the renaming of the columns on the sessions table, definitely clearer!

posthog/models/filters/mixins/sessions.py Show resolved Hide resolved
posthog/models/filters/mixins/sessions.py Show resolved Hide resolved
posthog/models/filters/sessions_filter.py Show resolved Hide resolved
posthog/models/sessions_filter.py Outdated Show resolved Hide resolved
posthog/api/sessions_filter.py Outdated Show resolved Hide resolved
onChange(currentOperator || 'exact', value)
}}
/>
{(operator === 'gt' || operator === 'lt') && isNaN(value) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like trying to type a string still generated an error (see below). Also, because the HTML input is type="number" then a string will never be accepted and this error never shown (unless I'm unaware of a different behavior in other browsers).

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to avoid refactoring this this PR - this bug was there previously, I just moved some code around.

export function PropertySelect({ optionGroups, value, onChange, placeholder, autoOpenIfEmpty }: Props): JSX.Element {
return (
<SelectGradientOverflow
className={rrwebBlockClass}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth adding this to the specific labels of non-default items so that the recordings make more sense? Feel free to disregard if outside the scope of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep it outside the scope

posthog/api/sessions_filter.py Outdated Show resolved Hide resolved
@macobo
Copy link
Contributor Author

macobo commented Jan 13, 2021

Unsure if this should be considered part of this PR or not, but the filters no longer have the same styling (the same height issue is something I believe @EDsCODE was working on)

Good point, something that likely sneaked in via rebases.

@macobo macobo temporarily deployed to posthog-insane-filters-henjhqd January 13, 2021 12:31 Inactive
@macobo macobo temporarily deployed to posthog-insane-filters-henjhqd January 13, 2021 12:35 Inactive
@macobo
Copy link
Contributor Author

macobo commented Jan 13, 2021

Most of the issues have been solved.

Re select height, this was fixed by rebasing again.
Re custom selector - needed to introduce a new dependency, though it was already in our bundle.

@macobo macobo temporarily deployed to posthog-insane-filters-henjhqd January 13, 2021 12:38 Inactive
@macobo macobo merged commit 19c50a2 into master Jan 13, 2021
@macobo macobo deleted the insane-filters branch January 13, 2021 12:55
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.

Filter sessions by recording available
3 participants