Move RBR to workspace chats instead of transaction threads #51893
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6316,65 +6316,53 @@ function shouldHideReport(report: OnyxEntry<Report>, currentReportId: string): b | |
| } | ||
|
|
||
| /** | ||
| * Should we display a RBR on the LHN on this report due to violations? | ||
| */ | ||
| function shouldDisplayViolationsRBRInLHN(report: OnyxEntry<Report>, transactionViolations: OnyxCollection<TransactionViolation[]>): boolean { | ||
| // We only show the RBR in the highest level, which is the workspace chat | ||
| if (!report || !isPolicyExpenseChat(report)) { | ||
| return false; | ||
| } | ||
|
|
||
| // We only show the RBR to the submitter | ||
| if (!isCurrentUserSubmitter(report.reportID ?? '')) { | ||
| return false; | ||
| } | ||
|
|
||
| // Get all potential reports, which are the ones that are: | ||
| // - Owned by the same user | ||
| // - Are either open or submitted | ||
| // - Belong to the same workspace | ||
| // And if any have a violation, then it should have a RBR | ||
|
There was a problem hiding this comment. I see this condition was deleted There was a problem hiding this comment. That's done with |
||
| const allReports = Object.values(ReportConnection.getAllReports() ?? {}) as Report[]; | ||
| const potentialReports = allReports.filter((r) => r.ownerAccountID === currentUserAccountID && (r.stateNum ?? 0) <= 1 && r.policyID === report.policyID); | ||
| return potentialReports.some( | ||
| (potentialReport) => hasViolations(potentialReport.reportID, transactionViolations) || hasWarningTypeViolations(potentialReport.reportID, transactionViolations), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Checks to see if a report contains a violation | ||
| */ | ||
| function hasViolations(reportID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, shouldShowInReview?: boolean): boolean { | ||
|
There was a problem hiding this comment. Can we make the last param mandatory? There was a problem hiding this comment. In the places we are calling this that I did not update, we do not care about the state of this variable, so no. |
||
| const transactions = reportsTransactions[reportID] ?? []; | ||
| return transactions.some((transaction) => TransactionUtils.hasViolation(transaction.transactionID, transactionViolations, shouldShowInReview)); | ||
| } | ||
|
|
||
| /** | ||
| * Checks to see if a report contains a violation of type `warning` | ||
| */ | ||
| function hasWarningTypeViolations(reportID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, shouldShowInReview?: boolean): boolean { | ||
|
There was a problem hiding this comment. Can we make the last param mandatory? |
||
| const transactions = reportsTransactions[reportID] ?? []; | ||
| return transactions.some((transaction) => TransactionUtils.hasWarningTypeViolation(transaction.transactionID, transactionViolations, shouldShowInReview)); | ||
| } | ||
|
|
||
| /** | ||
| * Checks to see if a report contains a violation of type `notice` | ||
| */ | ||
| function hasNoticeTypeViolations(reportID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, shouldShowInReview?: boolean): boolean { | ||
| const transactions = reportsTransactions[reportID] ?? []; | ||
| return transactions.some((transaction) => TransactionUtils.hasNoticeTypeViolation(transaction.transactionID, transactionViolations, shouldShowInReview)); | ||
| } | ||
|
|
||
| function hasReportViolations(reportID: string) { | ||
|
|
@@ -6396,23 +6384,6 @@ function shouldAdminsRoomBeVisible(report: OnyxEntry<Report>): boolean { | |
| return true; | ||
| } | ||
|
|
||
| type ReportErrorsAndReportActionThatRequiresAttention = { | ||
| errors: ErrorFields; | ||
| reportAction?: OnyxEntry<ReportAction>; | ||
|
|
@@ -6495,7 +6466,7 @@ function hasReportErrorsOtherThanFailedReceipt(report: Report, doesReportHaveVio | |
| let doesTransactionThreadReportHasViolations = false; | ||
| if (oneTransactionThreadReportID) { | ||
| const transactionReport = getReport(oneTransactionThreadReportID); | ||
| doesTransactionThreadReportHasViolations = !!transactionReport && shouldDisplayViolationsRBRInLHN(transactionReport, transactionViolations); | ||
| } | ||
| return ( | ||
| doesTransactionThreadReportHasViolations || | ||
|
|
@@ -8489,7 +8460,6 @@ export { | |
| chatIncludesConcierge, | ||
| createDraftTransactionAndNavigateToParticipantSelector, | ||
| doesReportBelongToWorkspace, | ||
| findLastAccessedReport, | ||
| findSelfDMReportID, | ||
| formatReportLastMessageText, | ||
|
|
@@ -8592,6 +8562,7 @@ export { | |
| hasUpdatedTotal, | ||
| hasViolations, | ||
| hasWarningTypeViolations, | ||
| hasNoticeTypeViolations, | ||
| isActionCreator, | ||
| isAdminRoom, | ||
| isAdminsOnlyPostingRoom, | ||
|
|
@@ -8693,7 +8664,7 @@ export { | |
| shouldDisableRename, | ||
| shouldDisableThread, | ||
| shouldDisplayThreadReplies, | ||
| shouldDisplayViolationsRBRInLHN, | ||
| shouldReportBeInOptionList, | ||
| shouldReportShowSubscript, | ||
| shouldShowFlagComment, | ||
|
|
@@ -8741,7 +8712,6 @@ export { | |
| buildOptimisticChangeFieldAction, | ||
| isPolicyRelatedReport, | ||
| hasReportErrorsOtherThanFailedReceipt, | ||
| getAllReportErrors, | ||
| getAllReportActionsErrorsAndReportActionThatRequiresAttention, | ||
| hasInvoiceReports, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -92,6 +92,9 @@ type TransactionViolation = { | |||||
|
|
||||||
| /** Additional violation information to provide the user */ | ||||||
| data?: TransactionViolationData; | ||||||
|
|
||||||
| /** Indicates if this violation should be shown in review */ | ||||||
| showInReview?: boolean; | ||||||
|
There was a problem hiding this comment. Does this need to be optional? There was a problem hiding this comment. Yes, because I am not sending it from PHP when it is not There was a problem hiding this comment.
Suggested change
This should be required to be set (and still allow for undefined). This is needed so we create the correct optimistic data. Not doing so led to a regression #54510 However, now we need to know which violations has There was a problem hiding this comment. Why would this need to be set for optimistic actions to work? 😕
I don't think there's a list, there's a bunch of logic controlling this property's value. There was a problem hiding this comment.
If we don't then expenses created offline that violate policy rules won't have the red dot set on their previews. This is due to this logic showInReview=true
There was a problem hiding this comment. Ah ok, so let's add that property to the optimistic actions (assuming the property will be removed when the data from the server is returned), but no need to mark this prop as required There was a problem hiding this comment. The problem with not making this required is that the bug could resurface (one could simply forget to add the field). Do you think we shouldn't optimize for this case? There was a problem hiding this comment. Do you think it actually prevents bugs? Allowing undefined gives the same signal to code authors as allowing it to not be passed, no? There was a problem hiding this comment. I see your point. Forcing the field to be set can reduce the occurrence of such bugs as you will get a chance to put in the correct value but it won't 100% prevent the bug. We will keep the types untouched. |
||||||
| }; | ||||||
|
|
||||||
| /** Collection of transaction violations */ | ||||||
|
|
||||||
This comment was marked as resolved.
Sorry, something went wrong.