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.

Sign up for GitHub

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

Jump to bottom

fix: create nft auto detection modal and remove nft polling logic #9857

Merged
merged 58 commits into from
Jun 26, 2024
Merged

fix: create nft auto detection modal and remove nft polling logic #9857

merged 58 commits into from
Jun 26, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Jun 5, 2024 •
edited by NicolasMassart
Loading

Description

This PR removes any code that triggers polling on NftDetectionController.

Calling detectNfts() function is now only triggered when the user clicks on the NFT tab.

It also Enables NFTDetection by default for new users.

Note

this PR has an asset-cpntroller patch from this core PR #4281
This patch keeps NFTDetection controller extending the polling controller while the PR makes it extend BaseController but it makes sure the polling is not triggered anymore in the client code.
It will be fully updated in newer versions of assets-controllers
I removed what triggers the start() of the polling, and now instead, mobile calls detectNfts() directly.
Then i made updates on the fct getOwnerNfts so that it only fetches information for a specific cursor instead of looping through all user nfts.
The loop is now being done inside the detectNfts() fct, and we basically fetch the first page of NFTs and save it to state so its available to clients to view.

New functionalities highlight:

1- Enable NFT autodetection by default to new users.
2- Old users who have the NFT autodetection off will see the new NFT detection modal to encourage them to enable the modal.
3- When users click on the banner notice, they are no longer directed to settings and instead we enable the NFT detection after they click on “enable nft autodetection” and we show the Toast. We wanted to reduce friction and we removed that redirection to settings.
4- The code should not do any of the 3mins polling in the background anymore.

Related issues

Related: MetaMask/core#4281

Manual testing steps

  1. Import MM application
  2. Go to settings => Security and privacy and you should see that the NFT detection is enabled.
  3. Go back to home page and click on NFT tab; you should be able to see your NFTs being added.
  4. Switch to another account that has NFTs
  5. Click on NFTs tab and you should also see your NFTs being added to sate.
  6. Go to settings => Security and privacy and turn OFF the NFT detection toggle.
  7. You should see a new modal. Clicking on "allow" button should re-enable the NFT detection toggle.

Screenshots/Recordings

Before

Screen.Recording.2024-06-10.at.15.40.23.mov

After

Testing a fresh wallet import:

after-.mov

Testing an upgrade scenario:

First build on main, where you should see the NFT auto detection disabled by default if the user did not enable it

Then build on this PR:
You should see the new NFT detection modal and clicking on allow button should enable the NFT detection in the settings.

Screen.Recording.2024-06-10.at.15.45.02.mov

NFT detection banner with new Toast:

Screen.Recording.2024-06-14.at.13.15.50.mov

Pre-merge author checklist

  • I’ve followed MetaMask 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.

@sahar-fehri sahar-fehri requested review from a team as code owners June 5, 2024 13:31
@sahar-fehri sahar-fehri marked this pull request as draft June 5, 2024 13:31
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 5, 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.

@sahar-fehri sahar-fehri force-pushed the feat/call-nft-detection-on-components branch from 72d4c1b to e0abc2e Compare June 10, 2024 08:18
@sahar-fehri sahar-fehri added needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. team-assets labels Jun 10, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.

Project coverage is 48.22%. Comparing base (b013c71) to head (24c6caa).
Report is 57 commits behind head on main.

❗ Current head 24c6caa differs from pull request most recent head c061bab

Please upload reports for the commit c061bab to get more accurate results.

Files Patch % Lines
app/components/Views/Wallet/index.tsx 76.92% 2 Missing and 1 partial ⚠️
...ws/NFTAutoDetectionModal/NFTAutoDetectionModal.tsx 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9857      +/-   ##
==========================================
+ Coverage   47.24%   48.22%   +0.97%     
==========================================
  Files        1370     1401      +31     
  Lines       33304    33865     +561     
  Branches     3586     3671      +85     
==========================================
+ Hits        15736    16330     +594     
+ Misses      16607    16526      -81     
- Partials      961     1009      +48

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

@sahar-fehri sahar-fehri added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jun 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 10, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c061bab
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7095807b-fab4-4ade-abbb-1fe478e12ff3

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

@sahar-fehri sahar-fehri added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 10, 2024
@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

4 similar comments
@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@sahar-fehri sahar-fehri added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 25, 2024
@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@github-actions GitHub Actions
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0dfe239e-8872-4c03-bfb9-de0d305a1356

Note

  • This comment will auto-update when build completes
  • 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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

3 similar comments
@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a6b3b00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/683edcfc-816c-4a72-91bb-5ef0ac3cf6db

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

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
62.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0dfe239e-8872-4c03-bfb9-de0d305a1356

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

@sahar-fehri sahar-fehri added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bf27218f-edda-40af-93b7-b89fa6e1440d

Note

  • This comment will auto-update when build completes
  • 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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bf27218f-edda-40af-93b7-b89fa6e1440d

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

1 similar comment
@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bf27218f-edda-40af-93b7-b89fa6e1440d

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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/652cb227-f5ad-49df-b0ba-930b2237d896

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

@cortisiko cortisiko added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bfe23d7a-88b1-497a-847c-4fe9c8ffc21a

Note

  • This comment will auto-update when build completes
  • 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

@metamaskbot
Copy link
Collaborator

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 899d714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bfe23d7a-88b1-497a-847c-4fe9c8ffc21a

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

@sahar-fehri sahar-fehri merged commit ba76567 into main Jun 26, 2024
43 checks passed
@sahar-fehri sahar-fehri deleted the feat/call-nft-detection-on-components branch June 26, 2024 18:09
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2024
@metamaskbot metamaskbot added the release-7.27.0 Issue or pull request that will be included in release 7.27.0 label Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cortisiko cortisiko cortisiko approved these changes

@tommasini tommasini tommasini approved these changes

@bergeron bergeron bergeron left review comments

@NicolasMassart NicolasMassart NicolasMassart left review comments

@brianacnguyen brianacnguyen brianacnguyen left review comments

Assignees
No one assigned
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-7.27.0 Issue or pull request that will be included in release 7.27.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-assets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sahar-fehri @codecov-commenter @cortisiko @metamaskbot @bergeron @NicolasMassart @brianacnguyen @tommasini