fix: request location permission after it is rejected during Ledger connection by dawnseeker8 · Pull Request #8745 · MetaMask/metamask-mobile

dawnseeker8 · 2024-02-27T12:26:26Z

Description

This PR is to fix #8643 and #8718 issue

This PR has done following change:

for Android 12 and above, we only need nearby devices permission for ledger bluetooth connection. not required location permission any more.
for Android 11, system will only require location permission with precise location on to work with ledger.
Add nearby devices permission error message in en.json
styling the ErrorDetail modal to fit the new nearby devices in the screen.

Related issues

Fixes: #8643
#8718

Manual testing steps

From Android settings>connected devices> see all make sure LNX not currently paired
Have LNX unlocked and the Ethereum app opened
Unlock MMM
Tap accounts menu from wallet view
Select add account or hardware wallet
Tap Add hardware wallet
Tap Ledger
When nearby permission is requested, decline it
when error page appear, click view settings which direct to setting page of metamask permission
enable Nearby Devices permission, and click back to app
metamask will go back to scan screen and then find LNX devices.
click continue, LNX account will be imported.

Screenshots/Recordings

Before

After

Screen_Recording_20240227_233546_MetaMask.mp4

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.

… to setting page.

github-actions · 2024-02-27T12:26:37Z

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.

codecov-commenter · 2024-02-27T12:38:12Z

Codecov Report

Attention: Patch coverage is 91.30435% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 45.93%. Comparing base (b90cb4a) to head (932b18e).

Files	Patch %	Lines
app/components/Views/LedgerConnect/Scan.tsx	50.00%	2 Missing ⚠️
...s/UI/LedgerModals/Steps/SearchingForDeviceStep.tsx	80.00%	0 Missing and 1 partial ⚠️
app/components/Views/LedgerConnect/index.tsx	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8745      +/-   ##
==========================================
+ Coverage   45.35%   45.93%   +0.58%     
==========================================
  Files        1272     1273       +1     
  Lines       31247    31274      +27     
  Branches     3189     3195       +6     
==========================================
+ Hits        14171    14366     +195     
+ Misses      16234    16066     -168     
  Partials      842      842

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

github-actions · 2024-02-27T15:39:20Z

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4607f381-3685-46c9-a234-ae462b621c95
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

github-actions · 2024-02-29T00:42:35Z

Bitrise

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

Commit hash: 8e051db
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/69c2ff59-8614-4104-84c6-bcdf6ce12473

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

plasmacorral · 2024-03-01T21:46:36Z

Android 11
Moto G Play 2021
Testing commit 962b740
LNX fw 2.2.3 w/ Eth App 1.10.3

