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

fix: allow creating lifecycle static cohorts #15729

Closed
wants to merge 1 commit into from
Closed

fix: allow creating lifecycle static cohorts #15729

wants to merge 1 commit into from

Conversation

psykomal
Copy link
Contributor

Problem

Fixes #15554 . Create static cohort from lifecycle insight using persons modal
This page mentions create cohort using this feature - https://posthog.com/docs/product-analytics/lifecycle

Changes

Added LifecycleActors and LifecycleFilter classes. Used in api/cohort.py::insert_cohort_actors_into_ch

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

How did you test this code?

Manually creating cohorts in lifecycle insight
Added tests

@psykomal psykomal changed the title lifecycle static cohort create + tests fix: lifecycle static cohort create + tests May 25, 2023
@psykomal psykomal force-pushed the lifecycle-static-cohort branch 2 times, most recently from 93276cd to a798902 Compare May 25, 2023 17:11
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@psykomal
Copy link
Contributor Author

psykomal commented Jun 2, 2023

@liyiy Could you take a look at this or add the right POC for a review. It fixes #15554

@posthog-bot posthog-bot removed the stale label Jun 5, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@neilkakkar neilkakkar reopened this Jun 23, 2023
neilkakkar
neilkakkar suggested changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@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.

Hey @psykomal - sorry it took a while for me to get to this!

This is mostly great! - Although there seems to be some duplication going on here. Have a look at PathActors and compare it to LifecycleActors. This place: https://github.com/PostHog/posthog/blob/master/posthog/api/person.py#L665 - shouldn't be separate from the new LifecycleActor class you're generating.

I'd recommend combining these into one, so there's only one code path to get persons for lifecycle, which is used by both cohorts & person querying.

neilkakkar
neilkakkar reviewed Jun 23, 2023
View reviewed changes
posthog/queries/trends/lifecycle_actors.py
if target_date is None:
raise ValidationError("Must include specified target date")

return (
Copy link
Contributor

@neilkakkar neilkakkar Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

when you reconcile the two, you'll find that you can use LIFECYCLE_PEOPLE_SQL sql from https://github.com/PostHog/posthog/blob/master/posthog/queries/trends/lifecycle.py#L61 here

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of re-writing the SQL here

@posthog-bot posthog-bot removed the stale label Jun 26, 2023
@psykomal psykomal force-pushed the lifecycle-static-cohort branch from a798902 to 9cf2a52 Compare June 26, 2023 09:45
@psykomal psykomal changed the title fix: lifecycle static cohort create + tests draft: lifecycle static cohort create + tests Jun 26, 2023
@psykomal psykomal changed the title draft: lifecycle static cohort create + tests wip: lifecycle static cohort create + tests Jun 26, 2023
@psykomal psykomal force-pushed the lifecycle-static-cohort branch 3 times, most recently from 3491014 to 07c1cc9 Compare June 26, 2023 11:01
@psykomal psykomal changed the title wip: lifecycle static cohort create + tests fix: lifecycle static cohort create + tests Jun 26, 2023
@psykomal
Copy link
Contributor Author

@neilkakkar Have made the changes. Do have a look and let me know.

neilkakkar
neilkakkar reviewed Jun 26, 2023
View reviewed changes
posthog/queries/trends/lifecycle.py Outdated
@@ -31,6 +27,8 @@


class Lifecycle:
# actor_query_class = LifecycleActors
Copy link
Contributor

Choose a reason for hiding this comment

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

debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, removed.

neilkakkar
neilkakkar approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@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.

one comment, looks good otherwise!

@psykomal psykomal force-pushed the lifecycle-static-cohort branch from 07c1cc9 to f005773 Compare June 26, 2023 12:52
@psykomal
Copy link
Contributor Author

@neilkakkar also should I run ./bin/tests to update snapshots for the failing tests or is there another way?

@neilkakkar
Copy link
Contributor

That's the correct way. (./bin/tests posthog/models/filters/test/test_filter.py::TestDjangoPropertiesToQ::test_array_property_as_string_on_persons ) Just merge master though & it should go away 👀

@psykomal psykomal force-pushed the lifecycle-static-cohort branch from f005773 to 3b56506 Compare June 26, 2023 14:16
@psykomal psykomal force-pushed the lifecycle-static-cohort branch from 3b56506 to 34f8226 Compare June 26, 2023 15:04
@psykomal
Copy link
Contributor Author

@neilkakkar updated snapshots. Please have a look. Not sure if I'm missing something but just merging didn't change anything.

neilkakkar
neilkakkar reviewed Jun 26, 2023
View reviewed changes
posthog/api/test/dashboards/__snapshots__/test_dashboard.ambr
@@ -2575,6 +2615,7 @@
"posthog_team"."ingested_event",
"posthog_team"."autocapture_opt_out",
"posthog_team"."autocapture_exceptions_opt_in",
"posthog_team"."autocapture_exceptions_errors_to_ignore",
Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @pauldambra was this left out somehow by the job in the last PR?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I've seen this recently with some dashboard changes where snapshots in master seemed to be "out of sync" (for want of a better word) with PRs

#16143

I didn't make time to track down the issue

@neilkakkar neilkakkar mentioned this pull request Jun 26, 2023
Merged
@neilkakkar neilkakkar changed the title fix: lifecycle static cohort create + tests fix: allow creating lifecycle static cohorts Jun 28, 2023
@neilkakkar
Copy link
Contributor

Seems like I can't merge this, will merge the duplicate I have here: #16248 - it still references your commit though so should get attributed properly :)

@neilkakkar neilkakkar closed this Jun 28, 2023
@neilkakkar
Copy link
Contributor

Thanks again for contributing! :D

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

@pauldambra pauldambra pauldambra left review comments

@neilkakkar neilkakkar neilkakkar approved these changes

@EDsCODE EDsCODE Awaiting requested review from EDsCODE

@liyiy liyiy Awaiting requested review from liyiy

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

Successfully merging this pull request may close these issues.

[Bug] Save as cohort on lifecycle persons doesn't work
4 participants
@psykomal @posthog-bot @neilkakkar @pauldambra