-
Notifications
You must be signed in to change notification settings - Fork 230
fix(settings): Add new glean reasons for email-first auto submit #20591
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -378,6 +378,10 @@ describe('IndexContainer', () => { | |
| queryParamModel: { email: 'test@example.com' }, | ||
| validationError: null, | ||
| }); | ||
| const gleanSubmitSuccessSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitSuccess' | ||
| ); | ||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
|
|
@@ -391,12 +395,11 @@ describe('IndexContainer', () => { | |
| await waitFor(() => { | ||
| expect(mockNavigate).toHaveBeenCalledTimes(1); | ||
| }); | ||
| // Glean event not emitted on automatic redirect, only on successful manual submission | ||
| const gleanSubmitSuccessSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitSuccess' | ||
| ); | ||
| expect(gleanSubmitSuccessSpy).not.toHaveBeenCalled(); | ||
| // Auto-submit emits with the '-auto' reason suffix, | ||
| // so we can differentiate from manual submission | ||
| expect(gleanSubmitSuccessSpy).toHaveBeenCalledWith({ | ||
| event: { reason: 'login-auto' }, | ||
| }); | ||
| const [calledUrl, options] = mockNavigate.mock.calls[0]; | ||
| expect(calledUrl).toMatch(/\/signin$/); | ||
| expect(options).toEqual({ | ||
|
|
@@ -423,6 +426,10 @@ describe('IndexContainer', () => { | |
| queryParamModel: { email: 'test@example.com' }, | ||
| validationError: null, | ||
| }); | ||
| const gleanSubmitSuccessSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitSuccess' | ||
| ); | ||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
|
|
@@ -436,12 +443,10 @@ describe('IndexContainer', () => { | |
| await waitFor(() => { | ||
| expect(mockNavigate).toHaveBeenCalledTimes(1); | ||
| }); | ||
| // Glean event not emitted on automatic redirect, only on successful manual submission | ||
| const gleanSubmitSuccessSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitSuccess' | ||
| ); | ||
| expect(gleanSubmitSuccessSpy).not.toHaveBeenCalled(); | ||
|
|
||
| expect(gleanSubmitSuccessSpy).toHaveBeenCalledWith({ | ||
| event: { reason: 'registration-auto' }, | ||
| }); | ||
| const [calledUrl, options] = mockNavigate.mock.calls[0]; | ||
| expect(calledUrl).toMatch(/\/signup$/); | ||
| expect(options).toEqual({ | ||
|
|
@@ -1043,6 +1048,152 @@ describe('IndexContainer', () => { | |
| event: { reason: 'registration' }, | ||
| }); | ||
| }); | ||
|
|
||
| it('emits submitFail with reason "login" when the user cancels account linking', async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We didn't have tests to cover the failure state and making sure we emit the correct events, just adding them here |
||
| mockOAuthNativeIntegration(); | ||
| mockUseFxAStatusResult = mockUseFxAStatus({ | ||
| supportsCanLinkAccountUid: false, | ||
| }); | ||
| (firefox.fxaCanLinkAccount as jest.Mock).mockResolvedValue({ | ||
| ok: false, | ||
| }); | ||
|
|
||
| const gleanSubmitFailSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitFail' | ||
| ); | ||
|
|
||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
| integration, | ||
| serviceName: MozServices.Default, | ||
| useFxAStatusResult: mockUseFxAStatusResult, | ||
| }} | ||
| /> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(currentIndexProps?.processEmailSubmission).toBeDefined(); | ||
| }); | ||
|
|
||
| await act(async () => { | ||
| await currentIndexProps?.processEmailSubmission(MOCK_EMAIL); | ||
| }); | ||
|
|
||
| expect(gleanSubmitFailSpy).toHaveBeenCalledWith({ | ||
| event: { reason: 'login' }, | ||
| }); | ||
| }); | ||
|
|
||
| it('emits submitFail with reason "registration-auto" when auto-submit fails domain validation', async () => { | ||
| mockUseValidatedQueryParams.mockReturnValue({ | ||
| queryParamModel: { email: 'test@example.com' }, | ||
| validationError: null, | ||
| }); | ||
| mockUseAuthClient.mockReturnValue({ | ||
| accountStatusByEmail: jest.fn().mockResolvedValue({ | ||
| exists: false, | ||
| hasLinkedAccount: false, | ||
| hasPassword: false, | ||
| }), | ||
| }); | ||
| (checkEmailDomain as jest.Mock).mockRejectedValueOnce( | ||
| AuthUiErrors.INVALID_EMAIL_DOMAIN | ||
| ); | ||
|
|
||
| const gleanSubmitFailSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitFail' | ||
| ); | ||
|
|
||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
| integration, | ||
| serviceName: MozServices.Default, | ||
| useFxAStatusResult: mockUseFxAStatusResult, | ||
| }} | ||
| /> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(gleanSubmitFailSpy).toHaveBeenCalledWith({ | ||
| event: { reason: 'registration-auto' }, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('does not emit submitFail when accountStatusByEmail rejects before accountExists is known', async () => { | ||
| // If we reach the catch before accountStatusByEmail resolves we can't | ||
| // attribute the failure to login vs registration, so the event should | ||
| // be skipped rather than recording a misleading reason. | ||
| mockUseAuthClient.mockReturnValue({ | ||
| accountStatusByEmail: jest | ||
| .fn() | ||
| .mockRejectedValue(new Error('network error')), | ||
| }); | ||
|
|
||
| const gleanSubmitFailSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitFail' | ||
| ); | ||
|
|
||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
| integration, | ||
| serviceName: MozServices.Default, | ||
| useFxAStatusResult: mockUseFxAStatusResult, | ||
| }} | ||
| /> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(currentIndexProps?.processEmailSubmission).toBeDefined(); | ||
| }); | ||
|
|
||
| await act(async () => { | ||
| await currentIndexProps?.processEmailSubmission(MOCK_EMAIL); | ||
| }); | ||
|
|
||
| expect(gleanSubmitFailSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('emits submitFail with reason "login-auto" when auto-submit hits a canceled can_link_account', async () => { | ||
| mockOAuthNativeIntegration(); | ||
| mockUseFxAStatusResult = mockUseFxAStatus({ | ||
| supportsCanLinkAccountUid: false, | ||
| }); | ||
| mockUseValidatedQueryParams.mockReturnValue({ | ||
| queryParamModel: { email: MOCK_EMAIL }, | ||
| validationError: null, | ||
| }); | ||
| (firefox.fxaCanLinkAccount as jest.Mock).mockResolvedValue({ | ||
| ok: false, | ||
| }); | ||
|
|
||
| const gleanSubmitFailSpy = jest.spyOn( | ||
| GleanMetrics.emailFirst, | ||
| 'submitFail' | ||
| ); | ||
|
|
||
| renderWithLocalizationProvider( | ||
| <IndexContainer | ||
| {...{ | ||
| integration, | ||
| serviceName: MozServices.Default, | ||
| useFxAStatusResult: mockUseFxAStatusResult, | ||
| }} | ||
| /> | ||
| ); | ||
|
|
||
| await waitFor(() => { | ||
| expect(gleanSubmitFailSpy).toHaveBeenCalledWith({ | ||
| event: { reason: 'login-auto' }, | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should redirect cached passwordless account to signin (not OTP) when sessionToken exists', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -328,10 +328,13 @@ const IndexContainer = ({ | |
| canLinkAccountOk = true; | ||
| } | ||
|
|
||
| isManualSubmission && | ||
| GleanMetrics.emailFirst.submitSuccess({ | ||
| event: { reason: accountExists ? 'login' : 'registration' }, | ||
| }); | ||
| GleanMetrics.emailFirst.submitSuccess({ | ||
| event: { | ||
| reason: `${accountExists ? 'login' : 'registration'}${ | ||
| isManualSubmission ? '' : '-auto' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the meat and potatoes, add a suffix to the event if it's not a manual submit so we can differentiate manual vs auto (from email query param) |
||
| }`, | ||
| }, | ||
| }); | ||
|
|
||
| handleSuccessNavigation( | ||
| exists, | ||
|
|
@@ -342,9 +345,17 @@ const IndexContainer = ({ | |
| passwordlessSupported | ||
| ); | ||
| } catch (error) { | ||
| if (isManualSubmission && isEmail(email)) { | ||
| // If we reach the catch before accountStatusByEmail resolved (e.g. a | ||
| // network error), accountExists is undefined and we can't attribute | ||
| // the failure to login vs registration. Skip the event in that case | ||
| // rather than recording a misleading reason. | ||
| if (isEmail(email) && typeof accountExists === 'boolean') { | ||
| GleanMetrics.emailFirst.submitFail({ | ||
| event: { reason: accountExists ? 'login' : 'registration' }, | ||
| event: { | ||
| reason: `${accountExists ? 'login' : 'registration'}${ | ||
| isManualSubmission ? '' : '-auto' | ||
| }`, | ||
| }, | ||
| }); | ||
|
nshirley marked this conversation as resolved.
|
||
| } | ||
| // if email verification fails, clear from params to avoid re-use | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The necessity to hoist this is up is because the event can now happen in a
useEffectbefore any page interactions, so we need the spy first. Then, to be consistent all the tests are updated to a similar state