Implement ViolationUtils lib

[cead22]

• Design Doc
• Implement a new ViolationUtils lib (src/libs/ViolationUtils.js or src/libs/ViolationUtils.ts)
  1. Add a method getViolationForField(object transactionViolation, string field) : string
     • getViolationForField will check transactionViolation and return an an array of strings with the translated copies of the violations for that field, eg
     • When field is ‘category’, the method will pull the violations that apply to the category field, and then call translate on the violation names, like missingCategory and return that
     • See code example below
  2. Add a method getViolationsOnyxData(Transaction transaction, object transactionViolations, bool policyRequiresTags, object[] policyTags, bool policyRequiresCategories, object[] policyCategories) that calculates the new transaction violations (by adding/removing those that have been addressed with the current user changes), and returns an object that we can save into onyx. The updatedTransactionViolations variable shown in the code snippet below will be set like so:
     • If policyRequiresCategories === true, the transactionViolations array doesn't have one with name: categoryOutOfPolicy and the passed transaction contains a category, and the policyCategories collection doesn’t have that category with its enabled value set to true, then it’ll add the object {‘name’:’categoryOutOfPolicy’, ‘type’: ‘violation’} to updatedTransactionViolations
     • If policyRequiresCategories === true, the passed transaction contains a category that is enabled in the policy, and the onyx data had a missingCategory violation, we’ll clear it from updatedTransactionViolations using _.reject(transactionViolations, e => e.name === 'missingCategory')
     • If policyRequiresTag === true, the transactionViolations array doesn't have one with name: tagOutOfPolicy and the passed transaction contains a tag, and the policyTags collection doesn’t have that tag with its enabled value set to true, then it’ll add the object {‘name’:’categoryOutOfPolicy’, ‘type’: ‘violation’} to updatedTransactionViolations
     • If policyRequiresTag === true, the passed transaction contains a tag that is enabled in the policy, and the onyx data had a missingTag violation, we’ll clear it from updatedTransactionViolations using `_.reject(transactionViolations, e => e.name === 'missingTag')
  3. Add const formFields = {merchant: 'merchant', amount: 'amount', category: 'category', ...}
  4. Add a const violationField = {missingCategory: 'category', categoryOutOfPolicy: 'category', ...} with a mapping of violation names to the name of the field where the violation is shown (more in code snippet below
  5. Feel free to add dummy translations to the EN and ES files for now for the violations

function getViolationForField(transactionViolations, field) {
    const {translate} = useLocalize();
    const fieldViolations = _.filter(transactionViolations, (violation) => violations[violation.name] === field);
    return fieldViolations.map(violation => translate(violation.name));
}

// Example object returned by getViolationsOnyxData
{
    onyxMethod: Onyx.METHOD.SET, 
    key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`
    value: updatedTransactionViolations
}

const formFields = {
    merchant: 'merchant',
    amount: 'amount',
    category: 'category', 
    date: 'date',
    tag: 'tag',
    comment: 'comment',
    billable: 'billable',
    receipt: 'receipt',
    tax: 'tax',
};

const violationFields = {
    perDayLimit: formFields.amount,
    maxAge: formFields.date,
    overLimit: formFields.amount,
    overLimitAttendee: formFields.amount,
    overCategoryLimit: formFields.amount,
    receiptRequired: formFields.receipt,
    missingCategory: formFields.category,
    categoryOutOfPolicy: formFields.category,
    missingTag: formFields.tag,
    tagOutOfPolicy: formFields.tag,
    missingComment: formFields.comment,
    taxRequired: formFields.tax,
    taxOutOfPolicy: formFields.tax,
    billableExpense: formFields.billable,
};

---

Example

getViolationForField([{name: 'overLimit', type: 'violation'}], 'amount') 
// should return => translate('overLimit')

getViolationForField([{name: 'maxAge', type: 'violation'}], 'amount') 
// should return => ''

getViolationForField([{name: 'maxAge', type: 'violation'}], 'date') 
// should return => translate('maxAge')

getViolationsOnyxData(
    transaction = {category: '', ...}, 
    transactionViolations = [], 
    policyRequiresTags = false,
    policyTags = [],
    policyRequiresCategories = true, 
    policyCategories = [{Entertainment: {name: 'Entertainment', enabled: true}}]
)
// should return 
{
    onyxMethod: Onyx.METHOD.SET, 
    key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
    value: [{name: 'missingCategory', type: 'violation'}],
}


getViolationsOnyxData(
    transaction = {tag: '', ...}, 
    transactionViolations = [], 
    policyRequiresTags = true,
    policyTags = [{Entertainment: {name: 'Engineering', enabled: true}}],
    policyRequiresCategories = true, 
    policyCategories = []
)
// should return
{
    onyxMethod: Onyx.METHOD.SET, 
    key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
    value: [{name: 'missingTag', type: 'violation'}],
}

[cead22]

@lindboe @cdanwards can you comment here so I can assign you please?

[cdanwards]

Commenting for assignment

[melvin-bot]

@cdanwards Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cdanwards]

Beginning work on this

[trevor-coleman]

Hi @cead22 I'm from Infinite Red and I've been assigned this issue from @cdanwards -- I had a quick question about approach.

tl;dr Are you ok with me using a more idiomatic typescript approach (string literal unions/Record types) to defining the possible values for violation names and fields?

The spec says to:

Add const formFields = {merchant: 'merchant', amount: 'amount', category: 'category', ...}

and

