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

IuliiaHerets commented 2 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.35-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to DM.
Submit two expenses.
Go to Search.
Click on the submitted expense.
Click on the header > Hold.
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
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Bug6604298_1726402698827!Screenshot_2024-09-15_at_20 14 25

https://github.com/user-attachments/assets/bcaf246b-fb4c-40f8-89da-d4f12bc4be8f

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[bot] commented 2 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.

IuliiaHerets commented 2 months ago

@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 commented 2 months ago

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.

https://github.com/Expensify/App/blob/84793c85836be656255230985dc68e12e37d6268/src/components/MoneyReportHeader.tsx#L255-L262

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}
          />

https://github.com/Expensify/App/blob/84793c85836be656255230985dc68e12e37d6268/src/hooks/useResponsiveLayout/index.ts#L68

What alternative solutions did you explore? (Optional)

NA

https://github.com/user-attachments/assets/2f365f40-ad45-4d92-a88f-a286b77eb5c6

Krishna2323 commented 2 months ago

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. https://github.com/Expensify/App/blob/8e3821c3508877ac0045b040ed6b0d595a693d68/src/components/MoneyReportHeader.tsx#L425 https://github.com/Expensify/App/blob/8e3821c3508877ac0045b040ed6b0d595a693d68/src/hooks/useResponsiveLayout/index.ts#L60-L68

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. https://github.com/Expensify/App/blob/8e3821c3508877ac0045b040ed6b0d595a693d68/src/components/MoneyReportHeader.tsx#L250-L262

[!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 commented 2 months ago

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[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $125

sakluger commented 2 months ago

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

luacmartins commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sakluger commented 1 month ago

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

rojiphil commented 1 month ago

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 commented 1 month ago

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.

https://github.com/Expensify/App/blob/a99cf941a355a412e7498c41a69a99b01e35431c/src/pages/home/ReportScreen.tsx#L316-L323

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 commented 1 month ago

@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 commented 1 month ago

@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.

https://github.com/Expensify/App/blob/a99cf941a355a412e7498c41a69a99b01e35431c/src/pages/home/ReportScreen.tsx#L295-L303

rojiphil commented 1 month ago

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[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @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[bot] commented 1 month ago

📣 @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 commented 1 month ago

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

sakluger commented 1 month ago

Bump @Krishna2323.

Krishna2323 commented 1 month ago

PR will be up within 24 hours.

melvin-bot[bot] commented 1 month ago

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

Krishna2323 commented 1 month ago

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

cc: @bernhardoj @rayane-djouah

rojiphil commented 1 month ago

this issue has been fixed in this PR

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

Expensify / App