Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: MMI adds note to trader support to the new Tx confirmation view #27214

Merged
merged 22 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/scripts/controllers/app-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,12 @@ export default class AppStateController extends EventEmitter {
});
}

setNoteToTraderMessage(message) {
this.store.updateState({
noteToTraderMessage: message,
});
}

///: END:ONLY_INCLUDE_IF

getSignatureSecurityAlertResponse(securityAlertId) {
Expand Down
19 changes: 19 additions & 0 deletions app/scripts/controllers/app-state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,5 +374,24 @@ describe('AppStateController', () => {

updateStateSpy.mockRestore();
});

it('set the setNoteToTraderMessage with a message', () => {
appStateController = createAppStateController();
const updateStateSpy = jest.spyOn(
appStateController.store,
'updateState',
);

const mockParams = 'some message';

appStateController.setNoteToTraderMessage(mockParams);

expect(updateStateSpy).toHaveBeenCalledTimes(1);
expect(updateStateSpy).toHaveBeenCalledWith({
noteToTraderMessage: mockParams,
});

updateStateSpy.mockRestore();
});
});
});
2 changes: 2 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -3614,6 +3614,8 @@ export default class MetamaskController extends EventEmitter {
),
setCustodianDeepLink:
appStateController.setCustodianDeepLink.bind(appStateController),
setNoteToTraderMessage:
appStateController.setNoteToTraderMessage.bind(appStateController),
///: END:ONLY_INCLUDE_IF

// snaps
Expand Down
9 changes: 4 additions & 5 deletions test/e2e/playwright/mmi/specs/extension.visual.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,12 @@ test.describe('MMI extension', () => {
await accountsPopup.connectCustodian(
process.env.MMI_E2E_CUSTODIAN_NAME as string,
);

const accountNamesWithCustodian = await accountsPopup.getAccountNames();

expect(
JSON.stringify(accountNamesWithCustodian) ===
JSON.stringify(arrayWithCustodianAccounts),
).toBeTruthy();
const containsAccount = arrayWithCustodianAccounts.some((account) =>
accountNamesWithCustodian.includes(account),
);
expect(containsAccount).toBeTruthy();

await accountsPopup.selectCustodyAccount(accountFrom);
// Check remove custodian token screen (aborted before removed)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`NoteToTrader should render the Note to trader component 1`] = `
<div>
<div
class="mm-box confirm-page-container-content__data"
class="mm-box mm-box--margin-bottom-4 mm-box--padding-0 mm-box--background-color-background-default mm-box--rounded-md"
>
<div
class="mm-box mm-box--padding-4 mm-box--display-flex mm-box--flex-direction-column"
Expand All @@ -20,7 +20,7 @@ exports[`NoteToTrader should render the Note to trader component 1`] = `
<p
class="mm-box mm-text note-header__counter mm-text--body-md mm-box--color-text-default"
>
9
0
/
280
</p>
Expand All @@ -29,13 +29,13 @@ exports[`NoteToTrader should render the Note to trader component 1`] = `
class="mm-box note-field mm-box--display-flex mm-box--flex-direction-column"
>
<textarea
class="mm-box mm-text mm-textarea mm-textarea--resize-vertical mm-text--body-md mm-box--padding-2 mm-box--padding-top-1 mm-box--padding-right-4 mm-box--padding-bottom-1 mm-box--padding-left-4 mm-box--width-full mm-box--height-full mm-box--color-text-default mm-box--background-color-background-default mm-box--rounded-sm mm-box--border-color-border-default mm-box--border-width-1 box--border-style-solid"
data-testid="transaction-note"
id="transaction-note"
maxlength="280"
placeholder=""
>
some text
</textarea>
placeholder="The approver will see this note when approving the transaction at the custodian."
resize="vertical"
/>
</div>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import React from 'react';
import {
Display,
FlexDirection,
JustifyContent,
} from '../../../helpers/constants/design-system';
import { Label, Box, Text } from '../../component-library';

type NoteToTraderProps = {
placeholder: string;
maxLength: number;
onChange: (value: string) => void;
noteText: string;
labelText: string;
};

const TabbedNoteToTrader: React.FC<NoteToTraderProps> = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be removed towards the end of the year.

placeholder,
maxLength,
onChange,
noteText,
labelText,
}) => {
return (
<Box className="confirm-page-container-content__data">
<Box
display={Display.Flex}
flexDirection={FlexDirection.Column}
padding={4}
>
<Box
className="note-header"
display={Display.Flex}
justifyContent={JustifyContent.spaceBetween}
>
<Label htmlFor="transaction-note">{labelText}</Label>
<Text className="note-header__counter">
{noteText.length}/{maxLength}
</Text>
</Box>
<Box
display={Display.Flex}
flexDirection={FlexDirection.Column}
className="note-field"
>
<textarea
id="transaction-note"
data-testid="transaction-note"
onChange={({ target: { value } }) => onChange(value)}
autoFocus
maxLength={maxLength}
placeholder={placeholder}
value={noteText}
/>
</Box>
</Box>
</Box>
);
};

export default TabbedNoteToTrader;
Original file line number Diff line number Diff line change
@@ -1,9 +1,23 @@
import React from 'react';
import { Meta } from '@storybook/react';
import { Provider } from 'react-redux';
import NoteToTrader from '.';
import { getMockContractInteractionConfirmState } from '../../../../test/data/confirmations/helper';
import configureStore from '../../../store/store';
import { ConfirmContextProvider } from '../../../pages/confirmations/context/confirm';

const store = configureStore(getMockContractInteractionConfirmState());

export default {
title: 'Components/Institutional/NoteToTrader',
component: NoteToTrader,
decorators: [
(story: () => Meta<typeof NoteToTrader>) => (
<Provider store={store}>
<ConfirmContextProvider>{story()}</ConfirmContextProvider>
</Provider>
),
],
args: {
placeholder:
'The approver will see this note when approving the transaction at the custodian.',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,31 @@
import { render, fireEvent } from '@testing-library/react';
import { fireEvent } from '@testing-library/react';
import React from 'react';
import configureMockStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { renderWithConfirmContextProvider } from '../../../../test/lib/confirmations/render-helpers';
import mockState from '../../../../test/data/mock-state.json';
import NoteToTrader from './note-to-trader';

jest.mock('../../../selectors/institutional/selectors', () => ({
getIsNoteToTraderSupported: () => true,
}));

const middleware = [thunk];
const store = configureMockStore(middleware)(mockState);

describe('NoteToTrader', () => {
it('should render the Note to trader component', () => {
const props = {
placeholder: '',
maxLength: 280,
noteText: 'some text',
labelText: 'Transaction note',
onChange: jest.fn(),
};
const { getByTestId, container } = renderWithConfirmContextProvider(
<NoteToTrader />,
store,
);

const { getByTestId, container } = render(<NoteToTrader {...props} />);
const transactionNoteInput = getByTestId(
'transaction-note',
) as HTMLInputElement;

fireEvent.change(transactionNoteInput);
expect(transactionNoteInput.value).toBe('some text');
expect(transactionNoteInput.value).toBe('');
expect(transactionNoteInput).toBeDefined();
expect(container).toMatchSnapshot();
});
Expand Down
78 changes: 53 additions & 25 deletions ui/components/institutional/note-to-trader/note-to-trader.tsx
Original file line number Diff line number Diff line change
@@ -1,28 +1,54 @@
import React from 'react';
import React, { useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import {
BackgroundColor,
BlockSize,
BorderRadius,
Display,
FlexDirection,
JustifyContent,
} from '../../../helpers/constants/design-system';
import { Label, Box, Text } from '../../component-library';
import { Textarea } from '../../component-library/textarea';
import { setNoteToTraderMessage } from '../../../store/institutional/institution-background';
import {
getIsNoteToTraderSupported,
State,
} from '../../../selectors/institutional/selectors';
import { toChecksumHexAddress } from '../../../../shared/modules/hexstring-utils';
import { useConfirmContext } from '../../../pages/confirmations/context/confirm';
import { getConfirmationSender } from '../../../pages/confirmations/components/confirm/utils';
import { useI18nContext } from '../../../hooks/useI18nContext';
import { isSignatureTransactionType } from '../../../pages/confirmations/utils';

type NoteToTraderProps = {
placeholder: string;
maxLength: number;
onChange: (value: string) => void;
noteText: string;
labelText: string;
};
const NoteToTrader: React.FC = () => {
const dispatch = useDispatch();
const t = useI18nContext();
const [noteText, setNoteText] = useState('');

const { currentConfirmation } = useConfirmContext();
const isSignature = isSignatureTransactionType(currentConfirmation);
const { from } = getConfirmationSender(currentConfirmation);
const fromChecksumHexAddress = toChecksumHexAddress(from || '');
const isNoteToTraderSupported = useSelector((state: State) =>
getIsNoteToTraderSupported(state, fromChecksumHexAddress),
);

const NoteToTrader: React.FC<NoteToTraderProps> = ({
placeholder,
maxLength,
onChange,
noteText,
labelText,
}) => {
return (
<Box className="confirm-page-container-content__data">
useEffect(() => {
const timer = setTimeout(() => {
dispatch(setNoteToTraderMessage(noteText));
}, 700);

return () => clearTimeout(timer);
}, [noteText]);

return isNoteToTraderSupported && !isSignature ? (
<Box
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit]: Can instead be expressed with an early return which is arguably more readable:

if (!isNotToTraderSupported || isSignature) {
  return null
}

backgroundColor={BackgroundColor.backgroundDefault}
borderRadius={BorderRadius.MD}
padding={0}
marginBottom={4}
>
<Box
display={Display.Flex}
flexDirection={FlexDirection.Column}
Expand All @@ -33,29 +59,31 @@ const NoteToTrader: React.FC<NoteToTraderProps> = ({
display={Display.Flex}
justifyContent={JustifyContent.spaceBetween}
>
<Label htmlFor="transaction-note">{labelText}</Label>
<Label htmlFor="transaction-note">{t('transactionNote')}</Label>
<Text className="note-header__counter">
{noteText.length}/{maxLength}
{noteText.length}/{280}
</Text>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the intended behaviour with this, but do we want to do {noteText.length}/280 or {noteText.length}/{MAX_LENGTH} with const MAX_LENGTH = 280?

Copy link
Contributor Author

@zone-live zone-live Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 280 is the max length. I'll assign it to a const in a next PR I have open 👍🏼

</Box>
<Box
display={Display.Flex}
flexDirection={FlexDirection.Column}
className="note-field"
>
<textarea
<Textarea
id="transaction-note"
data-testid="transaction-note"
onChange={({ target: { value } }) => onChange(value)}
autoFocus
maxLength={maxLength}
placeholder={placeholder}
onChange={({ target: { value } }) => setNoteText(value)}
value={noteText}
height={BlockSize.Full}
width={BlockSize.Full}
maxLength={280}
placeholder={t('notePlaceholder')}
padding={2}
/>
</Box>
</Box>
</Box>
);
) : null;
};

export default NoteToTrader;
6 changes: 4 additions & 2 deletions ui/hooks/useMMIConfirmations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ export function useMMIConfirmations() {
currentConfirmation.type === TransactionType.signTypedData) &&
Boolean(currentConfirmation?.custodyId),
mmiOnSignCallback: () => custodySignFn(currentConfirmation),
mmiOnTransactionCallback: () =>
custodyTransactionFn(currentConfirmation as TransactionMeta),
mmiOnTransactionCallback: (
transactionData: TransactionMeta,
noteToTraderMessage: string,
) => custodyTransactionFn(transactionData, noteToTraderMessage),
};
}
16 changes: 11 additions & 5 deletions ui/hooks/useMMICustodySendTransaction.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { useMMICustodySendTransaction } from './useMMICustodySendTransaction';

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useHistory: jest.fn(),
useHistory: jest.fn().mockReturnValue({ push: jest.fn() }),
}));

jest.mock('react-redux', () => ({
Expand All @@ -23,14 +23,20 @@ jest.mock('../store/institutional/institution-background', () => ({
mmiActionsFactory: jest.fn(),
}));

jest.mock('../selectors', () => ({
getAccountType: jest.fn(),
}));

jest.mock('../store/actions', () => ({
updateAndApproveTx: jest.fn(),
}));

jest.mock('../pages/confirmations/context/confirm', () => ({
useConfirmContext: () => ({
currentConfirmation: { from: '0x123' },
}),
}));

jest.mock('../pages/confirmations/components/confirm/utils', () => ({
getConfirmationSender: () => ({ from: '0x123' }),
}));

describe('useMMICustodySendTransaction', () => {
it('handles custody account type', async () => {
const dispatch = jest.fn();
Expand Down
Loading
Loading