From 1d81bdc6f9a676d075e6ad83b055b4ee43080a86 Mon Sep 17 00:00:00 2001 From: James Salter Date: Wed, 28 Jul 2021 09:37:08 +0100 Subject: [PATCH] Interface changes and anonymity fixes --- src/Lifecycle.ts | 10 +- src/PosthogAnalytics.ts | 118 +++++++++--------- src/components/structures/MatrixChat.tsx | 13 +- .../tabs/user/SecurityUserSettingsTab.js | 4 +- src/settings/Settings.tsx | 8 ++ .../PseudonymousAnalyticsController.ts | 26 ++++ test/PosthogAnalytics-test.ts | 44 ++++--- 7 files changed, 124 insertions(+), 99 deletions(-) create mode 100644 src/settings/controllers/PseudonymousAnalyticsController.ts diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index c27c774cd7..b0a521d886 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -48,7 +48,7 @@ import { Jitsi } from "./widgets/Jitsi"; import { SSO_HOMESERVER_URL_KEY, SSO_ID_SERVER_URL_KEY, SSO_IDP_ID_KEY } from "./BasePlatform"; import ThreepidInviteStore from "./stores/ThreepidInviteStore"; import CountlyAnalytics from "./CountlyAnalytics"; -import { Anonymity, getAnalytics, getPlatformProperties } from "./PosthogAnalytics"; +import { getAnalytics } from "./PosthogAnalytics"; import CallHandler from './CallHandler'; import LifecycleCustomisations from "./customisations/Lifecycle"; import ErrorDialog from "./components/views/dialogs/ErrorDialog"; @@ -574,13 +574,7 @@ async function doSetLoggedIn( await abortLogin(); } - if (SettingsStore.getValue("analyticsOptIn")) { - const analytics = getAnalytics(); - analytics.setAnonymity(Anonymity.Pseudonymous); - await analytics.identifyUser(credentials.userId); - } else { - getAnalytics().setAnonymity(Anonymity.Anonymous); - } + getAnalytics().updateAnonymityFromSettings(credentials.userId); Analytics.setLoggedIn(credentials.guest, credentials.homeserverUrl); diff --git a/src/PosthogAnalytics.ts b/src/PosthogAnalytics.ts index cdb23e582c..fa530b5309 100644 --- a/src/PosthogAnalytics.ts +++ b/src/PosthogAnalytics.ts @@ -85,14 +85,10 @@ export async function getRedactedCurrentLocation(origin: string, hash: string, p export class PosthogAnalytics { private anonymity = Anonymity.Anonymous; private posthog?: PostHog = null; - - // set true during init() if posthog config is present + // set true during the constructor if posthog config is present, otherwise false private enabled = false; - - // set to true after init() has been called - private initialised = false; - private static _instance = null; + private platformSuperProperties = {}; public static instance(): PosthogAnalytics { if (!this._instance) { @@ -103,10 +99,6 @@ export class PosthogAnalytics { constructor(posthog: PostHog) { this.posthog = posthog; - } - - public init(anonymity: Anonymity) { - this.anonymity = anonymity; const posthogConfig = SdkConfig.get()["posthog"]; if (posthogConfig) { this.posthog.init(posthogConfig.projectApiKey, { @@ -123,7 +115,6 @@ export class PosthogAnalytics { sanitize_properties: this.sanitizeProperties.bind(this), respect_dnt: true, }); - this.initialised = true; this.enabled = true; } else { this.enabled = false; @@ -159,19 +150,39 @@ export class PosthogAnalytics { return properties; } - public async identifyUser(userId: string) { - if (this.anonymity == Anonymity.Anonymous) return; - this.posthog.identify(await hashHex(userId)); + private static getAnonymityFromSettings(): Anonymity { + // determine the current anonymity level based on curernt user settings + + // "Send anonymous usage data which helps us improve Element. This will use a cookie." + const analyticsOptIn = SettingsStore.getValue("analyticsOptIn"); + + // "Send pseudonymous usage data which helps us improve Element. This will use a cookie." + // + // Currently, this is only a labs flag, for testing purposes. + const pseudonumousOptIn = SettingsStore.getValue("feature_pseudonymousAnalyticsOptIn"); + + let anonymity; + if (pseudonumousOptIn) { + anonymity = Anonymity.Pseudonymous; + } else if (analyticsOptIn) { + anonymity = Anonymity.Anonymous; + } else { + anonymity = Anonymity.Disabled; + } + + return anonymity; } - public registerSuperProperties(properties) { - if (this.enabled) { - this.posthog.register(properties); + public async identifyUser(userId: string) { + if (this.anonymity == Anonymity.Pseudonymous) { + this.posthog.identify(await hashHex(userId)); } } - public isInitialised() { - return this.initialised; + private registerSuperProperties(properties) { + if (this.enabled) { + this.posthog.register(properties); + } } public isEnabled() { @@ -179,6 +190,13 @@ export class PosthogAnalytics { } public setAnonymity(anonymity: Anonymity) { + if (this.enabled && (anonymity == Anonymity.Disabled || anonymity == Anonymity.Anonymous)) { + // when transitioning to Disabled or Anonymous ensure we clear out any prior state + // set in posthog e.g. distinct ID + this.posthog.reset(); + // Restore any previously set platform super properties + this.registerSuperProperties(this.platformSuperProperties); + } this.anonymity = anonymity; } @@ -194,9 +212,6 @@ export class PosthogAnalytics { } private async capture(eventName: string, properties: posthog.Properties) { - if (!this.initialised) { - throw Error("Tried to track event before PoshogAnalytics.init has completed"); - } if (!this.enabled) { return; } @@ -239,45 +254,36 @@ export class PosthogAnalytics { durationMs, }); } -} -export async function getPlatformProperties() { - const platform = PlatformPeg.get(); - let appVersion; - try { - appVersion = await platform.getAppVersion(); - } catch (e) { - // this happens if no version is set i.e. in dev - appVersion = "unknown"; + public async updatePlatformSuperProperties() { + this.platformSuperProperties = await PosthogAnalytics.getPlatformProperties(); + this.registerSuperProperties(this.platformSuperProperties); } - return { - appVersion, - appPlatform: platform.getHumanReadableName(), - }; + private static async getPlatformProperties() { + const platform = PlatformPeg.get(); + let appVersion; + try { + appVersion = await platform.getAppVersion(); + } catch (e) { + // this happens if no version is set i.e. in dev + appVersion = "unknown"; + } + + return { + appVersion, + appPlatform: platform.getHumanReadableName(), + }; + } + + public async updateAnonymityFromSettings(userId?: string) { + this.setAnonymity(PosthogAnalytics.getAnonymityFromSettings()); + if (userId && this.getAnonymity() == Anonymity.Pseudonymous) { + await this.identifyUser(userId); + } + } } export function getAnalytics(): PosthogAnalytics { return PosthogAnalytics.instance(); } - -export function getAnonymityFromSettings(): Anonymity { - // determine the current anonymity level based on curernt user settings - - // "Send anonymous usage data which helps us improve Element. This will use a cookie." - const analyticsOptIn = SettingsStore.getValue("analyticsOptIn"); - - // "Send pseudonymous usage data which helps us improve Element. This will use a cookie." - const pseudonumousOptIn = SettingsStore.getValue("pseudonymousAnalyticsOptIn"); - - let anonymity; - if (pseudonumousOptIn) { - anonymity = Anonymity.Pseudonymous; - } else if (analyticsOptIn) { - anonymity = Anonymity.Anonymous; - } else { - anonymity = Anonymity.Disabled; - } - - return anonymity; -} diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index bd54b0ebc9..1a477970fa 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -107,7 +107,7 @@ import UIStore, { UI_EVENTS } from "../../stores/UIStore"; import SoftLogout from './auth/SoftLogout'; import { makeRoomPermalink } from "../../utils/permalinks/Permalinks"; import { copyPlaintext } from "../../utils/strings"; -import { Anonymity, getAnalytics, getAnonymityFromSettings, getPlatformProperties } from '../../PosthogAnalytics'; +import { getAnalytics } from '../../PosthogAnalytics'; /** constants for MatrixChat.state.view */ export enum Views { @@ -390,10 +390,8 @@ export default class MatrixChat extends React.PureComponent { } const analytics = getAnalytics(); - analytics.init(getAnonymityFromSettings()); - // note this requires a network request in the browser, so some events can potentially - // before before registerSuperProperties has been called - getPlatformProperties().then((properties) => analytics.registerSuperProperties(properties)); + analytics.updateAnonymityFromSettings(); + analytics.updatePlatformSuperProperties(); CountlyAnalytics.instance.enable(/* anonymous = */ true); } @@ -831,11 +829,6 @@ export default class MatrixChat extends React.PureComponent { if (CountlyAnalytics.instance.canEnable()) { CountlyAnalytics.instance.enable(/* anonymous = */ false); } - getAnalytics().setAnonymity(Anonymity.Pseudonymous); - // TODO: this is an async call and we're not waiting for it to complete - - // so potentially an event could be fired prior to it completing and would be - // missing the user identification. - getAnalytics().identifyUser(MatrixClientPeg.get().getUserId()); break; case 'reject_cookies': SettingsStore.setValue("analyticsOptIn", null, SettingLevel.DEVICE, false); diff --git a/src/components/views/settings/tabs/user/SecurityUserSettingsTab.js b/src/components/views/settings/tabs/user/SecurityUserSettingsTab.js index 15b4992cd8..670e2ec757 100644 --- a/src/components/views/settings/tabs/user/SecurityUserSettingsTab.js +++ b/src/components/views/settings/tabs/user/SecurityUserSettingsTab.js @@ -36,7 +36,7 @@ import { UIFeature } from "../../../../../settings/UIFeature"; import { isE2eAdvancedPanelPossible } from "../../E2eAdvancedPanel"; import CountlyAnalytics from "../../../../../CountlyAnalytics"; import { replaceableComponent } from "../../../../../utils/replaceableComponent"; -import { Anonymity, getAnalytics } from "../../../../../PosthogAnalytics"; +import { getAnalytics } from "../../../../../PosthogAnalytics"; export class IgnoredUser extends React.Component { static propTypes = { @@ -107,7 +107,7 @@ export default class SecurityUserSettingsTab extends React.Component { _updateAnalytics = (checked) => { checked ? Analytics.enable() : Analytics.disable(); CountlyAnalytics.instance.enable(/* anonymous = */ !checked); - getAnalytics().setAnonymity(checked ? Anonymity.Pseudonymous : Anonymity.Anonymous); + getAnalytics().updateAnonymityFromSettings(MatrixClientPeg.get().getUserId()); }; _onExportE2eKeysClicked = () => { diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 830ea9e32e..db0cb05c9f 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -41,6 +41,7 @@ import { Layout } from "./Layout"; import ReducedMotionController from './controllers/ReducedMotionController'; import IncompatibleController from "./controllers/IncompatibleController"; import SdkConfig from "../SdkConfig"; +import PseudonymousAnalyticsController from './controllers/PseudonymousAnalyticsController'; // These are just a bunch of helper arrays to avoid copy/pasting a bunch of times const LEVELS_ROOM_SETTINGS = [ @@ -297,6 +298,13 @@ export const SETTINGS: {[setting: string]: ISetting} = { supportedLevels: LEVELS_FEATURE, default: false, }, + "feature_pseudonymousAnalyticsOptIn": { + isFeature: true, + supportedLevels: LEVELS_FEATURE, + displayName: _td('Send pseudonymous analytics data'), + default: false, + controller: new PseudonymousAnalyticsController(), + }, "advancedRoomListLogging": { // TODO: Remove flag before launch: https://github.com/vector-im/element-web/issues/14231 displayName: _td("Enable advanced debugging for the room list"), diff --git a/src/settings/controllers/PseudonymousAnalyticsController.ts b/src/settings/controllers/PseudonymousAnalyticsController.ts new file mode 100644 index 0000000000..d55efe3c74 --- /dev/null +++ b/src/settings/controllers/PseudonymousAnalyticsController.ts @@ -0,0 +1,26 @@ +/* +Copyright 2021 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import SettingController from "./SettingController"; +import { SettingLevel } from "../SettingLevel"; +import { getAnalytics } from "../../PosthogAnalytics"; +import { MatrixClientPeg } from "../../MatrixClientPeg"; + +export default class PseudonymousAnalyticsController extends SettingController { + public onChange(level: SettingLevel, roomId: string, newValue: any) { + getAnalytics().updateAnonymityFromSettings(MatrixClientPeg.get().getUserId()); + } +} diff --git a/test/PosthogAnalytics-test.ts b/test/PosthogAnalytics-test.ts index 7d81b6e86d..f726fe0b13 100644 --- a/test/PosthogAnalytics-test.ts +++ b/test/PosthogAnalytics-test.ts @@ -7,11 +7,15 @@ class FakePosthog { public capture; public init; public identify; + public reset; + public register; constructor() { this.capture = jest.fn(); this.init = jest.fn(); this.identify = jest.fn(); + this.reset = jest.fn(); + this.register = jest.fn(); } } @@ -37,12 +41,11 @@ export interface ITestRoomEvent extends IRoomEvent { } describe("PosthogAnalytics", () => { - let analytics: PosthogAnalytics; let fakePosthog: FakePosthog; beforeEach(() => { fakePosthog = new FakePosthog(); - analytics = new PosthogAnalytics(fakePosthog); + window.crypto = { subtle: crypto.webcrypto.subtle, }; @@ -53,26 +56,28 @@ describe("PosthogAnalytics", () => { }); describe("Initialisation", () => { - it("Should not initialise if config is not set", async () => { + it("Should not be enabled without config being set", () => { jest.spyOn(SdkConfig, "get").mockReturnValue({}); - analytics.init(Anonymity.Pseudonymous); + const analytics = new PosthogAnalytics(fakePosthog); expect(analytics.isEnabled()).toBe(false); }); - it("Should initialise if config is set", async () => { + it("Should be enabled if config is set", () => { jest.spyOn(SdkConfig, "get").mockReturnValue({ posthog: { projectApiKey: "foo", apiHost: "bar", }, }); - analytics.init(Anonymity.Pseudonymous); - expect(analytics.isInitialised()).toBe(true); + const analytics = new PosthogAnalytics(fakePosthog); + analytics.setAnonymity(Anonymity.Pseudonymous); expect(analytics.isEnabled()).toBe(true); }); }); describe("Tracking", () => { + let analytics: PosthogAnalytics; + beforeEach(() => { jest.spyOn(SdkConfig, "get").mockReturnValue({ posthog: { @@ -80,10 +85,12 @@ describe("PosthogAnalytics", () => { apiHost: "bar", }, }); + + analytics = new PosthogAnalytics(fakePosthog); }); it("Should pass trackAnonymousEvent() to posthog", async () => { - analytics.init(Anonymity.Pseudonymous); + analytics.setAnonymity(Anonymity.Pseudonymous); await analytics.trackAnonymousEvent("jest_test_event", { foo: "bar", }); @@ -92,7 +99,7 @@ describe("PosthogAnalytics", () => { }); it("Should pass trackRoomEvent to posthog", async () => { - analytics.init(Anonymity.Pseudonymous); + analytics.setAnonymity(Anonymity.Pseudonymous); const roomId = "42"; await analytics.trackRoomEvent("jest_test_event", roomId, { foo: "bar", @@ -104,7 +111,7 @@ describe("PosthogAnalytics", () => { }); it("Should pass trackPseudonymousEvent() to posthog", async () => { - analytics.init(Anonymity.Pseudonymous); + analytics.setAnonymity(Anonymity.Pseudonymous); await analytics.trackPseudonymousEvent("jest_test_pseudo_event", { foo: "bar", }); @@ -112,17 +119,8 @@ describe("PosthogAnalytics", () => { expect(fakePosthog.capture.mock.calls[0][1]["foo"]).toEqual("bar"); }); - it("Should blow up if not inititalised prior to tracking", async () => { - const fn = () => { - return analytics.trackAnonymousEvent("jest_test_event", { - foo: "bar", - }); - }; - await expect(fn()).rejects.toThrow(); - }); - it("Should not track pseudonymous messages if anonymous", async () => { - analytics.init(Anonymity.Anonymous); + analytics.setAnonymity(Anonymity.Anonymous); await analytics.trackPseudonymousEvent("jest_test_event", { foo: "bar", }); @@ -130,7 +128,7 @@ describe("PosthogAnalytics", () => { }); it("Should not track any events if disabled", async () => { - analytics.init(Anonymity.Disabled); + analytics.setAnonymity(Anonymity.Disabled); await analytics.trackPseudonymousEvent("jest_test_event", { foo: "bar", }); @@ -181,14 +179,14 @@ bd75b3e080945674c0351f75e0db33d1e90986fa07b318ea7edf776f5eef38d4`); }); it("Should identify the user to posthog if pseudonymous", async () => { - analytics.init(Anonymity.Pseudonymous); + analytics.setAnonymity(Anonymity.Pseudonymous); await analytics.identifyUser("foo"); expect(fakePosthog.identify.mock.calls[0][0]) .toBe("2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae"); }); it("Should not identify the user to posthog if anonymous", async () => { - analytics.init(Anonymity.Anonymous); + analytics.setAnonymity(Anonymity.Anonymous); await analytics.identifyUser("foo"); expect(fakePosthog.identify.mock.calls.length).toBe(0); });