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 tracking for receive action #11695

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add tracking for receive action #11695

wants to merge 2 commits into from

Conversation

jonybur
Copy link
Contributor

@jonybur jonybur commented Oct 9, 2024

Description

This PR adds metrics tracking to the interactions with the "Receive" and "Request" features.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3260

Manual testing steps

Click on top QR for scanner, onReceive being ran should trigger a trackingEvent
Same thing for action button on lower menu
Same thing for asset in asset list

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@jonybur jonybur requested review from a team as code owners October 9, 2024 01:30
Copy link
Contributor

github-actions bot commented Oct 9, 2024

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.

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/smart-transactions-controller@13.2.0 network 0 198 kB metamaskbot

🚮 Removed packages: npm/@metamask/smart-transactions-controller@13.0.0

View full report↗︎

@jonybur jonybur added the Run Smoke E2E Triggers smoke e2e on Bitrise label Oct 9, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5ddd60d
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/94013a13-0757-4c3b-bc65-4a0ee6a53bd4

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented Oct 9, 2024

Copy link

@naugtur naugtur left a comment

Choose a reason for hiding this comment

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

There's nothing interesting going on here supply-chain wise.
I'm surprised seeing arbitrary human readable strings assigned to a field called origin, but I'm likely missing context here.

Copy link
Contributor

@EtherWizard33 EtherWizard33 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -181,7 +181,7 @@
"@metamask/selected-network-controller": "^15.0.2",
"@metamask/signature-controller": "^19.1.0",
"@metamask/slip44": "3.1.0",
"@metamask/smart-transactions-controller": "^13.0.0",
"@metamask/smart-transactions-controller": "^13.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain pls why this upgrade is needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to import the new definitions from MetaMetricsEventCategory and MetaMetricsEventName, which are available on 13.2.0

@@ -119,6 +119,7 @@ const AssetOverview: React.FC<AssetOverviewProps> = ({
navigation.navigate(Routes.QR_TAB_SWITCHER, {
initialScreen: QRTabSwitcherScreens.Receive,
disableTabber: true,
origin: 'Token details',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: origin is sometimes defined as part of AppConstants; definitely not a blocker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

7 participants