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(cdp): allow disabling site destinations #1563

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion playground/nextjs/src/posthog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ export const configForConsent = (): Partial<PostHogConfig> => {
export const updatePostHogConsent = (consentGiven: boolean) => {
if (consentGiven) {
localStorage.setItem('cookie_consent', 'true')
posthog.opt_in_capturing()
} else {
localStorage.removeItem('cookie_consent')
posthog.opt_out_capturing()
}

posthog.set_config(configForConsent())
Expand All @@ -49,6 +51,7 @@ if (typeof window !== 'undefined') {
session_recording: {
recordCrossOriginIframes: true,
},
opt_in_site_apps: true,
debug: true,
disable_web_experiments: false,
scroll_root_selector: ['#scroll_element', 'html'],
Expand All @@ -57,7 +60,7 @@ if (typeof window !== 'undefined') {
persistence_name: `${process.env.NEXT_PUBLIC_POSTHOG_KEY}_nextjs`,
...configForConsent(),
})

;(window as any).posthog = posthog
MarconLP marked this conversation as resolved.
Show resolved Hide resolved
// Help with debugging(window as any).posthog = posthog
}

Expand Down
125 changes: 63 additions & 62 deletions src/__tests__/site-apps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { assignableWindow } from '../utils/globals'
import { logger } from '../utils/logger'
import '../entrypoints/external-scripts-loader'
import { isFunction } from '../utils/type-utils'
import { ConsentManager } from '../consent'

jest.mock('../utils/logger', () => ({
logger: {
Expand Down Expand Up @@ -50,6 +51,11 @@ describe('SiteApps', () => {
register: (props: Properties) => posthog.persistence!.register(props),
unregister: (key: string) => posthog.persistence!.unregister(key),
get_property: (key: string) => posthog.persistence!.props[key],
consent: {
isOptedOut(): boolean {
return false
},
} as unknown as ConsentManager,
capture: jest.fn(),
_addCaptureHook: jest.fn(),
_afterDecideResponse: jest.fn(),
Expand All @@ -73,42 +79,6 @@ describe('SiteApps', () => {
})

describe('constructor', () => {
it('sets enabled to true when opt_in_site_apps is true and advanced_disable_decide is false', () => {
posthog.config = {
...defaultConfig,
opt_in_site_apps: true,
advanced_disable_decide: false,
} as PostHogConfig

siteAppsInstance = new SiteApps(posthog)

expect(siteAppsInstance.enabled).toBe(true)
})
MarconLP marked this conversation as resolved.
Show resolved Hide resolved

it('sets enabled to false when opt_in_site_apps is false', () => {
posthog.config = {
...defaultConfig,
opt_in_site_apps: false,
advanced_disable_decide: false,
} as PostHogConfig

siteAppsInstance = new SiteApps(posthog)

expect(siteAppsInstance.enabled).toBe(false)
})

it('sets enabled to false when advanced_disable_decide is true', () => {
posthog.config = {
...defaultConfig,
opt_in_site_apps: true,
advanced_disable_decide: true,
} as PostHogConfig

siteAppsInstance = new SiteApps(posthog)

expect(siteAppsInstance.enabled).toBe(false)
})

it('initializes missedInvocations, loaded, appsLoading correctly', () => {
expect(siteAppsInstance.missedInvocations).toEqual([])
expect(siteAppsInstance.loaded).toBe(false)
Expand All @@ -126,17 +96,17 @@ describe('SiteApps', () => {

describe('eventCollector', () => {
it('does nothing if enabled is false', () => {
siteAppsInstance.enabled = false
posthog.config.opt_in_site_apps = false
siteAppsInstance.eventCollector('event_name', {} as CaptureResult)

expect(siteAppsInstance.missedInvocations.length).toBe(0)
})

it('collects event if enabled and loaded is false', () => {
siteAppsInstance.enabled = true
posthog.config.opt_in_site_apps = true
siteAppsInstance.loaded = false

const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as CaptureResult
const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as unknown as CaptureResult

jest.spyOn(siteAppsInstance, 'globalsForEvent').mockReturnValue({ some: 'globals' })

Expand All @@ -147,12 +117,12 @@ describe('SiteApps', () => {
})

it('trims missedInvocations to last 990 when exceeding 1000', () => {
siteAppsInstance.enabled = true
posthog.config.opt_in_site_apps = true
siteAppsInstance.loaded = false

siteAppsInstance.missedInvocations = new Array(1000).fill({})

const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as CaptureResult
const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as unknown as CaptureResult

jest.spyOn(siteAppsInstance, 'globalsForEvent').mockReturnValue({ some: 'globals' })

Expand Down Expand Up @@ -223,20 +193,22 @@ describe('SiteApps', () => {
})

describe('afterDecideResponse', () => {
it('sets loaded to true and enabled to false when response is undefined', () => {
siteAppsInstance.afterDecideResponse(undefined)
it('sets loaded to true response is undefined', () => {
const response = {
siteApps: [],
} as DecideResponse
siteAppsInstance.afterDecideResponse(response)

expect(siteAppsInstance.loaded).toBe(true)
expect(siteAppsInstance.enabled).toBe(false)
expect(siteAppsInstance._decideServerResponse.length).toBe(0)
})

it('loads site apps when enabled and opt_in_site_apps is true', (done) => {
posthog.config.opt_in_site_apps = true
siteAppsInstance.enabled = true
const response = {
siteApps: [
{ id: '1', url: '/site_app/1' },
{ id: '2', url: '/site_app/2' },
{ id: '1', type: 'site_app', url: '/site_app/1' },
{ id: '2', type: 'site_app', url: '/site_app/2' },
],
} as DecideResponse

Expand All @@ -255,25 +227,22 @@ describe('SiteApps', () => {
})

it('does not load site apps when enabled is false', () => {
siteAppsInstance.enabled = false
posthog.config.opt_in_site_apps = false
const response = {
siteApps: [{ id: '1', url: '/site_app/1' }],
siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)

expect(siteAppsInstance.loaded).toBe(true)
expect(siteAppsInstance.enabled).toBe(false)
expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).not.toHaveBeenCalled()
})

it('clears missedInvocations when all apps are loaded', (done) => {
posthog.config.opt_in_site_apps = true
siteAppsInstance.enabled = true
siteAppsInstance.missedInvocations = [{ some: 'data' }]
const response = {
siteApps: [{ id: '1', url: '/site_app/1' }],
siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)
Expand All @@ -288,28 +257,26 @@ describe('SiteApps', () => {

it('sets assignableWindow properties for each site app', () => {
posthog.config.opt_in_site_apps = true
siteAppsInstance.enabled = true
const response = {
siteApps: [{ id: '1', url: '/site_app/1' }],
siteApps: [{ id: '8', type: 'site_app', url: '/site_app/8' }],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)

expect(assignableWindow['__$$ph_site_app_1_posthog']).toBe(posthog)
expect(typeof assignableWindow['__$$ph_site_app_1_missed_invocations']).toBe('function')
expect(typeof assignableWindow['__$$ph_site_app_1_callback']).toBe('function')
expect(assignableWindow['__$$ph_site_app_8_posthog']).toBe(posthog)
expect(typeof assignableWindow['__$$ph_site_app_8_missed_invocations']).toBe('function')
expect(typeof assignableWindow['__$$ph_site_app_8_callback']).toBe('function')
expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).toHaveBeenCalledWith(
posthog,
'/site_app/1',
'/site_app/8',
expect.any(Function)
)
})

it('logs error if site apps are disabled but response contains site apps', () => {
posthog.config.opt_in_site_apps = false
siteAppsInstance.enabled = false
const response = {
siteApps: [{ id: '1', url: '/site_app/1' }],
siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)
Expand All @@ -321,7 +288,6 @@ describe('SiteApps', () => {
})

it('sets loaded to true if response.siteApps is empty', () => {
siteAppsInstance.enabled = true
posthog.config.opt_in_site_apps = true
const response = {
siteApps: [],
Expand All @@ -330,7 +296,42 @@ describe('SiteApps', () => {
siteAppsInstance.afterDecideResponse(response)

expect(siteAppsInstance.loaded).toBe(true)
expect(siteAppsInstance.enabled).toBe(false)
})

it('does not load site destinations if consent is not given', () => {
posthog.config.opt_in_site_apps = true
posthog.consent.isOptedOut = () => true
const response = {
siteApps: [
{ id: '5', type: 'site_app', url: '/site_app/5' },
{ id: '6', type: 'site_destination', url: '/site_app/6' },
],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)

expect(typeof assignableWindow['__$$ph_site_app_5_callback']).toBe('function')
expect(typeof assignableWindow['__$$ph_site_app_6_callback']).toBe('undefined')
})

it('load site destinations if consent is given at a later time', () => {
posthog.config.opt_in_site_apps = true
posthog.consent.isOptedOut = () => true
const response = {
siteApps: [
{ id: '5', type: 'site_app', url: '/site_app/5' },
{ id: '6', type: 'site_destination', url: '/site_app/6' },
],
} as DecideResponse

siteAppsInstance.afterDecideResponse(response)

posthog.consent.isOptedOut = () => false

siteAppsInstance.loadIfEnabled()

expect(typeof assignableWindow['__$$ph_site_app_5_callback']).toBe('function')
expect(typeof assignableWindow['__$$ph_site_app_6_callback']).toBe('function')
MarconLP marked this conversation as resolved.
Show resolved Hide resolved
})
})
})
3 changes: 2 additions & 1 deletion src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ export class PostHog {
this.scrollManager = new ScrollManager(this)
this.pageViewManager = new PageViewManager(this)
this.surveys = new PostHogSurveys(this)
this.siteApps = new SiteApps(this)
this.experiments = new WebExperiments(this)
this.exceptions = new PostHogExceptions(this)
this.rateLimiter = new RateLimiter(this)
Expand Down Expand Up @@ -434,7 +435,6 @@ export class PostHog {

new TracingHeaders(this).startIfEnabledOrStop()

this.siteApps = new SiteApps(this)
this.siteApps?.init()

this.sessionRecording = new SessionRecording(this)
Expand Down Expand Up @@ -1828,6 +1828,7 @@ export class PostHog {
this.autocapture?.startIfEnabled()
this.heatmaps?.startIfEnabled()
this.surveys.loadIfEnabled()
this.siteApps?.loadIfEnabled()
this._sync_opt_out_with_persistence()
}
}
Expand Down
47 changes: 31 additions & 16 deletions src/site-apps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,15 @@ import { PostHog } from './posthog-core'
import { CaptureResult, DecideResponse } from './types'
import { assignableWindow } from './utils/globals'
import { logger } from './utils/logger'
import { isArray } from './utils/type-utils'
import { isArray, isUndefined } from './utils/type-utils'

export class SiteApps {
instance: PostHog
enabled: boolean
_decideServerResponse?: DecideResponse['siteApps']
missedInvocations: Record<string, any>[]
loaded: boolean
appsLoading: Set<string>

constructor(instance: PostHog) {
this.instance = instance
// can't use if site apps are disabled, or if we're not asking /decide for site apps
this.enabled = !!this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide
constructor(private readonly instance: PostHog) {
// events captured between loading posthog-js and the site app; up to 1000 events
this.missedInvocations = []
// capture events until loaded
Expand All @@ -23,7 +19,9 @@ export class SiteApps {
}

eventCollector(_eventName: string, eventPayload?: CaptureResult | undefined) {
if (!this.enabled) {
// can't use if site apps are disabled, or if we're not asking /decide for site apps
const enabled = this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide
if (!enabled) {
return
}
if (!this.loaded && eventPayload) {
Expand Down Expand Up @@ -74,18 +72,33 @@ export class SiteApps {
return globals
}

afterDecideResponse(response?: DecideResponse): void {
if (isArray(response?.siteApps) && response.siteApps.length > 0) {
if (this.enabled && this.instance.config.opt_in_site_apps) {
loadIfEnabled() {
if (
this._decideServerResponse &&
isArray(this._decideServerResponse) &&
this._decideServerResponse.length > 0
) {
// can't use if site apps are disabled, or if we're not asking /decide for site apps
const enabled = this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide
if (enabled) {
const checkIfAllLoaded = () => {
// Stop collecting events once all site apps are loaded
if (this.appsLoading.size === 0) {
this.loaded = true
this.missedInvocations = []
}
}
for (const { id, url } of response['siteApps']) {
// TODO: if we have opted out and "type" is "site_destination", ignore it... but do include "site_app" types
for (const { id, type, url } of this._decideServerResponse) {
// if consent isn't given, skip site destinations
if (this.instance.consent.isOptedOut() && type === 'site_destination') {
checkIfAllLoaded()
continue
}
// if the site app is already loaded, skip it
if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) {
checkIfAllLoaded()
continue
}
MarconLP marked this conversation as resolved.
Show resolved Hide resolved
this.appsLoading.add(id)
assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance
assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations
Expand All @@ -101,17 +114,19 @@ export class SiteApps {
}
})
}
} else if (response['siteApps'].length > 0) {
} else if (this._decideServerResponse.length > 0) {
logger.error('PostHog site apps are disabled. Enable the "opt_in_site_apps" config to proceed.')
this.loaded = true
} else {
this.loaded = true
}
} else {
this.loaded = true
this.enabled = false
}
}

// TODO: opting out of stuff should disable this
afterDecideResponse(response: DecideResponse): void {
this._decideServerResponse = response['siteApps']
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable _decideServerResponse makes me think this contains the whole response, not just the siteApps key

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to "_decideServerSiteAppsResponse"

this.loadIfEnabled()
}
}
2 changes: 1 addition & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ export interface DecideResponse {
editorParams?: ToolbarParams /** @deprecated, renamed to toolbarParams, still present on older API responses */
toolbarVersion: 'toolbar' /** @deprecated, moved to toolbarParams */
isAuthenticated: boolean
siteApps: { id: string; url: string }[]
siteApps: { id: string; type: string; url: string }[]
heatmaps?: boolean
defaultIdentifiedOnly?: boolean
captureDeadClicks?: boolean
Expand Down
Loading