fix: Expense details - Violation is not displayed on description field when opening IOU details. #56405
+106
−73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d when opening IOU details. Signed-off-by: krishna2323 <[email protected]>
49 tasks
|
@Krishna2323 We need to fix typescript check errors |
Signed-off-by: krishna2323 <[email protected]>
|
@rojiphil, the jest test fail is not related to this PR. |
|
@rojiphil, checklist completed. |
|
cc: @mjasikowski |
|
@jjcoffee Please note that I am reviewing this as this is part of a regression fix |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari56405-web-chrome-001.mp456405-web-chrome-002.mp4MacOS: Desktop56405-desktop-001.mp4Android: Native56405-android-native-001.mp4Android: mWeb Chrome56405-mweb-chrome-001.mp4iOS: Native56405-ios-native-001.mp4iOS: mWeb Safari56405-mweb-safari-001.mp4 |
@Krishna2323 The unit tests failure looks completely unrelated to our PR but I also don’t find the tests failure in any of the recent PRs. Can you please double check on this? |
|
@rojiphil, merging main fixed the failing test. |
rojiphil
approved these changes
Feb 5, 2025
There was a problem hiding this comment.
Thanks @Krishna2323 for the quick fix.
Changes LGTM and works well.
@mjasikowski All yours for review. Thanks.
roryabraham
requested changes
Feb 5, 2025
There was a problem hiding this comment.
This implementation looks pretty inefficient. We don't need to subscribe to all transaction violations while we're looking at only one transaction:
diff --git a/src/components/BrokenConnectionDescription.tsx b/src/components/BrokenConnectionDescription.tsx
index a61a7a3f976..480283a6024 100644
--- a/src/components/BrokenConnectionDescription.tsx
+++ b/src/components/BrokenConnectionDescription.tsx
@@ -5,7 +5,7 @@ import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import {isInstantSubmitEnabled, isPolicyAdmin as isPolicyAdminPolicyUtils} from '@libs/PolicyUtils';
import {isCurrentUserSubmitter, isProcessingReport, isReportApproved, isReportManuallyReimbursed} from '@libs/ReportUtils';
-import {getTransactionViolations} from '@libs/TransactionUtils';
+import {isViolationDismissed} from '@libs/TransactionUtils';
import Navigation from '@navigation/Navigation';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
@@ -26,8 +26,9 @@ type BrokenConnectionDescriptionProps = {
function BrokenConnectionDescription({transactionID, policy, report}: BrokenConnectionDescriptionProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
- const [allTransactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}`);
- const transactionViolations = getTransactionViolations(transactionID, allTransactionViolations);
+ const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, {
+ selector: (violations) => (violations ?? []).filter((violation) => !isViolationDismissed(transactionID, violation)),
+ });
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);Edit: I found a lot of ways to improve this code and to make it more efficient. Here's a full diff:
Full diff
diff --git a/src/components/BrokenConnectionDescription.tsx b/src/components/BrokenConnectionDescription.tsx
index a61a7a3f976..786e9831b99 100644
--- a/src/components/BrokenConnectionDescription.tsx
+++ b/src/components/BrokenConnectionDescription.tsx
@@ -5,7 +5,7 @@ import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import {isInstantSubmitEnabled, isPolicyAdmin as isPolicyAdminPolicyUtils} from '@libs/PolicyUtils';
import {isCurrentUserSubmitter, isProcessingReport, isReportApproved, isReportManuallyReimbursed} from '@libs/ReportUtils';
-import {getTransactionViolations} from '@libs/TransactionUtils';
+import {isViolationDismissed} from '@libs/TransactionUtils';
import Navigation from '@navigation/Navigation';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
@@ -26,8 +26,15 @@ type BrokenConnectionDescriptionProps = {
function BrokenConnectionDescription({transactionID, policy, report}: BrokenConnectionDescriptionProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
- const [allTransactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}`);
- const transactionViolations = getTransactionViolations(transactionID, allTransactionViolations);
+ const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
+ const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, {
+ selector: (violations) => {
+ if (!transaction) {
+ return [];
+ }
+ return (violations ?? []).filter((violation) => !isViolationDismissed(transaction, violation));
+ },
+ });
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);
diff --git a/src/components/MoneyRequestHeader.tsx b/src/components/MoneyRequestHeader.tsx
index 684335d3d27..68f3a7ecc76 100644
--- a/src/components/MoneyRequestHeader.tsx
+++ b/src/components/MoneyRequestHeader.tsx
@@ -15,7 +15,6 @@ import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils
import {isCurrentUserSubmitter} from '@libs/ReportUtils';
import {
allHavePendingRTERViolation,
- getTransactionViolations,
hasPendingRTERViolation,
hasReceipt,
isDuplicate as isDuplicateTransactionUtils,
@@ -23,6 +22,7 @@ import {
isOnHold as isOnHoldTransactionUtils,
isPending,
isReceiptBeingScanned,
+ isViolationDismissed,
shouldShowBrokenConnectionViolation as shouldShowBrokenConnectionViolationTransactionUtils,
} from '@libs/TransactionUtils';
import variables from '@styles/variables';
@@ -67,7 +67,14 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
isMoneyRequestAction(parentReportAction) ? getOriginalMessage(parentReportAction)?.IOUTransactionID ?? CONST.DEFAULT_NUMBER_ID : CONST.DEFAULT_NUMBER_ID
}`,
);
- const [allTransactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
+ const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID ?? '-1'}`, {
+ selector: (violations) => {
+ if (!transaction) {
+ return [];
+ }
+ return (violations ?? []).filter((violation) => !isViolationDismissed(transaction, violation));
+ },
+ });
const [dismissedHoldUseExplanation, dismissedHoldUseExplanationResult] = useOnyx(ONYXKEYS.NVP_DISMISSED_HOLD_USE_EXPLANATION, {initialValue: true});
const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);
const isLoadingHoldUseExplained = isLoadingOnyxValue(dismissedHoldUseExplanationResult);
@@ -122,7 +129,7 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
),
};
}
- if (hasPendingRTERViolation(getTransactionViolations(transaction?.transactionID, allTransactionViolations))) {
+ if (hasPendingRTERViolation(transactionViolations)) {
return {icon: getStatusIcon(Expensicons.Hourglass), description: translate('iou.pendingMatchWithCreditCardDescription')};
}
if (isScanning) {
diff --git a/src/libs/TransactionUtils/index.ts b/src/libs/TransactionUtils/index.ts
index 63f91557e47..4c407696f93 100644
--- a/src/libs/TransactionUtils/index.ts
+++ b/src/libs/TransactionUtils/index.ts
@@ -770,7 +770,12 @@ function getTransactionViolations(transactionID: string | undefined, transaction
if (!transactionID || !transactionViolations) {
return null;
}
- return transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.filter((violation) => !isViolationDismissed(transactionID, violation)) ?? null;
+ const transaction = getTransaction(transactionID);
+ if (!transaction) {
+ return null;
+ }
+ const violations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
+ return violations.filter((violation) => !isViolationDismissed(transaction, violation));
}
/**
@@ -918,14 +923,18 @@ function isDuplicate(transactionID: string | undefined, checkDismissed = false):
(violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION,
);
- const hasDuplicatedViolation = !!duplicateViolation;
+ const hasDuplicateViolation = !!duplicateViolation;
if (!checkDismissed) {
return !!duplicateViolation;
}
- const didDismissedViolation = isViolationDismissed(transactionID, duplicateViolation);
+ const transaction = getTransaction(transactionID);
+ if (!transaction) {
+ return false;
+ }
- return hasDuplicatedViolation && !didDismissedViolation;
+ const didDismissViolation = hasDuplicateViolation ? isViolationDismissed(transaction, duplicateViolation) : false;
+ return hasDuplicateViolation && !didDismissViolation;
}
/**
@@ -947,17 +956,14 @@ function isOnHoldByTransactionID(transactionID: string | undefined | null): bool
return false;
}
- return isOnHold(allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]);
+ return isOnHold(getTransaction(transactionID));
}
/**
* Checks if a violation is dismissed for the given transaction
*/
-function isViolationDismissed(transactionID: string | undefined, violation: TransactionViolation | undefined): boolean {
- if (!transactionID || !violation) {
- return false;
- }
- return allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail] === `${currentUserAccountID}`;
+function isViolationDismissed(transaction: Transaction, violation: TransactionViolation): boolean {
+ return transaction.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail] === currentUserAccountID;
}
/**
@@ -968,14 +974,15 @@ function hasViolation(transactionID: string | undefined, transactionViolations:
return false;
}
const transaction = getTransaction(transactionID);
- if (isExpensifyCardTransaction(transaction) && isPending(transaction)) {
+ if (!transaction || (isExpensifyCardTransaction(transaction) && isPending(transaction))) {
return false;
}
- return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
- (violation: TransactionViolation) =>
+ const violations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
+ return violations.some(
+ (violation) =>
violation.type === CONST.VIOLATION_TYPES.VIOLATION &&
(showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
- !isViolationDismissed(transactionID, violation),
+ !isViolationDismissed(transaction, violation),
);
}
@@ -987,14 +994,15 @@ function hasNoticeTypeViolation(transactionID: string | undefined, transactionVi
return false;
}
const transaction = getTransaction(transactionID);
- if (isExpensifyCardTransaction(transaction) && isPending(transaction)) {
+ if (!transaction || (isExpensifyCardTransaction(transaction) && isPending(transaction))) {
return false;
}
- return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
- (violation: TransactionViolation) =>
+ const violations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
+ return violations.some(
+ (violation) =>
violation.type === CONST.VIOLATION_TYPES.NOTICE &&
(showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
- !isViolationDismissed(transactionID, violation),
+ !isViolationDismissed(transaction, violation),
);
}
@@ -1006,19 +1014,19 @@ function hasWarningTypeViolation(transactionID: string | undefined, transactionV
return false;
}
const transaction = getTransaction(transactionID);
- if (isExpensifyCardTransaction(transaction) && isPending(transaction)) {
+ if (!transaction || (isExpensifyCardTransaction(transaction) && isPending(transaction))) {
return false;
}
- const violations = transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID];
- const warningTypeViolations =
- violations?.filter(
- (violation: TransactionViolation) =>
- violation.type === CONST.VIOLATION_TYPES.WARNING &&
- (showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
- !isViolationDismissed(transactionID, violation),
- ) ?? [];
-
- const hasOnlyDupeDetectionViolation = warningTypeViolations?.every((violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION);
+ const violations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
+ const warningTypeViolations = violations.filter(
+ (violation: TransactionViolation) =>
+ violation.type === CONST.VIOLATION_TYPES.WARNING &&
+ (showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
+ !isViolationDismissed(transaction, violation),
+ );
+
+ const hasOnlyDupeDetectionViolation =
+ warningTypeViolations.length > 0 && warningTypeViolations.every((violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION);
if (hasOnlyDupeDetectionViolation) {
return false;
}
@@ -1146,11 +1154,10 @@ type FieldsToChange = {
};
function removeSettledAndApprovedTransactions(transactionIDs: string[]): string[] {
- return transactionIDs.filter(
- (transactionID) =>
- !isSettled(allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.reportID) &&
- !isReportIDApproved(allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.reportID),
- );
+ return transactionIDs.filter((transactionID) => {
+ const reportID = getTransaction(transactionID)?.reportID;
+ return !isSettled(reportID) && !isReportIDApproved(reportID);
+ });
}
/**
@@ -1337,7 +1344,7 @@ function getTransactionID(threadReportID?: string): string | undefined {
}
function buildNewTransactionAfterReviewingDuplicates(reviewDuplicateTransaction: OnyxEntry<ReviewDuplicates>): Partial<Transaction> {
- const originalTransaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${reviewDuplicateTransaction?.transactionID}`] ?? undefined;
+ const originalTransaction = getTransaction(reviewDuplicateTransaction?.transactionID);
const {duplicates, ...restReviewDuplicateTransaction} = reviewDuplicateTransaction ?? {};
return {
diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts
index 335cc644ec6..596c6a64fbc 100644
--- a/tests/unit/ViolationUtilsTest.ts
+++ b/tests/unit/ViolationUtilsTest.ts
@@ -402,8 +402,8 @@ describe('getViolations', () => {
await Onyx.multiSet({...transactionCollectionDataSet});
- const isSmartScanDismissed = isViolationDismissed(transaction.transactionID, smartScanFailedViolation);
- const isDuplicateViolationDismissed = isViolationDismissed(transaction.transactionID, duplicatedTransactionViolation);
+ const isSmartScanDismissed = isViolationDismissed(transaction, smartScanFailedViolation);
+ const isDuplicateViolationDismissed = isViolationDismissed(transaction, duplicatedTransactionViolation);
expect(isSmartScanDismissed).toBeTruthy();
expect(isDuplicateViolationDismissed).toBeFalsy();
Signed-off-by: krishna2323 <[email protected]>
Krishna2323
commented
Feb 5, 2025
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
roryabraham
requested changes
Feb 5, 2025
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {TransactionViolations} from '@src/types/onyx'; | ||
|
|
||
| function useTransactionViolations(transactionID?: string): TransactionViolations | undefined { |
There was a problem hiding this comment.
Rather that | undefined, why not just return an empty array if there are no violations?
diff --git a/src/hooks/useTransactionViolations.ts b/src/hooks/useTransactionViolations.ts
index 463d8af2730..a99adfbe1c4 100644
--- a/src/hooks/useTransactionViolations.ts
+++ b/src/hooks/useTransactionViolations.ts
@@ -3,8 +3,8 @@ import {getTransaction, isViolationDismissed} from '@libs/TransactionUtils';
import ONYXKEYS from '@src/ONYXKEYS';
import type {TransactionViolations} from '@src/types/onyx';
-function useTransactionViolations(transactionID?: string): TransactionViolations | undefined {
- const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, {
+function useTransactionViolations(transactionID?: string): TransactionViolations {
+ const [transactionViolations = []] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, {
selector: (violations) => {
const transaction = getTransaction(transactionID);
if (!transaction) {
There was a problem hiding this comment.
updated to use | undefined and return and empty array incase of no violations.
| function useTransactionViolations(transactionID?: string): TransactionViolations | undefined { | ||
| const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, { | ||
| selector: (violations) => { | ||
| const transaction = getTransaction(transactionID); |
There was a problem hiding this comment.
Thinking about this carefully, one concern I have with this implementation is that there's nothing that will re-run this selector when the transaction changes. You're getting an up-to-date version of the transaction in the selector only when the violations change, not when the transaction changes. You could fix this like so:
diff --git a/src/hooks/useTransactionViolations.ts b/src/hooks/useTransactionViolations.ts
index 463d8af2730..6d856261201 100644
--- a/src/hooks/useTransactionViolations.ts
+++ b/src/hooks/useTransactionViolations.ts
@@ -1,19 +1,13 @@
+import {useMemo} from 'react';
import {useOnyx} from 'react-native-onyx';
-import {getTransaction, isViolationDismissed} from '@libs/TransactionUtils';
+import {isViolationDismissed} from '@libs/TransactionUtils';
import ONYXKEYS from '@src/ONYXKEYS';
import type {TransactionViolations} from '@src/types/onyx';
-function useTransactionViolations(transactionID?: string): TransactionViolations | undefined {
- const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, {
- selector: (violations) => {
- const transaction = getTransaction(transactionID);
- if (!transaction) {
- return [];
- }
- return (violations ?? []).filter((violation) => !isViolationDismissed(transaction, violation));
- },
- });
- return transactionViolations;
+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]);
}
export default useTransactionViolations;There was a problem hiding this comment.
🧠 thanks for the catch🙌🏻, updated the hook.
Signed-off-by: krishna2323 <[email protected]>
roryabraham
requested changes
Feb 5, 2025
src/components/MoneyReportHeader.tsx
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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);There was a problem hiding this comment.
Quite a lot of optimizations have gone in here and I think we can optimize this too.
There was a problem hiding this comment.
@rojiphil I have updated this. Please review and let me know if there's something left. Thanks.
| @@ -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); | |||
There was a problem hiding this comment.
This is the same collection as the previous line?
There was a problem hiding this comment.
Yeah. A cleanup can be done here.
There was a problem hiding this comment.
This is the main remaining thing I want to see addressed before this PR is merged
There was a problem hiding this comment.
@rojiphil I have updated this. Please review and let me know if there's something left. Thanks.
Signed-off-by: krishna2323 <[email protected]>
rojiphil
reviewed
Feb 6, 2025
There was a problem hiding this comment.
@Krishna2323 Thanks for the changes. I have left some comments. Please have a look.
| const hasViolations = hasViolationTransactionUtils(transaction?.transactionID, allViolations, true); | ||
| const hasNoticeTypeViolations = hasNoticeTypeViolationTransactionUtils(transaction?.transactionID, allViolations, true) && isPaidGroupPolicy(iouReport); | ||
| const hasWarningTypeViolations = hasWarningTypeViolationTransactionUtils(transaction?.transactionID, allViolations, true); | ||
| const hasViolations = hasViolationTransactionUtils(transaction, violations, true); |
There was a problem hiding this comment.
Why didn't we pass transaction?.transactionID?
| @@ -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)); | |||
There was a problem hiding this comment.
Same question. Why not transaction.transactionID?
There was a problem hiding this comment.
@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.
| @@ -112,8 +112,11 @@ function MoneyRequestPreviewContent({ | |||
| const transactionID = isMoneyRequestAction ? getOriginalMessage(action)?.IOUTransactionID : undefined; | |||
| 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 [violations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, { | |||
There was a problem hiding this comment.
Can we not use transactionViolations instead?
There was a problem hiding this comment.
You mean the naming?
There was a problem hiding this comment.
No. I meant reuse const transactionViolations = useTransactionViolations(transaction?.transactionID); instead of violations.
There was a problem hiding this comment.
@rojiphil, we need OnyxCollection for hasViolationTransactionUtils, hasNoticeTypeViolationTransactionUtils & hasWarningTypeViolationTransactionUtils.
|
This is a large diff to CP straight to staging at this point in the regression tests. I think we should just revert the offending PR. |
rojiphil
approved these changes
Feb 6, 2025
There was a problem hiding this comment.
@mjasikowski @roryabraham @luacmartins The changes LGTM and works well. I think it is safe to merge but leave the final decision to you all. Thanks.
56405-web-safari-004.mp4
|
@rojiphil there's a bunch of conflicts now, you'll need to merge main and resolve |
@Krishna2323 I guess this is largely due to the revert. Can we include the original feature implementation here along with the changes for this here. And we can also tag the feature issue in this PR. |
|
Yes, we reverted the original PR so we need to reimplement those changes. |
|
@rojiphil I will re-implement the changes in the original PR today. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #56310
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4