Skip to content

feat: a hook to use Segment metrics#8471

Merged
NicolasMassart merged 5 commits into
feat/batch_1129_segmentfrom
1500_metrics_hook
Feb 2, 2024
Merged

feat: a hook to use Segment metrics#8471
NicolasMassart merged 5 commits into
feat/batch_1129_segmentfrom
1500_metrics_hook

Conversation

@NicolasMassart

@NicolasMassart NicolasMassart commented Jan 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

  • make MetaMetrics.getInstance synchronous and add an async init function + readiness check
  • destructurable object hook return value to use metrics in components without having to use useRef and useEffect to set the instance
  • interface for the destructurable object
  • HOC (High Order Component) wrapper for class components
  • new and updated unit tests

Related issues

fixes https://github.com/MetaMask/mobile-planning/issues/1500

Manual testing steps

none

Screenshots/Recordings

Before

NA

After

NA

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@github-actions

github-actions Bot commented Jan 30, 2024

Copy link
Copy Markdown
Contributor

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.

@NicolasMassart NicolasMassart force-pushed the 1500_metrics_hook branch 2 times, most recently from b37bfac to 3008d01 Compare January 30, 2024 16:54
wachunei
wachunei reviewed Jan 30, 2024
View reviewed changes
Comment thread app/components/hooks/useMetrics/useMetrics.ts Outdated
Cal-L
Cal-L reviewed Feb 1, 2024
View reviewed changes

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments

Comment thread app/components/hooks/useMetrics/withMetricsAwareness.tsx
Comment thread app/components/hooks/useMetrics/useMetrics.ts Outdated
Comment thread app/components/hooks/useMetrics/useMetrics.ts
Comment thread app/components/hooks/useMetrics/useMetrics.ts Outdated
@NicolasMassart NicolasMassart force-pushed the 1500_metrics_hook branch 2 times, most recently from af0bfe8 to 5d526a2 Compare February 1, 2024 17:37
@NicolasMassart NicolasMassart added team-mobile-platform Mobile Platform team No QA Needed Apply this label when your PR does not need any QA effort. labels Feb 1, 2024
@NicolasMassart
simplify hook
32a0e41 
and add async init
@MetaMask MetaMask deleted a comment from codecov-commenter Feb 1, 2024
@NicolasMassart NicolasMassart marked this pull request as ready for review February 1, 2024 18:54
@NicolasMassart NicolasMassart requested a review from a team as a code owner February 1, 2024 18:54
@github-actions

github-actions Bot commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d614e8d1-3a82-485e-b82e-76b94714fb38
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

wachunei
wachunei reviewed Feb 1, 2024
View reviewed changes
Comment thread app/components/hooks/useMetrics/useMetrics.ts Outdated
Cal-L
Cal-L reviewed Feb 1, 2024
View reviewed changes

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments

Comment thread app/components/hooks/useMetrics/withMetricsAwareness.tsx Outdated
Comment thread app/core/Analytics/MetaMetrics.ts Outdated
Comment thread app/core/Analytics/MetaMetrics.ts Outdated
@NicolasMassart
rename init to configure and remove it from hook
fe611c4 
- use interface for props types
- keep isConfigured private only
- update tests
- update HOC
@codecov-commenter

codecov-commenter commented Feb 2, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (06a526e) 40.90% compared to head (5093755) 40.94%.

Files Patch % Lines
app/core/Analytics/MetaMetrics.ts 89.65% 0 Missing and 3 partials ⚠️
Additional details and impacted files 
@@                     Coverage Diff                     @@
##           feat/batch_1129_segment    #8471      +/-   ##
===========================================================
+ Coverage                    40.90%   40.94%   +0.04%     
===========================================================
  Files                         1243     1245       +2     
  Lines                        30110    30127      +17     
  Branches                      2894     2899       +5     
===========================================================
+ Hits                         12315    12335      +20     
+ Misses                       17094    17090       -4     
- Partials                       701      702       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Cal-L
Cal-L reviewed Feb 2, 2024
View reviewed changes
Comment thread app/components/hooks/useMetrics/useMetrics.test.tsx Outdated
Cal-L
Cal-L approved these changes Feb 2, 2024
View reviewed changes

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud

sonarqubecloud Bot commented Feb 2, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
97.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@NicolasMassart NicolasMassart merged commit f6be241 into feat/batch_1129_segment Feb 2, 2024
@NicolasMassart NicolasMassart deleted the 1500_metrics_hook branch February 2, 2024 11:32
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Cal-L Cal-L Cal-L approved these changes

@wachunei wachunei Awaiting requested review from wachunei

Assignees

No one assigned

Labels

No QA Needed Apply this label when your PR does not need any QA effort. team-mobile-platform Mobile Platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@NicolasMassart @codecov-commenter @wachunei @Cal-L