From f0aad1ca700233a320d3b34fa0d758325432bc83 Mon Sep 17 00:00:00 2001 From: Matteo Scurati Date: Thu, 24 Oct 2024 16:05:49 +0200 Subject: [PATCH] fix: disable notifications when basic functionality off (#28045) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **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** Screenshot 2024-10-24 at 10 08 32 ### **After** Screenshot 2024-10-24 at 10 19 48 ## **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. --- .../useProfileSyncing.test.tsx | 34 +++++++++++++++---- .../useProfileSyncing.ts | 5 +++ ...ions-settings-allow-notifications.test.tsx | 4 ++- ...fications-settings-allow-notifications.tsx | 6 ++-- .../profile-sync-toggle.test.tsx | 5 ++- .../profile-sync-toggle.tsx | 13 ++++--- .../security-tab/security-tab.test.js | 24 ++++++++----- 7 files changed, 65 insertions(+), 26 deletions(-) diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx b/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx index 951cec333ade..298e442acd19 100644 --- 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; +}) => ( + + {children} + +); + describe('useProfileSyncing', () => { it('should enable profile syncing', async () => { const { store } = arrangeMocks(); const { result } = renderHook(() => useEnableProfileSyncing(), { - wrapper: ({ children }) => {children}, + wrapper: ({ children }) => ( + {children} + ), }); act(() => { @@ -96,7 +112,9 @@ describe('useProfileSyncing', () => { const { store } = arrangeMocks(); const { result } = renderHook(() => useDisableProfileSyncing(), { - wrapper: ({ children }) => {children}, + wrapper: ({ children }) => ( + {children} + ), }); act(() => { @@ -113,7 +131,9 @@ describe('useProfileSyncing', () => { }); renderHook(() => useAccountSyncingEffect(), { - wrapper: ({ children }) => {children}, + wrapper: ({ children }) => ( + {children} + ), }); await waitFor(() => { @@ -125,7 +145,9 @@ describe('useProfileSyncing', () => { const { store } = arrangeMocks(); renderHook(() => useAccountSyncingEffect(), { - wrapper: ({ children }) => {children}, + wrapper: ({ children }) => ( + {children} + ), }); await waitFor(() => { @@ -142,7 +164,7 @@ describe('useProfileSyncing', () => { () => useDeleteAccountSyncingDataFromUserStorage(), { wrapper: ({ children }) => ( - {children} + {children} ), }, ); diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.ts b/ui/hooks/metamask-notifications/useProfileSyncing.ts index 67899aa73927..520f682157be 100644 --- 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(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 index 9f90394db09d..7d1197f64acd 100644 --- 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 index 82310a7e4f85..a202acea1831 100644 --- a/ui/pages/notifications-settings/notifications-settings-allow-notifications.tsx +++ b/ui/pages/notifications-settings/notifications-settings-allow-notifications.tsx @@ -74,7 +74,7 @@ export function NotificationsSettingsAllowNotifications({ }, [isMetamaskNotificationsEnabled]); useEffect(() => { - if (isMetamaskNotificationsEnabled && !error) { + if (!error) { listNotifications(); } }, [isMetamaskNotificationsEnabled, error, listNotifications]); @@ -82,7 +82,6 @@ export function NotificationsSettingsAllowNotifications({ 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 index 6bc9bed95cb1..1c5cb237f0f8 100644 --- 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( - + + + , ); 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 index 534fe109d1c4..d71d503faaeb 100644 --- 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 index 5e31cfb68c57..1685c5417151 100644 --- 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( + {ui}, + store, + ); + } + function toggleCheckbox(testId, initialState, skipRender = false) { if (!skipRender) { - renderWithProvider(, mockStore); + renderWithProviders(, mockStore); } const container = screen.getByTestId(testId); @@ -73,7 +81,7 @@ describe('Security Tab', () => { } it('should match snapshot', () => { - const { container } = renderWithProvider(, mockStore); + const { container } = renderWithProviders(, mockStore); expect(container).toMatchSnapshot(); }); @@ -91,7 +99,7 @@ describe('Security Tab', () => { mockState.metamask.useNftDetection = false; const localMockStore = configureMockStore([thunk])(mockState); - renderWithProvider(, localMockStore); + renderWithProviders(, localMockStore); expect(await toggleCheckbox('useNftDetection', false, true)).toBe(true); }); @@ -129,7 +137,7 @@ describe('Security Tab', () => { }); it('toggles SRP Quiz', async () => { - renderWithProvider(, mockStore); + renderWithProviders(, mockStore); expect( screen.queryByTestId(`srp_stage_introduction`), @@ -150,7 +158,7 @@ describe('Security Tab', () => { it('sets IPFS gateway', async () => { const user = userEvent.setup(); - renderWithProvider(, mockStore); + renderWithProviders(, mockStore); const ipfsField = screen.getByDisplayValue(mockState.metamask.ipfsGateway); @@ -195,7 +203,7 @@ describe('Security Tab', () => { mockState.metamask.ipfsGateway = ''; const localMockStore = configureMockStore([thunk])(mockState); - renderWithProvider(, localMockStore); + renderWithProviders(, 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(, mockStore); + renderWithProviders(, 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(, localMockStore); + renderWithProviders(, localMockStore); expect( screen.queryByTestId(`delete-metametrics-data-button`),