[NoQA] feat: implement mergeReports() action#95182
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69689fffa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2683e7334e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ddc0ed44
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
🤖 Code review — overall this is a clean, well-tested implementation. The refactor that extracts 1.
|
|
@codex review |
|
@MelvinBot review |
|
🤖 Re-review (head
Two minor, non-blocking notes:
LGTM overall — clean refactor and solid test coverage. 👍 Reply with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beb63d2436
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8293411130
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ReviewSolid, well-factored PR. Checks are green (typecheck, ESLint, knip, tests all pass) and the refactor of 1. Silent behavior change in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6055da4b47
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| key: `${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, | ||
| value: { | ||
| hasOutstandingChildRequest: hasOutstandingChildRequest(chatReport, reports, currentUserEmailParam, currentUserAccountIDParam, allTransactionViolations, bankAccountList), | ||
| ...(shouldClearChatIOUReportID ? {iouReportID: null} : {}), |
There was a problem hiding this comment.
Preserve the destination report pointer when merging
When mergeReports deletes a source report that is currently stored as the policy chat's iouReportID, this helper always clears the pointer to null. In a merge, however, the expenses are moved into destinationReportID, so the chat still has an active outstanding report; clearing the pointer makes optimistic/offline state behave as if there is no active expense report until server data comes back. Pass the destination report through the merge path and set iouReportID to that report instead of always nulling it for source-report merges.
Useful? React with 👍 / 👎.
Regression scanNo functional regressions found in the success paths — the refactor is backward-compatible by construction. I traced every existing caller of the two touched shared functions:
|
|
|
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #94872 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Conflicts |
Thanks. Conflicts resolved |
|
Oops conflicts again |
|
@marcochavezf This is ready for review again. Thank you. |
|
@marcochavezf This PR is still waiting to be merged. |
|
🚧 marcochavezf has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.36-0 🚀
|
|
🤖 Help site review: no changes required. This PR adds internal plumbing for a merge-reports capability and does not change any documented, user-facing behavior:
There's no new UI, copy, menu label, or settings surface shipped here — it's foundational action-layer code that the merge action and bulk-action menu will depend on later. The one existing merge-related help article, How to Merge Duplicate Expenses, covers merging duplicate expenses/transactions, which is a distinct, already-documented feature and is unaffected by this change. If a follow-up PR wires up a user-facing "merge reports" flow (e.g. a bulk-action menu item or button), that would be the right time to add or update help site articles. No draft PR was created since no documentation updates are needed. @dmkt9, please confirm this assessment. If you believe a user-facing behavior here does warrant a help site update, let me know and I'll create the draft PR. |
Explanation of Change
Add the constants and command types that the merge action and bulk-action menu depend on.
CONST.SEARCH.BULK_ACTION_TYPES.MERGE_REPORTS— namespaced underSEARCH, matching the existing bulk-action constants.WRITE_COMMANDS.MERGE_REPORTS: 'MergeReports'insrc/libs/API/types.ts.MergeReportsParamsinWriteCommandParameters:destinationReportID: stringsourceReportIDs: string[](a real array, derived as selected minus destination — not a JSON-stringified string)Add the
mergeReports()action that firesWRITE_COMMANDS.MERGE_REPORTSwith optimistic data.changeTransactionsReport()insrc/libs/actions/Transaction.tsrather than reimplementing "move + delete empty" inReport.ts. That function already moves transactions, updates source totals/counts, manages transaction threads, and tracksstaleReportIDs.ONYXKEYS.COLLECTION.SNAPSHOTupdates.Fixed Issues
$ #94872
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari