[HOLD for payment 2024-04-15] [$500] Use Const for `'violation'` everywhere we check for violation type

[yuwenmemon]

Problem

Right now, in many places where we look to see if the type of a violation is the most severe (violation, as opposed to warn or notice), we check for the string 'violation'. This can lead to issues where it can make it harder to track down instances where code is checking for this. As violation is a pretty generic term.

Example 1 - transactionUtils.tsx
Example 2 - MoneyRequestPreviewContent.tsx
Example 3 - useViolations.tsx hook

Solution

Use a CONST for 'violation' instead. This will have the added benefit of having all Consts for violation types be in the same place once those other types are introduced. This will make the code easier to understand.

cc @cead22 @luacmartins

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f2b3e68e72d70030
• Upwork Job ID: 1773585200383107072
• Last Price Increase: 2024-03-29
• Automatic offers:
• akinwale | Reviewer | 0
• Krishna2323 | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f2b3e68e72d70030

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @akinwale (External)

[Krishna2323]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Use Const for 'violation' everywhere we check for violation type

What is the root cause of that problem?

Improvement.

What changes do you think we should make in order to solve the problem?

There are total 25 instances where we use violation. We use violation in MoneyRequestPreviewContent, useViolations, TransactionUtils, ViolationUtils and ViolationUtilsTest.

We need to create a const inside CONST.ts and replace all instances.

Result

Proposal

Please re-state the problem that we are trying to solve in this issue.

Use Const for 'violation' everywhere we check for violation type

What is the root cause of that problem?

In all these

App/src/libs/TransactionUtils.ts

Line 565 in 723b050

return Boolean(transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some((violation: TransactionViolation) => violation.type === 'violation'));

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 158 in 59c98ce

const violationsCount = violations.filter((v) => v.type === 'violation').length;

App/src/hooks/useViolations.ts

Line 52 in b17a7c9

const filteredViolations = violations.filter((violation) => violation.type === 'violation');

places in our codebase we are using check for the string 'violation'

What changes do you think we should make in order to solve the problem?

We need to create a const inside CONST.ts for 'violation'. Also, we can also add all the types of violations here.

What alternative solutions did you explore? (Optional)

N/A

[akinwale]

Rather straightforward fix. We can move forward with @Krishna2323's proposal since it was first chronologically.

🎀👀🎀 C+ reviewed.

[melvin-bot]

Current assignee @yuwenmemon is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Krishna2323]

Just for info, here we are also checking for notice type messages so we will also create a const for that.

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Use Const for 'violation' everywhere we check for violation type and also ensure that the same is followed in future.

What is the root cause of that problem?

New feature.

What changes do you think we should make in order to solve the problem?

Add a new constant in CONST.ts for violation.

Also, add an ESLint check like below:

create(context) {
        return {
            Literal(node) {
                if (node.value === 'violation') {
                    context.report({
                        node,
                        message: 'Use the constant VIOLATION instead of the string "violation"',
                        fix(fixer) {
                            return fixer.replaceText(node, CONST.VIOLATION);
                        },
                    });
                }
            },
        };
    }

This will ensure that the constant is used in the future as well instead of directly string matching.