Skip to content

fix(settings): fix metrics not always emitted for third party auth #20087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MagentaManifold wants to merge 1 commit into
base: main
Choose a base branch
from
+106 −138
Open

fix(settings): fix metrics not always emitted for third party auth #20087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions packages/fxa-react/lib/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ export function hardNavigate(
href: string,
additionalQueryParams: Record<string, string> = {},
includeCurrentQueryParams = false,
replace: boolean = false
replace: boolean = false,
timeoutMs: number = DEFAULT_NAVIGATE_TIMEOUT_MS
) {
// If there are any query params in the href, we automatically include them in the new url.
const url = new URL(href, window.location.origin);
Expand All @@ -40,11 +41,11 @@ export function hardNavigate(
if (replace) {
setTimeout(() => {
window.location.replace(url.href);
}, DEFAULT_NAVIGATE_TIMEOUT_MS);
}, timeoutMs);
} else {
setTimeout(() => {
window.location.href = url.href;
}, DEFAULT_NAVIGATE_TIMEOUT_MS);
}, timeoutMs);
}
}

Expand Down
12 changes: 6 additions & 6 deletions packages/fxa-settings/src/components/ThirdPartyAuth/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ exports[`ThirdPartyAuthComponent buttons match snapshot: apple 1`] = `
<button
aria-label="Continue with Apple"
class="w-[60px] h-[60px] p-4 flex items-center justify-center rounded-full border focus-visible-default outline-offset-2
bg-black border-transparent
"
type="submit"
bg-black border-transparent
"
type="button"
>
<svg
class="w-full h-auto"
Expand All @@ -20,9 +20,9 @@ exports[`ThirdPartyAuthComponent buttons match snapshot: google 1`] = `
<button
aria-label="Continue with Google"
class="w-[60px] h-[60px] p-4 flex items-center justify-center rounded-full border focus-visible-default outline-offset-2
bg-[#F9F4F4] border-[#747775] border-[1px]
"
type="submit"
bg-[#F9F4F4] border-[#747775] border-[1px]
"
type="button"
>
<svg
class="w-full h-auto"
Expand Down
107 changes: 44 additions & 63 deletions packages/fxa-settings/src/components/ThirdPartyAuth/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import React from 'react';
import { screen } from '@testing-library/react';
import { screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
import * as ReactUtils from 'fxa-react/lib/utils';
import { Subject } from './mocks';

const mockViewWithNoPasswordSet = jest.fn();
Expand All @@ -14,14 +16,14 @@ const mockStartGoogleAuthFromLogin = jest.fn();
const mockStartAppleAuthFromLogin = jest.fn();
const mockStartGoogleAuthFromReg = jest.fn();
const mockStartAppleAuthFromReg = jest.fn();
const mockGleanIsDone = jest.fn();
const mockGleanIsDone = jest.fn().mockResolvedValue(undefined);

jest.mock('../../lib/glean', () => {
return {
__esModule: true,
default: {
isDone: () => {
mockGleanIsDone();
return mockGleanIsDone();
},
emailFirst: {
googleOauthStart: () => {
Expand Down Expand Up @@ -56,22 +58,20 @@ function renderWith(props?: any) {
return renderWithLocalizationProvider(<Subject {...props} />);
}

const mockFormSubmit = jest.fn();
// jsdom does not implement HTMLFormElement.prototype.submit.
HTMLFormElement.prototype.submit = mockFormSubmit;

describe('ThirdPartyAuthComponent', () => {
// Form submission not supported in jest test. Instead, prevent the submission
// and illustrate a 'short circuit' operation.
const onSubmit = (e: any) => {
e.preventDefault();
return false;
};
const onContinueWithApple = jest.fn().mockImplementation(onSubmit);
const onContinueWithGoogle = jest.fn().mockImplementation(onSubmit);
const onContinueWithApple = jest.fn();
const onContinueWithGoogle = jest.fn();
let hardNavigateSpy: jest.SpyInstance;

beforeEach(() => {
hardNavigateSpy = jest
.spyOn(ReactUtils, 'hardNavigate')
.mockImplementation(() => {});
});

afterEach(() => {
jest.resetAllMocks();
hardNavigateSpy.mockRestore();
jest.clearAllMocks();
});

it('renders', async () => {
Expand All @@ -84,69 +84,68 @@ describe('ThirdPartyAuthComponent', () => {

await screen.findByLabelText('Continue with Google');
await screen.findByLabelText('Continue with Apple');
expect(mockFormSubmit).not.toHaveBeenCalled();

expect(
(await screen.findByTestId('google-signin-form-state')).getAttribute(
'value'
)
).toEqual('http%3A%2F%2Flocalhost%2F%3FflowId%3D123');

expect(
(await screen.findByTestId('apple-signin-form-state')).getAttribute(
'value'
)
).toEqual('http%3A%2F%2Flocalhost%2F%3FflowId%3D123');
});

it('submits apple form', async () => {
const user = userEvent.setup();
renderWith({
enabled: true,
showSeparator: true,
onContinueWithApple,
onContinueWithGoogle,
flowQueryParams: { flowId: '123' },
});

const button = await screen.findByLabelText('Continue with Apple');
button.click();
await user.click(button);

// Ensure the hidden input for state was updated.
expect(
(await screen.findByTestId('apple-signin-form-state')).getAttribute(
'value'
)
).not.toEqual('');
expect(onContinueWithApple).toHaveBeenCalled();
expect(onContinueWithGoogle).not.toHaveBeenCalled();
await waitFor(() => {
expect(hardNavigateSpy).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
state: 'http%3A%2F%2Flocalhost%2F%3FflowId%3D123',
}),
false,
false,
0
);
});
});

it('submits google form', async () => {
const user = userEvent.setup();
renderWith({
enabled: true,
showSeparator: true,
onContinueWithApple,
onContinueWithGoogle,
flowQueryParams: { flowId: '123' },
});

const button = await screen.findByLabelText('Continue with Google');
button.click();
await user.click(button);

// Ensure the hidden input for state was updated.
expect(
(await screen.findByTestId('google-signin-form-state')).getAttribute(
'value'
)
).not.toEqual('');
expect(onContinueWithGoogle).toHaveBeenCalled();
expect(onContinueWithApple).not.toHaveBeenCalled();
await waitFor(() => {
expect(hardNavigateSpy).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
state: 'http%3A%2F%2Flocalhost%2F%3FflowId%3D123',
}),
false,
false,
0
);
});
});

it('hides separator', async () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
});

expect(screen.queryByText('Or')).toBeNull();
Expand All @@ -157,8 +156,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: true,
onContinueWithApple,
onContinueWithGoogle,
});

expect(screen.queryByText('Or')).toBeDefined();
Expand All @@ -168,8 +165,6 @@ describe('ThirdPartyAuthComponent', () => {
it('buttons match snapshot', async () => {
renderWith({
enabled: true,
onContinueWithApple,
onContinueWithGoogle,
});

const googleButton = await screen.findByLabelText('Continue with Google');
Expand All @@ -183,8 +178,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
});
expect(mockViewWithNoPasswordSet).toHaveBeenCalled();
});
Expand All @@ -193,8 +186,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'index',
});
const button = await screen.findByLabelText('Continue with Google');
Expand All @@ -207,8 +198,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'index',
});
const button = await screen.findByLabelText('Continue with Apple');
Expand All @@ -220,8 +209,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'signin',
});
const button = await screen.findByLabelText('Continue with Google');
Expand All @@ -234,8 +221,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'signin',
});
const button = await screen.findByLabelText('Continue with Apple');
Expand All @@ -247,8 +232,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'signup',
});
const button = await screen.findByLabelText('Continue with Google');
Expand All @@ -261,8 +244,6 @@ describe('ThirdPartyAuthComponent', () => {
renderWith({
enabled: true,
showSeparator: false,
onContinueWithApple,
onContinueWithGoogle,
viewName: 'signup',
});
const button = await screen.findByLabelText('Continue with Apple');
Expand Down
Loading
Loading