[$125] [Search v1.2] - Hold expense modal appears on the top left when expense is held in Search

[IuliiaHerets]

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

---

Version Number: 9.0.35-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to DM.
3. Submit two expenses.
4. Go to Search.
5. Click on the submitted expense.
6. Click on the header > Hold.
7. Enter reason and save it.

Expected Result:

Hold expense modal will open as RHP.

Actual Result:

Hold expense modal appears on the top left when expense is held in Search.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6604298_1726402698833.20240915_201411.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021835782276333225172
• Upwork Job ID: 1835782276333225172
• Last Price Increase: 2024-09-23
• Automatic offers:
• rojiphil | Reviewer | 104106001
• Krishna2323 | Contributor | 104106003

Issue Owner

Current Issue Owner: @rojiphil

[melvin-bot]

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.

[IuliiaHerets]

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[nkdengineer]

Proposal

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

Hold expense modal appears on the top left when expense is held in Search.

What is the root cause of that problem?

In here we are passing down the prop shouldUseNarrowLayout, and in web screen, its value will be true when we are in RHP on the Search page logic here. As a result, it will show this popover instead of redirecting to the route PROCESS_MONEY_REQUEST_HOLD.

App/src/components/MoneyReportHeader.tsx

Lines 255 to 262 in 84793c8

	if (shouldUseNarrowLayout) {
	if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD) {
	Navigation.goBack();
	}
	} else {
	Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD);
	}
	}, [shouldUseNarrowLayout, shouldShowHoldMenu]);

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

We should pass isSmallScreenWidth instead of shouldUseNarrowLayout in this case like we did here and here. And do the same with MoneyRequestHeader

          <MoneyReportHeader
              report={report}
              policy={policy}
              transactionThreadReportID={transactionThreadReportID}
              reportActions={reportActions}
              shouldUseNarrowLayout={isSmallScreenWidth}
              onBackButtonPress={onBackButtonPress}
          />

App/src/hooks/useResponsiveLayout/index.ts

Line 68 in 84793c8

const shouldUseNarrowLayout = isSmallScreenWidth || isInNarrowPaneModal;

What alternative solutions did you explore? (Optional)

NA

Screen.Recording.2024-09-16.at.22.22.28.mov

[Krishna2323]

Proposal


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

Search - Hold expense modal appears on the top left when expense is held in Search

What is the root cause of that problem?

• shouldUseNarrowLayout is used to display the modal and shouldUseNarrowLayout will be true even if isInNarrowPaneModal is true.
  

  App/src/components/MoneyReportHeader.tsx

  Line 425 in 8e3821c

  {shouldUseNarrowLayout && shouldShowHoldMenu && (

  
  

  App/src/hooks/useResponsiveLayout/index.ts

  Lines 60 to 68 in 8e3821c

  	// The component calling this hook is in a "narrow pane modal" if:
  	const isInNarrowPaneModal =
  	// it's a child of the right-docked modal
  	activeModalType === CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED ||
  	// or there's a "right modal navigator" or "left modal navigator" on the top of the root navigation stack
  	// and the component calling this hook is not the child of another modal type, such as a confirm modal
  	(isDisplayedInNarrowModalNavigator && !activeModalType);
  	const shouldUseNarrowLayout = isSmallScreenWidth || isInNarrowPaneModal;

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


• In MoneyReportHeader & MoneyRequestHeader we should get isSmallScreenWidth from useResponsiveLayout hook and use that instead of shouldUseNarrowLayout.

• We also need to update the shouldUseNarrowLayout in the useEffect in both components.
  

  App/src/components/MoneyReportHeader.tsx

  Lines 250 to 262 in 8e3821c

  	useEffect(() => {
  	if (!shouldShowHoldMenu) {
  	return;
  	}
  	if (shouldUseNarrowLayout) {
  	if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD) {
  	Navigation.goBack();
  	}
  	} else {
  	Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD);
  	}
  	}, [shouldUseNarrowLayout, shouldShowHoldMenu]);

Note

We should not pass shouldUseNarrowLayout={isSmallScreenWidth} to MoneyReportHeader & MoneyRequestHeader, passing isSmallScreenWidth to shouldUseNarrowLayout will cause issues as it is being also used for showing back button and money request buttons according to the IOU report layout size.

What alternative solutions did you explore? (Optional)

Result

[sakluger]

This feels like a fairly minor issue and change, so I will set the price at $125. Feel free to leave a note if you disagree.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $125

[sakluger]

@luacmartins will you be fixing this one yourself? If so, we can remove the help wanted and external labels.

[luacmartins]

@sakluger no, I just assigned myself to make sure the proposal we select doesn't conflict with other Search related issues.

[melvin-bot]

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

[sakluger]

Hey @rojiphil could you please review the two proposals from last week? Thanks!

[rojiphil]

Thanks for the proposals.

@Krishna2323 I have a couple of questions for you:

We also need to update the shouldUseNarrowLayout in the useEffect in both components.

What code changes are you suggesting here and what would be the benefit of making these changes?

• In MoneyReportHeader & MoneyRequestHeader we should get isSmallScreenWidth from useResponsiveLayout hook and use that instead of shouldUseNarrowLayout.

passing isSmallScreenWidth to shouldUseNarrowLayout will cause issues as it is being also used for showing back button and money request buttons according to the IOU report layout size.

Both the above statements seem to contradict to each other. Are you not suggesting to pass isSmallScreenWidth to shouldUseNarrowLayout?

[Krishna2323]

Both the above statements seem to contradict to each other. Are you not suggesting to pass isSmallScreenWidth to shouldUseNarrowLayout?

@rojiphil, I mean we should not change the shouldUseNarrowLayout prop passed to MoneyReportHeader & MoneyRequestHeader instead we should get isSmallScreenWidth from useResponsiveLayout hook directly in MoneyReportHeader&MoneyRequestHeader ` and use that to show the modal.

App/src/pages/home/ReportScreen.tsx

Lines 316 to 323 in a99cf94

	<MoneyReportHeader
	report={report}
	policy={policy}
	transactionThreadReportID={transactionThreadReportID}
	reportActions={reportActions}
	shouldUseNarrowLayout={shouldUseNarrowLayout}
	onBackButtonPress={onBackButtonPress}
	/>

We also need to update the shouldUseNarrowLayout in the useEffect in both components.

You can check this PR, I think the author of the PR mistakenly replaced isSmallScreenWidth with shouldUseNarrowLayout . We should also look for similar cases in these changed files.

[rojiphil]

@Krishna2323

instead we should get isSmallScreenWidth from useResponsiveLayout hook directly in MoneyReportHeader&MoneyRequestHeader ` and use that to show the modal.

Hmm.. Isn’t this what @nkdengineer proposed earlier?

You can check this PR, I think the author of the PR mistakenly replaced isSmallScreenWidth  with shouldUseNarrowLayout . We should also look for similar cases in these changed files.

Ah! I see. So, you are referring to replacing shouldUseNarrowLayout with isSmallScreenWidth. But what additional value does it bring since the proposal already includes assigning isSmallScreenWidth to shouldUseNarrowLayout?

[Krishna2323]

@rojiphil, it purposed to update the shouldUseNarrowLayout prop which will cause regressions as it is also used to show the buttons and adjust the layout for narrow width.

App/src/pages/home/ReportScreen.tsx

Lines 295 to 303 in a99cf94

	if (isSingleTransactionView && report) {
	headerView = (
	<MoneyRequestHeader
	report={report}
	policy={policy}
	parentReportAction={parentReportAction}
	shouldUseNarrowLayout={shouldUseNarrowLayout}
	onBackButtonPress={onBackButtonPress}
	/>

[rojiphil]

Thanks. I get what you are saying now. I also think it is better to use isSmallScreenWidth from useResponsiveLayout hook directly in MoneyReportHeader & MoneyRequestHeader and fix the navigation issue here.
@Krishna2323 proposal LGTM.
🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

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

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

[sakluger]

@Krishna2323 do you have a PR for this one yet?

[sakluger]

Bump @Krishna2323.

[Krishna2323]

PR will be up within 24 hours.

[melvin-bot]

@rojiphil, @sakluger, @luacmartins, @Krishna2323 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Krishna2323]

@sakluger @rojiphil, this issue has been fixed in this PR. The comment mentioning the this bug the PR.

cc: @bernhardoj @rayane-djouah

[rojiphil]

this issue has been fixed in this PR

Oh! Then, let's close this issue and move on. cc @sakluger