[HOLD for payment 2023-11-22] [$500] Web - Opening split bill amount link displays no header

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app
2. Click on plus and click on split bill
3. Enter any amount and click on Next
4. Select any users and click on Next
5. Open amount tab and copy the link, link should look like: https://staging.new.expensify.com/split/new/amount/
6. Close RHN and paste the link in any chat report
7. Visit that chat report and click on the link
8. Observe that there is no header displayed to amount page

Expected Result:

App should display 'Amount' with back button in header if we visit split bill amount by deep link OR app should redirect to first step of split bill and display 'Split bill' with back button in header.

Actual Result:

On opening split bill amount page using deep link, app displays no header to the page

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.61.0
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
Notes/Photos/Videos: Any additional supporting documentation

no.header.split.bill.amount.page.mp4

Recording.4153.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692083367582229

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0156efafedabaef820
• Upwork Job ID: 1698790076926279680
• Last Price Increase: 2023-09-25
• Automatic offers:
• 0xmiroslav | Reviewer | 27456133
• GItGudRatio | Contributor | 27456135
• dhanashree-sawant | Reporter | 27456137

Issue Owner

Current Issue Owner: @anmurali

Issue Owner

Current Issue Owner: @anmurali

Issue Owner

Current Issue Owner: @anmurali

Issue Owner

Current Issue Owner: @anmurali

[GItGudRatio]

Proposal

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

Opening split bill or request amount link displays no header.

What is the root cause of that problem?

To determine if we are editing the amount or creating a new request, we check if route.path includes the string amount. When the navigate to the edit amount page using the Amount tab on confirmation page, the Navigation.navigate method is used, which adds the route.path variable.

When we navigate to the link directly, route.path is undefined. This causes the app to assume we are creating a new request instead and the issue is noticed.

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

In NewRequestAmountPage, instead of using route.path to determine the isEditing variable, we should use Navigation.getActiveRoute() instead.

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

Line 82 in 643e2e8

const isEditing = lodashGet(route, 'path', '').includes('amount');

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[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

[anmurali]

Can reproduce and is a bug

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0156efafedabaef820

[melvin-bot]

Current assignee @anmurali is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[Tony-MK]

Proposal

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

Opening split bill amount link displays no header

What is the root cause of that problem?

The root cause of the problem is that NewRequestAmountPage checks if the the bill is being editing by checking is the path includes the keyword amount from the route.path value in the component props.

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

Line 66 in c9dc4ba

const isEditing = lodashGet(route, 'path', '').includes('amount');

If the bill is being edited the NewRequestAmountPage renders without the header because the MoneyRequestSelectorPage has a header so only MoneyRequestAmountForm is returned.

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

Lines 157 to 172 in c9dc4ba

	const content = (
	<MoneyRequestAmountForm
	isEditing={isEditing}
	currency={currency}
	amount={iou.amount}
	ref={(e) => (textInput.current = e)}
	onCurrencyButtonPress={navigateToCurrencySelectionPage}
	onSubmitButtonPress={navigateToNextPage}
	/>
	);
	// ScreenWrapper is only needed in edit mode because we have a dedicated route for the edit amount page (MoneyRequestEditAmountPage).
	// The rest of the cases this component is rendered through <MoneyRequestSelectorPage /> which has it's own ScreenWrapper
	if (!isEditing) {
	return content;
	}

However when clicking the link, the route.path props for the NewRequestAmountPage is not available therefore the header doesn't get rendered.

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

While solving this problem, I assumed that the back button in the header will go back to wherever the user was, not the split page.

I believe replacing lodashGet(route, 'path', '') with window.location.pathname, in the line below, would get the correct path and the isEditing boolean will be true when the user clicks on the link.

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

Line 66 in c9dc4ba

const isEditing = lodashGet(route, 'path', '').includes('amount');

Result
https://github.com/Expensify/App/assets/22982173/8841d0a1-15bf-4bf0-adc6-d59d16d4c8d3

What alternative solutions did you explore? (Optional)

Instead of using window.location.pathname, we can the hook useLocation() to the get current path.

[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]

@anmurali, @0xmiroslav Still overdue 6 days?! Let's take care of this!

[anmurali]

@0xmiroslav can you evaluate that proposal above?

[0xmiros]

@Tony-MK thanks for the proposal. This also happens on native so we cannot use window.location.pathname.

[Tony-MK]

@0xmiroslav Sorry about that and thanks for the reviewing my proposal. Should I update my proposal to use either the useLocation or useRoute to get the current path?

[0xmiros]

@GItGudRatio thanks for the proposal. In your root cause, this is actually wrong.

When we navigate to the link directly, route.path is undefined

As you see my video, this happens only when click the link in chat. If you open link in another tab or refresh, it's redirected to /split/new. Can you please investigate why?

Screen.Recording.2023-09-13.at.6.56.55.PM.mov

[0xmiros]

@0xmiroslav Sorry about that and thanks for the reviewing my proposal. Should I update my proposal to use either the useLocation or useRoute to get the current path?

@Tony-MK same feedback above. For proposal to be accepted, both root cause and solution should be correct.

[GItGudRatio]

@0xmiroslav Thank you for clarifying, great question. Let me expand a bit more on the proposal.

The root cause still remains the same, when we click on a link sent in the chat, the route.path param is undefined. The solution is the same as well.

this happens only when click the link in chat. If you open link in another tab or refresh, it's redirected to /split/new.

As for why this only happens when we click on the link in chat, this is because since route.path is undefined, the parameter isEditing is false. This leads to the assumption that we are already on the new amount page. In this case, we do not navigate back to /split/new. This can further be confirmed as when the page is rendered, the text on the bottom button is Next, which is related to a new split bill request, instead of Save, which should be the case when we are editing a request.

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

Line 66 in 4ff85db

const isEditing = lodashGet(route, 'path', '').includes('amount');

The reason this happens is because we only redirect to the /split/new page if we assume we are on the editing page. The relevant code is here:

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

Lines 102 to 121 in 4ff85db

	if (isEditing) {
	// ID in Onyx could change by initiating a new request in a separate browser tab or completing a request
	if (prevMoneyRequestID.current !== iou.id) {
	// The ID is cleared on completing a request. In that case, we will do nothing.
	if (!iou.id) {
	return;
	}
	Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType, reportID), true);
	return;
	}
	const moneyRequestID = `${iouType}${reportID}`;
	const shouldReset = iou.id !== moneyRequestID;
	if (shouldReset) {
	IOU.resetMoneyRequestInfo(moneyRequestID);
	}
	if (!isDistanceRequestTab && (_.isEmpty(iou.participants) || iou.amount === 0 || shouldReset)) {
	Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType, reportID), true);
	}
	}

