-
Notifications
You must be signed in to change notification settings - Fork 138
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: Add customization to add all person profile properties as setPersonPropertiesForFlags #1517
Changes from all commits
93f95cc
133f040
ff47140
ed87333
da79af1
04016d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
import { uuidv7 } from '../../uuidv7' | ||
import { createPosthogInstance } from '../helpers/posthog-instance' | ||
import { setAllPersonProfilePropertiesAsPersonPropertiesForFlags } from '../../customizations/setAllPersonProfilePropertiesAsPersonPropertiesForFlags' | ||
import { STORED_PERSON_PROPERTIES_KEY } from '../../constants' | ||
|
||
jest.mock('../../utils/globals', () => { | ||
const orig = jest.requireActual('../../utils/globals') | ||
const mockURLGetter = jest.fn() | ||
const mockReferrerGetter = jest.fn() | ||
return { | ||
...orig, | ||
mockURLGetter, | ||
mockReferrerGetter, | ||
userAgent: 'Mozilla/5.0 (Android 4.4; Mobile; rv:41.0) Gecko/41.0 Firefox/41.0', | ||
navigator: { | ||
vendor: '', | ||
}, | ||
document: { | ||
...orig.document, | ||
createElement: (...args: any[]) => orig.document.createElement(...args), | ||
get referrer() { | ||
return mockReferrerGetter() | ||
}, | ||
get URL() { | ||
return mockURLGetter() | ||
}, | ||
}, | ||
get location() { | ||
const url = mockURLGetter() | ||
return { | ||
href: url, | ||
toString: () => url, | ||
} | ||
}, | ||
} | ||
}) | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-require-imports | ||
const { mockURLGetter, mockReferrerGetter } = require('../../utils/globals') | ||
|
||
describe('setAllPersonPropertiesForFlags', () => { | ||
beforeEach(() => { | ||
mockReferrerGetter.mockReturnValue('https://referrer.com') | ||
mockURLGetter.mockReturnValue('https://example.com?utm_source=foo') | ||
}) | ||
|
||
it('should called setPersonPropertiesForFlags with all saved properties that are used for person properties', async () => { | ||
// arrange | ||
const token = uuidv7() | ||
const posthog = await createPosthogInstance(token) | ||
|
||
// act | ||
setAllPersonProfilePropertiesAsPersonPropertiesForFlags(posthog) | ||
|
||
// assert | ||
expect(posthog.persistence?.props[STORED_PERSON_PROPERTIES_KEY]).toMatchInlineSnapshot(` | ||
Object { | ||
"$browser": "Mobile Safari", | ||
"$browser_version": null, | ||
"$current_url": "https://example.com?utm_source=foo", | ||
"$device_type": "Mobile", | ||
"$os": "Android", | ||
"$os_version": "4.4.0", | ||
"$referrer": "https://referrer.com", | ||
"$referring_domain": "referrer.com", | ||
"dclid": null, | ||
"fbclid": null, | ||
"gad_source": null, | ||
"gbraid": null, | ||
"gclid": null, | ||
"gclsrc": null, | ||
"igshid": null, | ||
"li_fat_id": null, | ||
"mc_cid": null, | ||
"msclkid": null, | ||
"rdt_cid": null, | ||
"ttclid": null, | ||
"twclid": null, | ||
"utm_campaign": null, | ||
"utm_content": null, | ||
"utm_medium": null, | ||
"utm_source": "foo", | ||
"utm_term": null, | ||
"wbraid": null, | ||
} | ||
`) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
import { version } from '../package.json' | ||
import packageInfo from '../package.json' | ||
|
||
// overridden in posthog-core, | ||
// e.g. Config.DEBUG = Config.DEBUG || instance.config.debug | ||
const Config = { | ||
DEBUG: false, | ||
LIB_VERSION: version, | ||
LIB_VERSION: packageInfo.version, | ||
} | ||
|
||
export default Config |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { PostHog } from '../posthog-core' | ||
import { CAMPAIGN_PARAMS, EVENT_TO_PERSON_PROPERTIES, Info } from '../utils/event-utils' | ||
import { each, extend, includes } from '../utils' | ||
|
||
export const setAllPersonProfilePropertiesAsPersonPropertiesForFlags = (posthog: PostHog): void => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could also do with a comment IMHO, why would I want to do this? |
||
const allProperties = extend({}, Info.properties(), Info.campaignParams(), Info.referrerInfo()) | ||
const personProperties: Record<string, string> = {} | ||
each(allProperties, function (v, k: string) { | ||
if (includes(CAMPAIGN_PARAMS, k) || includes(EVENT_TO_PERSON_PROPERTIES, k)) { | ||
personProperties[k] = v | ||
} | ||
}) | ||
|
||
posthog.setPersonPropertiesForFlags(personProperties) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,25 @@ export const CAMPAIGN_PARAMS = [ | |
'rdt_cid', // reddit | ||
] | ||
|
||
export const EVENT_TO_PERSON_PROPERTIES = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could do with a comment or a better name... I think it's saying "these are event properties that we will copy to person properties" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't disagree but it's copied directly from plugin server with that name, I'll add a comment though! |
||
// mobile params | ||
'$app_build', | ||
'$app_name', | ||
'$app_namespace', | ||
'$app_version', | ||
Comment on lines
+36
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are many more params than that such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those other ones aren't converted into person properties though, or are they? This list comes straight from plugin-server. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't check so the plugin-server should be up to date and all good. |
||
// web params | ||
'$browser', | ||
'$browser_version', | ||
'$device_type', | ||
'$current_url', | ||
'$pathname', | ||
'$os', | ||
'$os_name', // $os_name is a special case, it's treated as an alias of $os! | ||
'$os_version', | ||
'$referring_domain', | ||
'$referrer', | ||
] | ||
|
||
export const Info = { | ||
campaignParams: function (customParams?: string[]): Record<string, string> { | ||
if (!document) { | ||
|
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 got a warning about this, so I fixed it as a drive-by
Should not import the named export 'version' (imported as 'version') from default-exporting module (only default export is available soon)