Remove Onyx.connect() usage for ONYXKEYS.COLLECTION.REPORTS from src/libs/actions/IOU/Transaction.ts - #93734
Conversation
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.
|
…-fork into Guccio163/onyx-connect/transaction/report
|
@DylanDylann 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a7cc16a6
ℹ️ 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".
|
No product review needed |
…-fork into Guccio163/onyx-connect/transaction/report
…-fork into Guccio163/onyx-connect/transaction/report
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c70ce5a0bd
ℹ️ 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".
Resolved conflicts: - Kept onyx-connect changes (reports param / removed global allReports Onyx.connect) - Took main's version for files only carried as stale-main snapshots - Restored GroupCurrencyPopup.tsx from main - Consolidated DynamicIOURequestStepUpgradePage.tsx -> IOURequestStepUpgrade.tsx (main's name) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@DylanDylann kindly bump here 👀 |
…-fork into Guccio163/onyx-connect/transaction/report
|
@DylanDylann fixed, ready for a re-review |
|
@Guccio163 This current code looks wrong to me. Why do we use reportActionID to get the report object? App/src/libs/actions/Transaction.ts Line 1528 in c0a4080 Are you okay if we adjust this before refactoring? |
…-fork into Guccio163/onyx-connect/transaction/report
|
@Guccio163 Kindly bump |
|
Hi, I was OOO for the past week, that's why I didn't complete this one |
…-fork into Guccio163/onyx-connect/transaction/report
waitForCollectionCallback no longer exists on ConnectOptions (collection vs entry is now inferred from the key type), and two changeTransactionsReport calls had a reports/allReports typo.
… merges buildOptimisticEmptyReport doesn't set these two fields, so the freshly created report merged into reportsForCall was missing them, leaving failure-rollback unable to reset them (Onyx merge ignores undefined).
Covers source selection (reported vs unreported transaction), skipping the IOU action lookup for deleted transactions, and that the result is narrowed to only the computed ids.
|
I fixed the logic, patched for failureData and added new tests file for this hook, please take a look @DylanDylann 👀 |
|
@Guccio163 Regarding this problem, I asked the author who introduced it here, but got no response. Still, we need to fix it here too, to avoid spreading the problem to the new hook. This is my suggestion, could you check and update the PR? index b0fd166f203..7008b4a0dde 100644
--- a/src/hooks/useChangeTransactionsReportReports.ts
+++ b/src/hooks/useChangeTransactionsReportReports.ts
@@ -50,9 +50,6 @@ function useChangeTransactionsReportReports(transactionIDs: string[], allTransac
if (iouAction?.childReportID) {
ids.add(iouAction.childReportID);
}
- if (iouAction?.reportActionID) {
- ids.add(iouAction.reportActionID);
- }
}
const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (allReports) => {
diff --git a/src/libs/actions/Transaction.ts b/src/libs/actions/Transaction.ts
index 115b4651a72..4857b4bf977 100644
--- a/src/libs/actions/Transaction.ts
+++ b/src/libs/actions/Transaction.ts
@@ -1575,7 +1575,7 @@ function getChangeTransactionsReportOnyxData({
value: {
parentReportID: isUnreportedExpense ? selfDMReportID : oldReportID,
parentReportActionID: oldIOUAction.reportActionID,
- policyID: reports?.[`${ONYXKEYS.COLLECTION.REPORT}${oldIOUAction.reportActionID}`]?.policyID,
+ policyID: reports?.[`${ONYXKEYS.COLLECTION.REPORT}${oldIOUAction.childReportID}`]?.policyID,
},
});
} |
|
Ah yes, I must've missed that comment - I thought about the same issue, but decided that I'll leave it like that so we avoid regression, but since you've noticed it too I'll apply your proposed changes, they look good to me 👍 |
oldIOUAction.reportActionID was used as a REPORT key, which never matches a real report, so the restored policyID was always undefined. Use oldIOUAction.childReportID (the thread's own key) instead, and drop the now-unnecessary reportActionID entry from the hook's id set. Per DylanDylann's review on Expensify#93734.
…-fork into Guccio163/onyx-connect/transaction/report
Two reportNextStep tests never refreshed the reports snapshot after merging their own report data, so they asserted against stale data and threw before mockAPIWrite.mockRestore() ran — leaving the spy active and corrupting the two sibling-duplicate-violation tests that ran after them. Also fixes an allReports/reports typo in tests newly merged from main.
…-fork into Guccio163/onyx-connect/transaction/report
…-fork into Guccio163/onyx-connect/transaction/report
…-fork into Guccio163/onyx-connect/transaction/report
The feature they tested (moving all matching expenses via jsonQuery + hash) was reverted from Transaction.ts on main in d51ada0, but the merge into this branch left the tests behind, referencing fields (jsonQuery, hash) that no longer exist on ChangeTransactionsReportProps.
|
@DylanDylann ready ✅ |
|
|
||
| if (isEditing) { | ||
| const policyTagList = item?.policyID ? allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${item.policyID}`] : {}; | ||
| const reportsForCall = report?.reportID ? {[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, ...reports} : reports; |
There was a problem hiding this comment.
Why do we need to update the reports here?
There was a problem hiding this comment.
We need it because the hook here doesn't know the destination report yet when it runs — report is only picked later, inside this callback, so reports never contains it.
changeTransactionsReport's failure-rollback path reads the target report straight from reports, with no fallback to the newReport param (unlike the optimistic-update path, which does have that fallback). So if reports is missing the destination report, the rollback value on API failure is undefined — and since Onyx.merge ignores undefined, a failed request would leave the destination report's total/transactionCount stuck at the optimistic (wrong) value instead of rolling back.
Merging report in here fixes that. It's the same thing other callers of this hook already do for the same reason (SearchTransactionsChangeReport.tsx, DynamicIOURequestEditReport.tsx, IOURequestStepUpgrade.tsx, DynamicNewReportWorkspaceSelectionPage.tsx).
…RT_ID lookups Accept the resolved transactions and destination reportID directly instead of a transactionIDs/allTransactions pair, dropping the per-caller collection-building boilerplate. Also always include CONST.REPORT.UNREPORTED_REPORT_ID in the resolved report ids, matching what getChangeTransactionsReportOnyxData looks up for unreported transactions.
|
@DylanDylann applied the changes/replied to the comment, please take a look |
5 participants
Explanation of Change
This PR is part of a refactor to remove Onyx.connect for the keys: ONYXKEYS.COLLECTION.REPORT from the src/libs/actions/IOU/Transation.ts file and replace it with useOnyx.
It adds the
reportsargument to thechangeTransactionsReportfunction, extracted and filtered from the ONYXKEYS.COLLECTION.REPORT Onyx.connect key .Fixed Issues
$ #67779
PROPOSAL:
Tests
Prerequisites
Steps
preserved.
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Screen.Recording.2026-06-22.at.12.50.34.mov