Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.5k stars 2.85k forks source link

[Hold for Payment 2023-12-21][$1000] IOS - Permission popup indicates wrong message #23421

Closed kbecciv closed 10 months ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Action Performed: Go to phone settings Turn off Microphone permission.

Expected Result:

It should show Alert for Microphone permission.

Actual Result:

t shows Camera permission alert instead of microphone permission

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.42-26 Reproducible in staging?: n/a Reproducible in production?: n/a If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/f1171187-f0ae-4db4-89f4-4cc647426f25

Expensify/Expensify Issue URL: Issue reported by: @DinalJivani Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689948082634299

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01479aaf3e91037a82
  • Upwork Job ID: 1683857056510726144
  • Last Price Increase: 2023-08-08
  • Automatic offers:
    • situchan | Contributor | 26071892
    • DinalJivani | Reporter | 26071895
    • ishpaul777 | Contributor | 27357555
zanyrenney commented 1 year ago

Thanks for the update, yes is reproducible for me on iOS.

zanyrenney commented 1 year ago

So we're still looking for proposals here @situchan

zanyrenney commented 1 year ago

bump @situchan ?

situchan commented 1 year ago

Still looking for contributor who is able to test their fix on iOS

zanyrenney commented 1 year ago

what can we do to help get a contributor working on this @situchan ?

zanyrenney commented 1 year ago

Asking for a volunteer here - https://expensify.slack.com/archives/C01GTK53T8Q/p1695260641687059

pradeepmdk commented 1 year ago

I will help you here I have physical Device

allroundexperts commented 1 year ago

@pradeepmdk I'm already on it https://expensify.slack.com/archives/C01GTK53T8Q/p1695260958279589?thread_ts=1695260641.687059&cid=C01GTK53T8Q

allroundexperts commented 1 year ago

Just tested on my device. It seems like our builds aren't using the microphone permission anymore. @DinalJivani Can you confirm?

https://github.com/Expensify/App/assets/30054992/df94e4d9-1024-40b5-b8f6-cf9906147bc7

DinalJivani commented 1 year ago

@zanyrenney @mvtglobally I see this issue is still reproducible not sure what @mvtglobally is missing here.

May be you have fresh installed app and for that you have to go to further steps in which app asks user to enable Microphone, Then you have to follow these steps:

Go to phone settings New Expensify -> Turn off Microphone permission. Go to New Expensify App Go to setting -> Go to any workspace Setup Bank account -> On last step click Save & continue

Attaching full video here. https://github.com/Expensify/App/assets/58871336/612d02af-5a58-4eed-801a-96604c91d45d

@allroundexperts Please check this full video, To reach the microphone service usage you have to go till further process of Recording user's video, then only you will see the Microphone permission usage in Settings. This is because that is the only place using Microphone service.

allroundexperts commented 1 year ago

Okay. Tested again and reproduced it. The following error occurs when mic permission is not given. The approach we're using right now (getting error based on the onfido error) won't really work as its a generic error.

Screenshot 2023-09-22 at 1 58 49 AM
allroundexperts commented 1 year ago

Using react-native-permissions is working well. Here's the video:

https://github.com/Expensify/App/assets/30054992/29a6029f-ef9c-472d-9f4f-efbd1798a792

@situchan I've included the changes in this branch. Note that the required changes are in the index.native file. Rest of the changes are not needed and I've only included those so that I can easily test this on iOS if we decide to proceed.

zanyrenney commented 1 year ago

Awesome, thanks for the update @allroundexperts

zanyrenney commented 1 year ago

any update here @allroundexperts ?

allroundexperts commented 1 year ago

@zanyrenney Waiting for @situchan's feedback on https://github.com/Expensify/App/issues/23421#issuecomment-1730314775. If this looks good, then I can proceed to a PR!

zanyrenney commented 1 year ago

Bump @situchan please can you provide feedback so we can move ahead to the PR. Thanks!

zanyrenney commented 1 year ago

bump @situchan please?

situchan commented 1 year ago

@allroundexperts thanks for the demo. Can you test android as well as the change also affects android?

zanyrenney commented 1 year ago

bump @allroundexperts for the testing on android please?

melvin-bot[bot] commented 1 year ago

@zanyrenney, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

zanyrenney commented 1 year ago

bump @situchan @allroundexperts

allroundexperts commented 1 year ago

On it tomorrow!

zanyrenney commented 1 year ago

did you make progress @allroundexperts ?

allroundexperts commented 1 year ago

@situchan Android Onfido SDK behaves differently. Following is the behaviour:

https://github.com/Expensify/App/assets/30054992/606db2f6-0c0c-42d1-bcbb-fd03c4ca3967

DinalJivani commented 1 year ago

@allroundexperts I don't know if this would be helpful or not but want to mention that,

For Android I'm not sure but In iOS whenever System popup appears (i.e.: For Permission Request) app is considered to be in Background state, Also if you change any permission for app running in Background it will restart the app.

zanyrenney commented 1 year ago

@situchan please add feedback. If you are unable to prioritise this, please say so we can reassign this? I have had to bump you numerous times on this issue and it's getting to the point where it feels like we should find another C+ otherwise.

situchan commented 1 year ago

I will give feedback today

zanyrenney commented 1 year ago

asked in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1698054364350729

situchan commented 1 year ago

@allroundexperts I am not seeing your official proposal. Please submit so I can approve.

situchan commented 1 year ago

@ishpaul777's alternative solution looks good. But he was not able to test on physcial device and cannot attach screenshots which is required step in PR checklist. @allroundexperts volunteered on testing and shared demo video.

I understand that external contributors cannot easily test on iOS physical device. So I suggest @ishpaul777 raise PR and @allroundexperts test and fill missing part of checklist. And compensation suggestion: @ishpaul777: 75%, @allroundexperts: 25% If everyone agrees.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @chiragsalian, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

allroundexperts commented 1 year ago

I think a 50-50 split would be more fair because I've already created a branch (https://github.com/allroundexperts/Expensify/tree/fix-23421) which @ishpaul777 can use. Also, testing this is a more difficult task IMO.

ishpaul777 commented 1 year ago

I'd agree @allroundexperts, 50-50 split would be fair

situchan commented 1 year ago

@allroundexperts thanks for the suggestion. It seems @ishpaul777 gave 👍 ok, so share equal bounty, collaborate together on PR, both review and testing.

@chiragsalian all yours

chiragsalian commented 1 year ago

Cool sounds good to me, feel free to create the PR @ishpaul777.

melvin-bot[bot] commented 1 year ago

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

ishpaul777 commented 1 year ago

just one question @zanyrenney, shouldn't we have a similar copy for alert message for both camera and microphone permission errors.

cc @situchan @allroundexperts @chiragsalian

Microphone Camera
Screenshot 2023-10-25 at 3 34 17 PM Screenshot 2023-10-25 at 3 32 26 PM
situchan commented 1 year ago

Good point. We may need to confirm all copies once again.

English:

Camera permissions not granted You have not granted us camera access. We need access to complete verification.

Microphone Access Denied This app requires access to your device's microphone. Please enable microphone access for this app in Settings > Privacy > Microphone.

Spanish:

No has habilitado los permisos para acceder a la cámara No has habilitado los permisos para acceder a la cámara. Necesitamos acceso para completar la verificaciôn.

Acceso al micrófono denegado Esta aplicación requiere acceso al micrófono de su dispositivo. Habilite el acceso al micrófono para esta aplicación en Configuración > Privacidad > Micrófono

@chiragsalian can you please confirm internally or add Waiting for copy label?

chiragsalian commented 1 year ago

cool i just asked internally in our slack channel. I'll post back here once i get a reply. Edit: Realized we need to polish the english copy before getting translations.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @mallenexpensify (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

chiragsalian commented 1 year ago

Hi @mallenexpensify, could you help us out with the English copy here for the camera and microphone errors. Additionally we were wondering if we could have a similar copy for both to make it more consistent.

ishpaul777 commented 1 year ago

Draft PR is up. @allroundexperts Please test changes whenever you get the chance.

mallenexpensify commented 1 year ago

👋 Lotta comments here....

  1. Here's what we currently show in the Expensify App for 'enable camera' IMG_0473 I propose we use similar text (I removed of receipts since people will share non-receipt photos)

    Enable Access to Camera To use Expensify to take pictures you'll need to all Expensify to access your camera: Setting > New Expensify (where this is a button that links directly to Expensify Settings)

  2. For microphone, is there a use case for when someone would use their mic, and need to enable? I'm guessing we're pre-planning for the day someone would be able to record a video from the app, which seems like that's far off.
    Assume we need copy, let's go with this

    Allow New Expensify to access your microphone to record video Setting > New Expensify (where this is a button that links directly to Expensify Settings)

