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 https://github.com/Expensify/App/pull/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

$ https://github.com/Expensify/App/issues/31093 PROPOSAL:

Tests

• [x] 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

• [x] Verify that no errors appear in the JS console

Same as tests

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] 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)
  • [x] 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).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop
• [x] 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)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] 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)
  • [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] 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:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] 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.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] 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)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If any new file was added I verified that:
  • [x] 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
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• [x] 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.
• [x] 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)
• [x] 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.
• [x] 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.
• [x] If the PR modifies the form input styles:
  • [x] I verified that all the inputs inside a form are aligned with each other.
  • [x] I added Design label so the design team can review the changes.
• [x] 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.
• [x] 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[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]

@parasharrajat Main is merged 👍

[parasharrajat]

So I am seeing the following.

1. Updating category does not remove violation as this function is not updated yet.
2. No violation is shown for tag and category out of policy even after refreshing the page. The tag or category also does not hide from the list in the UI. I think I am missing something. Data is reflected correctly to show enabled false for those.

[parasharrajat]

Ok, so I noticed that I have to reselect either category or tag for outofPolicy violation to show up.

Here is a video.

https://github.com/Expensify/App/assets/24370807/03743bb2-843d-4531-83f1-213c34b59e84

[lindboe]

@parasharrajat Update pushed, the newer commands should be included now.

I noticed that I have to reselect either category or tag for outofPolicy violation to show up.

So the outOfPolicy violation showing up in Onyx is actually the responsibility of the backend, at this point. There's no code to respond to the policy configuration change being pushed, so we can only make client-side changes when the request itself is edited. (relevant slack thread here: https://expensify.slack.com/archives/C01GTK53T8Q/p1706134329225599?thread_ts=1706115595.439199&cid=C01GTK53T8Q). This means the client right now will only be responsible for removing the outOfPolicy violation in the optimistic data after a relevant update.

If you continue to see this issue, let us know so we can make Carlos aware of it, but be aware that there could potentially be some lag there.

[parasharrajat]

I see two issues now.

1. Category is set automatically to some value even if not selected while creating the request. But missingCategory violation is still visible on the data.

2. Tag and Category out of policy violations are not updating for me. I believe that somehow data is not updating on my account. It might have to do something with permissions.

https://github.com/Expensify/App/assets/24370807/be61b5de-90b2-46ef-af7e-75a5da35e9d0

[parasharrajat]

FYI, I can not access this https://expensify.slack.com/archives/C05EQBQG6E5/p1695182901255769 chat.

[lindboe]

@parasharrajat While I'm able to reproduce the "auto-set category" bug, I'm not able to reproduce the violation showing up for missing category anyways. I'm continuing to try. FWIW, I can trigger the bug by creating a request with a category, and then a request without a category -- the request without a category will receive the category from the last request I made. Seems like a bug with transaction drafts, going to check if it also exists on main.

[lindboe]

@parasharrajat Looks like the auto-categorization is a server-side feature, so that's another issue this PR cannot address. https://expensify.slack.com/archives/C01GTK53T8Q/p1706221657329989, but I pinged you on the thread as well.

For issue 2, I would check in your Onyx database to check if policy updates are propagating to your client, (policyCategories_POLICYID/policyTags_POLICYID), but beyond that, I think we'll need @cead22 to help resolving with issues with policy data syncing.

[parasharrajat]

Anyway, I think both things are unrelated to this PR and thus we can move forward here. Meanwhile, we can track the 2nd issue separately.

I will complete the testing while ignoring the 2nd issue.

[parasharrajat]

BUG: Looks like selecting the category does not remove the violation.

https://github.com/Expensify/App/assets/24370807/430436c5-0285-476e-aabd-8dd83177686d

[cead22]

@parasharrajat can you refresh and check if the violation goes away? I believe the reason it's not updating in there is because we're not sending violation data from UpdateMoneyRequestAmountAndCurrency which I'm currently working on fixing

[parasharrajat]

But we should be calculating the violations optimistically. So In theory, if I make this change offline then this violation should vanish.

Also, I update the category which is done via UpdateRequestCategory command.

[lindboe]

BUG: Looks like selecting the category does not remove the violation.

Okay yeah, I can reproduce that bug. This didn't happen before, so something new is wrong, looking into it

[lindboe]

@parasharrajat so sorry, it looks like I forgot to commit the update to the new commands where we pass in policy parameters in EditRequestPage. Pushed that update, and it should work as expected for you now:

https://github.com/Expensify/App/assets/5252268/e5ac0645-5549-4a4f-83ef-c38390c53849

[parasharrajat]

Screenshots

:black_square_button: iOS / native

https://github.com/Expensify/App/assets/24370807/e715ce5f-2fed-48a3-be08-5e23168851b5

:black_square_button: iOS / Safari

https://github.com/Expensify/App/assets/24370807/92c394d5-46fe-4003-bf2b-3abcc67288b1

:black_square_button: MacOS / Desktop

https://github.com/Expensify/App/assets/24370807/136ff190-471f-4215-b7bc-14e63815d40f

https://github.com/Expensify/App/assets/24370807/e765ab79-c5b4-4ae9-9b7c-6efd84a50640

:black_square_button: MacOS / Chrome

https://github.com/Expensify/App/assets/24370807/3fe78f0b-cad5-4775-adc0-2b86c36600b3

https://github.com/Expensify/App/assets/24370807/5dc536be-f13a-49de-b6a1-023f6f7a397e

:black_square_button: Android / Chrome

https://github.com/Expensify/App/assets/24370807/88a2250e-6f14-4e27-8afe-2386c89ddc2d

:black_square_button: Android / native

https://github.com/Expensify/App/assets/24370807/d266e23e-f1c3-4ef0-bb33-8f878c2935a0

[cead22]

@parasharrajat the videos don't show any of the tests, can you please update them to show the following?

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.

[cead22]

• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack

I also noticed console warnings showing in the videos, can you report them on slack and link here please?

[parasharrajat]

This PR does not make any visible changes; that is scoped to other tickets.

I think there are no visible changes here. Only Onyx data changes as per OP. @lindboe Can you please confirm?

[cead22]

The onyx data changes reflect in the UI in the form of violations

[parasharrajat]

Yeah, I got that part but I think this PR only makes Onyx changes. cc @lindboe

[lindboe]

@parasharrajat At the time this PR was created, there were a number of issues with violations reliably showing up in the UI that were addressed in other PRs, so I tried to write tests to show this part of the feature still works. At this point @cead22 knows more than me about where those stand.

I see there's a conflict, I'll merge and re-test.

[cead22]

Violations are showing as you can see in some of the screenshots in the PR description. We should test that they're all showing on the right fields when applicable, and that they don't show when it doesn't apply, just like we're checking whether the onyx data is there or not

[cead22]

@parasharrajat I added [email protected] to the violation beta, but let me know if you want me to add any other test accounts

[lindboe]

@parasharrajat @cead22 I think this should be good to go now, conflict-wise

[parasharrajat]

Ok. let me retest it then.

[parasharrajat]

@lindboe We made major changes to the UI and around navigation. Could you please merge main so that I can retest it?

Also, there are a couple of requested changes above. Please review them.