Repository navigation
Support 'Select all matching' when moving expenses to a report #98411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
90880c4
2e2ff61
c499c08
a6de496
f38ed17
ee1098c
4377d98
dd9acb7
c1618dc
3aa6a19
0adb19e
a23a3a4
f8ca62f
c4dbba0
2549256
f08762f
7637c11
3f1a462
8f2b1cc
67bee6d
dc3215e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import DistanceRequestUtils from '@libs/DistanceRequestUtils'; | |
| import {getMicroSecondOnyxErrorWithTranslationKey} from '@libs/ErrorUtils'; | ||
| import {toLocaleDigit} from '@libs/LocaleDigitUtils'; | ||
| import {translateLocal} from '@libs/Localize'; | ||
| import Log from '@libs/Log'; | ||
| import {buildOptimisticNextStep} from '@libs/NextStepUtils'; | ||
| import * as NumberUtils from '@libs/NumberUtils'; | ||
| import {rand64, roundToTwoDecimalPlaces} from '@libs/NumberUtils'; | ||
|
|
@@ -882,6 +883,8 @@ type ChangeTransactionsReportProps = { | |
| getCurrencyDecimals: CurrencyListActionsContextType['getCurrencyDecimals']; | ||
| getCurrencySymbol: CurrencyListActionsContextType['getCurrencySymbol']; | ||
| isVendorMatchingBetaEnabled: boolean | undefined; | ||
| jsonQuery?: string; | ||
| hash?: number; | ||
| }; | ||
|
|
||
| function getChangeTransactionsReportOnyxData({ | ||
|
|
@@ -2076,6 +2079,60 @@ function getChangeTransactionsReportOnyxData({ | |
| } | ||
|
|
||
| function changeTransactionsReport(props: ChangeTransactionsReportProps) { | ||
| const reportID = props.newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID; | ||
|
|
||
| // Without a hash the move falls back to the explicit list and only the loaded page moves, so surface that | ||
| if (props.jsonQuery && props.hash === undefined) { | ||
| Log.warn('changeTransactionsReport: received an all-matching jsonQuery without a hash; falling back to the explicit transaction list, which only moves the loaded transactions.'); | ||
| } | ||
|
|
||
| if (props.jsonQuery && props.hash !== undefined) { | ||
| // The backend moves the full query result while the client only has loaded transactions. | ||
| // Apply normal optimistic updates to loaded transactions, then send the request even if none are loaded. | ||
| const loadedTransactionsOnyxData = getChangeTransactionsReportOnyxData(props); | ||
|
|
||
| const optimisticData = [...(loadedTransactionsOnyxData?.optimisticData ?? [])]; | ||
| const successData = [...(loadedTransactionsOnyxData?.successData ?? [])]; | ||
| const failureData = [...(loadedTransactionsOnyxData?.failureData ?? [])]; | ||
|
|
||
| if (props.newReport) { | ||
| // Unloaded expenses are still moving server-side, so the destination stays pending for the whole request | ||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, | ||
| value: {pendingFields: {reportID: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}}, | ||
| }); | ||
| successData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, | ||
| value: {pendingFields: {reportID: null}}, | ||
| }); | ||
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, | ||
| value: {pendingFields: {reportID: null}}, | ||
| }); | ||
| } | ||
|
|
||
| const transactionIDToUpdatedCustomUnitRateID = loadedTransactionsOnyxData?.transactionIDToUpdatedCustomUnitRateID ?? {}; | ||
|
|
||
| const queryParameters: ChangeTransactionsReportParams = { | ||
| // Stays empty so the backend moves every matching expense from the query, not just the loaded page | ||
| transactionList: '', | ||
| reportID, | ||
| // Send the optimistic action and thread IDs so the backend reuses them instead of adding a second moved message | ||
| transactionIDToReportActionAndThreadData: JSON.stringify(loadedTransactionsOnyxData?.transactionIDToReportActionAndThreadData ?? {}), | ||
| ...(Object.keys(transactionIDToUpdatedCustomUnitRateID).length > 0 && { | ||
| transactionIDToUpdatedCustomUnitRateID: JSON.stringify(transactionIDToUpdatedCustomUnitRateID), | ||
| }), | ||
| jsonQuery: props.jsonQuery, | ||
| hash: props.hash, | ||
| }; | ||
|
|
||
| API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, queryParameters, {optimisticData, successData, failureData}); | ||
| return; | ||
| } | ||
|
|
||
| const changeTransactionsReportOnyxData = getChangeTransactionsReportOnyxData(props); | ||
| if (!changeTransactionsReportOnyxData) { | ||
| return; | ||
|
|
@@ -2088,8 +2145,6 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { | |
| return; | ||
| } | ||
|
|
||
| const reportID = props.newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID; | ||
|
|
||
| const parameters: ChangeTransactionsReportParams = { | ||
| transactionList: movedTransactionIDs.join(','), | ||
| reportID, | ||
|
|
@@ -2099,6 +2154,8 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { | |
| }), | ||
| }; | ||
|
|
||
| // The all-matching path above already returned after its own API.write, so only one of the two writes ever runs | ||
| // eslint-disable-next-line rulesdir/no-multiple-api-calls | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-5 (docs)This Add a justification comment above (or after) the disable, e.g.: // This second API.write is intentional: the query-based all-matching path above already returned, so this branch only runs for the explicit-transaction move.
// eslint-disable-next-line rulesdir/no-multiple-api-calls
API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, parameters, {If this is a temporary suppression, prefer widening the baseline via Reviewed at: a6de496 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f38ed17. Added a justification comment above the See |
||
| API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, parameters, { | ||
| optimisticData, | ||
| successData, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a
status:unreportedsearch spanning multiple pages, this bypasseshasUnloadedMatchingExpensesafter checkingcanChangeReportonly on the loaded rows. Unreported expenses are not universally movable:canUnreportedBeMoved()rejects per-diem transactions when their policy or rates are unavailable, so an invalid expense on a later page makes the backend reject the entire query move even though this option is offered. Do not treat the status filter alone as proof that all unseen matches are eligible.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — this is a real edge case, but I didn't change the behavior here because the honest fix is a product tradeoff that belongs to the PR author rather than a mechanical change, and I don't want to silently reverse the deliberate
|| isUnreportedOnlyQuerybypass.The core constraint: the client can't enumerate the unloaded matches, so it can't prove every unseen unreported expense passes
canUnreportedBeMoved()(per-diem with unavailable policy/rates being the counter-example you flagged). The backend is the authoritative enforcer either way. Two options:pendingFields.reportID— same silent behavior as the existing explicit-list path, so it isn't a regression, but it's worth fixing).!hasUnloadedMatchingExpenses. The move option then only appears when the client has loaded every match, socanAllTransactionsBeMovedfully covers the set. Cost: all-matching move isn't offered for unreported searches with more than one page of results.I'd lean toward (2) if guaranteed correctness matters more than covering the >1-page unreported case.
ZhenjaHorbach— which tradeoff do you want? Reply and I'll implement it.