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

Enable customising which events to consider on Paths #5741

Merged
merged 9 commits into from
Aug 26, 2021

Conversation

neilkakkar
Copy link
Contributor

Changes

Allows for choosing which events to include and which ones to exclude on Paths

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@neilkakkar neilkakkar requested review from macobo, EDsCODE and liyiy August 25, 2021 12:47
class TargetEventsMixin(BaseParamMixin):
@cached_property
def target_events(self) -> List[str]:
return self._data.get(PATHS_INCLUDE_EVENT_TYPES, [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liyiy - do you think there's a better way to pass information down from the frontend?

Right now it looks something like: https://github.com/PostHog/posthog/pull/5741/files#diff-9b53c3a3f635f0fa8c15a295b375b72af36bd9f60f638e833ca20acc609b7a57R325

@timgl timgl temporarily deployed to posthog-pr-5741 August 25, 2021 12:49 Inactive
@timgl timgl temporarily deployed to posthog-pr-5741 August 25, 2021 13:12 Inactive
@timgl timgl temporarily deployed to posthog-pr-5741 August 25, 2021 13:16 Inactive
@timgl timgl temporarily deployed to posthog-pr-5741 August 25, 2021 13:33 Inactive
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

lgtm

@timgl timgl temporarily deployed to posthog-pr-5741 August 26, 2021 09:21 Inactive
@neilkakkar neilkakkar enabled auto-merge (squash) August 26, 2021 09:22
@neilkakkar neilkakkar merged commit 6b916c9 into master Aug 26, 2021
@neilkakkar neilkakkar deleted the path_exclusions branch August 26, 2021 09:34
)

_fields = list(filter(None, _fields))
Copy link
Contributor

@macobo macobo Aug 26, 2021

Choose a reason for hiding this comment

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

What does this do :O I'd expect filter to accept a lambda. I guess this is a compact-like.

Also nitpick - now the three event query classes have a really different shape. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removes empty strings from the list so when you join with ,, you don't get , <empty> ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explain nitpick? I thought I updated the rest to follow this format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, trends. Will update, then all 3 will look the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, filter with None is basically a truthy filter: https://docs.python.org/3/library/functions.html#filter

Copy link
Contributor

@macobo macobo Aug 26, 2021

Choose a reason for hiding this comment

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

Please wait for #5705 - this change already created a bunch of nasty merge conflicts and rebasing seemingly broke everything.

Worth keeping other WIP in mind when multiple people working on similar things.

@EDsCODE EDsCODE mentioned this pull request Sep 6, 2021
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.

4 participants