[Telemetry] Fix OptedOut banner (#151084)

Resolves #135107.
2025-06-27 18:51:07 -04:00 · 2023-02-14 13:53:55 +01:00 · 2023-02-14 13:53:55 +01:00 · 58c68c94d4
commit 58c68c94d4
parent 9049386f78
5 changed files with 203 additions and 43 deletions
--- a/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts
+++ b/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts
@ -74,7 +74,8 @@ export class TelemetryNotifications {
   * Should the banner to opt-in be shown to the user?
   */
  public shouldShowOptInBanner = (): boolean => {
-    const isOptedIn = this.telemetryService.getIsOptedIn();
+    // Using `config.optIn` instead of the getter `getIsOptedIn()` because the latter only returns boolean, and we want to compare it against `null`.
+    const isOptedIn = this.telemetryService.config.optIn;
    const bannerOnScreen = typeof this.optInBannerId !== 'undefined';
    return !bannerOnScreen && isOptedIn === null;
  };
--- a/src/plugins/telemetry/server/telemetry_config/get_telemetry_opt_in.ts
+++ b/src/plugins/telemetry/server/telemetry_config/get_telemetry_opt_in.ts
@ -29,17 +29,12 @@ export const getTelemetryOptIn: GetTelemetryOptIn = ({
    return configTelemetryOptIn;
  }

-  if (typeof telemetrySavedObject.enabled !== 'boolean') {
-    return configTelemetryOptIn;
-  }
+  // If `enabled` is not available in the SO, fall back to the config value.
+  const savedOptIn = telemetrySavedObject.enabled ?? configTelemetryOptIn;

-  const savedOptIn = telemetrySavedObject.enabled;
-
-  // if enabled is true, return it
+  // if the stored value is true, return it
  if (savedOptIn === true) return savedOptIn;

-  // TODO: Should we split the logic below into another OptIn getter?
-
  // Additional check if they've already opted out (enabled: false):
  // - if the Kibana version has changed by at least a minor version,
  //   return null to re-prompt.
@ -49,7 +44,7 @@ export const getTelemetryOptIn: GetTelemetryOptIn = ({
  // if the last kibana version isn't set, or is somehow not a string, return null
  if (typeof lastKibanaVersion !== 'string') return null;

-  // if version hasn't changed, just return enabled value
+  // if version hasn't changed, just return the stored value
  if (lastKibanaVersion === currentKibanaVersion) return savedOptIn;

  const lastSemver = parseSemver(lastKibanaVersion);
@ -64,7 +59,7 @@ export const getTelemetryOptIn: GetTelemetryOptIn = ({
    if (currentSemver.minor > lastSemver.minor) return null;
  }

-  // current version X.Y is not greater than last version X.Y, return enabled
+  // current version X.Y is not greater than last version X.Y, return the stored value
  return savedOptIn;
 };

--- a/test/api_integration/apis/telemetry/telemetry_config.ts
+++ b/test/api_integration/apis/telemetry/telemetry_config.ts
@ -6,33 +6,38 @@
 * Side Public License, v 1.
 */

+import { AxiosError } from 'axios';
 import { FtrProviderContext } from '../../ftr_provider_context';

-export default function optInTest({ getService }: FtrProviderContext) {
-  const client = getService('es');
+const TELEMETRY_SO_TYPE = 'telemetry';
+const TELEMETRY_SO_ID = 'telemetry';
+
+export default function telemetryConfigTest({ getService }: FtrProviderContext) {
+  const kbnClient = getService('kibanaServer');
  const supertest = getService('supertest');

  describe('/api/telemetry/v2/config API Telemetry config', () => {
    before(async () => {
-      await client.delete(
-        {
-          index: '.kibana',
-          id: 'telemetry:telemetry',
-        },
-        { ignore: [404] }
-      );
+      try {
+        await kbnClient.savedObjects.delete({ type: TELEMETRY_SO_TYPE, id: TELEMETRY_SO_ID });
+      } catch (err) {
+        const is404Error = err instanceof AxiosError && err.response?.status === 404;
+        if (!is404Error) {
+          throw err;
+        }
+      }
    });

    it('GET should get the default config', async () => {
      await supertest.get('/api/telemetry/v2/config').set('kbn-xsrf', 'xxx').expect(200, {
        allowChangingOptInStatus: true,
-        optIn: false, // the config.js for this FTR sets it to `false`
+        optIn: null, // the config.js for this FTR sets it to `false`, we are bound to ask again.
        sendUsageFrom: 'server',
-        telemetryNotifyUserAboutOptInDefault: false, // it's not opted-in, so we don't notify about opt-in??
+        telemetryNotifyUserAboutOptInDefault: false, // it's not opted-in by default (that's what this flag is about)
      });
    });

-    it('GET should get when opted-in', async () => {
+    it('GET should get `true` when opted-in', async () => {
      // Opt-in
      await supertest
        .post('/api/telemetry/v2/optIn')
@ -44,9 +49,79 @@ export default function optInTest({ getService }: FtrProviderContext) {
        allowChangingOptInStatus: true,
        optIn: true,
        sendUsageFrom: 'server',
-        // it's not opted-in (in the YAML config) despite being opted-in via API/UI, and we still say false??
        telemetryNotifyUserAboutOptInDefault: false,
      });
    });
+
+    it('GET should get false when opted-out', async () => {
+      // Opt-in
+      await supertest
+        .post('/api/telemetry/v2/optIn')
+        .set('kbn-xsrf', 'xxx')
+        .send({ enabled: false })
+        .expect(200);
+
+      await supertest.get('/api/telemetry/v2/config').set('kbn-xsrf', 'xxx').expect(200, {
+        allowChangingOptInStatus: true,
+        optIn: false,
+        sendUsageFrom: 'server',
+        telemetryNotifyUserAboutOptInDefault: false,
+      });
+    });
+
+    describe('From a previous version', function () {
+      this.tags(['skipCloud']);
+
+      // Get current values
+      let attributes: Record<string, unknown>;
+      let currentVersion: string;
+      let previousMinor: string;
+
+      before(async () => {
+        [{ attributes }, currentVersion] = await Promise.all([
+          kbnClient.savedObjects.get({ type: TELEMETRY_SO_TYPE, id: TELEMETRY_SO_ID }),
+          kbnClient.version.get(),
+        ]);
+
+        const [major, minor, patch] = currentVersion.match(/^(\d+)\.(\d+)\.(\d+)/)!.map(parseInt);
+        previousMinor = `${minor === 0 ? major - 1 : major}.${
+          minor === 0 ? minor : minor - 1
+        }.${patch}`;
+      });
+
+      it('GET should get `true` when opted-in in the current version', async () => {
+        // Opt-in from a previous version
+        await kbnClient.savedObjects.create({
+          overwrite: true,
+          type: TELEMETRY_SO_TYPE,
+          id: TELEMETRY_SO_ID,
+          attributes: { ...attributes, enabled: true, lastVersionChecked: previousMinor },
+        });
+
+        await supertest.get('/api/telemetry/v2/config').set('kbn-xsrf', 'xxx').expect(200, {
+          allowChangingOptInStatus: true,
+          optIn: true,
+          sendUsageFrom: 'server',
+          telemetryNotifyUserAboutOptInDefault: false,
+        });
+      });
+
+      it('GET should get `null` when opted-out in a previous version', async () => {
+        // Opt-out from previous version
+        await kbnClient.savedObjects.create({
+          overwrite: true,
+          type: TELEMETRY_SO_TYPE,
+          id: TELEMETRY_SO_ID,
+          attributes: { ...attributes, enabled: false, lastVersionChecked: previousMinor },
+        });
+
+        await supertest.get('/api/telemetry/v2/config').set('kbn-xsrf', 'xxx').expect(200, {
+          allowChangingOptInStatus: true,
+          optIn: null,
+          sendUsageFrom: 'server',
+          telemetryNotifyUserAboutOptInDefault: false,
+        });
+      });
+    });
  });
 }
--- a/test/plugin_functional/config.ts
+++ b/test/plugin_functional/config.ts
@ -53,6 +53,8 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
        '--corePluginDeprecations.noLongerUsed=still_using',
        // for testing set buffer duration to 0 to immediately flush counters into saved objects.
        '--usageCollection.usageCounters.bufferDuration=0',
+        // We want to test when the banner is shown
+        '--telemetry.banner=true',
        // explicitly enable the cloud integration plugins to validate the rendered config keys
        '--xpack.cloud_integrations.chat.enabled=true',
        '--xpack.cloud_integrations.chat.chatURL=a_string',
--- a/test/plugin_functional/test_suites/telemetry/telemetry.ts
+++ b/test/plugin_functional/test_suites/telemetry/telemetry.ts
@ -10,36 +10,123 @@ import expect from '@kbn/expect';
 import { KBN_SCREENSHOT_MODE_ENABLED_KEY } from '@kbn/screenshot-mode-plugin/public';
 import { PluginFunctionalProviderContext } from '../../services';

+const TELEMETRY_SO_TYPE = 'telemetry';
+const TELEMETRY_SO_ID = 'telemetry';
+
 export default function ({ getService, getPageObjects }: PluginFunctionalProviderContext) {
+  const kbnClient = getService('kibanaServer');
  const browser = getService('browser');
+  const find = getService('find');
+  const supertest = getService('supertest');
  const PageObjects = getPageObjects(['common']);

  describe('Telemetry service', () => {
-    const checkCanSendTelemetry = (): Promise<boolean> => {
-      return browser.executeAsync<boolean>((cb) => {
-        (window as unknown as Record<string, () => Promise<boolean>>)
-          ._checkCanSendTelemetry()
-          .then(cb);
-      });
-    };
+    describe('Screenshot mode', () => {
+      const checkCanSendTelemetry = (): Promise<boolean> => {
+        return browser.executeAsync<boolean>((cb) => {
+          (window as unknown as Record<string, () => Promise<boolean>>)
+            ._checkCanSendTelemetry()
+            .then(cb);
+        });
+      };

-    after(async () => {
-      await browser.removeLocalStorageItem(KBN_SCREENSHOT_MODE_ENABLED_KEY);
-      await browser.executeAsync<void>((cb) => {
-        (window as unknown as Record<string, () => Promise<boolean>>)
-          ._resetTelemetry()
-          .then(() => cb());
+      after(async () => {
+        await browser.removeLocalStorageItem(KBN_SCREENSHOT_MODE_ENABLED_KEY);
+        await browser.executeAsync<void>((cb) => {
+          (window as unknown as Record<string, () => Promise<boolean>>)
+            ._resetTelemetry()
+            .then(() => cb());
+        });
+      });
+
+      it('detects that telemetry cannot be sent in screenshot mode', async () => {
+        await PageObjects.common.navigateToApp('home');
+        expect(await checkCanSendTelemetry()).to.be(true);
+
+        await browser.setLocalStorageItem(KBN_SCREENSHOT_MODE_ENABLED_KEY, 'true');
+        await PageObjects.common.navigateToApp('home');
+
+        expect(await checkCanSendTelemetry()).to.be(false);
      });
    });

-    it('detects that telemetry cannot be sent in screenshot mode', async () => {
-      await PageObjects.common.navigateToApp('home');
-      expect(await checkCanSendTelemetry()).to.be(true);
+    describe('Opt-in/out banners', function () {
+      this.tags(['skipCloud']);

-      await browser.setLocalStorageItem(KBN_SCREENSHOT_MODE_ENABLED_KEY, 'true');
-      await PageObjects.common.navigateToApp('home');
+      // Get current values
+      let attributes: Record<string, unknown>;
+      let currentVersion: string;
+      let previousMinor: string;

-      expect(await checkCanSendTelemetry()).to.be(false);
+      before(async () => {
+        [{ attributes }, currentVersion] = await Promise.all([
+          kbnClient.savedObjects.get({ type: TELEMETRY_SO_TYPE, id: TELEMETRY_SO_ID }),
+          kbnClient.version.get(),
+        ]);
+
+        const [major, minor, patch] = currentVersion.match(/^(\d+)\.(\d+)\.(\d+)/)!.map(parseInt);
+        previousMinor = `${minor === 0 ? major - 1 : major}.${
+          minor === 0 ? minor : minor - 1
+        }.${patch}`;
+
+        await kbnClient.savedObjects.delete({ type: TELEMETRY_SO_TYPE, id: TELEMETRY_SO_ID });
+      });
+
+      it('shows the banner in the default configuration', async () => {
+        await PageObjects.common.navigateToApp('home');
+        expect(await find.existsByCssSelector('[data-test-subj="enable"]')).to.eql(true);
+        expect(await find.existsByCssSelector('[data-test-subj="disable"]')).to.eql(true);
+      });
+
+      it('does not show the banner if opted-in', async () => {
+        await supertest
+          .post('/api/telemetry/v2/optIn')
+          .set('kbn-xsrf', 'xxx')
+          .send({ enabled: true })
+          .expect(200);
+
+        await PageObjects.common.navigateToApp('home');
+        expect(await find.existsByCssSelector('[data-test-subj="enable"]')).to.eql(false);
+        expect(await find.existsByCssSelector('[data-test-subj="disable"]')).to.eql(false);
+      });
+
+      it('does not show the banner if opted-out in this version', async () => {
+        await supertest
+          .post('/api/telemetry/v2/optIn')
+          .set('kbn-xsrf', 'xxx')
+          .send({ enabled: false })
+          .expect(200);
+
+        await PageObjects.common.navigateToApp('home');
+        expect(await find.existsByCssSelector('[data-test-subj="enable"]')).to.eql(false);
+        expect(await find.existsByCssSelector('[data-test-subj="disable"]')).to.eql(false);
+      });
+
+      it('shows the banner if opted-out in a previous version', async () => {
+        await kbnClient.savedObjects.create({
+          overwrite: true,
+          type: TELEMETRY_SO_TYPE,
+          id: TELEMETRY_SO_ID,
+          attributes: { ...attributes, enabled: false, lastVersionChecked: previousMinor },
+        });
+
+        await PageObjects.common.navigateToApp('home');
+        expect(await find.existsByCssSelector('[data-test-subj="enable"]')).to.eql(true);
+        expect(await find.existsByCssSelector('[data-test-subj="disable"]')).to.eql(true);
+      });
+
+      it('does not show the banner if opted-in in a previous version', async () => {
+        await kbnClient.savedObjects.create({
+          overwrite: true,
+          type: TELEMETRY_SO_TYPE,
+          id: TELEMETRY_SO_ID,
+          attributes: { ...attributes, enabled: true, lastVersionChecked: previousMinor },
+        });
+
+        await PageObjects.common.navigateToApp('home');
+        expect(await find.existsByCssSelector('[data-test-subj="enable"]')).to.eql(false);
+        expect(await find.existsByCssSelector('[data-test-subj="disable"]')).to.eql(false);
+      });
    });
  });
 }