Refactor delete money request for transaction Onyx Key by parasharrajat · Pull Request #98126 · Expensify/App

parasharrajat · 2026-08-10T11:02:05Z

Explanation of Change

Fixed Issues

$ #66517
PROPOSAL:

Tests

Merge two transactions

Steps:

Create two transactions that can be merged (same report, or one source in a self-DM/unreported state, e.g. a tracked expense) and wait for the merge entry to appear (duplicates detected / merge button on the transaction).
Open the Merge transactions flow: select the target (kept) transaction and the source transaction, review merged details, confirm Merge.
Verify: target transaction is updated with merged data; source transaction is deleted; when the source transaction had its own thread, that source thread report is deleted.

Review & merge duplicates

Steps:

Have two duplicate expenses flagged in one report (duplicate detection). Open the report header / duplicate banner → Review duplicates. Choose one of them and click Keep selected
Go through the review steps and on the confirmation page tap the merge action (Confirm).
Verify: the kept transaction reflects the merged fields; the discarded duplicate's transaction and its transaction thread are removed;

Verify that no errors appear in the JS console

Offline tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

02.09.2026_20.09.13_REC.mp4

… args

…e merge

…n thread data builder

…ad of getAllReports

…ve getAllReports from getCleanUp callers

codecov · 2026-08-10T13:07:46Z

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines	Coverage Δ
src/libs/actions/IOU/DeleteMoneyRequest.ts	`78.10% <100.00%> (ø)`
src/libs/actions/IOU/Duplicate.ts	`92.47% <100.00%> (+0.06%)`	⬆️
src/libs/actions/IOU/TrackExpense.ts	`84.87% <ø> (ø)`
src/libs/actions/MergeTransaction.ts	`79.60% <ø> (ø)`
...pages/TransactionMerge/DynamicConfirmationPage.tsx	`69.23% <100.00%> (+1.23%)`	⬆️
...s/TransactionDuplicate/DynamicConfirmationPage.tsx	`0.00% <0.00%> (ø)`
... and 5 files with indirect coverage changes

… callers

…money-request

melvin-bot · 2026-09-02T17:35:22Z

@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

parasharrajat · 2026-09-02T17:36:54Z

That test failure happens on main too.

ikevin127 · 2026-09-02T22:58:25Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Screen.Recording.2026-09-03.at.16.07.08.mov

parasharrajat · 2026-09-07T15:57:53Z

@ikevin127 Its ready for you.

ikevin127 · 2026-09-09T00:59:09Z

Thanks, doing a second review-pass now 🔄

ikevin127 · 2026-09-09T01:18:36Z

        policyTags: undefined,
        policyCategories: undefined,
        allTransactionViolations: createAllTransactionViolations(targetTransaction.transactionID, sourceTransaction.transactionID, mockViolations, mockViolations),
+        sourceIOUActionThreadReport: undefined,


🟡 tests/actions/MergeTransactionTest.ts - the self-DM fix from comment 1 has zero test coverage

All nine call sites still pass sourceIOUActionThreadReport: undefined.

That is fine for the eight expense-report cases, but it means the one behaviour this PR actually fixed, the unreported / tracked-expense merge path, is verified by nothing. If someone reverts the ?? selfDMSourceIOUAction?.childReportID fallback tomorrow, every test still passes.

DuplicateTest.ts now has the right pattern to copy, so this is mechanical: one case where sourceTransaction.reportID === CONST.REPORT.UNREPORTED_REPORT_ID, a selfDMReport and selfDMReportActions containing the IOU action with a childReportID, a real sourceIOUActionThreadReport, then assert the successData contains REPORT_${threadID}: null and the failureData restores the passed thread report.

If you fold in the fix above, the same fixture also covers the report-actions rollback.

ikevin127 · 2026-09-09T01:19:14Z

Revised test coverage

Roughly 35 to 40%, up from 10 to 15% at the first pass. Breaking down the six new data surfaces:

Surface	Covered
`transactionThread` on `getCleanUpTransactionThreadReportOnyxData`	✅ new precedence test
`allReportsList` (`expenseReport` / `sourceReport`)	✅ new precedence test
`iouReport` param	❌
`chatReport` param	❌
`transactionThread` on `getDeleteTrackExpenseInformation`	❌
`sourceIOUActionThreadReport` incl. the self-DM fix	❌ all 9 cases pass `undefined`

