-
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
fix: delete money request if failed to create chat #44142
Changes from 16 commits
dcc9ac9
ed0b37e
caae66e
7936055
68c4ca7
083c790
5951c38
7a9c89b
b383383
65bd81d
9b67b44
041bed4
41768f4
bd32dd9
0590632
21eafea
3730797
83c0c3f
51b8106
4b79f38
313b1ca
6ea8fc6
d71f595
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 |
---|---|---|
|
@@ -113,7 +113,7 @@ function MoneyRequestView({ | |
const styles = useThemeStyles(); | ||
const session = useSession(); | ||
const {isOffline} = useNetwork(); | ||
const {translate, toLocaleDigit} = useLocalize(); | ||
const {translate, toLocaleDigit, swapForTranslation} = useLocalize(); | ||
const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID); | ||
|
||
const parentReportAction = parentReportActions?.[report.parentReportActionID ?? '-1']; | ||
|
@@ -349,10 +349,18 @@ function MoneyRequestView({ | |
const shouldShowNotesViolations = !isReceiptBeingScanned && canUseViolations && ReportUtils.isPaidGroupPolicy(report); | ||
const shouldShowReceiptHeader = isReceiptAllowed && (shouldShowReceiptEmptyState || hasReceipt); | ||
|
||
const errors = { | ||
...(transaction?.errorFields?.route ?? transaction?.errors), | ||
...parentReportAction?.errors, | ||
}; | ||
const errors = useMemo(() => { | ||
const combinedErrors = { | ||
...(transaction?.errorFields?.route ?? transaction?.errors), | ||
...parentReportAction?.errors, | ||
}; | ||
return Object.fromEntries( | ||
Object.entries(combinedErrors).map(([key, value]) => | ||
// swap for translation for each error message | ||
[key, swapForTranslation(value as string)], | ||
), | ||
); | ||
}, [transaction?.errorFields?.route, transaction?.errors, parentReportAction?.errors, swapForTranslation]); | ||
|
||
const tagList = policyTagLists.map(({name, orderWeight}, index) => { | ||
const tagError = getErrorForField( | ||
|
@@ -404,6 +412,13 @@ function MoneyRequestView({ | |
if (!transaction?.transactionID) { | ||
return; | ||
} | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
if ((!!report?.errorFields?.createChat || !!report.isOptimisticReport) && parentReportAction) { | ||
dominictb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const urlToNavigateBack = IOU.cleanUpMoneyRequest(transaction.transactionID, parentReportAction, true); | ||
Navigation.goBack(urlToNavigateBack); | ||
Comment on lines
+416
to
+417
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. FYI, this caused the following issue: #50022 |
||
return; | ||
} | ||
|
||
if ( | ||
transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && | ||
Object.values(transaction?.errors ?? {})?.find((error) => ErrorUtils.isReceiptError(error)) | ||
|
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. Can we put these in CONST.ts instead, and use constants for the keys and values? I think we wouldn't have to eslint disable then and it would fit with our existing code patterns a little better. 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. Sure, but I'll wait for the team to decide on the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
const mapping: Record<string, string> = { | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
'The workspace is no longer accessible. Please try again on a different workspace.': 'iou.error.workspaceNotAccessible', | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
'Unexpected error submitting this expense. Please try again later.': 'iou.error.genericCreateFailureMessage', | ||
}; | ||
|
||
export default mapping; |
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.
This is kind of a new pattern, so I want to check with the team first to see if everyone is cool with it, as recommended by our checklist.
I don't really like that we're doing the swapping within the component. Ideally the swapping would be done when saving an Onyx update from the network or pusher. Let's pause this work until we hear what the team thinks. Sorry for the delay, but if we build this right it will be useful in the future instead of a headache.
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.
The approval conversation is here
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.
Sorry, do you mean we should pause this PR or I should revert all the
swapForTranslation
change in this PR (in case the team is indecisive for a long time)?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.
It's not easy to do that since usually the error message returned from the BE is in the BE onyxData, which we don't have much control.
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.
@dominictb yeah, please revert the
swapForTranslation
changes. I think we'll just deal with the slightly strange non-translated error message.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.
on it now.
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.
@neil-marcellini reverted