[melvin-bot]

@anmurali @0xmiroslav 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!

[0xmiros]

reviewing today

[0xmiros]

@Pluto0104 thanks for the proposal. The root cause is correct.

Specifically, when the action navigates to the RightModalNavigator and the last route on the root navigator is not RightModalNavigator, it initiates a push action. However, in this case, the route prop does not contain a path, resulting in isEditing being false, which leads to the absence of a header.

Can we fix this in navigation level?

the route prop does not contain a path

[GItGudRatio]

PR is up! Please help review, @0xmiroslav

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.99-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Opening split bill amount link displays no header #30704

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @GItGudRatio requires payment offer (Contributor)
• @0xmiroslav requires payment offer (Reviewer)
• @dhanashree-sawant requires payment offer (Reporter)

[melvin-bot]

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:

• [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav] 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:
• [@0xmiroslav] 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:
• [@0xmiroslav] Determine if we should create a regression test for this bug.
• [@0xmiroslav] 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.
• [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@puneetlath, @anmurali, @GItGudRatio, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

[0xmiros]

trying to find offending PR

[GItGudRatio]

@0xmiroslav any updates?

[melvin-bot]

@puneetlath, @anmurali, @GItGudRatio, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

Let's try to get this one closed out asap. @0xmiroslav still looking?

[0xmiros]

• The PR that introduced the bug has been identified. Link to the PR: Refactor/23149 money request page #23979
• 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: https://github.com/Expensify/App/pull/23979/files#r1412506814
• 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: N/A
• Determine if we should create a regression test for this bug.
• 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

1. Go to FAB > Request money > Manual
2. Enter any amount and click on next
3. Select any participant
4. Click on Amount field
5. Copy the browser link and send it in any chat
6. Click on the link
7. Verify that the header is shown properly

I think this flow should be considered on Tim's money request flow refactor

[puneetlath]

Ok great. Seems that all that is left is for @anmurali to pay then.

[melvin-bot]

@puneetlath, @anmurali, @GItGudRatio, @0xmiroslav Still overdue 6 days?! Let's take care of this!

[0xmiros]

Not overdue

[anmurali]

Paid.