Skip to content

Commit

Permalink
fix: banner alert to render multiple general alerts (#27339)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This PR aims to fix the banner alert to support rendering multiple
alerts. Previously we only rendered one alert and if there were more
alerts we rendered the banner with a default copy informing the user
there are multiple alerts.

- Fixed padding on the alerts modal based on
[figma](https://www.figma.com/design/gcwF9smHsgvFWQK83lT5UU/Confirmation-redesign-V4?node-id=3355-12480&node-type=frame&t=3Vbe0qFcmcfN5uCG-0)
- Fixed bug Contract Interaction and Alerts - 'Cannot read properties of
undefined (reading 'key')`
<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27339?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2873
#27238

## **Manual testing steps**

1. Create a transaction with high nonce
2. Go to test dapp
3. Trigger a malicious transaction from PPOM session

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

![Screenshot from 2024-09-23
13-38-10](https://github.com/user-attachments/assets/f4cbe8ee-7217-4718-998a-2016c9c60b88)

![Screenshot from 2024-09-23
14-09-42](https://github.com/user-attachments/assets/abb8c0c0-8cb8-4230-9469-d0b8b9f2a9a1)

![Screenshot from 2024-09-23
14-21-53](https://github.com/user-attachments/assets/0747e0d0-d50f-4f59-9a9e-0baefb4d9b5e)


[bug.webm](https://github.com/user-attachments/assets/eb447959-78f0-4ccc-a554-cca272e59b19)


<!-- [screenshots/recordings] -->

## **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**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

---------

Co-authored-by: Ariella Vu <[email protected]>
  • Loading branch information
vinistevam and digiwand authored Oct 8, 2024
1 parent bff1e21 commit 44aa027
Show file tree
Hide file tree
Showing 26 changed files with 170 additions and 176 deletions.
6 changes: 0 additions & 6 deletions app/_locales/de/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/el/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/en_GB/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/es/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/fr/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/hi/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/id/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/ja/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/ko/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/pt/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/ru/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/tl/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/tr/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/vi/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions app/_locales/zh_CN/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions ui/components/app/alert-system/alert-modal/alert-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,9 @@ function AlertDetails({
<Box
key={selectedAlert.key}
display={Display.InlineBlock}
padding={2}
padding={customDetails ? 0 : 2}
width={BlockSize.Full}
backgroundColor={customDetails ? undefined : severityStyle.background}
gap={2}
borderRadius={BorderRadius.SM}
>
{customDetails ?? (
Expand Down Expand Up @@ -209,12 +208,11 @@ export function AcknowledgeCheckboxBase({
return (
<Box
display={Display.Flex}
padding={3}
padding={4}
width={BlockSize.Full}
gap={3}
backgroundColor={severityStyle.background}
marginTop={4}
borderRadius={BorderRadius.LG}
marginTop={4}
>
<Checkbox
label={label ?? t('alertModalAcknowledge')}
Expand Down Expand Up @@ -375,6 +373,7 @@ export function AlertModal({
display={Display.Flex}
flexDirection={FlexDirection.Column}
gap={4}
paddingTop={2}
width={BlockSize.Full}
>
{customAcknowledgeButton ?? (
Expand Down
2 changes: 0 additions & 2 deletions ui/components/app/alert-system/alert-modal/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,5 @@

&__acknowledge-checkbox {
@include design-system.H6;

padding-top: 2px;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ function ConfirmDetails({
{t('confirmationAlertModalDetails')}
</Text>
<ButtonLink
paddingTop={5}
paddingBottom={5}
marginTop={4}
size={ButtonLinkSize.Inherit}
textProps={{
variant: TextVariant.bodyMd,
Expand All @@ -103,11 +102,7 @@ function ConfirmDetails({
rel="noopener noreferrer"
data-testid="confirm-alert-modal-review-all-alerts"
>
<Icon
name={IconName.SecuritySearch}
size={IconSize.Inherit}
marginLeft={1}
/>
<Icon name={IconName.SecuritySearch} size={IconSize.Inherit} />
{t('alertModalReviewAllAlerts')}
</ButtonLink>
</Box>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export type GeneralAlertProps = {
provider?: SecurityProvider;
reportUrl?: string;
severity: AlertSeverity;
title: string;
title?: string;
};

function ReportLink({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { fireEvent } from '@testing-library/react';
import { Severity } from '../../../../helpers/constants/design-system';
import { renderWithProvider } from '../../../../../test/lib/render-helpers';
import mockState from '../../../../../test/data/mock-state.json';
import * as useAlertsModule from '../../../../hooks/useAlerts';
import {
MultipleAlertModal,
MultipleAlertModalProps,
Expand Down Expand Up @@ -84,6 +85,56 @@ describe('MultipleAlertModal', () => {
},
});

it('defaults to the first alert if the selected alert is not found', async () => {
const setAlertConfirmedMock = jest.fn();
const useAlertsSpy = jest.spyOn(useAlertsModule, 'default');
const dangerAlertMock = alertsMock.find(
(alert) => alert.key === DATA_ALERT_KEY_MOCK,
);
(useAlertsSpy as jest.Mock).mockReturnValue({
setAlertConfirmed: setAlertConfirmedMock,
alerts: alertsMock,
generalAlerts: [],
fieldAlerts: alertsMock,
getFieldAlerts: () => alertsMock,
isAlertConfirmed: () => false,
});

const { getByText, queryByText, rerender } = renderWithProvider(
<MultipleAlertModal
{...defaultProps}
alertKey={CONTRACT_ALERT_KEY_MOCK}
/>,
mockStore,
);

// shows the contract alert
expect(getByText(alertsMock[2].message)).toBeInTheDocument();

// Update the mock to return only the data alert
(useAlertsSpy as jest.Mock).mockReturnValue({
setAlertConfirmed: setAlertConfirmedMock,
alerts: [dangerAlertMock],
generalAlerts: [],
fieldAlerts: [dangerAlertMock],
getFieldAlerts: () => [dangerAlertMock],
isAlertConfirmed: () => false,
});

// Rerender the component to apply the updated mock
rerender(
<MultipleAlertModal
{...defaultProps}
alertKey={CONTRACT_ALERT_KEY_MOCK}
/>,
);

// verifies the data alert is shown
expect(queryByText(alertsMock[0].message)).not.toBeInTheDocument();
expect(getByText(alertsMock[1].message)).toBeInTheDocument();
useAlertsSpy.mockRestore();
});

it('renders the multiple alert modal', () => {
const { getByTestId } = renderWithProvider(
<MultipleAlertModal {...defaultProps} />,
Expand All @@ -107,7 +158,7 @@ describe('MultipleAlertModal', () => {
expect(onAcknowledgeClickMock).toHaveBeenCalledTimes(1);
});

it('render the next alert when the "Got it" button is clicked', () => {
it('renders the next alert when the "Got it" button is clicked', () => {
const { getByTestId, getByText } = renderWithProvider(
<MultipleAlertModal {...defaultProps} alertKey={DATA_ALERT_KEY_MOCK} />,
mockStoreAcknowledgeAlerts,
Expand All @@ -134,6 +185,20 @@ describe('MultipleAlertModal', () => {
expect(onAcknowledgeClickMock).toHaveBeenCalledTimes(1);
});

it('resets to the first alert if there are unconfirmed alerts and the final alert is acknowledged', () => {
const { getByTestId, getByText } = renderWithProvider(
<MultipleAlertModal
{...defaultProps}
alertKey={CONTRACT_ALERT_KEY_MOCK}
/>,
mockStore,
);

fireEvent.click(getByTestId('alert-modal-button'));

expect(getByText(alertsMock[0].message)).toBeInTheDocument();
});

describe('Navigation', () => {
it('calls next alert when the next button is clicked', () => {
const { getByTestId, getByText } = renderWithProvider(
Expand All @@ -144,6 +209,7 @@ describe('MultipleAlertModal', () => {
fireEvent.click(getByTestId('alert-modal-next-button'));

expect(getByText(alertsMock[2].message)).toBeInTheDocument();
expect(getByText(alertsMock[2].message)).toBeInTheDocument();
});

it('calls previous alert when the previous button is clicked', () => {
Expand Down
Loading

0 comments on commit 44aa027

Please sign in to comment.