fix: disable notifications when basic functionality off (#28045)

## **Description** This PR ensures that notifications are correctly disabled when the user deactivates Basic functionality. The current implementation only disables profile sync when Basic functionality is turned off, leaving notifications in an incorrect state—they can’t be fetched but remain active. This causes a series of UI errors, the most obvious being that the counters stay visible. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28045?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. With notifications active 2. Deactivate Basic functionality 3. Verify that notifications cannot be enabled unless Basic functionality is reactivated 4. Verify that the counter no longer shows any unread notifications ## **Screenshots/Recordings**  ### **Before** <img width="1624" alt="Screenshot 2024-10-24 at 10 08 32" src="https://github.com/user-attachments/assets/077fefd4-1cac-45b6-8a40-0a0787abdb94"> ### **After** <img width="1624" alt="Screenshot 2024-10-24 at 10 19 48" src="https://github.com/user-attachments/assets/f9e14f65-4e64-4d7d-9633-8a5e685df4fd"> ## **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** - [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.
MetaMask · Oct 24, 2024 · f0aad1c · f0aad1c
1 parent f1db383
commit f0aad1c
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 26 deletions.
diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx b/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx
@@ -1,9 +1,11 @@
-import React from 'react';
+import React, { ReactNode } from 'react';
 import { Provider } from 'react-redux';
+import { Store } from 'redux';
 import { renderHook, act } from '@testing-library/react-hooks';
 import configureStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
 import { waitFor } from '@testing-library/react';
+import { MetamaskNotificationsProvider } from '../../contexts/metamask-notifications';
 import * as actions from '../../store/actions';
 import {
   useEnableProfileSyncing,
@@ -77,12 +79,26 @@ const arrangeMocks = (
   return { store };
 };
 
+const RenderWithProviders = ({
+  store,
+  children,
+}: {
+  store: Store;
+  children: ReactNode;
+}) => (
+  <Provider store={store}>
+    <MetamaskNotificationsProvider>{children}</MetamaskNotificationsProvider>
+  </Provider>
+);
+
 describe('useProfileSyncing', () => {
   it('should enable profile syncing', async () => {
     const { store } = arrangeMocks();
 
     const { result } = renderHook(() => useEnableProfileSyncing(), {
-      wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
+      wrapper: ({ children }) => (
+        <RenderWithProviders store={store}>{children}</RenderWithProviders>
+      ),
     });
 
     act(() => {
@@ -96,7 +112,9 @@ describe('useProfileSyncing', () => {
     const { store } = arrangeMocks();
 
     const { result } = renderHook(() => useDisableProfileSyncing(), {
-      wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
+      wrapper: ({ children }) => (
+        <RenderWithProviders store={store}>{children}</RenderWithProviders>
+      ),
     });
 
     act(() => {
@@ -113,7 +131,9 @@ describe('useProfileSyncing', () => {
     });
 
     renderHook(() => useAccountSyncingEffect(), {
-      wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
+      wrapper: ({ children }) => (
+        <RenderWithProviders store={store}>{children}</RenderWithProviders>
+      ),
     });
 
     await waitFor(() => {
@@ -125,7 +145,9 @@ describe('useProfileSyncing', () => {
     const { store } = arrangeMocks();
 
     renderHook(() => useAccountSyncingEffect(), {
-      wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
+      wrapper: ({ children }) => (
+        <RenderWithProviders store={store}>{children}</RenderWithProviders>
+      ),
     });
 
     await waitFor(() => {
@@ -142,7 +164,7 @@ describe('useProfileSyncing', () => {
       () => useDeleteAccountSyncingDataFromUserStorage(),
       {
         wrapper: ({ children }) => (
-          <Provider store={store}>{children}</Provider>
+          <RenderWithProviders store={store}>{children}</RenderWithProviders>
         ),
       },
     );

diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.ts b/ui/hooks/metamask-notifications/useProfileSyncing.ts
@@ -2,6 +2,7 @@ import { useState, useCallback, useEffect, useMemo } from 'react';
 import { useDispatch, useSelector } from 'react-redux';
 import type { InternalAccount } from '@metamask/keyring-api';
 import log from 'loglevel';
+import { useMetamaskNotificationsContext } from '../../contexts/metamask-notifications/metamask-notifications';
 import {
   disableProfileSyncing as disableProfileSyncingAction,
   enableProfileSyncing as enableProfileSyncingAction,
@@ -74,6 +75,7 @@ export function useDisableProfileSyncing(): {
   error: string | null;
 } {
   const dispatch = useDispatch();
+  const { listNotifications } = useMetamaskNotificationsContext();
 
   const [error, setError] = useState<string | null>(null);
 
@@ -83,6 +85,9 @@ export function useDisableProfileSyncing(): {
     try {
       // disable profile syncing
       await dispatch(disableProfileSyncingAction());
+
+      // list notifications to update the counter
+      await listNotifications();
     } catch (e) {
       const errorMessage =
         e instanceof Error ? e.message : JSON.stringify(e ?? '');

diff --git a/ui/pages/notifications-settings/notifications-settings-allow-notifications.test.tsx b/ui/pages/notifications-settings/notifications-settings-allow-notifications.test.tsx
@@ -2,10 +2,12 @@ import React from 'react';
 import { render } from '@testing-library/react';
 import { Provider } from 'react-redux';
 import configureStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
 import { MetamaskNotificationsProvider } from '../../contexts/metamask-notifications/metamask-notifications';
 import { NotificationsSettingsAllowNotifications } from './notifications-settings-allow-notifications';
 
-const mockStore = configureStore();
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
 const store = mockStore({
   metamask: {
     isMetamaskNotificationsEnabled: false,

diff --git a/ui/pages/notifications-settings/notifications-settings-allow-notifications.tsx b/ui/pages/notifications-settings/notifications-settings-allow-notifications.tsx
@@ -74,15 +74,14 @@ export function NotificationsSettingsAllowNotifications({
   }, [isMetamaskNotificationsEnabled]);
 
   useEffect(() => {
-    if (isMetamaskNotificationsEnabled && !error) {
+    if (!error) {
       listNotifications();
     }
   }, [isMetamaskNotificationsEnabled, error, listNotifications]);
 
   const toggleNotifications = useCallback(async () => {
     setLoading(true);
     if (isMetamaskNotificationsEnabled) {
-      await disableNotifications();
       trackEvent({
         category: MetaMetricsEventCategory.NotificationSettings,
         event: MetaMetricsEventName.NotificationsSettingsUpdated,
@@ -93,8 +92,8 @@ export function NotificationsSettingsAllowNotifications({
           new_value: false,
         },
       });
+      await disableNotifications();
     } else {
-      await enableNotifications();
       trackEvent({
         category: MetaMetricsEventCategory.NotificationSettings,
         event: MetaMetricsEventName.NotificationsSettingsUpdated,
@@ -105,6 +104,7 @@ export function NotificationsSettingsAllowNotifications({
           new_value: true,
         },
       });
+      await enableNotifications();
     }
     setLoading(false);
     setToggleValue(!toggleValue);

diff --git a/ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.test.tsx b/ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.test.tsx
@@ -2,6 +2,7 @@ import React from 'react';
 import * as Redux from 'react-redux';
 import configureMockStore from 'redux-mock-store';
 import { render, fireEvent } from '@testing-library/react';
+import { MetamaskNotificationsProvider } from '../../../../contexts/metamask-notifications';
 import * as ProfileSyncingHook from '../../../../hooks/metamask-notifications/useProfileSyncing';
 import ProfileSyncToggle from './profile-sync-toggle';
 
@@ -20,7 +21,9 @@ describe('ProfileSyncToggle', () => {
   it('renders correctly', () => {
     const { getByTestId } = render(
       <Redux.Provider store={mockStore(initialStore())}>
-        <ProfileSyncToggle />
+        <MetamaskNotificationsProvider>
+          <ProfileSyncToggle />
+        </MetamaskNotificationsProvider>
       </Redux.Provider>,
     );
     expect(getByTestId('profileSyncToggle')).toBeInTheDocument();

diff --git a/ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.tsx b/ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.tsx
@@ -5,7 +5,6 @@ import { MetaMetricsContext } from '../../../../contexts/metametrics';
 import {
   useEnableProfileSyncing,
   useDisableProfileSyncing,
-  useSetIsProfileSyncingEnabled,
 } from '../../../../hooks/metamask-notifications/useProfileSyncing';
 import {
   MetaMetricsEventCategory,
@@ -31,14 +30,14 @@ import { getUseExternalServices } from '../../../../selectors';
 
 function ProfileSyncBasicFunctionalitySetting() {
   const basicFunctionality: boolean = useSelector(getUseExternalServices);
-  const { setIsProfileSyncingEnabled } = useSetIsProfileSyncingEnabled();
+  const { disableProfileSyncing } = useDisableProfileSyncing();
 
-  // Effect - toggle profile syncing off when basic functionality is off
+  // Effect - disable profile syncing when basic functionality is off
   useEffect(() => {
     if (basicFunctionality === false) {
-      setIsProfileSyncingEnabled(false);
+      disableProfileSyncing();
     }
-  }, [basicFunctionality, setIsProfileSyncingEnabled]);
+  }, [basicFunctionality, disableProfileSyncing]);
 
   return {
     isProfileSyncDisabled: !basicFunctionality,
@@ -72,7 +71,6 @@ const ProfileSyncToggle = () => {
         showModal({
           name: 'CONFIRM_TURN_OFF_PROFILE_SYNCING',
           turnOffProfileSyncing: () => {
-            disableProfileSyncing();
             trackEvent({
               category: MetaMetricsEventCategory.Settings,
               event: MetaMetricsEventName.SettingsUpdated,
@@ -84,11 +82,11 @@ const ProfileSyncToggle = () => {
                 was_notifications_on: isMetamaskNotificationsEnabled,
               },
             });
+            disableProfileSyncing();
           },
         }),
       );
     } else {
-      await enableProfileSyncing();
       trackEvent({
         category: MetaMetricsEventCategory.Settings,
         event: MetaMetricsEventName.SettingsUpdated,
@@ -100,6 +98,7 @@ const ProfileSyncToggle = () => {
           was_notifications_on: isMetamaskNotificationsEnabled,
         },
       });
+      await enableProfileSyncing();
     }
   };
 

diff --git a/ui/pages/settings/security-tab/security-tab.test.js b/ui/pages/settings/security-tab/security-tab.test.js
@@ -3,6 +3,7 @@ import userEvent from '@testing-library/user-event';
 import React from 'react';
 import configureMockStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
+import { MetamaskNotificationsProvider } from '../../../contexts/metamask-notifications';
 // TODO: Remove restricted import
 // eslint-disable-next-line import/no-restricted-paths
 import { getEnvironmentType } from '../../../../app/scripts/lib/util';
@@ -51,9 +52,16 @@ describe('Security Tab', () => {
 
   const mockStore = configureMockStore([thunk])(mockState);
 
+  function renderWithProviders(ui, store) {
+    return renderWithProvider(
+      <MetamaskNotificationsProvider>{ui}</MetamaskNotificationsProvider>,
+      store,
+    );
+  }
+
   function toggleCheckbox(testId, initialState, skipRender = false) {
     if (!skipRender) {
-      renderWithProvider(<SecurityTab />, mockStore);
+      renderWithProviders(<SecurityTab />, mockStore);
     }
 
     const container = screen.getByTestId(testId);
@@ -73,7 +81,7 @@ describe('Security Tab', () => {
   }
 
   it('should match snapshot', () => {
-    const { container } = renderWithProvider(<SecurityTab />, mockStore);
+    const { container } = renderWithProviders(<SecurityTab />, mockStore);
 
     expect(container).toMatchSnapshot();
   });
@@ -91,7 +99,7 @@ describe('Security Tab', () => {
     mockState.metamask.useNftDetection = false;
 
     const localMockStore = configureMockStore([thunk])(mockState);
-    renderWithProvider(<SecurityTab />, localMockStore);
+    renderWithProviders(<SecurityTab />, localMockStore);
 
     expect(await toggleCheckbox('useNftDetection', false, true)).toBe(true);
   });
@@ -129,7 +137,7 @@ describe('Security Tab', () => {
   });
 
   it('toggles SRP Quiz', async () => {
-    renderWithProvider(<SecurityTab />, mockStore);
+    renderWithProviders(<SecurityTab />, mockStore);
 
     expect(
       screen.queryByTestId(`srp_stage_introduction`),
@@ -150,7 +158,7 @@ describe('Security Tab', () => {
 
   it('sets IPFS gateway', async () => {
     const user = userEvent.setup();
-    renderWithProvider(<SecurityTab />, mockStore);
+    renderWithProviders(<SecurityTab />, mockStore);
 
     const ipfsField = screen.getByDisplayValue(mockState.metamask.ipfsGateway);
 
@@ -195,7 +203,7 @@ describe('Security Tab', () => {
     mockState.metamask.ipfsGateway = '';
 
     const localMockStore = configureMockStore([thunk])(mockState);
-    renderWithProvider(<SecurityTab />, localMockStore);
+    renderWithProviders(<SecurityTab />, localMockStore);
 
     expect(await toggleCheckbox('ipfsToggle', false, true)).toBe(true);
     expect(await toggleCheckbox('ipfsToggle', true, true)).toBe(true);
@@ -209,7 +217,7 @@ describe('Security Tab', () => {
 
   it('clicks "Add Custom Network"', async () => {
     const user = userEvent.setup();
-    renderWithProvider(<SecurityTab />, mockStore);
+    renderWithProviders(<SecurityTab />, mockStore);
 
     // Test the default path where `getEnvironmentType() === undefined`
     await user.click(screen.getByText(tEn('addCustomNetwork')));
@@ -229,7 +237,7 @@ describe('Security Tab', () => {
     mockState.metamask.metaMetricsId = 'fake-metametrics-id';
 
     const localMockStore = configureMockStore([thunk])(mockState);
-    renderWithProvider(<SecurityTab />, localMockStore);
+    renderWithProviders(<SecurityTab />, localMockStore);
 
     expect(
       screen.queryByTestId(`delete-metametrics-data-button`),