From c42d217423d566ad3f03e4ca0f4df4a95b352f7e Mon Sep 17 00:00:00 2001 From: Teodora Sandu <81559517+teodora-sandu@users.noreply.github.com> Date: Wed, 7 Feb 2024 06:54:00 +0000 Subject: [PATCH] fix: apply changes to custom endpoint [IDE-36] (#429) * refactor: pass in configuration as an argument * fix: check analytics permitted in enqueueEvent * fix: add userId argument to identify function * fix: check analytics permitted in identify * refactor: move check in a function * chore: update CHANGELOG --- CHANGELOG.md | 6 + src/snyk/common/analytics/itly.ts | 42 ++-- .../common/watchers/configurationWatcher.ts | 2 +- src/snyk/extension.ts | 9 +- src/test/unit/common/analytics/itly.test.ts | 218 +++++++++++++++--- 5 files changed, 207 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7c4fe92e..d1e8222cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Snyk Security - Code and Open Source Dependencies Changelog +## [2.2.4] + +### Fixes + +- Changing the custom endpoints has an effect on whether we sent Amplitude events or not + ## [2.2.3] ### Fixed diff --git a/src/snyk/common/analytics/itly.ts b/src/snyk/common/analytics/itly.ts index 684c24acb..e2f8c605f 100644 --- a/src/snyk/common/analytics/itly.ts +++ b/src/snyk/common/analytics/itly.ts @@ -8,7 +8,7 @@ import itly, { AnalysisIsTriggeredProperties as _AnalysisIsTriggeredProperties, QuickFixIsDisplayedProperties as _QuickFixIsDisplayedProperties, } from '../../../ampli'; -import { Configuration } from '../configuration/configuration'; +import { Configuration, IConfiguration } from '../configuration/configuration'; import { SnykConfiguration } from '../configuration/snykConfiguration'; import { IDE_NAME } from '../constants/general'; import { ErrorHandler } from '../error/errorHandler'; @@ -37,7 +37,6 @@ export type QuickFixIsDisplayedProperties = _QuickFixIsDisplayedProperties & { export interface IAnalytics { load(): Iteratively | null; flush(): Promise; - setShouldReportEvents(shouldReportEvents: boolean): void; identify(userId: string): Promise; logIssueInTreeIsClicked(properties: IssueInTreeIsClickedProperties): void; logAnalysisIsReady(properties: AnalysisIsReadyProperties): void; @@ -63,23 +62,15 @@ export class Iteratively implements IAnalytics { constructor( private readonly user: User, private logger: ILog, - private shouldReportEvents: boolean, - private analyticsPermitted: boolean, - private isDevelopment: boolean, + private configuration: IConfiguration, private snykConfiguration?: SnykConfiguration, ) {} - setShouldReportEvents(shouldReportEvents: boolean): void { - this.shouldReportEvents = shouldReportEvents; - this.load(); - } - load(): Iteratively | null { - if (!this.shouldReportEvents || !this.analyticsPermitted) { - this.logger.debug(`Analytics are disabled. No analytics will be collected.`); + if (!this.configuration.shouldReportEvents) { + this.logger.debug(`Analytics are disabled in Settings.`); return null; } - this.logger.debug(`Analytics are enabled. Analytics will be collected.`); const segmentWriteKey = this.snykConfiguration?.segmentWriteKey; if (!segmentWriteKey) { @@ -88,12 +79,12 @@ export class Iteratively implements IAnalytics { } const segment = new SegmentPlugin(segmentWriteKey); - const isDevelopment = this.isDevelopment; + const isDevelopment = this.configuration.isDevelopment; if (!this.loaded) { try { itly.load({ - disabled: !this.shouldReportEvents, + disabled: !this.configuration.shouldReportEvents, environment: isDevelopment ? 'development' : 'production', plugins: [segment, new ItlyErrorPlugin(this.logger)], }); @@ -109,19 +100,14 @@ export class Iteratively implements IAnalytics { public flush = (): Promise => itly.flush(); - async identify(): Promise { - if (!this.canReportEvents()) { - return; - } - - if (!this.user.authenticatedId) { - this.logger.error('Tried to identify non-authenticated user'); + async identify(userId: string): Promise { + if (!this.allowedToReportEvents()) { return; } // Calling identify is the preferred way to merge authenticated user with anonymous one, // see https://snyk.slack.com/archives/C01U2SPRB3Q/p1624276750134700?thread_ts=1624030602.128900&cid=C01U2SPRB3Q - itly.identify(this.user.authenticatedId, undefined, { + itly.identify(userId, undefined, { segment: { options: { anonymousId: this.user.anonymousId, @@ -232,8 +218,8 @@ export class Iteratively implements IAnalytics { }); } - private enqueueEvent(eventFunction: () => void, mustBeAuthenticated = true): void { - if (!this.canReportEvents()) { + enqueueEvent(eventFunction: () => void, mustBeAuthenticated = true): void { + if (!this.allowedToReportEvents()) { return; } if (mustBeAuthenticated && !this.user.authenticatedId) { @@ -249,13 +235,17 @@ export class Iteratively implements IAnalytics { return false; } - if (!this.shouldReportEvents) { + if (!this.configuration.shouldReportEvents) { return false; } return true; } + private allowedToReportEvents(): boolean { + return this.canReportEvents() && this.configuration.analyticsPermitted; + } + private getAnonymousSegmentOptions(): TrackOptions { return { segment: { diff --git a/src/snyk/common/watchers/configurationWatcher.ts b/src/snyk/common/watchers/configurationWatcher.ts index 5b3a74440..260b8e6f0 100644 --- a/src/snyk/common/watchers/configurationWatcher.ts +++ b/src/snyk/common/watchers/configurationWatcher.ts @@ -30,7 +30,7 @@ class ConfigurationWatcher implements IWatcher { if (key === ADVANCED_ADVANCED_MODE_SETTING) { return extension.checkAdvancedMode(); } else if (key === YES_TELEMETRY_SETTING) { - return this.analytics.setShouldReportEvents(configuration.shouldReportEvents); + this.analytics.load(); } else if (key === OSS_ENABLED_SETTING) { extension.viewManagerService.refreshOssView(); } else if (key === CODE_SECURITY_ENABLED_SETTING || key === CODE_QUALITY_ENABLED_SETTING) { diff --git a/src/snyk/extension.ts b/src/snyk/extension.ts index 371cd8418..925dfdfd2 100644 --- a/src/snyk/extension.ts +++ b/src/snyk/extension.ts @@ -121,14 +121,7 @@ class SnykExtension extends SnykLib implements IExtension { this.user = await User.getAnonymous(this.context, Logger); - this.analytics = new Iteratively( - this.user, - Logger, - configuration.shouldReportEvents, - configuration.analyticsPermitted, - configuration.isDevelopment, - snykConfiguration, - ); + this.analytics = new Iteratively(this.user, Logger, configuration, snykConfiguration); SecretStorageAdapter.init(vscodeContext); diff --git a/src/test/unit/common/analytics/itly.test.ts b/src/test/unit/common/analytics/itly.test.ts index a5c0570ed..046db489b 100644 --- a/src/test/unit/common/analytics/itly.test.ts +++ b/src/test/unit/common/analytics/itly.test.ts @@ -1,46 +1,26 @@ import { strictEqual } from 'assert'; +import * as sinon from 'sinon'; +import itly from '../../../../ampli'; import { Iteratively } from '../../../../snyk/common/analytics/itly'; +import { IConfiguration } from '../../../../snyk/common/configuration/configuration'; import { SnykConfiguration } from '../../../../snyk/common/configuration/snykConfiguration'; import { User } from '../../../../snyk/common/user'; import { LoggerMock } from '../../mocks/logger.mock'; suite('Iteratively', () => { - const snykConfig = {} as SnykConfiguration; - const isDevelopment = false; + const config = { + shouldReportEvents: false, + analyticsPermitted: false, + isDevelopment: false, + } as IConfiguration; + const snykConfig = { + segmentWriteKey: 'test', + } as SnykConfiguration; suite('.load()', () => { - suite('when analytics are not permitted', () => { - const analyticsPermitted = false; - [true, false].forEach(shouldReportEvents => { - test(`Returns "null" when shouldReportEvents == ${shouldReportEvents}`, () => { - const iteratively = new Iteratively( - new User(), - new LoggerMock(), - shouldReportEvents, - analyticsPermitted, - isDevelopment, - snykConfig, - ); - - const result = iteratively.load(); - - strictEqual(result, null); - }); - }); - }); - suite('when analytics are permitted', () => { - const analyticsPermitted = true; - test('Returns "null" when shouldReportEvents == false', () => { - const iteratively = new Iteratively( - new User(), - new LoggerMock(), - false, - analyticsPermitted, - isDevelopment, - snykConfig, - ); + const iteratively = new Iteratively(new User(), new LoggerMock(), config, snykConfig); const result = iteratively.load(); @@ -51,9 +31,10 @@ suite('Iteratively', () => { const iteratively = new Iteratively( new User(), new LoggerMock(), - true, - analyticsPermitted, - isDevelopment, + { + ...config, + shouldReportEvents: true, + }, snykConfig, ); @@ -63,4 +44,171 @@ suite('Iteratively', () => { }); }); }); + + suite('.enqueueEvent', () => { + test('Enqueues event if shouldReportEvents === true and analyticsPermitted === true', () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: true, + analyticsPermitted: true, + }, + snykConfig, + ); + iteratively.load(); + + let eventFunctionWasCalled = false; + iteratively.enqueueEvent(() => { + eventFunctionWasCalled = true; + }, false); + strictEqual(eventFunctionWasCalled, true); + }); + + test('Does not enqueue event if shouldReportEvents === false and analyticsPermitted === true', () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: false, + analyticsPermitted: true, + }, + snykConfig, + ); + iteratively.load(); + + let eventFunctionWasCalled = false; + iteratively.enqueueEvent(() => { + eventFunctionWasCalled = true; + }, false); + strictEqual(eventFunctionWasCalled, false); + }); + + test('Does not enqueue event if shouldReportEvents === true and analyticsPermitted === false', () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: true, + analyticsPermitted: false, + }, + snykConfig, + ); + iteratively.load(); + + let eventFunctionWasCalled = false; + iteratively.enqueueEvent(() => { + eventFunctionWasCalled = true; + }, false); + strictEqual(eventFunctionWasCalled, false); + }); + + test('Does not enqueue event if shouldReportEvents === false and analyticsPermitted === false', () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: false, + analyticsPermitted: false, + }, + snykConfig, + ); + iteratively.load(); + + let eventFunctionWasCalled = false; + iteratively.enqueueEvent(() => { + eventFunctionWasCalled = true; + }, false); + strictEqual(eventFunctionWasCalled, false); + }); + }); + + suite('.identify', () => { + let identify: sinon.SinonSpy; + const user = new User(); + + setup(() => { + identify = sinon.spy(itly, 'identify'); + }); + + teardown(() => { + identify.restore(); + }); + + test('Identifies a user if shouldReportEvents === true and analyticsPermitted === true', async () => { + const iteratively = new Iteratively( + user, + new LoggerMock(), + { + ...config, + shouldReportEvents: true, + analyticsPermitted: true, + }, + snykConfig, + ); + iteratively.load(); + + await iteratively.identify('test'); + strictEqual(identify.called, true); + strictEqual(identify.calledOnce, true); + }); + + test('Does not identify a user if shouldReportEvents === true and analyticsPermitted === false', async () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: true, + analyticsPermitted: false, + }, + snykConfig, + ); + iteratively.load(); + + await iteratively.identify('test'); + strictEqual(identify.called, false); + strictEqual(identify.calledOnce, false); + }); + + test('Does not identify a user if shouldReportEvents === false and analyticsPermitted === true', async () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: false, + analyticsPermitted: true, + }, + snykConfig, + ); + iteratively.load(); + + await iteratively.identify('test'); + strictEqual(identify.called, false); + strictEqual(identify.calledOnce, false); + }); + + test('Does not identify a user if shouldReportEvents === false and analyticsPermitted === false', async () => { + const iteratively = new Iteratively( + new User(), + new LoggerMock(), + { + ...config, + shouldReportEvents: false, + analyticsPermitted: false, + }, + snykConfig, + ); + iteratively.load(); + + await iteratively.identify('test'); + strictEqual(identify.called, false); + strictEqual(identify.calledOnce, false); + }); + }); });