Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/fxa-settings/src/lib/glean/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,26 @@ describe('lib/glean', () => {
);
});

// The dispatcher case arm is the only thing deciding what reaches Glean
// for this event, so both the populated and empty reason are pinned here
// (FXA-14133).
it('submits a ping with the cad_firefox_choice_view event name and a reason', async () => {
GleanMetrics.cadFireFox.choiceView({
event: { reason: 'otp_login' },
});
await GleanMetrics.isDone();
sinon.assert.calledOnce(setEventNameStub);
sinon.assert.calledWith(setEventNameStub, 'cad_firefox_choice_view');
sinon.assert.calledWith(setEventReasonStub, 'otp_login');
});

it('submits a ping with the cad_firefox_choice_view event name and no reason', async () => {
GleanMetrics.cadFireFox.choiceView();
await GleanMetrics.isDone();
sinon.assert.calledOnce(setEventNameStub);
sinon.assert.calledWith(setEventNameStub, 'cad_firefox_choice_view');
});

it('submits a ping with the email_first_passkey_submit_success event name', async () => {
GleanMetrics.emailFirst.passkeySubmitSuccess();
await GleanMetrics.isDone();
Expand Down
4 changes: 3 additions & 1 deletion packages/fxa-settings/src/lib/glean/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,9 @@ const recordEventMetric = (
cadFirefox.view.record();
break;
case 'cad_firefox_choice_view':
cadFirefox.choiceView.record();
cadFirefox.choiceView.record({
reason: gleanPingMetrics?.event?.['reason'] || '',
});
break;
case 'cad_firefox_choice_engage':
cadFirefox.choiceEngage.record({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ describe('InlineRecoveryKeySetupContainer', () => {
);

expect(hardNavigateSpy).toHaveBeenCalledWith(
'/pair?showSuccessMessage=true'
'/pair?showSuccessMessage=true&pairReason=password_login'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason you would need this would be for Backbone pairing. I don't see any changes in fxa-content-server (and don't think we need them) so, remove?

@dschom dschom Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Initially Claude also included content server here, but I rolled those back and this fell through the cracks.

);
expect(InlineRecoveryKeySetupModule.default).not.toHaveBeenCalled();
});
Expand Down
59 changes: 56 additions & 3 deletions packages/fxa-settings/src/pages/Pair/Index/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,20 @@ import * as ReactUtils from 'fxa-react/lib/utils';
import { MOCK_ERROR } from './mocks';
import { MOCK_CMS_INFO } from '../../mocks';
import Pair, { viewName } from '.';
import { PAIR_GLEAN_REASONS } from 'fxa-shared/metrics/glean/pair-reasons';

jest.mock('../../../lib/metrics', () => ({
usePageViewEvent: jest.fn(),
}));

let mockLocationState: unknown = null;
let mockLocationSearch = '';
const mockNavigate = jest.fn();
jest.mock('react-router', () => ({
...jest.requireActual('react-router'),
useLocation: () => ({
pathname: '/pair',
search: '',
search: mockLocationSearch,
state: mockLocationState,
}),
useNavigate: () => mockNavigate,
Expand Down Expand Up @@ -105,6 +107,7 @@ describe('Pair', () => {
afterEach(() => {
jest.clearAllMocks();
mockLocationState = null;
mockLocationSearch = '';
});

// Render Pair and wait for the bootstrap spinner to clear before asserting.
Expand Down Expand Up @@ -139,9 +142,59 @@ describe('Pair', () => {
).toBeInTheDocument();
});

it('fires choiceView Glean event on render', async () => {
// Helper reads the reason actually handed to Glean rather than pinning the
// whole call shape, so a behaviour-preserving change to how the argument is
// built doesn't break these.
const recordedReason = () =>
(GleanMetrics.cadFireFox.choiceView as jest.Mock).mock.calls[0]?.[0]
?.event?.reason;

it('fires choiceView with no reason when there is no pairReason', async () => {
await renderPair();
expect(GleanMetrics.cadFireFox.choiceView).toHaveBeenCalledTimes(1);
expect(recordedReason()).toBeUndefined();
});

it.each(PAIR_GLEAN_REASONS)(
'fires choiceView with reason %s from location state',
async (pairReason) => {
mockLocationState = { pairReason };
await renderPair();
expect(recordedReason()).toBe(pairReason);
}
);

// Flows that stop at /signup_confirmed_sync or /inline_recovery_key_setup
// reach /pair by hard navigation, so the reason arrives as a query param.
it.each(PAIR_GLEAN_REASONS)(
'fires choiceView with reason %s from the query param',
async (pairReason) => {
mockLocationSearch = `?pairReason=${pairReason}`;
await renderPair();
expect(recordedReason()).toBe(pairReason);
}
);

it('prefers location state over the query param', async () => {
mockLocationState = { pairReason: 'otp_login' };
mockLocationSearch = '?pairReason=password_login';
await renderPair();
expect(recordedReason()).toBe('otp_login');
});

it.each(['not-a-real-flow', '<script>alert(1)</script>', ' otp_login'])(
'ignores the unrecognized query param %j',
async (pairReason) => {
mockLocationSearch = `?pairReason=${encodeURIComponent(pairReason)}`;
await renderPair();
expect(recordedReason()).toBeUndefined();
}
);

it('ignores an unrecognized value in location state', async () => {
mockLocationState = { pairReason: 'not-a-real-flow' };
await renderPair();
expect(GleanMetrics.cadFireFox.choiceView).toHaveBeenCalled();
expect(recordedReason()).toBeUndefined();
});

it('enables Continue button after selecting a radio', async () => {
Expand Down
48 changes: 35 additions & 13 deletions packages/fxa-settings/src/pages/Pair/Index/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,14 @@
* 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 React, { useCallback, useEffect, useRef, useState } from 'react';
import React, {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react';
import { isPairGleanReason } from 'fxa-shared/metrics/glean/pair-reasons';
import { Link, useLocation } from 'react-router';
import { useNavigateWithQuery } from '../../../lib/hooks/useNavigateWithQuery';
import { FtlMsg } from 'fxa-react/lib/utils';
Expand Down Expand Up @@ -68,11 +75,7 @@ type PairProps = {
};
export const viewName = 'pair';

const Pair = ({
error,
cmsInfo: cmsInfoProp,
integration,
}: PairProps) => {
const Pair = ({ error, cmsInfo: cmsInfoProp, integration }: PairProps) => {
usePageViewEvent(viewName, REACT_ENTRYPOINT);
const ftlMsgResolver = useFtlMsgResolver();
const localizedQRCodeLabel = ftlMsgResolver.getMsg(
Expand Down Expand Up @@ -161,22 +164,41 @@ const Pair = ({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

// Banner variant is driven by router state from getSyncNavigate.
const { origin: pairOrigin } = (location.state ?? {}) as Pick<
SigninLocationState,
'origin'
>;

// Router state is the primary channel, but flows that stop at an interstitial
// (/signup_confirmed_sync, /inline_recovery_key_setup) reach here through a
// hard navigation that carries the query param instead, and a reload drops
// router state entirely. Both are validated: `location.state` is untyped at
// runtime, and the param is user-controllable.
const pairReason = useMemo(() => {
const fromState = (location.state as { pairReason?: unknown } | null)
?.pairReason;
if (isPairGleanReason(fromState)) {
return fromState;
}
const fromQuery = new URLSearchParams(location.search).get('pairReason');
return isPairGleanReason(fromQuery) ? fromQuery : undefined;
}, [location.state, location.search]);

// Fire Glean view events only after the bootstrap reveals the page;
// otherwise users redirected during bootstrap would skew the metric.
useEffect(() => {
if (bootstrapping) return;
if (currentView === 'choice') {
GleanMetrics.cadFireFox.choiceView();
// Recorded as an empty reason when /pair is reached outside a sign-in or
// sign-up flow, or from a flow with no sanctioned bucket (third-party
// auth) — the dispatcher coerces a missing reason to ''.
GleanMetrics.cadFireFox.choiceView({ event: { reason: pairReason } });
return;
}
GleanMetrics.cadFireFox.view();
}, [bootstrapping, currentView]);
}, [bootstrapping, currentView, pairReason]);

// Banner variant is driven by reach-router state from getSyncNavigate.
const { origin: pairOrigin } = (location.state ?? {}) as Pick<
SigninLocationState,
'origin'
>;
const bannerCopy =
currentView === 'choice' && pairOrigin ? PAIR_BANNER_FTL[pairOrigin] : null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ const SetPasswordContainer = ({
handleFxaOAuthLogin: true,
showSignupConfirmedSync: true,
origin: 'post-verify-set-password',
// Sync needs keys, so every passwordless OTP/passkey sign-in lands
// here before it can reach /pair. This is the only place that still
// knows how the session was established, so it drives the /pair
// `choice_view` reason.
passwordCreationReason,
syncEngines: {
offeredEngines: offeredSyncEngines,
declinedEngines: declinedSyncEngines,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,20 @@ describe('SigninPasskeyFallback container', () => {
);
});

// This is the only producer of isPasskeySession, and therefore the only
// source of the passkey_login reason on /pair for accounts that already
// have a password (FXA-14133).
it('marks the session as passkey-established so /pair attributes passkey_login', async () => {
const { getByTestId } = render();
submitPassword(getByTestId);

await waitFor(() => {
expect(mockHandleNavigation).toHaveBeenCalledWith(
expect.objectContaining({ isPasskeySession: true })
);
});
});

it('passes the flow metricsContext to sessionReauth so the deferred account.login is correlated', async () => {
const { getByTestId } = render();
submitPassword(getByTestId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ const SigninPasskeyFallbackContainer = ({
// messages; navigating the WebView away would interrupt it and leave
// Sync paused. Desktop finishes by navigating.
performNavigation: !integration.isFirefoxMobileClient(),
// The session was established by the passkey assertion; the password
// entered here only unwraps keys. Keeps /pair's `choice_view` reason
// attributed to the passkey flow rather than password sign-in.
isPasskeySession: true,
authClient,
});
if (navError) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ const SigninPasswordlessCode = ({
if (isSyncDesktopV3Integration(integration)) {
const { to } = getSyncNavigate(location.search, {
showSignupConfirmedSync: true,
origin: 'signup',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is acting like a sinup action... but let's double check...

});
navigate(to);
} else if (isOAuthIntegration(integration)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,71 @@ describe('PageSigninRecoveryCode', () => {
expect.anything()
);
});

// Without this, /pair tags cad_firefox.choice_view as password_login for a
// user who actually signed in with an email OTP code.
it('forwards isPasswordlessOtpSignin to handleNavigation for an OTP sign-in', async () => {
const user = userEvent.setup();
const handleNavigationSpy = jest
.spyOn(SigninUtils, 'handleNavigation')
.mockResolvedValue({ error: undefined });
const integration = createMockSigninOAuthNativeSyncIntegration();
integration.requiresPasswordForLogin = () => false;

renderWithLocalizationProvider(
<MemoryRouter>
<SigninRecoveryCode
finishOAuthFlowHandler={mockFinishOAuthFlowHandler}
integration={integration}
navigateToRecoveryPhone={jest.fn()}
signinState={{
...mockSigninLocationState,
isPasswordlessOtpSignin: true,
}}
submitRecoveryCode={submitSuccess()}
supportsKeysOptionalLogin={true}
/>
</MemoryRouter>
);
await user.type(screen.getByRole('textbox'), MOCK_BACKUP_CODE);
await user.click(screen.getByRole('button', { name: 'Confirm' }));

await waitFor(() => {
expect(handleNavigationSpy).toHaveBeenCalledWith(
expect.objectContaining({ isPasswordlessOtpSignin: true })
);
});
});

it('leaves isPasswordlessOtpSignin unset for a password sign-in', async () => {
const user = userEvent.setup();
const handleNavigationSpy = jest
.spyOn(SigninUtils, 'handleNavigation')
.mockResolvedValue({ error: undefined });
const integration = createMockSigninOAuthNativeSyncIntegration();
integration.requiresPasswordForLogin = () => false;

renderWithLocalizationProvider(
<MemoryRouter>
<SigninRecoveryCode
finishOAuthFlowHandler={mockFinishOAuthFlowHandler}
integration={integration}
navigateToRecoveryPhone={jest.fn()}
signinState={mockSigninLocationState}
submitRecoveryCode={submitSuccess()}
supportsKeysOptionalLogin={true}
/>
</MemoryRouter>
);
await user.type(screen.getByRole('textbox'), MOCK_BACKUP_CODE);
await user.click(screen.getByRole('button', { name: 'Confirm' }));

await waitFor(() => {
expect(handleNavigationSpy).toHaveBeenCalledWith(
expect.objectContaining({ isPasswordlessOtpSignin: undefined })
);
});
});
});

describe('submit with error', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ const SigninRecoveryCode = ({
handleFxaLogin: true,
handleFxaOAuthLogin: true,
performNavigation: !integration.isFirefoxMobileClient(),
isPasswordlessOtpSignin: signinState.isPasswordlessOtpSignin,
authClient,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,45 @@ describe('SigninRecoveryPhoneContainer', () => {
expect.anything()
);
});

// Without this, /pair tags cad_firefox.choice_view as password_login for a
// user who actually signed in with an email OTP code (FXA-14133).
it('forwards isPasswordlessOtpSignin to handleNavigation for an OTP sign-in', async () => {
const integration =
createMockSigninOAuthNativeSyncIntegration() as Integration;
integration.requiresPasswordForLogin = () => false;
mockReachRouter('/signin_recovery_phone', {
signinState: {
...mockSigninLocationState,
isPasswordlessOtpSignin: true,
},
lastFourPhoneDigits: '1234',
});
renderSigninRecoveryPhoneContainer(integration);

await currentPageProps?.verifyCode('123456');

expect(handleNavigation).toHaveBeenCalledWith(
expect.objectContaining({ isPasswordlessOtpSignin: true })
);
});

it('leaves isPasswordlessOtpSignin unset for a password sign-in', async () => {
const integration =
createMockSigninOAuthNativeSyncIntegration() as Integration;
integration.requiresPasswordForLogin = () => false;
mockReachRouter('/signin_recovery_phone', {
signinState: mockSigninLocationState,
lastFourPhoneDigits: '1234',
});
renderSigninRecoveryPhoneContainer(integration);

await currentPageProps?.verifyCode('123456');

expect(handleNavigation).toHaveBeenCalledWith(
expect.objectContaining({ isPasswordlessOtpSignin: undefined })
);
});
});

describe('with mobile integration', () => {
Expand Down
Loading
Loading