From 16f535cef0832c207582a7194231520b5c322cbd Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 25 Nov 2024 18:13:28 +0000 Subject: [PATCH] fix: breaking selector due to missing controller state (#12375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The selector is trying to access some engine state that is removed at build time through code-fences. This fix ensures that the selectors are using the engine state or default to the controllers default state. Long term - we need to investigate and ensure that the UI is not calling the selectors. ## **Related issues** Fixes: https://github.com/MetaMask/metamask-mobile/issues/11909 ## **Manual testing steps** 1. Can you start up the application without it breaking? ## **Screenshots/Recordings** Before / After Recording https://www.loom.com/share/badc4b4cd3b0446486497973dcf337f6?sid=33bcfe23-c495-41bf-89f5-e57b4b72e3d2 I was not able to replicate the controller state being undefined/missing, so forced it to be undefined when testing. ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.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. --- .../UI/Notification/List/index.test.tsx | 5 + .../SecuritySettings.test.tsx | 8 + .../SecuritySettings.test.tsx.snap | 184 +----------------- app/selectors/notifications/index.tsx | 11 +- .../hooks/useCreateSession.test.tsx | 5 + .../notifications/hooks/useCreateSession.ts | 5 + .../hooks/useNotifications.test.tsx | 5 + .../notifications/hooks/useNotifications.ts | 40 +++- .../hooks/useProfileSyncing.test.tsx | 5 + .../notifications/hooks/useProfileSyncing.ts | 9 + .../hooks/usePushNotifications.ts | 12 +- .../hooks/useSwitchNotifications.test.tsx | 26 ++- .../hooks/useSwitchNotifications.ts | 21 +- 13 files changed, 126 insertions(+), 210 deletions(-) diff --git a/app/components/UI/Notification/List/index.test.tsx b/app/components/UI/Notification/List/index.test.tsx index 5908fb62a6e..987450d0dc1 100644 --- a/app/components/UI/Notification/List/index.test.tsx +++ b/app/components/UI/Notification/List/index.test.tsx @@ -21,6 +21,11 @@ const mockNavigation = createNavigationProps({}); const mockTrackEvent = jest.fn(); +jest.mock('../../../../util/notifications/constants', () => ({ + ...jest.requireActual('../../../../util/notifications/constants'), + isNotificationsFeatureEnabled: () => true, +})); + jest.mock('../../../../util/notifications/services/NotificationService', () => ({ ...jest.requireActual('../../../../util/notifications/services/NotificationService'), getBadgeCount: jest.fn(), diff --git a/app/components/Views/Settings/SecuritySettings/SecuritySettings.test.tsx b/app/components/Views/Settings/SecuritySettings/SecuritySettings.test.tsx index 07dfc2dbdd6..a42b87705f9 100644 --- a/app/components/Views/Settings/SecuritySettings/SecuritySettings.test.tsx +++ b/app/components/Views/Settings/SecuritySettings/SecuritySettings.test.tsx @@ -29,6 +29,9 @@ const initialState = { backgroundState: { ...backgroundState, AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE, + UserStorageController: { + isProfileSyncingEnabled: false, + }, }, }, security: { @@ -69,6 +72,11 @@ jest.mock('../../../../util/navigation/navUtils', () => ({ useParams: jest.fn(() => mockUseParamsValues), })); +jest.mock('../../../../util/notifications/constants', () => ({ + ...jest.requireActual('../../../../util/notifications/constants'), + isNotificationsFeatureEnabled: () => false, +})); + describe('SecuritySettings', () => { beforeEach(() => { mockUseParamsValues = { diff --git a/app/components/Views/Settings/SecuritySettings/__snapshots__/SecuritySettings.test.tsx.snap b/app/components/Views/Settings/SecuritySettings/__snapshots__/SecuritySettings.test.tsx.snap index e988700ff2d..8f218fb0c8b 100644 --- a/app/components/Views/Settings/SecuritySettings/__snapshots__/SecuritySettings.test.tsx.snap +++ b/app/components/Views/Settings/SecuritySettings/__snapshots__/SecuritySettings.test.tsx.snap @@ -3583,188 +3583,8 @@ exports[`SecuritySettings should render correctly 1`] = ` swipeDirection="down" swipeThreshold={100} transparent={true} - visible={true} - > - - - - - - - -  - - - - - - Disabling profile syncing... - - - - + visible={false} + /> `; diff --git a/app/selectors/notifications/index.tsx b/app/selectors/notifications/index.tsx index e1c8221119e..b5a8c387961 100644 --- a/app/selectors/notifications/index.tsx +++ b/app/selectors/notifications/index.tsx @@ -17,18 +17,21 @@ type AuthenticationState = type UserStorageState = UserStorageController.UserStorageControllerState; const selectAuthenticationControllerState = (state: RootState) => - state?.engine?.backgroundState?.AuthenticationController; + state?.engine?.backgroundState?.AuthenticationController ?? + AuthenticationController.defaultState; const selectUserStorageControllerState = (state: RootState) => - state?.engine?.backgroundState?.UserStorageController; + state?.engine?.backgroundState?.UserStorageController ?? + UserStorageController.defaultState; const selectNotificationServicesControllerState = (state: RootState) => - state?.engine?.backgroundState?.NotificationServicesController; + state?.engine?.backgroundState?.NotificationServicesController ?? + NotificationServicesController.defaultState; export const selectIsProfileSyncingEnabled = createSelector( selectUserStorageControllerState, (userStorageControllerState: UserStorageState) => - userStorageControllerState.isProfileSyncingEnabled, + userStorageControllerState?.isProfileSyncingEnabled, ); export const selectIsProfileSyncingUpdateLoading = createSelector( selectUserStorageControllerState, diff --git a/app/util/notifications/hooks/useCreateSession.test.tsx b/app/util/notifications/hooks/useCreateSession.test.tsx index 27af4f996ce..ae2bd99d712 100644 --- a/app/util/notifications/hooks/useCreateSession.test.tsx +++ b/app/util/notifications/hooks/useCreateSession.test.tsx @@ -11,6 +11,11 @@ import * as Selectors from '../../../selectors/notifications'; import * as Actions from '../../../actions/notification/helpers'; import useCreateSession from './useCreateSession'; +jest.mock('../constants', () => ({ + ...jest.requireActual('../constants'), + isNotificationsFeatureEnabled: () => true, +})); + function arrangeStore() { const store = createMockStore()(initialRootState); diff --git a/app/util/notifications/hooks/useCreateSession.ts b/app/util/notifications/hooks/useCreateSession.ts index 443c1859ed7..e203d146a53 100644 --- a/app/util/notifications/hooks/useCreateSession.ts +++ b/app/util/notifications/hooks/useCreateSession.ts @@ -11,6 +11,7 @@ import { disableProfileSyncing, signIn, } from '../../../actions/notification/helpers'; +import { isNotificationsFeatureEnabled } from '../constants'; /** * Custom hook to manage the creation of a session based on the user's authentication status, @@ -31,6 +32,10 @@ function useCreateSession(): UseCreateSessionReturn { const [error, setError] = useState(undefined); const createSession = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + // If the user is already signed in, no need to create a new session if (isSignedIn) { return; diff --git a/app/util/notifications/hooks/useNotifications.test.tsx b/app/util/notifications/hooks/useNotifications.test.tsx index 3e6c6581bf3..99b4eda7ab4 100644 --- a/app/util/notifications/hooks/useNotifications.test.tsx +++ b/app/util/notifications/hooks/useNotifications.test.tsx @@ -24,6 +24,11 @@ import { createMockNotificationEthSent, } from '../../../components/UI/Notification/__mocks__/mock_notifications'; +jest.mock('../constants', () => ({ + ...jest.requireActual('../constants'), + isNotificationsFeatureEnabled: () => true, +})); + function arrangeStore() { const store = createMockStore()(initialRootState); diff --git a/app/util/notifications/hooks/useNotifications.ts b/app/util/notifications/hooks/useNotifications.ts index 0060e39ceb9..3845a4b1963 100644 --- a/app/util/notifications/hooks/useNotifications.ts +++ b/app/util/notifications/hooks/useNotifications.ts @@ -21,6 +21,8 @@ import { } from '../../../actions/notification/helpers'; import { getNotificationsList } from '../../../selectors/notifications'; import { usePushNotifications } from './usePushNotifications'; +import { isNotificationsFeatureEnabled } from '../constants'; + /** * Custom hook to fetch and update the list of notifications. * Manages loading and error states internally. @@ -33,6 +35,10 @@ export function useListNotifications(): ListNotificationsReturn { const [error, setError] = useState(); const listNotifications = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { @@ -68,6 +74,10 @@ export function useCreateNotifications(): CreateNotificationsReturn { const [error, setError] = useState(); const createNotifications = useCallback(async (accounts: string[]) => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { @@ -106,12 +116,19 @@ export function useEnableNotifications(): EnableNotificationsReturn { const [error, setError] = useState(); const { switchPushNotifications } = usePushNotifications(); const enableNotifications = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { const errorEnablingNotifications = await enableNotificationServices(); - const errorEnablingPushNotifications = await switchPushNotifications(true); - const errorMessage = errorEnablingNotifications || errorEnablingPushNotifications; + const errorEnablingPushNotifications = await switchPushNotifications( + true, + ); + const errorMessage = + errorEnablingNotifications || errorEnablingPushNotifications; if (errorMessage) { setError(getErrorMessage(errorMessage)); @@ -143,12 +160,19 @@ export function useDisableNotifications(): DisableNotificationsReturn { const [error, setError] = useState(); const { switchPushNotifications } = usePushNotifications(); const disableNotifications = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { const errorDisablingNotifications = await disableNotificationServices(); - const errorDisablingPushNotifications = await switchPushNotifications(false); - const errorMessage = errorDisablingNotifications || errorDisablingPushNotifications; + const errorDisablingPushNotifications = await switchPushNotifications( + false, + ); + const errorMessage = + errorDisablingNotifications || errorDisablingPushNotifications; if (errorMessage) { setError(getErrorMessage(errorMessage)); @@ -182,6 +206,10 @@ export function useMarkNotificationAsRead(): MarkNotificationAsReadReturn { const markNotificationAsRead = useCallback( async (notifications: MarkAsReadNotificationsParam) => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { @@ -221,6 +249,10 @@ export function useDeleteNotificationsStorageKey(): deleteNotificationsStorageKe const [error, setError] = useState(); const deleteNotificationsStorageKey = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { diff --git a/app/util/notifications/hooks/useProfileSyncing.test.tsx b/app/util/notifications/hooks/useProfileSyncing.test.tsx index 3e9885dd8ef..e49a44fca31 100644 --- a/app/util/notifications/hooks/useProfileSyncing.test.tsx +++ b/app/util/notifications/hooks/useProfileSyncing.test.tsx @@ -11,6 +11,11 @@ import * as Actions from '../../../actions/notification/helpers'; import initialRootState from '../../../util/test/initial-root-state'; import { useProfileSyncing } from './useProfileSyncing'; +jest.mock('../constants', () => ({ + ...jest.requireActual('../constants'), + isNotificationsFeatureEnabled: () => true, +})); + function arrangeStore() { const store = createMockStore()(initialRootState); diff --git a/app/util/notifications/hooks/useProfileSyncing.ts b/app/util/notifications/hooks/useProfileSyncing.ts index 5f0d742dd22..258e6ffe23a 100644 --- a/app/util/notifications/hooks/useProfileSyncing.ts +++ b/app/util/notifications/hooks/useProfileSyncing.ts @@ -5,6 +5,7 @@ import { disableProfileSyncing as disableProfileSyncingAction, enableProfileSyncing as enableProfileSyncingAction, } from '../../../actions/notification/helpers'; +import { isNotificationsFeatureEnabled } from '../constants'; /** * Custom hook to enable profile syncing. This hook handles the process of signing in @@ -17,6 +18,10 @@ export function useProfileSyncing(): ProfileSyncingReturn { const [error, setError] = useState(); const enableProfileSyncing = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { @@ -36,6 +41,10 @@ export function useProfileSyncing(): ProfileSyncingReturn { }, []); const disableProfileSyncing = useCallback(async () => { + if (!isNotificationsFeatureEnabled()) { + return; + } + setLoading(true); setError(undefined); try { diff --git a/app/util/notifications/hooks/usePushNotifications.ts b/app/util/notifications/hooks/usePushNotifications.ts index e373660361f..ef356fa968c 100644 --- a/app/util/notifications/hooks/usePushNotifications.ts +++ b/app/util/notifications/hooks/usePushNotifications.ts @@ -6,7 +6,7 @@ import { } from '../../../actions/notification/helpers'; import { mmStorage } from '../settings'; import { UserStorage } from '@metamask/notification-services-controller/dist/NotificationServicesController/types/user-storage/index.cjs'; - +import { isNotificationsFeatureEnabled } from '../constants'; export function usePushNotifications() { const [loading, setLoading] = useState(false); @@ -18,6 +18,10 @@ export function usePushNotifications() { const switchPushNotifications = useCallback( async (state: boolean) => { + if (!isNotificationsFeatureEnabled()) { + return; + } + resetStates(); setLoading(true); let errorMessage: string | undefined; @@ -26,7 +30,10 @@ export function usePushNotifications() { const userStorage: UserStorage = mmStorage.getLocal('pnUserStorage'); if (state) { const fcmToken = mmStorage.getLocal('metaMaskFcmToken'); - errorMessage = await enablePushNotifications(userStorage, fcmToken?.data); + errorMessage = await enablePushNotifications( + userStorage, + fcmToken?.data, + ); } else { errorMessage = await disablePushNotifications(userStorage); } @@ -43,7 +50,6 @@ export function usePushNotifications() { [resetStates], ); - return { switchPushNotifications, loading, diff --git a/app/util/notifications/hooks/useSwitchNotifications.test.tsx b/app/util/notifications/hooks/useSwitchNotifications.test.tsx index 87b23072404..dec1378d520 100644 --- a/app/util/notifications/hooks/useSwitchNotifications.test.tsx +++ b/app/util/notifications/hooks/useSwitchNotifications.test.tsx @@ -20,6 +20,11 @@ import { Hex } from '@metamask/utils'; import { KeyringTypes } from '@metamask/keyring-controller'; import Engine from '../../../core/Engine'; +jest.mock('../constants', () => ({ + ...jest.requireActual('../constants'), + isNotificationsFeatureEnabled: () => true, +})); + jest.mock('../../../core/Engine', () => ({ context: { NotificationServicesController: { @@ -170,15 +175,13 @@ describe('useAccountSettingsProps', () => { .spyOn(Selectors, 'selectIsUpdatingMetamaskNotificationsAccount') .mockReturnValue([MOCK_ACCOUNTS[0].address]); - const isMetamaskNotificationsEnabled = jest - .spyOn(Selectors, - 'selectIsMetamaskNotificationsEnabled', - ) + const isMetamaskNotificationsEnabled = jest + .spyOn(Selectors, 'selectIsMetamaskNotificationsEnabled') .mockReturnValue(true); return { selectIsUpdatingMetamaskNotificationsAccount, - isMetamaskNotificationsEnabled + isMetamaskNotificationsEnabled, }; } @@ -189,9 +192,12 @@ describe('useAccountSettingsProps', () => { [MOCK_ACCOUNTS[1].address]: false, }); - Engine.context.NotificationServicesController.checkAccountsPresence = mockCheckAccountsPresence; + Engine.context.NotificationServicesController.checkAccountsPresence = + mockCheckAccountsPresence; - mockSelectors.selectIsUpdatingMetamaskNotificationsAccount.mockReturnValue([]); + mockSelectors.selectIsUpdatingMetamaskNotificationsAccount.mockReturnValue( + [], + ); mockSelectors.isMetamaskNotificationsEnabled.mockReturnValue(true); const { hook, store } = arrangeHook(MOCK_ACCOUNTS); @@ -200,13 +206,15 @@ describe('useAccountSettingsProps', () => { await hook.result.current.updateAndfetchAccountSettings(); }); - expect(mockCheckAccountsPresence).toHaveBeenCalledWith(MOCK_ACCOUNTS.map(account => account.address)); + expect(mockCheckAccountsPresence).toHaveBeenCalledWith( + MOCK_ACCOUNTS.map((account) => account.address), + ); expect(store.dispatch).toHaveBeenCalledWith( updateAccountState({ [MOCK_ACCOUNTS[0].address]: true, [MOCK_ACCOUNTS[1].address]: false, - }) + }), ); }); }); diff --git a/app/util/notifications/hooks/useSwitchNotifications.ts b/app/util/notifications/hooks/useSwitchNotifications.ts index 81a65e482b6..4d9c32e603c 100644 --- a/app/util/notifications/hooks/useSwitchNotifications.ts +++ b/app/util/notifications/hooks/useSwitchNotifications.ts @@ -11,6 +11,7 @@ import { useDispatch } from 'react-redux'; import { updateAccountState } from '../../../core/redux/slices/notifications'; import { Account } from '../../../components/hooks/useAccounts/useAccounts.types'; import Logger from '../../../util/Logger'; +import { isNotificationsFeatureEnabled } from '../constants'; export function useSwitchNotifications() { const [loading, setLoading] = useState(false); @@ -22,6 +23,10 @@ export function useSwitchNotifications() { const switchFeatureAnnouncements = useCallback( async (state: boolean) => { + if (!isNotificationsFeatureEnabled()) { + return; + } + resetStates(); setLoading(true); @@ -89,14 +94,14 @@ export function useAccountSettingsProps(accounts: Account[]) { // Memoize the accounts array to avoid unnecessary re-fetching const memoAccounts = useMemo(() => accounts.map((account) => account.address),[accounts]); const updateAndfetchAccountSettings = useCallback(async () => { - try { - const result = await Engine.context.NotificationServicesController.checkAccountsPresence(memoAccounts); - dispatch(updateAccountState(result)); - return result; - } catch (err) { - Logger.log('Failed to get account settings:', err); - } -}, [dispatch, memoAccounts]); + try { + const result = await Engine.context.NotificationServicesController.checkAccountsPresence(memoAccounts); + dispatch(updateAccountState(result)); + return result; + } catch (err) { + Logger.log('Failed to get account settings:', err); + } + }, [dispatch, memoAccounts]); return { updateAndfetchAccountSettings }; }