lanitochka17 commented 10 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: 1.4.3-6 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: Applause - Internal Team Slack conversation:

Action Performed:

Go to FAB > Request money
Switch to Manual
Enter amount > Next
Select a user
On the confirm page go to 'more', the three dots at the top
Add receipt
On the Receipt page, Reload the page
Click the back button, and notice you are retaken to the receipt page
Click the back button again

Expected Result:

Navigates back to the previous page or reloading takes you to the Request money page

Actual Result:

After Reloading on the Receipt page clicking the back button retakes you to the receipt page again and clicking back again takes you to the main chat

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/4b5ccc8f-e12f-45b5-aff2-03e090c11eed

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01845ea13bf2a2f8c4
Upwork Job ID: 1728472644379082752
Last Price Increase: 2023-12-02

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01845ea13bf2a2f8c4

melvin-bot[bot] commented 10 months ago

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

ishpaul777 commented 10 months ago

Proposal

Problem

After Reloading on Receipt page the back button takes you to main chat

Root Cause

In EditRequestReceiptPage, for onBackButtonPress in HeaderWithBackButton we we dont provide a fallback route so it just links back to the home when the route is first in the modal stack after reload.

Changes

We should set a backTo param while navigating to the receipt selector from money request confirm page and provide it as fallback route in goBack,so when app is reloaded and back button links to correct page.

Note: using a definte fallback route or setting fallback based on condition is not a good idea when we integrate edit receipt page to some other flow conditions may fail and and component needs a refactor again and i dont think thats a good idea.

DylanDylann commented 10 months ago

Proposal

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

Request Money - After Reloading on Receipt page the back button takes you to main chat
What is the root cause of that problem?
In here: https://github.com/Expensify/App/blob/d985a0ee2604686911f34c99f5298af059622244/src/pages/EditRequestReceiptPage.js#L50 we do not pass the fallback route, so after refresh, and then pressing on bac button, the app is just take user to main chat.
What changes do you think we should make in order to solve the problem?

We need to create the fallbackRoute variable in EditRequestReceiptPage like below:

const fallBackRoute = transactionID ? ' ' : ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(iou.iouType, iou.iouReportID);   // transactionID to check if it is edit request`s receipt or add a receipt to new request flow

Then update: https://github.com/Expensify/App/blob/d985a0ee2604686911f34c99f5298af059622244/src/pages/EditRequestReceiptPage.js#L50 to:
```
onBackButtonPress={() => Navigation.goBack(fallBackRoute)}
```
iou.iouType and iou.iouReportID are used to store the previous iouType and reportID (before refreshing the page). We can update these variable by updating: https://github.com/Expensify/App/blob/d985a0ee2604686911f34c99f5298af059622244/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L88-L92 to:
```
const navigateToConfirmationStep = (moneyRequestType) => {
    IOU.setMoneyRequestId(moneyRequestType);
    IOU.resetMoneyRequestCategory();
    IOU.resetMoneyRequestTag();
```
IOU.setIouType(moneyRequestType);
IOU.setIouReportID(reportID); Navigation.navigate(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(moneyRequestType, reportID)); };

with

function setIouType(iouType) {
    Onyx.merge(ONYXKEYS.IOU, {iouType});
}

function setIouReportID(iouReportID) {
    Onyx.merge(ONYXKEYS.IOU, {iouReportID});
}

What alternative solutions did you explore? (Optional)

NA

giltron commented 10 months ago

Proposal

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

The Back button does not behave as expected on page refresh on the desktop. The back button must be pressed twice and the 2nd time pressed it dimisses the Request Money prompt.

What is the root cause of that problem?

There are 2 issues that need to be addressed: 1) The URL changes on refresh, and 2) there is missing fallback route for the onBack

1) When refreshing the URL changes (trailing slash is lost and adds another route with the same structure)

In ROUTES.ts it should not add a trailing / to multiple routes when no reportID is present. This is not needed and not consistent with other URLS.

Before refresh the url is: https://dev.new.expensify.com:8082/request/new/receipt/

After refresh the url is: https://dev.new.expensify.com:8082/request/new/receipt

Note: I was able to reproduce the same double back press behaviour on the MONEY_REQUEST_CONFIRMATION (/request/new/confirmation) route.

2) There is no fallbackRoute passed on EditRequestReceiptPage's onBackButtonPress

<HeaderWithBackButton
    title={translate('common.receipt')}
    onBackButtonPress={Navigation.goBack}
/>

Without 1) the same navigation issue happens on the route request/new/confirmation if you enter the details,

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

1) In ROUTES.js we can add a function to only conditionally add the trailing / to the URL:

function urlWithSubResource(baseUrl: string, id?: string)

So instead of

MONEY_REQUEST_RECEIPT: {
        route: ':iouType/new/receipt/:reportID?',
        getRoute: (iouType: string, reportID = '') => `${iouType}/new/receipt/${reportID}`,
    },

with result: /request/new/receipt/

It becomes

MONEY_REQUEST_RECEIPT: {
        route: ':iouType/new/receipt/:reportID?',
        getRoute: (iouType: string, reportID = '') =>  urlWithSubResource(`${iouType}/new/receipt`, reportID),
    },

with result: /request/new/receipt

