Update requestMoney function to remove allTransaction usages - #86038
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.
|
|
@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
changed the title
![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: 8fe5638ee3
ℹ️ 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".
parasharrajat
changed the title
| const existingTransactionID = existingTransactionDraft?.transactionID; | ||
| const existingTransaction = action === CONST.IOU.ACTION.SUBMIT ? existingTransactionDraft : allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${existingTransactionID}`]; | ||
| const existingTransactionRef = | ||
| action === CONST.IOU.ACTION.SUBMIT ? existingTransactionDraft : (existingTransaction ?? allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${existingTransactionID}`]); |
There was a problem hiding this comment.
allTransactions will be removed in later PR from here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f974afa7ed
ℹ️ 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".
| @@ -6425,7 +6427,8 @@ function requestMoney(requestMoneyInformation: RequestMoneyInformation): {iouRep | |||
| const moneyRequestReportID = isMoneyRequestReport ? report?.reportID : ''; | |||
| const isMovingTransactionFromTrackExpense = isMovingTransactionFromTrackExpenseIOUUtils(action); | |||
| const existingTransactionID = existingTransactionDraft?.transactionID; | |||
There was a problem hiding this comment.
requestMoney now accepts existingTransaction, but existingTransactionID is still derived only from existingTransactionDraft. In the new call paths (e.g. where existingTransaction is fetched from Onyx separately), existingTransactionDraft can be undefined while existingTransaction is present; this leaves existingTransactionID undefined and getMoneyRequestInformation() falls back to a new random transaction ID, so the submit/move flow creates a new transaction instead of updating the linked tracked expense.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is how it was working before.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-04-02.at.16.43.29.mov |
|
@parasharrajat Found a regression that doesn't happen on staging, with the same steps on the same account:
Expected: Expense is moved to WS chat report successfully (like it does below in the Staging video). Actual: We get 🔴
|
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const transactionIDs = useMemo(() => transactions?.map((transaction) => transaction.transactionID), [transactions.length]); | ||
| const transactionIDs = useMemo(() => transactionDrafts?.map((transaction) => transaction.transactionID), [transactionDrafts.length]); | ||
| const [transactions] = useTransactionsByID(transactionIDs); |
There was a problem hiding this comment.
Changes Analysis:
- Renamed
transactionstotransactionDrafts(lines 171-175) - Added new
transactionsfromuseTransactionsByIDhook (line 176) - Updated
currentTransactionIndexto usetransactionDrafts(lines 180-183) - Modified
requestMoneycallback to passexistingTransaction: existingStoredTransaction(line 822)
🚨 CRITICAL ISSUE: Variable naming confusion
The renaming from transactions to transactionDrafts while simultaneously adding a new transactions variable is confusing and potentially error-prone:
// Lines 171-176
const [transactionDrafts] = useOptimisticDraftTransactions(initialTransaction); // Was: transactions
const hasMultipleTransactions = transactionDrafts.length > 1;
const transactionIDs = useMemo(() => transactionDrafts?.map((tx) => tx.transactionID), [transactionDrafts.length]);
const [transactions] = useTransactionsByID(transactionIDs); // NEW: stored transactions from OnyxLater in the code:
// Line 740
const existingStoredTransaction = existingTransactionID
? transactions?.find((tx) => tx?.transactionID === existingTransactionID)
: undefined;The issue: existingStoredTransaction is fetched from transactions which is derived from transactionIDs which comes from transactionDrafts. This means it's looking up the transaction from the list of transactionIDs, but the existingTransactionID may not be in that list if it's a tracked expense being moved to a workspace.
BUG Scenario:
- User has a tracked expense in Self DM with
transactionID: "tracker-123". - User submits this to a workspace, creating a new draft transaction with
transactionID: "draft-456". - The
transactionDraftsarray contains[{transactionID: "draft-456"}]. - So
transactionIDs = ["draft-456"]. useTransactionsByID(["draft-456"])fetches only transaction "draft-456".- But
existingTransactionIDis"tracker-123". transactions?.find((tx) => tx?.transactionID === "tracker-123")returnsundefined!.
BEFORE (broken):
// Line 176
const [transactions] = useTransactionsByID(transactionIDs);
...
// Line 740
const existingStoredTransaction = existingTransactionID
? transactions?.find((tx) => tx?.transactionID === existingTransactionID)
: undefined;AFTER (fixed):
// Fetch the actual stored transaction for the tracked expense
const [existingStoredTransaction] = useOnyx(
`${ONYXKEYS.COLLECTION.TRANSACTION}${getNonEmptyStringOnyxID(existingTransactionID)}`
);Or if you must keep the current approach, add a fallback:
const existingStoredTransaction = existingTransactionID
? transactions?.find((tx) => tx?.transactionID === existingTransactionID)
?? allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${existingTransactionID}`]
: undefined;Caution
This is what's causing this regression that I reported.
There was a problem hiding this comment.
I updated the PR, but I am not sure if this is a real concern. The useTransactionsByID hook uses ONYXKEYS.COLLECTION.TRANSACTION subscription internally.
Now this logic is moved to src/pages/iou/request/step/confirmation/useExpenseSubmission.ts
|
🔄 Once the ☝️ regression issue is addressed, I'll verify and approve as otherwise changes LGTM 🟢 |
|
Working on this. Looks like there has been many changes to these files. Will reapply changes. |
|
I can't reproduce this issue #86038 (comment). Maybe, at that time, the PR had outdated code. |
|
Ready for review @ikevin127 |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
Not sure what it was before, probably BE related as the issue is not reproducible anymore - good to merge!
|
Bump @tgolen |
|
|
||
| const [recentWaypoints] = useOnyx(ONYXKEYS.NVP_RECENT_WAYPOINTS); | ||
| const existingTransactionID = getExistingTransactionID(transaction?.linkedTrackedExpenseReportAction); | ||
| // `transaction` prop can be a transactionDraft or stored transaction. Here we will make sure to use stored transaction. |
There was a problem hiding this comment.
Can you please update this comment to explain why it uses a stored transaction and not the draft?
There was a problem hiding this comment.
Done. Simplified and updated.
|
@tgolen let's merge so that I can put up next. |
|
@danieldoglas looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @danieldoglas has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.3.79-1 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is an internal code refactoring that changes how the |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.79-4 🚀
|
6 participants
Explanation of Change
Fixed Issues
$ #66510
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
02.04.2026_18.44.13_REC.mp4
02.04.2026_18.54.02_REC.mp4