Skip to content

Commit

Permalink
refactor: ♻️ change the way we are handling some metamask noti… (#24269)
Browse files Browse the repository at this point in the history
## **Description**

This PR changes the management of three states handled by the controller
to make their definition assertive.

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask 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**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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
matteoscurati committed May 1, 2024
1 parent 9c83db2 commit 8700083
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ describe('metamask-notifications - checkAccountsPresence()', () => {
});
});

describe('metamask-notifications - toggleMetamaskNotificationsEnabled()', () => {
describe('metamask-notifications - setMetamaskNotificationsEnabled()', () => {
test('flips enabled state', async () => {
const { messenger, mockIsSignedIn } = mockNotificationMessenger();
mockIsSignedIn.mockReturnValue(true);
Expand All @@ -189,7 +189,7 @@ describe('metamask-notifications - toggleMetamaskNotificationsEnabled()', () =>
state: { ...defaultState, isMetamaskNotificationsEnabled: false },
});

await controller.toggleMetamaskNotificationsEnabled();
await controller.setMetamaskNotificationsEnabled(true);

expect(controller.state.isMetamaskNotificationsEnabled).toBe(true);
});
Expand Down Expand Up @@ -227,7 +227,7 @@ describe('metamask-notifications - setMetamaskNotificationsFeatureSeen()', () =>
});
});

describe('metamask-notifications - toggleFeatureAnnouncementsEnabled()', () => {
describe('metamask-notifications - setFeatureAnnouncementsEnabled()', () => {
test('flips state when the method is called', async () => {
const { messenger, mockIsSignedIn } = mockNotificationMessenger();
mockIsSignedIn.mockReturnValue(true);
Expand All @@ -237,7 +237,7 @@ describe('metamask-notifications - toggleFeatureAnnouncementsEnabled()', () => {
state: { ...defaultState, isFeatureAnnouncementsEnabled: false },
});

await controller.toggleFeatureAnnouncementsEnabled();
await controller.setFeatureAnnouncementsEnabled(true);

expect(controller.state.isFeatureAnnouncementsEnabled).toBe(true);
});
Expand All @@ -252,13 +252,13 @@ describe('metamask-notifications - toggleFeatureAnnouncementsEnabled()', () => {
});

await expect(() =>
controller.toggleFeatureAnnouncementsEnabled(),
controller.setFeatureAnnouncementsEnabled(false),
).rejects.toThrow();
expect(controller.state.isFeatureAnnouncementsEnabled).toBe(false); // this flag was never flipped
});
});

describe('metamask-notifications - toggleSnapNotificationsEnabled()', () => {
describe('metamask-notifications - setSnapNotificationsEnabled()', () => {
test('flips state when the method is called', async () => {
const { messenger, mockIsSignedIn } = mockNotificationMessenger();
mockIsSignedIn.mockReturnValue(true);
Expand All @@ -268,7 +268,7 @@ describe('metamask-notifications - toggleSnapNotificationsEnabled()', () => {
state: { ...defaultState, isSnapNotificationsEnabled: false },
});

await controller.toggleSnapNotificationsEnabled();
await controller.setSnapNotificationsEnabled(true);

expect(controller.state.isSnapNotificationsEnabled).toBe(true);
});
Expand All @@ -282,7 +282,7 @@ describe('metamask-notifications - toggleSnapNotificationsEnabled()', () => {
state: { ...defaultState, isSnapNotificationsEnabled: false },
});

await controller.toggleSnapNotificationsEnabled();
await controller.setSnapNotificationsEnabled(false);

expect(controller.state.isSnapNotificationsEnabled).toBe(false); // this flag was never flipped
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -420,20 +420,21 @@ export class MetamaskNotificationsController extends BaseController<
}

/**
* Toggles the enabled state of metamask notifications.
* Ensures that auth is enabled before toggling notifications.
* Sets the enabled state of MetaMask notifications.
* This method first checks if the user is authenticated before attempting to toggle the notification settings.
*
* **Action** - Used to Enable/Disable notifications
* **Action** - This method is used to enable or disable MetaMask notifications based on the provided state.
*
* @param state - A boolean value indicating the desired enabled state of the notifications.
* @async
* @throws {Error} If updating the state fails.
* @throws {Error} If the user is not authenticated or if there is an error updating the state.
*/
public async toggleMetamaskNotificationsEnabled() {
public async setMetamaskNotificationsEnabled(state: boolean) {
try {
this.#assertAuthEnabled();

this.update((s) => {
s.isMetamaskNotificationsEnabled = !s.isMetamaskNotificationsEnabled;
s.isMetamaskNotificationsEnabled = state;
});
} catch (e) {
log.error('Unable to toggle notifications', e);
Expand Down Expand Up @@ -463,19 +464,20 @@ export class MetamaskNotificationsController extends BaseController<
}

/**
* Toggles the enabled state of feature announcements.
* Sets the enabled state of feature announcements.
*
* **Action** - used in the notification settings to enable/disable feature announcements.
*
* @param state - A boolean value indicating the desired enabled state of the feature announcements.
* @async
* @throws {Error} If the BearerToken token or storage key is missing.
*/
public async toggleFeatureAnnouncementsEnabled() {
public async setFeatureAnnouncementsEnabled(state: boolean) {
try {
this.#assertAuthEnabled();

this.update((s) => {
s.isFeatureAnnouncementsEnabled = !s.isFeatureAnnouncementsEnabled;
s.isFeatureAnnouncementsEnabled = state;
});
} catch (e) {
log.error('Unable to toggle feature announcements', e);
Expand All @@ -484,19 +486,20 @@ export class MetamaskNotificationsController extends BaseController<
}

/**
* Toggles the enabled state of Snap notifications.
* Sets the enabled state of Snap notifications.
*
* **Action** - used in the notifications settings page to enable/disable snap notifications.
*
* @param state - A boolean value indicating the desired enabled state of the snap notifications.
* @async
* @throws {Error} If the BearerToken token or storage key is missing.
*/
public async toggleSnapNotificationsEnabled() {
public async setSnapNotificationsEnabled(state: boolean) {
try {
this.#assertAuthEnabled();

this.update((s) => {
s.isSnapNotificationsEnabled = !s.isSnapNotificationsEnabled;
s.isSnapNotificationsEnabled = state;
});
} catch (e) {
log.error('Unable to toggle snap notifications', e);
Expand Down Expand Up @@ -548,6 +551,11 @@ export class MetamaskNotificationsController extends BaseController<
// Write the new userStorage (triggers are now "enabled")
await this.#storage.setNotificationStorage(JSON.stringify(userStorage));

// Update the state of the controller
this.setFeatureAnnouncementsEnabled(true);
this.setMetamaskNotificationsEnabled(true);
this.setSnapNotificationsEnabled(true);

return userStorage;
} catch (err) {
log.error('Failed to create On Chain triggers', err);
Expand Down

0 comments on commit 8700083

Please sign in to comment.