Fixture realism across the six updated DuplicateTest cases is also much better now, which matters independently of the assertion count.

ikevin127 · 2026-09-09T01:20:19Z

Approval state

Not approving yet, but close. One round away.

What I want before approval:

The one-line sourceTransactionThreadReportActions scope fix. It is free, it is inside the lines this PR already changed, and shipping the pair half-fixed is worse than not having touched either.

If that is the only thing that changes, I will approve. The rest are nice-to-haves I will not block on:

One MergeTransactionTest case covering the self-DM merge path, so the comment 1 fix cannot silently regress. I would push for this but will not hold the PR if you would rather file it as a follow-up, given the fix itself is verified by the manual video.
iouReport / chatReport precedence coverage, genuinely optional, the fallbacks make them low risk today.

Everything else from pass 1 is either fixed or correctly rebutted. Code quality on the fixes is good, the new precedence test is a real test rather than a box-tick, and CI is fully green.

cc @parasharrajat

…money-request

parasharrajat · 2026-09-10T13:13:04Z

Updated.

ikevin127

🟢 LGTM - Thanks for addressing all comments

danieldoglas

LGTM

OSBotify · 2026-09-14T19:38:21Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2026-09-14T19:48:20Z

🚧 danieldoglas has triggered a test Expensify/App build. You can view the workflow run here.

github-actions · 2026-09-14T19:54:59Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #98126.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-android-Adhoc-c670e1c-7d6c53b-b3b7e26646452cb72041ebdbd0394e45612e8beb/index.html	https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-ios-device-AdHoc-c670e1c-7d6c53b-f9cb04bd37c68212aea4b9efd3447f473680ddc2/index.html

Web 🕸️
https://98126.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

OSBotify · 2026-09-15T01:27:04Z

🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.4.78-0 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

MelvinBot · 2026-09-15T01:31:18Z

No help site changes are required for this PR, so I did not create a docs PR.

This is an internal Onyx refactor, not a behavior or copy change. Every change swaps a global getAllReports() read for reports passed in explicitly from useOnyx:

src/libs/actions/IOU/DeleteMoneyRequest.ts:620-701 — adds optional transactionThread, iouReport, and chatReport params that fall back to the old lookups.
src/libs/actions/IOU/Duplicate.ts:191-320 — mergeDuplicates now takes allReportsList instead of calling getAllReports().
src/pages/TransactionMerge/DynamicConfirmationPage.tsx:92-100 — falls back to the self-DM IOU action so the source thread is found and torn down when merging a tracked expense.

The only user-visible effect is that the discarded expense's transaction thread is reliably deleted after a merge. The existing articles already describe that outcome, so nothing needs editing:

How to Merge Duplicate Expenses already states the other expense "is merged into it and no longer appears separately."
How to Find and Resolve Flagged Duplicate Expenses covers the Review duplicates flow, which is unchanged.

