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.

Sign up for GitHub

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

Jump to bottom

feat(person-on-events): Person on events funnels #9822

Merged
merged 110 commits into from
May 25, 2022
Merged

feat(person-on-events): Person on events funnels #9822

merged 110 commits into from
May 25, 2022

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented May 17, 2022 •
edited
Loading

Problem

  • adds new code path for person on events funnels

  • group property tests

  • use using_person_on_events pattern

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

EDsCODE and others added 30 commits May 4, 2022 18:46
@EDsCODE
add constance setting
9dec8ec
@EDsCODE
add minimum viable code
e0a5519
@EDsCODE
update journeys_for
19250ba
@EDsCODE
working test case
1b8616a
@EDsCODE
chore: remove eventserializer in unnecessary places
e50541b
@EDsCODE
change execution
b6501f0
@EDsCODE
change imports
de8179f
@EDsCODE
fix typing and tests
87a62ad
@EDsCODE
change format
dfcb658
@EDsCODE
adjust
59e7cbc
@EDsCODE
reset
9f9ac5d
@EDsCODE
Merge branch 'refactor-eventserializer' into person-on-events-trends
347dcbb
@EDsCODE
Merge branch 'master' into refactor-eventserializer
602c967
@EDsCODE
Merge branch 'refactor-eventserializer' into person-on-events-trends
7d8d518
@EDsCODE
split group path in trend breakdown
9f2fb9a
@EDsCODE
fix: typing
1126c78
@EDsCODE
fix test
83b2ac6
@neilkakkar
fix test, rename
992e887
@neilkakkar
automatically add persons to events in tests on flush
e93f3e5
@neilkakkar
test CI with person-on-events
0c7fb10
@neilkakkar
wip
33a6235
@neilkakkar
fix
1d70928
@neilkakkar
fix ci
a7aeef4
@neilkakkar
merge master
6b4a263
@neilkakkar
add comma
a209368
@neilkakkar
reduce test explosion
79a06a3
@neilkakkar
fix CH server image version #9743
f17b33b
@neilkakkar
update conditional
6155027
@neilkakkar
split person on events
a38c21c
@neilkakkar
split person on events
1bfbf41
@neilkakkar
Copy link
Collaborator

Not sure about this bit: #9822 (comment) (a test around this would be noice) - but LGTM!

@EDsCODE EDsCODE requested a review from neilkakkar May 24, 2022 23:31
neilkakkar
neilkakkar approved these changes May 25, 2022
View reviewed changes
Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Excellent work!

Did a final pass with few nits + realised that format_action_filter() for trend action breakdown was a bit borked (always going for pdi.distinct_id) so fixed that + added a test.

@neilkakkar
Copy link
Collaborator

.. now to see if my changes bork some other tests 🙈

@neilkakkar neilkakkar merged commit 52f68a9 into master May 25, 2022
@neilkakkar neilkakkar deleted the person-on-events-funnels branch May 25, 2022 12:19
macobo pushed a commit that referenced this pull request May 26, 2022
@EDsCODE @macobo
feat(person-on-events): Person on events funnels (#9822)
4b092ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilkakkar neilkakkar neilkakkar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@EDsCODE @macobo @neilkakkar