Skip to content
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

fix: Remove previousUserTraits from metametrics controller state #30621

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Feb 27, 2025

Description

Truly solving https://github.com/MetaMask/MetaMask-planning/issues/3932 required us to clear previousUserTraits from metametrics controller state. That property just functions as a cache, and we could actually maintain it in memory.

The reason we need to clear it is that there are many users who had the has_marketing_consent trait populated to true when the application had a bug that prevented that trait from being submitted to segment. That bug is fixed, so new users don't hit that problem. However, to correctly track existing users that have has_marketing_consent set to true, we need the comparison of previous and current user traits in the _buildUserTraitsObject function to fail its equality check.

To do that, we are clearing previousUserTraits so that, upon update of the extension and when the first metrics event is set to segment, the current user trait values will compare to undefined, and they will all be submitted to segment, including has_marketing_consent.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Install a build from this branch
  2. Open the background console and the network tab
  3. Start onboarding. On the metametrics screen, click the checkbox and then "I agree"
  4. The network tab should now include a request to segment with user traits, where has_marketing_consent is set to true

--

  1. Follow the above steps on the v12.9.3 build (step 4 will fail)
  2. Update the version of that install to the build from this branch
  3. Log in.
  4. The network tab should now include a request to segment with user traits, where has_marketing_consent is set to true

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

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.

@metamaskbot metamaskbot added the team-extension-platform Extension Platform team label Feb 27, 2025
@danjm danjm changed the title Remove previousUserTraits from metametrics controller state fix: Remove previousUserTraits from metametrics controller state Feb 28, 2025
@danjm danjm force-pushed the clear-previousUserTraits branch from a4f0ac1 to 6701546 Compare February 28, 2025 19:22
@metamaskbot
Copy link
Collaborator

Builds ready [41a1918]
Page Load Metrics (1593 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint35120651545321154
domContentLoaded13742049156516881
load13772069159317082
domInteractive237939199
backgroundConnect8115332613
firstReactRender147127209
getState35811147
initialActions01000
loadScripts9881566116414570
setupStore75615167
uiStartup15882390181619694
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 694 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: -22 Bytes (-0.00%)

@DDDDDanica
Copy link
Contributor

LGTM ! Can approve once the unit test is fixed

@metamaskbot
Copy link
Collaborator

Builds ready [ecceb13]
Page Load Metrics (1708 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14222105170920297
domContentLoaded14072059167819996
load14202096170820498
domInteractive25142493316
backgroundConnect118032199
firstReactRender1467302010
getState56117189
initialActions01000
loadScripts10291504125015976
setupStore76418189
uiStartup158623651936227109
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 694 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: -22 Bytes (-0.00%)

@desi desi added this pull request to the merge queue Mar 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2025
@desi desi added this pull request to the merge queue Mar 5, 2025
Merged via the queue into main with commit b6d7bc9 Mar 5, 2025
74 checks passed
@desi desi deleted the clear-previousUserTraits branch March 5, 2025 15:30
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2025
@metamaskbot metamaskbot added the release-12.15.0 Issue or pull request that will be included in release 12.15.0 label Mar 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-attribution release-12.15.0 Issue or pull request that will be included in release 12.15.0 team-extension-platform Extension Platform team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants