Merge pull request mozilla#15409 from mozilla/FXA-7201

task(settings): Ensure metrics are only emitted once

packages/fxa-settings/src/components/App/index.test.tsx

@@ -6,11 +6,12 @@ import React, { ReactNode } from 'react';
 import { render, act } from '@testing-library/react';
 import App from '.';
 import * as Metrics from '../../lib/metrics';
-import { useInitialState } from '../../models';
+import { useAccount, useInitialState } from '../../models';
 
 jest.mock('../../models', () => ({
   ...jest.requireActual('../../models'),
   useInitialState: jest.fn(),
+  useAccount: jest.fn(),
 }));
 
 jest.mock('react-markdown', () => {});
@@ -49,10 +50,19 @@ describe('metrics', () => {
   });
 
   it('Initializes metrics flow data when present', async () => {
+    const mockAccount = {
+      metricsEnabled: true,
+      recoveryKey: true,
+      totpActive: true,
+      hasSecondaryVerifiedEmail: false,
+    };
+    (useAccount as jest.Mock).mockReturnValue(mockAccount);
     (useInitialState as jest.Mock).mockReturnValue({ loading: true });
     const DEVICE_ID = 'yoyo';
     const BEGIN_TIME = 123456;
     const FLOW_ID = 'abc123';
+    const flowInit = jest.spyOn(Metrics, 'init');
+    const userPreferencesInit = jest.spyOn(Metrics, 'initUserPreferences');
     const updatedFlowQueryParams = {
       deviceId: DEVICE_ID,
       flowBeginTime: BEGIN_TIME,
@@ -68,6 +78,7 @@ describe('metrics', () => {
       flowId: FLOW_ID,
       flowBeginTime: BEGIN_TIME,
     });
+    expect(userPreferencesInit).toHaveBeenCalledWith(mockAccount);
     expect(window.location.replace).not.toHaveBeenCalled();
   });
 });

packages/fxa-settings/src/components/App/index.tsx

@@ -53,17 +53,22 @@ export const App = ({
 
   const { showReactApp } = flowQueryParams;
   const { loading, error } = useInitialState();
-  const { metricsEnabled } = useAccount();
+  const account = useAccount();
   const [email, setEmail] = useState<string>();
   const [emailLookupComplete, setEmailLookupComplete] =
     useState<boolean>(false);
 
   const config = useConfig();
   const sessionTokenId = sessionToken();
 
+  const { metricsEnabled } = account;
+
   useEffect(() => {
     Metrics.init(metricsEnabled || !isSignedIn, flowQueryParams);
-  }, [metricsEnabled, isSignedIn, flowQueryParams]);
+    if (metricsEnabled) {
+      Metrics.initUserPreferences(account);
+    }
+  }, [account, metricsEnabled, isSignedIn, flowQueryParams]);
 
   useEffect(() => {
     if (!loading && error?.message.includes('Invalid token')) {

packages/fxa-settings/src/components/Settings/PageRecoveryKeyAdd/index.test.tsx

@@ -10,6 +10,7 @@ import { mockAppContext, renderWithRouter } from '../../../models/mocks';
 import { Account, AppContext } from '../../../models';
 import { PageRecoveryKeyAdd } from '.';
 import * as Metrics from '../../../lib/metrics';
+import { resetOnce } from '../../../lib/utilities';
 
 jest.mock('base32-encode', () =>
   jest.fn().mockReturnValue('00000000000000000000000000000000')
@@ -106,13 +107,14 @@ describe('PageRecoveryKeyAdd', () => {
         .spyOn(Metrics, 'logViewEvent')
         .mockImplementation();
       logPageViewEventSpy = jest
-        .spyOn(Metrics, 'logPageViewEvent')
+        .spyOn(Metrics, 'logPageViewEventOnce')
         .mockImplementation();
     });
 
     afterEach(() => {
       logViewEventSpy.mockReset();
       logPageViewEventSpy.mockReset();
+      resetOnce();
     });
 
     afterAll(() => {

packages/fxa-settings/src/components/Settings/PageSecondaryEmailAdd/index.test.tsx

@@ -10,6 +10,7 @@ import { PageSecondaryEmailAdd } from '.';
 import { Account, AppContext } from '../../../models';
 import { AuthUiErrors } from 'fxa-settings/src/lib/auth-errors/auth-errors';
 import * as Metrics from '../../../lib/metrics';
+import { resetOnce } from '../../../lib/utilities';
 
 window.console.error = jest.fn();
 
@@ -21,6 +22,10 @@ afterAll(() => {
   (window.console.error as jest.Mock).mockReset();
 });
 
+afterEach(() => {
+  resetOnce();
+});
+
 describe('PageSecondaryEmailAdd', () => {
   describe('no secondary email set', () => {
     it('renders as expected', () => {

packages/fxa-settings/src/lib/metrics.test.ts

@@ -2,6 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+import { MOCK_ACCOUNT } from '../models/mocks';
 import {
   init,
   reset,
@@ -13,10 +14,10 @@ import {
   addExperiment,
   setUserPreference,
   setNewsletters,
-  useUserPreferences,
   useViewEvent,
   usePageViewEvent,
   logErrorEvent,
+  initUserPreferences,
 } from './metrics';
 
 import { window } from './window';
@@ -91,6 +92,11 @@ function initFlow(enabled = true) {
     flowBeginTime,
     flowId,
   });
+  initUserPreferences({
+    hasSecondaryVerifiedEmail: MOCK_ACCOUNT.emails.length > 1,
+    recoveryKey: MOCK_ACCOUNT.recoveryKey,
+    totpActive: MOCK_ACCOUNT.totp.exists && MOCK_ACCOUNT.totp.verified,
+  });
 }
 
 function initAndLog(slug = eventSlug, eventProperties = {}) {
@@ -404,7 +410,6 @@ describe('setUserPreference', () => {
 
 describe('setUserPreferences', () => {
   it('sets three user prefs', () => {
-    useUserPreferences();
     initAndLog();
     const payloadData = parsePayloadData();
     // strict equal since _all three_ properties must be present

packages/fxa-settings/src/lib/metrics.ts

@@ -3,12 +3,11 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import sentryMetrics from 'fxa-shared/lib/sentry';
-import { useEffect, useRef } from 'react';
-import { useAccount } from '../models';
 import { window } from './window';
 import { v4 as uuid } from 'uuid';
 import { QueryParams } from '..';
-import { config } from 'process';
+import { once } from './utilities';
+import { useEffect } from 'react';
 
 export const settingsViewName = 'settings';
 
@@ -240,42 +239,29 @@ export async function init(enabled: boolean, flowQueryParams: QueryParams) {
   }
 }
 
+/**
+ * Initializes the user preference state for metrics.
+ * @param account - An account model containing user preferences applicable to metrics
+ */
+export function initUserPreferences(account: {
+  recoveryKey: boolean;
+  hasSecondaryVerifiedEmail: boolean;
+  totpActive: boolean;
+}) {
+  setUserPreference('account-recovery', account.recoveryKey);
+  setUserPreference('emails', account.hasSecondaryVerifiedEmail);
+  setUserPreference('two-step-authentication', account.totpActive);
+}
+
 /**
  * Often we need to log a metric event in a place where hooks are not allowed.
  * However, we do want initialize some event data through the use of hooks.
  * It also provide a couple functions that ensure the event is logged once.
  */
-export function useMetrics(): {
-  logViewEventOnce: typeof logViewEvent;
-  logPageViewEventOnce: typeof logPageViewEvent;
-} {
-  useUserPreferences();
-  const makeLogOnceFn = useRef((f: Function) => {
-    let hasLogged = false;
-
-    return (
-      ...args:
-        | Parameters<typeof logViewEvent>
-        | Parameters<typeof logPageViewEvent>
-    ) => {
-      if (!hasLogged) {
-        hasLogged = true;
-        f(...args);
-      }
-    };
-  });
-
-  const logViewEventOnce = useRef<
-    (...args: Parameters<typeof logViewEvent>) => void
-  >(makeLogOnceFn.current(logViewEvent));
-
-  const logPageViewEventOnce = useRef<
-    (...args: Parameters<typeof logPageViewEvent>) => void
-  >(makeLogOnceFn.current(logPageViewEvent));
-
+export function useMetrics() {
   return {
-    logViewEventOnce: logViewEventOnce.current,
-    logPageViewEventOnce: logPageViewEventOnce.current,
+    logViewEventOnce: logViewEventOnce,
+    logPageViewEventOnce: logPageViewEventOnce,
   };
 }
 
@@ -408,13 +394,20 @@ export function logViewEvent(
 }
 
 /**
- * A non-hook version of usePageViewEvent.  See comments for that function.
+ * Same as logViewEvent, but ensures that event can be logged at most once per page lifecylce.
+ * The event is memoized based on viewName and eventName.
+ * @param viewName
+ * @param eventName
+ * @param eventProperties - Additional properties to log with the event
  */
-export function logPageViewEvent(
+export function logViewEventOnce(
   viewName: string,
+  eventName: string,
   eventProperties: Hash<any> = {}
 ) {
-  logEvents([`screen.${viewName}`], eventProperties);
+  once(`${viewName}.${eventName}`, () => {
+    logViewEvent(viewName, eventName, eventProperties);
+  });
 }
 
 /**
@@ -428,15 +421,35 @@ export function logPageViewEvent(
 export function useViewEvent(
   viewName: string,
   eventName: string,
-  eventProperties: Hash<any> = {},
-  dependencies: any[] = []
+  eventProperties: Hash<any> = {}
 ) {
-  useUserPreferences();
-
   useEffect(() => {
-    logViewEvent(viewName, eventName, eventProperties);
+    logViewEventOnce(viewName, eventName, eventProperties);
     // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, dependencies);
+  }, []);
+}
+
+/**
+ * A non-hook version of usePageViewEvent.  See comments for that function.
+ */
+export function logPageViewEvent(
+  viewName: string,
+  eventProperties: Hash<any> = {}
+) {
+  logEvents([`screen.${viewName}`], eventProperties);
+}
+
+/**
+ * A non-hook version of usePageViewEvent.  Note that this function will log at most one
+ * event per page lifecycle. The event is memoized based on viewName.
+ */
+export function logPageViewEventOnce(
+  viewName: string,
+  eventProperties: Hash<any> = {}
+) {
+  once(viewName, () => {
+    logPageViewEvent(viewName, eventProperties);
+  });
 }
 
 /**
@@ -447,10 +460,9 @@ export function useViewEvent(
  */
 export function usePageViewEvent(
   viewName: string,
-  eventProperties: Hash<any> = {},
-  dependencies: any[] = []
+  eventProperties: Hash<any> = {}
 ) {
-  useViewEvent('screen', viewName, eventProperties, dependencies);
+  useViewEvent('screen', viewName, eventProperties);
 }
 
 /**
@@ -479,21 +491,6 @@ export function setUserPreference(prefName: string, value: boolean) {
   configurableProperties.userPreferences[prefName] = value;
 }
 
-/**
- * All three properties must be included if userPreferences is in the request
- * payload.
- */
-export function useUserPreferences() {
-  try {
-    const account = useAccount();
-    setUserPreference('account-recovery', account.recoveryKey);
-    setUserPreference('emails', account.hasSecondaryVerifiedEmail);
-    setUserPreference('two-step-authentication', account.totpActive);
-  } catch {
-    // noop, just means the user isn't logged in
-  }
-}
-
 /**
  * Log subscribed newsletters for a user.
  *

packages/fxa-settings/src/lib/utilities.test.ts

@@ -5,6 +5,8 @@
 import {
   deepMerge,
   isBase32Crockford,
+  once,
+  resetOnce,
   searchParam,
   searchParams,
 } from './utilities';
@@ -115,3 +117,33 @@ describe('isBase32Crockford', () => {
     expect(isBase32Crockford('U')).toBe(false);
   });
 });
+
+describe('once', () => {
+  let count = 0;
+  const cb = () => {
+    count++;
+  };
+
+  beforeEach(() => {
+    count = 0;
+    resetOnce();
+  });
+
+  it('calls at least once', () => {
+    once('foo', cb);
+    expect(count).toEqual(1);
+  });
+
+  it('calls no more than once', () => {
+    once('foo', cb);
+    once('foo', cb);
+    expect(count).toEqual(1);
+  });
+
+  it('call no more than once per key', () => {
+    once('foo', cb);
+    once('bar', cb);
+    once('bar', cb);
+    expect(count).toEqual(2);
+  });
+});