-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: remove metametrics and update choose password screen #19077
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
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
|
| "createDataDeletionTask": [MockFunction], | ||
| "createEventBuilder": [MockFunction], | ||
| "enable": [MockFunction], | ||
| "enableSocialLogin": undefined, |
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.
why its not a mockFunction like rest ?
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.
Resolved
| * @returns Boolean indicating if MetaMetrics is enabled or disabled | ||
| */ | ||
| isEnabled = () => this.enabled; | ||
| isEnabled = () => this.isSocialLoginEnabled || this.enabled; |
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.
Bug: Metrics Enablement Inconsistency
The isEnabled() method now returns true if any metrics (social login or general) are enabled. This change could inadvertently enable all metrics tracking, potentially violating user privacy if a user opted out of general metrics. It also creates an inconsistency where isEnabled() might report metrics are active, but actual event tracking (which still checks only this.enabled) won't occur if only social login metrics are enabled.
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.
We always reset isSocialLoginEnabled to false when user start general (srp) login flow, so it wont create any issues.
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.
Can we add that to the description?
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.
Bug: Metrics Reporting Inconsistency
The isEnabled() method now combines isSocialLoginEnabled and enabled with an OR operator. This conflates two distinct metrics states, causing isEnabled() to report metrics as active even when regular metrics are disabled. This creates an inconsistency with trackEvent(), which relies solely on this.enabled, resulting in isEnabled() reporting active metrics while no events are actually tracked.
app/core/Analytics/MetaMetrics.ts#L659-L660
metamask-mobile/app/core/Analytics/MetaMetrics.ts
Lines 659 to 660 in 1de40e0
| */ | |
| isEnabled = () => this.isSocialLoginEnabled || this.enabled; |
app/core/Analytics/MetaMetrics.ts#L725-L729
metamask-mobile/app/core/Analytics/MetaMetrics.ts
Lines 725 to 729 in 1de40e0
| * .addSensitiveProperties({ sensitiveProp: 'value' }) | |
| * .build()); | |
| * | |
| * @param event - Analytics event built with {@link MetricsEventBuilder} | |
| * @param saveDataRecording - param to skip saving the data recording flag (optional) |
| const { loading, isSelected, password, confirmPassword } = this.state; | ||
| const passwordsMatch = password !== '' && password === confirmPassword; | ||
| const canSubmit = passwordsMatch && isSelected; | ||
| const canSubmit = passwordsMatch; |
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.
Bug: Inconsistent Form Validation and User Experience
The canSubmit logic was simplified, removing the isSelected checkbox requirement from form submission for all users. This creates an inconsistent user experience for SRP users, who see a password recovery warning checkbox that appears required but isn't enforced on submission. Further, onPressCreate's canSubmit check omits the MIN_PASSWORD_LENGTH validation present in renderContent, potentially enabling the submit button when the password is too short.
Additional Locations (1)
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.
Code outdated
| canSubmit = passwordsMatch; | ||
| } else { | ||
| canSubmit = passwordsMatch && isSelected; | ||
| } |
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.
Bug: OAuth Password Validation Mismatch
The ChoosePassword component has inconsistent password validation for OAuth users. The onPressCreate method's canSubmit check only verifies matching passwords, while the renderContent method's canSubmit for the UI button also requires the minimum password length. This creates a confusing user experience where the button state doesn't accurately reflect submission requirements.
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 button activates only after minimum password length condition meets.
|
himanshuchawla009
left a comment
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.
lgtm, some tests are duplicate and needs better description. We can improve it in upcoming PR.



Description
Changelog
CHANGELOG entry: 1) Remove metametrics: https://consensyssoftware.atlassian.net/browse/SL-128
2) Update change password screen: https://consensyssoftware.atlassian.net/browse/SL-128
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
old_flow.mp4
After
New_flow.mp4
Screen.Recording.2025-09-03.at.3.45.30.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist