[Violations] [MEDIUM] IOU - The disabled category message does not disappear immediately after the change is made

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.25-1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Preconditions:
In OldDot under admin, create a Collect group policy, enable Categories, add 3 Categories, add an employee to the policy.

1. Open https://staging.new.expensify.com/
2. Log in to the employee's account
3. Tap on the green plus button (FAB)
4. Select Request money
5. Select any currency and amount
6. Click next
7. Choose a WS room
8. Click on "Show more"
9. In the "Category" section, select any category
10. Complete the IOU
11. Open https://staging.expensify.com/
12. Log in to the admin account
13. Navigate to the Collect group policy settings
14. In the category tab, disable the employee's selected category from step 9
15. In the employee's account in ND, open the IOU details.
16. Click on the category field
17. Select an enabled category

Expected Result:

The message about a disabled category should immediately disappear after selecting an enabled category

Actual Result:

The disabled category message does not disappear immediately after selecting an enabled category

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6343627_1705355438570.Recording__1091.mp4

View all open jobs on GitHub

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[mountiny]

Not a blocker, new feature

[paultsimura]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The categoryOutOfPolicy violation message doesn't disappear on the category change.

What is the root cause of that problem?

We do not optimistically remove the category violation when updating the Money Request's category.

What changes do you think we should make in order to solve the problem?

When editing a money request, if the changes contain category, along with updating the recently used category, we should remove the category violation from this transaction's Onyx entry: ${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}

It can be done like this (added a concern at the end):

const transactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`];
        optimisticData.push({
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
            value: transactionViolations.filter((violation) => violation.name === 'categoryOutOfPolicy'),
        });

App/src/libs/actions/IOU.js

Lines 2342 to 2352 in c13c497

	// Update recently used categories if the category is changed
	if (_.has(transactionChanges, 'category')) {
	const optimisticPolicyRecentlyUsedCategories = Policy.buildOptimisticPolicyRecentlyUsedCategories(iouReport.policyID, transactionChanges.category);
	if (!_.isEmpty(optimisticPolicyRecentlyUsedCategories)) {
	optimisticData.push({
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES}${iouReport.policyID}`,
	value: optimisticPolicyRecentlyUsedCategories,
	});
	}
	}

Also, we need to introduce a BE change to update the TRANSACTION_VIOLATIONS Onyx key on the EditMoneyRequest API response.

Concern: IMO, it's quite risky to operate an Onyx entry that holds an array, because it doesn't support the MERGE operation properly. It's worth thinking of using an Object for them in Onyx, this will enable better control over the specific violations.
Asked to clarify here: https://expensify.slack.com/archives/C01GTK53T8Q/p1705400084950499

What alternative solutions did you explore? (Optional)

[mountiny]

@cead22 is making lots of changes to violations like this so we should be ware we dont waste energy and money on something that will be changed by his work

[cead22]

Concern: IMO, it's quite risky to operate an Onyx entry that holds an array, because it doesn't support the MERGE operation properly. It's worth thinking of using an Object for them in Onyx, this will enable better control over the specific violations.
Asked to clarify here: https://expensify.slack.com/archives/C01GTK53T8Q/p1705400084950499

Thanks for raising this in expensify-open-source. For context, this was discussed quite a bit and we landed on using the array because this array is primarily set from the backend, and the violations array is never too big that it should cause performance issues. That said, we do set some violations from the front end in ViolationUtils.getViolationsOnyxData

I'm going to move this issue to the violations project and take it internally with Infinite Red

cc @lindboe @trevor-coleman please comment for assignment

[cead22]

I believe this is the same issue as https://github.com/Expensify/Expensify/issues/361948, but I'll make sure and get them resolved

[lindboe]

@cead22 this should be covered by #31093, right?

[cead22]

For categories and tags, yes, but for amount for instance, no. We have some backend commands updating money requests that should be sending down the updated violation data, and they currently aren't

[luacmartins]

Just FYI that we have this potentially dupe issue - #34945

[cead22]

I'll look into this today

[cead22]

No updates yet

[cead22]

No updates

[cead22]

No update on this one, though I agree with it being polish

[cead22]

I'm not sure if this is reproducible anymore (and the site is kinda shaky right now to test), but I think this is mostly something we'll need to take into account when moving violations to auth.

Specifically, we need to make sure that when changes are made to policies -- and other things that can affect violations like domain security groups, that we somehow update the affected transactions and their corresponding violations in real-time in all clients

[JmillsExpensify]

Noted, should we move this issue out of #wave-control then? Or keep it here, pending the completion of 1:1:1 for violations in #newdot-quality?

[cead22]

Up to you -- you probably know better. I'm good with either

[cead22]

In the doc to implement violations in auth we're discussing whether re-fetching violations, which will be necessary in some cases like this (as well as others) so violations update in real-time.

If we agreed on leaving that out of scope for that project, I'll close this, and we can solve these issues as they come up when user start experiencing them

[cead22]

We have no pushback on leaving this out of the scope of that doc, so closing this