From d65a8f306201d12577c65d04f50754003130b377 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 27 Mar 2024 13:58:25 +0800 Subject: [PATCH 1/9] don't optimistically add the violation if it's a partial transaction --- src/libs/actions/IOU.ts | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index b67e44fcf5ad..51656e2ea405 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -807,16 +807,18 @@ function buildOnyxDataForMoneyRequest( if (!policy || !PolicyUtils.isPaidGroupPolicy(policy)) { return [optimisticData, successData, failureData]; } + const isPartialTransaction = TransactionUtils.isMerchantMissing(transaction) && TransactionUtils.isAmountMissing(transaction); + if (!isPartialTransaction) { + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); - - if (violationsOnyxData) { - optimisticData.push(violationsOnyxData); - failureData.push({ - onyxMethod: Onyx.METHOD.SET, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`, - value: [], - }); + if (violationsOnyxData) { + optimisticData.push(violationsOnyxData); + failureData.push({ + onyxMethod: Onyx.METHOD.SET, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`, + value: [], + }); + } } return [optimisticData, successData, failureData]; @@ -1698,7 +1700,8 @@ function getUpdateMoneyRequestParams( }); } - if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction) { + const isPartialTransaction = TransactionUtils.isMerchantMissing(updatedTransaction) && TransactionUtils.isAmountMissing(updatedTransaction); + if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction && !isPartialTransaction) { const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? []; optimisticData.push( ViolationsUtils.getViolationsOnyxData( From f9fc96d8a2a857834c1a82561c70250ddd78b600 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 27 Mar 2024 18:31:45 +0800 Subject: [PATCH 2/9] move the partial transaciton check to getViolationsOnyxData --- src/libs/Violations/ViolationsUtils.ts | 8 +++- src/libs/actions/IOU.ts | 66 +++++++++++++------------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index fe2e5af537a7..be8fe79445dc 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -2,6 +2,7 @@ import reject from 'lodash/reject'; import Onyx from 'react-native-onyx'; import type {OnyxUpdate} from 'react-native-onyx'; import type {Phrase, PhraseParameters} from '@libs/Localize'; +import * as TransactionUtils from '@libs/TransactionUtils'; import CONST from '@src/CONST'; import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -119,7 +120,12 @@ const ViolationsUtils = { policyTagList: PolicyTagList, policyRequiresCategories: boolean, policyCategories: PolicyCategories, - ): OnyxUpdate { + ): OnyxUpdate | null { + const isPartialTransaction = TransactionUtils.isMerchantMissing(updatedTransaction) && TransactionUtils.isAmountMissing(updatedTransaction); + if (isPartialTransaction) { + return null; + } + let newTransactionViolations = [...transactionViolations]; // Calculate client-side category violations diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index 51656e2ea405..f6bd73afd082 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -807,18 +807,16 @@ function buildOnyxDataForMoneyRequest( if (!policy || !PolicyUtils.isPaidGroupPolicy(policy)) { return [optimisticData, successData, failureData]; } - const isPartialTransaction = TransactionUtils.isMerchantMissing(transaction) && TransactionUtils.isAmountMissing(transaction); - if (!isPartialTransaction) { - const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); - if (violationsOnyxData) { - optimisticData.push(violationsOnyxData); - failureData.push({ - onyxMethod: Onyx.METHOD.SET, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`, - value: [], - }); - } + const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {}); + + if (violationsOnyxData) { + optimisticData.push(violationsOnyxData); + failureData.push({ + onyxMethod: Onyx.METHOD.SET, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`, + value: [], + }); } return [optimisticData, successData, failureData]; @@ -1700,24 +1698,24 @@ function getUpdateMoneyRequestParams( }); } - const isPartialTransaction = TransactionUtils.isMerchantMissing(updatedTransaction) && TransactionUtils.isAmountMissing(updatedTransaction); - if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction && !isPartialTransaction) { + if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction) { const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? []; - optimisticData.push( - ViolationsUtils.getViolationsOnyxData( - updatedTransaction, - currentTransactionViolations, - !!policy.requiresTag, - policyTagList ?? {}, - !!policy.requiresCategory, - policyCategories ?? {}, - ), + const updatedViolationsOnyxData = ViolationsUtils.getViolationsOnyxData( + updatedTransaction, + currentTransactionViolations, + !!policy.requiresTag, + policyTagList ?? {}, + !!policy.requiresCategory, + policyCategories ?? {}, ); - failureData.push({ - onyxMethod: Onyx.METHOD.MERGE, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, - value: currentTransactionViolations, - }); + if (updatedViolationsOnyxData) { + optimisticData.push(updatedViolationsOnyxData); + failureData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, + value: currentTransactionViolations, + }); + } } // Reset the transaction thread to its original state @@ -3508,12 +3506,14 @@ function editRegularMoneyRequest( !!policy.requiresCategory, policyCategories, ); - optimisticData.push(updatedViolationsOnyxData); - failureData.push({ - onyxMethod: Onyx.METHOD.MERGE, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, - value: currentTransactionViolations, - }); + if (updatedViolationsOnyxData) { + optimisticData.push(updatedViolationsOnyxData); + failureData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, + value: currentTransactionViolations, + }); + } } // STEP 6: Call the API endpoint From bdcdc471543de25c3107e3e5c9c2996b21e60436 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 27 Mar 2024 18:37:25 +0800 Subject: [PATCH 3/9] null safety --- tests/unit/ViolationUtilsTest.ts | 40 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index 354a90802077..4c9a2c344363 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -57,7 +57,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: 'violation'}, ]; const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining(transactionViolations)); + expect(result?.value).toEqual(expect.arrayContaining(transactionViolations)); }); describe('policyRequiresCategories', () => { @@ -70,18 +70,18 @@ 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); - expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); + 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); - expect(result.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); + 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); - expect(result.value).not.toContainEqual(categoryOutOfPolicyViolation); + expect(result?.value).not.toContainEqual(categoryOutOfPolicyViolation); }); it('should add categoryOutOfPolicy violation to existing violations if they exist', () => { @@ -93,7 +93,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); + expect(result?.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); }); it('should add missingCategory violation to existing violations if they exist', () => { @@ -105,7 +105,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); + expect(result?.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); }); }); @@ -117,8 +117,8 @@ describe('getViolationsOnyxData', () => { it('should not add any violations when categories are not required', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).not.toContainEqual([categoryOutOfPolicyViolation]); - expect(result.value).not.toContainEqual([missingCategoryViolation]); + expect(result?.value).not.toContainEqual([categoryOutOfPolicyViolation]); + expect(result?.value).not.toContainEqual([missingCategoryViolation]); }); }); @@ -142,7 +142,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); - expect(result.value).toEqual(transactionViolations); + expect(result?.value).toEqual(transactionViolations); }); it('should add a missingTag violation if none is provided and policy requires tags', () => { @@ -150,7 +150,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}])); + expect(result?.value).toEqual(expect.arrayContaining([{...missingTagViolation}])); }); it('should add a tagOutOfPolicy violation when policy requires tags and tag is not in the policy', () => { @@ -158,7 +158,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual([]); + expect(result?.value).toEqual([]); }); it('should add tagOutOfPolicy violation to existing violations if transaction has tag that is not in the policy', () => { @@ -170,7 +170,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations])); + expect(result?.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations])); }); it('should add missingTag violation to existing violations if transaction does not have a tag', () => { @@ -182,7 +182,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations])); + expect(result?.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations])); }); }); @@ -194,8 +194,8 @@ describe('getViolationsOnyxData', () => { it('should not add any violations when tags are not required', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).not.toContainEqual([tagOutOfPolicyViolation]); - expect(result.value).not.toContainEqual([missingTagViolation]); + expect(result?.value).not.toContainEqual([tagOutOfPolicyViolation]); + expect(result?.value).not.toContainEqual([missingTagViolation]); }); }); describe('policy has multi level tags', () => { @@ -248,31 +248,31 @@ describe('getViolationsOnyxData', () => { // Test case where transaction has no tags let result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual([someTagLevelsRequiredViolation]); + expect(result?.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 1 tag transaction.tag = 'Accounting'; someTagLevelsRequiredViolation.data = {errorIndexes: [1, 2]}; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual([someTagLevelsRequiredViolation]); + expect(result?.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 2 tags transaction.tag = 'Accounting::Project1'; someTagLevelsRequiredViolation.data = {errorIndexes: [1]}; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual([someTagLevelsRequiredViolation]); + expect(result?.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has all tags transaction.tag = 'Accounting:Africa:Project1'; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual([]); + 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 = 'Accounting:Africa:Project1'; const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); const violation = {...tagOutOfPolicyViolation, data: {tagName: 'Department'}}; - expect(result.value).toEqual([violation]); + expect(result?.value).toEqual([violation]); }); }); }); From 39b8c46885695e70983ea14d0e4177835d0c9f80 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 27 Mar 2024 18:39:36 +0800 Subject: [PATCH 4/9] add test --- tests/unit/ViolationUtilsTest.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index 4c9a2c344363..7da8352cdb53 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -60,6 +60,16 @@ describe('getViolationsOnyxData', () => { expect(result?.value).toEqual(expect.arrayContaining(transactionViolations)); }); + it('should not add violation when the transaction is partial', () => { + const partialTransaction = {...transaction, amount: 0, merchant: ''}; + transactionViolations = [ + {name: 'duplicatedTransaction', type: 'violation'}, + {name: 'receiptRequired', type: 'violation'}, + ]; + const result = ViolationsUtils.getViolationsOnyxData(partialTransaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); + expect(result).toBeNull(); + }); + describe('policyRequiresCategories', () => { beforeEach(() => { policyRequiresCategories = true; From 2c9efa0dca63e63600452a45891bc3f01ec0e74f Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 28 Mar 2024 11:22:01 +0800 Subject: [PATCH 5/9] update partial merchant condition --- src/libs/Violations/ViolationsUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index be8fe79445dc..c707b9d45451 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -121,7 +121,7 @@ const ViolationsUtils = { policyRequiresCategories: boolean, policyCategories: PolicyCategories, ): OnyxUpdate | null { - const isPartialTransaction = TransactionUtils.isMerchantMissing(updatedTransaction) && TransactionUtils.isAmountMissing(updatedTransaction); + const isPartialTransaction = TransactionUtils.isPartialMerchant(TransactionUtils.getMerchant(updatedTransaction)) && TransactionUtils.isAmountMissing(updatedTransaction); if (isPartialTransaction) { return null; } From 3a5827ff56cd0622f093e5195019e8ca6cc5c88d Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 28 Mar 2024 11:26:20 +0800 Subject: [PATCH 6/9] fix test case --- tests/unit/ViolationUtilsTest.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index 7da8352cdb53..10c08266a1e2 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -1,6 +1,7 @@ import {beforeEach} from '@jest/globals'; import Onyx from 'react-native-onyx'; import ViolationsUtils from '@libs/Violations/ViolationsUtils'; +import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation} from '@src/types/onyx'; @@ -61,7 +62,7 @@ describe('getViolationsOnyxData', () => { }); it('should not add violation when the transaction is partial', () => { - const partialTransaction = {...transaction, amount: 0, merchant: ''}; + const partialTransaction = {...transaction, amount: 0, merchant: CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT}; transactionViolations = [ {name: 'duplicatedTransaction', type: 'violation'}, {name: 'receiptRequired', type: 'violation'}, From 0612ffd05ba26b5d6c346cd9a99ecf556090f69c Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 29 Mar 2024 15:09:51 +0800 Subject: [PATCH 7/9] return current violation if it's a partial transaction --- src/libs/Violations/ViolationsUtils.ts | 8 +++-- src/libs/actions/IOU.ts | 43 ++++++++++++-------------- tests/unit/ViolationUtilsTest.ts | 42 ++++++++++++------------- 3 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index c707b9d45451..0d24c3bd2757 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -120,10 +120,14 @@ const ViolationsUtils = { policyTagList: PolicyTagList, policyRequiresCategories: boolean, policyCategories: PolicyCategories, - ): OnyxUpdate | null { + ): OnyxUpdate { const isPartialTransaction = TransactionUtils.isPartialMerchant(TransactionUtils.getMerchant(updatedTransaction)) && TransactionUtils.isAmountMissing(updatedTransaction); if (isPartialTransaction) { - return null; + return { + onyxMethod: Onyx.METHOD.SET, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${updatedTransaction.transactionID}`, + value: transactionViolations, + }; } let newTransactionViolations = [...transactionViolations]; diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index f6bd73afd082..b67e44fcf5ad 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -1700,22 +1700,21 @@ function getUpdateMoneyRequestParams( if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction) { const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? []; - const updatedViolationsOnyxData = ViolationsUtils.getViolationsOnyxData( - updatedTransaction, - currentTransactionViolations, - !!policy.requiresTag, - policyTagList ?? {}, - !!policy.requiresCategory, - policyCategories ?? {}, + optimisticData.push( + ViolationsUtils.getViolationsOnyxData( + updatedTransaction, + currentTransactionViolations, + !!policy.requiresTag, + policyTagList ?? {}, + !!policy.requiresCategory, + policyCategories ?? {}, + ), ); - if (updatedViolationsOnyxData) { - optimisticData.push(updatedViolationsOnyxData); - failureData.push({ - onyxMethod: Onyx.METHOD.MERGE, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, - value: currentTransactionViolations, - }); - } + failureData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, + value: currentTransactionViolations, + }); } // Reset the transaction thread to its original state @@ -3506,14 +3505,12 @@ function editRegularMoneyRequest( !!policy.requiresCategory, policyCategories, ); - if (updatedViolationsOnyxData) { - optimisticData.push(updatedViolationsOnyxData); - failureData.push({ - onyxMethod: Onyx.METHOD.MERGE, - key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, - value: currentTransactionViolations, - }); - } + optimisticData.push(updatedViolationsOnyxData); + failureData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`, + value: currentTransactionViolations, + }); } // STEP 6: Call the API endpoint diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index 10c08266a1e2..08d7e8cd3d08 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -58,7 +58,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: 'violation'}, ]; const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining(transactionViolations)); + expect(result.value).toEqual(expect.arrayContaining(transactionViolations)); }); it('should not add violation when the transaction is partial', () => { @@ -68,7 +68,7 @@ describe('getViolationsOnyxData', () => { {name: 'receiptRequired', type: 'violation'}, ]; const result = ViolationsUtils.getViolationsOnyxData(partialTransaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result).toBeNull(); + expect(result.value).toEqual(transactionViolations); }); describe('policyRequiresCategories', () => { @@ -81,18 +81,18 @@ 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); - expect(result?.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); + 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); - expect(result?.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); + 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); - expect(result?.value).not.toContainEqual(categoryOutOfPolicyViolation); + expect(result.value).not.toContainEqual(categoryOutOfPolicyViolation); }); it('should add categoryOutOfPolicy violation to existing violations if they exist', () => { @@ -104,7 +104,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); + expect(result.value).toEqual(expect.arrayContaining([categoryOutOfPolicyViolation, ...transactionViolations])); }); it('should add missingCategory violation to existing violations if they exist', () => { @@ -116,7 +116,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); + expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations])); }); }); @@ -128,8 +128,8 @@ describe('getViolationsOnyxData', () => { it('should not add any violations when categories are not required', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).not.toContainEqual([categoryOutOfPolicyViolation]); - expect(result?.value).not.toContainEqual([missingCategoryViolation]); + expect(result.value).not.toContainEqual([categoryOutOfPolicyViolation]); + expect(result.value).not.toContainEqual([missingCategoryViolation]); }); }); @@ -153,7 +153,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); - expect(result?.value).toEqual(transactionViolations); + expect(result.value).toEqual(transactionViolations); }); it('should add a missingTag violation if none is provided and policy requires tags', () => { @@ -161,7 +161,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining([{...missingTagViolation}])); + expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}])); }); it('should add a tagOutOfPolicy violation when policy requires tags and tag is not in the policy', () => { @@ -169,7 +169,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual([]); + expect(result.value).toEqual([]); }); it('should add tagOutOfPolicy violation to existing violations if transaction has tag that is not in the policy', () => { @@ -181,7 +181,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations])); + expect(result.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations])); }); it('should add missingTag violation to existing violations if transaction does not have a tag', () => { @@ -193,7 +193,7 @@ describe('getViolationsOnyxData', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations])); + expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations])); }); }); @@ -205,8 +205,8 @@ describe('getViolationsOnyxData', () => { it('should not add any violations when tags are not required', () => { const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).not.toContainEqual([tagOutOfPolicyViolation]); - expect(result?.value).not.toContainEqual([missingTagViolation]); + expect(result.value).not.toContainEqual([tagOutOfPolicyViolation]); + expect(result.value).not.toContainEqual([missingTagViolation]); }); }); describe('policy has multi level tags', () => { @@ -259,31 +259,31 @@ describe('getViolationsOnyxData', () => { // Test case where transaction has no tags let result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual([someTagLevelsRequiredViolation]); + expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 1 tag transaction.tag = 'Accounting'; someTagLevelsRequiredViolation.data = {errorIndexes: [1, 2]}; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual([someTagLevelsRequiredViolation]); + expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has 2 tags transaction.tag = 'Accounting::Project1'; someTagLevelsRequiredViolation.data = {errorIndexes: [1]}; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual([someTagLevelsRequiredViolation]); + expect(result.value).toEqual([someTagLevelsRequiredViolation]); // Test case where transaction has all tags transaction.tag = 'Accounting:Africa:Project1'; result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result?.value).toEqual([]); + 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 = 'Accounting:Africa:Project1'; const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); const violation = {...tagOutOfPolicyViolation, data: {tagName: 'Department'}}; - expect(result?.value).toEqual([violation]); + expect(result.value).toEqual([violation]); }); }); }); From 9e7f5b790a84b8c359dab6e31c6428690ce6ace7 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 30 Mar 2024 13:56:20 +0800 Subject: [PATCH 8/9] update test --- tests/unit/ViolationUtilsTest.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/unit/ViolationUtilsTest.ts b/tests/unit/ViolationUtilsTest.ts index e112f2908590..8389086f8720 100644 --- a/tests/unit/ViolationUtilsTest.ts +++ b/tests/unit/ViolationUtilsTest.ts @@ -61,16 +61,6 @@ describe('getViolationsOnyxData', () => { expect(result.value).toEqual(expect.arrayContaining(transactionViolations)); }); - it('should not add violation when the transaction is partial', () => { - const partialTransaction = {...transaction, amount: 0, merchant: CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT}; - transactionViolations = [ - {name: 'duplicatedTransaction', type: 'violation'}, - {name: 'receiptRequired', type: 'violation'}, - ]; - const result = ViolationsUtils.getViolationsOnyxData(partialTransaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories); - expect(result.value).toEqual(transactionViolations); - }); - describe('policyRequiresCategories', () => { beforeEach(() => { policyRequiresCategories = true; @@ -95,6 +85,12 @@ describe('getViolationsOnyxData', () => { 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); + expect(result.value).not.toContainEqual(missingCategoryViolation); + }); + it('should add categoryOutOfPolicy violation to existing violations if they exist', () => { transaction.category = 'Bananas'; transactionViolations = [ @@ -172,6 +168,12 @@ describe('getViolationsOnyxData', () => { 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); + expect(result.value).not.toContainEqual(missingTagViolation); + }); + it('should add tagOutOfPolicy violation to existing violations if transaction has tag that is not in the policy', () => { transaction.tag = 'Bananas'; transactionViolations = [ From 37a9a6a888efae02094d54856d175a26043cf11b Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 30 Mar 2024 14:04:47 +0800 Subject: [PATCH 9/9] prettier --- src/libs/Violations/ViolationsUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Violations/ViolationsUtils.ts b/src/libs/Violations/ViolationsUtils.ts index d3c1e6e19cd0..83f3b2fcc154 100644 --- a/src/libs/Violations/ViolationsUtils.ts +++ b/src/libs/Violations/ViolationsUtils.ts @@ -2,8 +2,8 @@ import reject from 'lodash/reject'; import Onyx from 'react-native-onyx'; import type {OnyxUpdate} from 'react-native-onyx'; import type {Phrase, PhraseParameters} from '@libs/Localize'; -import * as TransactionUtils from '@libs/TransactionUtils'; import {getSortedTagKeys} from '@libs/PolicyUtils'; +import * as TransactionUtils from '@libs/TransactionUtils'; import CONST from '@src/CONST'; import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS';