From 2f6a9511e80fc13a05908426d37513201ac73d10 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Tue, 17 Sep 2024 09:57:13 +0100 Subject: [PATCH 01/13] fix: message on the confirm alert modal --- app/_locales/en/messages.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index b512383c874e..fea9fb35610a 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -1019,10 +1019,10 @@ "message": "I have acknowledged the alert and still want to proceed" }, "confirmAlertModalDetails": { - "message": "If you sign in, a third party known for scams might take all your assets. Please review the alerts before you proceed." + "message": "To protect your assets and login information, we suggest you reject the request." }, "confirmAlertModalTitle": { - "message": "Your assets may be at risk" + "message": "This request is suspicious" }, "confirmConnectCustodianRedirect": { "message": "We will redirect you to $1 upon clicking continue." @@ -4529,6 +4529,9 @@ "revealTheSeedPhrase": { "message": "Reveal seed phrase" }, + "reviewAlert": { + "message": "Review alert" + }, "reviewAlerts": { "message": "Review alerts" }, From f022dfe57afc828f2211f7dbc2d27167430d1163 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Tue, 17 Sep 2024 09:59:30 +0100 Subject: [PATCH 02/13] fix confirm modal to display after all unconfirmed multiple alerts modal --- .../confirm-alert-modal.test.tsx | 2 +- .../confirm-alert-modal.tsx | 4 +- .../components/confirm/footer/footer.test.tsx | 77 ++++++++++++++----- .../components/confirm/footer/footer.tsx | 24 +++++- 4 files changed, 81 insertions(+), 26 deletions(-) diff --git a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx index ad365d78a9d2..eb8e934fdf2f 100644 --- a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx +++ b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx @@ -68,7 +68,7 @@ describe('ConfirmAlertModal', () => { mockStore, ); - expect(getByText('Your assets may be at risk')).toBeInTheDocument(); + expect(getByText('This request is suspicious')).toBeInTheDocument(); }); it('disables submit button when confirm modal is not acknowledged', () => { diff --git a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx index d46595e6b6be..5ec3edd97c68 100644 --- a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx +++ b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx @@ -126,9 +126,9 @@ export function ConfirmAlertModal({ const [confirmCheckbox, setConfirmCheckbox] = useState(false); - // if there are multiple alerts, show the multiple alert modal + // if there are unconfirmed danger alerts, show the multiple alert modal const [multipleAlertModalVisible, setMultipleAlertModalVisible] = - useState(unconfirmedDangerAlerts.length > 1); + useState(unconfirmedDangerAlerts.length > 0); const handleCloseMultipleAlertModal = useCallback( (request?: { recursive?: boolean }) => { diff --git a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx index 2c735a508422..98c709534e09 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx @@ -27,6 +27,7 @@ import { Severity } from '../../../../../helpers/constants/design-system'; import { SignatureRequestType } from '../../../types/confirm'; import * as confirmContext from '../../../context/confirm'; +import { Alert } from '../../../../../ducks/confirm-alerts/confirm-alerts'; import Footer from './footer'; jest.mock('react-redux', () => ({ @@ -247,7 +248,8 @@ describe('ConfirmFooter', () => { const OWNER_ID_MOCK = '123'; const KEY_ALERT_KEY_MOCK = 'Key'; const ALERT_MESSAGE_MOCK = 'Alert 1'; - const alertsMock = [ + + const alertsMock: Alert[] = [ { key: KEY_ALERT_KEY_MOCK, field: KEY_ALERT_KEY_MOCK, @@ -257,30 +259,65 @@ describe('ConfirmFooter', () => { alertDetails: ['Detail 1', 'Detail 2'], }, ]; - const stateWithAlertsMock = getMockPersonalSignConfirmStateForRequest( - { - ...unapprovedPersonalSignMsg, - id: OWNER_ID_MOCK, - msgParams: { - from: '0xc42edfcc21ed14dda456aa0756c153f7985d8813', - }, - } as SignatureRequestType, - { - confirmAlerts: { - alerts: { [OWNER_ID_MOCK]: alertsMock }, - confirmed: { - [OWNER_ID_MOCK]: { [KEY_ALERT_KEY_MOCK]: false }, + + const createStateWithAlerts = ( + alerts: Alert[], + confirmed: Record, + ) => { + return getMockPersonalSignConfirmStateForRequest( + { + ...unapprovedPersonalSignMsg, + id: OWNER_ID_MOCK, + msgParams: { + from: '0xc42edfcc21ed14dda456aa0756c153f7985d8813', }, + } as SignatureRequestType, + { + confirmAlerts: { + alerts: { [OWNER_ID_MOCK]: alerts }, + confirmed: { [OWNER_ID_MOCK]: confirmed }, + }, + metamask: {}, }, - metamask: {}, - }, - ); - it('renders the review alerts button when there are unconfirmed alerts', () => { + ); + }; + + const stateWithAlertsMock = createStateWithAlerts(alertsMock, { + [KEY_ALERT_KEY_MOCK]: false, + }); + + it('renders the "review alerts" button when there are unconfirmed alerts', () => { + const stateWithMultipleDangerAlerts = createStateWithAlerts( + [ + alertsMock[0], + { + ...alertsMock[0], + key: 'From', + }, + ], + { [KEY_ALERT_KEY_MOCK]: false }, + ); + const { getByText } = render(stateWithMultipleDangerAlerts); + expect(getByText('Review alerts')).toBeInTheDocument(); + }); + + it('renders the "review alert" button when there are unconfirmed alerts', () => { const { getByText } = render(stateWithAlertsMock); + expect(getByText('Review alert')).toBeInTheDocument(); + }); + + it('renders the "confirm" button when there are confirmed danger alerts', () => { + const stateWithConfirmedDangerAlertMock = createStateWithAlerts( + alertsMock, + { + [KEY_ALERT_KEY_MOCK]: true, + }, + ); + const { getByText } = render(stateWithConfirmedDangerAlertMock); expect(getByText('Confirm')).toBeInTheDocument(); }); - it('renders the confirm button when there are no unconfirmed alerts', () => { + it('renders the "confirm" button when there are no alerts', () => { const { getByText } = render(); expect(getByText('Confirm')).toBeInTheDocument(); }); @@ -288,7 +325,7 @@ describe('ConfirmFooter', () => { it('sets the alert modal visible when the review alerts button is clicked', () => { const { getByTestId } = render(stateWithAlertsMock); fireEvent.click(getByTestId('confirm-footer-button')); - expect(getByTestId('confirm-alert-modal-submit-button')).toBeDefined(); + expect(getByTestId('alert-modal-button')).toBeDefined(); }); }); }); diff --git a/ui/pages/confirmations/components/confirm/footer/footer.tsx b/ui/pages/confirmations/components/confirm/footer/footer.tsx index b7951c299255..39e2820c0570 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.tsx @@ -38,6 +38,7 @@ import { import { useConfirmContext } from '../../../context/confirm'; import { getConfirmationSender } from '../utils'; import { MetaMetricsEventLocation } from '../../../../../../shared/constants/metametrics'; +import { Alert } from '../../../../../ducks/confirm-alerts/confirm-alerts'; export type OnCancelHandler = ({ location, @@ -45,6 +46,20 @@ export type OnCancelHandler = ({ location: MetaMetricsEventLocation; }) => void; +function reviewAlertButtonText(unconfirmedDangerAlerts: Alert[]) { + const t = useI18nContext(); + + if (unconfirmedDangerAlerts.length === 1) { + return t('reviewAlert'); + } + + if (unconfirmedDangerAlerts.length > 1) { + return t('reviewAlerts'); + } + + return t('confirm'); +} + const ConfirmButton = ({ alertOwnerId = '', disabled, @@ -61,8 +76,11 @@ const ConfirmButton = ({ const [confirmModalVisible, setConfirmModalVisible] = useState(false); - const { dangerAlerts, hasDangerAlerts, hasUnconfirmedDangerAlerts } = - useAlerts(alertOwnerId); + const { + hasDangerAlerts, + hasUnconfirmedDangerAlerts, + unconfirmedDangerAlerts, + } = useAlerts(alertOwnerId); const handleCloseConfirmModal = useCallback(() => { setConfirmModalVisible(false); @@ -92,7 +110,7 @@ const ConfirmButton = ({ size={ButtonSize.Lg} startIconName={IconName.Danger} > - {dangerAlerts?.length > 1 ? t('reviewAlerts') : t('confirm')} + {reviewAlertButtonText(unconfirmedDangerAlerts)} ) : ( ) : ( ) : ( @@ -308,6 +308,7 @@ export function AlertModal({ customDetails, customAcknowledgeCheckbox, customAcknowledgeButton, + showCloseIcon = true, }: AlertModalProps) { const { isAlertConfirmed, setAlertConfirmed, alerts } = useAlerts(ownerId); const { trackAlertRender } = useAlertMetrics(); @@ -342,13 +343,14 @@ export function AlertModal({ diff --git a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx index 4c308cca05f2..c5e5923ac845 100644 --- a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx +++ b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.test.tsx @@ -134,28 +134,4 @@ describe('ConfirmAlertModal', () => { expect(getByText(DATA_ALERT_MESSAGE_MOCK)).toBeInTheDocument(); }); }); - - describe('when there is a blocking alert', () => { - it('closes the modal when the "Got it" button is clicked', () => { - const blockingAlert = { ...alertsMock[0], isBlocking: true }; - const mockStoreBlockingAlert = configureMockStore([])({ - ...STATE_MOCK, - confirmAlerts: { - alerts: { [OWNER_ID_MOCK]: [blockingAlert] }, - confirmed: { - [OWNER_ID_MOCK]: { - [DATA_ALERT_KEY_MOCK]: false, - }, - }, - }, - }); - const { getByTestId } = renderWithProvider( - , - mockStoreBlockingAlert, - ); - - fireEvent.click(getByTestId('alert-modal-close-button')); - expect(onCloseMock).toHaveBeenCalledTimes(1); - }); - }); }); diff --git a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx index 8bb92326f868..8f5c04e3491c 100644 --- a/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx +++ b/ui/components/app/alert-system/confirm-alert-modal/confirm-alert-modal.tsx @@ -126,26 +126,33 @@ export function ConfirmAlertModal({ const [confirmCheckbox, setConfirmCheckbox] = useState(false); - const unconfirmedDangerFieldAlerts = fieldAlerts.filter( + const unconfirmedDangerAlerts = fieldAlerts.filter( (alert) => !isAlertConfirmed(alert.key) && alert.severity === Severity.Danger, ); - const hasBlockingAlerts = fieldAlerts.some((alert) => alert.isBlocking); + const hasDangerBlockingAlerts = fieldAlerts.some( + (alert) => alert.severity === Severity.Danger && alert.isBlocking, + ); + const hasUnconfirmedDangerAlerts = unconfirmedDangerAlerts.length > 0; // if there are unconfirmed danger alerts, show the multiple alert modal const [multipleAlertModalVisible, setMultipleAlertModalVisible] = - useState(unconfirmedDangerFieldAlerts.length > 0); + useState(hasUnconfirmedDangerAlerts); const handleCloseMultipleAlertModal = useCallback( (request?: { recursive?: boolean }) => { setMultipleAlertModalVisible(false); - if (request?.recursive || hasBlockingAlerts) { + if ( + request?.recursive || + hasUnconfirmedDangerAlerts || + hasDangerBlockingAlerts + ) { onClose(); } }, - [onClose], + [onClose, hasUnconfirmedDangerAlerts, hasDangerBlockingAlerts], ); const handleOpenMultipleAlertModal = useCallback(() => { @@ -162,6 +169,7 @@ export function ConfirmAlertModal({ ownerId={ownerId} onFinalAcknowledgeClick={handleCloseMultipleAlertModal} onClose={handleCloseMultipleAlertModal} + showCloseIcon={false} /> ); } diff --git a/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.test.tsx b/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.test.tsx index 4d55c0922ad3..c4b79fb28b7c 100644 --- a/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.test.tsx +++ b/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.test.tsx @@ -70,6 +70,20 @@ describe('MultipleAlertModal', () => { onClose: onCloseMock, }; + const mockStoreAcknowledgeAlerts = configureMockStore([])({ + ...STATE_MOCK, + confirmAlerts: { + alerts: { [OWNER_ID_MOCK]: alertsMock }, + confirmed: { + [OWNER_ID_MOCK]: { + [FROM_ALERT_KEY_MOCK]: true, + [DATA_ALERT_KEY_MOCK]: true, + [CONTRACT_ALERT_KEY_MOCK]: false, + }, + }, + }, + }); + it('renders the multiple alert modal', () => { const { getByTestId } = renderWithProvider( , @@ -80,19 +94,6 @@ describe('MultipleAlertModal', () => { }); it('invokes the onFinalAcknowledgeClick when the button is clicked', () => { - const mockStoreAcknowledgeAlerts = configureMockStore([])({ - ...STATE_MOCK, - confirmAlerts: { - alerts: { [OWNER_ID_MOCK]: alertsMock }, - confirmed: { - [OWNER_ID_MOCK]: { - [FROM_ALERT_KEY_MOCK]: true, - [DATA_ALERT_KEY_MOCK]: true, - [CONTRACT_ALERT_KEY_MOCK]: true, - }, - }, - }, - }); const { getByTestId } = renderWithProvider( { }); it('render the next alert when the "Got it" button is clicked', () => { - const mockStoreAcknowledgeAlerts = configureMockStore([])({ - ...STATE_MOCK, - confirmAlerts: { - alerts: { [OWNER_ID_MOCK]: alertsMock }, - confirmed: { - [OWNER_ID_MOCK]: { - [FROM_ALERT_KEY_MOCK]: true, - [DATA_ALERT_KEY_MOCK]: true, - [CONTRACT_ALERT_KEY_MOCK]: false, - }, - }, - }, - }); const { getByTestId, getByText } = renderWithProvider( , mockStoreAcknowledgeAlerts, @@ -130,6 +118,22 @@ describe('MultipleAlertModal', () => { expect(getByText(alertsMock[1].message)).toBeInTheDocument(); }); + it('closes modal when the "Got it" button is clicked', () => { + onAcknowledgeClickMock.mockReset(); + const { getByTestId } = renderWithProvider( + , + mockStoreAcknowledgeAlerts, + ); + + fireEvent.click(getByTestId('alert-modal-button')); + + expect(onAcknowledgeClickMock).toHaveBeenCalledTimes(1); + }); + describe('Navigation', () => { it('calls next alert when the next button is clicked', () => { const { getByTestId, getByText } = renderWithProvider( diff --git a/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx b/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx index ed900c52b199..d3b289343d00 100644 --- a/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx +++ b/ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx @@ -30,6 +30,10 @@ export type MultipleAlertModalProps = { onClose: (request?: { recursive?: boolean }) => void; /** The unique identifier of the entity that owns the alert. */ ownerId: string; + /** Whether to show the close icon in the modal header. */ + showCloseIcon?: boolean; + /** Whether to skip the unconfirmed alerts validation and close the modal directly. */ + skipAlertNavigation?: boolean; }; function PreviousButton({ @@ -145,6 +149,8 @@ export function MultipleAlertModal({ onClose, onFinalAcknowledgeClick, ownerId, + showCloseIcon = true, + skipAlertNavigation = false, }: MultipleAlertModalProps) { const { isAlertConfirmed, fieldAlerts: alerts } = useAlerts(ownerId); @@ -173,6 +179,11 @@ export function MultipleAlertModal({ }, []); const handleAcknowledgeClick = useCallback(() => { + if (skipAlertNavigation) { + onFinalAcknowledgeClick(); + return; + } + if (selectedIndex + 1 === alerts.length) { if (!hasUnconfirmedAlerts) { onFinalAcknowledgeClick(); @@ -189,6 +200,7 @@ export function MultipleAlertModal({ selectedIndex, alerts.length, hasUnconfirmedAlerts, + skipAlertNavigation, ]); return ( @@ -205,6 +217,7 @@ export function MultipleAlertModal({ selectedIndex={selectedIndex} /> } + showCloseIcon={showCloseIcon} /> ); } diff --git a/ui/components/app/confirm/info/row/alert-row/alert-row.tsx b/ui/components/app/confirm/info/row/alert-row/alert-row.tsx index 532bd5987c7c..3956cc3095eb 100644 --- a/ui/components/app/confirm/info/row/alert-row/alert-row.tsx +++ b/ui/components/app/confirm/info/row/alert-row/alert-row.tsx @@ -85,6 +85,8 @@ export const ConfirmInfoAlertRow = ({ ownerId={ownerId} onFinalAcknowledgeClick={handleModalClose} onClose={handleModalClose} + showCloseIcon={false} + skipAlertNavigation={true} /> )} diff --git a/ui/pages/confirmations/components/confirm/footer/footer.tsx b/ui/pages/confirmations/components/confirm/footer/footer.tsx index cef35e4b8397..22404d8a2b4e 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.tsx @@ -62,6 +62,22 @@ function reviewAlertButtonText( return t('confirm'); } +function getButtonDisabledState( + hasUnconfirmedDangerAlerts: boolean, + hasBlockingAlerts: boolean, + disabled: boolean, +) { + if (hasBlockingAlerts) { + return true; + } + + if (hasUnconfirmedDangerAlerts) { + return false; + } + + return disabled; +} + const ConfirmButton = ({ alertOwnerId = '', disabled, @@ -85,11 +101,15 @@ const ConfirmButton = ({ isAlertConfirmed, } = useAlerts(alertOwnerId); - const unconfirmedDangerAlertsFields = fieldAlerts.filter( + const unconfirmedDangerAlerts = fieldAlerts.filter( (alert) => !isAlertConfirmed(alert.key) && alert.severity === Severity.Danger, ); + const hasDangerBlockingAlerts = fieldAlerts.some( + (alert) => alert.severity === Severity.Danger && alert.isBlocking, + ); + const handleCloseConfirmModal = useCallback(() => { setConfirmModalVisible(false); }, []); @@ -113,16 +133,20 @@ const ConfirmButton = ({ block danger data-testid="confirm-footer-button" - disabled={hasUnconfirmedDangerAlerts ? false : disabled} + disabled={getButtonDisabledState( + hasUnconfirmedDangerAlerts, + hasDangerBlockingAlerts, + disabled, + )} onClick={handleOpenConfirmModal} size={ButtonSize.Lg} startIconName={ - unconfirmedDangerAlertsFields.length > 0 + unconfirmedDangerAlerts.length > 0 ? IconName.SecuritySearch : IconName.Danger } > - {reviewAlertButtonText(unconfirmedDangerAlertsFields, t)} + {reviewAlertButtonText(unconfirmedDangerAlerts, t)} ) : (