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: add migration for profile syncing controller #26004

Merged
merged 8 commits into from
Jul 22, 2024
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
/**
* 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 () => {
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
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';
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
import { captureException } from '@sentry/browser';
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved

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

export const version = 120.1;
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved

/**
* 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;
}
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved

function transformState(
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
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')) {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
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;
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
}

// 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'),
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
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,
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
"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,
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
"isProfileSyncingUpdateLoading": "boolean",
"subscriptionAccountsSeen": "object",
"isMetamaskNotificationsFeatureSeen": "boolean",
Expand Down