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

feat: Add metrics for alerts (transactions redesign) #26121

Merged
merged 23 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0835c5a
add alert system metrics
vinistevam Jul 24, 2024
1fbd28b
improve alerts metrics
vinistevam Jul 25, 2024
94f12b3
lint
vinistevam Jul 25, 2024
6906ad0
use alert row to support inline alerts
vinistevam Jul 25, 2024
9a93288
clean up
vinistevam Jul 25, 2024
552754a
clean up
vinistevam Jul 25, 2024
051fd66
use context to save state
vinistevam Jul 25, 2024
c888953
reorg
vinistevam Jul 25, 2024
97c9a12
add unit tests
vinistevam Jul 25, 2024
08f08c3
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Jul 25, 2024
b342751
fix test type
vinistevam Jul 26, 2024
a0e233e
fix sonar, tests, storybook and pipeline
vinistevam Jul 26, 2024
c948f7f
fix unit tests
vinistevam Jul 26, 2024
2609816
fix unit tests
vinistevam Jul 26, 2024
5eea34a
improve coverage
vinistevam Jul 26, 2024
4e65436
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Jul 26, 2024
f4de32e
Update ui/pages/confirmations/hooks/useConfirmationAlertMetrics.ts
vinistevam Jul 28, 2024
32298a7
applied suggestion and small fixes
vinistevam Jul 29, 2024
07e9778
verify the actual contents of the properties
vinistevam Jul 30, 2024
04900f9
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Jul 30, 2024
50fd47d
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Jul 30, 2024
dcb1364
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Aug 1, 2024
aa3aae0
Merge branch 'develop' into feat/redesign-transaction-metrics-alerts
vinistevam Aug 12, 2024
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
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
49 changes: 49 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 @@ -6,6 +6,7 @@ import { renderWithProvider } from '../../../../../test/lib/render-helpers';
import * as useAlertsModule from '../../../../hooks/useAlerts';
import mockState from '../../../../../test/data/mock-state.json';
import { Alert } from '../../../../ducks/confirm-alerts/confirm-alerts';
import { AlertsActionMetrics } from '../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';
import { AlertModal } from './alert-modal';

const onProcessActionMock = jest.fn();
Expand All @@ -18,6 +19,13 @@ jest.mock('../contexts/alertActionHandler', () => ({
useAlertActionHandler: jest.fn(() => mockAlertActionHandlerProviderValue),
}));

const mockTrackAlertMetrics = jest.fn();
jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackAlertMetrics: mockTrackAlertMetrics,
})),
}));

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

describe('Track alert metrics', () => {
it('calls trackAlertMetrics 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(mockTrackAlertMetrics).toHaveBeenCalledWith({
alertKey: FROM_ALERT_KEY_MOCK,
action: AlertsActionMetrics.AlertVisualized,
});
});

it('calls trackAlertMetrics 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(mockTrackAlertMetrics).toHaveBeenCalledWith({
alertKey: CONTRACT_ALERT_KEY_MOCK,
action: AlertsActionMetrics.AlertActionClicked,
});
});
});
});
38 changes: 35 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,11 @@ import useAlerts from '../../../../hooks/useAlerts';
import { Alert } from '../../../../ducks/confirm-alerts/confirm-alerts';
import { useAlertActionHandler } from '../contexts/alertActionHandler';
import { AlertProvider } from '../alert-provider';
import {
AlertsActionMetrics,
UseAlertSystemMetricsProps,
} from '../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';
import { useAlertMetrics } from '../contexts/alertMetricsContext';

export type AlertModalProps = {
/**
Expand Down Expand Up @@ -257,20 +262,33 @@ function AcknowledgeButton({
function ActionButton({
action,
onClose,
metrics,
}: {
action?: { key: string; label: string };
onClose: (request: { recursive?: boolean } | void) => void;
metrics: {
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved
trackAlertMetrics: ({
alertKey,
action,
}: UseAlertSystemMetricsProps) => void;
alertKey: string;
};
}) {
const { processAction } = useAlertActionHandler();
const { trackAlertMetrics, alertKey } = metrics;

const handleClick = useCallback(() => {
if (!action) {
return;
}
trackAlertMetrics({
alertKey,
action: AlertsActionMetrics.AlertActionClicked,
});

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

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

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

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

useEffect(() => {
if (selectedAlert) {
trackAlertMetrics({
alertKey: selectedAlert.key,
action: AlertsActionMetrics.AlertVisualized,
});
}
}, [selectedAlert, trackAlertMetrics]);

if (!selectedAlert) {
return null;
}
Expand All @@ -322,7 +350,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 +408,10 @@ export function AlertModal({
key={action.key}
action={action}
onClose={handleClose}
metrics={{
trackAlertMetrics,
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,12 @@ import {
ConfirmAlertModal,
} from './confirm-alert-modal';

jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackAlertMetrics: 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,42 @@
import React from 'react';
import { renderHookWithProvider } from '../../../../../test/lib/render-helpers';
import {
AlertsActionMetrics,
UseAlertSystemMetricsProps,
} from '../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';
import { useAlertMetrics } from './alertMetricsContext';

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

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

it('provides trackAlertMetrics function from context', () => {
(React.useContext as jest.Mock).mockReturnValue({
trackAlertMetrics: jest.fn(),
});
const { result } = renderHookWithProvider(useAlertMetrics);

expect(result.current).toBeDefined();
expect(result.current.trackAlertMetrics).toBeDefined();
expect(typeof result.current.trackAlertMetrics).toBe('function');

const mockProps: UseAlertSystemMetricsProps = {
alertKey: 'testKey',
action: AlertsActionMetrics.InlineAlertClicked,
};
expect(() => result.current.trackAlertMetrics(mockProps)).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'),
);
});
});
33 changes: 33 additions & 0 deletions ui/components/app/alert-system/contexts/alertMetricsContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import React, { createContext, useContext, ReactNode, useMemo } from 'react';
import {
UseAlertSystemMetricsProps,
useConfirmationAlertMetrics,
} from '../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';

const AlertMetricsContext = createContext<{
trackAlertMetrics: (props?: UseAlertSystemMetricsProps) => void;
} | null>(null);

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

const value = useMemo(() => ({ trackAlertMetrics }), [trackAlertMetrics]);

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

Choose a reason for hiding this comment

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

We use context for shared state, but there is no shared state. Can we avoid creating this new context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize for any confusion. I have now updated the PR description to explain the rationale behind this approach. The context is necessary to aggregate and persist state across multiple events (e.g., user clicks on inline alerts, visualizations, and action clicks) within the alert system. This ensures that all interactions are captured and combined into a single set of properties, which are then added to the transaction event. Without the context, we would not be able to maintain this aggregated state across different components effectively.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since hook useConfirmationAlertMetrics is not used outside the context, may be code can be embedded in context itself.


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,12 @@ import {
MultipleAlertModalProps,
} from './multiple-alert-modal';

jest.mock('../contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackAlertMetrics: 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 @@ -5,6 +5,7 @@ import { Text } from '../../../../../component-library';
import { renderWithProvider } from '../../../../../../../test/lib/render-helpers';
import { Severity } from '../../../../../../helpers/constants/design-system';
import mockState from '../../../../../../../test/data/mock-state.json';
import { AlertsActionMetrics } from '../../../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';
import { ConfirmInfoAlertRow, ConfirmInfoAlertRowProps } from './alert-row';

const onProcessActionMock = jest.fn();
Expand All @@ -17,6 +18,13 @@ jest.mock('../../../../alert-system/contexts/alertActionHandler', () => ({
useAlertActionHandler: jest.fn(() => mockAlertActionHandlerProviderValue),
}));

const mockTrackAlertMetrics = jest.fn();
jest.mock('../../../../alert-system/contexts/alertMetricsContext', () => ({
useAlertMetrics: jest.fn(() => ({
trackAlertMetrics: mockTrackAlertMetrics,
})),
}));

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

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

describe('ProcessAlertAction', () => {
it('renders dynamic action button', () => {
const { getByTestId, getByText } = renderAlertRow({
Expand Down
10 changes: 9 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,8 @@ import {
} from '../row';
import { Box } from '../../../../../component-library';
import { MultipleAlertModal } from '../../../../alert-system/multiple-alert-modal';
import { AlertsActionMetrics } from '../../../../../../pages/confirmations/hooks/useConfirmationAlertMetrics';
import { useAlertMetrics } from '../../../../alert-system/contexts/alertMetricsContext';

export type ConfirmInfoAlertRowProps = ConfirmInfoRowProps & {
alertKey: string;
Expand Down Expand Up @@ -42,10 +44,12 @@ export const ConfirmInfoAlertRow = ({
variant,
...rowProperties
}: ConfirmInfoAlertRowProps) => {
const { trackAlertMetrics } = 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 +59,10 @@ export const ConfirmInfoAlertRow = ({

const handleInlineAlertClick = () => {
setAlertModalVisible(true);
trackAlertMetrics({
alertKey: selectedAlertKey,
action: AlertsActionMetrics.InlineAlertClicked,
});
};

const confirmInfoRowProps = {
Expand All @@ -77,7 +85,7 @@ export const ConfirmInfoAlertRow = ({
<>
{alertModalVisible && (
<MultipleAlertModal
alertKey={fieldAlerts[0].key}
alertKey={selectedAlertKey}
ownerId={ownerId}
onFinalAcknowledgeClick={handleModalClose}
onClose={handleModalClose}
Expand Down
Loading