ishpaul777 commented 1 year ago

Thank you for providing the copy @mallenexpensify. Just to provide some context for why we need this copy: it's required to address the situation when a user has denied access to their microphone during the Onfido video verification process. When this happens, the Onfido flow generates an error due to the denied access, currently we are not showing correct message (showing camera permission denied instead of microphone) the issue aims to fix that.

mallenexpensify commented 1 year ago

Thanks @ishpaul777 , it prolly would have been beneficial if I had read more of the issue :ohnothing:

So... the mic access is specific to Onfido then... right? If so, should we address that?

Personally, I have a pet peeve for apps that want access to my mic when it's not necessary (ie. Instagram where I don't post vids).

How about these?

Enable Camera Access We need access to your camera to complete bank account verification. Please enable via Setting > New Expensify (where this is a button that links directly to Expensify Settings)

Enable Microphone Access We need access to your microphone to complete bank account verification. Please enable via Setting > New Expensify (where this is a button that links directly to Expensify Settings)

ishpaul777 commented 12 months ago

@mallenexpensify Thanks for the updated copy.

So... the mic access is specific to Onfido then... right? If so, should we address that?

Yes, Imo we should because it will be better if we correct error message instead of wrong (we are showing camera access denied instead of mic, even when camera access is allowed)

To be on same page, I want to clarify that depending on phone OS (android or Ios) the destination of permission may be different and not always Settings > New Expensify in my opinion it will be better off if dont include the path. Also we already link the user to App settings when user clicks "settings" button on right. thoughts?

277953810-235c7aef-ae1c-4380-b7de-b3cd19559882

cc @situchan @chiragsalian

mallenexpensify commented 12 months ago

Thanks @ishpaul777 , I focused on iOS cuz I saw it was the one that was ✅ in the OP

image

If the pop up for both iOS and Android provide for a direct link to settings, where camera access can be allowed, then we can provide a more generic message for enabling mic and/or camera.

ishpaul777 commented 12 months ago

I am not 100% sure if onfido throws a Mic error on android and behaves similar to ios, @allroundexperts is volunteering for PR testing https://github.com/Expensify/App/issues/23421#issuecomment-1776574706, I am inclined to showing a generic error message for camera and mic if there's a error (other than these errors) from onfido and user is not allowed permissions of camera and microphone in android and ios, as both of them are required for verification and we can't rely on error messages from Onfido permission message incase they change our logic will fail and its not future safe for handling permissions. would love to get more thoughts here?

cc @chiragsalian @situchan @mallenexpensify


@allroundexperts can you please test current changes from https://github.com/Expensify/App/pull/30374

chiragsalian commented 12 months ago

in my opinion it will be better off if dont include the path

I'd like path to be included. I feel like most users are not really tech savvy and would loose interest in the product fast if they can't find what needs to be enabled quickly. But this is my opinion, if others disagree with me that's cool too.

i wasn't sure which other point you wanted to discuss, was it this,

to showing a generic error message for camera and mic

Like one error regardless if its camera or mic error? Hmm i prefer 1 error each just because i think better user feedback results in faster resolution of issues because its clearer communication. If you meant the above just very specific to showing a generic error if its not one of the onfido errors I'm fine with that.

ishpaul777 commented 12 months ago

I think i misspoke the "generic error" By generic error i mean different error message for camera and microphone but similar in both ios and android (like dont change error messages based on OS, we have to if we decide to include path)

by not depending on onfido error messages i mean currently we only show permission error if onfido throws a error related to permission though errror messages from onfido, i suggest we check for permission and show error if not allowed if there's a error even if not related to permission because without camera and microphone it is not possible to complete verification.

Alternate to ^, i found that the error thrown by onfido is specific to ios and on android no error is thrown but there is a screen for camera/mic access denied

Screenshot 2023-11-02 at 1 47 38 AM

If agreed We can make this change specific to ios only like this:

  const os = getPlatform();
  if (!_.isEmpty(errorMessage) && os === CONST.PLATFORM.IOS) {
  checkMultiple([PERMISSIONS.IOS.MICROPHONE, PERMISSIONS.IOS.CAMERA]).then((statuses) => {
  // rest of code