From 43e030b5d42bf5f19bc7dcb93a6cc3d44890ba3c Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Tue, 23 Apr 2024 03:32:30 +0100 Subject: [PATCH 01/16] fix(money request): missing tags in violations translations --- .../ReportActionItem/MoneyRequestView.tsx | 2 +- src/hooks/useViolations.ts | 41 ++++++++++++++++--- src/libs/Violations/ViolationsUtils.ts | 2 + 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index 73fc7e9bae6e..9032c8026012 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -162,7 +162,7 @@ function MoneyRequestView({ // A flag for showing tax rate const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy); - const {getViolationsForField} = useViolations(transactionViolations ?? []); + const {getViolationsForField} = useViolations(transactionViolations ?? [], transaction, policy, policyTagList); const hasViolations = useCallback( (field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0, [canUseViolations, getViolationsForField], diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 7aadb4ce4ca3..1d25a5e6f3d2 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -1,6 +1,8 @@ import {useCallback, useMemo} from 'react'; +import type {OnyxEntry} from 'react-native-onyx'; +import ViolationsUtils from '@libs/Violations/ViolationsUtils'; import CONST from '@src/CONST'; -import type {TransactionViolation, ViolationName} from '@src/types/onyx'; +import type {Policy, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx'; /** * Names of Fields where violations can occur. @@ -48,7 +50,12 @@ const violationFields: Record = { type ViolationsMap = Map; -function useViolations(violations: TransactionViolation[]) { +function useViolations( + violations: TransactionViolation[], + transaction: OnyxEntry = {} as Transaction, + policy: OnyxEntry = {} as Policy, + policyTagList: OnyxEntry = {}, +) { const violationsByField = useMemo((): ViolationsMap => { const filteredViolations = violations.filter((violation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION); const violationGroups = new Map(); @@ -63,10 +70,11 @@ function useViolations(violations: TransactionViolation[]) { const getViolationsForField = useCallback( (field: ViolationField, data?: TransactionViolation['data']) => { const currentViolations = violationsByField.get(field) ?? []; + const firstViolation = currentViolations[0]; // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation // tagListIndex can be 0 so we compare with undefined - if (currentViolations[0]?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) { + if (firstViolation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(firstViolation?.data?.errorIndexes)) { return currentViolations .filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) .map((violation) => ({ @@ -78,14 +86,37 @@ function useViolations(violations: TransactionViolation[]) { })); } + // missingTag has special logic because if its data is null, we need to convert it to someTagLevelsRequired + if (firstViolation?.name === 'missingTag' && firstViolation?.data === null) { + const newViolations = + Object.keys(policyTagList ?? {}).length === 1 + ? ViolationsUtils.getTagViolationsForSingleLevelTags(transaction ?? ({} as Transaction), violations, !!policy?.requiresTag, policyTagList ?? {}) + : ViolationsUtils.getTagViolationsForMultiLevelTags(transaction ?? ({} as Transaction), violations, !!policy?.requiresTag, policyTagList ?? {}); + const newViolation = newViolations.find( + (currentViolation) => currentViolation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolation?.data?.errorIndexes), + ); + if (newViolation) { + return newViolations + .filter((currentViolation) => currentViolation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) + .map((currentViolation) => ({ + ...currentViolation, + data: { + ...currentViolation.data, + tagName: data?.tagListName, + }, + })); + } + return test; + } + // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on - if (currentViolations[0]?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) { + if (firstViolation?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && firstViolation?.data?.tagName) { return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } return currentViolations; }, - [violationsByField], + [policy?.requiresTag, policyTagList, transaction, violations, violationsByField], ); return { diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 42f58be1d699..3565a3d28b9a 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -110,6 +110,8 @@ function getTagViolationsForMultiLevelTags( } const ViolationsUtils = { + getTagViolationsForSingleLevelTags, + getTagViolationsForMultiLevelTags, /** * Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction * violations. From b9d8b24bfa1dd3346fe92f82d3e4542b50d98b17 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Tue, 23 Apr 2024 03:39:22 +0100 Subject: [PATCH 02/16] refactor: remove unexpected return --- src/hooks/useViolations.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 1d25a5e6f3d2..cf02545e20b0 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -106,7 +106,6 @@ function useViolations( }, })); } - return test; } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on From 0a0cac1e1baa810315b5686c0cfb6332e0c9573e Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Wed, 24 Apr 2024 22:04:08 +0100 Subject: [PATCH 03/16] chore: apply PR suggestions --- .../ReportActionItem/MoneyRequestView.tsx | 9 +++-- src/hooks/useViolations.ts | 36 ++++--------------- src/libs/PolicyUtils.ts | 8 +++++ src/libs/Violations/ViolationsUtils.ts | 17 +++++++-- src/libs/actions/IOU.ts | 14 ++++++-- src/types/onyx/PolicyTag.ts | 6 ++++ src/types/onyx/TransactionViolation.ts | 1 + 7 files changed, 54 insertions(+), 37 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index b9bf70f22a9d..2cb1d1d6ae84 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -174,7 +174,7 @@ function MoneyRequestView({ // A flag for showing tax rate const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy); - const {getViolationsForField} = useViolations(transactionViolations ?? [], transaction, policy, policyTagList); + const {getViolationsForField} = useViolations(transactionViolations ?? []); const hasViolations = useCallback( (field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0, [canUseViolations, getViolationsForField], @@ -467,11 +467,16 @@ function MoneyRequestView({ getErrorForField('tag', { tagListIndex: index, tagListName: name, + policyHasDependentTagLists: PolicyUtils.hasDependentTags(policy, policyTagList), }) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined } - error={getErrorForField('tag', {tagListIndex: index, tagListName: name})} + error={getErrorForField('tag', { + tagListIndex: index, + tagListName: name, + policyHasDependentTagLists: PolicyUtils.hasDependentTags(policy, policyTagList), + })} /> ))} diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index cf02545e20b0..eb6138799973 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -1,8 +1,6 @@ import {useCallback, useMemo} from 'react'; -import type {OnyxEntry} from 'react-native-onyx'; -import ViolationsUtils from '@libs/Violations/ViolationsUtils'; import CONST from '@src/CONST'; -import type {Policy, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx'; +import type {TransactionViolation, ViolationName} from '@src/types/onyx'; /** * Names of Fields where violations can occur. @@ -50,12 +48,7 @@ const violationFields: Record = { type ViolationsMap = Map; -function useViolations( - violations: TransactionViolation[], - transaction: OnyxEntry = {} as Transaction, - policy: OnyxEntry = {} as Policy, - policyTagList: OnyxEntry = {}, -) { +function useViolations(violations: TransactionViolation[]) { const violationsByField = useMemo((): ViolationsMap => { const filteredViolations = violations.filter((violation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION); const violationGroups = new Map(); @@ -86,26 +79,9 @@ function useViolations( })); } - // missingTag has special logic because if its data is null, we need to convert it to someTagLevelsRequired - if (firstViolation?.name === 'missingTag' && firstViolation?.data === null) { - const newViolations = - Object.keys(policyTagList ?? {}).length === 1 - ? ViolationsUtils.getTagViolationsForSingleLevelTags(transaction ?? ({} as Transaction), violations, !!policy?.requiresTag, policyTagList ?? {}) - : ViolationsUtils.getTagViolationsForMultiLevelTags(transaction ?? ({} as Transaction), violations, !!policy?.requiresTag, policyTagList ?? {}); - const newViolation = newViolations.find( - (currentViolation) => currentViolation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolation?.data?.errorIndexes), - ); - if (newViolation) { - return newViolations - .filter((currentViolation) => currentViolation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) - .map((currentViolation) => ({ - ...currentViolation, - data: { - ...currentViolation.data, - tagName: data?.tagListName, - }, - })); - } + // missingTag has special logic because we have to take into account dependent tags + if (firstViolation?.data?.policyHasDependentTagLists && firstViolation?.name === 'missingTag' && firstViolation?.data?.tagName === data?.tagListName) { + return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on @@ -115,7 +91,7 @@ function useViolations( return currentViolations; }, - [policy?.requiresTag, policyTagList, transaction, violations, violationsByField], + [violationsByField], ); return { diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index 731dc5700c8e..3b60e0cba262 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -378,6 +378,13 @@ function getPolicy(policyID: string | undefined): Policy | EmptyObject { return allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? {}; } +function hasDependentTags(policy: OnyxEntry, policyTagList: OnyxEntry) { + return ( + !!policy?.hasMultipleTagLists && + Object.values(policyTagList ?? {}).some((tagList) => Object.values(tagList.tags).some((tag) => !!tag.rules?.parentTagsFilter || !!tag.parentTagsFilter)) + ); +} + export { getActivePolicies, hasAccountingConnections, @@ -421,6 +428,7 @@ export { getSubmitToAccountID, getAdminEmployees, getPolicy, + hasDependentTags, }; export type {MemberEmailsToAccountIDs}; diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 3565a3d28b9a..ffa346754d8a 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -56,6 +56,7 @@ function getTagViolationsForMultiLevelTags( transactionViolations: TransactionViolation[], policyRequiresTags: boolean, policyTagList: PolicyTagList, + hasDependentTags: boolean, ): TransactionViolation[] { const policyTagKeys = getSortedTagKeys(policyTagList); const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? []; @@ -64,6 +65,17 @@ function getTagViolationsForMultiLevelTags( (violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY, ); + if (hasDependentTags && !updatedTransaction.tag) { + Object.values(policyTagList).forEach((tagList) => { + newTransactionViolations.push({ + name: CONST.VIOLATIONS.MISSING_TAG, + type: CONST.VIOLATION_TYPES.VIOLATION, + data: {tagName: tagList.name}, + }); + }); + return newTransactionViolations; + } + // We first get the errorIndexes for someTagLevelsRequired. If it's not empty, we puth SOME_TAG_LEVELS_REQUIRED in Onyx. // Otherwise, we put TAG_OUT_OF_POLICY in Onyx (when applicable) const errorIndexes = []; @@ -110,8 +122,6 @@ function getTagViolationsForMultiLevelTags( } const ViolationsUtils = { - getTagViolationsForSingleLevelTags, - getTagViolationsForMultiLevelTags, /** * Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction * violations. @@ -123,6 +133,7 @@ const ViolationsUtils = { policyTagList: PolicyTagList, policyRequiresCategories: boolean, policyCategories: PolicyCategories, + hasDependentTags: boolean, ): OnyxUpdate { const isPartialTransaction = TransactionUtils.isPartialMerchant(TransactionUtils.getMerchant(updatedTransaction)) && TransactionUtils.isAmountMissing(updatedTransaction); if (isPartialTransaction) { @@ -168,7 +179,7 @@ const ViolationsUtils = { newTransactionViolations = Object.keys(policyTagList).length === 1 ? getTagViolationsForSingleLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList) - : getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList); + : getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList, hasDependentTags); } return { diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index 30ab2688b593..64eb7ba3a645 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -765,7 +765,7 @@ function buildOnyxDataForMoneyRequest( return [optimisticData, successData, failureData]; } - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}, true); if (violationsOnyxData) { optimisticData.push(violationsOnyxData); @@ -1102,7 +1102,15 @@ function buildOnyxDataForTrackExpense( return [optimisticData, successData, failureData]; } - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData( + transaction, + [], + !!policy.requiresTag, + policyTagList ?? {}, + !!policy.requiresCategory, + policyCategories ?? {}, + PolicyUtils.hasDependentTags(policy, policyTagList ?? {}), + ); if (violationsOnyxData) { optimisticData.push(violationsOnyxData); @@ -2054,6 +2062,7 @@ function getUpdateMoneyRequestParams( policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}, + PolicyUtils.hasDependentTags(policy, policyTagList ?? {}), ), ); failureData.push({ @@ -4375,6 +4384,7 @@ function editRegularMoneyRequest( policyTags, !!policy.requiresCategory, policyCategories, + PolicyUtils.hasDependentTags(policy, policyTags), ); optimisticData.push(updatedViolationsOnyxData); failureData.push({ diff --git a/src/types/onyx/PolicyTag.ts b/src/types/onyx/PolicyTag.ts index 37e979fb58f6..88b5297dbd38 100644 --- a/src/types/onyx/PolicyTag.ts +++ b/src/types/onyx/PolicyTag.ts @@ -13,6 +13,12 @@ type PolicyTag = OnyxCommon.OnyxValueWithOfflineFeedback<{ /** A list of errors keyed by microtime */ errors?: OnyxCommon.Errors | null; + + rules?: { + parentTagsFilter?: string; + }; + + parentTagsFilter?: string; }>; type PolicyTags = Record; diff --git a/src/types/onyx/TransactionViolation.ts b/src/types/onyx/TransactionViolation.ts index 28de4582bd5e..6859c6749567 100644 --- a/src/types/onyx/TransactionViolation.ts +++ b/src/types/onyx/TransactionViolation.ts @@ -10,6 +10,7 @@ type TransactionViolation = { type: string; name: ViolationName; data?: { + policyHasDependentTagLists?: boolean; rejectedBy?: string; rejectReason?: string; formattedLimit?: string; From a174c60a79bd5e8cd92eb6afd8913e76f60dd4ee Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Thu, 25 Apr 2024 22:18:10 +0100 Subject: [PATCH 04/16] fix: tag name missing in error UI --- src/hooks/useViolations.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index eb6138799973..65e17785500d 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -80,8 +80,16 @@ function useViolations(violations: TransactionViolation[]) { } // missingTag has special logic because we have to take into account dependent tags - if (firstViolation?.data?.policyHasDependentTagLists && firstViolation?.name === 'missingTag' && firstViolation?.data?.tagName === data?.tagListName) { - return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); + if (data?.policyHasDependentTagLists && firstViolation?.name === 'missingTag' && data?.tagListName) { + return [ + { + ...firstViolation, + data: { + ...firstViolation.data, + tagName: data?.tagListName, + }, + }, + ]; } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on From 72f85755fab249ec89a8e8b674094baec90d261e Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Mon, 29 Apr 2024 21:41:03 +0100 Subject: [PATCH 05/16] refactor: apply pull request suggestions --- .../ReportActionItem/MoneyRequestView.tsx | 4 +- src/hooks/useViolations.ts | 25 +++---- src/libs/PolicyUtils.ts | 8 +-- src/libs/Violations/ViolationsUtils.ts | 71 ++++++++++++------- src/libs/actions/IOU.ts | 10 ++- src/types/onyx/PolicyTag.ts | 4 ++ src/types/onyx/TransactionViolation.ts | 2 +- 7 files changed, 80 insertions(+), 44 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index 2cb1d1d6ae84..fa3707605044 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -467,7 +467,7 @@ function MoneyRequestView({ getErrorForField('tag', { tagListIndex: index, tagListName: name, - policyHasDependentTagLists: PolicyUtils.hasDependentTags(policy, policyTagList), + policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), }) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined @@ -475,7 +475,7 @@ function MoneyRequestView({ error={getErrorForField('tag', { tagListIndex: index, tagListName: name, - policyHasDependentTagLists: PolicyUtils.hasDependentTags(policy, policyTagList), + policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), })} /> diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 65e17785500d..578db947ed57 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -63,29 +63,30 @@ function useViolations(violations: TransactionViolation[]) { const getViolationsForField = useCallback( (field: ViolationField, data?: TransactionViolation['data']) => { const currentViolations = violationsByField.get(field) ?? []; - const firstViolation = currentViolations[0]; + const violation = currentViolations[0]; // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation // tagListIndex can be 0 so we compare with undefined - if (firstViolation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(firstViolation?.data?.errorIndexes)) { + if (violation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(violation?.data?.errorIndexes)) { return currentViolations - .filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) - .map((violation) => ({ - ...violation, + .filter((currentViolation) => currentViolation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) + .map((currentViolation) => ({ + ...currentViolation, data: { - ...violation.data, + ...currentViolation.data, tagName: data?.tagListName, }, })); } - // missingTag has special logic because we have to take into account dependent tags - if (data?.policyHasDependentTagLists && firstViolation?.name === 'missingTag' && data?.tagListName) { + // missingTag has special logic for policies with dependent tags, because only violation is returned for all tags + // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here + if (data?.policyHasDependentTags && violation?.name === 'missingTag' && data?.tagListName) { return [ { - ...firstViolation, + ...violation, data: { - ...firstViolation.data, + ...violation.data, tagName: data?.tagListName, }, }, @@ -93,8 +94,8 @@ function useViolations(violations: TransactionViolation[]) { } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on - if (firstViolation?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && firstViolation?.data?.tagName) { - return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); + if (violation?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && violation?.data?.tagName) { + return currentViolations.filter((currentViolation) => currentViolation.data?.tagName === data?.tagListName); } return currentViolations; diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index 657a3387ce7f..374ffa2436a0 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -391,10 +391,10 @@ function canSendInvoice(policies: OnyxCollection): boolean { } function hasDependentTags(policy: OnyxEntry, policyTagList: OnyxEntry) { - return ( - !!policy?.hasMultipleTagLists && - Object.values(policyTagList ?? {}).some((tagList) => Object.values(tagList.tags).some((tag) => !!tag.rules?.parentTagsFilter || !!tag.parentTagsFilter)) - ); + if (!policy?.hasMultipleTagLists) { + return false; + } + return Object.values(policyTagList ?? {}).some((tagList) => Object.values(tagList.tags).some((tag) => !!tag.rules?.parentTagsFilter || !!tag.parentTagsFilter)); } export { diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index ffa346754d8a..0ef102318d78 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -49,36 +49,31 @@ function getTagViolationsForSingleLevelTags( } /** - * Calculates some tag levels required and missing tag violations for the given transaction + * Calculates missing tag violations for policies with dependent tags, + * by returning one per tag with its corresponding tagName in the data */ -function getTagViolationsForMultiLevelTags( - updatedTransaction: Transaction, - transactionViolations: TransactionViolation[], - policyRequiresTags: boolean, - policyTagList: PolicyTagList, - hasDependentTags: boolean, -): TransactionViolation[] { - const policyTagKeys = getSortedTagKeys(policyTagList); - const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? []; - let newTransactionViolations = [...transactionViolations]; - newTransactionViolations = newTransactionViolations.filter( - (violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY, - ); +function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[]) { + return [ + ...transactionViolations, + ...Object.values(policyTagList).map((tagList) => ({ + name: CONST.VIOLATIONS.MISSING_TAG, + type: CONST.VIOLATION_TYPES.VIOLATION, + data: {tagName: tagList.name}, + })), + ]; +} - if (hasDependentTags && !updatedTransaction.tag) { - Object.values(policyTagList).forEach((tagList) => { - newTransactionViolations.push({ - name: CONST.VIOLATIONS.MISSING_TAG, - type: CONST.VIOLATION_TYPES.VIOLATION, - data: {tagName: tagList.name}, - }); - }); - return newTransactionViolations; - } +/** + * Calculates missing tag violations for policies with independent tags, + * by returning one per tag with its corresponding tagName in the data + */ +function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], policyTagKeys: string[], selectedTags: string[]) { + let newTransactionViolations = [...transactionViolations]; // We first get the errorIndexes for someTagLevelsRequired. If it's not empty, we puth SOME_TAG_LEVELS_REQUIRED in Onyx. // Otherwise, we put TAG_OUT_OF_POLICY in Onyx (when applicable) const errorIndexes = []; + for (let i = 0; i < policyTagKeys.length; i++) { const isTagRequired = policyTagList[policyTagKeys[i]].required ?? true; const isTagSelected = Boolean(selectedTags[i]); @@ -86,6 +81,7 @@ function getTagViolationsForMultiLevelTags( errorIndexes.push(i); } } + if (errorIndexes.length !== 0) { newTransactionViolations.push({ name: CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, @@ -96,10 +92,12 @@ function getTagViolationsForMultiLevelTags( }); } else { let hasInvalidTag = false; + for (let i = 0; i < policyTagKeys.length; i++) { const selectedTag = selectedTags[i]; const tags = policyTagList[policyTagKeys[i]].tags; const isTagInPolicy = Object.values(tags).some((tag) => tag.name === selectedTag && Boolean(tag.enabled)); + if (!isTagInPolicy) { newTransactionViolations.push({ name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY, @@ -112,15 +110,40 @@ function getTagViolationsForMultiLevelTags( break; } } + if (!hasInvalidTag) { newTransactionViolations = reject(newTransactionViolations, { name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY, }); } } + return newTransactionViolations; } +/** + * Calculates some tag levels required and missing tag violations for the given transaction + */ +function getTagViolationsForMultiLevelTags( + updatedTransaction: Transaction, + transactionViolations: TransactionViolation[], + policyRequiresTags: boolean, + policyTagList: PolicyTagList, + hasDependentTags: boolean, +): TransactionViolation[] { + const policyTagKeys = getSortedTagKeys(policyTagList); + const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? []; + const filteredTransactionViolations = transactionViolations.filter( + (violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY, + ); + + if (hasDependentTags && !updatedTransaction.tag) { + return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations); + } + + return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, policyTagKeys, selectedTags); +} + const ViolationsUtils = { /** * Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index 1df688ce3bed..f605663793d2 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -779,7 +779,15 @@ function buildOnyxDataForMoneyRequest( return [optimisticData, successData, failureData]; } - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}, true); + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData( + transaction, + [], + !!policy.requiresTag, + policyTagList ?? {}, + !!policy.requiresCategory, + policyCategories ?? {}, + PolicyUtils.hasDependentTags(policy, policyTagList ?? {}), + ); if (violationsOnyxData) { optimisticData.push(violationsOnyxData); diff --git a/src/types/onyx/PolicyTag.ts b/src/types/onyx/PolicyTag.ts index 88b5297dbd38..9d48b14ff444 100644 --- a/src/types/onyx/PolicyTag.ts +++ b/src/types/onyx/PolicyTag.ts @@ -15,6 +15,10 @@ type PolicyTag = OnyxCommon.OnyxValueWithOfflineFeedback<{ errors?: OnyxCommon.Errors | null; rules?: { + /** + * String representation of regex to match against parent tag. Eg, if San Francisco is a child tag of California + * its parentTagsFilter will be ^California$ + */ parentTagsFilter?: string; }; diff --git a/src/types/onyx/TransactionViolation.ts b/src/types/onyx/TransactionViolation.ts index 6859c6749567..39d0dc394753 100644 --- a/src/types/onyx/TransactionViolation.ts +++ b/src/types/onyx/TransactionViolation.ts @@ -10,7 +10,7 @@ type TransactionViolation = { type: string; name: ViolationName; data?: { - policyHasDependentTagLists?: boolean; + policyHasDependentTags?: boolean; rejectedBy?: string; rejectReason?: string; formattedLimit?: string; From 5c7e16ce6ad6aea0268431926dc965e2408c12a9 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Mon, 29 Apr 2024 22:24:24 +0100 Subject: [PATCH 06/16] chore(typescript): add missing argument --- tests/unit/ViolationUtilsTest.ts | 58 ++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index b967617918c1..02668da462e0 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -43,7 +43,7 @@ describe('getViolationsOnyxData', () => { }); it('should return an object with correct shape and with empty transactionViolations array', () => { - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result).toEqual({ onyxMethod: Onyx.METHOD.SET, @@ -57,7 +57,7 @@ describe('getViolationsOnyxData', () => { {name: 'duplicatedTransaction', type: CONST.VIOLATION_TYPES.VIOLATION}, {name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION}, ]; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining(transactionViolations)); }); @@ -70,24 +70,32 @@ describe('getViolationsOnyxData', () => { it('should add missingCategory violation if no category is included', () => { transaction.category = undefined; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); }); it('should add categoryOutOfPolicy violation when category is not in policy', () => { transaction.category = 'Bananas'; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); }); it('should not include a categoryOutOfPolicy violation when category is in policy', () => { - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).not.toContainEqual(categoryOutOfPolicyViolation); }); it('should not add a category violation when the transaction is partial', () => { const partialTransaction = {...transaction, amount: 0, merchant: CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT, category: undefined}; - const result = ViolationsUtils.getViolationsOnyxData(partialTransaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData( + partialTransaction, + transactionViolations, + policyRequiresTags, + policyTags, + policyRequiresCategories, + policyCategories, + false, + ); expect(result.value).not.toContainEqual(missingCategoryViolation); }); @@ -98,7 +106,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION}, ]; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); }); @@ -110,7 +118,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION}, ]; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); }); @@ -122,7 +130,7 @@ describe('getViolationsOnyxData', () => { }); it('should not add any violations when categories are not required', () => { - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).not.toContainEqual([categoryOutOfPolicyViolation]); expect(result.value).not.toContainEqual([missingCategoryViolation]); @@ -147,7 +155,7 @@ describe('getViolationsOnyxData', () => { }); it("shouldn't update the transactionViolations if the policy requires tags and the transaction has a tag from the policy", () => { - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(transactionViolations); }); @@ -155,7 +163,7 @@ describe('getViolationsOnyxData', () => { it('should add a missingTag violation if none is provided and policy requires tags', () => { transaction.tag = undefined; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}])); }); @@ -163,14 +171,22 @@ describe('getViolationsOnyxData', () => { it('should add a tagOutOfPolicy violation when policy requires tags and tag is not in the policy', () => { policyTags = {}; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual([]); }); it('should not add a tag violation when the transaction is partial', () => { const partialTransaction = {...transaction, amount: 0, merchant: CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT, tag: undefined}; - const result = ViolationsUtils.getViolationsOnyxData(partialTransaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData( + partialTransaction, + transactionViolations, + policyRequiresTags, + policyTags, + policyRequiresCategories, + policyCategories, + false, + ); expect(result.value).not.toContainEqual(missingTagViolation); }); @@ -181,7 +197,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION}, ]; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations])); }); @@ -193,7 +209,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION}, ]; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations])); }); @@ -205,7 +221,7 @@ describe('getViolationsOnyxData', () => { }); it('should not add any violations when tags are not required', () => { - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).not.toContainEqual([tagOutOfPolicyViolation]); expect(result.value).not.toContainEqual([missingTagViolation]); @@ -260,30 +276,30 @@ describe('getViolationsOnyxData', () => { }; // Test case where transaction has no tags - let result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + let result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 1 tag transaction.tag = 'Africa'; someTagLevelsRequiredViolation.data = {errorIndexes: [1, 2]}; - result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 2 tags transaction.tag = 'Africa::Project1'; someTagLevelsRequiredViolation.data = {errorIndexes: [1]}; - result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has all tags transaction.tag = 'Africa:Accounting:Project1'; - result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); expect(result.value).toEqual([]); }); it('should return tagOutOfPolicy when a tag is not enabled in the policy but is set in the transaction', () => { policyTags.Department.tags.Accounting.enabled = false; transaction.tag = 'Africa:Accounting:Project1'; - const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, false); const violation = {...tagOutOfPolicyViolation, data: {tagName: 'Department'}}; expect(result.value).toEqual([violation]); }); From 3d6478f10c327d7b57e628b98295a3185124b232 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Mon, 29 Apr 2024 22:25:26 +0100 Subject: [PATCH 07/16] chore(tests): add violation case for missing dependent tags --- tests/unit/ViolationUtilsTest.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index 02668da462e0..5e8f93a56dbc 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -303,5 +303,13 @@ describe('getViolationsOnyxData', () => { const violation = {...tagOutOfPolicyViolation, data: {tagName: 'Department'}}; expect(result.value).toEqual([violation]); }); + it('should return missingTag when all dependent tags are enabled in the policy but are not set in the transaction', () => { + const missingDepartmentTag = {...missingTagViolation, data: {tagName: 'Department'}}; + const missingRegionTag = {...missingTagViolation, data: {tagName: 'Region'}}; + const missingProjectTag = {...missingTagViolation, data: {tagName: 'Project'}}; + transaction.tag = undefined; + const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories, true); + expect(result.value).toEqual(expect.arrayContaining([missingDepartmentTag, missingRegionTag, missingProjectTag])); + }); }); }); From e29768615c1e43b9708e4048aa0feb64041e6c7f Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 3 May 2024 09:29:44 +0100 Subject: [PATCH 08/16] refactor: apply pull request suggestions --- src/hooks/useViolations.ts | 23 +++++++++++------------ src/libs/Violations/ViolationsUtils.ts | 11 ++++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 578db947ed57..6257a9251d08 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -63,30 +63,29 @@ function useViolations(violations: TransactionViolation[]) { const getViolationsForField = useCallback( (field: ViolationField, data?: TransactionViolation['data']) => { const currentViolations = violationsByField.get(field) ?? []; - const violation = currentViolations[0]; // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation // tagListIndex can be 0 so we compare with undefined - if (violation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(violation?.data?.errorIndexes)) { + if (currentViolations[0]?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) { return currentViolations - .filter((currentViolation) => currentViolation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) - .map((currentViolation) => ({ - ...currentViolation, + .filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) + .map((violation) => ({ + ...violation, data: { - ...currentViolation.data, + ...violation.data, tagName: data?.tagListName, }, })); } - // missingTag has special logic for policies with dependent tags, because only violation is returned for all tags + // missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here - if (data?.policyHasDependentTags && violation?.name === 'missingTag' && data?.tagListName) { + if (data?.policyHasDependentTags && currentViolations[0]?.name === 'missingTag' && data?.tagListName) { return [ { - ...violation, + ...currentViolations[0], data: { - ...violation.data, + ...currentViolations[0].data, tagName: data?.tagListName, }, }, @@ -94,8 +93,8 @@ function useViolations(violations: TransactionViolation[]) { } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on - if (violation?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && violation?.data?.tagName) { - return currentViolations.filter((currentViolation) => currentViolation.data?.tagName === data?.tagListName); + if (currentViolations[0]?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) { + return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } return currentViolations; diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 0ef102318d78..c7bcd66aef5e 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -67,7 +67,9 @@ function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transact * Calculates missing tag violations for policies with independent tags, * by returning one per tag with its corresponding tagName in the data */ -function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], policyTagKeys: string[], selectedTags: string[]) { +function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], transaction: Transaction) { + const policyTagKeys = getSortedTagKeys(policyTagList); + const selectedTags = transaction.tag?.split(CONST.COLON) ?? []; let newTransactionViolations = [...transactionViolations]; // We first get the errorIndexes for someTagLevelsRequired. If it's not empty, we puth SOME_TAG_LEVELS_REQUIRED in Onyx. @@ -131,17 +133,16 @@ function getTagViolationsForMultiLevelTags( policyTagList: PolicyTagList, hasDependentTags: boolean, ): TransactionViolation[] { - const policyTagKeys = getSortedTagKeys(policyTagList); - const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? []; const filteredTransactionViolations = transactionViolations.filter( - (violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY, + (violation) => + violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY && violation.name !== CONST.VIOLATIONS.MISSING_TAG, ); if (hasDependentTags && !updatedTransaction.tag) { return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations); } - return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, policyTagKeys, selectedTags); + return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, updatedTransaction); } const ViolationsUtils = { From a8ef1c390be0a38c687303101ec3169ddda7de15 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 3 May 2024 10:19:07 +0100 Subject: [PATCH 09/16] chore(typescript): add missing argument --- src/libs/actions/IOU.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index 7b962e712cf2..e552c5eac7d0 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -1087,7 +1087,15 @@ function buildOnyxDataForInvoice( return [optimisticData, successData, failureData]; } - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData( + transaction, + [], + !!policy.requiresTag, + policyTagList ?? {}, + !!policy.requiresCategory, + policyCategories ?? {}, + PolicyUtils.hasDependentTags(policy, policyTagList ?? {}), + ); if (violationsOnyxData) { optimisticData.push(violationsOnyxData); From 5e206959fb7c2fe723c892ff7e30f840ed7f1d58 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Wed, 29 May 2024 09:38:05 +0100 Subject: [PATCH 10/16] fix: allTagLevels required violation showing on tags that are already filled --- src/components/ReportActionItem/MoneyRequestView.tsx | 2 ++ src/hooks/useViolations.ts | 11 ++++++++--- src/types/onyx/TransactionViolation.ts | 1 + 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index d2b92b9a0c41..73844630e2cc 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -479,6 +479,7 @@ function MoneyRequestView({ getErrorForField('tag', { tagListIndex: index, tagListName: name, + tagListValue: TransactionUtils.getTagForDisplay(transaction, index), policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), }) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR @@ -487,6 +488,7 @@ function MoneyRequestView({ errorText={getErrorForField('tag', { tagListIndex: index, tagListName: name, + tagListValue: TransactionUtils.getTagForDisplay(transaction, index), policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), })} /> diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 582d997f436a..7dad3be026fe 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -67,7 +67,7 @@ function useViolations(violations: TransactionViolation[]) { // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation // tagListIndex can be 0 so we compare with undefined - if (currentViolations[0]?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) { + if (currentViolations[0]?.name === CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) { return currentViolations .filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1)) .map((violation) => ({ @@ -81,7 +81,7 @@ function useViolations(violations: TransactionViolation[]) { // missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here - if (data?.policyHasDependentTags && currentViolations[0]?.name === 'missingTag' && data?.tagListName) { + if (data?.policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { return [ { ...currentViolations[0], @@ -94,7 +94,12 @@ function useViolations(violations: TransactionViolation[]) { } // tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on - if (currentViolations[0]?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) { + if (currentViolations[0]?.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) { + return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); + } + + // allTagLevelsRequired has special logic because we have to account for tags that are already filled + if (currentViolations[0]?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && data?.tagListValue) { return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } diff --git a/src/types/onyx/TransactionViolation.ts b/src/types/onyx/TransactionViolation.ts index 3dfa5fc8ba42..99ce3d86dfe1 100644 --- a/src/types/onyx/TransactionViolation.ts +++ b/src/types/onyx/TransactionViolation.ts @@ -29,6 +29,7 @@ type TransactionViolation = { taxName?: string; tagListIndex?: number; tagListName?: string; + tagListValue?: string; errorIndexes?: number[]; pendingPattern?: boolean; }; From d67febbd775ae455c99dff200bf095a906da8ddd Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Mon, 3 Jun 2024 23:34:05 +0100 Subject: [PATCH 11/16] feat: calculate all tag required violations locally --- src/libs/Violations/ViolationsUtils.ts | 42 ++++++++++++++++++-------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 742b03c7554e..29516d3e560e 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -7,7 +7,7 @@ import * as TransactionUtils from '@libs/TransactionUtils'; import CONST from '@src/CONST'; import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; -import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation} from '@src/types/onyx'; +import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx'; /** * Calculates tag out of policy and missing tag violations for the given transaction @@ -52,15 +52,29 @@ function getTagViolationsForSingleLevelTags( * Calculates missing tag violations for policies with dependent tags, * by returning one per tag with its corresponding tagName in the data */ -function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[]) { - return [ - ...transactionViolations, - ...Object.values(policyTagList).map((tagList) => ({ - name: CONST.VIOLATIONS.MISSING_TAG, - type: CONST.VIOLATION_TYPES.VIOLATION, - data: {tagName: tagList.name}, - })), - ]; +function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], tagName: string | undefined) { + const tagViolations = [...transactionViolations]; + + if (!tagName) { + Object.values(policyTagList).forEach((tagList) => + tagViolations.push({ + name: CONST.VIOLATIONS.MISSING_TAG, + type: CONST.VIOLATION_TYPES.VIOLATION, + data: {tagName: tagList.name}, + }), + ); + } else { + const tags = TransactionUtils.getTagArrayFromName(tagName); + if (Object.keys(policyTagList).length !== tags.length || tags.includes('')) { + tagViolations.push({ + name: CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED, + type: CONST.VIOLATION_TYPES.VIOLATION, + data: undefined, + }); + } + } + + return tagViolations; } /** @@ -135,11 +149,13 @@ function getTagViolationsForMultiLevelTags( ): TransactionViolation[] { const filteredTransactionViolations = transactionViolations.filter( (violation) => - violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY && violation.name !== CONST.VIOLATIONS.MISSING_TAG, + !( + [CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, CONST.VIOLATIONS.TAG_OUT_OF_POLICY, CONST.VIOLATIONS.MISSING_TAG, CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED] as ViolationName[] + ).includes(violation.name), ); - if (hasDependentTags && !updatedTransaction.tag) { - return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations); + if (hasDependentTags) { + return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations, updatedTransaction.tag); } return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, updatedTransaction); From 30189caf9c1239a176a45aa944d4550b23a5ba83 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Wed, 5 Jun 2024 11:07:15 +0100 Subject: [PATCH 12/16] refactor: apply pull request suggestions --- .../ReportActionItem/MoneyRequestView.tsx | 41 +++++++++++-------- src/hooks/useViolations.ts | 7 ++-- src/libs/Violations/ViolationsUtils.ts | 20 +++++---- src/types/onyx/TransactionViolation.ts | 1 - 4 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index 434395e4095c..fbba403e8e0b 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -174,7 +174,8 @@ function MoneyRequestView({ const {getViolationsForField} = useViolations(transactionViolations ?? []); const hasViolations = useCallback( - (field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0, + (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags?: boolean): boolean => + !!canUseViolations && getViolationsForField(field, data, policyHasDependentTags).length > 0, [canUseViolations, getViolationsForField], ); @@ -241,7 +242,7 @@ function MoneyRequestView({ const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => transaction?.pendingFields?.[fieldPath] ?? pendingAction; const getErrorForField = useCallback( - (field: ViolationField, data?: OnyxTypes.TransactionViolation['data']) => { + (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags?: boolean) => { // Checks applied when creating a new expense // NOTE: receipt field can return multiple violations, so we need to handle it separately const fieldChecks: Partial> = { @@ -267,14 +268,14 @@ function MoneyRequestView({ } // Return violations if there are any - if (canUseViolations && hasViolations(field, data)) { - const violations = getViolationsForField(field, data); + if (hasViolations(field, data, policyHasDependentTags)) { + const violations = getViolationsForField(field, data, policyHasDependentTags); return ViolationsUtils.getViolationTranslation(violations[0], translate); } return ''; }, - [transactionAmount, isSettled, isCancelled, isPolicyExpenseChat, isEmptyMerchant, transactionDate, hasErrors, canUseViolations, hasViolations, translate, getViolationsForField], + [transactionAmount, isSettled, isCancelled, isPolicyExpenseChat, isEmptyMerchant, transactionDate, hasErrors, hasViolations, translate, getViolationsForField], ); const distanceRequestFields = canUseP2PDistanceRequests ? ( @@ -497,21 +498,27 @@ function MoneyRequestView({ ) } brickRoadIndicator={ - getErrorForField('tag', { - tagListIndex: index, - tagListName: name, - tagListValue: TransactionUtils.getTagForDisplay(transaction, index), - policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), - }) + getErrorForField( + 'tag', + { + tagListIndex: index, + tagListName: name, + tagListValue: TransactionUtils.getTagForDisplay(transaction, index), + }, + PolicyUtils.hasDependentTags(policy, policyTagList), + ) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined } - errorText={getErrorForField('tag', { - tagListIndex: index, - tagListName: name, - tagListValue: TransactionUtils.getTagForDisplay(transaction, index), - policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList), - })} + errorText={getErrorForField( + 'tag', + { + tagListIndex: index, + tagListName: name, + tagListValue: TransactionUtils.getTagForDisplay(transaction, index), + }, + PolicyUtils.hasDependentTags(policy, policyTagList), + )} /> ))} diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 7dad3be026fe..9120c0709d9a 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -62,7 +62,7 @@ function useViolations(violations: TransactionViolation[]) { }, [violations]); const getViolationsForField = useCallback( - (field: ViolationField, data?: TransactionViolation['data']) => { + (field: ViolationField, data?: TransactionViolation['data'], policyHasDependentTags?: boolean) => { const currentViolations = violationsByField.get(field) ?? []; // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation @@ -81,7 +81,7 @@ function useViolations(violations: TransactionViolation[]) { // missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here - if (data?.policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { + if (!!policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { return [ { ...currentViolations[0], @@ -98,7 +98,8 @@ function useViolations(violations: TransactionViolation[]) { return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } - // allTagLevelsRequired has special logic because we have to account for tags that are already filled + // allTagLevelsRequired has special logic because it is returned when one but not all the tags are set, + // so we need to return the violation for the tag fields without a tag set if (currentViolations[0]?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && data?.tagListValue) { return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 29516d3e560e..d433d123c903 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -9,6 +9,13 @@ import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx'; +const TRANSACTION_VIOLATIONS_FILTER = [ + CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, + CONST.VIOLATIONS.TAG_OUT_OF_POLICY, + CONST.VIOLATIONS.MISSING_TAG, + CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED, +] as ViolationName[]; + /** * Calculates tag out of policy and missing tag violations for the given transaction */ @@ -52,7 +59,7 @@ function getTagViolationsForSingleLevelTags( * Calculates missing tag violations for policies with dependent tags, * by returning one per tag with its corresponding tagName in the data */ -function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], tagName: string | undefined) { +function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], tagName: string) { const tagViolations = [...transactionViolations]; if (!tagName) { @@ -138,7 +145,7 @@ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transac } /** - * Calculates some tag levels required and missing tag violations for the given transaction + * Calculates tag violations for a transaction on a policy with multi level tags */ function getTagViolationsForMultiLevelTags( updatedTransaction: Transaction, @@ -147,15 +154,10 @@ function getTagViolationsForMultiLevelTags( policyTagList: PolicyTagList, hasDependentTags: boolean, ): TransactionViolation[] { - const filteredTransactionViolations = transactionViolations.filter( - (violation) => - !( - [CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, CONST.VIOLATIONS.TAG_OUT_OF_POLICY, CONST.VIOLATIONS.MISSING_TAG, CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED] as ViolationName[] - ).includes(violation.name), - ); + const filteredTransactionViolations = transactionViolations.filter((violation) => !TRANSACTION_VIOLATIONS_FILTER.includes(violation.name)); if (hasDependentTags) { - return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations, updatedTransaction.tag); + return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations, updatedTransaction.tag ?? ''); } return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, updatedTransaction); diff --git a/src/types/onyx/TransactionViolation.ts b/src/types/onyx/TransactionViolation.ts index 99ce3d86dfe1..e81f7196209b 100644 --- a/src/types/onyx/TransactionViolation.ts +++ b/src/types/onyx/TransactionViolation.ts @@ -10,7 +10,6 @@ type TransactionViolation = { type: string; name: ViolationName; data?: { - policyHasDependentTags?: boolean; rejectedBy?: string; rejectReason?: string; formattedLimit?: string; From 492a6726ef093c645e6800a8afe5f8a7e490e930 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 7 Jun 2024 16:39:03 +0100 Subject: [PATCH 13/16] refactor: apply suggestions --- .../ReportActionItem/MoneyRequestView.tsx | 14 ++++----- src/hooks/useViolations.ts | 6 ++-- src/libs/Violations/ViolationsUtils.ts | 31 ++++++++----------- src/types/onyx/TransactionViolation.ts | 1 - 4 files changed, 23 insertions(+), 29 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index f3b0b3e863ed..d1fc875832ec 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -171,8 +171,8 @@ function MoneyRequestView({ const {getViolationsForField} = useViolations(transactionViolations ?? []); const hasViolations = useCallback( - (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags?: boolean): boolean => - !!canUseViolations && getViolationsForField(field, data, policyHasDependentTags).length > 0, + (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string): boolean => + !!canUseViolations && getViolationsForField(field, data, policyHasDependentTags, tagValue).length > 0, [canUseViolations, getViolationsForField], ); @@ -239,7 +239,7 @@ function MoneyRequestView({ const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => transaction?.pendingFields?.[fieldPath] ?? pendingAction; const getErrorForField = useCallback( - (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags?: boolean) => { + (field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string) => { // Checks applied when creating a new expense // NOTE: receipt field can return multiple violations, so we need to handle it separately const fieldChecks: Partial> = { @@ -265,8 +265,8 @@ function MoneyRequestView({ } // Return violations if there are any - if (hasViolations(field, data, policyHasDependentTags)) { - const violations = getViolationsForField(field, data, policyHasDependentTags); + if (hasViolations(field, data, policyHasDependentTags, tagValue)) { + const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue); return ViolationsUtils.getViolationTranslation(violations[0], translate); } @@ -492,9 +492,9 @@ function MoneyRequestView({ { tagListIndex: index, tagListName: name, - tagListValue: TransactionUtils.getTagForDisplay(transaction, index), }, PolicyUtils.hasDependentTags(policy, policyTagList), + TransactionUtils.getTagForDisplay(transaction, index), ) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined @@ -504,9 +504,9 @@ function MoneyRequestView({ { tagListIndex: index, tagListName: name, - tagListValue: TransactionUtils.getTagForDisplay(transaction, index), }, PolicyUtils.hasDependentTags(policy, policyTagList), + TransactionUtils.getTagForDisplay(transaction, index), )} /> diff --git a/src/hooks/useViolations.ts b/src/hooks/useViolations.ts index 9120c0709d9a..83c725a48db0 100644 --- a/src/hooks/useViolations.ts +++ b/src/hooks/useViolations.ts @@ -62,7 +62,7 @@ function useViolations(violations: TransactionViolation[]) { }, [violations]); const getViolationsForField = useCallback( - (field: ViolationField, data?: TransactionViolation['data'], policyHasDependentTags?: boolean) => { + (field: ViolationField, data?: TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string) => { const currentViolations = violationsByField.get(field) ?? []; // someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation @@ -81,7 +81,7 @@ function useViolations(violations: TransactionViolation[]) { // missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags // when no tags are present, so the tag name isn't set in the violation data. That's why we add it here - if (!!policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { + if (policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { return [ { ...currentViolations[0], @@ -100,7 +100,7 @@ function useViolations(violations: TransactionViolation[]) { // allTagLevelsRequired has special logic because it is returned when one but not all the tags are set, // so we need to return the violation for the tag fields without a tag set - if (currentViolations[0]?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && data?.tagListValue) { + if (currentViolations[0]?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && tagValue) { return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName); } diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 2786fdd6ad05..00de9a2175ae 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -9,13 +9,6 @@ import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx'; -const TRANSACTION_VIOLATIONS_FILTER = [ - CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, - CONST.VIOLATIONS.TAG_OUT_OF_POLICY, - CONST.VIOLATIONS.MISSING_TAG, - CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED, -] as ViolationName[]; - /** * Calculates tag out of policy and missing tag violations for the given transaction */ @@ -76,7 +69,7 @@ function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transact tagViolations.push({ name: CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED, type: CONST.VIOLATION_TYPES.VIOLATION, - data: undefined, + data: {}, }); } } @@ -84,19 +77,19 @@ function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transact return tagViolations; } -/** - * Calculates missing tag violations for policies with independent tags, - * by returning one per tag with its corresponding tagName in the data - */ +/** Calculates missing tag violations for policies with independent tags */ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], transaction: Transaction) { const policyTagKeys = getSortedTagKeys(policyTagList); const selectedTags = transaction.tag?.split(CONST.COLON) ?? []; let newTransactionViolations = [...transactionViolations]; + newTransactionViolations = newTransactionViolations.filter( + (violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY, + ); + // We first get the errorIndexes for someTagLevelsRequired. If it's not empty, we puth SOME_TAG_LEVELS_REQUIRED in Onyx. // Otherwise, we put TAG_OUT_OF_POLICY in Onyx (when applicable) const errorIndexes = []; - for (let i = 0; i < policyTagKeys.length; i++) { const isTagRequired = policyTagList[policyTagKeys[i]].required ?? true; const isTagSelected = !!selectedTags[i]; @@ -104,7 +97,6 @@ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transac errorIndexes.push(i); } } - if (errorIndexes.length !== 0) { newTransactionViolations.push({ name: CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, @@ -115,7 +107,6 @@ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transac }); } else { let hasInvalidTag = false; - for (let i = 0; i < policyTagKeys.length; i++) { const selectedTag = selectedTags[i]; const tags = policyTagList[policyTagKeys[i]].tags; @@ -132,14 +123,12 @@ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transac break; } } - if (!hasInvalidTag) { newTransactionViolations = reject(newTransactionViolations, { name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY, }); } } - return newTransactionViolations; } @@ -153,7 +142,13 @@ function getTagViolationsForMultiLevelTags( policyTagList: PolicyTagList, hasDependentTags: boolean, ): TransactionViolation[] { - const filteredTransactionViolations = transactionViolations.filter((violation) => !TRANSACTION_VIOLATIONS_FILTER.includes(violation.name)); + const tagViolations = [ + CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED, + CONST.VIOLATIONS.TAG_OUT_OF_POLICY, + CONST.VIOLATIONS.MISSING_TAG, + CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED, + ] as ViolationName[]; + const filteredTransactionViolations = transactionViolations.filter((violation) => !tagViolations.includes(violation.name)); if (hasDependentTags) { return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations, updatedTransaction.tag ?? ''); diff --git a/src/types/onyx/TransactionViolation.ts b/src/types/onyx/TransactionViolation.ts index e81f7196209b..ab2037ff336a 100644 --- a/src/types/onyx/TransactionViolation.ts +++ b/src/types/onyx/TransactionViolation.ts @@ -28,7 +28,6 @@ type TransactionViolation = { taxName?: string; tagListIndex?: number; tagListName?: string; - tagListValue?: string; errorIndexes?: number[]; pendingPattern?: boolean; }; From df3326a8ee1faad996cb41cc7f10b0bd9b224478 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 7 Jun 2024 16:47:37 +0100 Subject: [PATCH 14/16] refactor: remove unused argument --- src/libs/Violations/ViolationsUtils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index 00de9a2175ae..d03c2d0f4324 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -138,7 +138,6 @@ function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transac function getTagViolationsForMultiLevelTags( updatedTransaction: Transaction, transactionViolations: TransactionViolation[], - policyRequiresTags: boolean, policyTagList: PolicyTagList, hasDependentTags: boolean, ): TransactionViolation[] { @@ -215,7 +214,7 @@ const ViolationsUtils = { newTransactionViolations = Object.keys(policyTagList).length === 1 ? getTagViolationsForSingleLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList) - : getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList, hasDependentTags); + : getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyTagList, hasDependentTags); } return { From e7f175fa239c1ccf8a893b640a7da6d789b07338 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 7 Jun 2024 17:39:48 +0100 Subject: [PATCH 15/16] refactor: apply suggestions --- .../ReportActionItem/MoneyRequestView.tsx | 74 ++++++++----------- src/types/onyx/PolicyTag.ts | 4 + 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index d1fc875832ec..4324b865cee0 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -334,6 +334,37 @@ function MoneyRequestView({ ...parentReportAction?.errors, }; + const TagList = policyTagLists.map(({name, orderWeight}, index) => { + const tagError = getErrorForField( + 'tag', + { + tagListIndex: index, + tagListName: name, + }, + PolicyUtils.hasDependentTags(policy, policyTagList), + TransactionUtils.getTagForDisplay(transaction, index), + ); + return ( + + + Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(CONST.IOU.ACTION.EDIT, iouType, orderWeight, transaction?.transactionID ?? '', report.reportID)) + } + brickRoadIndicator={tagError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined} + errorText={tagError} + /> + + ); + }); + return ( {shouldShowAnimatedBackground && } @@ -469,48 +500,7 @@ function MoneyRequestView({ /> )} - {shouldShowTag && - policyTagLists.map(({name, orderWeight}, index) => ( - - - Navigation.navigate( - ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(CONST.IOU.ACTION.EDIT, iouType, orderWeight, transaction?.transactionID ?? '', report.reportID), - ) - } - brickRoadIndicator={ - getErrorForField( - 'tag', - { - tagListIndex: index, - tagListName: name, - }, - PolicyUtils.hasDependentTags(policy, policyTagList), - TransactionUtils.getTagForDisplay(transaction, index), - ) - ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR - : undefined - } - errorText={getErrorForField( - 'tag', - { - tagListIndex: index, - tagListName: name, - }, - PolicyUtils.hasDependentTags(policy, policyTagList), - TransactionUtils.getTagForDisplay(transaction, index), - )} - /> - - ))} + {shouldShowTag && TagList} {isCardTransaction && ( ; From df5e19f7bd5faab54cdbd605ed5de19fa6d88308 Mon Sep 17 00:00:00 2001 From: Pedro Guerreiro Date: Fri, 7 Jun 2024 18:12:55 +0100 Subject: [PATCH 16/16] refactor: apply suggestions --- src/components/ReportActionItem/MoneyRequestView.tsx | 4 ++-- src/libs/Violations/ViolationsUtils.ts | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestView.tsx b/src/components/ReportActionItem/MoneyRequestView.tsx index 4324b865cee0..cc99a3f6e108 100644 --- a/src/components/ReportActionItem/MoneyRequestView.tsx +++ b/src/components/ReportActionItem/MoneyRequestView.tsx @@ -334,7 +334,7 @@ function MoneyRequestView({ ...parentReportAction?.errors, }; - const TagList = policyTagLists.map(({name, orderWeight}, index) => { + const tagList = policyTagLists.map(({name, orderWeight}, index) => { const tagError = getErrorForField( 'tag', { @@ -500,7 +500,7 @@ function MoneyRequestView({ /> )} - {shouldShowTag && TagList} + {shouldShowTag && tagList} {isCardTransaction && (