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.56k stars 2.9k forks source link

[HOLD for Payment 2024-09-06] [$250] Navigated back to the home screen after granting for Camera permission #46422

Closed m-natarajan closed 2 months ago

m-natarajan commented 3 months 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!


Version Number: 9.0.13-3 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: @cristipaval Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722253630425759

Action Performed:

  1. Start Submit Expense flow from Global Create
  2. Tap on the Scan tab
  3. Go to the Phone settings from the system prompt asking for Camera permission
  4. Go back to the App by tapping on <- NewExpensify on the top left corner of the iPhone

    Expected Result:

    Should resume the Submit Expense flow from where I left

    Actual Result:

    Landed on the home screen and I had to start the Submit Expense flow again

    Workaround:

    Unknown

    Platforms:

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

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [X] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/264bf230-249b-4786-bd29-69f93ecd840f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a4e1ed614d93976d
  • Upwork Job ID: 1818030342834766613
  • Last Price Increase: 2024-08-05
  • Automatic offers:
    • DylanDylann | Reviewer | 103467157
    • ravindra-encoresky | Contributor | 103467159
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

sakluger commented 3 months ago

Let's make sure to test this for other SmartScan flows where the user grants camera permission (i.e. track expense).

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01a4e1ed614d93976d

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @DylanDylann (External)

DylanDylann commented 3 months ago

I can't reproduce this bug on IOS

ravindra-encoresky commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

On scan tab, alert for asking camera permission open System Setting app, coming back after grating camera permission reloads the app. It should resume from previous page

What is the root cause of that problem?

if we change any app permission in iOS from System Setting, it reloads the app to apply it. As its iOS's default behaviour.

What changes do you think we should make in order to solve the problem?

Since it’s OS' natural behaviour to reload the app when permissions changes in System Settings, we can use the Onyx database to track the last screen accessed before the settings change. When the app returns from the settings, we’ll retrieve the last screen from Onyx and navigate back to it if available.

  1. We will create a action to save permission asked status
    function handlePermissionAfterRestart(status: boolean) {
    Onyx.merge(ONYXKEYS.HANDLE_PERMISSION_AFTER_APP_RESTART, status);
    }
  2. Set value to true below Linking.openSettings(); https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/libs/fileDownload/FileUtils.ts#L74-L79
handlePermissionAfterRestart(true);
  1. Check this value after app reload and navigate to scan screen if value is true. Also reset value back to false in Onyx database https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.tsx#L27-L30
    if (handlePermissions) {
            handlePermissionAfterRestart(false);
            interceptAnonymousUser(() =>
                IOU.startMoneyRequest(
                    CONST.IOU.TYPE.SUBMIT,
                    // When starting to create an expense from the global FAB, there is not an existing report yet. A random optimistic reportID is generated and used
                    // for all of the routes in the creation flow.
                    ReportUtils.generateReportID(),
                ),
            );
        }

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 3 months ago

📣 @ravindra-encoresky! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
ravindra-encoresky commented 3 months ago

Contributor details Your Expensify account email: ravindra.singh@encoresky.com Upwork Profile Link: https://www.upwork.com/freelancers/~01f09435e9ae8b77c7

melvin-bot[bot] commented 3 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

sakluger commented 3 months ago

@DylanDylann just to make sure, when you tried reproducing, did you remove camera permissions first?

sakluger commented 3 months ago

Commenting for melvin

DylanDylann commented 3 months ago

I can reproduce now. To reproduce we need to toggle on the permission before navigating back

https://github.com/user-attachments/assets/2bc08858-5bcd-4a82-b96d-4ec40c7e1958

DylanDylann commented 3 months ago

More information for contributors, When an iOS app requests camera access and the user grants permission, the app restarts due to changes in the app's permission settings. We need to handle this more smoothly and avoid the appearance of a reload.

ravindra-encoresky commented 3 months ago

@DylanDylann thanks for the more clarification, I've tried other solutions as well but somehow it had to take a little navigation before it re-opens the previous page like I added in proposal above.

thanks.

DylanDylann commented 3 months ago

@ravindra-encoresky Thanks for your proposal. Will review soon

DylanDylann commented 3 months ago

@ravindra-encoresky Thanks for your idea. Could you please provide a testing branch for your solution?

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ravindra-encoresky commented 3 months ago

@ravindra-encoresky Thanks for your idea. Could you please provide a testing branch for your solution?

Here is the branch with working sample - https://github.com/ravindra-encoresky/expensify/tree/46422-Navigated-home-after-camera-permission

Branch contain the code with basic working sample, we can improve it as our requirements

DylanDylann commented 3 months ago

https://github.com/user-attachments/assets/b71ab0d4-2198-4eaf-82e0-e0522ff2ae6b

@ravindra-encoresky After going back to scanning page, an error happens

ravindra-encoresky commented 3 months ago

https://github.com/user-attachments/assets/76950b43-82cc-444f-bd0f-99fa09ef6bdc

I am not facing this issue, please can you provide me more detail, on which ios version you are checking.

ravindra-encoresky commented 3 months ago

@DylanDylann I think you are testing in iOS simulator, camera is not available in iOS simulator thats why you are getting this issue. Also it has no relation to particular changes in this branch, this is existing behaviour

DylanDylann commented 3 months ago

@ravindra-encoresky Thanks for your update. This is an edge case when the app restarts due to changes in the app's permission settings. Let's go with @ravindra-encoresky's proposal

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

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

ravindra-encoresky commented 3 months ago

hi @roryabraham proposal submitted on upwork.

roryabraham commented 3 months ago

Thanks for the proposal @ravindra-encoresky, and good job finding the root cause (that the iOS app reloads with changed permissions). However, the solution you've proposed simply isn't flexible or reusable enough. There could be other flows that trigger permissions changes, and we won't always want to navigate to the scan receipt page.

I think you're on the right track though - when triggering a permissions change modal, we want to stash the current navigation state (is the route enough?) and then restore it when the app reopens on permissions change.

I'd also be curious to see if this is discussed in the React Navigation docs and perhaps has a prescribed solution?

DylanDylann commented 3 months ago

I think you're on the right track though - when triggering a permissions change modal, we want to stash the current navigation state (is the route enough?) and then restore it when the app reopens on permissions change.

Agree at this point, we already did that in the test branch by using a new Onyx key called lastScreen. In the PR phase, maybe we need to adjust to set the current route to lastScreen field when the user opens the permission setting and then use lastScreen to restore the old state

DylanDylann commented 3 months ago

I'd also be curious to see if this is discussed in the React Navigation docs and perhaps has a prescribed solution?

It sounds great to me

cc @ravindra-encoresky

ravindra-encoresky commented 3 months ago

I'd also be curious to see if this is discussed in the React Navigation docs and perhaps has a prescribed solution?

I have not found anything specific in React Navigation Docs but on multiple places same solution is recommended.

https://github.com/zoontek/react-native-permissions/issues/307 https://stackoverflow.com/questions/31706447/how-can-i-prevent-ios-apps-from-resetting-after-changing-camera-permissions

melvin-bot[bot] commented 3 months ago

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

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @ravindra-encoresky 🎉 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 📖

ravindra-encoresky commented 3 months ago

📣 @ravindra-encoresky 🎉 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 📖

Offer accepted. PR will be submitted by today 12 Aug 2024.

ravindra-encoresky commented 2 months ago

hi @m-natarajan @sakluger since the PR is done, could you please complete the job on Upwork ?

thanks.

DylanDylann commented 2 months ago

@sakluger The PR was deployed to production on August 31st. The payment should be issued on September 6th

ravindra-encoresky commented 2 months ago

@DylanDylann @sakluger thanks, waiting for the job to be completed on upwork.

sakluger commented 2 months ago

Thanks everyone and sorry for the delay - our usual automation failed, so I will handle the updates and payments manually now. @DylanDylann can you please let us know if you think we need regression test steps for this case?

sakluger commented 2 months ago

Summarizing payment on this issue:

Contributor: @ravindra-encoresky $250, paid via Upwork Contributor+: @DylanDylann $250, paid via Upwork

DylanDylann commented 2 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA [@DylanDylann] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA [@DylanDylann] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: NA [@DylanDylann] Determine if we should create a regression test for this bug. Yes [@DylanDylann] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal: Only for IOS native App

  1. Start Submit Expense flow from Global Create
  2. Tap on the Scan tab
  3. Go to the Phone settings from the system prompt asking for Camera permission
  4. Go back to the App by tapping on <- NewExpensify on the top left corner of the iPhone
  5. Verify that App navigate to scan screen

Do we agree 👍 or 👎

sakluger commented 2 months ago

Looks good. Thanks!