Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page #44337

Closed
6 tasks done
lanitochka17 opened this issue Jun 24, 2024 · 21 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 24, 2024

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.1-10
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Submit two expenses to any user
  3. Go to Search
  4. Open the transaction RHP of any of the submitted expenses
  5. Hold and unhold the expense from 3-dot menu
  6. Note that the transaction RHP does not dismiss when holding and unholding expense from the 3-dot menu
  7. Open the same transaction RHP again
  8. Click on the report header
  9. Hold and unhold the expense from the report details page
  10. Note that the transaction RHP dismisses each time the expense is held and unheld

Expected Result:

When holding and unholding expense in Search via report details RHP, the transaction RHP will not dismiss

Actual Result:

When holding and unholding expense in Search via report details RHP, the transaction RHP dismisses

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

Add any screenshot/video evidence

Bug6523376_1719260134813.20240625_041020.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018b190e03e676aa45
  • Upwork Job ID: 1805348998427340557
  • Last Price Increase: 2024-06-24
  • Automatic offers:
    • Krishna2323 | Contributor | 102862197
Issue OwnerCurrent Issue Owner: @sobitneupane
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@lanitochka17
Copy link
Author

@neil-marcellini 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

@yuwenmemon yuwenmemon added External Added to denote the issue can be worked on by a contributor and removed DeployBlocker Indicates it should block deploying the API labels Jun 24, 2024
@melvin-bot melvin-bot bot changed the title Search - Transaction RHP dismisses when holding/unholding expense from report details page [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

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

@neil-marcellini
Copy link
Contributor

👀 taking a look now

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jun 24, 2024

Proposal

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

Search - Transaction RHP dismisses when holding/unholding expense from report details page

What is the root cause of that problem?

When we select hold/unhold option from report details RHP, we dismiss the modal instead of returning back.

onSelected: () => {
Navigation.dismissModal();
ReportUtils.changeMoneyRequestHoldStatus(reportAction);
},

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

  1. We will remove Navigation.dismissModal();
  2. When we are unholding a request we can simply go back
  3. When we navigate to the hold reasons page, we need to provide the backTo parameter according to the topmostCentralPaneRoute. If topmostCentralPaneRoute is SCREENS.SEARCH.CENTRAL_PANE, we need to pass ROUTES.SEARCH_REPORT.getRoute(currentQuery?.query ?? CONST.SEARCH.TAB.ALL, reportAction?.childReportID ?? '') as backTo. This is because calling Navigation.goBack() will return to the report details page, but we need to go to the report details page.
        onSelected: () => {
            if (!isTextHold) {
                Navigation.goBack();
            }
            const topmostCentralPaneRoute = getTopmostCentralPaneRoute(navigationRef.getRootState() as State<RootStackParamList>);
            const isReportInRHP = topmostCentralPaneRoute?.name === SCREENS.SEARCH.CENTRAL_PANE;
            
            // dismiss the modal if isReportInRHP is false
            if (!isReportInRHP && isTextHold) {
                Navigation.dismissModal();
                ReportUtils.changeMoneyRequestHoldStatus(reportAction);
                return;
            }
            
            // Instead of dismissing the modal or calling goBack, we just provide backTo route when navigating to hold reasons page
            const currentQuery = topmostCentralPaneRoute?.params as CentralPaneNavigatorParamList['Search_Central_Pane'];
            ReportUtils.changeMoneyRequestHoldStatus(reportAction, ROUTES.SEARCH_REPORT.getRoute(currentQuery?.query ?? CONST.SEARCH.TAB.ALL, reportAction?.childReportID ?? ''));
        },

What alternative solutions did you explore? (Optional)

@neil-marcellini
Copy link
Contributor

Thanks @Krishna2323! That sounds good, hired. Please get started right away.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

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

@neil-marcellini
Copy link
Contributor

@Krishna2323 how's it going. When do you expect to have the PR up for review?

@Krishna2323
Copy link
Contributor

@neil-marcellini, sorry for delay, the solution is much more complex than I thought, I'm about to push the changes in few minutes.

@neil-marcellini
Copy link
Contributor

Ah ok, thanks for the update. If you end up implementing something much different than your proposal it would be a good idea to update it.

@Krishna2323
Copy link
Contributor

@neil-marcellini, I have updated my proposal to match my implementation in the PR, pls let me know if that makes sense to you, after that I will add the recordings.

Result

fix_rhp_when_holding_exense.mp4

@neil-marcellini
Copy link
Contributor

Yeah I think that all sounds good. I'll review the PR now.

@mountiny mountiny removed the DeployBlockerCash This issue or pull request should block deployment label Jun 25, 2024
@mountiny
Copy link
Contributor

THe PR will be validated after being CPed

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 25, 2024
@melvin-bot melvin-bot bot changed the title [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page [HOLD for payment 2024-07-02] [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page Jun 25, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 25, 2024
Copy link

melvin-bot bot commented Jun 25, 2024

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

Copy link

melvin-bot bot commented Jun 25, 2024

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

If no regressions arise, payment will be issued on 2024-07-02. 🎊

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 2, 2024
Copy link

melvin-bot bot commented Jul 2, 2024

Issue is ready for payment but no BZ is assigned. @OfstadC you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

@OfstadC
Copy link
Contributor

OfstadC commented Jul 2, 2024

Payment Summary:

Contributor: @Krishna2323 paid $250 via Upwork
Contributor+: @sobitneupane is due $250 via NewDot

@OfstadC OfstadC closed this as completed Jul 2, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-02] [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] Search - Transaction RHP dismisses when holding/unholding expense from report details page Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

@JmillsExpensify
Copy link

$250 approved for @sobitneupane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants