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.

Sign up for GitHub

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: Load surveys via RemoteConfig #1652

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+139 −140
Open

feat: Load surveys via RemoteConfig #1652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d1f2514
Fix survey loading
benjackwhite Jan 15, 2025
5a196bf
Fixes
benjackwhite Jan 15, 2025
10dc861
Revert
benjackwhite Jan 15, 2025
5357c3b
revert
benjackwhite Jan 15, 2025
8807aa8
Fixes
benjackwhite Jan 15, 2025
5d7d4d4
Fix some things
benjackwhite Jan 15, 2025
9ff477d
Fixes
benjackwhite Jan 15, 2025
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
141 changes: 64 additions & 77 deletions src/__tests__/surveys.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '../extensions/surveys/surveys-utils'
import { PostHogPersistence } from '../posthog-persistence'
import { PostHog } from '../posthog-core'
import { DecideResponse, PostHogConfig, Properties } from '../types'
import { DecideResponse, PostHogConfig, Properties, RemoteConfig } from '../types'
import { window } from '../utils/globals'
import { RequestRouter } from '../utils/request-router'
import { assignableWindow } from '../utils/globals'
Expand All @@ -27,7 +27,6 @@ describe('surveys', () => {
let config: PostHogConfig
let instance: PostHog
let surveys: PostHogSurveys
let surveysResponse: { status?: number; surveys?: Survey[] }
const originalWindowLocation = assignableWindow.location

const decideResponse = {
Expand All @@ -51,20 +50,6 @@ describe('surveys', () => {
} as unknown as Survey,
]

const secondSurveys: Survey[] = [
{
name: 'first survey',
description: 'first survey description',
type: SurveyType.Popover,
questions: [{ type: SurveyQuestionType.Open, question: 'what is a bokoblin?' }],
} as unknown as Survey,
{
name: 'second survey',
description: 'second survey description',
type: SurveyType.Popover,
questions: [{ type: SurveyQuestionType.Open, question: 'what is a moblin?' }],
} as unknown as Survey,
]
const surveysWithEvents: Survey[] = [
{
name: 'first survey',
Expand Down Expand Up @@ -160,9 +145,17 @@ describe('surveys', () => {
} as unknown as Survey,
]

beforeEach(() => {
surveysResponse = { surveys: firstSurveys }
const setSurveys = (_surveys: Survey[]) => {
instance.persistence?.clear()
instance._send_request = jest
.fn()
.mockImplementation(({ callback }) => callback({ statusCode: 200, json: { surveys: _surveys } }))
surveys.onRemoteConfig({
surveys: _surveys,
} as RemoteConfig)
}

beforeEach(() => {
const loadScriptMock = jest.fn()

loadScriptMock.mockImplementation((_ph, _path, callback) => {
Expand All @@ -189,7 +182,7 @@ describe('surveys', () => {
get_property: (key: string) => instance.persistence?.props[key],
_send_request: jest
.fn()
.mockImplementation(({ callback }) => callback({ statusCode: 200, json: surveysResponse })),
.mockImplementation(({ callback }) => callback({ statusCode: 200, json: { surveys: [] } })),
featureFlags: {
_send_request: jest
.fn()
Expand All @@ -210,6 +203,8 @@ describe('surveys', () => {
// all being squashed into a mock posthog so...
instance.getActiveMatchingSurveys = instance.surveys.getActiveMatchingSurveys.bind(instance.surveys)

setSurveys(firstSurveys)

Object.defineProperty(window, 'location', {
configurable: true,
enumerable: true,
Expand All @@ -230,6 +225,10 @@ describe('surveys', () => {
})

it('getSurveys gets a list of surveys if not present already', () => {
// Clear RemoteConfig surveys
surveys['_loadedSurveys'] = undefined
instance.persistence?.clear()

surveys.getSurveys((data) => {
expect(data).toEqual(firstSurveys)
})
Expand All @@ -241,7 +240,6 @@ describe('surveys', () => {
expect(instance._send_request).toHaveBeenCalledTimes(1)
expect(instance.persistence?.props.$surveys).toEqual(firstSurveys)

surveysResponse = { surveys: secondSurveys }
surveys.getSurveys((data) => {
expect(data).toEqual(firstSurveys)
})
Expand All @@ -260,42 +258,44 @@ describe('surveys', () => {
})

it('getSurveys registers the survey event receiver if a survey has events', () => {
surveysResponse = { surveys: surveysWithEvents }
setSurveys(surveysWithEvents)
surveys.getSurveys((data) => {
expect(data).toEqual(surveysWithEvents)
}, true)
})

const registry = surveys._surveyEventReceiver?.getEventToSurveys()
const registry = surveys['_surveyEventReceiver']?.getEventToSurveys()
expect(registry.has('user_subscribed')).toBeTruthy()
expect(registry.get('user_subscribed')).toEqual(['first-survey', 'third-survey'])

expect(registry.has('address_changed')).toBeTruthy()
expect(registry.get('address_changed')).toEqual(['third-survey'])
})

it('getSurveys force reloads when called with true', () => {
surveys.getSurveys((data) => {
expect(data).toEqual(firstSurveys)
})
expect(instance._send_request).toHaveBeenCalledWith({
url: 'https://us.i.posthog.com/api/surveys/?token=testtoken',
method: 'GET',
callback: expect.any(Function),
})
expect(instance._send_request).toHaveBeenCalledTimes(1)
expect(instance.persistence?.props.$surveys).toEqual(firstSurveys)

surveysResponse = { surveys: secondSurveys }

surveys.getSurveys((data) => {
expect(data).toEqual(secondSurveys)
}, true)
expect(instance.persistence?.props.$surveys).toEqual(secondSurveys)
expect(instance._send_request).toHaveBeenCalledTimes(2)
})
// it('getSurveys force reloads when called with true', () => {
// surveys.getSurveys((data) => {
// expect(data).toEqual(firstSurveys)
// })
// expect(instance._send_request).toHaveBeenCalledWith({
// url: 'https://us.i.posthog.com/api/surveys/?token=testtoken',
// method: 'GET',
// callback: expect.any(Function),
// })
// expect(instance._send_request).toHaveBeenCalledTimes(1)
// expect(instance.persistence?.props.$surveys).toEqual(firstSurveys)

// setSurveys(secondSurveys)

// surveys.getSurveys((data) => {
// expect(data).toEqual(secondSurveys)
// })
// expect(instance.persistence?.props.$surveys).toEqual(secondSurveys)
// expect(instance._send_request).toHaveBeenCalledTimes(2)
// })

it('getSurveys returns empty array if surveys are undefined', () => {
surveysResponse = { status: 0 }
instance.persistence?.clear()
surveys['_loadedSurveys'] = undefined
instance._send_request = jest.fn().mockImplementation(({ callback }) => callback({ statusCode: 0 }))
surveys.getSurveys((data) => {
expect(data).toEqual([])
})
Expand Down Expand Up @@ -509,17 +509,15 @@ describe('surveys', () => {
} as unknown as Survey

it('returns surveys that are active', () => {
surveysResponse = { surveys: [draftSurvey, activeSurvey, completedSurvey] }
setSurveys([draftSurvey, activeSurvey, completedSurvey])

surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([activeSurvey])
})
})

it('returns surveys based on url and selector matching', () => {
surveysResponse = {
surveys: [surveyWithUrl, surveyWithSelector, surveyWithUrlAndSelector],
}
setSurveys([surveyWithUrl, surveyWithSelector, surveyWithUrlAndSelector])
// eslint-disable-next-line compat/compat
assignableWindow.location = new URL('https://posthog.com') as unknown as Location
surveys.getActiveMatchingSurveys((data) => {
Expand Down Expand Up @@ -550,15 +548,13 @@ describe('surveys', () => {
})

it('returns surveys based on url with urlMatchType settings', () => {
surveysResponse = {
surveys: [
surveyWithRegexUrl,
surveyWithParamRegexUrl,
surveyWithWildcardRouteUrl,
surveyWithWildcardSubdomainUrl,
surveyWithExactUrlMatch,
],
}
setSurveys([
surveyWithRegexUrl,
surveyWithParamRegexUrl,
surveyWithWildcardRouteUrl,
surveyWithWildcardSubdomainUrl,
surveyWithExactUrlMatch,
])

const originalWindowLocation = assignableWindow.location
// eslint-disable-next-line compat/compat
Expand Down Expand Up @@ -598,9 +594,7 @@ describe('surveys', () => {
})

it('returns surveys based on exclusion conditions', () => {
surveysResponse = {
surveys: [surveyWithUrlDoesNotContain, surveyWithIsNotUrlMatch, surveyWithUrlDoesNotContainRegex],
}
setSurveys([surveyWithUrlDoesNotContain, surveyWithIsNotUrlMatch, surveyWithUrlDoesNotContainRegex])

// eslint-disable-next-line compat/compat
assignableWindow.location = new URL('https://posthog.com') as unknown as Location
Expand Down Expand Up @@ -628,51 +622,44 @@ describe('surveys', () => {
})

it('returns surveys that match linked and targeting feature flags', () => {
surveysResponse = { surveys: [activeSurvey, surveyWithFlags, surveyWithEverything] }
setSurveys([activeSurvey, surveyWithFlags, surveyWithEverything])
surveys.getActiveMatchingSurveys((data) => {
// active survey is returned because it has no flags aka there are no restrictions on flag enabled for it
expect(data).toEqual([activeSurvey, surveyWithFlags])
})
})

it('does not return surveys that have flag keys but no matching flags', () => {
surveysResponse = { surveys: [surveyWithFlags, surveyWithUnmatchedFlags] }
setSurveys([surveyWithFlags, surveyWithUnmatchedFlags])
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([surveyWithFlags])
})
})

it('returns surveys that match internal feature flags', () => {
surveysResponse = {
surveys: [surveyWithEnabledInternalFlag, surveyWithDisabledInternalFlag],
}
setSurveys([surveyWithEnabledInternalFlag, surveyWithDisabledInternalFlag])
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([surveyWithEnabledInternalFlag])
})
})

it('does not return event based surveys that didnt observe an event', () => {
surveysResponse = {
surveys: [surveyWithEnabledInternalFlag, surveyWithEvents],
}
setSurveys([surveyWithEnabledInternalFlag, surveyWithEvents])
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([surveyWithEnabledInternalFlag])
})
})

it('returns event based surveys that observed an event', () => {
// TODO this test fails when run in isolation

surveysResponse = {
surveys: [surveyWithEnabledInternalFlag, surveyWithEvents],
}
;(surveys._surveyEventReceiver as any)?.on('user_subscribed')
setSurveys([surveyWithEnabledInternalFlag, surveyWithEvents])
;(surveys['_surveyEventReceiver'] as any)?.onEvent('user_subscribed')
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([surveyWithEnabledInternalFlag])
expect(data).toEqual([surveyWithEnabledInternalFlag, surveyWithEvents])
})
})
it('does not return surveys that have internal flag keys but no matching internal flags', () => {
surveysResponse = { surveys: [surveyWithEnabledInternalFlag, surveyWithDisabledInternalFlag] }
setSurveys([surveyWithEnabledInternalFlag, surveyWithDisabledInternalFlag])
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([surveyWithEnabledInternalFlag])
})
Expand All @@ -682,15 +669,15 @@ describe('surveys', () => {
// eslint-disable-next-line compat/compat
assignableWindow.location = new URL('https://posthogapp.com') as unknown as Location
document.body.appendChild(document.createElement('div')).className = 'test-selector'
surveysResponse = { surveys: [activeSurvey, surveyWithSelector, surveyWithEverything] }
setSurveys([activeSurvey, surveyWithSelector, surveyWithEverything])
// activeSurvey returns because there are no restrictions on conditions or flags on it
surveys.getActiveMatchingSurveys((data) => {
expect(data).toEqual([activeSurvey, surveyWithSelector, surveyWithEverything])
})
})

it('returns only surveys with enabled feature flags', () => {
surveysResponse = { surveys: surveysWithFeatureFlagKeys }
setSurveys(surveysWithFeatureFlagKeys)

surveys.getActiveMatchingSurveys((data) => {
// Should include:
Expand Down
8 changes: 4 additions & 4 deletions src/extensions/surveys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export class SurveyManager {
)
}

public callSurveysAndEvaluateDisplayLogic = (forceReload: boolean = false): void => {
public callSurveysAndEvaluateDisplayLogic = (): void => {
this.posthog?.getActiveMatchingSurveys((surveys) => {
const nonAPISurveys = surveys.filter((survey) => survey.type !== 'api')

Expand Down Expand Up @@ -240,7 +240,7 @@ export class SurveyManager {
this.handlePopoverSurvey(survey)
}
})
}, forceReload)
})
}

private addSurveyToFocus = (id: string): void => {
Expand Down Expand Up @@ -353,11 +353,11 @@ export function generateSurveys(posthog: PostHog) {
}

const surveyManager = new SurveyManager(posthog)
surveyManager.callSurveysAndEvaluateDisplayLogic(true)
surveyManager.callSurveysAndEvaluateDisplayLogic()

// recalculate surveys every second to check if URL or selectors have changed
setInterval(() => {
surveyManager.callSurveysAndEvaluateDisplayLogic(false)
surveyManager.callSurveysAndEvaluateDisplayLogic()
}, 1000)
return surveyManager
}
Expand Down
8 changes: 4 additions & 4 deletions src/posthog-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,13 +1285,13 @@ export class PostHog {
}

/** Get list of all surveys. */
getSurveys(callback: SurveyCallback, forceReload = false): void {
this.surveys.getSurveys(callback, forceReload)
getSurveys(callback: SurveyCallback): void {
Copy link
Member

@marandaneto marandaneto Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Removing forceReload from public APIs will break customers that are using the methods directly.
Is there a reason to remove the forceReload?

It'll contradict whats written here:

Surveys are requested on first load and then cached by default by the JavaScript SDK. If you want to force an API call to get an updated list of surveys, pass true to the forceReload parameter. You only need to do this if you want changed surveys to appear mid-session for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ewww - I wish we hadn't allowed people to do that tbh. But I can add it back in. We have a bug currently that causes multiple loads anyways

Copy link
Member

Choose a reason for hiding this comment

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

I mean it's useful that people do not have to call the API directly, since the "API" mode is one of the supported presentation mode, so yeah I'd add it back, fixing the bug is the extra sauce :D

this.surveys.getSurveys(callback)
}

/** Get surveys that should be enabled for the current user. */
getActiveMatchingSurveys(callback: SurveyCallback, forceReload = false): void {
this.surveys.getActiveMatchingSurveys(callback, forceReload)
getActiveMatchingSurveys(callback: SurveyCallback): void {
this.surveys.getActiveMatchingSurveys(callback)
}

/** Render a survey on a specific element. */
Expand Down
2 changes: 1 addition & 1 deletion src/posthog-featureflags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ export class PostHogFeatureFlags {

if (!this._decideCalled) {
this._decideCalled = true
this.instance._onRemoteConfig(response.json ?? {})
this.instance._onRemoteConfig((response.json as DecideResponse) ?? {})
}

if (data.disable_flags) {
Expand Down
Loading
Loading