Skip to content

Commit

Permalink
feat: Add metrics for alerts (transactions redesign) (#26121)
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**

Recently the Alert System was introduced to redesigned confirmations
with this the alerts for those confirmations were migrated to use the
new Alert System. This PR introduces infrastructure to track metrics for
alerts in the redesigned confirmation system. It ensures that alert
metrics are captured and included in the necessary places for tracking.


The use of a context to manage shared state for alert metrics within the
confirmation flow. The context approach was chosen because it allows us
to maintain a single instance of the shared state per confirmation. This
is crucial for ensuring that clicks on different inline alerts and
actions are persisted and aggregated into a unified set of properties.
These aggregated properties are then added to the transaction event,
providing a comprehensive view of user interactions within the
confirmation process.

<!--
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/26121?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2709

## **Manual testing steps**

1. Go to the test dapp
2. in the session `PPOM - Malicious Transactions and Signatures` > mint
ERC20

## **Screenshots/Recordings**

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


[confirm.webm](https://github.com/user-attachments/assets/43d88b5a-c10e-472f-8603-83a344ca71ca)


[reject.webm](https://github.com/user-attachments/assets/ce13cb90-89f7-43d7-8336-281ab784f4eb)

### **Before**

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

### **After**

<!-- [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: Matthew Walsh <[email protected]>
  • Loading branch information
vinistevam and matthewwalsh0 authored Aug 12, 2024
1 parent 91dc6ea commit bc6539b
Show file tree
Hide file tree
Showing 26 changed files with 629 additions and 36 deletions.
17 changes: 10 additions & 7 deletions .storybook/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { metamaskStorybookTheme } from './metamask-storybook-theme';
import { DocsContainer } from '@storybook/addon-docs';
import { useDarkMode } from 'storybook-dark-mode';
import { themes } from '@storybook/theming';
import { AlertMetricsProvider } from '../ui/components/app/alert-system/contexts/alertMetricsContext';

// eslint-disable-next-line
/* @ts-expect-error: Avoids error from window property not existing */
Expand Down Expand Up @@ -124,13 +125,15 @@ const metamaskDecorator = (story, context) => {
<Provider store={store}>
<Router history={history}>
<MetaMetricsProviderStorybook>
<I18nProvider
currentLocale={currentLocale}
current={current}
en={allLocales.en}
>
<LegacyI18nProvider>{story()}</LegacyI18nProvider>
</I18nProvider>
<AlertMetricsProvider>
<I18nProvider
currentLocale={currentLocale}
current={current}
en={allLocales.en}
>
<LegacyI18nProvider>{story()}</LegacyI18nProvider>
</I18nProvider>
</AlertMetricsProvider>
</MetaMetricsProviderStorybook>
</Router>
</Provider>
Expand Down
47 changes: 47 additions & 0 deletions ui/components/app/alert-system/alert-modal/alert-modal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ jest.mock('../contexts/alertActionHandler', () => ({
useAlertActionHandler: jest.fn(() => mockAlertActionHandlerProviderValue),
}));

const mockTrackAlertActionClicked = jest.fn();
const mockTrackAlertRender = jest.fn();
jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackAlertActionClicked: mockTrackAlertActionClicked,
trackInlineAlertClicked: jest.fn(),
trackAlertRender: mockTrackAlertRender,
})),
}));

describe('AlertModal', () => {
const OWNER_ID_MOCK = '123';
const FROM_ALERT_KEY_MOCK = 'from';
Expand Down Expand Up @@ -250,4 +260,41 @@ describe('AlertModal', () => {
expect(getByTestId('alert-modal-button')).toBeDefined();
});
});