Manual testing steps above do not allow me to pair Ledger after enabling location permission in android 11. Result is error❌
Happy path with location on works on Android 11.
When location is off pairing error presents without actionable instruction for user (likely related to report in 8643)
When bluetooth is off View settings goes to app level info rather than device level bluetooth settings (previously reported in 4920.

Results for Android 11 device

Manual test steps with bluetooth and location both enabled

Recording
Screenshot of where user lands after tapping view settings
Screenshot of error after enabling Location permission in Android 11

From Android settings>connected devices> see all make sure LNX not currently paired
Have LNX unlocked and the Ethereum app opened
Unlock MMM
Tap accounts menu from wallet view
Select add account or hardware wallet
Tap Add hardware wallet
Tap Ledger
When ~~nearby permission~~ Location permission is requested, decline it
NEW STEP: Decline location permission when it is requested for a second time❌
when error page appear, click view settings which direct to setting page of MetaMask permission (ACTUAL- directed to App Info for MetaMask. I had to then know to click Permissions and tap Location and change selection from Don't Allow to Ask every time)❌
enable ~~Nearby Devices~~ Location permission, and click back to app
MetaMask will go back to scan screen and then find LNX devices. (ACTUAL- returned to An error occured with Location permission prompt overtop)❌
NEW STEP: Select Only this time on the location permission prompt
After granting permission returned to An error occured with View settings CTA
NEW STEP: Tap X on error page to return to Connect Ledger❌
click continue, LNX account will be imported. (ACTUAL- Unable to pair)❌

Bluetooth on/Location on (Happy path)

Recording
Location prompt screenshot

Access Settings>Applications>MetaMask>Storage & Cache and clear storage (to simulate fresh install)
Access Settings>Connected devices> to forget previously paired devices
Have Bluetooth enabled
Have Location enabled
Launch app
import SRP
Access accounts drop down
tap Add account or hardware wallet
tap Add hardware wallet
Tap Ledger
Tap Only this time or Ask every time equivalent on Location permission prompt
Tap continue once device is found
Tap Pair and connect on android
Tap connect on android
Tap both buttons on Ledger to confirm pairing
Confirm Ledger account appears in account list

Bluetooth on/Location off

Recording
Screenshot of pairing error

Access Settings>Applications>MetaMask>Storage & Cache and clear storage (to simulate fresh install)
Access Settings>Connected devices> to forget previously paired devices
Have Bluetooth enabled
Have Location disabled (OFF)
Launch app
import SRP
Access accounts drop down
tap Add account or hardware wallet
tap Add hardware wallet
Tap Ledger
Tap Only this time or Ask every time equivalent on Location permission prompt
Note generic error with no instruction to enable location

Bluetooth off/Location on

Recording
Screenshot of where user lands after tapping view settings

Access Settings>Applications>MetaMask>Storage & Cache and clear storage (to simulate fresh install)
Access Settings>Connected devices> to forget previously paired devices
Have Bluetooth disabled (OFF)
Have Location enabled
Launch app
import SRP
Access accounts drop down
tap Add account or hardware wallet
tap Add hardware wallet
Tap Ledger
Tap Only this time or Ask every time equivalent on Location permission prompt
Error presented Bluetooth is off
Tap View Settings
takes user to Settings>App info>MetaMask, but instead should be Device bluetooth settings at Settings>Connected Devices

github-actions · 2024-03-27T09:33:35Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 872b9ae
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1ed8db43-277e-4df1-ac97-7a3a4e5d94ae

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

NicolasMassart

Looks good to me.
Thanks again for all the effort you put in following advice and taking account of feedback!

vivek-consensys · 2024-04-02T14:32:02Z

Tested on Samsung S21 using Android 14.
Bitrise build:- https://app.bitrise.io/build/ce998116-a194-487f-a46d-315a8386cecb?tab=artifacts

Screen_Recording_20240402_152101_MetaMask-QA.mp4

…omponent to avoid the side effect.

socket-security · 2024-04-03T03:15:00Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@ledgerhq/devices@8.2.2	None	`+1`	395 kB	ldg-github-ci
npm/@ledgerhq/react-native-hw-transport-ble@6.32.3	None	`0`	316 kB	ldg-github-ci

🚮 Removed packages: npm/@ledgerhq/devices@8.2.1, npm/@ledgerhq/react-native-hw-transport-ble@6.32.0

View full report↗︎

github-actions · 2024-04-03T03:18:57Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 932b18e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/63a0f8de-293d-4fe5-8d1d-8d9fec79cda5

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

sonarqubecloud · 2024-04-03T03:25:30Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarCloud

NicolasMassart

Looks good to me.

dawnseeker8 · 2024-04-03T09:53:55Z

I have also tested the android 9 with location permission, and all the issue marked by Mike has gone with latest library upgrade.

dawnseeker8 and others added 9 commits February 26, 2024 18:13

fix: make a backup of remove location permission in android 12

11cd35b

fix(8718): modify nearby devices permission wording + related enum

7e9f43d

Merge branch 'main' into fix/8643-location-permission-issue

5448fa0

Merge branch 'fix/8718' into fix/8643-location-permission-issue

30eb467

fix: 8643 add NearbyDeivcesAccessBlocked error message to prompt user…

9cabb86

… to setting page.

fix: modify the popup message text for android 12 above and android 11

d61fb70

fix: Fix lint issue from yarn lint

2269aa9

fix: add back location permissions enums and wording

b98dc59

fix: improve the code use mutlipleRequest for NearbyDevice permission.

859ea3c

dawnseeker8 added the team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead label Feb 27, 2024

dawnseeker8 changed the title ~~Fix/8643 location permission issue~~ Fix: Fix 8643 location permission issue in ledgfer Feb 27, 2024

dawnseeker8 changed the title ~~Fix: Fix 8643 location permission issue in ledgfer~~ fix: a bug fix for 8643 location permission issue in ledger Feb 27, 2024

dawnseeker8 added 2 commits February 27, 2024 23:12

fix: Styling the nearby device permisson message in errorStep.

cf70328

fix: Revert file to main version

8e051db

dawnseeker8 marked this pull request as ready for review February 27, 2024 15:37

dawnseeker8 requested a review from a team as a code owner February 27, 2024 15:37

github-actions bot added the Run Smoke E2E label Feb 27, 2024

Akaryatrh mentioned this pull request Feb 27, 2024

fix(8718): modify nearby devices permission wording + related enum #8730

Closed

13 tasks

dawnseeker8 added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E and removed Run Smoke E2E labels Feb 29, 2024

dawnseeker8 added 3 commits February 29, 2024 08:46

Merge branch 'main' into fix/8643-location-permission-issue

22f0918

Merge branch 'main' into fix/8643-location-permission-issue

2bf53d9

fix: Refactory the code to reduce code complexity

962b740

plasmacorral mentioned this pull request Mar 1, 2024

If user rejects Bluetooth permissions for MMM app, error brings them to Bluetooth settings rather than app settings #4920

Closed

dawnseeker8 added 2 commits March 27, 2024 08:09

Fix: update scan component snapshot.

d4c8f01

Fix: update LedgerConnect component snapshot.

872b9ae

dawnseeker8 added Run Smoke E2E and removed Run Smoke E2E labels Mar 27, 2024

NicolasMassart previously approved these changes Mar 27, 2024

View reviewed changes

NicolasMassart added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 28, 2024

Merge branch 'main' into fix/8643-location-permission-issue

0c242ff

vivek-consensys added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 2, 2024

vivek-consensys added QA in Progress QA has started on the feature. and removed QA Passed QA testing has been completed and passed labels Apr 2, 2024

Fix: Fix the styling issue due to testId apply, put testId to title c…

9537547

…omponent to avoid the side effect.

dawnseeker8 dismissed NicolasMassart’s stale review via 9537547 April 3, 2024 03:04

Merge branch 'main' into fix/8643-location-permission-issue

932b18e

dawnseeker8 added Run Smoke E2E and removed Run Smoke E2E labels Apr 3, 2024

NicolasMassart approved these changes Apr 3, 2024

View reviewed changes

vivek-consensys added QA Passed QA testing has been completed and passed and removed QA in Progress QA has started on the feature. labels Apr 3, 2024

dawnseeker8 merged commit 9b25acf into main Apr 3, 2024

dawnseeker8 deleted the fix/8643-location-permission-issue branch April 3, 2024 09:54

github-actions bot locked and limited conversation to collaborators Apr 3, 2024

metamaskbot added the release-7.21.0 Issue or pull request that will be included in release 7.21.0 label Apr 3, 2024

Uh oh!

Conversation

dawnseeker8 commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Feb 27, 2024

Uh oh!

codecov-commenter commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Feb 27, 2024

Uh oh!

github-actions bot commented Feb 29, 2024

Bitrise

Uh oh!

plasmacorral commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Results for Android 11 device

Manual test steps with bluetooth and location both enabled

Bluetooth on/Location on (Happy path)

Bluetooth on/Location off

Bluetooth off/Location on

Uh oh!

github-actions bot commented Mar 27, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

NicolasMassart left a comment

Choose a reason for hiding this comment

Uh oh!

vivek-consensys commented Apr 2, 2024

Uh oh!

socket-security bot commented Apr 3, 2024

Uh oh!

github-actions bot commented Apr 3, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

sonarqubecloud bot commented Apr 3, 2024

Quality Gate passed

Uh oh!

NicolasMassart left a comment

Choose a reason for hiding this comment

Uh oh!

dawnseeker8 commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dawnseeker8 commented Feb 27, 2024 •

edited

Loading

codecov-commenter commented Feb 27, 2024 •

edited

Loading

plasmacorral commented Mar 1, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Apr 3, 2024 •

edited by metamaskbot

Loading