Fix client side tag violations and Improve function signatures

[cead22]

Details

• This started as a follow up to this PR Add violation message to MoneyRequestPreview #33969 to address comments we didn't get to address before the end of last week when Infinite Red finished working with us on violations. Namely, updating the method signatures of TransactionUtils.hasViolation, TransactionUtils.getTransactionViolations, and ReportUtils.isSettled
• I was getting Malformed currency code when testing (see slack thread in expensify-open-source), so I added a fix to this PR
• I also noticed the missing tag violation wasn't working and dug into it, and added a fix as well (I believe it was broken here Allow for multiple levels of tags on workspace #34983)

Fixed Issues

$ #36889

• Followup to Add violation message to MoneyRequestPreview #33969
• PROPOSAL: N/A

Tests

Setup

• Make sure emails sent to the testing email addresses go to an inbox you can access to validate your account(s)
• Add the logins you’re using to test to the violation beta
• Create a new collect policy on OldDot and;
  • Set the approval mode to “submit and approve”
  • Invite an additional user to the policy to play the member role in these testing flows
  • From the policy editor, run the script below from the workspace editor in old dot to enable the policy for workspace creation for a paid plan.
  • From the policy editor > Categories, switch on “People must categorize expenses”, and add a some categories
  • From the policy editor > Tags, switch on “People must tag expenses”, and add some tags

p = Policy.getCurrent();
p.policy.isPolicyExpenseChatEnabled = "true";
p.save();

Test

• Tap big plus sign > request money
• Select Manual tab (from Manual | Scan | Distance)
• Enter amount and tap next
• Tap the workspace from the pre-testing steps (tap the row, not the split button in the row)
• Tap the show more button if it’s there
  • Verify there’s a Required label on the Category row
  • Verify there’s a Required label on the tag row
• Tap the Request $X.XX button
  • Verify the report preview in the workspace chat has an RBR
• Tap the report preview
  • Verify the money request preview in the iou report has an RBR
• Tap the money request preview
  • Verify you see the Missing Category red text under the category row
• Tap the category row
• Tap on a category
  • Verify you no longer see the Missing Category red text under the category row
• Tap the Tag row
• Tap on a tag
  • Verify you no longer see the Missing Tag red text under the category row
• Tap on the IOU chat report link at the top
  • Verify the money request preview in the iou report no longer has an RBR
• Tap on the policy expense chat report link at the top
  • Verify the report preview in the policy expense chat report no longer has an RBR

---

• Verify that no errors appear in the JS console

Offline tests

Same as test

QA Steps

Same as tests

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

androidnative.mp4

Android: mWeb Chrome

Note I'm still trying to figure out how to test this, and will rely on the reviewer checklist for this

iOS: Native

iosnative.mp4

iOS: mWeb Safari

RPReplay_Final1708627316.MP4

MacOS: Chrome / Safari

desktopbrowser.mp4

MacOS: Desktop

desktop.mp4

[allroundexperts]

@cead22 Am I supposed to review this? This one seems to be a WIP.

[cead22]

@allroundexperts no, or not yet at least, I assigned you and Yuwen as reviewers cause you had context on the deploy blocker. I could've just cc'ed you, but if you want to be the C+ to review this PR, that works

[melvin-bot]

@rlinoz 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]

@cead22

Tap the Request $X.XX button
Verify the report preview in the workspace chat has an RBR

We shouldn't let the user proceed with creating the request if the required fields are missing

[cead22]

We shouldn't let the user proceed with creating the request if the required fields are missing

No that's not true, users should be able to create money requests without entering category and tag even if they're required. And for merchant, we already prevent it

[s77rt]

@cead22 So they are required in the term that they cannot be submitted?

[cead22]

They can also be submitted, approved, and paid. They're used to highlight violations to users submitting the requests, and those approving them as well

[cead22]

@rlinoz @s77rt this is ready for review. Please note that I remove code for violations for multi-level tags to avoid bloating the scope of this PR, and I'll follow up with another PR to add violation support for multi tags

[rlinoz]

CategoryPicker is broken for me
Uncaught TypeError: Cannot read properties of undefined (reading 'enabled'), but it is also happening on main.

I will try to understand what is happening tomorrow.

[cead22]

@rlinoz I'm pretty sure this is known and happening for the tags as well, which is being fixed here (for tags, not categories) https://github.com/Expensify/App/pull/36994/files

[s77rt]

For multi level tags still seeing RBR in the money request preview - Do you want to handle this in the follow up as well?

Screen.Recording.2024-02-23.at.5.42.58.AM.mov

[s77rt]

For single tag, the tag selection works as expected but cannot test category due to other bugs

[s77rt]

@cead22 Can you please merge main and resolve the conflicts?

[cead22]

@s77rt merged main

For multi level tags still seeing RBR in the money request preview - Do you want to handle this in the follow up as well?

Yes

[s77rt]

Revert?

[cead22]

@s77rt I committed your suggestions

[s77rt]

@cead22 Can you also check #36821 (comment)

[cead22]

Yeah that change was intentional. I created an issue to follow up to implement violations for multi level tags #37117

[s77rt]

That removed condition is for empty tags list, does not seem related to the multi tags.

[cead22]

Is there a specific case you're worried about not working? I tested again and things seem to work fine. There's a couple of non-obvious things at play here

1. The removed condition is checking for empty tag lists on a workspace, and I don't think that can ever happen. You may have a workspace with a single tag list with no tags enabled, but there will still be a tag list
2. Even if the workspace had no tag lists, the workspace's "requires tag" property/logic should return false. Similarly for the case of a single tag lists that has tags in it, but where all the tags are disabled, the workspace "requires tag" property/logic should return false

So for the case where there is only 1 tag list, with no tags enabled, and the money request has a tag selected (that is disabled), we don't go into the if (policyRequiresTags) {

I say "requires tag" property/logic should return false because that may not be the case on the new dot client, but it is the case in our php logic, which looks like the code below

    public function requiresTag()
    {
...
        if (!$this->hasEnabledTags()) {
            return false;
        }
...
    public function hasEnabledTags()
    {
...
        // If the policy has independent tags, ensure we check each tag list to see if there are any required tags
        if ($this->hasIndependentTags()) {
            return ArrayUtils::some($this->getTagListsArray(), function (array $tagList, int $i): bool {
                $tagCollection = $this->getIndependentTags($i);
                return $tagCollection->hasEnabledTags();
            });
        }

[s77rt]

@cead22 Thank you! That was helpful!

[melvin-bot]

@neil-marcellini 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]

[cead22]

@neil-marcellini not sure why you were added here, but a review from either you or @rlinoz works for me :)

[rlinoz]

Code looks good and tests well, thanks!

[cead22]

Thank you! Scratch my previous request @neil-marcellini

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.44-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