From 9c44e7f85c447a599583f5a8d2eebbf4bdca2d6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Ferna=CC=81ndez=20Haro?= Date: Wed, 22 Apr 2020 17:34:53 +0100 Subject: [PATCH] [Telemetry] Fix optIn telemetry report bug --- src/plugins/telemetry/public/mocks.ts | 23 +++- .../public/services/telemetry_service.test.ts | 103 +++++++++++++++--- .../public/services/telemetry_service.ts | 16 ++- .../server/routes/telemetry_opt_in.ts | 28 +++-- 4 files changed, 137 insertions(+), 33 deletions(-) diff --git a/src/plugins/telemetry/public/mocks.ts b/src/plugins/telemetry/public/mocks.ts index 4e0f02242961ad..fd88f520205f55 100644 --- a/src/plugins/telemetry/public/mocks.ts +++ b/src/plugins/telemetry/public/mocks.ts @@ -25,11 +25,20 @@ import { httpServiceMock } from '../../../core/public/http/http_service.mock'; import { notificationServiceMock } from '../../../core/public/notifications/notifications_service.mock'; import { TelemetryService } from './services/telemetry_service'; import { TelemetryNotifications } from './services/telemetry_notifications/telemetry_notifications'; -import { TelemetryPluginStart } from './plugin'; +import { TelemetryPluginStart, TelemetryPluginConfig } from './plugin'; + +// The following is to be able to access private methods +/* eslint-disable dot-notation */ + +export interface TelemetryServiceMockOptions { + reportOptInStatusChange?: boolean; + config?: Partial; +} export function mockTelemetryService({ reportOptInStatusChange, -}: { reportOptInStatusChange?: boolean } = {}) { + config: configOverride = {}, +}: TelemetryServiceMockOptions = {}) { const config = { enabled: true, url: 'http://localhost', @@ -39,14 +48,22 @@ export function mockTelemetryService({ banner: true, allowChangingOptInStatus: true, telemetryNotifyUserAboutOptInDefault: true, + ...configOverride, }; - return new TelemetryService({ + const telemetryService = new TelemetryService({ config, http: httpServiceMock.createStartContract(), notifications: notificationServiceMock.createStartContract(), reportOptInStatusChange, }); + + const originalReportOptInStatus = telemetryService['reportOptInStatus']; + telemetryService['reportOptInStatus'] = jest.fn().mockImplementation(optInPayload => { + return originalReportOptInStatus(optInPayload); // Actually calling the original method + }); + + return telemetryService; } export function mockTelemetryNotifications({ diff --git a/src/plugins/telemetry/public/services/telemetry_service.test.ts b/src/plugins/telemetry/public/services/telemetry_service.test.ts index 0a49b0ae3084e3..16faa0cfc7536b 100644 --- a/src/plugins/telemetry/public/services/telemetry_service.test.ts +++ b/src/plugins/telemetry/public/services/telemetry_service.test.ts @@ -67,17 +67,31 @@ describe('TelemetryService', () => { }); describe('setOptIn', () => { + it('does not call the api if canChangeOptInStatus==false', async () => { + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: false }, + }); + expect(await telemetryService.setOptIn(true)).toBe(false); + + expect(telemetryService['http'].post).toBeCalledTimes(0); + }); + it('calls api if canChangeOptInStatus', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: false }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: true }, + }); await telemetryService.setOptIn(true); expect(telemetryService['http'].post).toBeCalledTimes(1); }); it('sends enabled true if optedIn: true', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: false }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: true }, + }); const optedIn = true; await telemetryService.setOptIn(optedIn); @@ -87,8 +101,10 @@ describe('TelemetryService', () => { }); it('sends enabled false if optedIn: false', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: false }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: true }, + }); const optedIn = false; await telemetryService.setOptIn(optedIn); @@ -98,9 +114,10 @@ describe('TelemetryService', () => { }); it('does not call reportOptInStatus if reportOptInStatusChange is false', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: false }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); - telemetryService['reportOptInStatus'] = jest.fn(); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: true }, + }); await telemetryService.setOptIn(true); expect(telemetryService['reportOptInStatus']).toBeCalledTimes(0); @@ -108,9 +125,10 @@ describe('TelemetryService', () => { }); it('calls reportOptInStatus if reportOptInStatusChange is true', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: true }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); - telemetryService['reportOptInStatus'] = jest.fn(); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: true, + config: { allowChangingOptInStatus: true }, + }); await telemetryService.setOptIn(true); expect(telemetryService['reportOptInStatus']).toBeCalledTimes(1); @@ -118,9 +136,10 @@ describe('TelemetryService', () => { }); it('adds an error toast on api error', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: false }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); - telemetryService['reportOptInStatus'] = jest.fn(); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: false, + config: { allowChangingOptInStatus: true }, + }); telemetryService['http'].post = jest.fn().mockImplementation((url: string) => { if (url === '/api/telemetry/v2/optIn') { throw Error('failed to update opt in.'); @@ -133,9 +152,13 @@ describe('TelemetryService', () => { expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1); }); + // This one should not happen because the entire method is fully caught but hey! :) it('adds an error toast on reportOptInStatus error', async () => { - const telemetryService = mockTelemetryService({ reportOptInStatusChange: true }); - telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true); + const telemetryService = mockTelemetryService({ + reportOptInStatusChange: true, + config: { allowChangingOptInStatus: true }, + }); + telemetryService['reportOptInStatus'] = jest.fn().mockImplementation(() => { throw Error('failed to report OptIn Status.'); }); @@ -146,4 +169,50 @@ describe('TelemetryService', () => { expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1); }); }); + + describe('getTelemetryUrl', () => { + it('should return the config.url parameter', async () => { + const url = 'http://test.com'; + const telemetryService = mockTelemetryService({ + config: { url }, + }); + + expect(telemetryService.getTelemetryUrl()).toBe(url); + }); + }); + + describe('setUserHasSeenNotice', () => { + it('should hit the API and change the config', async () => { + const telemetryService = mockTelemetryService({ + config: { telemetryNotifyUserAboutOptInDefault: undefined }, + }); + + expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined); + expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false); + await telemetryService.setUserHasSeenNotice(); + expect(telemetryService['http'].put).toBeCalledTimes(1); + expect(telemetryService.userHasSeenOptedInNotice).toBe(true); + expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(true); + }); + + it('should show a toast notification if the request fail', async () => { + const telemetryService = mockTelemetryService({ + config: { telemetryNotifyUserAboutOptInDefault: undefined }, + }); + + telemetryService['http'].put = jest.fn().mockImplementation((url: string) => { + if (url === '/api/telemetry/v2/userHasSeenNotice') { + throw Error('failed to update opt in.'); + } + }); + + expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined); + expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false); + await telemetryService.setUserHasSeenNotice(); + expect(telemetryService['http'].put).toBeCalledTimes(1); + expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1); + expect(telemetryService.userHasSeenOptedInNotice).toBe(false); + expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false); + }); + }); }); diff --git a/src/plugins/telemetry/public/services/telemetry_service.ts b/src/plugins/telemetry/public/services/telemetry_service.ts index cac4e3fdf5f506..6d87a74197fe57 100644 --- a/src/plugins/telemetry/public/services/telemetry_service.ts +++ b/src/plugins/telemetry/public/services/telemetry_service.ts @@ -122,11 +122,15 @@ export class TelemetryService { } try { - await this.http.post('/api/telemetry/v2/optIn', { + // Report the option to the Kibana server to store the settings. + // It returns the encrypted update to send to the telemetry cluster [{cluster_uuid, opt_in_status}] + const optInPayload = await this.http.post('/api/telemetry/v2/optIn', { body: JSON.stringify({ enabled: optedIn }), }); if (this.reportOptInStatusChange) { - await this.reportOptInStatus(optedIn); + // Use the response to report about the change to the remote telemetry cluster. + // If it's opt-out, this will be the last communication to the remote service. + await this.reportOptInStatus(optInPayload); } this.isOptedIn = optedIn; } catch (err) { @@ -162,7 +166,11 @@ export class TelemetryService { } }; - private reportOptInStatus = async (OptInStatus: boolean): Promise => { + /** + * Pushes the encrypted payload [{cluster_uuid, opt_in_status}] to the remote telemetry service + * @param optInPayload [{cluster_uuid, opt_in_status}] encrypted by the server into an array of strings + */ + private reportOptInStatus = async (optInPayload: string[]): Promise => { const telemetryOptInStatusUrl = this.getOptInStatusUrl(); try { @@ -171,7 +179,7 @@ export class TelemetryService { headers: { 'Content-Type': 'application/json', }, - body: JSON.stringify({ enabled: OptInStatus }), + body: JSON.stringify(optInPayload), }); } catch (err) { // Sending the ping is best-effort. Telemetry tries to send the ping once and discards it immediately if sending fails. diff --git a/src/plugins/telemetry/server/routes/telemetry_opt_in.ts b/src/plugins/telemetry/server/routes/telemetry_opt_in.ts index e65ade0ab8aaa9..4ed5dbf2512759 100644 --- a/src/plugins/telemetry/server/routes/telemetry_opt_in.ts +++ b/src/plugins/telemetry/server/routes/telemetry_opt_in.ts @@ -22,7 +22,10 @@ import { Observable } from 'rxjs'; import { take } from 'rxjs/operators'; import { schema } from '@kbn/config-schema'; import { IRouter } from 'kibana/server'; -import { TelemetryCollectionManagerPluginSetup } from 'src/plugins/telemetry_collection_manager/server'; +import { + StatsGetterConfig, + TelemetryCollectionManagerPluginSetup, +} from 'src/plugins/telemetry_collection_manager/server'; import { getTelemetryAllowChangingOptInStatus } from '../../common/telemetry_config'; import { sendTelemetryOptInStatus } from './telemetry_opt_in_stats'; @@ -79,23 +82,30 @@ export function registerTelemetryOptInRoutes({ }); } + const statsGetterConfig: StatsGetterConfig = { + start: moment() + .subtract(20, 'minutes') + .toISOString(), + end: moment().toISOString(), + unencrypted: false, + }; + + const optInStatus = await telemetryCollectionManager.getOptInStats( + newOptInStatus, + statsGetterConfig + ); + if (config.sendUsageFrom === 'server') { const optInStatusUrl = config.optInStatusUrl; await sendTelemetryOptInStatus( telemetryCollectionManager, { optInStatusUrl, newOptInStatus }, - { - start: moment() - .subtract(20, 'minutes') - .toISOString(), - end: moment().toISOString(), - unencrypted: false, - } + statsGetterConfig ); } await updateTelemetrySavedObject(context.core.savedObjects.client, attributes); - return res.ok({}); + return res.ok({ body: optInStatus }); } ); }