-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix: Expense - Not here page shows up briefly when deleting the expense #52740
base: main
Are you sure you want to change the base?
Changes from all commits
7d69e66
b32c78b
bc8f8ed
f6781e6
5b205e6
0bd45cc
eb051bb
c2cecdc
7e8d4f4
66e3de8
9ec0a26
9bce4f0
e2cd642
980e8e5
0288efb
aef7070
5d982cc
4447344
71c3d41
82668c5
390d357
576e194
7145f31
1256e03
a343297
e41e182
7123b2f
19dccb4
6c96672
38a7323
964626b
67eb827
62aaa28
051d3cc
0887779
78f8f71
df0100f
0ea2bba
9c0c5d1
9cedf92
65199e2
213fe69
fee7dc7
1f6a99d
51daaa0
6b0b5a6
5e5026d
5ddba66
71728f6
fd39c99
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 |
---|---|---|
|
@@ -60,6 +60,7 @@ import type {IOUAction, IOUType} from '@src/CONST'; | |
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import ROUTES from '@src/ROUTES'; | ||
import type {Route} from '@src/ROUTES'; | ||
import type * as OnyxTypes from '@src/types/onyx'; | ||
import type {Attendee, Participant, Split} from '@src/types/onyx/IOU'; | ||
import type {ErrorFields, Errors} from '@src/types/onyx/OnyxCommon'; | ||
|
@@ -1784,10 +1785,21 @@ function getDeleteTrackExpenseInformation( | |
|
||
if (shouldDeleteTransactionThread) { | ||
optimisticData.push( | ||
// Use merge instead of set to avoid deleting the report too quickly, which could cause a brief "not found" page to appear. | ||
// The remaining parts of the report object will be removed after the API call is successful. | ||
{ | ||
onyxMethod: Onyx.METHOD.SET, | ||
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
value: null, | ||
value: { | ||
reportID: null, | ||
stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
statusNum: CONST.REPORT.STATUS_NUM.CLOSED, | ||
participants: { | ||
[userAccountID]: { | ||
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
onyxMethod: Onyx.METHOD.SET, | ||
|
@@ -1827,6 +1839,19 @@ function getDeleteTrackExpenseInformation( | |
}, | ||
]; | ||
|
||
// Ensure that any remaining data is removed upon successful completion, even if the server sends a report removal response. | ||
// This is done to prevent the removal update from lingering in the applyHTTPSOnyxUpdates function. | ||
if (shouldDeleteTransactionThread && transactionThread) { | ||
successData.push({ | ||
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
value: Object.keys(transactionThread).reduce<Record<string, null>>((acc, key) => { | ||
acc[key] = null; | ||
return acc; | ||
}, {}), | ||
}); | ||
} | ||
|
||
Comment on lines
+1844
to
+1854
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. Why is this needed? 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. @luacmartins Ensure that any remaining data is removed upon successful completion, even if the server sends a report removal response. This is done to prevent the removal update from lingering in the 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. Same here, let's add a comment about it. We should add a comment for all instances to make sure people don't revert this 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. done |
||
const failureData: OnyxUpdate[] = []; | ||
|
||
if (shouldDeleteTransactionFromOnyx) { | ||
|
@@ -5393,10 +5418,9 @@ function updateMoneyRequestAmountAndCurrency({ | |
* | ||
* @param transactionID - The transactionID of IOU | ||
* @param reportAction - The reportAction of the transaction in the IOU report | ||
* @param isSingleTransactionView - whether we are in the transaction thread report | ||
* @return the url to navigate back once the money request is deleted | ||
*/ | ||
function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxTypes.ReportAction, isSingleTransactionView = false) { | ||
function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxTypes.ReportAction) { | ||
// STEP 1: Get all collections we're updating | ||
const allReports = ReportConnection.getAllReports(); | ||
const iouReportID = ReportActionsUtils.isMoneyRequestAction(reportAction) ? ReportActionsUtils.getOriginalMessage(reportAction)?.IOUReportID : '-1'; | ||
|
@@ -5497,19 +5521,6 @@ function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxT | |
updatedReportPreviewAction.childMoneyRequestCount = reportPreviewAction.childMoneyRequestCount - 1; | ||
} | ||
|
||
// STEP 5: Calculate the url that the user will be navigated back to | ||
// This depends on which page they are on and which resources were deleted | ||
let reportIDToNavigateBack: string | undefined; | ||
if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) { | ||
reportIDToNavigateBack = iouReport.reportID; | ||
} | ||
|
||
if (iouReport?.chatReportID && shouldDeleteIOUReport) { | ||
reportIDToNavigateBack = iouReport.chatReportID; | ||
} | ||
|
||
const urlToNavigateBack = reportIDToNavigateBack ? ROUTES.REPORT_WITH_ID.getRoute(reportIDToNavigateBack) : undefined; | ||
|
||
return { | ||
shouldDeleteTransactionThread, | ||
shouldDeleteIOUReport, | ||
|
@@ -5523,10 +5534,59 @@ function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxT | |
transactionViolations, | ||
reportPreviewAction, | ||
iouReport, | ||
urlToNavigateBack, | ||
}; | ||
} | ||
|
||
/** | ||
* Calculate the URL to navigate to after a money request deletion | ||
* @param transactionID - The ID of the money request being deleted | ||
* @param reportAction - The report action associated with the money request | ||
* @param isSingleTransactionView - whether we are in the transaction thread report | ||
* @returns The URL to navigate to | ||
*/ | ||
function getNavigationUrlOnMoneyRequestDelete(transactionID: string, reportAction: OnyxTypes.ReportAction, isSingleTransactionView = false): Route | undefined { | ||
const {shouldDeleteTransactionThread, shouldDeleteIOUReport, iouReport} = prepareToCleanUpMoneyRequest(transactionID, reportAction); | ||
|
||
// Determine which report to navigate back to | ||
if (iouReport && isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) { | ||
return ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID); | ||
} | ||
|
||
if (iouReport?.chatReportID && shouldDeleteIOUReport) { | ||
return ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID); | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
/** | ||
* Calculate the URL to navigate to after a track expense deletion | ||
* @param chatReportID - The ID of the chat report containing the track expense | ||
* @param transactionID - The ID of the track expense being deleted | ||
* @param reportAction - The report action associated with the track expense | ||
* @param isSingleTransactionView - Whether we're in single transaction view | ||
* @returns The URL to navigate to | ||
*/ | ||
function getNavigationUrlAfterTrackExpenseDelete(chatReportID: string, transactionID: string, reportAction: OnyxTypes.ReportAction, isSingleTransactionView = false): Route | undefined { | ||
const chatReport = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`] ?? null; | ||
|
||
// If not a self DM, handle it as a regular money request | ||
if (!ReportUtils.isSelfDM(chatReport)) { | ||
return getNavigationUrlOnMoneyRequestDelete(transactionID, reportAction, isSingleTransactionView); | ||
} | ||
|
||
const transactionThreadID = reportAction.childReportID; | ||
const shouldDeleteTransactionThread = transactionThreadID ? (reportAction?.childVisibleActionCount ?? 0) === 0 : false; | ||
|
||
// Only navigate if in single transaction view and the thread will be deleted | ||
if (isSingleTransactionView && shouldDeleteTransactionThread && chatReport?.reportID) { | ||
// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report. | ||
return ROUTES.REPORT_WITH_ID.getRoute(chatReport.reportID); | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
/** | ||
* | ||
* @param transactionID - The transactionID of IOU | ||
|
@@ -5545,9 +5605,9 @@ function cleanUpMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repo | |
chatReport, | ||
iouReport, | ||
reportPreviewAction, | ||
urlToNavigateBack, | ||
} = prepareToCleanUpMoneyRequest(transactionID, reportAction, isSingleTransactionView); | ||
} = prepareToCleanUpMoneyRequest(transactionID, reportAction); | ||
|
||
const urlToNavigateBack = getNavigationUrlOnMoneyRequestDelete(transactionID, reportAction, isSingleTransactionView); | ||
// build Onyx data | ||
|
||
// Onyx operations to delete the transaction, update the IOU report action and chat report action | ||
|
@@ -5691,8 +5751,9 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor | |
transactionViolations, | ||
iouReport, | ||
reportPreviewAction, | ||
urlToNavigateBack, | ||
} = prepareToCleanUpMoneyRequest(transactionID, reportAction, isSingleTransactionView); | ||
} = prepareToCleanUpMoneyRequest(transactionID, reportAction); | ||
|
||
const urlToNavigateBack = getNavigationUrlOnMoneyRequestDelete(transactionID, reportAction, isSingleTransactionView); | ||
|
||
// STEP 2: Build Onyx data | ||
// The logic mostly resembles the cleanUpMoneyRequest function | ||
|
@@ -5712,10 +5773,21 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor | |
|
||
if (shouldDeleteTransactionThread) { | ||
optimisticData.push( | ||
// Use merge instead of set to avoid deleting the report too quickly, which could cause a brief "not found" page to appear. | ||
// The remaining parts of the report object will be removed after the API call is successful. | ||
{ | ||
onyxMethod: Onyx.METHOD.SET, | ||
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
value: null, | ||
value: { | ||
reportID: null, | ||
stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
statusNum: CONST.REPORT.STATUS_NUM.CLOSED, | ||
participants: { | ||
[userAccountID]: { | ||
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
}, | ||
}, | ||
}, | ||
Comment on lines
+5781
to
+5790
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. Same here 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. done |
||
}, | ||
{ | ||
onyxMethod: Onyx.METHOD.SET, | ||
|
@@ -5813,6 +5885,19 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor | |
}, | ||
]; | ||
|
||
// Ensure that any remaining data is removed upon successful completion, even if the server sends a report removal response. | ||
// This is done to prevent the removal update from lingering in the applyHTTPSOnyxUpdates function. | ||
if (shouldDeleteTransactionThread && transactionThread) { | ||
successData.push({ | ||
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
value: Object.keys(transactionThread).reduce<Record<string, null>>((acc, key) => { | ||
acc[key] = null; | ||
return acc; | ||
}, {}), | ||
}); | ||
} | ||
|
||
Comment on lines
+5890
to
+5900
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. and here 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. done |
||
if (shouldDeleteIOUReport) { | ||
successData.push({ | ||
onyxMethod: Onyx.METHOD.SET, | ||
|
@@ -5916,15 +6001,18 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor | |
} | ||
|
||
function deleteTrackExpense(chatReportID: string, transactionID: string, reportAction: OnyxTypes.ReportAction, isSingleTransactionView = false) { | ||
const urlToNavigateBack = getNavigationUrlAfterTrackExpenseDelete(chatReportID, transactionID, reportAction, isSingleTransactionView); | ||
|
||
// STEP 1: Get all collections we're updating | ||
const chatReport = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`] ?? null; | ||
if (!ReportUtils.isSelfDM(chatReport)) { | ||
return deleteMoneyRequest(transactionID, reportAction, isSingleTransactionView); | ||
deleteMoneyRequest(transactionID, reportAction, isSingleTransactionView); | ||
return urlToNavigateBack; | ||
} | ||
|
||
const whisperAction = ReportActionsUtils.getTrackExpenseActionableWhisper(transactionID, chatReportID); | ||
const actionableWhisperReportActionID = whisperAction?.reportActionID; | ||
const {parameters, optimisticData, successData, failureData, shouldDeleteTransactionThread} = getDeleteTrackExpenseInformation( | ||
const {parameters, optimisticData, successData, failureData} = getDeleteTrackExpenseInformation( | ||
chatReportID, | ||
transactionID, | ||
reportAction, | ||
|
@@ -5939,10 +6027,7 @@ function deleteTrackExpense(chatReportID: string, transactionID: string, reportA | |
CachedPDFPaths.clearByKey(transactionID); | ||
|
||
// STEP 7: Navigate the user depending on which page they are on and which resources were deleted | ||
if (isSingleTransactionView && shouldDeleteTransactionThread) { | ||
// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report. | ||
return ROUTES.REPORT_WITH_ID.getRoute(chatReport?.reportID ?? '-1'); | ||
} | ||
return urlToNavigateBack; | ||
} | ||
|
||
/** | ||
|
@@ -8488,5 +8573,7 @@ export { | |
updateLastLocationPermissionPrompt, | ||
resolveDuplicates, | ||
getIOUReportActionToApproveOrPay, | ||
getNavigationUrlOnMoneyRequestDelete, | ||
getNavigationUrlAfterTrackExpenseDelete, | ||
}; | ||
export type {GPSPoint as GpsPoint, IOURequestType}; |
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.
Why are we changing this? When the transaction is deleted we should delete the transaction thread, no?
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.
@luacmartins We switched from using
SET
toMERGE
to prevent the appearance of the not found page due to platforms updating the value too quickly. This change has already been made forleaveRoom
andleaveGroupChat
to address a similar issue #49468.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.
Thanks for clarifying. I think that makes sense, it just feels a bit odd to do that. Let's add a comment explaining why we're doing it so people don't try to revert this change
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.
done