Delete all linked report when clearing optimistic chat and transaction error #44923
Conversation
|
Having a problem with iOS native, really lagging. |
|
Reviewing today. @bernhardoj Meanwhile, can you please fix the lint error? |
|
Fixed. |
|
Whoops, another lint. |
|
Everything should be fixed now |
There was a problem hiding this comment.
@bernhardoj I noticed a following error during testing (i.e. while clearing the errors) but I only got that once. And I am not sure what exactly caused this. Looking strictly from the code perspective, I think we can keep regressions at bay if we do an early return as mentioned in my review comment. Please have a look. Overall the code LGTM though.
| Object.values(transaction?.errors ?? {})?.find((error) => ErrorUtils.isReceiptError(error)) | ||
| ) { | ||
| deleteTransaction(parentReport, parentReportAction); | ||
| if (transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { |
There was a problem hiding this comment.
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?
if (ReportUtils.getAddWorkspaceRoomOrChatReportErrors(chatReportID)) {
Report.navigateToConciergeChatAndDeleteReport(chatReportID, true);
return;
}
if (Object.values(transaction?.errors ?? {})?.find((error) => ErrorUtils.isReceiptError(error))) {
deleteTransaction(parentReport, parentReportAction);
}
There was a problem hiding this comment.
That makes sense, updated!
src/libs/TransactionUtils.ts
Outdated
| @@ -581,7 +581,7 @@ function getAllReportTransactions(reportID?: string, transactions?: OnyxCollecti | |||
| // `reportID` from the `/CreateDistanceRequest` endpoint return's number instead of string for created `transaction`. | |||
| // For reference, https://github.com/Expensify/App/pull/26536#issuecomment-1703573277. | |||
| // We will update this in a follow-up Issue. According to this comment: https://github.com/Expensify/App/pull/26536#issuecomment-1703591019. | |||
| const nonNullableTransactions: Transaction[] = Object.values(transactions ?? allTransactions ?? {}).filter((transaction): transaction is Transaction => transaction !== null); | |||
| const nonNullableTransactions = Object.values(transactions ?? allTransactions ?? {}).filter(transaction => !!transaction); | |||
There was a problem hiding this comment.
As for the error, we don't filter out undefined values here, which caused the crash, so I update it to include that here too.
There was a problem hiding this comment.
Thanks @bernhardoj for the changes. LGTM and works well too.
@arosiclair Over to you for review.
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari44293-web-safari-004.mp4MacOS: Desktop44923-desktop-004.mp4Android: Native44923-android-native-004.mp4Android: mWeb Chrome44923-mweb-chrome-004.mp4iOS: Native44923-ios-native-004.mp4iOS: mWeb Safari44923-mweb-safari-004.mp4 |
src/libs/ReportUtils.ts
Outdated
| @@ -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.
We shouldn't fetch the report in the lib here if we don't need to. Can we just get the report in the MoneyRequestView with useOnyx and pass it in?
src/libs/actions/Report.ts
Outdated
| if (!reportAction.childReportID) { | ||
| return; | ||
| } | ||
| deleteReport(reportAction.childReportID); |
There was a problem hiding this comment.
We should probably not always delete child reports. Can we put this behind a shouldDeleteChildReports flag?
|
@bernhardoj The recent changes seem not to work as the navigation to concierge chat fails. Please have a look at the test video below. 44923-web-safari-i-1.mp4 |
|
It works fine for me even though it redirects to the concierge chat instead of the concierge chat itself, but that's another bug of web.1.mp4 |
There was a problem hiding this comment.
I did a clean build and did not notice the problem as observed here. We can ignore that.
I also observed the behavior as mentioned here with navigation to concierge chat (as shown in the video below for mweb). Seems intentional but not sure. Anyway, that is a matter of another discussion.
Meanwhile, the code changes LGTM and tests well too.
44923-mweb-safari-i-2.mp4
44923-web-safari-i-2.mp4
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.0.7-3 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 9.0.7-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀
|
Details
Clearing transaction error doesn't delete the report. This PR fix it by deleting all linked report of the transaction and its chat report.
Fixed Issues
$ #43481
PROPOSAL: #43481 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web_error.mp4
MacOS: Desktop
desktop_error.mp4