describe('Track alert metrics', () => {
it('calls mockTrackAlertRender when alert modal is opened', () => {
const { getByText } = renderWithProvider(
<AlertModal
ownerId={OWNER_ID_MOCK}
onAcknowledgeClick={onAcknowledgeClickMock}
onClose={onCloseMock}
alertKey={FROM_ALERT_KEY_MOCK}
/>,
mockStore,
);

expect(getByText(ALERT_MESSAGE_MOCK)).toBeInTheDocument();
expect(mockTrackAlertRender).toHaveBeenCalledWith(FROM_ALERT_KEY_MOCK);
});

it('calls trackAlertActionClicked when action button is clicked', () => {
const { getByText } = renderWithProvider(
<AlertModal
ownerId={OWNER_ID_MOCK}
onAcknowledgeClick={onAcknowledgeClickMock}
onClose={onCloseMock}
alertKey={CONTRACT_ALERT_KEY_MOCK}
/>,
mockStore,
);

expect(getByText(ACTION_LABEL_MOCK)).toBeInTheDocument();

fireEvent.click(getByText(ACTION_LABEL_MOCK));

expect(mockTrackAlertActionClicked).toHaveBeenCalledWith(
CONTRACT_ALERT_KEY_MOCK,
);
});
});
});
19 changes: 16 additions & 3 deletions ui/components/app/alert-system/alert-modal/alert-modal.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback } from 'react';
import React, { useCallback, useEffect } from 'react';
import { ButtonVariant } from '@metamask/snaps-sdk';

import { SecurityProvider } from '../../../../../shared/constants/security-provider';
Expand Down Expand Up @@ -36,6 +36,7 @@ import useAlerts from '../../../../hooks/useAlerts';
import { Alert } from '../../../../ducks/confirm-alerts/confirm-alerts';
import { useAlertActionHandler } from '../contexts/alertActionHandler';
import { AlertProvider } from '../alert-provider';
import { useAlertMetrics } from '../contexts/alertMetricsContext';

