Clean up Violation tests

[cead22]

As a follow up to this PR #31656 (review), let's address the comments in that review

@trevor-coleman @cdanwards @lindboe please comment so I can assign you

[cdanwards]

Comment for assignment

[trevor-coleman]

Comment for assignment

[melvin-bot]

📣 @trevor-coleman! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[trevor-coleman]

The outstanding issues are I think:

1. Whether to keep transactionViolations as a variable in ViolationUtilsTest
2. Whether to keep the export of hasViolations from the useViolations hook

I'll copy my comments over so we can discuss here:

1. transactionViolations

From this thread:

transactionViolations is used in every test as it's one of the parameters of the function that is being tested, so even if it is not being redefined, it's base value is being used in every function call.

Declaring it in each test would also mean that if the base case changed in the future, every test would need to be edited to reflect this change.

In Jest, there's a specific approach to handling variables across tests

1. Shared Variable Scope: Variables are declared once in the outer describe scope, shared across tests. Their values are reset in beforeEach, ensuring test isolation.

2. beforeEach Initialization: Executes before each test, assigning default values to shared variables (declared in the outer scope with let to allow reassignment). This guarantees a clean data set for each test, avoiding interference from preceding tests.

3. Consistent Starting Point: Default initialization in beforeEach (e.g., transactionViolations as an empty array) provides predictability and readability, a common Jest practice.

4. Naming Consistency: Best practice is have a single variable for each parameter of the function being tested, named identically to the parameter for easy overrides and clear understanding of modifications within tests.

If you really want me to break the standard I can do that, but I strongly recommend against it as consistent variable handling in jest makes tests much easier to maintain for future devs on the project.

2. hasViolations()

From this thread:

I get the concerns about the apparent redundancy of getViolationsForField and hasViolations. Right now, neither is used in the main codebase yet because this issue & PR are only focused on setting them up for future use in other PRs, which aren't visible here yet.

The improved readability will be most important when we actually start to use this code in the MoneyRequestView component.

In React development, as you know, it's best to keep the component body as clean as possible for readability's sake. This becomes especially important when dealing with complex JSX syntax and nested components, which can quickly become hard to navigate.

Take, for instance, our MenuItemWithTopDescription component – it's already getting pretty large:

<MenuItemWithTopDescription
    description={translate('common.description')}
    shouldParseTitle
    title={transactionDescription}
    interactive={canEdit}
    shouldShowRightIcon={canEdit}
    titleStyle={styles.flex1}
    onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION))}
    wrapperStyle={[styles.pv2, styles.taskDescriptionMenuItem]}
    brickRoadIndicator={hasViolations('comment') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} 
    numberOfLinesTitle={0}
/>

Replacing

brickRoadIndicator={hasViolations('comment') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} 

with

brickRoadIndicator={getViolationForField('comment').length > 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

May seem to be a relatively small increase in complexity in itself, but the problem is cumulative, so we need to work to improve readability as much as possible where we can.

Since this logic repeats across several components, implementing hasViolations makes sense for consistency and clarity. Placing it in the useViolations() hook aligns it with related code, making it easier to understand.

[cead22]

• re: 1. transactionVilations, I don't feel strongly, so I'll lean on what you're suggesting here
• re: 2. hasViolations, I still think we don't need that function, and I don't agree that the second version of the code below is worse than the first, even if the problem you're describing is cumulative. Is hasViolation only going to be called in MoneyRequestView ? If so, perhaps we can define a local hasViolations function inside MoneyRequestView, though I still like the simplicity of using getViolationForField better

brickRoadIndicator={hasViolations('comment') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} 

brickRoadIndicator={getViolationForField('comment').length > 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

[trevor-coleman]

In react the first is definitely preferred to the second, but if you feel strongly about it I can not use hasViolations and instead add the conditional each time (I think there are six) that we need to calcuate that value.

[trevor-coleman]

Should I open a new PR for this? And if so, should I wait for the old PR to get merged first?

[cead22]

Yes, and yes, but the old PR is already merged, right?

[trevor-coleman]

Yeah that message was pre-merge.

I think all of the feedback from the PR got into the other branch before it was merged -- or are there outstanding issues you still wanted addressed?

[cead22]

If all the other feedback was addressed, hasViolations is the only outstanding change, but if that's really the only one, we can just close this issue and do that later if needed

[trevor-coleman]

If that's not removed in that branch, I've removed it in the MoneyRequestView branch so it'll get cleaned up regardless.