Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add missing violation data #54603

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+132 −9
Open

fix: add missing violation data #54603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
87a8010
fix: add missing violation data
daledah Dec 27, 2024
8e3ecda
fix: lint
daledah Dec 27, 2024
5b40b67
fix: violation test
daledah Dec 27, 2024
b81691d
fix: add new tests for violation utils
daledah Dec 30, 2024
5a9b1a0
fix: change future date violation logics and tests, remove redundant …
daledah Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/libs/DateUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,15 @@ const getDayValidationErrorKey = (inputDate: Date): string => {
return '';
};

/**
* Checks if the input time is after the reference date
* param {Date} inputDate - The date to validate.
* returns {boolean} - Returns true if the input date is after the reference date, otherwise false.
*/
const isFutureDate = (inputDate: Date): boolean => {
return isAfter(startOfDay(inputDate), startOfDay(new Date()));
};

Comment on lines +681 to +689
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove startOfDay?

/**
* Checks if the input time is at least one minute in the future compared to the reference time.
* param {Date} inputTime - The time to validate.
Expand Down Expand Up @@ -937,6 +946,7 @@ const DateUtils = {
isValidDateString,
getFormattedDurationBetweenDates,
getFormattedDuration,
isFutureDate,
};

export default DateUtils;
52 changes: 51 additions & 1 deletion src/libs/Violations/ViolationsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import reject from 'lodash/reject';
import Onyx from 'react-native-onyx';
import type {OnyxUpdate} from 'react-native-onyx';
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import * as CurrencyUtils from '@libs/CurrencyUtils';
import DateUtils from '@libs/DateUtils';
import {getDistanceRateCustomUnitRate, getSortedTagKeys} from '@libs/PolicyUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -187,6 +189,11 @@ const ViolationsUtils = {
const hasMissingCategoryViolation = transactionViolations.some((violation) => violation.name === 'missingCategory');
const categoryKey = updatedTransaction.category;
const isCategoryInPolicy = categoryKey ? policyCategories?.[categoryKey]?.enabled : false;
const hasReceiptRequiredViolation = transactionViolations.some((violation) => violation.name === 'receiptRequired');
const hasOverLimitViolation = transactionViolations.some((violation) => violation.name === 'overLimit');
const shouldShowReceiptRequiredViolation =
policy.maxExpenseAmountNoReceipt && Math.abs(updatedTransaction.amount) > policy.maxExpenseAmountNoReceipt && !updatedTransaction.receipt?.receiptID;
Comment on lines +194 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

Use TransactionUtils.hasReceipt

const shouldShowOverLimitViolation = policy.maxExpenseAmount && Math.abs(updatedTransaction.amount) > policy.maxExpenseAmount;

// Add 'categoryOutOfPolicy' violation if category is not in policy
if (!hasCategoryOutOfPolicyViolation && categoryKey && !isCategoryInPolicy) {
Expand All @@ -205,7 +212,37 @@ const ViolationsUtils = {

// Add 'missingCategory' violation if category is required and not set
if (!hasMissingCategoryViolation && policyRequiresCategories && !categoryKey) {
newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION});
newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION, showInReview: true});
}

if (!hasReceiptRequiredViolation && shouldShowReceiptRequiredViolation) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.RECEIPT_REQUIRED,
data: {
formattedLimit: CurrencyUtils.convertAmountToDisplayString(policy.maxExpenseAmountNoReceipt),
},
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
});
}

if (hasReceiptRequiredViolation && !shouldShowReceiptRequiredViolation) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.RECEIPT_REQUIRED});
}

if (!hasOverLimitViolation && shouldShowOverLimitViolation) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.OVER_LIMIT,
data: {
formattedLimit: CurrencyUtils.convertAmountToDisplayString(policy.maxExpenseAmount),
},
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
});
}

if (hasOverLimitViolation && !shouldShowOverLimitViolation) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.OVER_LIMIT});
}
}

Expand All @@ -222,6 +259,19 @@ const ViolationsUtils = {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY});
}

const inputDate = new Date(updatedTransaction.created);
Copy link
Contributor

Choose a reason for hiding this comment

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

We have two fields created and modifiedCreated. If you update the transaction to a non-future date, will the violation go away correctly? (or do we have to check modifiedCreated)

(same goes for amount)

const shouldDisplayFutureDateViolation = DateUtils.isFutureDate(inputDate) && policy.type === CONST.POLICY.TYPE.CORPORATE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be && type != TEAM?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what about this being an invoice? There is not violation in this case either

const hasFutureDateViolation = transactionViolations.some((violation) => violation.name === 'futureDate');
// Add 'futureDate' violation if transaction date is in the future and policy type is corporate
if (!hasFutureDateViolation && shouldDisplayFutureDateViolation) {
newTransactionViolations.push({name: CONST.VIOLATIONS.FUTURE_DATE, type: CONST.VIOLATION_TYPES.VIOLATION, showInReview: true});
}

// Remove 'futureDate' violation if transaction date is not in the future
if (hasFutureDateViolation && !shouldDisplayFutureDateViolation) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.FUTURE_DATE});
}

return {
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${updatedTransaction.transactionID}`,
Expand Down
79 changes: 71 additions & 8 deletions tests/unit/ViolationUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {beforeEach} from '@jest/globals';
import Onyx from 'react-native-onyx';
import * as CurrencyUtils from '@libs/CurrencyUtils';
import ViolationsUtils from '@libs/Violations/ViolationsUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -13,6 +14,31 @@ const categoryOutOfPolicyViolation = {
const missingCategoryViolation = {
name: CONST.VIOLATIONS.MISSING_CATEGORY,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
};

const futureDateViolation = {
name: CONST.VIOLATIONS.FUTURE_DATE,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
};

const receiptRequiredViolation = {
name: CONST.VIOLATIONS.RECEIPT_REQUIRED,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
data: {
formattedLimit: CurrencyUtils.convertAmountToDisplayString(CONST.POLICY.DEFAULT_MAX_AMOUNT_NO_RECEIPT),
},
};

const overLimitViolation = {
name: CONST.VIOLATIONS.OVER_LIMIT,
type: CONST.VIOLATION_TYPES.VIOLATION,
showInReview: true,
data: {
formattedLimit: CurrencyUtils.convertAmountToDisplayString(CONST.POLICY.DEFAULT_MAX_EXPENSE_AMOUNT),
},
};

const customUnitOutOfPolicyViolation = {
Expand Down Expand Up @@ -112,11 +138,40 @@ describe('getViolationsOnyxData', () => {
});
});

describe('futureDateViolations', () => {
beforeEach(() => {
transaction.created = '9999-12-31T23:59:59Z';
policy.type = 'corporate';
});

it('should not add futureDate violation if the policy is not corporate', () => {
policy.type = 'personal';
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
expect(result.value).toEqual(transactionViolations);
});

it('should add futureDate violation if the transaction has a future date and policy is corporate', () => {
policy.type = 'corporate';
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
expect(result.value).toEqual(expect.arrayContaining([futureDateViolation, ...transactionViolations]));
});

it('should remove futureDate violation if the policy is downgraded', () => {
policy.type = 'personal';
transactionViolations = [futureDateViolation];
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
expect(result.value).not.toContainEqual(futureDateViolation);
});
Comment on lines +147 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rewrite this to be based on whether the policy is team or not

});

describe('policyRequiresCategories', () => {
beforeEach(() => {
policy.requiresCategory = true;
policyCategories = {Food: {name: 'Food', unencodedName: '', enabled: true, areCommentsRequired: false, externalID: '1234', origin: '12345'}};
transaction.category = 'Food';
transaction.amount = 100;
policy.maxExpenseAmount = CONST.POLICY.DEFAULT_MAX_EXPENSE_AMOUNT;
policy.maxExpenseAmountNoReceipt = CONST.POLICY.DEFAULT_MAX_AMOUNT_NO_RECEIPT;
});

it('should add missingCategory violation if no category is included', () => {
Expand All @@ -125,6 +180,18 @@ describe('getViolationsOnyxData', () => {
expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations]));
});

it('should add receiptRequired violation if the transaction has no receipt', () => {
transaction.amount = 1000000;
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
expect(result.value).toEqual(expect.arrayContaining([receiptRequiredViolation, ...transactionViolations]));
});

it('should add overLimit violation if the transaction amount is over the policy limit', () => {
transaction.amount = 1000000;
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
expect(result.value).toEqual(expect.arrayContaining([overLimitViolation, ...transactionViolations]));
});

it('should add categoryOutOfPolicy violation when category is not in policy', () => {
transaction.category = 'Bananas';
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);
Expand All @@ -144,10 +211,8 @@ describe('getViolationsOnyxData', () => {

it('should add categoryOutOfPolicy violation to existing violations if they exist', () => {
transaction.category = 'Bananas';
transactionViolations = [
{name: 'duplicatedTransaction', type: CONST.VIOLATION_TYPES.VIOLATION},
{name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION},
];
transaction.amount = 1000000;
transactionViolations = [{name: 'duplicatedTransaction', type: CONST.VIOLATION_TYPES.VIOLATION}, receiptRequiredViolation];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);

Expand All @@ -156,10 +221,8 @@ describe('getViolationsOnyxData', () => {

it('should add missingCategory violation to existing violations if they exist', () => {
transaction.category = undefined;
transactionViolations = [
{name: 'duplicatedTransaction', type: CONST.VIOLATION_TYPES.VIOLATION},
{name: 'receiptRequired', type: CONST.VIOLATION_TYPES.VIOLATION},
];
transaction.amount = 1000000;
transactionViolations = [{name: 'duplicatedTransaction', type: CONST.VIOLATION_TYPES.VIOLATION}, receiptRequiredViolation];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policy, policyTags, policyCategories, false);

Expand Down
Loading