Client-side violations for money request updates

[lindboe]

Details

Adds optimistic client-side updates for money requests for missing tags, categories, or disabled tags/categories. This PR handles edits, while #32528 handled creation of money requests. Requires setting up a policy in old dot and configuring it to have a workspace (see testing instructions). This PR does not make any visible changes; that is scoped to other tickets.

Fixed Issues

$ #31093
PROPOSAL:

Tests

• Verify that no errors appear in the JS console

Setup:

1. Follow the instructions here to set up a policy with a workspace in New Dot.
2. Within the policy in Old Dot, navigate Settings > Categories and verify that People must categorize expenses is enabled, and make sure there's at least two categories on the policy
3. Within the policy in Old Dot, navigate Settings > Tags and verify that People must tag expenses is enabled, and make sure there's at least two tags enabled on the policy

Instructions for all tests

Expectations for violations, generally:

1. Violations should be visible when viewing the request on each menu item with text and a RBR indicator. Here, we're generally concerned the following violations that we calculate client-side: missing category, missing tag, category out of policy, and tag out of policy. Other visible violations should not be affected by this PR.

Testing the violations were stored correctly (web only):

1. Navigate to the ONYXDB -> keyvaluepairs within your browser storage tab
2. Identify the transaction that was created with the dollar amount you specified, should look like transactions_someID. If you open chrome dev tools before making the money request, you can inspect the response which should have the transactionID
3. Verify there's a transaction violations object with the corresponding ID like transactionViolations_SameIDasTransaction, with the array content specified by the test

Test no client-side violations

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, category, and tag. Ensure your request amount is not above the maximum set by the policy, and not above the amount the policy requires for a receipt.
2. (Web) Once the request is created, check your browser storage tab. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation.
3. Edit the request's date and save. There should be no visible errors on the request or the LHN.
4. (Web) inspect the transaction violations key again. The violations list should still only contain a cashExpenseWithNoReceipt violation.

Test missing tag

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and category, but DO NOT set tag. Save the request.
2. (Web) inspect the transaction violations object. You should see a matching transactionViolations_ID key, with an array containing a missingTag violation, and possibly also a cashExpenseWithNoReceipt violation.
3. Edit the request to add a tag. There should be no visible errors on the request or the LHN.
4. 2. (Web) inspect the transaction violations key again. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation.

Test missing category

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag, but DO NOT set category. Save the request.
2. (Web) inspect the transaction violations object. You should see a matching transactionViolations_ID key, with an array containing a missingCategory violation, and possibly also a cashExpenseWithNoReceipt violation.
3. Edit the request to add a category. There should be no visible errors on the request or the LHN.
4. 2. (Web) inspect the transaction violations key again. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation, or no violations.

Test tag out of policy

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
2. On old dot, disable the tag you used. Refresh new dot, and ensure a tagOutOfPolicy violation is shown in the transaction violations list.
3. Edit the request to use a tag that is enabled in the policy, and ensure that tagOutOfPolicy is no longer in the transaction violations list.

Test category out of policy

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
2. On old dot, disable the category you used. Refresh new dot, and ensure a categoryOutOfPolicy violation is shown in the transaction violations list.
3. Edit the request to use a category that is enabled in the policy, and ensure that categoryOutOfPolicy is no longer in the transaction violations list.

Offline tests

Repeat the tests outlined above while completely offline, except for "tag/category out of policy", because you can't change policy while offline. Different steps for those tests are provided below.

Then, in addition, after each test, go online and verify the transactionViolations key remains the same, with the potential addition of a cashExpenseWithNoReceipt violation, or no violations.

Test tag out of policy (offline)

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
2. On old dot, disable the tag you used. Refresh new dot, and ensure a tagOutOfPolicy violation is shown in the transaction violations list.
3. On new dot, go offline. Select a tag that has remained enabled on your policy on the request instead. Save the request, and ensure the transaction violations list no longer has a tagOutOfPolicy violation.
4. Go online, and ensure that the transaction violations list does not change.

Test category out of policy (offline)

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
2. On old dot, disable the category you used. Refresh new dot, and ensure a categoryOutOfPolicy violation is shown in the transaction violations list.
3. On new dot, go offline. Select a category that has remained enabled on your policy on the request instead. Save the request, and ensure the transaction violations list no longer has a categoryOutOfPolicy violation.
4. Go online, and ensure that the transaction violations list does not change.

Test failure to edit request (web only, requires Chrome)

