From 6ed72f0159f839dd0d76a28308c745bd79232b80 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 21 Aug 2024 00:38:47 +0800 Subject: [PATCH] fix: name being out of sync in account list (#26542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the issue where the account name being created in the connect account flow is out of sync. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26542?quickstart=1) ## **Related issues** Fixes: [25846](https://github.com/MetaMask/metamask-extension/issues/25846) ## **Manual testing steps** 1. Create a snap/hw account 2. Go to test dapp 3. Click connect 4. Click on create new account 5. See that the suggested name is Account 3 6. Create the new account 7. See in the list that there is `Account 3` in the list ## **Screenshots/Recordings** ### **Before** https://github.com/user-attachments/assets/9c0d511a-a84a-4da8-80e5-4dbadbb7cee6 ### **After** https://github.com/user-attachments/assets/daf22aed-593f-4e81-8535-11a26cd5e4fc ## **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. --- .../new-account-modal.component.js | 4 +- .../new-account-modal.container.js | 29 ++++--- .../new-account-modal.test.tsx | 87 +++++++++++++++++++ .../permissions-connect.test.tsx | 43 +++++++-- 4 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 ui/components/app/modals/new-account-modal/new-account-modal.test.tsx diff --git a/ui/components/app/modals/new-account-modal/new-account-modal.component.js b/ui/components/app/modals/new-account-modal/new-account-modal.component.js index 72290c538afd..692ee4d5128e 100644 --- a/ui/components/app/modals/new-account-modal/new-account-modal.component.js +++ b/ui/components/app/modals/new-account-modal/new-account-modal.component.js @@ -26,8 +26,8 @@ export default class NewAccountModal extends Component { }); }; - onSubmit = () => { - this.props.onSave(this.state.alias).then(this.props.hideModal); + onSubmit = async () => { + await this.props.onSave(this.state.alias).then(this.props.hideModal); }; onKeyPress = (e) => { diff --git a/ui/components/app/modals/new-account-modal/new-account-modal.container.js b/ui/components/app/modals/new-account-modal/new-account-modal.container.js index 2a1ba3175e80..13af5f15868b 100644 --- a/ui/components/app/modals/new-account-modal/new-account-modal.container.js +++ b/ui/components/app/modals/new-account-modal/new-account-modal.container.js @@ -1,5 +1,10 @@ import { connect } from 'react-redux'; -import * as actions from '../../../../store/actions'; +import { + addNewAccount, + setAccountLabel, + forceUpdateMetamaskState, + hideModal, +} from '../../../../store/actions'; import NewAccountModal from './new-account-modal.component'; function mapStateToProps(state) { @@ -10,14 +15,14 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch) { return { - hideModal: () => dispatch(actions.hideModal()), - createAccount: (newAccountName) => { - return dispatch(actions.addNewAccount()).then((newAccountAddress) => { - if (newAccountName) { - dispatch(actions.setAccountLabel(newAccountAddress, newAccountName)); - } - return newAccountAddress; - }); + hideModal: () => dispatch(hideModal()), + createAccount: async (newAccountName) => { + const newAccountAddress = await dispatch(addNewAccount()); + if (newAccountName) { + dispatch(setAccountLabel(newAccountAddress, newAccountName)); + } + await forceUpdateMetamaskState(dispatch); + return newAccountAddress; }, }; } @@ -30,9 +35,9 @@ function mergeProps(stateProps, dispatchProps) { ...stateProps, ...dispatchProps, onSave: (newAccountName) => { - return createAccount(newAccountName).then((newAccountAddress) => - onCreateNewAccount(newAccountAddress), - ); + return createAccount(newAccountName).then((newAccountAddress) => { + onCreateNewAccount(newAccountAddress); + }); }, }; } diff --git a/ui/components/app/modals/new-account-modal/new-account-modal.test.tsx b/ui/components/app/modals/new-account-modal/new-account-modal.test.tsx new file mode 100644 index 000000000000..4c3fdf8de3e2 --- /dev/null +++ b/ui/components/app/modals/new-account-modal/new-account-modal.test.tsx @@ -0,0 +1,87 @@ +import React from 'react'; +import thunk from 'redux-thunk'; +import { waitFor, fireEvent } from '@testing-library/react'; +import configureStore from 'redux-mock-store'; +import { renderWithProvider } from '../../../../../test/jest'; +import mockState from '../../../../../test/data/mock-state.json'; +import messages from '../../../../../app/_locales/en/messages.json'; +import NewAccountModal from './new-account-modal.container'; + +const mockOnCreateNewAccount = jest.fn(); +const mockNewAccountNumber = 2; +const mockNewMetamaskState = { + ...mockState.metamask, + currentLocale: 'en', +}; +const mockAddress = '0x1234567890'; + +const mockSubmitRequestToBackground = jest.fn().mockImplementation((method) => { + switch (method) { + case 'addNewAccount': + return mockAddress; + case 'setAccountLabel': + return {}; + case 'getState': + return mockNewMetamaskState; + default: + return {}; + } +}); + +jest.mock('../../../../store/background-connection', () => ({ + ...jest.requireActual('../../../../store/background-connection'), + submitRequestToBackground: (method: string, args: unknown) => + mockSubmitRequestToBackground(method, args), +})); + +const renderModal = ( + props = { + onCreateNewAccount: mockOnCreateNewAccount, + newAccountNumber: mockNewAccountNumber, + }, +) => { + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + currentLocale: 'en', + }, + appState: { + ...mockState.appState, + modal: { + ...mockState.appState.modal, + modalState: { + name: 'NEW_ACCOUNT', + props, + }, + }, + }, + }; + const middlewares = [thunk]; + const mockStore = configureStore(middlewares); + const store = mockStore(state); + + return { + render: renderWithProvider(, store), + store, + }; +}; + +describe('NewAccountModal', () => { + it('calls forceUpdateMetamaskState after adding account', async () => { + const { render } = renderModal(); + const { getByText } = render; + const addAccountButton = getByText(messages.save.message); + expect(addAccountButton).toBeInTheDocument(); + + fireEvent.click(addAccountButton); + + await waitFor(() => { + expect(mockSubmitRequestToBackground).toHaveBeenNthCalledWith( + 2, + 'getState', + undefined, + ); + }); + }); +}); diff --git a/ui/pages/permissions-connect/permissions-connect.test.tsx b/ui/pages/permissions-connect/permissions-connect.test.tsx index fe78ab2d1933..aaabd7621d98 100644 --- a/ui/pages/permissions-connect/permissions-connect.test.tsx +++ b/ui/pages/permissions-connect/permissions-connect.test.tsx @@ -4,6 +4,7 @@ import thunk from 'redux-thunk'; import { ApprovalType } from '@metamask/controller-utils'; import { BtcAccountType } from '@metamask/keyring-api'; +import { fireEvent } from '@testing-library/react'; import messages from '../../../app/_locales/en/messages.json'; import { renderWithProvider } from '../../../test/lib/render-helpers'; import mockState from '../../../test/data/mock-state.json'; @@ -16,6 +17,7 @@ const mockPermissionRequestId = '0cbc1f26-8772-4512-8ad7-f547d6e8b72c'; jest.mock('../../store/actions', () => { return { + ...jest.requireActual('../../store/actions'), getRequestAccountTabIds: jest.fn().mockReturnValue({ type: 'SET_REQUEST_ACCOUNT_TABS', payload: {}, @@ -110,24 +112,31 @@ const render = ( const mockStore = configureStore(middlewares); const store = mockStore(state); - return renderWithProvider( - , + return { + render: renderWithProvider( + , + store, + `${CONNECT_ROUTE}/${mockPermissionRequestId}`, + ), store, - `${CONNECT_ROUTE}/${mockPermissionRequestId}`, - ); + }; }; describe('PermissionApprovalContainer', () => { describe('ConnectPath', () => { it('renders correctly', () => { - const { container, getByText } = render(); + const { + render: { container, getByText }, + } = render(); expect(getByText(messages.next.message)).toBeInTheDocument(); expect(getByText(messages.cancel.message)).toBeInTheDocument(); expect(container).toMatchSnapshot(); }); it('renders the list without BTC accounts', async () => { - const { getByText, queryByText } = render(); + const { + render: { getByText, queryByText }, + } = render(); expect( getByText( `${mockAccount.metadata.name} (${shortenAddress( @@ -144,4 +153,26 @@ describe('PermissionApprovalContainer', () => { ).not.toBeInTheDocument(); }); }); + + describe('Add new account', () => { + it('displays the correct account number', async () => { + const { + render: { getByText }, + store, + } = render(); + fireEvent.click(getByText(messages.newAccount.message)); + + const dispatchedActions = store.getActions(); + + expect(dispatchedActions).toHaveLength(2); // first action is 'SET_REQUEST_ACCOUNT_TABS' + expect(dispatchedActions[1]).toStrictEqual({ + type: 'UI_MODAL_OPEN', + payload: { + name: 'NEW_ACCOUNT', + onCreateNewAccount: expect.any(Function), + newAccountNumber: 2, + }, + }); + }); + }); });