fix: Clicking on report link does not add external_link_clicked to si… #29969

pnarayanaswamy · 2025-01-29T15:30:01Z

…gnature events

Description

Fixes the bug that the external_link_clicked property is not added to the Signature approved, signature rejected , transaction approved and transaction rejected events
Adds integration tests to verify the property is added to the metrics

Related issues

Fixes: #23995

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

…gnature events

github-actions · 2025-01-29T15:30:15Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2025-01-30T09:29:20Z

Builds ready [b22bea6]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1619 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	466	1770	1564	266	128
		domContentLoaded	1435	1718	1591	88	42
		load	1445	1768	1619	91	44
		domInteractive	23	78	42	21	10
		backgroundConnect	8	61	24	18	9
		firstReactRender	15	99	37	28	14
		getState	4	71	16	18	9
		initialActions	0	0	0	0	0
		loadScripts	1020	1303	1166	81	39
		setupStore	7	81	17	21	10
		uiStartup	1667	2432	1903	212	102

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 399 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-01-30T14:10:31Z

Builds ready [a3f8f6d]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1705 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	300	2066	1643	342	164
		domContentLoaded	1465	2056	1682	147	71
		load	1474	2069	1705	155	74
		domInteractive	23	171	45	33	16
		backgroundConnect	8	87	27	24	12
		firstReactRender	15	100	41	27	13
		getState	4	56	15	17	8
		initialActions	0	1	0	0	0
		loadScripts	1034	1510	1216	110	53
		setupStore	6	80	15	18	9
		uiStartup	1701	2440	1959	199	96

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 667 Bytes (0.01%)
common: 0 Bytes (0.00%)

jpuri · 2025-02-03T09:55:22Z

ui/components/app/alert-system/general-alert/general-alert.tsx

@@ -45,6 +45,7 @@ function ReportLink({
    <Text marginTop={1} display={Display.Flex}>
      {t('somethingDoesntLookRight', [
        <ButtonLink
+          data-testid="alert-provider-report-link"


We avoid adding data-testid in extension, in case its possible to query using button text it may be better.

I like this suggestion. I do like using findByText to also test text expectations simultaneously

But text can change, making tests flaky and prone to failure due to content updates. Coupling tests to specific text, especially when supporting multiple languages is not recommended. Why do we not want to use test-ids? it supports testing and makes things stable.

digiwand

one thing, I think we should remove the console.log before merging. other than this, Lgtm!

test/integration/confirmations/signatures/signature-helpers.ts

test/integration/confirmations/signatures/permit.test.tsx

test/integration/confirmations/signatures/signature-helpers.ts

OGPoyraz · 2025-02-03T11:35:16Z

ui/pages/confirmations/components/confirm/title/title.tsx

+  const onClickSupportLink = () => {
+    const properties = {
+      properties: {
+        external_link_clicked: 'security_alert_support_link',
+      },
+    };
+    updateSignatureEventFragment(properties);
+    updateTransactionEventFragment(properties, ownerId);
+  };
+


Should we create this callback with useCallback if it has ownerId dependency?

Suggested change

const onClickSupportLink = () => {

const properties = {

properties: {

external_link_clicked: 'security_alert_support_link',

},

};

updateSignatureEventFragment(properties);

updateTransactionEventFragment(properties, ownerId);

};

const onClickSupportLink = useCallback(() => {

const properties = {

properties: {

external_link_clicked: 'security_alert_support_link',

},

};

updateSignatureEventFragment(properties);

updateTransactionEventFragment(properties, ownerId);

}, [ownerId]);

it is already done in ui/pages/confirmations/hooks/useTransactionEventFragment.js, so adding it here won't make any difference right?

I think only fallback is settling in the updateTransactionEventFragment not the given transactionId (in this case ownerId) In my opinion, it's good practice to have it right in order to prevent any side effects.

Co-authored-by: digiwand <[email protected]>

Co-authored-by: OGPoyraz <[email protected]>

metamaskbot · 2025-02-04T13:05:00Z

Builds ready [79a2a4f]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1745 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	252	2088	1676	358	172
		domContentLoaded	1489	2075	1721	155	75
		load	1499	2092	1745	154	74
		domInteractive	24	164	45	31	15
		backgroundConnect	9	99	31	23	11
		firstReactRender	16	96	40	27	13
		getState	4	59	12	12	6
		initialActions	0	1	0	0	0
		loadScripts	1035	1473	1246	125	60
		setupStore	8	86	21	23	11
		uiStartup	1715	2545	2005	227	109

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 667 Bytes (0.01%)
common: 0 Bytes (0.00%)

OGPoyraz · 2025-02-04T13:55:40Z

test/integration/confirmations/transactions/contract-interaction.test.tsx

+    await waitFor(() => {
+      updateTransactionEventFragment =
+        mockedBackgroundConnection.submitRequestToBackground.mock.calls?.find(
+          (call) =>
+            call[0] === 'updateEventFragment' &&
+            JSON.stringify(call[1]).includes(
+              JSON.stringify({
+                properties: {
+                  external_link_clicked: 'security_alert_support_link',
+                },
+              }),
+            ),
+        );
+
+      expect(updateTransactionEventFragment).toBeDefined();
+    });
+  });


Do we want to change this to use jest matchers as well?

metamaskbot · 2025-02-05T09:53:01Z

Builds ready [9ece707]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1562 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1399	1836	1563	124	60
		domContentLoaded	1382	1822	1544	119	57
		load	1391	1839	1562	124	60
		domInteractive	23	67	35	15	7
		backgroundConnect	8	54	27	19	9
		firstReactRender	15	95	47	28	14
		getState	4	10	6	2	1
		initialActions	0	1	0	0	0
		loadScripts	966	1337	1104	106	51
		setupStore	7	52	10	10	5
		uiStartup	1608	2191	1788	172	83

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 667 Bytes (0.01%)
common: 0 Bytes (0.00%)

fix: Clicking on report link does not add external_link_clicked to si…

4bc69ef

…gnature events

pnarayanaswamy requested a review from a team as a code owner January 29, 2025 15:30

metamaskbot added the team-qa QA team label Jan 29, 2025

Merge branch 'main' into fix-signature-metrics

b22bea6

pnarayanaswamy added 2 commits January 30, 2025 14:22

test: add integration tests

990cc0d

fix: transaction event fragment

a3f8f6d

pnarayanaswamy added the team-confirmations Push issues to confirmations team label Jan 30, 2025

jpuri reviewed Feb 3, 2025

View reviewed changes

pedronfigueiredo previously approved these changes Feb 3, 2025

View reviewed changes

digiwand requested changes Feb 3, 2025

View reviewed changes

test/integration/confirmations/signatures/signature-helpers.ts Outdated Show resolved Hide resolved

test/integration/confirmations/signatures/signature-helpers.ts Outdated Show resolved Hide resolved

OGPoyraz reviewed Feb 3, 2025

View reviewed changes

Welbert5753 approved these changes Feb 3, 2025

View reviewed changes

Update test/integration/confirmations/signatures/signature-helpers.ts

ffe1d9a

Co-authored-by: digiwand <[email protected]>

pnarayanaswamy dismissed pedronfigueiredo’s stale review via ffe1d9a February 4, 2025 12:16

pnarayanaswamy and others added 3 commits February 4, 2025 13:17

Update test/integration/confirmations/signatures/permit.test.tsx

a3b7a0a

Co-authored-by: OGPoyraz <[email protected]>

Update ui/pages/confirmations/components/confirm/title/title.tsx

95ca259

Co-authored-by: OGPoyraz <[email protected]>

chore: address review comments

79a2a4f

OGPoyraz reviewed Feb 4, 2025

View reviewed changes

chore: lint fix

9ece707

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Clicking on report link does not add external_link_clicked to si… #29969

fix: Clicking on report link does not add external_link_clicked to si… #29969

pnarayanaswamy commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025

metamaskbot commented Jan 30, 2025

metamaskbot commented Jan 30, 2025

jpuri Feb 3, 2025

digiwand Feb 3, 2025

pnarayanaswamy Feb 4, 2025

digiwand left a comment

OGPoyraz Feb 3, 2025

pnarayanaswamy Feb 4, 2025

OGPoyraz Feb 4, 2025

metamaskbot commented Feb 4, 2025

OGPoyraz Feb 4, 2025

metamaskbot commented Feb 5, 2025

fix: Clicking on report link does not add external_link_clicked to si… #29969

Are you sure you want to change the base?

fix: Clicking on report link does not add external_link_clicked to si… #29969

Conversation

pnarayanaswamy commented Jan 29, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 29, 2025

metamaskbot commented Jan 30, 2025

metamaskbot commented Jan 30, 2025

jpuri Feb 3, 2025

Choose a reason for hiding this comment

digiwand Feb 3, 2025

Choose a reason for hiding this comment

pnarayanaswamy Feb 4, 2025

Choose a reason for hiding this comment

digiwand left a comment

Choose a reason for hiding this comment

OGPoyraz Feb 3, 2025

Choose a reason for hiding this comment

pnarayanaswamy Feb 4, 2025

Choose a reason for hiding this comment

OGPoyraz Feb 4, 2025

Choose a reason for hiding this comment

metamaskbot commented Feb 4, 2025

OGPoyraz Feb 4, 2025

Choose a reason for hiding this comment

metamaskbot commented Feb 5, 2025

pnarayanaswamy commented Jan 29, 2025 •

edited

Loading