[$500] IOU - Amount page reloaded when navigate back from final confirmation and click next btn

[izarutskaya]

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.3.87-1
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:

Precondition: user should be signed in

1. Open app
2. Click on FAB
3. Select the Request Money
4. Select Manual tab
5. Write any amount and click Next button
6. Select any attendees (two or more)
7. Click back button twice in a row
8. Click next button

Expected Result:

User should be navigated on the select attendees page

Actual Result:

The same page reloaded with default amount (the default amount is zero)

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Recording.1775.mp4

MacOS: Desktop

Bug6243732_1697763904692.Recording__419.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014945c77a33a9f852
• Upwork Job ID: 1715293865929134080
• Last Price Increase: 2023-11-03
• Automatic offers:
• jjcoffee | Reviewer | 27561168
• graylewis | Contributor | 27561170

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014945c77a33a9f852

[melvin-bot]

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]

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

[graylewis]

Proposal

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

When using the IOU manual request flow, attempting to go back to the requestMoneyAmount page and then moving forward again breaks the flow and resets the request

What is the root cause of that problem?

Once you reach the confirmation page, the route of the app changes from /request to /split

On the participants page:
http://localhost:8082/request/new/participants/

On the confirmation page:
http://localhost:8082/split/new/confirmation/

Because the ID of a request is generated from the route, the safety measure preventing two separate IOUs on different tabs from interfering with each other is triggered, due to the conflict between IDs. This causes the request to be reset.

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

Currently, hitting the back button in this flow triggers a typical back action. We can maintain the ID properly by enforcing a fallback route rather than allowing the browser to determine which route to navigate back to.

The navigateBack function for the confirmation page before (note that the Navigation.goBack() function provides a fallback, but doesn't supply the optional second parameter to enforce navigation to the fallback):

    const navigateBack = () => {
        let fallback;
        if (reportID.current) {
            fallback = ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current);
        } else {
            fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType.current);
        }
        Navigation.goBack(fallback);
    };

The navigateBack function after (note the optional second parameter enforcing the fallback route in the Navigation.goBack call:

    const navigateBack = () => {
        let fallback;
        if (reportID.current) {
            fallback = ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current);
        } else {
            fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType.current);
        }
        Navigation.goBack(fallback, true);
    };

I've tested this on my local machine, and it fixes the problem perfectly. I can't think of any edge cases which could cause this not to work, as no other pages navigate directly to the end of the manual request flow.

We could also conditionally enforce the fallback route when the iouType is not "request", and it would work the same but maintain the current approach when it makes sense.

    const navigateBack = () => {
        let fallback;
        const shouldUseFallback = (iouType.current !== "request")
        if (reportID.current) {
            fallback = ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current);
        } else {
            fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType.current);
        }
        Navigation.goBack(fallback, shouldUseFallback);
    };

[dukenv0307]

Proposal

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

Amount page reloaded when navigate back from final confirmation and click next btn

What is the root cause of that problem?

After we choose the split, we navigate back with shouldEnforceFallback as false that makes the iouType in the params changed from split to request. And then when we click on the next btn again, the id is stored in iou is split that is different from the id from the route that is request. So the amount page is reset again.

App/src/pages/iou/steps/MoneyRequestConfirmPage.js

Line 146 in 69cf29e

Navigation.goBack(fallback);

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

We should call goBack function with the shouldEnforceFallback param as true

Navigation.goBack(fallback, true);

App/src/pages/iou/steps/MoneyRequestConfirmPage.js

Line 146 in 69cf29e

Navigation.goBack(fallback);

onBackButtonPress={() => navigateBack(true)}

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 117 in 69cf29e

onBackButtonPress={navigateBack}

What alternative solutions did you explore? (Optional)

[FitseTLT]

Proposal

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

Amount page re-navigating to itself when user comes back from confirmation page of a split request

What is the root cause of that problem?

The root cause of the problem is when a user split the iou by pressing split button on a user on participants page and navigates to the confirmation page iou.id is changed to 'split' so if he navigates from the confirmation page -> participants page -> amount page and enters amount and presses next, the code below will reset iou b/c there is a mismatch between iou.id('split' set when we navigated to confirmation page) and iouType('request'/'send' according to the params in route) by this line of code

App/src/libs/actions/IOU.js

Lines 2899 to 2907 in 5d45f0f

	function navigateToNextPage(iou, iouType, report, path = '') {
	const moneyRequestID = `${iouType}${report.reportID || ''}`;
	const shouldReset = iou.id !== moneyRequestID;
	// If the money request ID in Onyx does not match the ID from params, we want to start a new request
	// with the ID from params. We need to clear the participants in case the new request is initiated from FAB.
	if (shouldReset) {
	resetMoneyRequestInfo(moneyRequestID);
	}

Then the page will navigate to participants page but immediately this effect on the code below will make it to navigate back to amount page because iou is reset and iou.amount is 0.

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Lines 103 to 105 in 5d45f0f

	if (!isDistanceRequest && ((iou.amount === 0 && !iou.receiptPath) || shouldReset)) {
	navigateBack(true);
	}

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

The perfect solution is to revert back the application state when the user navigates back to participants from confirmation page so that there will be no confusion and unexpected behaviour of the application. I choose this approach because it will allow us to leave other working features of the existing code to be intact and fix this specific issue. When the user select the split participant and presses the add to split button only the iou.id is changed to 'split' so if the user navigates back to participants page reverting it back to previous state (iou.id) will be the appropriate solution that won't affect the already working code.
So we need to add this code to MoneyRequestParticipantsPage to revert back the iou.id when the user navigates back to the participants page ( the screen is focused back) to the value corresponding to the iouType found from the route params

    const isFocused = useIsFocused();

    useEffect(() => {
        const moneyRequestId = `${iouType}${reportID}`;
        const shouldReset = iou.id !== moneyRequestId;
        if (shouldReset && isSplitRequest && isFocused) {
            IOU.setMoneyRequestId(moneyRequestId);
        }
    }, [isFocused]);

Then we should change this code

App/src/pages/iou/steps/MoneyRequestConfirmPage.js

Lines 113 to 122 in 5d45f0f

	useEffect(() => {
	// ID in Onyx could change by initiating a new request in a separate browser tab or completing a request
	if (!isDistanceRequest && prevMoneyRequestId.current !== props.iou.id) {
	// The ID is cleared on completing a request. In that case, we will do nothing.
	if (props.iou.id) {
	Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID), true);
	}
	return;
	}

to

    const isFocused = useIsFocused();

    useEffect(() => {
        // ID in Onyx could change by initiating a new request in a separate browser tab or completing a request
        if (!isDistanceRequest && prevMoneyRequestId.current !== props.iou.id) {
            // The ID is cleared on completing a request. In that case, we will do nothing.
            if (props.iou.id && isFocused) {
                Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID), true);
            }
            return;
        }

so that Navigation.goBack function is not called immediately after it navigates back from the confirmation page because the participants page effect we have added above would run and change the iou.id and because at that time this page (confirmation page) is still not unmounted because it takes some time to finish the animation. But this change will not affect the previous purpose of the code which is to navigate it back when another type of iou is created on another tab as it is supposed to run if the screen is on focus.(I also tested it 👍 ).

POC:

2023-11-01.13-13-17.mp4

What alternative solutions did you explore? (Optional)

[graylewis]

This is related to #29972 and I recommend the same solution I gave there that will solve both of the issues.

I wouldn't say this solution for #29972 actually solves this issue. It would stop the request amount from being reset but the rest of the stored information (participants, description, etc.) would be reset upon navigating back to the start of the user flow. Since the route in this case is supposed to act as the source of truth for the state of the app, it makes more sense to reflect the state of the app accurately in the route.

[FitseTLT]

This is related to #29972 and I recommend the same solution I gave there that will solve both of the issues.

I wouldn't say this solution for #29972 actually solves this issue. It would stop the request amount from being reset but the rest of the stored information (participants, description, etc.) would be reset upon navigating back to the start of the user flow. Since the route in this case is supposed to act as the source of truth for the state of the app, it makes more sense to reflect the state of the app accurately in the route.

We can also reset only id and other necessary props of the iou in here

App/src/libs/actions/IOU.js

Lines 2863 to 2871 in eb5fec2

	function navigateToNextPage(iou, iouType, report, path = '') {
	const moneyRequestID = `${iouType}${report.reportID || ''}`;
	const shouldReset = iou.id !== moneyRequestID;
	// If the money request ID in Onyx does not match the ID from params, we want to start a new request
	// with the ID from params. We need to clear the participants in case the new request is initiated from FAB.
	if (shouldReset) {
	resetMoneyRequestInfo(moneyRequestID);
	}

But my logic is this function is called after amount is already input by the user (whether from manual, distance, ...) so resetting iou.amount because there is a mismatch b/n iouType and iou.id is not logical and that's why the app re-navigates to the amount page.

[melvin-bot]

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

[peterdbarkerUK]

I do wonder if this issue is related to/derivative of #29972, so I've asked @Ollyws to take a look

[melvin-bot]

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

[melvin-bot]

@peterdbarkerUK, @jjcoffee Huh... This is 4 days overdue. Who can take care of this?

[peterdbarkerUK]

Ollyws confirmed this is separate!

@jjcoffee could you review the proposals?

[FitseTLT]

@peterdbarkerUK @jjcoffee please check my updated proposal

[jjcoffee]

Reviewing tomorrow!

[jjcoffee]

@graylewis's proposal LGTM! The RCA is correct and the solution seems appropriate. Just a side note though, it's much easier to review your proposal if you provide links to the parts of code you're talking about (see @dukenv0307's proposal for inspiration!).

🎀👀🎀 C+ reviewed

[melvin-bot]

📣 @jjcoffee 🎉 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]

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

[peterdbarkerUK]

Assigned to @graylewis, with an extra point towards the proposal feedback here, and the additional consideration to account for here.

[jjcoffee]

Actually we might need to put this one on hold for #28618 as this would be fixed there. I'm unsure how quickly that will be rolled out though, so depends on whether or not we want a fix in the meantime. cc @peterdbarkerUK

[graylewis]

@jjcoffee @peterdbarkerUK I'm a little confused. I'm already hired on upwork to handle the fix. Should I just go ahead and fix it or should I hold off?

[jjcoffee]

Let's wait on @peterdbarkerUK's response for now. Sorry for the confusion!

[melvin-bot]

@graylewis @peterdbarkerUK @jjcoffee @roryabraham this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot]

@graylewis @peterdbarkerUK @jjcoffee @roryabraham this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[peterdbarkerUK]

@graylewis I'm sorry for the confusion! Let's put this on hold for #28618!

[roryabraham]

Still on HOLD

[paultsimura]

This looks very much like a duplicate of #28129, which was created earlier and was on hold as well up until now.
Btw, it has a quite similar proposal to the one accepted here, which also was posted earlier.

[roryabraham]

Still on HOLD

[roryabraham]

Looks like this can come off HOLD

[jjcoffee]

No longer reproducible. Let's close this @peterdbarkerUK or @roryabraham