No strings in src/languages/* changed, and no button, tab, or page label changed, so there is nothing to re-verify against the live UI.

@parasharrajat, please confirm this matches the behavior you shipped. If you think a docs update is warranted anyway, reply with what changed for the user and I'll create the draft PR.

view run · no recording available

jponikarchuk · 2026-09-15T05:19:04Z

Deploy Blocker #101157 was identified to be related to this PR.

jponikarchuk · 2026-09-15T12:33:56Z

@parasharrajat @ikevin127 Could you please confirm the expected result for step 2 Go through the review steps and on the confirmation page tap the merge action (Merge). We have no "Merge" option on confirm page. Should we log this?

Grabacion.de.pantalla.2026-09-14.a.la.s.22.online-video-cutter.com.mp4

luacmartins · 2026-09-15T20:44:50Z

@danieldoglas could you help with the above please?

danieldoglas · 2026-09-15T21:34:06Z

@jponikarchuk I've updated the tests steps. Please try again.

OSBotify · 2026-09-15T23:43:27Z

🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.78-3 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

Bundle Size Analysis (Sentry):

Android

parasharrajat added 6 commits August 7, 2026 11:32

Refactor getCleanUpTransactionThreadReportOnyxData to accept optional…

84044b5

… args

Pass transactionThread to getCleanUpTransactionThreadReportOnyxData

64eedc5

Pass reports to getCleanUpTransactionThreadReportOnyxData in duplicat…

d5e5682

…e merge

refactor pass chatReport to cleanup transaction thread data builder

b284170

Pass source transaction thread report via hooks to cleanup transactio…

5b558d5

…n thread data builder

Source transaction thread and duplicate merge reports via hooks inste…

c2d565e

…ad of getAllReports

melvin-bot Bot assigned parasharrajat Aug 10, 2026

Source transaction thread and duplicate merge reports via hooks, remo…

030f959

…ve getAllReports from getCleanUp callers

Thread reports via hooks to getCleanUpTransactionThreadReportOnyxData…

69abde3

… callers

ikevin127 mentioned this pull request Aug 19, 2026

[More PRs incoming] [Partial Payments] Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/actions/IOU.ts #66517

Open

parasharrajat added 8 commits September 1, 2026 17:34

Merge branch 'main' of github.com:Expensify/App into refactor-delete-…

ffe3635

…money-request

changes

967176f

Remove unused

74feda4

refactor params

2b300cb

fixes

d7cf7ce

More cleanup

484f91a

fix tests

e3f63b2

Merge branch 'main' of github.com:Expensify/App into refactor-delete-…

edd864e

…money-request

parasharrajat marked this pull request as ready for review September 2, 2026 17:35

parasharrajat requested review from a team as code owners September 2, 2026 17:35

melvin-bot Bot requested review from ikevin127 and trjExpensify and removed request for a team September 2, 2026 17:35

melvin-bot Bot removed the request for review from a team September 2, 2026 17:35

trjExpensify removed their request for review September 3, 2026 00:49

parasharrajat added 4 commits September 6, 2026 20:40

fix tests

19c5766

fix tests

ac766ba

fix tests

d8f7a93

fix tests

718e0f4

ikevin127 reviewed Sep 9, 2026

View reviewed changes

Comment thread src/pages/TransactionMerge/DynamicConfirmationPage.tsx Outdated

ikevin127 reviewed Sep 9, 2026

View reviewed changes

parasharrajat added 4 commits September 10, 2026 18:23

fix

e89c533

Add unit test

e681749

Merge branch 'main' of github.com:Expensify/App into refactor-delete-…

cfd8c74

…money-request

Unit tests

ce2d680

ikevin127 approved these changes Sep 10, 2026

View reviewed changes

melvin-bot Bot requested a review from danieldoglas September 10, 2026 21:03

danieldoglas approved these changes Sep 14, 2026

View reviewed changes

danieldoglas merged commit c670e1c into Expensify:main Sep 14, 2026
35 of 37 checks passed

OSBotify mentioned this pull request Sep 15, 2026

Deploy Checklist: New Expensify 2026-09-15 #101132

Closed

62 tasks

Conversation

parasharrajat commented Aug 10, 2026 • edited by danieldoglas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Merge two transactions

Review & merge duplicates

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

codecov Bot commented Aug 10, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melvin-bot Bot commented Sep 2, 2026

Uh oh!

parasharrajat commented Sep 2, 2026

Uh oh!

ikevin127 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

parasharrajat commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikevin127 commented Sep 9, 2026

Uh oh!

Uh oh!

ikevin127 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

ikevin127 commented Sep 9, 2026

Revised test coverage

Uh oh!

ikevin127 commented Sep 9, 2026

Approval state

Uh oh!

parasharrajat commented Sep 10, 2026

Uh oh!

ikevin127 left a comment

Choose a reason for hiding this comment

Uh oh!

danieldoglas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OSBotify commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026

Uh oh!

OSBotify commented Sep 15, 2026

Uh oh!

MelvinBot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jponikarchuk commented Sep 15, 2026

Uh oh!

jponikarchuk commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luacmartins commented Sep 15, 2026

Uh oh!

danieldoglas commented Sep 15, 2026

Uh oh!

OSBotify commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

parasharrajat commented Aug 10, 2026 •

edited by danieldoglas

Loading

codecov Bot commented Aug 10, 2026 •

edited by github-actions Bot

Loading

ikevin127 commented Sep 2, 2026 •

edited

Loading

parasharrajat commented Sep 7, 2026 •

edited

Loading

MelvinBot commented Sep 15, 2026 •

edited

Loading

jponikarchuk commented Sep 15, 2026 •

edited

Loading