[HOLD for payment 2024-01-17] Implement Client-side Violations when creating money requests

[cead22]

• Design Doc
• The following should only apply to users in the violations beta
• Let's connect MoneyRequestConfirmPage to the POLICY, POLICY_CATEGORIES, and POLICY_TAGS Onyx collections so we can get
  • requiresTag, requiresCategory, hasMultipleTagLists, and isTaxTrackingEnabled from the policy
  • The policy categories, and
  • The policy tags
• In src/pages/iou/steps/MoneyRequestConfirmPage.js let's pass props.policy.policyID to IOU.requestMoney > from there to getMoneyRequestInformation > and from there to buildOnyxDataForMoneyRequest
• In buildOnyxDataForMoneyRequest
  • We’ll check If policyID is not available, like for the case of P2P, and return early in that case
  • Then call ViolationUtils.getViolationsOnyxData(transaction, [], policyRequiresTags, policyTags, policyRequiresCategories, policyCategories) and push the return value to optimisticData
  • Note: we're not gonna block users from submitting a money request with violations
  • Refer Implement ViolationUtils lib #31083 to see the data returned by getViolationsOnyxData
  • We can set the value of failure data to an object like the snippet below, to remove the violations if the request isn't created
• The idea here is that when requests are created, the transactions calculated on the client by ViolationUtils.getViolationsOnyxData should show right away (eg, missing tag, missing category, category out of policy,...)

{
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
    value: [],
}

[cead22]

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

[cdanwards]

Commenting for assignment

[melvin-bot]

@cdanwards Whoops! This issue is 2 days overdue. Let's get this updated quick!

[cdanwards]

Beginning work on this

[melvin-bot]

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

[cdanwards]

Hey @cead22 I've been working on this and had a question about approach.

Right now, we're connecting POLICY_CATEGORIES and POLICY_TAGS collections in the MoneyRequestPage but not using them, nor passing them down to the subsequent function calls. Will we be using them in another step in this file?

I had a thought that maybe we could update the policy PropTypes to include requiresTag, requiresCategory, hasMultipleTagLists, and isTaxTrackingEnabled and subsequently in IOU.buildOnyxDataForMoneyRequest we could call ReportUtils.getPolicy to get those policy properties.

Similarly we could call ReportUtils.getPolicyTags and ReportUtils.getPolicyCategories in the same manner so that we have access to pass those into the ViolationsUtils.getViolationsOnyxData

Here's the gist of what it would look like:

    if (!policyID) {
        return [optimisticData, successData, failureData];
    }
    const policy = ReportUtils.getPolicy(policyID);
    const policyTags = ReportUtils.getPolicyTags(policyID);
    const policyCategories = ReportUtils.getPolicyCategories(policyID);


    const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], policy.requiresTags, policyTags, policy.requiresCategory, policyCategories);

    if (violationsOnyxData) {
        optimisticData.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
            value: violationsOnyxData,
        });
        failureData.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
            value: [],
        });
    }

Let me know what your thoughts are!

[cead22]

Right now, we're connecting POLICY_CATEGORIES and POLICY_TAGS collections in the MoneyRequestPage

You mean MoneyRequestConfirmPage?

but not using them, nor passing them down to the subsequent function calls. Will we be using them in another step in this file?

You mean as part of this PR you're working on? I don't see the connection to those collection in main. If that's the case, let me know and I'll check the design doc to see if we're going to use them somewhere else, and we just haven't added the code for it yet

had a thought that maybe we could update the policy PropTypes to include requiresTag, requiresCategory...

I don't see the benefit of this, but I'm kind of a react noob, can you perhaps spell it out?

Similarly we could call ReportUtils.getPolicyTags and ReportUtils.getPolicyCategories

I don't see why it's better to have these getters, instead of accessing them like policy.requiresCategory & policy.requiresTag since we already have the policy object with those properties. If we had a policy object and were returning it from ReportUtils.getPolicy(policyID);, it'd make sense to have policy.requiresCategory() & policy.requiresTag() methods on the policy object itself

[melvin-bot]

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

[cdanwards]

@cead22

You mean MoneyRequestConfirmPage?

Yes! Sorry, I thought I was referring to the correct component.

You mean as part of this PR you're working on? I don't see the connection to those collection in main. If that's the case, let me know and I'll check the design doc to see if we're going to use them somewhere else, and we just haven't added the code for it yet

I think this was just me making an assumption. Those properties weren't listed as being passed down through the functions but they are needed in the subsequent ViolationsUtils.getViolationsOnyxData call. And after looking through how those getter methods are working I think the point I was getting to was a bit of a moot point 😅 So we can do as you specified originally.

had a thought that maybe we could update the policy PropTypes to include requiresTag, requiresCategory...

I don't see the benefit of this, but I'm kind of a react noob, can you perhaps spell it out?

While originally I was thinking this would be most useful when using the getter getPolicy (which we won't do), I do still think this would be valuable to document in the type that a policy can have these properties.

[cead22]

While originally I was thinking this would be most useful when using the getter getPolicy (which we won't do), I do still think this would be valuable to document in the type that a policy can have these properties.

Thanks for clarifying, that makes sense to me, so long as it doesn't break anything (and let's not block on it)

[cdanwards]

@cead22 Great! This is mostly finished, just making sure 31656 doesn't have any last changes then I'll get this up to a PR.

[cead22]

Off hold

[cdanwards]

Now unblocked. Working to get this ready for PR!

[lindboe]

Commenting for assignment

[melvin-bot]

This issue has not been updated in over 15 days. @cead22, @lindboe, @cdanwards eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[lindboe]

The PR is done, @cead22 can this be closed?

[cead22]

Yes

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.23-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Adds Client-side Violations when creating money requests #32528

If no regressions arise, payment will be issued on 2024-01-17. 🎊

For reference, here are some details about the assignees on this issue:

• @lindboe does not require payment (Contractor)
• @cdanwards does not require payment (Contractor)