Add const violationField = {missingCategory: 'category', categoryOutOfPolicy: 'category', ...} with a mapping of violation names to the name of the field where the violation is shown (more in code snippet below

As I read it, the goal of these objects is to:

1. have the list of possible values discoverable and easy to maintain
2. enable intellisense/autocomplete for allowed values
3. throw an error when an invalid value is used

In that case are you ok with me using string-literal unions and Record types to accomplish this?

It's a more idiomatic typescript approach that will have the same result, and provide stronger type safety.

For instance:

First we declare the string-literal unions like this:

/**
 * Names of Fields where violations can occur
 */
type ViolationField =
    | 'merchant'
    | 'amount'
    | 'category'
    | 'date'
    | 'tag'
    | 'comment'
    | 'billable'
    | 'receipt'
    | 'tax';
    
 /**
 * Names of the various Transaction Violation types
 */
type ViolationName =
    | "perDayLimit"
    | "maxAge"
    | "overLimit"
    | "overLimitAttendee"
    | "overCategoryLimit"
    | "receiptRequired"
    | "missingCategory"
    | "categoryOutOfPolicy"
    | "missingTag"
    | "tagOutOfPolicy"
    | "missingComment"
    | "taxRequired"
    | "taxOutOfPolicy"
    | "billableExpense";

Then we use them to define the type of the map:

/**
 * Map from Violation Names to the field where that violation can occur
 */
const violationFields: Record<ViolationName, ViolationField> = {
    perDayLimit: 'amount',
    maxAge: 'date',
   // ...
};

When defined this way, trying to add an unknown violation name will cause a red squiggle:

and the same if you try to assign an invalid field to a valid ViolationName

Even in Javascript files, the typechecker will still throw a warning:

and provide autocomplete constrained to possible values:

[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>

[cead22]

@trevor-coleman Hi Trevor, nice to meet you and thanks for taking this over and helping with this. What you're proposing sounds good to me, but let's see what @hayata-suenaga thinks since he's more well versed on TS types than me

[hayata-suenaga]

I agree with @trevor-coleman. Your approach to use an union of string literals seems much simpler approach. Let's go with that plan 👍

[trevor-coleman]

Perfect -- It's pretty much done, I've just gotta run the checklist and I'll have a PR for you monday. 🚀

[melvin-bot]

@cdanwards Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trevor-coleman]

Quick question: Should getViolationForField return a single string, or an array of strings?

These examples show the function returning a single string:

getViolationForField([{name: 'overLimit', type: 'violation'}], 'amount') 
// should return => translate('overLimit')

getViolationForField([{name: 'maxAge', type: 'violation'}], 'amount') 
// should return => ''

getViolationForField([{name: 'maxAge', type: 'violation'}], 'date') 
// should return => translate('maxAge')

but then the provided function returns an array of strings

function getViolationForField(transactionViolations, field) {
    const {translate} = useLocalize();
    const fieldViolations = _.filter(transactionViolations, (violation) => violations[violation.name] === field);
    return fieldViolations.map(violation => translate(violation.name));
}

// Examples:

getViolationForField([{name: 'overLimit', type: 'violation'}], 'amount') 
// would return => [translate('overLimit')]

getViolationForField([{name: 'maxAge', type: 'violation'}], 'amount') 
// should return => ['']

// etc.

I'm guessing from the singluar name that you want to return only a single value, but wanted to check before getting too far along with the change. Are there any cases where a given field would have multiple violations?

My fix is to just return the first element of the array -- this will be undefined if nothing matches the filter, which will allow the result to be checked as a boolean.

getViolationForField(transactionViolations: TransactionViolation[], field: ViolationField, translate: (key: string) => string): string {
        const fieldViolations = transactionViolations.filter((violation) => possibleViolationsByField[field]?.includes(violation.name)).map((violation) => translate(violation.name));
        return fieldViolations[0];
    },

[cead22]

Great question, and yes the reason for this is at first I thought we'd only have 1 violation max per field, but we're going to put more than 1 under the receipt, so to answer your question: it should return an array of strings, and if there's aren't any violations, an empty array.

In the code that uses it, we'd check if there's a non-empty array. and if so display whatever is in the array

[trevor-coleman]

Ah, ok -- then should we also update the code on the display side to display multiple violations per field? And if so, how do we handle that.

We could either create multiple 'rows' each with one violation, or do something simpler like just violations.join(', ') to list them separated by commas.

Edit: Going to ask this question on the display ticket (see this comment)

[cead22]

I replied in the other issue. Feel free to bring up these questions on slack and tag me, since I imagine I'll be quicker to respond that way

[cead22]

@cdanwards is working on this

[melvin-bot]

@cdanwards Eep! 4 days overdue now. Issues have feelings too...

[trevor-coleman]

Opened a PR here: #31656

[melvin-bot]

Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[trevor-coleman]

@cead22 -- should this be reassigned to me?

[situchan]

@trevor-coleman nvm. @marcaaron is internal engineer and was assigned just to review PR

[situchan]

@cead22 can you please reopen and add New feature label for payment?

[melvin-bot]

Triggered auto assignment to @johncschuster (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[cead22]

Bump @johncschuster, can you handle payment for the review please?

[johncschuster]

Sure thing! I'm catching up on the PR now

[johncschuster]

@situchan I've invited you to the job, here. Can you accept that and let me know when you have so I can issue payment? Thank you!

[cead22]

@johncschuster can we close this one?

[situchan]

Not paid yet. I accepted offer on Feb 13

[johncschuster]

Sorry for the delay! This has now been paid! Thank you!