Skip to content
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 details - Violation is not displayed on description field when opening IOU details. #56405

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
4 changes: 2 additions & 2 deletions src/components/BrokenConnectionDescription.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import React from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionViolations from '@hooks/useTransactionViolations';
import {isInstantSubmitEnabled, isPolicyAdmin as isPolicyAdminPolicyUtils} from '@libs/PolicyUtils';
import {isCurrentUserSubmitter, isProcessingReport, isReportApproved, isReportManuallyReimbursed} from '@libs/ReportUtils';
import {getTransactionViolations} from '@libs/TransactionUtils';
import Navigation from '@navigation/Navigation';
import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
Expand All @@ -24,7 +24,7 @@ type BrokenConnectionDescriptionProps = {
function BrokenConnectionDescription({transactionID, policy, report}: BrokenConnectionDescriptionProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const transactionViolations = getTransactionViolations(transactionID);
const transactionViolations = useTransactionViolations(transactionID);

const brokenConnection530Error = transactionViolations?.find((violation) => violation.data?.rterType === CONST.RTER_VIOLATION_TYPES.BROKEN_CARD_CONNECTION_530);
const brokenConnectionError = transactionViolations?.find((violation) => violation.data?.rterType === CONST.RTER_VIOLATION_TYPES.BROKEN_CARD_CONNECTION);
Expand Down
5 changes: 3 additions & 2 deletions src/components/MoneyReportHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
const isOnHold = isOnHoldTransactionUtils(transaction);
const isDeletedParentAction = !!requestParentReportAction && isDeletedAction(requestParentReportAction);
const isDuplicate = isDuplicateTransactionUtils(transaction?.transactionID);
const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB because we need to get this merged and CP'd, but we could also optimize this to only select the violations for the transactions that we care about, so the component doesn't re-render if other violations change:

diff --git a/src/components/MoneyReportHeader.tsx b/src/components/MoneyReportHeader.tsx
index 45585a03b14..f157668ee44 100644
--- a/src/components/MoneyReportHeader.tsx
+++ b/src/components/MoneyReportHeader.tsx
@@ -133,7 +133,6 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
     const isOnHold = isOnHoldTransactionUtils(transaction);
     const isDeletedParentAction = !!requestParentReportAction && isDeletedAction(requestParentReportAction);
     const isDuplicate = isDuplicateTransactionUtils(transaction?.transactionID);
-    const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
 
     // Only the requestor can delete the request, admins can only edit it.
     const isActionOwner =
@@ -151,8 +150,12 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
         return !!transactions && transactions.length > 0 && transactions.every((t) => isExpensifyCardTransaction(t) && isPending(t));
     }, [transactions]);
     const transactionIDs = transactions?.map((t) => t.transactionID) ?? [];
-    const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs, allTransactionViolations);
-    const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDs, moneyRequestReport, policy, allTransactionViolations);
+    const [violations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, {
+        selector: (allTransactions) =>
+            Object.fromEntries(Object.entries(allTransactions ?? {}).filter(([key]) => transactionIDs.includes(key.replace(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, '')))),
+    });
+    const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs, violations);
+    const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDs, moneyRequestReport, policy, violations);
     const hasOnlyHeldExpenses = hasOnlyHeldExpensesReportUtils(moneyRequestReport?.reportID);
     const isPayAtEndExpense = isPayAtEndExpenseTransactionUtils(transaction);
     const isArchivedReport = isArchivedReportWithID(moneyRequestReport?.reportID);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a lot of optimizations have gone in here and I think we can optimize this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rojiphil I have updated this. Please review and let me know if there's something left. Thanks.


// Only the requestor can delete the request, admins can only edit it.
const isActionOwner =
Expand All @@ -150,8 +151,8 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
return !!transactions && transactions.length > 0 && transactions.every((t) => isExpensifyCardTransaction(t) && isPending(t));
}, [transactions]);
const transactionIDs = transactions?.map((t) => t.transactionID) ?? [];
const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs);
const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDs, moneyRequestReport, policy);
const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs, allTransactionViolations);
const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDs, moneyRequestReport, policy, allTransactionViolations);
const hasOnlyHeldExpenses = hasOnlyHeldExpensesReportUtils(moneyRequestReport?.reportID);
const isPayAtEndExpense = isPayAtEndExpenseTransactionUtils(transaction);
const isArchivedReport = isArchivedReportWithID(moneyRequestReport?.reportID);
Expand Down
10 changes: 6 additions & 4 deletions src/components/MoneyRequestHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import useLocalize from '@hooks/useLocalize';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionViolations from '@hooks/useTransactionViolations';
import {markAsCash as markAsCashUtil} from '@libs/actions/Transaction';
import Navigation from '@libs/Navigation/Navigation';
import {isPolicyAdmin} from '@libs/PolicyUtils';
import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils';
import {isCurrentUserSubmitter} from '@libs/ReportUtils';
import {
allHavePendingRTERViolation,
getTransactionViolations,
hasPendingRTERViolation,
hasReceipt,
isDuplicate as isDuplicateTransactionUtils,
Expand Down Expand Up @@ -67,6 +67,8 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
isMoneyRequestAction(parentReportAction) ? getOriginalMessage(parentReportAction)?.IOUTransactionID ?? CONST.DEFAULT_NUMBER_ID : CONST.DEFAULT_NUMBER_ID
}`,
);
const transactionViolations = useTransactionViolations(transaction?.transactionID);
const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const [dismissedHoldUseExplanation, dismissedHoldUseExplanationResult] = useOnyx(ONYXKEYS.NVP_DISMISSED_HOLD_USE_EXPLANATION, {initialValue: true});
const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);
const isLoadingHoldUseExplained = isLoadingOnyxValue(dismissedHoldUseExplanationResult);
Expand All @@ -80,9 +82,9 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
const isReportInRHP = route.name === SCREENS.SEARCH.REPORT_RHP;
const shouldDisplaySearchRouter = !isReportInRHP || isSmallScreenWidth;
const transactionIDList = transaction ? [transaction.transactionID] : [];
const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDList);
const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDList, allTransactionViolations);

const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDList, parentReport, policy);
const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationTransactionUtils(transactionIDList, parentReport, policy, allTransactionViolations);

const shouldShowMarkAsCashButton = hasAllPendingRTERViolations || (shouldShowBrokenConnectionViolation && (!isPolicyAdmin(policy) || isCurrentUserSubmitter(parentReport?.reportID)));

Expand Down Expand Up @@ -121,7 +123,7 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
),
};
}
if (hasPendingRTERViolation(getTransactionViolations(transaction?.transactionID))) {
if (hasPendingRTERViolation(transactionViolations)) {
return {icon: getStatusIcon(Expensicons.Hourglass), description: translate('iou.pendingMatchWithCreditCardDescription')};
}
if (isScanning) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionViolations from '@hooks/useTransactionViolations';
import useWindowDimensions from '@hooks/useWindowDimensions';
import ControlSelection from '@libs/ControlSelection';
import {convertToDisplayString} from '@libs/CurrencyUtils';
Expand All @@ -48,7 +49,6 @@ import type {TransactionDetails} from '@libs/ReportUtils';
import StringUtils from '@libs/StringUtils';
import {
compareDuplicateTransactionFields,
getTransactionViolations,
hasMissingSmartscanFields,
hasNoticeTypeViolation as hasNoticeTypeViolationTransactionUtils,
hasPendingUI,
Expand Down Expand Up @@ -113,7 +113,8 @@ function MoneyRequestPreviewContent({
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
const [walletTerms] = useOnyx(ONYXKEYS.WALLET_TERMS);
const [allViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const transactionViolations = getTransactionViolations(transaction?.transactionID);
const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same collection as the previous line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. A cleanup can be done here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main remaining thing I want to see addressed before this PR is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rojiphil I have updated this. Please review and let me know if there's something left. Thanks.

const transactionViolations = useTransactionViolations(transaction?.transactionID);

const sessionAccountID = session?.accountID;
const managerID = iouReport?.managerID ?? CONST.DEFAULT_NUMBER_ID;
Expand Down Expand Up @@ -146,7 +147,7 @@ function MoneyRequestPreviewContent({
const isOnHold = isOnHoldTransactionUtils(transaction);
const isSettlementOrApprovalPartial = !!iouReport?.pendingFields?.partial;
const isPartialHold = isSettlementOrApprovalPartial && isOnHold;
const hasViolations = hasViolationTransactionUtils(transaction?.transactionID, allViolations, true);
const hasViolations = hasViolationTransactionUtils(transaction, allViolations, true);
const hasNoticeTypeViolations = hasNoticeTypeViolationTransactionUtils(transaction?.transactionID, allViolations, true) && isPaidGroupPolicy(iouReport);
const hasWarningTypeViolations = hasWarningTypeViolationTransactionUtils(transaction?.transactionID, allViolations, true);
const hasFieldErrors = hasMissingSmartscanFields(transaction);
Expand Down Expand Up @@ -281,7 +282,7 @@ function MoneyRequestPreviewContent({
if (isPending(transaction)) {
return {shouldShow: true, messageIcon: Expensicons.CreditCardHourglass, messageDescription: translate('iou.transactionPending')};
}
if (shouldShowBrokenConnectionViolation(transaction ? [transaction.transactionID] : [], iouReport, policy)) {
if (shouldShowBrokenConnectionViolation(transaction ? [transaction.transactionID] : [], iouReport, policy, allTransactionViolations)) {
return {shouldShow: true, messageIcon: Expensicons.Hourglass, messageDescription: translate('violations.brokenConnection530Error')};
}
if (hasPendingUI(transaction, transactionViolations)) {
Expand Down
4 changes: 2 additions & 2 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionViolations from '@hooks/useTransactionViolations';
import useViolations from '@hooks/useViolations';
import type {ViolationField} from '@hooks/useViolations';
import {convertToDisplayString} from '@libs/CurrencyUtils';
Expand Down Expand Up @@ -49,7 +50,6 @@ import {
getDistanceInMeters,
getTagForDisplay,
getTaxName,
getTransactionViolations,
hasMissingSmartscanFields,
hasReceipt as hasReceiptTransactionUtils,
hasReservationList,
Expand Down Expand Up @@ -134,7 +134,7 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals

const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${linkedTransactionID ?? CONST.DEFAULT_NUMBER_ID}`);
const [transactionBackup] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${linkedTransactionID ?? CONST.DEFAULT_NUMBER_ID}`);
const transactionViolations = getTransactionViolations(linkedTransactionID);
const transactionViolations = useTransactionViolations(transaction?.transactionID);

const {
created: transactionDate,
Expand Down
19 changes: 10 additions & 9 deletions src/components/ReportActionItem/ReportPreview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import useNetwork from '@hooks/useNetwork';
import usePolicy from '@hooks/usePolicy';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionViolations from '@hooks/useTransactionViolations';
import ControlSelection from '@libs/ControlSelection';
import {convertToDisplayString} from '@libs/CurrencyUtils';
import {canUseTouchScreen} from '@libs/DeviceCapabilities';
Expand Down Expand Up @@ -71,7 +72,6 @@ import StringUtils from '@libs/StringUtils';
import {
getDescription,
getMerchant,
getTransactionViolations,
hasPendingUI,
isCardTransaction,
isPartialMerchant,
Expand Down Expand Up @@ -147,7 +147,8 @@ function ReportPreview({
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {
selector: (_transactions) => reportTransactionsSelector(_transactions, iouReportID),
});
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const lastTransaction = transactions?.at(0);
const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
const [userWallet] = useOnyx(ONYXKEYS.USER_WALLET);
const [invoiceReceiverPolicy] = useOnyx(
`${ONYXKEYS.COLLECTION.POLICY}${chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : CONST.DEFAULT_NUMBER_ID}`,
Expand Down Expand Up @@ -229,17 +230,17 @@ function ReportPreview({
const hasErrors =
(hasMissingSmartscanFields && !iouSettled) ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
hasViolations(iouReportID, transactionViolations, true) ||
hasNoticeTypeViolations(iouReportID, transactionViolations, true) ||
hasWarningTypeViolations(iouReportID, transactionViolations, true) ||
hasViolations(iouReportID, allTransactionViolations, true) ||
hasNoticeTypeViolations(iouReportID, allTransactionViolations, true) ||
hasWarningTypeViolations(iouReportID, allTransactionViolations, true) ||
(isReportOwner(iouReport) && hasReportViolations(iouReportID)) ||
hasActionsWithErrors(iouReportID);
const lastThreeTransactions = transactions?.slice(-3) ?? [];
const lastTransaction = transactions?.at(0);
const lastThreeReceipts = lastThreeTransactions.map((transaction) => ({...getThumbnailAndImageURIs(transaction), transaction}));
const transactionIDList = transactions?.map((reportTransaction) => reportTransaction.transactionID) ?? [];
const showRTERViolationMessage = numberOfRequests === 1 && hasPendingUI(lastTransaction, getTransactionViolations(lastTransaction?.transactionID));
const shouldShowBrokenConnectionViolation = numberOfRequests === 1 && shouldShowBrokenConnectionViolationTransactionUtils(transactionIDList, iouReport, policy);
const lastTransactionViolations = useTransactionViolations(lastTransaction?.transactionID);
const showRTERViolationMessage = numberOfRequests === 1 && hasPendingUI(lastTransaction, lastTransactionViolations);
const shouldShowBrokenConnectionViolation = numberOfRequests === 1 && shouldShowBrokenConnectionViolationTransactionUtils(transactionIDList, iouReport, policy, allTransactionViolations);
let formattedMerchant = numberOfRequests === 1 ? getMerchant(lastTransaction) : null;
const formattedDescription = numberOfRequests === 1 ? getDescription(lastTransaction) : null;

Expand All @@ -250,7 +251,7 @@ function ReportPreview({
const isArchived = isArchivedReportWithID(iouReport?.reportID);
const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
const filteredTransactions = transactions?.filter((transaction) => transaction) ?? [];
const shouldShowSubmitButton = canSubmitReport(iouReport, policy, filteredTransactions);
const shouldShowSubmitButton = canSubmitReport(iouReport, policy, filteredTransactions, allTransactionViolations);

const shouldDisableSubmitButton = shouldShowSubmitButton && !isAllowedToSubmitDraftExpenseReport(iouReport);

Expand Down
13 changes: 13 additions & 0 deletions src/hooks/useTransactionViolations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import {useMemo} from 'react';
import {useOnyx} from 'react-native-onyx';
import {isViolationDismissed} from '@libs/TransactionUtils';
import ONYXKEYS from '@src/ONYXKEYS';
import type {TransactionViolations} from '@src/types/onyx';

function useTransactionViolations(transactionID?: string): TransactionViolations {
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
const [transactionViolations = []] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`);
return useMemo(() => transactionViolations.filter((violation) => !isViolationDismissed(transaction, violation)), [transaction, transactionViolations]);
}

