[HOLD for payment 2025-01-02] [$250] Hold - Hold banner shows up after deleting dupe expense

[lanitochka17]

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.72-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: #53167
Email or phone of affected tester (no customers): nathanmulugetatesting+2349@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: Create a workspace and invite a user as employee.

1. [Employee] Submit two expenses that are duplicated
2. [Admin] Go to the transaction thread of a transaction
3. [Admin] Review the duplicate and choose any the first transaction to keep
4. [Employee] Open expense report
5. [Employee] Right click on the second transaction and delete it (The one that is on hold)

Expected Result:

Hold banner doesn't show up since there is no transaction that is on hold at the moment

Actual Result:

Hold banner appears even if there is no transaction on hold

Workaround:

Unknown

Platforms:

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

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6685478_1733424577498.bandicam_2024-12-05_21-39-33-560.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021865063509444295109
• Upwork Job ID: 1865063509444295109
• Last Price Increase: 2024-12-06
• Automatic offers:
• dominictb | Contributor | 105259939
• FitseTLT | Contributor | 105373054

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

Triggered auto assignment to @trjExpensify (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.

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-12-05 20:44:02 UTC.

Proposal

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

Hold - Hold banner shows up after deleting dupe expense

What is the root cause of that problem?

There are two problems here if both the admin account and employee account are new accounts where they haven't dismissedHoldUseExplanation yet

1. From the Admin side, when we open the first transaction thread and resolve the duplicate. The second transactionThread report is not available in onyx (until we open it). So when we resolve the duplicate the transactionID here will -1 because we are using TransactionUtils.getTransactionID to indirectly get transactionID which requires the availablity of the transaction thread that is to be put on hold (in this case the second transaction thread)
   

   App/src/libs/actions/IOU.ts

   Line 8577 in 4eb86b4

   const transactionID = TransactionUtils.getTransactionID(transactionThreadReportID ?? '-1');

   
   so we set comment.hold on a tranasaction_-1 key so whenever we open the money report with two transaction thread the transactionThreadReportID will be -1
   

   App/src/components/MoneyReportHeader.tsx

   Lines 83 to 88 in 4eb86b4

   	const transaction =
   	transactions?.[
   	`${ONYXKEYS.COLLECTION.TRANSACTION}${
   	ReportActionsUtils.isMoneyRequestAction(requestParentReportAction) ? ReportActionsUtils.getOriginalMessage(requestParentReportAction)?.IOUTransactionID ?? -1 : -1
   	}`
   	] ?? undefined;

   
   so the isHold will be true as we have set hold on transactionID -1 and hence the banner is shown every time we open the money report until hold use explanation is dismissed
2. From the employee side after the admin has resolved the duplicate transactions if we delete the held expense the hold menu will be shown because the transactionViolation for the first transaction is not immediately pushed from BE and the first transaction in onyx will have duplicate violation (until page refresh on which the data will be successfully fetched from BE) so the modal will be shown not for the second transaction but for the second one. This one can only be fixed in BE.

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

Solution for problem 1: We should directly get the transaction id from iou action (parent actioninstead of indirectly viaTransactionUtils.getTransactionIDhttps://github.com/Expensify/App/blob/4eb86b4435761c97f4c7257dd7e03ac565274d53/src/libs/actions/IOU.ts#L8574-L8578 because we don't needtransactionThreadReportIDList` anymore we will directly iterate on iouActionList like:

iouActionList.forEach((action ) => {
        const transactionThreadReportID = action?.childReportID;
        const createdReportAction = ReportUtils.buildOptimisticHoldReportAction();
        reportActionIDList.push(createdReportAction.reportActionID);
        const transactionID = ReportActionsUtils.isMoneyRequestAction(action) ? ReportActionsUtils.getOriginalMessage(action)?.IOUTransactionID ?? '-1' : '-1';

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can make a test for resolveDuplicates by setting the appropriate value in onyx (of course without setting the second transactionThread report in onyx) and also passing the correct params and then assert that it correctly sets the second transaction on hold and also it doesn't set the hold for transactioin_-1

What alternative solutions did you explore? (Optional)

[mkzie2]

Proposal

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

Hold banner appears even if there is no transaction on hold

What is the root cause of that problem?

1. The transaction thread report is unavailable in Onyx from the admin side until the admin opens this report. When we resolve the duplicate and choose the first transaction, transactionID of the second transaction is wrong here because we're getting this based on the data of the transaction thread report which is unavailable as I mentioned above.

App/src/libs/actions/IOU.ts

Line 8577 in 3baf965

const transactionID = TransactionUtils.getTransactionID(transactionThreadReportID ?? '-1');

App/src/libs/TransactionUtils/index.ts

Lines 1196 to 1199 in 3baf965

	function getTransactionID(threadReportID: string): string {
	const report = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${threadReportID}`] ?? null;
	const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');
	const IOUTransactionID = ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction)?.IOUTransactionID ?? '-1' : '-1';

2. After the admin resolves the duplicate, the backend doesn't send a pusher to remove the duplicate violation then the employee still sees the duplicate. When the second transaction is deleted, the expense report becomes the combine report so the hold banner appears.

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

1. We already have orderedTransactionIDList here that has the same order with transactionThreadReportIDList, we can use it here

transactionThreadReportIDList.forEach((transactionThreadReportID, index) => {
    const transactionID = orderedTransactionIDList?.at(index) ?? '-1';

App/src/libs/actions/IOU.ts

Line 8577 in 3baf965

const transactionID = TransactionUtils.getTransactionID(transactionThreadReportID ?? '-1');

2. For the second problem, we should fix it from the backend side to send the pusher to clear the duplicate violation after the admin resolves the duplicate.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can create an API test with resolveDuplicates. We need to prepare the corresponding Onyx data (the data that cause this issue) after we call this API, we need to verify that all resolved transactions are holds expect the selected transaction in resolveDuplicates

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

@dominictb what do you think of the above?

[dominictb]

@trjExpensify I'm reviewing both proposals but need more time for testing. Will provide update in several hours.

[trjExpensify]

Sounds good!

…

On Wed, Dec 11, 2024 at 11:28 AM Dominic ***@***.***> wrote: @trjExpensify <https://github.com/trjExpensify> I'm reviewing both proposals but need more time for testing. Will provide update in several hours. — Reply to this email directly, view it on GitHub <#53651 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD3246M6ECSPMSD57FKM7MD2FAHUHAVCNFSM6AAAAABTDF3YSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZVGQ2TQMRSHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *Tom Rhys Jones * *Expensify*

[trjExpensify]

@dominictb can we get an update, please? Thanks!

[dominictb]

on it now

[dominictb]

@FitseTLT Friendly bump!

[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 9.0.78-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix - Hold - Hold banner shows up after deleting dupe expense #54408

If no regressions arise, payment will be issued on 2025-01-02. 🎊

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

• @FitseTLT requires payment automatic offer (Contributor)
• @dominictb requires payment automatic offer (Contributor)

[melvin-bot]

@dominictb @trjExpensify @dominictb The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[trjExpensify]

👋 can we get the checklist here please, @dominictb?

[trjExpensify]

Still awaiting the checklist, Melv.

[dominictb]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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/50383/files#r1904215346

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. Yes

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Test:

• Precondition:
  Sign in with a new account
  Create a workspace and invite a user as employee.

1. [Employee] Submit two expenses that are duplicate
2. [Admin] Go to the expense report and open the first transaction thread and when the hold banner appears don't press on the Got it button just close by tapping outside of the modal
3. [Admin] Review the duplicate and choose the first transaction to keep
4. [Admin] Go back to the expense report
5. [Admin] Verify that the educational hold modal doesn't appear while on the expense report

Do we agree 👍 or 👎

[dominictb]

@trjExpensify I did the checklists

[trjExpensify]

Thanks! Payment summary as follows:

• $250 to @FitseTLT for the fix
• $250 to @dominictb for the C+ review

Paid, closing!