Conversation
| queryParamModel: { email: 'test@example.com' }, | ||
| validationError: null, | ||
| }); | ||
| const gleanSubmitSuccessSpy = jest.spyOn( |
There was a problem hiding this comment.
The necessity to hoist this is up is because the event can now happen in a useEffect before any page interactions, so we need the spy first. Then, to be consistent all the tests are updated to a similar state
There was a problem hiding this comment.
Pull request overview
This PR updates email-first Glean telemetry so that success/failure events are emitted for both manual submissions and automatic submissions (prefilled via query params or cached account), distinguishing auto-submits by appending a -auto suffix to the reason extra key.
Changes:
- Emit
emailFirst.submitSuccessfor both manual and auto-submit flows, usinglogin[-auto]/registration[-auto]reasons. - Emit
emailFirst.submitFailfor both manual and auto-submit flows, using the same reason suffixing. - Update Glean metric descriptions and expand unit tests to cover the new
-autoreason behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fxa-shared/metrics/glean/web/email.ts | Updates generated metric docstrings to reflect auto-submission behavior. |
| packages/fxa-shared/metrics/glean/fxa-ui-metrics.yaml | Documents new reason values (login-auto, registration-auto) for submit success/fail events. |
| packages/fxa-settings/src/pages/Index/container.tsx | Emits success/fail Glean events for auto-submit with -auto suffix. |
| packages/fxa-settings/src/pages/Index/container.test.tsx | Adds/updates tests asserting Glean emission for auto-submit success and failure cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GleanMetrics.emailFirst.submitSuccess({ | ||
| event: { | ||
| reason: `${accountExists ? 'login' : 'registration'}${ | ||
| isManualSubmission ? '' : '-auto' |
There was a problem hiding this comment.
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)
| }); | ||
| }); | ||
|
|
||
| it('emits submitFail with reason "login" when the user cancels account linking', async () => { |
There was a problem hiding this comment.
We didn't have tests to cover the failure state and making sure we emit the correct events, just adding them here
Becuase: - We have a bug where events are not emitted when there is an email query param This Commit: - Adds a sufix to the emailFirstSuccess and emailFirstFailure event reasons to indicate if it was an auto-submittal Closes: FXA-13703
|
Latest force push was to address another, related bug. See here |
Becuase:
This Commit:
Closes: FXA-13703
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.