Skip to content

Commit b7afe24

Browse files
refactor: cp-13.13.0 gate ProfileMetricsController with pna25Acknowledged (#38562)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> The PR adds an additional check to the `assertUserOptedIn` function passed to the `ProfileMetricsController` to ensure that profile metrics are only collected after the user has acknowledged the PNA25 notice. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/38562?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Gates profile metrics collection behind `pna25Acknowledged` and updates E2E tests to cover acknowledgment and other gating conditions. > > - **Controller init**: > - Update `assertUserOptedIn` in `app/scripts/controller-init/profile-metrics-controller-init.ts` to also require `AppStateController.state.pna25Acknowledged === true` alongside the feature flag and MetaMetrics opt-in. > - **E2E tests** (`test/e2e/tests/profile-metrics/profile-metrics.spec.ts`): > - Positive path: include `.withAppStateController({ pna25Acknowledged: true })` when MetaMetrics is enabled and the feature flag is on; verify existing and new accounts trigger API calls. > - Negative paths: parameterize cases to assert no API calls when MetaMetrics is disabled, the feature flag is off, or the user has not acknowledged the privacy change. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1424dce. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Jongsun Suh <[email protected]>
1 parent 84ce04b commit b7afe24

File tree

2 files changed

+73
-74
lines changed

2 files changed

+73
-74
lines changed

app/scripts/controller-init/profile-metrics-controller-init.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,12 @@ export const ProfileMetricsControllerInit: ControllerInitFunction<
2424
'RemoteFeatureFlagController',
2525
);
2626
const metaMetricsController = getController('MetaMetricsController');
27+
const appStateController = getController('AppStateController');
2728
const assertUserOptedIn = () =>
2829
remoteFeatureFlagController.state.remoteFeatureFlags.extensionUxPna25 ===
29-
true && metaMetricsController.state.participateInMetaMetrics === true;
30+
true &&
31+
appStateController.state.pna25Acknowledged === true &&
32+
metaMetricsController.state.participateInMetaMetrics === true;
3033

3134
const controller = new ProfileMetricsController({
3235
messenger: controllerMessenger,

test/e2e/tests/profile-metrics/profile-metrics.spec.ts

Lines changed: 69 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,17 @@ async function waitForEndpointToBeCalled(
7777
}
7878

7979
describe('Profile Metrics', function () {
80-
describe('when MetaMetrics is enabled and the feature flag is on', function () {
80+
describe('when MetaMetrics is enabled, the feature flag is on, and the user acknowledged the privacy change', function () {
8181
it('sends exising accounts to the API on wallet unlock after activating MetaMetrics', async function () {
8282
await withFixtures(
8383
{
8484
fixtures: new FixtureBuilder()
8585
.withMetaMetricsController({
8686
participateInMetaMetrics: true,
8787
})
88+
.withAppStateController({
89+
pna25Acknowledged: true,
90+
})
8891
.build(),
8992
testSpecificMock: async (server: Mockttp) => [
9093
await mockAuthService(server),
@@ -121,6 +124,9 @@ describe('Profile Metrics', function () {
121124
.withMetaMetricsController({
122125
participateInMetaMetrics: true,
123126
})
127+
.withAppStateController({
128+
pna25Acknowledged: true,
129+
})
124130
.build(),
125131
testSpecificMock: async (server: Mockttp) => [
126132
await mockAuthService(server),
@@ -159,77 +165,67 @@ describe('Profile Metrics', function () {
159165
});
160166
});
161167

162-
describe('when MetaMetrics is disabled or the feature flag is off', function () {
163-
it('does not send existing accounts to the API on wallet unlock if MetaMetrics is disabled', async function () {
164-
await withFixtures(
165-
{
166-
fixtures: new FixtureBuilder()
167-
.withMetaMetricsController({
168-
participateInMetaMetrics: false,
169-
})
170-
.build(),
171-
testSpecificMock: async (server: Mockttp) => [
172-
await mockAuthService(server),
173-
await mockSendFeatureFlag(true)(server),
174-
],
175-
title: this.test?.fullTitle(),
176-
},
177-
async ({
178-
driver,
179-
mockedEndpoint,
180-
}: {
181-
driver: Driver;
182-
mockedEndpoint: MockedEndpoint[];
183-
}) => {
184-
await loginWithBalanceValidation(driver);
185-
186-
await driver.delay(5000);
187-
188-
const [authCall] = mockedEndpoint;
189-
const requests = await authCall.getSeenRequests();
190-
assert.equal(
191-
requests.length,
192-
0,
193-
'Expected no requests to the auth API.',
168+
[
169+
{
170+
title: 'when MetaMetrics is disabled',
171+
participateInMetaMetrics: false,
172+
featureFlag: true,
173+
pna25Acknowledged: true,
174+
},
175+
{
176+
title: 'when the relevant feature flag is off',
177+
participateInMetaMetrics: true,
178+
featureFlag: false,
179+
pna25Acknowledged: true,
180+
},
181+
{
182+
title: 'when the user has not acknowledged the privacy change',
183+
participateInMetaMetrics: true,
184+
featureFlag: true,
185+
pna25Acknowledged: false,
186+
},
187+
].forEach(
188+
({ title, participateInMetaMetrics, featureFlag, pna25Acknowledged }) => {
189+
describe(title, function () {
190+
it('does not send existing accounts to the API on wallet unlock', async function () {
191+
await withFixtures(
192+
{
193+
fixtures: new FixtureBuilder()
194+
.withMetaMetricsController({
195+
participateInMetaMetrics,
196+
})
197+
.withAppStateController({
198+
pna25Acknowledged,
199+
})
200+
.build(),
201+
testSpecificMock: async (server: Mockttp) => [
202+
await mockAuthService(server),
203+
await mockSendFeatureFlag(featureFlag)(server),
204+
],
205+
title: this.test?.fullTitle(),
206+
},
207+
async ({
208+
driver,
209+
mockedEndpoint,
210+
}: {
211+
driver: Driver;
212+
mockedEndpoint: MockedEndpoint[];
213+
}) => {
214+
await loginWithBalanceValidation(driver);
215+
216+
await driver.delay(5000);
217+
218+
const [authCall] = mockedEndpoint;
219+
const requests = await authCall.getSeenRequests();
220+
assert.equal(
221+
requests.length,
222+
0,
223+
'Expected no requests to the auth API.',
224+
);
225+
},
194226
);
195-
},
196-
);
197-
});
198-
199-
it('does not send existing accounts to the API on wallet unlock if feature flag is disabled', async function () {
200-
await withFixtures(
201-
{
202-
fixtures: new FixtureBuilder()
203-
.withMetaMetricsController({
204-
participateInMetaMetrics: true,
205-
})
206-
.build(),
207-
testSpecificMock: async (server: Mockttp) => [
208-
await mockAuthService(server),
209-
await mockSendFeatureFlag(false)(server),
210-
],
211-
title: this.test?.fullTitle(),
212-
},
213-
async ({
214-
driver,
215-
mockedEndpoint,
216-
}: {
217-
driver: Driver;
218-
mockedEndpoint: MockedEndpoint[];
219-
}) => {
220-
await loginWithBalanceValidation(driver);
221-
222-
await driver.delay(5000);
223-
224-
const [authCall] = mockedEndpoint;
225-
const requests = await authCall.getSeenRequests();
226-
assert.equal(
227-
requests.length,
228-
0,
229-
'Expected no requests to the auth API.',
230-
);
231-
},
232-
);
233-
});
234-
});
227+
});
228+
});
229+
},
230+
);
235231
});

0 commit comments

Comments
 (0)