-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Save app properties and others to Person from events #17393
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be consistent with both params (i.e. make campaignParams also do $last_
, can't think of a reason why these would be different & as a user I'd find that confusing.
Otherwise this seems good, but we might need to do a few things before we can ship it:
- make sure we don't break folks's existing insights <- they might be using browser etc (I think there's a way to query that ... Tim did it for something else similar)
- maybe some user communication that Joe/marketing can help us with 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to comment ... test failures
Regarding 1 - this change doesn't break that. It has been broken a long time so I don't think we need to be concerned about it. 2 makes sense if we rename the campaign params but couldn't we do that after ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
maybe some user communication that Joe/marketing can help us with 🤷♀️
As it is I think the changelog is a good place for something additive like this...
I think we could add $last_blah for campaign params rather than renaming and breaking things or needing to migrate them... but then you'd have both which is arguably more confusing for a user.
As a compromise you could not add the $last_ to things here so that the $initial_ is the only qualifier but I don't have strong feelings on what's better there
No-one has time to revisit things at PostHog & migrating/user communication several times is extremely annoying, so why don't we just make the change and add it to the release notes like we want to have it in the future. If that causes problems for some users with old properties / queries for the last without last etc, then we can just script to update their person properties accordingly ... and do it once & be done with it. Edit: rethinking this let's go with Paul's idea
|
posthog/frontend/src/lib/taxonomy.tsx Line 63 in bee0b07
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇♀️ Thanks for making the code more readable too :)
Problem
Fixes PostHog/posthog-js-lite#102
We already have
$intial_X
for some properties but it happens that people want to filter or configure flags based on the initial values or also the latest value.Changes
$X
to the $set properties in addition to the$initial_X
Not sure if there would be any downstream impact in terms of higher number of properties stored but chatted about it in Slack and we seemed to agree it makes sense as an approach
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?
Tests