diff --git a/src/__tests__/surveys.test.ts b/src/__tests__/surveys.test.ts index 1a465f73d..ce143f69a 100644 --- a/src/__tests__/surveys.test.ts +++ b/src/__tests__/surveys.test.ts @@ -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' @@ -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 = { @@ -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', @@ -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) => { @@ -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() @@ -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, @@ -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) }) @@ -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) }) @@ -260,12 +258,12 @@ 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']) @@ -273,29 +271,31 @@ describe('surveys', () => { 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([]) }) @@ -509,7 +509,7 @@ 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]) @@ -517,9 +517,7 @@ describe('surveys', () => { }) 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) => { @@ -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 @@ -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 @@ -628,7 +622,7 @@ 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]) @@ -636,25 +630,21 @@ describe('surveys', () => { }) 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]) }) @@ -662,17 +652,14 @@ describe('surveys', () => { 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]) }) @@ -682,7 +669,7 @@ 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]) @@ -690,7 +677,7 @@ describe('surveys', () => { }) it('returns only surveys with enabled feature flags', () => { - surveysResponse = { surveys: surveysWithFeatureFlagKeys } + setSurveys(surveysWithFeatureFlagKeys) surveys.getActiveMatchingSurveys((data) => { // Should include: diff --git a/src/extensions/surveys.tsx b/src/extensions/surveys.tsx index 402da1bb4..4c6501c26 100644 --- a/src/extensions/surveys.tsx +++ b/src/extensions/surveys.tsx @@ -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') @@ -240,7 +240,7 @@ export class SurveyManager { this.handlePopoverSurvey(survey) } }) - }, forceReload) + }) } private addSurveyToFocus = (id: string): void => { @@ -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 } diff --git a/src/posthog-core.ts b/src/posthog-core.ts index ab30d98a6..3a571a64c 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -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 { + 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. */ diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 0ec31e9f0..e3f8e2234 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -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) { diff --git a/src/posthog-surveys.ts b/src/posthog-surveys.ts index 3d3c8a488..cfc1d03c3 100644 --- a/src/posthog-surveys.ts +++ b/src/posthog-surveys.ts @@ -12,7 +12,7 @@ import { SurveyEventReceiver } from './utils/survey-event-receiver' import { assignableWindow, document, window } from './utils/globals' import { RemoteConfig } from './types' import { createLogger } from './utils/logger' -import { isNullish } from './utils/type-utils' +import { isArray, isNullish } from './utils/type-utils' import { getSurveySeenStorageKeys } from './extensions/surveys/surveys-utils' const logger = createLogger('[Surveys]') @@ -59,18 +59,21 @@ function getRatingBucketForResponseValue(responseValue: number, scale: number) { } export class PostHogSurveys { - private _decideServerResponse?: boolean - public _surveyEventReceiver: SurveyEventReceiver | null + private _surveysEnabledRemotely?: boolean + private _surveyEventReceiver: SurveyEventReceiver private _surveyManager: any + private _loadedSurveys?: Survey[] constructor(private readonly instance: PostHog) { - // we set this to undefined here because we need the persistence storage for this type - // but that's not initialized until loadIfEnabled is called. - this._surveyEventReceiver = null + this._surveyEventReceiver = new SurveyEventReceiver(this.instance) } onRemoteConfig(response: RemoteConfig) { - this._decideServerResponse = !!response['surveys'] + if (isArray(response.surveys)) { + this.onSurveys(response.surveys) + } else { + this._surveysEnabledRemotely = !!response['surveys'] + } this.loadIfEnabled() } @@ -83,76 +86,84 @@ export class PostHogSurveys { loadIfEnabled() { const surveysGenerator = assignableWindow?.__PosthogExtensions__?.generateSurveys - if (!this.instance.config.disable_surveys && this._decideServerResponse && !surveysGenerator) { - if (this._surveyEventReceiver == null) { - this._surveyEventReceiver = new SurveyEventReceiver(this.instance) - } - + if (!this.instance.config.disable_surveys && this._surveysEnabledRemotely && !surveysGenerator) { assignableWindow.__PosthogExtensions__?.loadExternalDependency?.(this.instance, 'surveys', (err) => { if (err) { return logger.error('Could not load surveys script', err) } + logger.info('surveys loaded') + this._surveyManager = assignableWindow.__PosthogExtensions__?.generateSurveys?.(this.instance) }) } } - getSurveys(callback: SurveyCallback, forceReload = false) { + private reloadSurveys(callback: SurveyCallback) { + logger.info('Reloading surveys') + + // TODO: Move this to RemoteConfig loading instead (or just remove entirely) + + this.instance._send_request({ + url: this.instance.requestRouter.endpointFor('api', `/api/surveys/?token=${this.instance.config.token}`), + method: 'GET', + callback: (response) => { + if (response.statusCode !== 200 || !response.json) { + return callback([]) + } + + const surveys = response.json.surveys || [] + + this.onSurveys(surveys) + + return callback(surveys) + }, + }) + } + + private onSurveys(surveys: Survey[]) { + this._loadedSurveys = surveys + this._surveysEnabledRemotely = surveys.length > 0 + + const eventOrActionBasedSurveys = surveys.filter( + (survey: Survey) => + (survey.conditions?.events && + survey.conditions?.events?.values && + survey.conditions?.events?.values?.length > 0) || + (survey.conditions?.actions && + survey.conditions?.actions?.values && + survey.conditions?.actions?.values?.length > 0) + ) + + if (eventOrActionBasedSurveys.length > 0) { + this._surveyEventReceiver?.register(eventOrActionBasedSurveys) + } + + this.instance.persistence?.register({ [SURVEYS]: surveys }) + } + + getSurveys(callback: SurveyCallback) { // In case we manage to load the surveys script, but config says not to load surveys // then we shouldn't return survey data if (this.instance.config.disable_surveys) { return callback([]) } - if (this._surveyEventReceiver == null) { - this._surveyEventReceiver = new SurveyEventReceiver(this.instance) - } - - const existingSurveys = this.instance.get_property(SURVEYS) - - if (!existingSurveys || forceReload) { - this.instance._send_request({ - url: this.instance.requestRouter.endpointFor( - 'api', - `/api/surveys/?token=${this.instance.config.token}` - ), - method: 'GET', - callback: (response) => { - if (response.statusCode !== 200 || !response.json) { - return callback([]) - } - const surveys = response.json.surveys || [] - - const eventOrActionBasedSurveys = surveys.filter( - (survey: Survey) => - (survey.conditions?.events && - survey.conditions?.events?.values && - survey.conditions?.events?.values?.length > 0) || - (survey.conditions?.actions && - survey.conditions?.actions?.values && - survey.conditions?.actions?.values?.length > 0) - ) - - if (eventOrActionBasedSurveys.length > 0) { - this._surveyEventReceiver?.register(eventOrActionBasedSurveys) - } + const existingSurveys: Survey[] | undefined = + this._loadedSurveys ?? (this.instance.get_property(SURVEYS) as Survey[]) ?? undefined - this.instance.persistence?.register({ [SURVEYS]: surveys }) - return callback(surveys) - }, - }) + if (!existingSurveys) { + return this.reloadSurveys(callback) } else { return callback(existingSurveys) } } - getActiveMatchingSurveys(callback: SurveyCallback, forceReload = false) { + getActiveMatchingSurveys(callback: SurveyCallback) { this.getSurveys((surveys) => { const activeSurveys = surveys.filter((survey) => { return !!(survey.start_date && !survey.end_date) }) - const conditionMatchedSurveys = activeSurveys.filter((survey) => { if (!survey.conditions) { return true @@ -214,7 +225,7 @@ export class PostHogSurveys { }) return callback(targetingMatchedSurveys) - }, forceReload) + }) } checkFlags(survey: Survey): boolean { diff --git a/src/types.ts b/src/types.ts index 6c059b5ae..0ab6c8400 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,6 +1,7 @@ import { PostHog } from './posthog-core' import type { SegmentAnalytics } from './extensions/segment-integration' import { recordOptions } from './extensions/replay/sessionrecording-utils' +import { Survey } from './posthog-surveys-types' export type Property = any export type Properties = Record @@ -547,7 +548,7 @@ export interface RemoteConfig { urlBlocklist?: SessionRecordingUrlTrigger[] eventTriggers?: string[] } - surveys?: boolean + surveys?: boolean | Survey[] toolbarParams: ToolbarParams editorParams?: ToolbarParams /** @deprecated, renamed to toolbarParams, still present on older API responses */ toolbarVersion: 'toolbar' /** @deprecated, moved to toolbarParams */ @@ -559,7 +560,7 @@ export interface RemoteConfig { hasFeatureFlags?: boolean // Indicates if the team has any flags enabled (if not we don't need to load them) } -export interface DecideResponse extends RemoteConfig { +export interface DecideResponse extends Omit { featureFlags: Record featureFlagPayloads: Record errorsWhileComputingFlags: boolean