From a14ec15bd4d61d633c3e0eec2adee9a400d787f5 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Tue, 7 Jun 2022 13:34:49 -0300 Subject: [PATCH 1/3] remove syncTaskComposite for code simplicity --- src/sync/submitters/submitterManager.ts | 20 +++++++++++++++++-- src/sync/syncTaskComposite.ts | 26 ------------------------- 2 files changed, 18 insertions(+), 28 deletions(-) delete mode 100644 src/sync/syncTaskComposite.ts diff --git a/src/sync/submitters/submitterManager.ts b/src/sync/submitters/submitterManager.ts index 523e5ab5..7bd99dde 100644 --- a/src/sync/submitters/submitterManager.ts +++ b/src/sync/submitters/submitterManager.ts @@ -1,4 +1,3 @@ -import { syncTaskComposite } from '../syncTaskComposite'; import { eventsSubmitterFactory } from './eventsSubmitter'; import { impressionsSubmitterFactory } from './impressionsSubmitter'; import { impressionCountsSubmitterFactory } from './impressionCountsSubmitter'; @@ -17,5 +16,22 @@ export function submitterManagerFactory(params: ISdkFactoryContextSync) { const telemetrySubmitter = telemetrySubmitterFactory(params); if (telemetrySubmitter) submitters.push(telemetrySubmitter); - return syncTaskComposite(submitters); + + return { + start() { + submitters.forEach(submitter => submitter.start()); + }, + stop() { + submitters.forEach(submitter => submitter.stop()); + }, + isRunning() { + return submitters.some(submitter => submitter.isRunning()); + }, + execute() { + return Promise.all(submitters.map(submitter => submitter.execute())); + }, + isExecuting() { + return submitters.some(submitter => submitter.isExecuting()); + } + }; } diff --git a/src/sync/syncTaskComposite.ts b/src/sync/syncTaskComposite.ts deleted file mode 100644 index b16a1704..00000000 --- a/src/sync/syncTaskComposite.ts +++ /dev/null @@ -1,26 +0,0 @@ -import { ISyncTask } from './types'; - -/** - * Composite Sync Task: group of sync tasks that are treated as a single one. - */ -export function syncTaskComposite(syncTasks: ISyncTask[]): ISyncTask { - - return { - start() { - syncTasks.forEach(syncTask => syncTask.start()); - }, - stop() { - syncTasks.forEach(syncTask => syncTask.stop()); - }, - isRunning() { - return syncTasks.some(syncTask => syncTask.isRunning()); - }, - execute() { - return Promise.all(syncTasks.map(syncTask => syncTask.execute())); - }, - isExecuting() { - return syncTasks.some(syncTask => syncTask.isExecuting()); - } - }; - -} From b625f42a8bcdde0c8122fbe5a84471e195b6ca31 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Tue, 7 Jun 2022 16:08:56 -0300 Subject: [PATCH 2/3] implementation and test updates --- src/consent/__tests__/sdkUserConsent.spec.ts | 14 ++++----- src/consent/sdkUserConsent.ts | 7 +++-- src/listeners/__tests__/browser.spec.ts | 5 ++-- src/listeners/browser.ts | 14 +++++---- src/sync/__tests__/syncManagerOnline.spec.ts | 31 ++++++++++++++------ src/sync/submitters/submitterManager.ts | 19 +++++++----- src/sync/submitters/types.ts | 7 +++++ src/sync/syncManagerOnline.ts | 11 ++++--- src/sync/types.ts | 3 +- 9 files changed, 69 insertions(+), 42 deletions(-) diff --git a/src/consent/__tests__/sdkUserConsent.spec.ts b/src/consent/__tests__/sdkUserConsent.spec.ts index 706b6ca9..e7981871 100644 --- a/src/consent/__tests__/sdkUserConsent.spec.ts +++ b/src/consent/__tests__/sdkUserConsent.spec.ts @@ -4,7 +4,7 @@ import { fullSettings } from '../../utils/settingsValidation/__tests__/settings. test('createUserConsentAPI', () => { const settings = { ...fullSettings, userConsent: 'UNKNOWN' }; - const syncManager = { submitter: syncTaskFactory() }; + const syncManager = { submitterManager: syncTaskFactory() }; const storage = { events: { clear: jest.fn() }, impressions: { clear: jest.fn() } @@ -20,15 +20,15 @@ test('createUserConsentAPI', () => { // setting user consent to 'GRANTED' expect(props.setStatus(true)).toBe(true); expect(props.setStatus(true)).toBe(true); // calling again has no affect - expect(syncManager.submitter.start).toBeCalledTimes(1); // submitter resumed - expect(syncManager.submitter.stop).toBeCalledTimes(0); + expect(syncManager.submitterManager.start).toBeCalledTimes(1); // submitter resumed + expect(syncManager.submitterManager.stop).toBeCalledTimes(0); expect(props.getStatus()).toBe(props.Status.GRANTED); // setting user consent to 'DECLINED' expect(props.setStatus(false)).toBe(true); expect(props.setStatus(false)).toBe(true); // calling again has no affect - expect(syncManager.submitter.start).toBeCalledTimes(1); - expect(syncManager.submitter.stop).toBeCalledTimes(1); // submitter paused + expect(syncManager.submitterManager.start).toBeCalledTimes(1); + expect(syncManager.submitterManager.stop).toBeCalledTimes(1); // submitter paused expect(props.getStatus()).toBe(props.Status.DECLINED); expect(storage.events.clear).toBeCalledTimes(1); // storage tracked data dropped expect(storage.impressions.clear).toBeCalledTimes(1); @@ -39,7 +39,7 @@ test('createUserConsentAPI', () => { expect(props.setStatus(undefined)).toBe(false); expect(props.setStatus({})).toBe(false); - expect(syncManager.submitter.start).toBeCalledTimes(1); - expect(syncManager.submitter.stop).toBeCalledTimes(1); + expect(syncManager.submitterManager.start).toBeCalledTimes(1); + expect(syncManager.submitterManager.stop).toBeCalledTimes(1); expect(props.getStatus()).toBe(props.Status.DECLINED); }); diff --git a/src/consent/sdkUserConsent.ts b/src/consent/sdkUserConsent.ts index ac8af3d8..e8f12156 100644 --- a/src/consent/sdkUserConsent.ts +++ b/src/consent/sdkUserConsent.ts @@ -34,9 +34,10 @@ export function createUserConsentAPI(params: ISdkFactoryContext) { settings.userConsent = newConsentStatus; if (consent) { // resumes submitters if transitioning to GRANTED - syncManager?.submitter?.start(); - } else { // pauses submitters and drops tracked data if transitioning to DECLINED - syncManager?.submitter?.stop(); + syncManager?.submitterManager?.start(); + } else { // pauses submitters (except telemetry), and drops tracked data if transitioning to DECLINED + syncManager?.submitterManager?.stop(true); + // @ts-ignore, clear method is present in storage for standalone and partial consumer mode if (events.clear) events.clear(); // @ts-ignore if (impressions.clear) impressions.clear(); diff --git a/src/listeners/__tests__/browser.spec.ts b/src/listeners/__tests__/browser.spec.ts index 4657d24e..a73194d3 100644 --- a/src/listeners/__tests__/browser.spec.ts +++ b/src/listeners/__tests__/browser.spec.ts @@ -258,11 +258,12 @@ test('Browser JS listener / standalone mode / user consent status', () => { settings.userConsent = 'DECLINED'; triggerUnloadEvent(); - // Unload event was triggered when user consent was unknown and declined. Thus sendBeacon and post services should not be called - expect(global.window.navigator.sendBeacon).toBeCalledTimes(0); + // Unload event was triggered when user consent was unknown and declined. Thus sendBeacon and post services should be called only for telemetry + expect(global.window.navigator.sendBeacon).toBeCalledTimes(2); expect(fakeSplitApi.postTestImpressionsBulk).not.toBeCalled(); expect(fakeSplitApi.postEventsBulk).not.toBeCalled(); expect(fakeSplitApi.postTestImpressionsCount).not.toBeCalled(); + (global.window.navigator.sendBeacon as jest.Mock).mockClear(); settings.userConsent = 'GRANTED'; triggerUnloadEvent(); diff --git a/src/listeners/browser.ts b/src/listeners/browser.ts index faf5c956..134de65e 100644 --- a/src/listeners/browser.ts +++ b/src/listeners/browser.ts @@ -67,7 +67,7 @@ export class BrowserSignalListener implements ISignalListener { flushData() { if (!this.syncManager) return; // In consumer mode there is not sync manager and data to flush - // Flush data if there is user consent + // Flush impressions & events data if there is user consent if (isConsentGranted(this.settings)) { const eventsUrl = this.settings.urls.events; const extraMetadata = { @@ -78,11 +78,13 @@ export class BrowserSignalListener implements ISignalListener { this._flushData(eventsUrl + '/testImpressions/beacon', this.storage.impressions, this.serviceApi.postTestImpressionsBulk, this.fromImpressionsCollector, extraMetadata); this._flushData(eventsUrl + '/events/beacon', this.storage.events, this.serviceApi.postEventsBulk); if (this.storage.impressionCounts) this._flushData(eventsUrl + '/testImpressions/count/beacon', this.storage.impressionCounts, this.serviceApi.postTestImpressionsCount, fromImpressionCountsCollector); - if (this.storage.telemetry) { - const telemetryUrl = this.settings.urls.telemetry; - const telemetryCacheAdapter = telemetryCacheStatsAdapter(this.storage.telemetry, this.storage.splits, this.storage.segments); - this._flushData(telemetryUrl + '/v1/metrics/usage/beacon', telemetryCacheAdapter, this.serviceApi.postMetricsUsage); - } + } + + // Flush telemetry data + if (this.storage.telemetry) { + const telemetryUrl = this.settings.urls.telemetry; + const telemetryCacheAdapter = telemetryCacheStatsAdapter(this.storage.telemetry, this.storage.splits, this.storage.segments); + this._flushData(telemetryUrl + '/v1/metrics/usage/beacon', telemetryCacheAdapter, this.serviceApi.postMetricsUsage); } // Close streaming connection diff --git a/src/sync/__tests__/syncManagerOnline.spec.ts b/src/sync/__tests__/syncManagerOnline.spec.ts index c47cad86..08b6bcaa 100644 --- a/src/sync/__tests__/syncManagerOnline.spec.ts +++ b/src/sync/__tests__/syncManagerOnline.spec.ts @@ -14,29 +14,42 @@ test('syncManagerOnline should start or not the submitter depending on user cons // @ts-ignore const syncManager = syncManagerOnlineFactory()({ settings }); - const submitter = syncManager.submitter!; + const submitterManager = syncManager.submitterManager!; syncManager.start(); - expect(submitter.start).toBeCalledTimes(1); // Submitter should be started if userConsent is undefined + expect(submitterManager.start).toBeCalledTimes(1); + expect(submitterManager.start).lastCalledWith(false); // SubmitterManager should start all submitters, if userConsent is undefined syncManager.stop(); - expect(submitter.stop).toBeCalledTimes(1); + expect(submitterManager.stop).toBeCalledTimes(1); settings.userConsent = 'UNKNOWN'; syncManager.start(); - expect(submitter.start).toBeCalledTimes(1); // Submitter should not be started if userConsent is unknown + expect(submitterManager.start).toBeCalledTimes(2); + expect(submitterManager.start).lastCalledWith(true); // SubmitterManager should start only telemetry submitter, if userConsent is unknown syncManager.stop(); - expect(submitter.stop).toBeCalledTimes(2); + expect(submitterManager.stop).toBeCalledTimes(2); + syncManager.flush(); + expect(submitterManager.execute).toBeCalledTimes(1); + expect(submitterManager.execute).lastCalledWith(true); // SubmitterManager should flush only telemetry, if userConsent is unknown settings.userConsent = 'GRANTED'; syncManager.start(); - expect(submitter.start).toBeCalledTimes(2); // Submitter should be started if userConsent is granted + expect(submitterManager.start).toBeCalledTimes(3); + expect(submitterManager.start).lastCalledWith(false); // SubmitterManager should start all submitters, if userConsent is granted syncManager.stop(); - expect(submitter.stop).toBeCalledTimes(3); + expect(submitterManager.stop).toBeCalledTimes(3); + syncManager.flush(); + expect(submitterManager.execute).toBeCalledTimes(2); + expect(submitterManager.execute).lastCalledWith(false); // SubmitterManager should flush all submitters, if userConsent is granted settings.userConsent = 'DECLINED'; syncManager.start(); - expect(submitter.start).toBeCalledTimes(2); // Submitter should not be started if userConsent is declined + expect(submitterManager.start).toBeCalledTimes(4); + expect(submitterManager.start).lastCalledWith(true); // SubmitterManager should start only telemetry submitter, if userConsent is declined syncManager.stop(); - expect(submitter.stop).toBeCalledTimes(4); + expect(submitterManager.stop).toBeCalledTimes(4); + syncManager.flush(); + expect(submitterManager.execute).toBeCalledTimes(3); + expect(submitterManager.execute).lastCalledWith(true); // SubmitterManager should flush only telemetry, if userConsent is unknown }); diff --git a/src/sync/submitters/submitterManager.ts b/src/sync/submitters/submitterManager.ts index 7bd99dde..7ec32bdb 100644 --- a/src/sync/submitters/submitterManager.ts +++ b/src/sync/submitters/submitterManager.ts @@ -3,8 +3,9 @@ import { impressionsSubmitterFactory } from './impressionsSubmitter'; import { impressionCountsSubmitterFactory } from './impressionCountsSubmitter'; import { telemetrySubmitterFactory } from './telemetrySubmitter'; import { ISdkFactoryContextSync } from '../../sdkFactory/types'; +import { ISubmitterManager } from './types'; -export function submitterManagerFactory(params: ISdkFactoryContextSync) { +export function submitterManagerFactory(params: ISdkFactoryContextSync): ISubmitterManager { const submitters = [ impressionsSubmitterFactory(params), @@ -14,21 +15,23 @@ export function submitterManagerFactory(params: ISdkFactoryContextSync) { const impressionCountsSubmitter = impressionCountsSubmitterFactory(params); if (impressionCountsSubmitter) submitters.push(impressionCountsSubmitter); const telemetrySubmitter = telemetrySubmitterFactory(params); - if (telemetrySubmitter) submitters.push(telemetrySubmitter); - return { - start() { - submitters.forEach(submitter => submitter.start()); + start(onlyTelemetry?: boolean) { + if (!onlyTelemetry) submitters.forEach(submitter => submitter.start()); + if (telemetrySubmitter) telemetrySubmitter.start(); }, - stop() { + stop(allExceptTelemetry?: boolean) { submitters.forEach(submitter => submitter.stop()); + if (!allExceptTelemetry && telemetrySubmitter) telemetrySubmitter.stop(); }, isRunning() { return submitters.some(submitter => submitter.isRunning()); }, - execute() { - return Promise.all(submitters.map(submitter => submitter.execute())); + execute(onlyTelemetry?: boolean) { + const promises = onlyTelemetry ? [] : submitters.map(submitter => submitter.execute()); + if (telemetrySubmitter) promises.push(telemetrySubmitter.execute()); + return Promise.all(promises); }, isExecuting() { return submitters.some(submitter => submitter.isExecuting()); diff --git a/src/sync/submitters/types.ts b/src/sync/submitters/types.ts index f4eb8c7b..fe9fac26 100644 --- a/src/sync/submitters/types.ts +++ b/src/sync/submitters/types.ts @@ -1,5 +1,6 @@ import { IMetadata } from '../../dtos/types'; import { SplitIO } from '../../types'; +import { ISyncTask } from '../types'; export type ImpressionsPayload = { /** Split name */ @@ -191,3 +192,9 @@ export type TelemetryConfigStatsPayload = TelemetryConfigStats & { i?: Array, // integrations uC: number, // userConsent } + +export interface ISubmitterManager extends ISyncTask { + start(onlyTelemetry?: boolean): void, + stop(allExceptTelemetry?: boolean): void, + execute(onlyTelemetry?: boolean): Promise +} diff --git a/src/sync/syncManagerOnline.ts b/src/sync/syncManagerOnline.ts index 61f0603d..c4cd4ee5 100644 --- a/src/sync/syncManagerOnline.ts +++ b/src/sync/syncManagerOnline.ts @@ -40,7 +40,7 @@ export function syncManagerOnlineFactory( /** Submitter Manager */ // It is not inyected as push and polling managers, because at the moment it is required - const submitter = submitterManagerFactory(params); + const submitterManager = submitterManagerFactory(params); /** Sync Manager logic */ @@ -79,7 +79,7 @@ export function syncManagerOnlineFactory( // E.g.: user consent, app state changes (Page hide, Foreground/Background, Online/Offline). pollingManager, pushManager, - submitter, + submitterManager, /** * Method used to start the syncManager for the first time, or resume it after being stopped. @@ -102,7 +102,7 @@ export function syncManagerOnlineFactory( } // start periodic data recording (events, impressions, telemetry). - if (isConsentGranted(settings)) submitter.start(); + submitterManager.start(!isConsentGranted(settings)); }, /** @@ -116,7 +116,7 @@ export function syncManagerOnlineFactory( if (pollingManager && pollingManager.isRunning()) pollingManager.stop(); // stop periodic data recording (events, impressions, telemetry). - submitter.stop(); + submitterManager.stop(); }, isRunning() { @@ -124,8 +124,7 @@ export function syncManagerOnlineFactory( }, flush() { - if (isConsentGranted(settings)) return submitter.execute(); - else return Promise.resolve(); + return submitterManager.execute(!isConsentGranted(settings)); }, // [Only used for client-side] diff --git a/src/sync/types.ts b/src/sync/types.ts index 22f51eb9..81727ca9 100644 --- a/src/sync/types.ts +++ b/src/sync/types.ts @@ -2,6 +2,7 @@ import { IReadinessManager } from '../readiness/types'; import { IStorageSync } from '../storages/types'; import { IPollingManager } from './polling/types'; import { IPushManager } from './streaming/types'; +import { ISubmitterManager } from './submitters/types'; export interface ITask { /** @@ -39,7 +40,7 @@ export interface ISyncManager extends ITask { flush(): Promise, pushManager?: IPushManager, pollingManager?: IPollingManager, - submitter?: ISyncTask + submitterManager?: ISubmitterManager } export interface ISyncManagerCS extends ISyncManager { From 164270ae43fa8624233d65f90247b5e9380cc8f7 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 15 Jun 2022 17:28:33 -0300 Subject: [PATCH 3/3] updated some comments --- src/sync/submitters/submitterManager.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/sync/submitters/submitterManager.ts b/src/sync/submitters/submitterManager.ts index 7ec32bdb..298f61a4 100644 --- a/src/sync/submitters/submitterManager.ts +++ b/src/sync/submitters/submitterManager.ts @@ -17,22 +17,29 @@ export function submitterManagerFactory(params: ISdkFactoryContextSync): ISubmit const telemetrySubmitter = telemetrySubmitterFactory(params); return { + // `onlyTelemetry` true if SDK is created with userConsent not GRANTED start(onlyTelemetry?: boolean) { if (!onlyTelemetry) submitters.forEach(submitter => submitter.start()); if (telemetrySubmitter) telemetrySubmitter.start(); }, + + // `allExceptTelemetry` true if userConsent is changed to DECLINED stop(allExceptTelemetry?: boolean) { submitters.forEach(submitter => submitter.stop()); if (!allExceptTelemetry && telemetrySubmitter) telemetrySubmitter.stop(); }, + isRunning() { return submitters.some(submitter => submitter.isRunning()); }, + + // Flush data. Called with `onlyTelemetry` true if SDK is destroyed with userConsent not GRANTED execute(onlyTelemetry?: boolean) { const promises = onlyTelemetry ? [] : submitters.map(submitter => submitter.execute()); if (telemetrySubmitter) promises.push(telemetrySubmitter.execute()); return Promise.all(promises); }, + isExecuting() { return submitters.some(submitter => submitter.isExecuting()); }