export type AlertModalProps = {
/**
Expand Down Expand Up @@ -257,20 +258,24 @@ function AcknowledgeButton({
function ActionButton({
action,
onClose,
alertKey,
}: {
action?: { key: string; label: string };
onClose: (request: { recursive?: boolean } | void) => void;
alertKey: string;
}) {
const { processAction } = useAlertActionHandler();
const { trackAlertActionClicked } = useAlertMetrics();

const handleClick = useCallback(() => {
if (!action) {
return;
}
trackAlertActionClicked(alertKey);

processAction(action.key);
onClose({ recursive: true });
}, [action, onClose, processAction]);
}, [action, onClose, processAction, trackAlertActionClicked, alertKey]);

if (!action) {
return null;
Expand Down Expand Up @@ -304,6 +309,7 @@ export function AlertModal({
enableProvider = true,
}: AlertModalProps) {
const { isAlertConfirmed, setAlertConfirmed, alerts } = useAlerts(ownerId);
const { trackAlertRender } = useAlertMetrics();

const handleClose = useCallback(
(...args) => {
Expand All @@ -314,6 +320,12 @@ export function AlertModal({

const selectedAlert = alerts.find((alert: Alert) => alert.key === alertKey);

useEffect(() => {
if (selectedAlert) {
trackAlertRender(selectedAlert.key);
}
}, [selectedAlert, trackAlertRender]);

if (!selectedAlert) {
return null;
}
Expand All @@ -322,7 +334,7 @@ export function AlertModal({

const handleCheckboxClick = useCallback(() => {
return setAlertConfirmed(selectedAlert.key, !isConfirmed);
}, [isConfirmed, selectedAlert.key]);
}, [isConfirmed, selectedAlert.key, setAlertConfirmed]);

return (
<Modal isOpen onClose={handleClose}>
Expand Down Expand Up @@ -380,6 +392,7 @@ export function AlertModal({
key={action.key}
action={action}
onClose={handleClose}
alertKey={selectedAlert.key}
/>
),
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ export const TemplateStory: StoryFn<typeof ConfirmAlertModal> = (args) => {
{isOpen && (
<ConfirmAlertModal
{...args}
alertKey={'From'}
onClose={handleOnClose}
onCancel={handleOnClose}
onSubmit={handleOnClose}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ import {
ConfirmAlertModal,
} from './confirm-alert-modal';

jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackInlineAlertClicked: jest.fn(),
trackAlertRender: jest.fn(),
trackAlertActionClicked: jest.fn(),
})),
}));

describe('ConfirmAlertModal', () => {
const OWNER_ID_MOCK = '123';
const FROM_ALERT_KEY_MOCK = 'from';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React from 'react';
import { renderHookWithProvider } from '../../../../../test/lib/render-helpers';
import { useAlertMetrics } from './alertMetricsContext';

jest.mock('react', () => ({
...jest.requireActual('react'),
useContext: jest.fn(),
}));

describe('useAlertMetrics', () => {
beforeEach(() => {
jest.resetAllMocks();
});

it('provides trackAlertActionClicked, trackAlertRender, and trackInlineAlertClicked functions from context', () => {
(React.useContext as jest.Mock).mockReturnValue({
trackAlertActionClicked: jest.fn(),
trackAlertRender: jest.fn(),
trackInlineAlertClicked: jest.fn(),
});
const ALERT_KEY_MOCK = 'testKey';
const { result } = renderHookWithProvider(useAlertMetrics);

expect(result.current).toBeDefined();
expect(typeof result.current.trackAlertActionClicked).toBe('function');
expect(typeof result.current.trackAlertRender).toBe('function');
expect(typeof result.current.trackInlineAlertClicked).toBe('function');

expect(() =>
result.current.trackAlertActionClicked(ALERT_KEY_MOCK),
).not.toThrow();
expect(() => result.current.trackAlertRender(ALERT_KEY_MOCK)).not.toThrow();
expect(() =>
result.current.trackInlineAlertClicked(ALERT_KEY_MOCK),
).not.toThrow();
});

it('throws an error if used outside of AlertMetricsProvider', () => {
const { result } = renderHookWithProvider(() => useAlertMetrics());
expect(result.error).toEqual(
new Error('useAlertMetrics must be used within an AlertMetricsProvider'),
);
});
});
40 changes: 40 additions & 0 deletions ui/components/app/alert-system/contexts/alertMetricsContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React, { createContext, useContext, ReactNode, useMemo } from 'react';
import { useConfirmationAlertMetrics } from '../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';

const AlertMetricsContext = createContext<{
trackAlertActionClicked: (alertKey: string) => void;
trackAlertRender: (alertKey: string) => void;
trackInlineAlertClicked: (alertKey: string) => void;
} | null>(null);

export const AlertMetricsProvider: React.FC<{ children: ReactNode }> = ({
children,
}) => {
const { trackAlertActionClicked, trackAlertRender, trackInlineAlertClicked } =
useConfirmationAlertMetrics();

const value = useMemo(
() => ({
trackAlertActionClicked,
trackAlertRender,
trackInlineAlertClicked,
}),
[trackAlertActionClicked, trackAlertRender, trackInlineAlertClicked],
);

return (
<AlertMetricsContext.Provider value={value}>
{children}
</AlertMetricsContext.Provider>
);
};

export const useAlertMetrics = () => {
const context = useContext(AlertMetricsContext);
if (!context) {
throw new Error(
'useAlertMetrics must be used within an AlertMetricsProvider',
);
}
return context;
};
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ import {
MultipleAlertModalProps,
} from './multiple-alert-modal';

jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackInlineAlertClicked: jest.fn(),
trackAlertRender: jest.fn(),
trackAlertActionClicked: jest.fn(),
})),
}));

describe('MultipleAlertModal', () => {
const OWNER_ID_MOCK = '123';
const FROM_ALERT_KEY_MOCK = 'from';
Expand Down
22 changes: 22 additions & 0 deletions ui/components/app/confirm/info/row/alert-row/alert-row.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ jest.mock('../../../../alert-system/contexts/alertActionHandler', () => ({
useAlertActionHandler: jest.fn(() => mockAlertActionHandlerProviderValue),
}));

const mockTrackInlineAlertClicked = jest.fn();
jest.mock('../../../../alert-system/contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackInlineAlertClicked: mockTrackInlineAlertClicked,
trackAlertRender: jest.fn(),
trackAlertActionClicked: jest.fn(),
})),
}));

describe('AlertRow', () => {
const OWNER_ID_MOCK = '123';
const OWNER_ID_NO_ALERT_MOCK = '000';
Expand Down Expand Up @@ -121,6 +130,19 @@ describe('AlertRow', () => {
});
});

describe('Track alert metrics', () => {
it('calls trackInlineAlertClicked when inline alert is clicked', () => {
const { getByTestId } = renderAlertRow({
alertKey: KEY_ALERT_KEY_MOCK,
ownerId: OWNER_ID_MOCK,
});
fireEvent.click(getByTestId('inline-alert'));
expect(mockTrackInlineAlertClicked).toHaveBeenCalledWith(
KEY_ALERT_KEY_MOCK,
);
});
});

describe('ProcessAlertAction', () => {
it('renders dynamic action button', () => {
const { getByTestId, getByText } = renderAlertRow({
Expand Down
6 changes: 5 additions & 1 deletion ui/components/app/confirm/info/row/alert-row/alert-row.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from '../row';
import { Box } from '../../../../../component-library';
import { MultipleAlertModal } from '../../../../alert-system/multiple-alert-modal';
import { useAlertMetrics } from '../../../../alert-system/contexts/alertMetricsContext';

export type ConfirmInfoAlertRowProps = ConfirmInfoRowProps & {
alertKey: string;
Expand Down Expand Up @@ -42,10 +43,12 @@ export const ConfirmInfoAlertRow = ({
variant,
...rowProperties
}: ConfirmInfoAlertRowProps) => {
const { trackInlineAlertClicked } = useAlertMetrics();
const { getFieldAlerts } = useAlerts(ownerId);
const fieldAlerts = getFieldAlerts(alertKey);
const hasFieldAlert = fieldAlerts.length > 0;
const selectedAlertSeverity = fieldAlerts[0]?.severity;
const selectedAlertKey = fieldAlerts[0]?.key;

const [alertModalVisible, setAlertModalVisible] = useState<boolean>(false);

Expand All @@ -55,6 +58,7 @@ export const ConfirmInfoAlertRow = ({

const handleInlineAlertClick = () => {
setAlertModalVisible(true);
trackInlineAlertClicked(selectedAlertKey);
};

const confirmInfoRowProps = {
Expand All @@ -77,7 +81,7 @@ export const ConfirmInfoAlertRow = ({
<>
{alertModalVisible && (
<MultipleAlertModal
alertKey={fieldAlerts[0].key}
alertKey={selectedAlertKey}
ownerId={ownerId}
onFinalAcknowledgeClick={handleModalClose}
onClose={handleModalClose}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@ import React, { ReactElement } from 'react';
import { AlertActionHandlerProvider } from '../../../../../components/app/alert-system/contexts/alertActionHandler';
import useConfirmationAlertActions from '../../../hooks/useConfirmationAlertActions';
import setConfirmationAlerts from '../../../hooks/setConfirmationAlerts';
import { AlertMetricsProvider } from '../../../../../components/app/alert-system/contexts/alertMetricsContext';

const ConfirmAlerts = ({ children }: { children: ReactElement }) => {
const processAction = useConfirmationAlertActions();
setConfirmationAlerts();

return (
<AlertActionHandlerProvider onProcessAction={processAction}>
{children}
</AlertActionHandlerProvider>
<AlertMetricsProvider>
<AlertActionHandlerProvider onProcessAction={processAction}>
{children}
</AlertActionHandlerProvider>
</AlertMetricsProvider>
);
};

Expand Down
Loading

0 comments on commit bc6539b

Please sign in to comment.