Skip to content

Commit

Permalink
fix: add migration for profile syncing controller (#26004)
Browse files Browse the repository at this point in the history
<!--
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**

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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26004?quickstart=1)

## **Related issues**

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

## **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**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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.
  • Loading branch information
Prithpal-Sooriya authored Jul 22, 2024
1 parent edb401a commit afe1120
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ 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;
/**
* Loading state for the profile syncing update
*/
Expand Down
75 changes: 75 additions & 0 deletions app/scripts/migrations/120.1.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { migrate, version } from './120.1';

const oldVersion = 120;

describe('migration #120.1', () => {
afterEach(() => jest.resetAllMocks());

it('updates the version metadata', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.meta).toStrictEqual({ version });
});

it('sets isProfileSyncingEnabled to null', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
UserStorageController: {
isProfileSyncingEnabled: true,
isProfileSyncingUpdateLoading: false,
},
},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.data.UserStorageController).toStrictEqual({
isProfileSyncingEnabled: null,
isProfileSyncingUpdateLoading: false,
});
});

it('initializes a default user storage state if it did not exist before', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
OtherController: {},
},
};

const expectedStorageData = {
OtherController: {},
UserStorageController: {
isProfileSyncingEnabled: null,
},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.data).toStrictEqual(expectedStorageData);
});

it('should do nothing if existing UserStorageController state is malformed', async () => {
const actAssertInvalidUserStorageState = async (state: unknown) => {
const oldStorage = {
meta: { version: oldVersion },
data: {
OtherController: {},
UserStorageController: state,
},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.data).toStrictEqual(oldStorage.data);
};

actAssertInvalidUserStorageState('user storage state is not an object');
actAssertInvalidUserStorageState({
// missing the isProfileSyncingEnabled field
isProfileSyncingUpdateLoading: false,
});
});
});
52 changes: 52 additions & 0 deletions app/scripts/migrations/120.1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { cloneDeep, isObject } from 'lodash';
import { hasProperty } from '@metamask/utils';
import { captureException } from '@sentry/browser';

type VersionedData = {
meta: { version: number };
data: Record<string, unknown>;
};

export const version = 120.1;

/**
* Add a default value for importTime in the InternalAccount
*
* @param originalVersionedData
*/
export async function migrate(
originalVersionedData: VersionedData,
): Promise<VersionedData> {
const versionedData = cloneDeep(originalVersionedData);
versionedData.meta.version = version;
transformState(versionedData.data);
return versionedData;
}

function transformState(
state: Record<string, unknown>,
): Record<string, unknown> {
// Existing users who do not have UserStorageController state.
// Provide some initial state & nullify `isProfileSyncingEnabled`
if (!hasProperty(state, 'UserStorageController')) {
state.UserStorageController = {
isProfileSyncingEnabled: null,
};
return state;
}

if (
!isObject(state.UserStorageController) ||
!hasProperty(state.UserStorageController, 'isProfileSyncingEnabled')
) {
captureException(
`Migration ${version}: Invalid UserStorageController state: ${typeof state.UserStorageController}`,
);
return state;
}

// Existing users who do have UserStorageController state.
// nullify `isProfileSyncingEnabled`
state.UserStorageController.isProfileSyncingEnabled = null;
return state;
}
1 change: 1 addition & 0 deletions app/scripts/migrations/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ const migrations = [
require('./118'),
require('./119'),
require('./120'),
require('./120.1'),
require('./121'),

require('./123'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@
},
"UserOperationController": { "userOperations": "object" },
"UserStorageController": {
"isProfileSyncingEnabled": true,
"isProfileSyncingEnabled": null,
"isProfileSyncingUpdateLoading": "boolean"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@
"nameSources": "object",
"userOperations": "object",
"isSignedIn": "boolean",
"isProfileSyncingEnabled": true,
"isProfileSyncingEnabled": null,
"isProfileSyncingUpdateLoading": "boolean",
"subscriptionAccountsSeen": "object",
"isMetamaskNotificationsFeatureSeen": "boolean",
Expand Down

0 comments on commit afe1120

Please sign in to comment.