fix: add notification call prevention logic #5081
Conversation
…the global notifications toggle is enabled
| // Testing util to clean up verbose logs when testing errors | ||
| const mockErrorLog = () => | ||
| jest.spyOn(log, 'error').mockImplementation(jest.fn()); | ||
| const mockWarnLog = () => jest.spyOn(log, 'warn').mockImplementation(jest.fn()); |
There was a problem hiding this comment.
test:verbose is a bit too messy since we are testing a lot of failure cases.
This cleans up the logs only for cases we know we are failing on.
| return controller; | ||
| }; | ||
|
|
||
| it('processes and shows all notifications (announcements, wallet, and snap notifications)', async () => { |
There was a problem hiding this comment.
The tests for the fetchAndUpdateMetamaskNotifications method were messy, cleaned them up.
| @@ -577,7 +577,7 @@ export default class NotificationServicesController extends BaseController< | |||
| #registerMessageHandlers(): void { | |||
| this.messagingSystem.registerActionHandler( | |||
| `${controllerName}:updateMetamaskNotificationsList`, | |||
| async (...args) => this.updateMetamaskNotificationsList(...args), | |||
| this.updateMetamaskNotificationsList.bind(this), | |||
There was a problem hiding this comment.
This is equivalent to the previous version. It aligns with the other action handlers.
| ).catch(() => []) | ||
| : []; | ||
| const rawFeatureAnnouncementNotifications = | ||
| isGlobalNotifsEnabled && this.state.isFeatureAnnouncementsEnabled |
There was a problem hiding this comment.
Added isGlobalNotifsEnabled check to make sure we fetch announcements if the global flag is enabled.
| ).catch(() => []); | ||
|
|
||
| rawOnChainNotifications.push(...notifications); | ||
| if (isGlobalNotifsEnabled) { |
There was a problem hiding this comment.
Surrounded code with this isGlobalNotifsEnabled check to ensure we fetch wallet notifications if the global flag is enabled.
| // Snap Notifications (original) | ||
| // We do not want to remove them | ||
| const snapNotifications = this.state.metamaskNotificationsList.filter( | ||
| (notification) => notification.type === TRIGGER_TYPES.SNAP, | ||
| ); |
There was a problem hiding this comment.
Reordering snap notifications, so the code is ordered in steps.
| (notification) => notification.type === TRIGGER_TYPES.SNAP, | ||
| ); | ||
|
|
||
| // Combine Notifications |
There was a problem hiding this comment.
Added better comments to explain each step.
NOTE - this function does to a lot of things.
I think when we do the larger refactor we can think on how to isolate each notifications responsibilities.
Prithpal-Sooriya
changed the title
Prithpal-Sooriya
deleted the
fix-add-notification-call-prevention-logic
branch
Explanation
We've added logic on platforms to make sure that the global notifications switch is enabled before we fetch. But ideally this should be internal logic.
So we've updated the logic to ensure that Feature Announcements and Wallet Notifications will bail early if the toggle is off. Snap notifications however will not obey this flag (to keep compatibility with the original snap notifications feature).
References
MetaMask/metamask-extension#28173
Changelog
@metamask/notification-services-controllerisNotificationServicesEnabledcheck insidefetchAndUpdateMetamaskNotificationsmethod.false.Checklist