fix: Ensure has_marketing_consent user trait are correctly set and se…

…nt after participateInMetaMetrics is set to true (#29871)  ## **Description** It was observed in production metrics that the `has_marketing_consent` user trait was not set (either `null` or `undefined`) for 40-50% of users. Meanwhile, another ~40% of users had it set to false, and the remainder had it set to true. The problem was in the metametrics controller. The controller sets `state.previousUserTraits` in the `_buildUserTraitsObject` function, which gets called when there are state updates. That function also uses `state.previousUserTraits` to determine which values need to be returned, by comparing it to the new `currentTraits`. The problem is that if a trait is set while `participateInMetaMetrics` is false, then the trait will not be sent in a request to Segment, BUT the trait will be saved in `state.previousUserTraits`. Later, if `participateInMetaMetrics` is set to true, `_buildUserTraitsObject` will not return a trait currently matches `state.previousUserTraits` The `has_marketing_consent` trait is set to true when the user checks the associated checkbox on the metametrics screen of the onboarding flow, but then if the user clicks confirm/yes on that screen to opt-in to metametrics, the aforementioned problem is hit. To solve this, we just ensure that `state.previousUserTraits` is not saved if `participateInMetaMetrics` is false. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29871?quickstart=1) ## **Related issues** Fixes: MetaMask/MetaMask-planning#3932 ## **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 -- 1. Install a build from this branch 2. Open the background console and the network tab 3. Start onboarding. On the metametrics screen, DO NOT click the checkbox and then click "No thanks" 4. The network tab should not include a request to segment 5. Complete onboarding and go to settings and toggle "Participate in MetaMetrics" to true 6. The network tab tab should now include a request to segment with user traits, where `has_marketing_consent` is set to false. If the marketing consent toggle is turned on, there should then be a network request where the user trait is true > [!IMPORTANT] > This requires manual test from QA. - Create a new user and click both "I agree" and "we'll use this data... " when user is on "help us improve Metamask" page - There should be 1 request to segment with user traits, where both has_marketing_consent and participate_in_metametrics set to true. - Proceed on onboarding - Head to privacy page in settings - Opt out "participate in metametrics" - There's no more request to segment after ## **Screenshots/Recordings** https://github.com/user-attachments/assets/0d335973-eb52-4f61-9709-2999efde4021 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --------- Co-authored-by: dddddanica <[email protected]>
MetaMask · Feb 6, 2025 · c4aa6a9 · c4aa6a9
1 parent ba5f5b1
commit c4aa6a9
Show file tree

Hide file tree

Showing 14 changed files with 240 additions and 16 deletions.
diff --git a/app/scripts/controllers/metametrics-controller.ts b/app/scripts/controllers/metametrics-controller.ts
@@ -1239,7 +1239,7 @@ export default class MetaMetricsController extends BaseController<
       ),
     };
 
-    if (!previousUserTraits) {
+    if (!previousUserTraits && metamaskState.participateInMetaMetrics) {
       this.update((state) => {
         state.previousUserTraits = currentTraits;
       });
@@ -1252,9 +1252,13 @@ export default class MetaMetricsController extends BaseController<
         const previous = previousUserTraits[k];
         return !isEqual(previous, v);
       });
-      this.update((state) => {
-        state.previousUserTraits = currentTraits;
-      });
+
+      if (metamaskState.participateInMetaMetrics) {
+        this.update((state) => {
+          state.previousUserTraits = currentTraits;
+        });
+      }
+
       return updates;
     }
 

diff --git a/test/e2e/page-objects/flows/onboarding.flow.ts b/test/e2e/page-objects/flows/onboarding.flow.ts
@@ -17,17 +17,20 @@ import { E2E_SRP } from '../../default-fixture';
  * @param [options.password] - The password to create. Defaults to WALLET_PASSWORD.
  * @param [options.participateInMetaMetrics] - Whether to participate in MetaMetrics. Defaults to false.
  * @param [options.needNavigateToNewPage] - Indicates whether to navigate to a new page before starting the onboarding flow. Defaults to true.
+ * @param [options.dataCollectionForMarketing] - Whether to opt in to data collection for marketing. Defaults to false.
  */
 export const createNewWalletOnboardingFlow = async ({
   driver,
   password = WALLET_PASSWORD,
   participateInMetaMetrics = false,
   needNavigateToNewPage = true,
+  dataCollectionForMarketing = false,
 }: {
   driver: Driver;
   password?: string;
   participateInMetaMetrics?: boolean;
   needNavigateToNewPage?: boolean;
+  dataCollectionForMarketing?: boolean;
 }): Promise<void> => {
   console.log('Starting the creation of a new wallet onboarding flow');
   if (needNavigateToNewPage) {
@@ -40,6 +43,9 @@ export const createNewWalletOnboardingFlow = async ({
 
   const onboardingMetricsPage = new OnboardingMetricsPage(driver);
   await onboardingMetricsPage.check_pageIsLoaded();
+  if (dataCollectionForMarketing) {
+    await onboardingMetricsPage.clickDataCollectionForMarketingCheckbox();
+  }
   if (participateInMetaMetrics) {
     await onboardingMetricsPage.clickIAgreeButton();
   } else {
@@ -109,24 +115,28 @@ export const importSRPOnboardingFlow = async ({
  * @param [options.password] - The password to use. Defaults to WALLET_PASSWORD.
  * @param [options.participateInMetaMetrics] - Whether to participate in MetaMetrics. Defaults to false.
  * @param [options.needNavigateToNewPage] - Indicates whether to navigate to a new page before starting the onboarding flow. Defaults to true.
+ * @param [options.dataCollectionForMarketing] - Whether to opt in to data collection for marketing. Defaults to false.
  */
 export const completeCreateNewWalletOnboardingFlow = async ({
   driver,
   password = WALLET_PASSWORD,
   participateInMetaMetrics = false,
   needNavigateToNewPage = true,
+  dataCollectionForMarketing = false,
 }: {
   driver: Driver;
   password?: string;
   participateInMetaMetrics?: boolean;
   needNavigateToNewPage?: boolean;
+  dataCollectionForMarketing?: boolean;
 }): Promise<void> => {
   console.log('start to complete create new wallet onboarding flow ');
   await createNewWalletOnboardingFlow({
     driver,
     password,
     participateInMetaMetrics,
     needNavigateToNewPage,
+    dataCollectionForMarketing,
   });
   const onboardingCompletePage = new OnboardingCompletePage(driver);
   await onboardingCompletePage.check_pageIsLoaded();

diff --git a/test/e2e/page-objects/pages/onboarding/onboarding-metrics-page.ts b/test/e2e/page-objects/pages/onboarding/onboarding-metrics-page.ts
@@ -5,6 +5,9 @@ class OnboardingMetricsPage {
 
   private readonly iAgreeButton = '[data-testid="metametrics-i-agree"]';
 
+  private readonly dataCollectionForMarketingCheckbox =
+    '[data-testid="metametrics-data-collection-checkbox"]';
+
   private readonly metametricsMessage = {
     text: 'Help us improve MetaMask',
     tag: 'h2',
@@ -39,6 +42,10 @@ class OnboardingMetricsPage {
   async clickIAgreeButton(): Promise<void> {
     await this.driver.clickElementAndWaitToDisappear(this.iAgreeButton);
   }
+
+  async clickDataCollectionForMarketingCheckbox(): Promise<void> {
+    await this.driver.clickElement(this.dataCollectionForMarketingCheckbox);
+  }
 }
 
 export default OnboardingMetricsPage;
diff --git a/test/e2e/page-objects/pages/settings/privacy-settings.ts b/test/e2e/page-objects/pages/settings/privacy-settings.ts
@@ -85,6 +85,12 @@ class PrivacySettings {
 
   private readonly revealSrpWrongPasswordMessage = '.mm-help-text';
 
+  private readonly participateInMetaMetricsToggle =
+    '[data-testid="participate-in-meta-metrics-toggle"] .toggle-button';
+
+  private readonly dataCollectionForMarketingToggle =
+    '[data-testid="data-collection-for-marketing-toggle"] .toggle-button';
+
   constructor(driver: Driver) {
     this.driver = driver;
   }
@@ -212,6 +218,20 @@ class PrivacySettings {
       text: expectedSrpText,
     });
   }
+
+  async toggleParticipateInMetaMetrics(): Promise<void> {
+    console.log(
+      'Toggle participate in meta metrics in Security and Privacy settings page',
+    );
+    await this.driver.clickElement(this.participateInMetaMetricsToggle);
+  }
+
+  async toggleDataCollectionForMarketing(): Promise<void> {
+    console.log(
+      'Toggle data collection for marketing in Security and Privacy settings page',
+    );
+    await this.driver.clickElement(this.dataCollectionForMarketingToggle);
+  }
 }
 
 export default PrivacySettings;
diff --git a/test/e2e/tests/metrics/marketing-cookieid.spec.ts b/test/e2e/tests/metrics/marketing-cookieid.spec.ts
@@ -18,7 +18,7 @@ const selectors = {
   globalMenuSettingsButton: '[data-testid="global-menu-settings"]',
   securityAndPrivacySettings: { text: 'Security & privacy', tag: 'div' },
   dataCollectionForMarketingToggle:
-    '[data-testid="dataCollectionForMarketing"] .toggle-button',
+    '[data-testid="data-collection-for-marketing-toggle"] .toggle-button',
   dataCollectionWarningAckButton: { text: 'Okay', tag: 'Button' },
 };
 

diff --git a/test/e2e/tests/metrics/metametrics-persistence.spec.js b/test/e2e/tests/metrics/metametrics-persistence.spec.js
@@ -49,7 +49,7 @@ describe('MetaMetrics ID persistence', function () {
 
         // toggle off
         await driver.clickElement(
-          '[data-testid="participateInMetaMetrics"] .toggle-button',
+          '[data-testid="participate-in-meta-metrics-toggle"] .toggle-button',
         );
 
         // wait for state to update
@@ -65,7 +65,7 @@ describe('MetaMetrics ID persistence', function () {
 
         // toggle back on
         await driver.clickElement(
-          '[data-testid="participateInMetaMetrics"] .toggle-button',
+          '[data-testid="participate-in-meta-metrics-toggle"] .toggle-button',
         );
 
         // wait for state to update

diff --git a/test/e2e/tests/metrics/segment-user-traits.spec.ts b/test/e2e/tests/metrics/segment-user-traits.spec.ts
@@ -0,0 +1,176 @@
+import { strict as assert } from 'assert';
+import { Mockttp } from 'mockttp';
+import { getEventPayloads, withFixtures } from '../../helpers';
+import FixtureBuilder from '../../fixture-builder';
+import {
+  completeCreateNewWalletOnboardingFlow,
+  createNewWalletOnboardingFlow,
+} from '../../page-objects/flows/onboarding.flow';
+import { MOCK_META_METRICS_ID } from '../../constants';
+import HeaderNavbar from '../../page-objects/pages/header-navbar';
+import SettingsPage from '../../page-objects/pages/settings/settings-page';
+import PrivacySettings from '../../page-objects/pages/settings/privacy-settings';
+
+async function mockSegment(mockServer: Mockttp) {
+  return [
+    await mockServer
+      .forPost('https://api.segment.io/v1/batch')
+      .withJsonBodyIncluding({
+        batch: [{ type: 'identify' }],
+      })
+      .thenCallback(() => {
+        return {
+          statusCode: 200,
+        };
+      }),
+  ];
+}
+
+describe('Segment User Traits', function () {
+  it('sends identify event when user opts in both metrics and data collection during onboarding', async function () {
+    await withFixtures(
+      {
+        fixtures: new FixtureBuilder({ onboarding: true })
+          .withMetaMetricsController({
+            metaMetricsId: MOCK_META_METRICS_ID,
+          })
+          .build(),
+        title: this.test?.fullTitle(),
+        testSpecificMock: mockSegment,
+      },
+      async ({ driver, mockedEndpoint: mockedEndpoints }) => {
+        await createNewWalletOnboardingFlow({
+          driver,
+          participateInMetaMetrics: true,
+          dataCollectionForMarketing: true,
+        });
+        const events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 1);
+        assert.deepStrictEqual(events[0].traits.is_metrics_opted_in, true);
+        assert.deepStrictEqual(events[0].traits.has_marketing_consent, true);
+      },
+    );
+  });
+
+  it('sends identify event when user opts into metrics but not data collection during onboarding', async function () {
+    await withFixtures(
+      {
+        fixtures: new FixtureBuilder({ onboarding: true })
+          .withMetaMetricsController({
+            metaMetricsId: MOCK_META_METRICS_ID,
+          })
+          .build(),
+        title: this.test?.fullTitle(),
+        testSpecificMock: mockSegment,
+      },
+      async ({ driver, mockedEndpoint: mockedEndpoints }) => {
+        await createNewWalletOnboardingFlow({
+          driver,
+          participateInMetaMetrics: true,
+          dataCollectionForMarketing: false,
+        });
+        const events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 1);
+        assert.deepStrictEqual(events[0].traits.is_metrics_opted_in, true);
+        assert.deepStrictEqual(events[0].traits.has_marketing_consent, false);
+      },
+    );
+  });
+
+  it('will not send identify event when user opts out of both metrics and data collection during onboarding', async function () {
+    await withFixtures(
+      {
+        fixtures: new FixtureBuilder({ onboarding: true })
+          .withMetaMetricsController({
+            metaMetricsId: MOCK_META_METRICS_ID,
+            participateInMetaMetrics: true,
+          })
+          .build(),
+        title: this.test?.fullTitle(),
+        testSpecificMock: mockSegment,
+      },
+      async ({ driver, mockedEndpoint: mockedEndpoints }) => {
+        await createNewWalletOnboardingFlow({
+          driver,
+          participateInMetaMetrics: false,
+          dataCollectionForMarketing: false,
+        });
+        const events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 0);
+      },
+    );
+  });
+
+  it('sends identify event when user enables metrics in privacy settings after opting out during onboarding', async function () {
+    await withFixtures(
+      {
+        fixtures: new FixtureBuilder({ onboarding: true })
+          .withMetaMetricsController({
+            metaMetricsId: MOCK_META_METRICS_ID,
+            participateInMetaMetrics: false,
+          })
+          .build(),
+        title: this.test?.fullTitle(),
+        testSpecificMock: mockSegment,
+      },
+      async ({ driver, mockedEndpoint: mockedEndpoints }) => {
+        let events = [];
+        await completeCreateNewWalletOnboardingFlow({
+          driver,
+          participateInMetaMetrics: false,
+        });
+        events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 0);
+        await new HeaderNavbar(driver).openSettingsPage();
+        const settingsPage = new SettingsPage(driver);
+        await settingsPage.check_pageIsLoaded();
+        await settingsPage.goToPrivacySettings();
+
+        const privacySettings = new PrivacySettings(driver);
+        await privacySettings.check_pageIsLoaded();
+        await privacySettings.toggleParticipateInMetaMetrics();
+        events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 1);
+        assert.deepStrictEqual(events[0].traits.is_metrics_opted_in, true);
+        assert.deepStrictEqual(events[0].traits.has_marketing_consent, false);
+      },
+    );
+  });
+
+  it('sends identify event when user opts in both metrics and data in privacy settings after opting out during onboarding', async function () {
+    await withFixtures(
+      {
+        fixtures: new FixtureBuilder({ onboarding: true })
+          .withMetaMetricsController({
+            metaMetricsId: MOCK_META_METRICS_ID,
+            participateInMetaMetrics: false,
+          })
+          .build(),
+        title: this.test?.fullTitle(),
+        testSpecificMock: mockSegment,
+      },
+      async ({ driver, mockedEndpoint: mockedEndpoints }) => {
+        let events = [];
+        await completeCreateNewWalletOnboardingFlow({
+          driver,
+          participateInMetaMetrics: false,
+        });
+        events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 0);
+        await new HeaderNavbar(driver).openSettingsPage();
+        const settingsPage = new SettingsPage(driver);
+        await settingsPage.check_pageIsLoaded();
+        await settingsPage.goToPrivacySettings();
+
+        const privacySettings = new PrivacySettings(driver);
+        await privacySettings.check_pageIsLoaded();
+        await privacySettings.toggleParticipateInMetaMetrics();
+        await privacySettings.toggleDataCollectionForMarketing();
+        events = await getEventPayloads(driver, mockedEndpoints);
+        assert.equal(events.length, 1);
+        assert.deepStrictEqual(events[0].traits.is_metrics_opted_in, true);
+        assert.deepStrictEqual(events[0].traits.has_marketing_consent, true);
+      },
+    );
+  });
+});
diff --git a/ui/pages/onboarding-flow/metametrics/__snapshots__/metametrics.test.js.snap b/ui/pages/onboarding-flow/metametrics/__snapshots__/metametrics.test.js.snap
@@ -101,6 +101,7 @@ exports[`Onboarding Metametrics Component should match snapshot 1`] = `
     </ul>
     <label
       class="mm-box mm-text mm-checkbox mm-text--body-md mm-box--padding-bottom-3 mm-box--display-inline-flex mm-box--align-items-center mm-box--color-text-default"
+      data-testid="metametrics-data-collection-checkbox"
       for="metametrics-opt-in"
     >
       <span
@@ -256,6 +257,7 @@ exports[`Onboarding Metametrics Component should match snapshot after new policy
     </ul>
     <label
       class="mm-box mm-text mm-checkbox mm-text--body-md mm-box--padding-bottom-3 mm-box--display-inline-flex mm-box--align-items-center mm-box--color-text-default"
+      data-testid="metametrics-data-collection-checkbox"
       for="metametrics-opt-in"
     >
       <span

diff --git a/ui/pages/onboarding-flow/metametrics/metametrics.js b/ui/pages/onboarding-flow/metametrics/metametrics.js
@@ -196,6 +196,7 @@ export default function OnboardingMetametrics() {
       </ul>
       <Checkbox
         id="metametrics-opt-in"
+        data-testid="metametrics-data-collection-checkbox"
         isChecked={dataCollectionForMarketing}
         onClick={() =>
           dispatch(setDataCollectionForMarketing(!dataCollectionForMarketing))

diff --git a/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap b/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap
@@ -1442,7 +1442,7 @@ exports[`Security Tab should match snapshot 1`] = `
           </div>
           <div
             class="settings-page__content-item-col"
-            data-testid="participateInMetaMetrics"
+            data-testid="participate-in-meta-metrics-toggle"
           >
             <label
               class="toggle-button toggle-button--off"
@@ -1469,7 +1469,7 @@ exports[`Security Tab should match snapshot 1`] = `
                   />
                 </div>
                 <input
-                  data-testid="toggleButton"
+                  data-testid="participate-in-meta-metrics-toggle-button"
                   style="border: 0px; height: 1px; margin: -1px; overflow: hidden; padding: 0px; position: absolute; width: 1px;"
                   type="checkbox"
                   value="false"
@@ -1512,7 +1512,7 @@ exports[`Security Tab should match snapshot 1`] = `
         </div>
         <div
           class="settings-page__content-item-col"
-          data-testid="dataCollectionForMarketing"
+          data-testid="data-collection-for-marketing-toggle"
         >
           <label
             class="toggle-button toggle-button--off"