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

chore: refactor the DeeplinkManager into smaller parts and add unit tests #7925

Conversation

omridan159
Copy link
Contributor

@omridan159 omridan159 commented Nov 24, 2023

Description

Refactored the DeeplinkManager class into smaller, more manageable components, enhancing code readability and maintainability.
This change simplifies future modifications and debugging. Additionally, comprehensive unit tests were added to ensure each component functions correctly, boosting overall software reliability.

Related issues

Fixes: #

Manual testing steps

IOS

  1. Test the deep links behavior with the devnext project on a testing device.
  2. Test the deep links behavior with the devreactnative project on a testing device.

Android:

  1. Test the deep links behavior with the devnext project on a testing device.
  2. Test the deep links behavior with the devreactnative project on a testing device.

Screenshots/Recordings

Before

After

Screen.Recording.2023-11-24.at.11.00.23.mov

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.

@omridan159 omridan159 added the team-sdk SDK team label Nov 24, 2023
@omridan159 omridan159 requested a review from a team as a code owner November 24, 2023 09:18
Copy link
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.

@omridan159 omridan159 changed the base branch from main to chore_transfer-DeeplinkManager-file-from-JS-to-TS November 24, 2023 09:19
Copy link
Contributor

@abretonc7s abretonc7s left a comment

Choose a reason for hiding this comment

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

I think we should rename all the handleParseMetamask/Universal/Walle by handleMetamask/Univer/Walle.

It would make it easier to match the filename to the functionality without opening the file

@omridan159 omridan159 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Nov 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

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

Comparison is base (89eb8cd) 36.59% compared to head (9cec8a9) 36.99%.
Report is 5 commits behind head on main.

Files Patch % Lines
...eeplinkManager/ParseManager/handleUniversalLink.ts 75.67% 6 Missing and 3 partials ⚠️
...linkManager/ParseManager/handleMetaMaskDeeplink.ts 75.00% 4 Missing and 2 partials ⚠️
.../core/DeeplinkManager/Handlers/handleBrowserUrl.ts 16.66% 4 Missing and 1 partial ⚠️
...core/DeeplinkManager/Handlers/handleEthereumUrl.ts 85.00% 1 Missing and 2 partials ⚠️
app/core/DeeplinkManager/Handlers/switchNetwork.ts 71.42% 1 Missing and 1 partial ⚠️
...core/DeeplinkManager/ParseManager/parseDeeplink.ts 90.47% 1 Missing and 1 partial ⚠️
...nkManager/TransactionManager/approveTransaction.ts 88.88% 0 Missing and 2 partials ⚠️
app/components/Views/QRScanner/index.tsx 0.00% 1 Missing ⚠️
...core/DeeplinkManager/ParseManager/connectWithWC.ts 75.00% 1 Missing ⚠️
...e/DeeplinkManager/ParseManager/extractURLParams.ts 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7925      +/-   ##
==========================================
+ Coverage   36.59%   36.99%   +0.40%     
==========================================
  Files        1120     1130      +10     
  Lines       29188    29110      -78     
  Branches     2717     2723       +6     
==========================================
+ Hits        10682    10770      +88     
+ Misses      17894    17715     -179     
- Partials      612      625      +13     

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

Copy link

@deeeed deeeed 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
Contributor

@abretonc7s abretonc7s left a comment

Choose a reason for hiding this comment

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

LGTM

abretonc7s
abretonc7s previously approved these changes Dec 4, 2023
Copy link
Contributor

@abretonc7s abretonc7s left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from chore_transfer-DeeplinkManager-file-from-JS-to-TS to main December 4, 2023 16:19
@christopherferreira9 christopherferreira9 dismissed abretonc7s’s stale review December 4, 2023 16:19

The base branch was changed.

@christopherferreira9
Copy link
Contributor

@christopherferreira9
Copy link
Contributor

Checked for both Android and iOS

Only one issue found @omridan159 .
Steps to reproduce:

  1. Open https://metamask.github.io/metamask-deeplinks/#
  2. Select "Payment request"
  3. Enter any details you want with and without chainId (on separate tries)
  4. Tap the link
  5. See issue

Video:

Screen.Recording.2023-12-04.at.17.45.23.mov

Copy link

sonarcloud bot commented Dec 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

86.6% 86.6% Coverage
0.0% 0.0% Duplication

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

@christopherferreira9
Copy link
Contributor

The issue mentioned above is fixed.

@christopherferreira9 christopherferreira9 added QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. labels Dec 6, 2023
@christopherferreira9 christopherferreira9 merged commit 94e5e08 into main Dec 6, 2023
31 of 32 checks passed
@christopherferreira9 christopherferreira9 deleted the chore_split-DeeplinkManager-class-into-smaller-chunks branch December 6, 2023 15:23
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
@metamaskbot metamaskbot added the release-7.14.0 Issue or pull request that will be included in release 7.14.0 label Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.14.0 Issue or pull request that will be included in release 7.14.0 team-sdk SDK team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants