From 8ad47846ca0d36c5c4e797f92d4a4b9b182aa184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Fern=C3=A1ndez=20Haro?= Date: Thu, 10 Sep 2020 14:49:09 +0100 Subject: [PATCH] [Telemetry] Only show Opt-In banner when user can change settings (#76883) Co-authored-by: Christiane (Tina) Heiligers Co-authored-by: Elastic Machine --- .../public/application/components/welcome.tsx | 4 +-- src/plugins/telemetry/public/mocks.ts | 1 + src/plugins/telemetry/public/plugin.ts | 18 +++++++++++ .../telemetry_notifications.test.ts | 24 ++++++++++++++ .../telemetry_notifications.ts | 4 +-- .../public/services/telemetry_service.test.ts | 31 ++++++++++++++++--- .../public/services/telemetry_service.ts | 22 +++++++++++-- ...telemetry_management_section.test.tsx.snap | 2 -- 8 files changed, 92 insertions(+), 14 deletions(-) diff --git a/src/plugins/home/public/application/components/welcome.tsx b/src/plugins/home/public/application/components/welcome.tsx index cacb507009c70..404185de3d2ea 100644 --- a/src/plugins/home/public/application/components/welcome.tsx +++ b/src/plugins/home/public/application/components/welcome.tsx @@ -76,7 +76,7 @@ export class Welcome extends React.Component { componentDidMount() { const { telemetry } = this.props; this.services.trackUiMetric(METRIC_TYPE.LOADED, 'welcomeScreenMount'); - if (telemetry) { + if (telemetry?.telemetryService.userCanChangeSettings) { telemetry.telemetryNotifications.setOptedInNoticeSeen(); } document.addEventListener('keydown', this.hideOnEsc); @@ -88,7 +88,7 @@ export class Welcome extends React.Component { private renderTelemetryEnabledOrDisabledText = () => { const { telemetry } = this.props; - if (!telemetry) { + if (!telemetry || !telemetry.telemetryService.userCanChangeSettings) { return null; } diff --git a/src/plugins/telemetry/public/mocks.ts b/src/plugins/telemetry/public/mocks.ts index dd7e5a4cc4ce3..5f38b27144d02 100644 --- a/src/plugins/telemetry/public/mocks.ts +++ b/src/plugins/telemetry/public/mocks.ts @@ -48,6 +48,7 @@ export function mockTelemetryService({ banner: true, allowChangingOptInStatus: true, telemetryNotifyUserAboutOptInDefault: true, + userCanChangeSettings: true, ...configOverride, }; diff --git a/src/plugins/telemetry/public/plugin.ts b/src/plugins/telemetry/public/plugin.ts index 3846e7cb96a19..9fefa2ebdd02e 100644 --- a/src/plugins/telemetry/public/plugin.ts +++ b/src/plugins/telemetry/public/plugin.ts @@ -25,6 +25,7 @@ import { PluginInitializerContext, SavedObjectsClientContract, SavedObjectsBatchResponse, + ApplicationStart, } from '../../../core/public'; import { TelemetrySender, TelemetryService, TelemetryNotifications } from './services'; @@ -61,6 +62,7 @@ export interface TelemetryPluginConfig { optInStatusUrl: string; sendUsageFrom: 'browser' | 'server'; telemetryNotifyUserAboutOptInDefault?: boolean; + userCanChangeSettings?: boolean; } export class TelemetryPlugin implements Plugin { @@ -69,6 +71,7 @@ export class TelemetryPlugin implements Plugin) { this.currentKibanaVersion = initializerContext.env.packageInfo.version; @@ -91,6 +94,9 @@ export class TelemetryPlugin implements Plugin { expect(telemetryService.setUserHasSeenNotice).toBeCalledTimes(1); }); }); + +describe('shouldShowOptedInNoticeBanner', () => { + it("should return true because a banner hasn't been shown, the notice hasn't been seen and the user has privileges to edit saved objects", () => { + const telemetryService = mockTelemetryService(); + telemetryService.getUserShouldSeeOptInNotice = jest.fn().mockReturnValue(true); + const telemetryNotifications = mockTelemetryNotifications({ telemetryService }); + expect(telemetryNotifications.shouldShowOptedInNoticeBanner()).toBe(true); + }); + + it('should return false because the banner is already on screen', () => { + const telemetryService = mockTelemetryService(); + telemetryService.getUserShouldSeeOptInNotice = jest.fn().mockReturnValue(true); + const telemetryNotifications = mockTelemetryNotifications({ telemetryService }); + telemetryNotifications['optedInNoticeBannerId'] = 'bruce-banner'; + expect(telemetryNotifications.shouldShowOptedInNoticeBanner()).toBe(false); + }); + + it("should return false because the banner has already been seen or the user doesn't have privileges to change saved objects", () => { + const telemetryService = mockTelemetryService(); + telemetryService.getUserShouldSeeOptInNotice = jest.fn().mockReturnValue(false); + const telemetryNotifications = mockTelemetryNotifications({ telemetryService }); + expect(telemetryNotifications.shouldShowOptedInNoticeBanner()).toBe(false); + }); +}); diff --git a/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts b/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts index bf25bb592db82..fc44a4db7cf5e 100644 --- a/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts +++ b/src/plugins/telemetry/public/services/telemetry_notifications/telemetry_notifications.ts @@ -39,9 +39,9 @@ export class TelemetryNotifications { } public shouldShowOptedInNoticeBanner = (): boolean => { - const userHasSeenOptedInNotice = this.telemetryService.getUserHasSeenOptedInNotice(); + const userShouldSeeOptInNotice = this.telemetryService.getUserShouldSeeOptInNotice(); const bannerOnScreen = typeof this.optedInNoticeBannerId !== 'undefined'; - return !bannerOnScreen && userHasSeenOptedInNotice; + return !bannerOnScreen && userShouldSeeOptInNotice; }; public renderOptedInNoticeBanner = (): void => { diff --git a/src/plugins/telemetry/public/services/telemetry_service.test.ts b/src/plugins/telemetry/public/services/telemetry_service.test.ts index 16faa0cfc7536..655bbfe746c2a 100644 --- a/src/plugins/telemetry/public/services/telemetry_service.test.ts +++ b/src/plugins/telemetry/public/services/telemetry_service.test.ts @@ -184,15 +184,15 @@ describe('TelemetryService', () => { describe('setUserHasSeenNotice', () => { it('should hit the API and change the config', async () => { const telemetryService = mockTelemetryService({ - config: { telemetryNotifyUserAboutOptInDefault: undefined }, + config: { telemetryNotifyUserAboutOptInDefault: undefined, userCanChangeSettings: true }, }); expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined); - expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(false); await telemetryService.setUserHasSeenNotice(); expect(telemetryService['http'].put).toBeCalledTimes(1); expect(telemetryService.userHasSeenOptedInNotice).toBe(true); - expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(true); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(true); }); it('should show a toast notification if the request fail', async () => { @@ -207,12 +207,33 @@ describe('TelemetryService', () => { }); expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined); - expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false); + expect(telemetryService.getUserShouldSeeOptInNotice()).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); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(false); + }); + }); + + describe('getUserShouldSeeOptInNotice', () => { + it('returns whether the user can update the telemetry config (has SavedObjects access)', () => { + const telemetryService = mockTelemetryService({ + config: { userCanChangeSettings: undefined }, + }); + expect(telemetryService.config.userCanChangeSettings).toBe(undefined); + expect(telemetryService.userCanChangeSettings).toBe(false); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(false); + + telemetryService.userCanChangeSettings = false; + expect(telemetryService.config.userCanChangeSettings).toBe(false); + expect(telemetryService.userCanChangeSettings).toBe(false); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(false); + + telemetryService.userCanChangeSettings = true; + expect(telemetryService.config.userCanChangeSettings).toBe(true); + expect(telemetryService.userCanChangeSettings).toBe(true); + expect(telemetryService.getUserShouldSeeOptInNotice()).toBe(true); }); }); }); diff --git a/src/plugins/telemetry/public/services/telemetry_service.ts b/src/plugins/telemetry/public/services/telemetry_service.ts index 6d87a74197fe5..c807aa9e1d35e 100644 --- a/src/plugins/telemetry/public/services/telemetry_service.ts +++ b/src/plugins/telemetry/public/services/telemetry_service.ts @@ -87,9 +87,25 @@ export class TelemetryService { return telemetryUrl; }; - public getUserHasSeenOptedInNotice = () => { - return this.config.telemetryNotifyUserAboutOptInDefault || false; - }; + /** + * Returns if an user should be shown the notice about Opt-In/Out telemetry. + * The decision is made based on whether any user has already dismissed the message or + * the user can't actually change the settings (in which case, there's no point on bothering them) + */ + public getUserShouldSeeOptInNotice(): boolean { + return ( + (this.config.telemetryNotifyUserAboutOptInDefault && this.config.userCanChangeSettings) ?? + false + ); + } + + public get userCanChangeSettings() { + return this.config.userCanChangeSettings ?? false; + } + + public set userCanChangeSettings(userCanChangeSettings: boolean) { + this.config = { ...this.config, userCanChangeSettings }; + } public getIsOptedIn = () => { return this.isOptedIn; diff --git a/src/plugins/telemetry_management_section/public/components/__snapshots__/telemetry_management_section.test.tsx.snap b/src/plugins/telemetry_management_section/public/components/__snapshots__/telemetry_management_section.test.tsx.snap index dd4ee61fd1148..ab29656c557c2 100644 --- a/src/plugins/telemetry_management_section/public/components/__snapshots__/telemetry_management_section.test.tsx.snap +++ b/src/plugins/telemetry_management_section/public/components/__snapshots__/telemetry_management_section.test.tsx.snap @@ -228,7 +228,6 @@ exports[`TelemetryManagementSectionComponent renders null because allowChangingO "getIsOptedIn": [Function], "getOptInStatusUrl": [Function], "getTelemetryUrl": [Function], - "getUserHasSeenOptedInNotice": [Function], "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -430,7 +429,6 @@ exports[`TelemetryManagementSectionComponent renders null because query does not "getIsOptedIn": [Function], "getOptInStatusUrl": [Function], "getTelemetryUrl": [Function], - "getUserHasSeenOptedInNotice": [Function], "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object {