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.

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: swap the default to identified_only #1468

Merged
merged 11 commits into from
Oct 17, 2024
106 changes: 103 additions & 3 deletions src/__tests__/personProcessing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { createPosthogInstance } from './helpers/posthog-instance'
import { uuidv7 } from '../uuidv7'
import { logger } from '../utils/logger'
import { INITIAL_CAMPAIGN_PARAMS, INITIAL_REFERRER_INFO } from '../constants'
import { DecideResponse } from '../types'

jest.mock('../utils/logger')

Expand Down Expand Up @@ -44,18 +45,23 @@ describe('person processing', () => {
mockURLGetter.mockReturnValue('https://example.com?utm_source=foo')
})

const setup = async (person_profiles: 'always' | 'identified_only' | 'never' | undefined, token?: string) => {
const setup = async (
person_profiles: 'always' | 'identified_only' | 'never' | undefined,
token?: string,
persistence_name?: string
) => {
token = token || uuidv7()
const onCapture = jest.fn()
const posthog = await createPosthogInstance(token, {
_onCapture: onCapture,
person_profiles,
persistence_name,
})
return { token, onCapture, posthog }
}

describe('init', () => {
it("should default to 'always' person_profiles", async () => {
it("should default to 'identified_only' person_profiles", async () => {
// arrange
const token = uuidv7()

Expand All @@ -65,7 +71,7 @@ describe('person processing', () => {
})

// assert
expect(posthog.config.person_profiles).toEqual('always')
expect(posthog.config.person_profiles).toEqual('identified_only')
})
it('should read person_profiles from init config', async () => {
// arrange
Expand Down Expand Up @@ -547,4 +553,98 @@ describe('person processing', () => {
expect(onCapture.mock.calls[2][1].properties.$process_person_profile).toEqual(false)
})
})

describe('persistence', () => {
it('should remember that a user set the mode to always on a previous visit', async () => {
// arrange
const persistenceName = uuidv7()
const { posthog: posthog1, onCapture: onCapture1 } = await setup('always', undefined, persistenceName)
posthog1.capture('custom event 1')
const { posthog: posthog2, onCapture: onCapture2 } = await setup(
'identified_only',
undefined,
persistenceName
)

// act
posthog2.capture('custom event 2')

// assert
expect(onCapture1.mock.calls.length).toEqual(1)
expect(onCapture2.mock.calls.length).toEqual(1)
expect(onCapture1.mock.calls[0][1].properties.$process_person_profile).toEqual(true)
expect(onCapture2.mock.calls[0][1].properties.$process_person_profile).toEqual(true)
})

it('should work when always is set on a later visit', async () => {
// arrange
const persistenceName = uuidv7()
const { posthog: posthog1, onCapture: onCapture1 } = await setup(
'identified_only',
undefined,
persistenceName
)
posthog1.capture('custom event 1')
const { posthog: posthog2, onCapture: onCapture2 } = await setup('always', undefined, persistenceName)

// act
posthog2.capture('custom event 2')

// assert
expect(onCapture1.mock.calls.length).toEqual(1)
expect(onCapture2.mock.calls.length).toEqual(1)
expect(onCapture1.mock.calls[0][1].properties.$process_person_profile).toEqual(false)
expect(onCapture2.mock.calls[0][1].properties.$process_person_profile).toEqual(true)
})
})

describe('decide', () => {
it('should change the person mode from default when decide response is handled', async () => {
// arrange
const { posthog, onCapture } = await setup(undefined)
posthog.capture('startup page view')

// act
posthog._afterDecideResponse({ defaultIdentifiedOnly: false } as DecideResponse)
posthog.capture('custom event')

// assert
expect(onCapture.mock.calls.length).toEqual(2)
expect(onCapture.mock.calls[0][1].properties.$process_person_profile).toEqual(false)
expect(onCapture.mock.calls[1][1].properties.$process_person_profile).toEqual(true)
})

it('should NOT change the person mode from user-defined when decide response is handled', async () => {
// arrange
const { posthog, onCapture } = await setup('identified_only')
posthog.capture('startup page view')

// act
posthog._afterDecideResponse({ defaultIdentifiedOnly: false } as DecideResponse)
posthog.capture('custom event')

// assert
expect(onCapture.mock.calls.length).toEqual(2)
expect(onCapture.mock.calls[0][1].properties.$process_person_profile).toEqual(false)
expect(onCapture.mock.calls[1][1].properties.$process_person_profile).toEqual(false)
})

it('should persist when the default person mode is overridden by decide', async () => {
// arrange
const persistenceName = uuidv7()
const { posthog: posthog1, onCapture: onCapture1 } = await setup(undefined, undefined, persistenceName)

// act
posthog1._afterDecideResponse({ defaultIdentifiedOnly: false } as DecideResponse)
posthog1.capture('custom event 1')
const { posthog: posthog2, onCapture: onCapture2 } = await setup(undefined, undefined, persistenceName)
posthog2.capture('custom event 2')

// assert
expect(onCapture1.mock.calls.length).toEqual(1)
expect(onCapture2.mock.calls.length).toEqual(1)
expect(onCapture1.mock.calls[0][1].properties.$process_person_profile).toEqual(true)
expect(onCapture2.mock.calls[0][1].properties.$process_person_profile).toEqual(true)
})
})
})
4 changes: 4 additions & 0 deletions src/__tests__/posthog-core.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ describe('posthog core', () => {
const { posthog, onCapture } = setup({
token,
persistence_name: token,
person_profiles: 'always',
})

// act
Expand All @@ -135,6 +136,7 @@ describe('posthog core', () => {
const { posthog: posthog1 } = setup({
token,
persistence_name: token,
person_profiles: 'always',
})
posthog1.capture(eventName, eventProperties)
mockReferrerGetter.mockReturnValue('https://referrer2.example.com/some/path')
Expand Down Expand Up @@ -165,6 +167,7 @@ describe('posthog core', () => {
const { posthog: posthog1 } = setup({
token,
persistence_name: token,
person_profiles: 'always',
})
posthog1.capture(eventName, eventProperties)
mockReferrerGetter.mockReturnValue('https://referrer2.example.com/some/path')
Expand Down Expand Up @@ -195,6 +198,7 @@ describe('posthog core', () => {
const { posthog, onCapture } = setup({
token,
persistence_name: token,
person_profiles: 'always',
})

// act
Expand Down
27 changes: 22 additions & 5 deletions src/__tests__/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Info } from '../utils/event-utils'
import { document, window } from '../utils/globals'
import { uuidv7 } from '../uuidv7'
import * as globals from '../utils/globals'
import { USER_STATE } from '../constants'
import { ENABLE_PERSON_PROCESSING, USER_STATE } from '../constants'
import { createPosthogInstance, defaultPostHog } from './helpers/posthog-instance'
import { logger } from '../utils/logger'
import { DecideResponse, PostHogConfig } from '../types'
Expand Down Expand Up @@ -345,6 +345,20 @@ describe('posthog core', () => {

expect(posthog.compression).toEqual('gzip-js')
})
it('uses defaultIdentifiedOnly from decide response', () => {
const posthog = posthogWith({})

posthog._afterDecideResponse({ defaultIdentifiedOnly: true } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('identified_only')

posthog._afterDecideResponse({ defaultIdentifiedOnly: false } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('always')
})
it('defaultIdentifiedOnly does not override person_profiles if already set', () => {
const posthog = posthogWith({ person_profiles: 'always' })
posthog._afterDecideResponse({ defaultIdentifiedOnly: true } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('always')
})

it('enables compression from decide response when only one received', () => {
const posthog = posthogWith({})
Expand Down Expand Up @@ -387,6 +401,8 @@ describe('posthog core', () => {
properties: () => ({ distinct_id: 'abc', persistent: 'prop', $is_identified: false }),
remove_event_timer: jest.fn(),
get_property: () => 'anonymous',
props: {},
register: jest.fn(),
} as unknown as PostHogPersistence,
sessionPersistence: {
properties: () => ({ distinct_id: 'abc', persistent: 'prop' }),
Expand Down Expand Up @@ -425,7 +441,7 @@ describe('posthog core', () => {
$window_id: 'windowId',
$session_id: 'sessionId',
$is_identified: false,
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand All @@ -447,7 +463,7 @@ describe('posthog core', () => {
$session_id: 'sessionId',
$lib_custom_api_host: 'https://custom.posthog.com',
$is_identified: false,
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand All @@ -465,7 +481,7 @@ describe('posthog core', () => {
posthog._calculate_event_properties('custom_event', { event: 'prop' }, new Date())[
'$process_person_profile'
]
).toEqual(true)
).toEqual(false)
})

it('only adds token and distinct_id if event_name is $snapshot', () => {
Expand Down Expand Up @@ -496,7 +512,7 @@ describe('posthog core', () => {
expect(posthog._calculate_event_properties('custom_event', { event: 'prop' }, new Date())).toEqual({
event_name: 'custom_event',
token: 'testtoken',
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand All @@ -510,6 +526,7 @@ describe('posthog core', () => {
)

posthog.persistence.get_initial_props = () => ({ initial: 'prop' })
posthog.persistence.props[ENABLE_PERSON_PROCESSING] = true // person processing is needed for $set_once
expect(posthog._calculate_set_once_properties({ key: 'prop' })).toEqual({
event_name: '$set_once',
token: undefined,
Expand Down
27 changes: 24 additions & 3 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export const defaultConfig = (): PostHogConfig => ({
bootstrap: {},
disable_compression: false,
session_idle_timeout_seconds: 30 * 60, // 30 minutes
person_profiles: 'always',
person_profiles: 'identified_only',
Copy link
Member Author

Choose a reason for hiding this comment

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

the main change for swapping the default

__add_tracing_headers: false,
})

Expand Down Expand Up @@ -272,6 +272,7 @@ export class PostHog {
decideEndpointWasHit: boolean
analyticsDefaultEndpoint: string
version = Config.LIB_VERSION
_initialPersonProfilesConfig: 'always' | 'never' | 'identified_only' | null

SentryIntegration: typeof SentryIntegration
sentryIntegration: (options?: SentryIntegrationOptions) => ReturnType<typeof sentryIntegration>
Expand All @@ -294,6 +295,7 @@ export class PostHog {
this.__loaded = false
this.analyticsDefaultEndpoint = '/e/'
this._initialPageviewCaptured = false
this._initialPersonProfilesConfig = null

this.featureFlags = new PostHogFeatureFlags(this)
this.toolbar = new Toolbar(this)
Expand Down Expand Up @@ -390,6 +392,10 @@ export class PostHog {
this.config = {} as PostHogConfig // will be set right below
this._triggered_notifs = []

if (config.person_profiles) {
this._initialPersonProfilesConfig = config.person_profiles
}
Comment on lines +395 to +397
Copy link
Member Author

Choose a reason for hiding this comment

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

in order to roll this out slowly, we need to know if the person has already set their config - that it's not just using the default. So we need to store the initial config somewhere.


this.set_config(
extend({}, defaultConfig(), configRenames(config), {
name: name,
Expand All @@ -408,6 +414,8 @@ export class PostHog {
this.config.persistence === 'sessionStorage'
? this.persistence
: new PostHogPersistence({ ...this.config, persistence: 'sessionStorage' })

// should I store the initial person profiles config in persistence?
Copy link
Member Author

Choose a reason for hiding this comment

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

question

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I've handled this so far is to save whether or not they've sent an event with person processing. If they have then we persist ENABLE_PERSON_PROCESSING to true.

I'm just wondering whether saving their config gives us anything beyond this.

Here's a couple of cases:

Case 1

  • posthog-js starts up with the user setting 'always'
  • some events are sent, with person profiles, so ENABLE_PERSON_PROCESSING is set
  • user closes the browser
  • new visit, posthog-js starts up with no setting so uses the default identified_only
  • some events are sent, as ENABLE_PERSON_PROCESSING was set then these have person profiles

Case 2

  • posthog-js starts up with the user setting 'always'
  • no events are sent, so ENABLE_PERSON_PROCESSING is not set
  • user closes the browser
  • new visit, posthog-js starts up with no setting so uses the default identified_only
  • some events are sent, these are anonymous (if we'd persisted the setting from before, they'd have person profiles)

In case 2 there's a difference, but I'd argue that those events should be sent as anonymous. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR no I don't think so :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooook so it turns out we don't actually handle this case correctly. I added a fix and some tests to this PR. I hope you don't mind me piggybacking like this, I can very much move it to its own PR if you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

No this is great, thanks!

const initialPersistenceProps = { ...this.persistence.props }
const initialSessionProps = { ...this.sessionPersistence.props }

Expand Down Expand Up @@ -538,6 +546,14 @@ export class PostHog {
this.analyticsDefaultEndpoint = response.analytics.endpoint
}

this.set_config({
person_profiles: this._initialPersonProfilesConfig
? this._initialPersonProfilesConfig
: response['defaultIdentifiedOnly']
? 'identified_only'
: 'always',
})

this.sessionRecording?.afterDecideResponse(response)
this.autocapture?.afterDecideResponse(response)
this.heatmaps?.afterDecideResponse(response)
Expand Down Expand Up @@ -977,8 +993,13 @@ export class PostHog {
properties = sanitize_properties(properties, event_name)
}

// add person processing flag as very last step, so it cannot be overridden. process_person=true is default
properties['$process_person_profile'] = this._hasPersonProcessing()
// add person processing flag as very last step, so it cannot be overridden
const hasPersonProcessing = this._hasPersonProcessing()
properties['$process_person_profile'] = hasPersonProcessing
// if the event has person processing, ensure that all future events will too, even if the setting changes
if (hasPersonProcessing) {
this._requirePersonProcessing('_calculate_event_properties')
}

return properties
}
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ export interface DecideResponse {
isAuthenticated: boolean
siteApps: { id: number; url: string }[]
heatmaps?: boolean
defaultIdentifiedOnly?: boolean
Copy link
Member Author

Choose a reason for hiding this comment

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

this will be returned with /decide

}

export type FeatureFlagsCallback = (
Expand Down
Loading