1. Create a request and request money from the policy you created in setup. Set an amount, merchant, and category, and tag. Save the request.
2. Go to old dot and disable the category you used.
3. Refresh new dot.
4. Inspect the corresponding transactionViolations_ key to ensure there is a categoryOutOfPolicy violation there.
5. Now, cause the request to the backend to fail:
6. With the Network tab open in Chrome devtools, edit a DIFFERENT request's category. Look for a request entry matching api?command=EditMoneyRequest (it is possible that instead it might be a more specific command in the future, like UpdateMoneyRequestTag). Right-click and select "Block request URL". This will cause subsequent calls to this URL to fail.
7. Go offline and edit the request to use an enabled category. Save the request, and inspect the transaction violations key. The categoryOutOfPolicy violation should be gone.
8. Go online and allow the request to be sent and fail.
9. Wait a little bit for the request to fail after several retries (you should see an error in the transaction thread), and verify that the transaction violations list in Onyx is reset to what it was before you fixed the request (i.e., it contains a categoryOutOfPolicy violation).
10. Unblock the endpoint you blocked before, and again update the request to use an enabled category. The transactionViolations list should be updated to remove the categoryOutOfPolicy violation.

QA Steps

• Verify that no errors appear in the JS console

Same as tests

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

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Tag out of policy tests

Category out of policy tests

Failure data tests

MacOS: Desktop

[melvin-bot]

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

[lindboe]

@parasharrajat Hi, are you going to be able to review this PR soon?

[parasharrajat]

@lindboe Please merge main. Reviewing it now.

[lindboe]

@parasharrajat Looks like a significant change from upstream -- the editMoneyRequest command was removed entirely in favor of the more detailed, specific commands like updateMoneyRequestAmountAndCurrency. Easy to fix conflict-wise, but this does mean it has changed what command is used since it was last tested, so just FYI.

Fixing conflicts now...

[lindboe]

Okay, so that this can move forward, I'm aiming for minimal changes:

1. I un-deleted EditMoneyRequest since there's a deletion test now using it
2. I added a @ts-expect-error to the type issue in MoneyRequestView with the conflict between the component's typing as PolicyTags (not correct type for what the data returns and the ViolationsUtils function's use of PolicyTagList). I'll make a more clear report of this and share it in open-source so someone can work on fixing the policy tags typing and utils.
3. I changed the policy* defaultProps to null where TypeScript was complaining because OnyxEntry<Policy*> = Policy* | null, but defaults props was returning an empty object, and this difference was leading to some very confusing type conflicts. Based on my conversation here, null is the expected "empty" data type once components are typescript, so may as well change it now instead of trying to handle both everywhere.

Going to run through some test cases and make sure it's all good now.

[lindboe]

Okay @parasharrajat this should be good to go again, except the perf-tests that appear to be failing on main as well. Should have a message for open-source about the broken policy tags types in a little while.

[lindboe]

Asked about policyTags types here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707263531033829

[cead22]

1. @cead22 do you have input on a preferred approach here?

Thanks, I was going to suggest what you already did (to ignore the type errors for now, since the more we add to this PR the harder it'll get to get it merged)

[lindboe]

Another day, another refactor update: the command to update description now lives in its own component. Updated.

@cead22 while testing this update, I noticed an assumption in earlier decisions doesn't seem to be true: in a few places, we're checking if policy?.id exists, because "if it's a P2P request, there won't be a policy, and so we should skip trying to calculate transaction violations". That may have been true at one point, but isn't now, I do see a policy when I make a P2P request:

{
    "isFromFullPolicy": false,
    "id": "B88059F8975D1B23",
    "name": "Lizzi's Expenses",
    "role": "...",
    "type": "personal",
    "owner": "...",
    "ownerAccountID": ...,
    "outputCurrency": "USD",
    "avatar": "",
    "employeeList": [],
    "isPolicyExpenseChatEnabled": false,
    "lastModified": ...,
    "chatReportIDAnnounce": 0,
    "chatReportIDAdmins": 0
}

I'm assuming isFromFullPolicy or type are fields we should be checking against instead?

[lindboe]

Above issue with policy?.id discussed and resolved here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707339619099769.

@parasharrajat tests are passing again. Just to clarify, next steps were for you to test this out again right?

[parasharrajat]

Screenshots

🔲 iOS / native

Screen.Recording.2024-02-09.at.2.51.16.PM.mov

🔲 iOS / Safari

Screen.Recording.2024-02-09.at.2.31.25.PM.mov

🔲 MacOS / Desktop

Screen.Recording.2024-02-09.at.2.25.42.PM.mov

🔲 MacOS / Chrome

Screen.Recording.2024-02-09.at.1.01.29.PM.mov

🔲 Android / Chrome

Screen.Recording.2024-02-09.at.2.34.42.PM.mov

🔲 Android / native

Screen.Recording.2024-02-09.at.2.50.14.PM.mov

[parasharrajat]

LGTM. There are couple of prop warnings but I think those will be removed when these components are migrated to TS.

I will also report some to Slack in sometime.

[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.40-0 🚀

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

[mountiny]

Noting here that QA did not really validate this PR as the QA steps were not clear enough for them it seems like. I believe its fine not to block deploy on this and we will internally test these and report any issues as we find them

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀

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

[allgandalf]

This incomplete condition caused #43767.

We should have also checked whether transactionChanges are made either for category or tag and not execute this check for every change in the transaction