The same should be appled to MONEY_REQUEST_CONFIRMATION (/request/new/confirmation) to ensure it does not suffer from the same browser refresh/back button issue.

2) ADd the fallbackRoute passed to EditRequestReceiptPage's onBackButtonPress

<HeaderWithBackButton
    title={translate('common.receipt')}
    onBackButtonPress={() => Navigation.goBack(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(route.params.iouType, route.params.reportID))}
/>

Alternative Solutions

On browser refresh, close the right hand panel

codeweb05 commented 10 months ago

Issue Description

Problem

After reloading the Receipt page, the back button redirects users to the main chat instead of the expected behavior of navigating back to the previous page.

Root Cause

In the EditRequestReceiptPage, the onBackButtonPress function in HeaderWithBackButton does not provide a fallback route. This results in the back button linking to the home page when the route is first in the modal stack after a reload.

Proposed Solutions

Proposal 1

Problem

After reloading the Receipt page, the back button redirects users to the main chat.

Root Cause

In EditRequestReceiptPage.js, the onBackButtonPress in HeaderWithBackButton lacks a fallback route, causing the app to redirect to the main chat after a refresh.

Changes

Set a backTo parameter when navigating to the receipt selector from the money request confirm page. Provide it as a fallback route in goBack to ensure the correct page is linked to when the app is reloaded and the back button is clicked.

Proposal 2

Problem

After reloading on the Receipt page, the back button redirects users to the main chat.

Root Cause

In EditRequestReceiptPage.js, the onBackButtonPress function lacks a fallback route, causing the app to redirect to the main chat after a refresh.

Changes

Create a fallbackRoute variable in EditRequestReceiptPage. Update the onBackButtonPress to include the fallback route, ensuring that after a reload, the back button links to the correct page.

Proposal 3

Problem

The back button does not behave as expected on page refresh on the desktop. It must be pressed twice, and the second press dismisses the Request Money prompt.

Root Cause

The URL changes on refresh, losing the trailing slash and adding another route with the same structure.
There is no fallback route passed on EditRequestReceiptPage's onBackButtonPress.

Changes

In ROUTES.js, add a function to conditionally add the trailing slash to the URL. Apply this to the MONEY_REQUEST_RECEIPT and MONEY_REQUEST_CONFIRMATION routes.
Add the fallbackRoute parameter to EditRequestReceiptPage's onBackButtonPress to ensure proper navigation after a reload.

Alternative Solutions

On browser refresh, close the right-hand panel.

Conclusion

Choose the proposal that best aligns with the codebase and development standards. Ensure that the chosen solution addresses both the reloading issue and the back button behavior. Implement the proposed changes and thoroughly test to guarantee the desired outcomes.

stephanieelliott commented 10 months ago

@thesahindia some proposals to review here -- can you give feedback when you get a sec?

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

thesahindia commented 9 months ago

@ishpaul777's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 9 months ago

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

DylanDylann commented 9 months ago

@thesahindia I think we have a lot of flow that can lead to the Receipt page, so we need to handle these case

DylanDylann commented 9 months ago

@thesahindia We have the similar case here https://github.com/Expensify/App/pull/31658

ishpaul777 commented 9 months ago

i think the most used pattern in app is using backto param we already use in countrypicker, bank account flow and many other common pages in different flows its simple and safer for future reusability of the component.

DylanDylann commented 9 months ago

@cristipaval There are the two solutions, what do you think about it:

@ishpaul777 `s proposal stores the information about the previous page in the URL path and based on it, navigates the user to the correct screen. (Approved by C+, but I think it will have issue if user edit the URL and open the new tab without any params)
@DylanDylann `s proposal is based on the iou data which are stored in Onyx to navigate users to the correct screen. (This also use with the Referral Page (see https://github.com/Expensify/App/pull/31658 )

ishpaul777 commented 9 months ago

a minor clarification: if user is opening the url to another window without backto it should be somewhat expected to user that flow will not be the same as with backto param and practically this is very edge case, and we might already have considered and agreed on the case when accepting the pattern for other pages.

melvin-bot[bot] commented 9 months ago

❌ There was an error making the offer to @ishpaul777 for the Contributor role. The BZ member will need to manually hire the contributor.

cristipaval commented 9 months ago

Thanks for raising your concerns, @DylanDylann! I wouldn't worry about the corner case you mentioned. I wouldn't underestimate the user, and they would expect a different behavior if they changed the URL.

stephanieelliott commented 9 months ago

@ishpaul777 I've sent you the offer on Upwork, please accept when you get a chance!

melvin-bot[bot] commented 9 months ago

@cristipaval @stephanieelliott @thesahindia @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

ishpaul777 commented 9 months ago

I was about to create a PR but it looks the issue is resolved on staging already, using the same change as proposed, by another PR i think we can close @cristipaval @stephanieelliott

cristipaval commented 9 months ago

Thanks @ishpaul777!

Expensify / App

[$500] Request Money - After Reloading on Receipt page the back button takes you to main chat #31869

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

Problem

Root Cause

Changes

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

Alternative Solutions

Issue Description

Problem

Root Cause

Proposed Solutions

Proposal 1

Problem

Root Cause

Changes

Proposal 2

Problem

Root Cause

Changes

Proposal 3

Problem

Root Cause

Changes

Alternative Solutions

Conclusion