[Telemetry] Fix optIn telemetry report bug (elastic#64214) (elastic#6…

…4290) Co-authored-by: Elastic Machine <[email protected]>
gchaps · Apr 23, 2020 · df164e8 · df164e8
1 parent dba0179
commit df164e8
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 33 deletions.
diff --git 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<TelemetryPluginConfig>;
+}
 
 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
@@ -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,29 +114,32 @@ 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);
       expect(telemetryService['http'].post).toBeCalledTimes(1);
     });
 
     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);
       expect(telemetryService['http'].post).toBeCalledTimes(1);
     });
 
     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
@@ -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<string[]>('/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<void> => {
+  /**
+   * 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<void> => {
     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
@@ -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 });
     }
   );
 }