roryabraham marked this conversation as resolved.
Show resolved Hide resolved
export default useTransactionViolations;
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6920,7 +6920,7 @@ function hasViolations(
reportTransactions?: SearchTransaction[],
): boolean {
const transactions = reportTransactions ?? getReportTransactions(reportID);
return transactions.some((transaction) => hasViolation(transaction.transactionID, transactionViolations, shouldShowInReview));
return transactions.some((transaction) => hasViolation(transaction, transactionViolations, shouldShowInReview));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question. Why not transaction.transactionID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rojiphil, the util function hasViolation requires the transaction object for passing it to isExpensifyCardTransaction, isPending & isViolationDismissed so it's preferred to pass the transaction directly.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/libs/SearchUIUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ function getAction(data: OnyxTypes.SearchResults['data'], key: string): SearchTr
}

// We check for isAllowedToApproveExpenseReport because if the policy has preventSelfApprovals enabled, we disable the Submit action and in that case we want to show the View action instead
if (canSubmitReport(report, policy, allReportTransactions) && isAllowedToApproveExpenseReport) {
if (canSubmitReport(report, policy, allReportTransactions, allViolations) && isAllowedToApproveExpenseReport) {
return CONST.SEARCH.ACTION_TYPES.SUBMIT;
}

Expand Down
Loading