-
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
fix: use distinct ids for cohort exports #23929
fix: use distinct ids for cohort exports #23929
Conversation
Hey @slshults, this is my first time contributing. How do I get eyes on this for review? Tagging you because you created the issue. Thanks! |
Hey @derek-li , Very cool, thank you! I've requested a review from one of our product analytics engineers, @anirudhpillai . |
Btw, @derek-li , could you please send me an email, steven @ posthog.com ? I'd like to send you some credit for our merch store! 🙂 |
@@ -54,7 +54,9 @@ export async function startDownload( | |||
) | |||
} | |||
if (exportContext['columns'].includes('person')) { | |||
exportContext['columns'] = exportContext['columns'].map((c: string) => (c === 'person' ? 'person.id' : c)) | |||
exportContext['columns'] = exportContext['columns'].map((c: string) => |
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 a lot for the changes Derek!
Any reason you name the column 'person.distinct_ids.0' over person.distinct_id
?
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.
@anirudhpillai distinct_id
does not exist as a property of person
. The back end is doing some special logic (not 100% clear on what's happening) to query all distinct_ids
for a user, but csvs are not great at handling lists which is why I pick the first one
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. The first column won't always show the first distinct id on the UI. It will sometimes show the email...., the logic for that is here. Example is the test data locally
But this is still useful functionality to have the distinct id there so let's get this in.
Co-authored-by: Anirudh Pillai <[email protected]>
Problem
Fixes #23462
When on a cohort page, clicking export results in duplicate columns
Changes
ids are now consistent:
Does this work well for both Cloud and self-hosted?
n/a
How did you test this code?
Checked new export files against in-app table contents