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(3932): Fix User traits changed during onboarding are never sent to Segment #29865

Closed
wants to merge 1 commit into from

Conversation

DDDDDanica
Copy link
Contributor

@DDDDDanica DDDDDanica commented Jan 23, 2025

Description

User traits are not logged during onboarding (including the initial values, and any changes made during onboarding).
After following the steps https://github.com/MetaMask/MetaMask-planning/issues/3932#issuecomment-2604742483, The data changes are as follows:

  • initialize the extension with default state
    {participateInMetaMetrics : null, dataCollectionForMarketing: null}
  • during onboarding, when we change participateInMetaMetrics to false by clicking "No thanks", and state is {participateInMetaMetrics : false, dataCollectionForMarketing: null}
  • after onboarding, user navigates to privacy settings, and toggle on "Participate in MetaMetrics", state is {participateInMetaMetrics : true, dataCollectionForMarketing: null}

=> the state of dataCollectionForMarketing is not changed from this action, hence the batch request will not include unchanged value dataCollectionForMarketing, which means has_marketing_consent is not going to be included in first identify call to segment.

Hence the solution is to modify the value of dataCollectionForMarketing to false in the state and the difference of state will be picked up and included in traits sent to segment in identify call.

You can find some e2e tests covering the situation in test/e2e/tests/metrics/segment-user-traits.spec.ts

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3932

Manual testing steps

  • Install the MetaMask extension
  • Must use production build, or build with SEGMENT_WRITE_KEY and SEGMENT_HOST set
  • See here for more information on how to set these in a dev build: https://github.com/MetaMask/metamask-extension/tree/main/development#debugging-with-the-mock-segment-api
  • Open dev tools for the background process (background.html / service worker)
  • In the dev tools networks tab, filter for Segment requests
  • For a production build, filter to segment. For a dev build, filter to whatever you set for SEGMENT_HOST
  • Proceed through onboarding, opting out of metametrics and marketing data consent
  • After onboarding, go to privacy settings and enable "Participate in metametrics"
  • You should see a request with type: "identify" to Segment, it comes with traits of {has_marketing_consent : true, is_metrics_opted_in: false}

Screenshots/Recordings

Before

After

https://www.loom.com/share/9375153003c44231a65a685305ec1828?sid=c58f98f0-6fba-400f-8f96-8e53c316879f

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.

@DDDDDanica DDDDDanica self-assigned this Jan 23, 2025
@DDDDDanica DDDDDanica requested a review from a team as a code owner January 23, 2025 00:25
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.


if (
participateInMetaMetrics &&
this.state.dataCollectionForMarketing === null
Copy link
Member

@Gudahtt Gudahtt Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effectively this is working around the underlying bug, which is that initial values for traits (and modifications to traits while metrics are disabled) are never sent to Segment. Why not solve the underlying problem? The issue still affects every other trait.

This also fails to meet a design goal in the implementation of this work, which is to ensure the user has been prompted to make a decision about the marketing consent toggle one way or the other. null represents the state where they haven't made a decision. If we overwrite it with false, it may result in some users not getting the prompt (e.g. for an existing user, if they enable metrics in settings).

@DDDDDanica
Copy link
Contributor Author

This PR will provide the solution to fix the issue from the root instead of only covering one case
#29871

@DDDDDanica DDDDDanica closed this Jan 31, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants