fix: add migration for profile syncing controller #26004

Prithpal-Sooriya · 2024-07-22T13:36:06Z

Description

For existing users, we want to ensure that profile syncing is disabled (opt-in). This migration ensures that the isProfileSyncingEnabled controller state is changed to false for existing users.

This will need to be cherry-picked into v12.0.0

Related issues

Fixes: V12 testing issue of the state for profile syncing.

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

we want existing users to have profile syncing disabled, this will remove/nullify the isEnableProfileSyncing state field.

github-actions · 2024-07-22T13:36:19Z

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.

Prithpal-Sooriya · 2024-07-22T13:36:33Z

app/scripts/controllers/user-storage/user-storage-controller.ts

@@ -27,15 +27,15 @@ export type UserStorageControllerState = {
  /**
   * Condition used by UI and to determine if we can use some of the User Storage methods.
   */
-  isProfileSyncingEnabled: boolean;
+  isProfileSyncingEnabled: boolean | null;


Checked all references (JS and TS files) it seems okay to change this field to be nullable.

We do not perform strict boolean checks in references, it is truthy checks (isProfileSyncingEnabled && ... or isProfileSyncingEnabled || ...)

… fix/add-profile-syncing-enabled-migration

app/scripts/controllers/user-storage/user-storage-controller.ts

app/scripts/migrations/120.1.ts

…ration we need to also handle migration of new users who migrated from 11.16 that might have UserStorage in their state

codecov · 2024-07-22T14:18:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.67%. Comparing base (721a38b) to head (5d46b4d).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #26004   +/-   ##
========================================
  Coverage    69.67%   69.67%           
========================================
  Files         1402     1403    +1     
  Lines        49652    49676   +24     
  Branches     13720    13726    +6     
========================================
+ Hits         34594    34611   +17     
- Misses       15058    15065    +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

app/scripts/migrations/120.1.test.ts

Prithpal-Sooriya · 2024-07-22T15:12:22Z

Tested the migration on v12 and v11. Seems to work as intended

app/scripts/migrations/120.1.ts

Gudahtt

LGTM! One pending suggestion about how to handle corrupted state, which would be ideal to fix but doesn't seem critical enough to block.

greptile-apps

PR Summary

This pull request introduces a migration to ensure that profile syncing is disabled by default for existing users, aligning with the opt-in policy.

Migration Script: Added app/scripts/migrations/120.1.ts to set isProfileSyncingEnabled to null for existing users.
Controller Update: Modified app/scripts/controllers/user-storage/user-storage-controller.ts to handle null state for isProfileSyncingEnabled.
Unit Tests: Added app/scripts/migrations/120.1.test.ts to validate the migration logic.
Migration Index: Updated app/scripts/migrations/index.js to include the new migration script.
E2E Test States: Modified multiple E2E test state files to reflect the new default null value for isProfileSyncingEnabled.

_{8 file(s) reviewed, 10 comment(s)}
_{Edit PR Review Bot Settings}

app/scripts/controllers/user-storage/user-storage-controller.ts

app/scripts/migrations/120.1.ts

app/scripts/migrations/index.js

test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json

test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json

test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json

test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a migration to ensure that profile syncing is disabled by default for existing users, aligning with the opt-in policy.

Migration Script Update: Enhanced app/scripts/migrations/120.1.ts to handle malformed UserStorageController states and added Sentry error handling.
Test Case Addition: Updated app/scripts/migrations/120.1.test.ts to include scenarios where the UserStorageController state is invalid.
E2E Test State Updates: Modified multiple E2E test state files (errors-after-init-opt-in-background-state.json, errors-after-init-opt-in-ui-state.json, errors-before-init-opt-in-background-state.json, errors-before-init-opt-in-ui-state.json) to set isProfileSyncingEnabled to true by default.

_{6 file(s) reviewed, 3 comment(s)}
_{Edit PR Review Bot Settings}

app/scripts/migrations/120.1.test.ts

app/scripts/migrations/120.1.ts

Gudahtt · 2024-07-22T17:02:37Z

Hmm, still some e2e test failure in the error capture tests

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a migration to ensure that profile syncing is disabled by default for existing users, aligning with the opt-in policy.

Migration Script Update: Enhanced app/scripts/migrations/120.1.ts to handle malformed UserStorageController states and added Sentry error handling.
Test Case Addition: Updated app/scripts/migrations/120.1.test.ts to include scenarios where the UserStorageController state is invalid.
E2E Test State Updates: Modified multiple E2E test state files (errors-before-init-opt-in-background-state.json, errors-before-init-opt-in-ui-state.json) to set isProfileSyncingEnabled to true by default.

_{4 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

app/scripts/migrations/120.1.test.ts

app/scripts/migrations/120.1.ts

metamaskbot · 2024-07-22T18:28:09Z

Builds ready [5d46b4d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (254 ± 242 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	309	127	53	26
		domContentLoaded	11	98	32	20	9
		load	49	1835	254	503	242
		domInteractive	11	98	32	20	9

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.18 KiB (0.04%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarqubecloud · 2024-07-22T18:37:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

greptile-apps

PR Summary

(updates since last review)

The pull request introduces a migration to ensure that profile syncing is disabled by default for existing users, aligning with the opt-in policy.

Mock Active Tab for E2E Tests: Updated app/scripts/ui.js to mock the active tab during E2E tests using the activeTabOrigin query string parameter.
New SIWE E2E Tests: Added test/e2e/tests/confirmations/signatures/siwe.spec.ts to cover SIWE functionality.
Request Queuing E2E Tests: Enhanced test/e2e/tests/request-queuing/ui.spec.js with new scenarios and helper functions.
Constrain Window Size in WebDriver: Modified test/e2e/webdriver/chrome.js, test/e2e/webdriver/firefox.js, and test/e2e/webdriver/index.js to add constrainWindowSize option.
SIWE Message Handling: Updated ui/pages/confirmations/hooks/useCurrentConfirmation.ts and its test to handle SIWE messages correctly.

_{8 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

matteoscurati

Go!

metamaskbot · 2024-07-22T19:29:07Z

Builds ready [8c483f9]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (151 ± 168 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	72	139	99	17	8
		domContentLoaded	11	52	23	10	5
		load	48	1674	151	350	168
		domInteractive	11	52	22	10	5

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 1.18 KiB (0.04%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

fix: add migration for profile syncing controller

f95bf2d

we want existing users to have profile syncing disabled, this will remove/nullify the isEnableProfileSyncing state field.

Prithpal-Sooriya commented Jul 22, 2024

View reviewed changes

metamaskbot added the team-notifications Notifications team label Jul 22, 2024

Prithpal-Sooriya marked this pull request as ready for review July 22, 2024 13:40

Prithpal-Sooriya requested review from a team as code owners July 22, 2024 13:40

Prithpal-Sooriya added 2 commits July 22, 2024 14:40

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

724cee1

… fix/add-profile-syncing-enabled-migration

test: add some additional tests for this migration

e543fe5

Gudahtt reviewed Jul 22, 2024

View reviewed changes

app/scripts/controllers/user-storage/user-storage-controller.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.ts Show resolved Hide resolved

fix: fix small mistake and also cover other edge cases during the mig…

4c332fd

…ration we need to also handle migration of new users who migrated from 11.16 that might have UserStorage in their state

Gudahtt reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.test.ts Outdated Show resolved Hide resolved

test: update test names

c3d453f

Prithpal-Sooriya requested a review from Gudahtt July 22, 2024 14:46

Gudahtt reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.ts Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Jul 22, 2024

View reviewed changes

Prithpal-Sooriya dismissed Gudahtt’s stale review via 5c08942 July 22, 2024 16:12

Prithpal-Sooriya force-pushed the fix/add-profile-syncing-enabled-migration branch from 2ed2f4c to 5c08942 Compare July 22, 2024 16:12

chore: update e2e test snapshots

5c08942

greptile-apps bot reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.test.ts Show resolved Hide resolved

app/scripts/migrations/120.1.ts Show resolved Hide resolved

app/scripts/migrations/120.1.ts Show resolved Hide resolved

Gudahtt previously approved these changes Jul 22, 2024

View reviewed changes

Prithpal-Sooriya dismissed Gudahtt’s stale review via 5d46b4d July 22, 2024 17:08

Prithpal-Sooriya force-pushed the fix/add-profile-syncing-enabled-migration branch from a3b8a3a to 5d46b4d Compare July 22, 2024 17:08

refactor: handle and capture migration failures due to malformed state

5d46b4d

greptile-apps bot reviewed Jul 22, 2024

View reviewed changes

app/scripts/migrations/120.1.test.ts Show resolved Hide resolved

app/scripts/migrations/120.1.ts Show resolved Hide resolved

Gudahtt approved these changes Jul 22, 2024

View reviewed changes

Merge branch 'develop' into fix/add-profile-syncing-enabled-migration

8c483f9

greptile-apps bot reviewed Jul 22, 2024

View reviewed changes

matteoscurati approved these changes Jul 22, 2024

View reviewed changes

Prithpal-Sooriya merged commit afe1120 into develop Jul 22, 2024
76 of 77 checks passed

Prithpal-Sooriya deleted the fix/add-profile-syncing-enabled-migration branch July 22, 2024 19:26

github-actions bot locked and limited conversation to collaborators Jul 22, 2024

metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add migration for profile syncing controller #26004

fix: add migration for profile syncing controller #26004

Prithpal-Sooriya commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024

Prithpal-Sooriya Jul 22, 2024

Prithpal-Sooriya Jul 22, 2024

codecov bot commented Jul 22, 2024 •

edited

Loading

Prithpal-Sooriya commented Jul 22, 2024

Gudahtt left a comment •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot left a comment

Gudahtt commented Jul 22, 2024

greptile-apps bot left a comment

metamaskbot commented Jul 22, 2024

sonarqubecloud bot commented Jul 22, 2024

greptile-apps bot left a comment

matteoscurati left a comment

metamaskbot commented Jul 22, 2024

fix: add migration for profile syncing controller #26004

fix: add migration for profile syncing controller #26004

Conversation

Prithpal-Sooriya commented Jul 22, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jul 22, 2024

Prithpal-Sooriya Jul 22, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Jul 22, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

Prithpal-Sooriya commented Jul 22, 2024

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Gudahtt commented Jul 22, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

metamaskbot commented Jul 22, 2024

sonarqubecloud bot commented Jul 22, 2024

Quality Gate passed

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

matteoscurati left a comment

Choose a reason for hiding this comment

metamaskbot commented Jul 22, 2024

Prithpal-Sooriya commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

Gudahtt left a comment •

edited

Loading