-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Delete all linked report when clearing optimistic chat and transaction error #44923
Changes from 10 commits
6363963
0925b52
af16ea6
35cd311
51cf9e9
4e0171d
1a0c0c3
14aceb5
360f07c
5fd50f0
fccb7cb
4b97867
81241ac
ba95292
1e6c518
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 |
---|---|---|
|
@@ -6113,7 +6113,8 @@ function isValidReportIDFromPath(reportIDFromPath: string): boolean { | |
/** | ||
* Return the errors we have when creating a chat or a workspace room | ||
*/ | ||
function getAddWorkspaceRoomOrChatReportErrors(report: OnyxEntry<Report>): Errors | null | undefined { | ||
function getAddWorkspaceRoomOrChatReportErrors(reportOrID: OnyxEntry<Report> | string): Errors | null | undefined { | ||
const report = typeof reportOrID === 'string' ? ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportOrID}`] : reportOrID; | ||
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. We shouldn't fetch the report in the lib here if we don't need to. Can we just get the report 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. Done |
||
// We are either adding a workspace room, or we're creating a chat, it isn't possible for both of these to have errors for the same report at the same time, so | ||
// simply looking up the first truthy value will get the relevant property if it's set. | ||
return report?.errorFields?.addWorkspaceRoom ?? report?.errorFields?.createChat; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2168,6 +2168,13 @@ function deleteReport(reportID: string) { | |
|
||
Onyx.multiSet(onyxData); | ||
|
||
Object.values(reportActionsForReport ?? {}).forEach((reportAction) => { | ||
if (!reportAction.childReportID) { | ||
return; | ||
} | ||
deleteReport(reportAction.childReportID); | ||
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. We should probably not always delete child reports. Can we put this behind a 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 |
||
}); | ||
|
||
// Delete linked IOU report | ||
if (report?.iouReportID) { | ||
deleteReport(report.iouReportID); | ||
|
@@ -2177,9 +2184,12 @@ function deleteReport(reportID: string) { | |
/** | ||
* @param reportID The reportID of the policy report (workspace room) | ||
*/ | ||
function navigateToConciergeChatAndDeleteReport(reportID: string) { | ||
function navigateToConciergeChatAndDeleteReport(reportID: string, shouldPopToTop?: boolean) { | ||
// Dismiss the current report screen and replace it with Concierge Chat | ||
Navigation.goBack(); | ||
if (shouldPopToTop) { | ||
Navigation.setShouldPopAllStateOnUP(true); | ||
} | ||
Navigation.goBack(undefined, undefined, shouldPopToTop); | ||
navigateToConciergeChat(); | ||
deleteReport(reportID); | ||
} | ||
|
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.
Let us do an early return here to keep possible regressions at bay. Also, I think doing any further steps does not make sense when the chat report and all related reports are deleted. Something like this can be done within the
if
condition here. What do you think?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.
That makes sense, updated!