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

Funnel with Strict Step Ordering #4890

Merged
merged 40 commits into from
Jul 1, 2021
Merged

Funnel with Strict Step Ordering #4890

merged 40 commits into from
Jul 1, 2021

Conversation

neilkakkar
Copy link
Contributor

Changes

I'm thinking if it's worth going this route or refactoring these flags to be class variables. For now, this seems fine to me. Perhaps worth revisiting if our number of flags start exploding.

Will go in after #4868

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
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)
  • Breaking changes are backwards-compatible. Ensure old/new frontend requests work with new/old backends, and vice versa.

buwilliams and others added 30 commits June 18, 2021 08:48
@buwilliams
wip: pagination for persons on clickhouse funnels
537bdb3
@buwilliams
wip: added offset support for getting a list of persons; added suppor…
605f9de 
…t for conversion window;
@buwilliams
fixed mypy exception
a9a5f12
@buwilliams
helper function to insert data for local testing
e952308
@buwilliams
moved generate code into separate class for more functionality later
ceda12e
@buwilliams
corrected person_distinct_id to use the person id from postgres
1a29fdb
@buwilliams
minor corrections to generate local class along with addition of data…
1990907 
… cleanup via destroy() method
@buwilliams
reduce the number of persons who make it to each step
7daeaaa
@buwilliams
moved funnel queries to a new folder for better organization; separat…
fba259e 
…ed funnel_persons and funnel_trends_persons into individual classes;
@EDsCODE
Merge branch 'master' into funnel-persons-pagination-conversion-window
cc24758
@buwilliams
funnel persons and tests
142e7b6
@buwilliams
Merge branch 'funnel-persons-pagination-conversion-window' of github.…
df44123 
…com:PostHog/posthog into funnel-persons-pagination-conversion-window
@EDsCODE
initial implementation
589e937
@buwilliams
invoke the funnel or funnel trends class respectively
ac6d601
@EDsCODE
add a test
34d5bad
@EDsCODE
add breakdown handling and first test
b483277
@EDsCODE
add test stubs
9242414
@EDsCODE
remove repeats
3042e59
@buwilliams
mypy corrections and PR feedback
474ba42
@EDsCODE
run funnel test suite on new query implementation
4abecaf
@EDsCODE
remove imports
62f2919
@buwilliams
corrected tests
4fb5a43
@EDsCODE
minor test updates
74bfa1b
@EDsCODE
Merge branch 'funnel-persons-pagination-conversion-window' into funne…
0aa36f9 
…l-step-query-new
@EDsCODE
correct func name
5c9dcd2
@EDsCODE
fix types
5df0cad
@EDsCODE
merge master
d7d2e60
@EDsCODE
func name change
0427159
@EDsCODE
move builder functions to funnel base
b9cae01
@EDsCODE
add test classe for new funnel
7c5e606
EDsCODE and others added 3 commits June 24, 2021 21:53
@EDsCODE
Merge branch 'master' into funnel-step-query-new
49f8c92
@neilkakkar
Handle multiple same events in the funnel (#4863)
716e8af 
* dedup + tests

* deep equality. Tests to come

* write test for entity equality

* finish testing funnels

* clean up comments
@neilkakkar
add strict option for funnels
fa1fb91
@timgl timgl temporarily deployed to posthog-pr-4890 June 28, 2021 13:32 Inactive
Base automatically changed from funnel-step-query-new to master June 28, 2021 14:20
@EDsCODE
Copy link
Member

EDsCODE commented Jun 29, 2021

should merge in master to reduce the noise here

@timgl timgl temporarily deployed to posthog-pr-4890 June 29, 2021 09:29 Inactive
@timgl timgl temporarily deployed to posthog-pr-4890 July 1, 2021 08:34 Inactive
@neilkakkar neilkakkar marked this pull request as ready for review July 1, 2021 08:35
@neilkakkar neilkakkar requested review from Twixes and EDsCODE July 1, 2021 08:35
@timgl timgl temporarily deployed to posthog-pr-4890 July 1, 2021 11:03 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4890 July 1, 2021 16:29 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4890 July 1, 2021 17:01 Inactive
EDsCODE
EDsCODE approved these changes Jul 1, 2021
View reviewed changes
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.

added persons handling and adjusted query result format to handle step times

@EDsCODE EDsCODE merged commit a5b7b31 into master Jul 1, 2021
@EDsCODE EDsCODE deleted the funnel-strict branch July 1, 2021 17:14
@neilkakkar
Copy link
Contributor Author

Nice, thank you! :) I was going to do this as a separate PR since doing it too early creates lots of dependency annoyance, but this order was just right, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EDsCODE EDsCODE EDsCODE approved these changes

@Twixes Twixes Awaiting requested review from Twixes

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

Successfully merging this pull request may close these issues.
4 participants
@neilkakkar @EDsCODE @timgl @buwilliams