fix: add missing violation data #54603

daledah · 2024-12-27T09:21:28Z

Explanation of Change

Fixed Issues

$ #54510
PROPOSAL: #54510 (comment)

Tests

Precondition:

In workspace, have a category with GL code and Payroll code
Enable “Member must categorize all expenses”

Steps:

Go Offline
Create an expense with big amount and set to a future date
Go to WS chat
Verify that: RBR is shown
Open expense report
Verify that: RBR and error text is displayed in receipt, amount and date field.

Verify that no errors appear in the JS console

Offline tests

Same as above

QA Steps

Precondition:

In workspace, have a category with GL code and Payroll code
Enable “Member must categorize all expenses”

Steps:

Go Offline
Create an expense with big amount and set to a future date
Go to WS chat
Verify that: RBR is shown
Open expense report
Verify that: RBR and error text is displayed in receipt, amount and date field.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-12-27.at.12.37.14.mov

Android: mWeb Chrome

Screen.Recording.2024-12-27.at.12.38.46.mov

iOS: Native

Screen.Recording.2024-12-27.at.12.40.46.mov

iOS: mWeb Safari

Screen.Recording.2024-12-27.at.12.41.54.mov

MacOS: Chrome / Safari

Screen.Recording.2024-12-27.at.12.15.54.mov

MacOS: Desktop

Screen.Recording.2024-12-27.at.12.42.34.mov

melvin-bot · 2024-12-30T09:09:36Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

s77rt · 2024-12-30T14:31:15Z

src/libs/Violations/ViolationsUtils.ts

+                policy.maxExpenseAmountNoReceipt &&
+                Math.abs(updatedTransaction.amount) > policy.maxExpenseAmountNoReceipt &&
+                !updatedTransaction.receipt?.receiptID


Declare this as a new const

s77rt · 2024-12-30T14:31:56Z

src/libs/Violations/ViolationsUtils.ts

+                newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.RECEIPT_REQUIRED});
+            }
+
+            if (!hasOverLimitViolation && policyMaxExpenseAmount && Math.abs(updatedTransaction.amount) > policyMaxExpenseAmount) {


Same ^ (only the part after !hasOverLimitViolation &&)

s77rt · 2024-12-30T14:33:38Z

src/libs/Violations/ViolationsUtils.ts

+            const hasFutureDateViolation = transactionViolations.some((violation) => violation.name === 'futureDate');
+            const hasReceiptRequiredViolation = transactionViolations.some((violation) => violation.name === 'receiptRequired');
+            const hasOverLimitViolation = transactionViolations.some((violation) => violation.name === 'overLimit');
+            const policyMaxExpenseAmount = policy.maxExpenseAmount;


Suggested change

const policyMaxExpenseAmount = policy.maxExpenseAmount;

Unnecessary const

s77rt · 2024-12-30T14:36:33Z

tests/unit/ViolationUtilsTest.ts

@@ -125,6 +154,26 @@ describe('getViolationsOnyxData', () => {
            expect(result.value).toEqual(expect.arrayContaining([missingCategoryViolation, ...transactionViolations]));
        });

+        it('should add futureDate violation if the transaction has a future date', () => {


Are we sure that all workspaces disallow future-dated expenses?

…code

s77rt · 2025-01-06T17:02:58Z

src/libs/Violations/ViolationsUtils.ts

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

+        const inputDate = new Date(updatedTransaction.created);
+        const shouldDisplayFutureDateViolation = DateUtils.isFutureDate(inputDate) && policy.type === CONST.POLICY.TYPE.CORPORATE;


Should this be && type != TEAM?

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

s77rt · 2025-01-06T17:12:48Z

src/libs/Violations/ViolationsUtils.ts

+            const shouldShowReceiptRequiredViolation =
+                policy.maxExpenseAmountNoReceipt && Math.abs(updatedTransaction.amount) > policy.maxExpenseAmountNoReceipt && !updatedTransaction.receipt?.receiptID;


Use TransactionUtils.hasReceipt

s77rt · 2025-01-06T17:42:42Z

src/libs/Violations/ViolationsUtils.ts

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

+        const inputDate = new Date(updatedTransaction.created);


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)

s77rt · 2025-01-06T17:44:25Z

src/libs/DateUtils.ts

+/**
+ * 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()));
+};
+


Can we remove startOfDay?

s77rt · 2025-01-06T17:55:52Z

tests/unit/ViolationUtilsTest.ts

+        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);
+        });


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

daledah added 4 commits December 27, 2024 16:19

fix: add missing violation data

87a8010

fix: lint

8e3ecda

fix: violation test

5b40b67

fix: add new tests for violation utils

b81691d

daledah marked this pull request as ready for review December 30, 2024 09:09

daledah requested a review from a team as a code owner December 30, 2024 09:09

melvin-bot bot requested review from s77rt and removed request for a team December 30, 2024 09:09

s77rt suggested changes Dec 30, 2024

View reviewed changes

fix: change future date violation logics and tests, remove redundant …

5a9b1a0

…code

s77rt suggested changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing violation data #54603

fix: add missing violation data #54603

daledah commented Dec 27, 2024

melvin-bot bot commented Dec 30, 2024

s77rt Dec 30, 2024

s77rt Dec 30, 2024

s77rt Dec 30, 2024

s77rt Dec 30, 2024

s77rt Jan 6, 2025

s77rt Jan 6, 2025

s77rt Jan 6, 2025

s77rt Jan 6, 2025

s77rt Jan 6, 2025

s77rt Jan 6, 2025

		const shouldShowReceiptRequiredViolation =
		policy.maxExpenseAmountNoReceipt && Math.abs(updatedTransaction.amount) > policy.maxExpenseAmountNoReceipt && !updatedTransaction.receipt?.receiptID;

fix: add missing violation data #54603

Are you sure you want to change the base?

fix: add missing violation data #54603

Conversation

daledah commented Dec 27, 2024

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment