[HOLD for payment 2024-04-15] [Violations][$500] Scan - Category row shows violation briefly after the scan request is created

[kavimuru]

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.50-2 Reproducible in staging?: y Reproducible in production?: y Issue reported by: applause internal team

Action Performed:

Precondition:

• User is an employee of Collect workspace.
• The Collect workspace has toggled on "People must categorize expenses" for Categories on Old Dot.

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Create a scan request without category.
4. Go to request details page right after it is created.

Expected Result:

Category row will not show violation until after smartscanning is complete.

Actual Result:

Category row shows "missing category" violation briefly after the scan request is created.

Workaround:

unknown

Platforms:

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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/43996225/0e631a0e-f9f7-41bd-bfcd-33ec07452ca6

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e13a4e6b1d86de89
• Upwork Job ID: 1770584960942833664
• Last Price Increase: 2024-03-20
• Automatic offers:
• eh2077 | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot[bot]]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[kavimuru]

We think this bug might be related to #wave-collect - RELEASE 1

cc @trjExpensify

[kavimuru]

@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[trjExpensify]

Hm, why wouldn't we show the category violation right away if categories are required on the workspace? 🤔 CC: @JmillsExpensify @cead22

[bfitzexpensify]

Yes, I think the bug here is that the violation disappears, not that it appears in the first place.

[bfitzexpensify]

@trjExpensify do you agree with this comment?

If so, I'll update the OP and send external

[trjExpensify]

Yeah, I agree with it insofar as I don't know why it's disappearing. This is violations related though, so one for #wave-control. :)

[cead22]

It looks like this is happening because we compute missingTag/missingCategory/tagOutOfPolicy/categoryOutOfPolicy/someTagLevelsRequired violations on the client, and when the request to OpenReport comes back, it clears the violations in Onyx which makes them disappear.

The reason violations are cleared is because we don't compute those violations when a transaction is a "partial transaction", meaning it's merchant is set to (none) and its amount is 0.

Let's make this issue external, have the contributors confirm that ^ is the root cause in their proposal, and see if adding a check for partial transactions in getViolationsOnyxData, or something along those lines fixes the issue

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01e13a4e6b1d86de89

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

[allgandalf]

Proposal

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

Scan - Category row shows violation briefly after the scan request is created

What is the root cause of that problem?

We don't add a isScanning check before we fetch the violation: https://github.com/Expensify/App/blob/a979d7d57d399542853a351db894700e528d8c84/src/components/ReportActionItem/MoneyRequestView.tsx#L365-L381

RCA FOR Alternative Proposal

In getViolationsOnyxData, we check for the missing tag condition as: https://github.com/Expensify/App/blob/ba0816bd4fd62dc5e7e013df3ef17b015e6c275a/src/libs/Violations/ViolationsUtils.ts#L128 But here we do not consider the state of the transaction where the transaction is still scanning and is in Pending state, so for the first few seconds we display a missing tag violation but then OpenReport api call returns the data and the violation error is cleared

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

Add a isScanning check above.

const isScanning = hasReceipt && TransactionUtils.isReceiptBeingScanned(transaction);

Then update MenuItemWithTopDescription in MoneyRequestView as follows:

<MenuItemWithTopDescription 
             description={translate('common.category')} 
             title={transactionCategory} 
             interactive={canEdit} 
             shouldShowRightIcon={canEdit} 
             titleStyle={styles.flex1} 
             onPress={() => 
                 Navigation.navigate( 
                     ROUTES.MONEY_REQUEST_STEP_CATEGORY.getRoute(CONST.IOU.ACTION.EDIT, CONST.IOU.TYPE.REQUEST, transaction?.transactionID ?? '', report.reportID), 
                 ) 
             } 
-             brickRoadIndicator={getErrorForField('category') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR :  
-  undefined}  
-             error={getErrorForField('category')} 
+             brickRoadIndicator={!isScanning && (getErrorForField('category') ?  
+ CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined)} 
+    error={isScanning ? undefined : getErrorForField('category')} 
         /> 

What alternative solutions did you explore? (Optional)

OR

We can directly update the getViolationsOnyxData for category check:

https://github.com/Expensify/App/blob/ba0816bd4fd62dc5e7e013df3ef17b015e6c275a/src/libs/Violations/ViolationsUtils.ts#L128

Update the above to:

const hasMissingCategoryViolation = transactionViolations.some((violation) => violation.name === 'missingCategory')
                                                                   && transaction?.status === CONST.TRANSACTION.STATUS.POSTED

[allgandalf]

Proposed based on comments, i feel we should call the getErrorForField only when the scanning has been completed.

c.c. @cead22

[abzokhattab]

Proposal

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

Scan - Category row shows violation disappear after a while

What is the root cause of that problem?

• The issue arises from the canUseViolations function, which determines if the violations should be shown based on the user's permissions https://github.com/Expensify/App/blob/c2006ab88aa4d376dce9d5492ef6ad320afd72fe/src/components/ReportActionItem/MoneyRequestView.tsx#L93
• This function checks if the user's current permissions include violations, as defined in Permissions.ts. https://github.com/Expensify/App/blob/834399d5662ecc407205b2eaf21545c1042404d8/src/libs/Permissions.ts#L25-L27
• However, the discrepancy occurs because the violation betas coming from the server is labeled as serverViolations, not violations, leading to the canUseViolations always returning false. https://github.com/Expensify/App/blob/c8b2b17174e1d817e535abf3767d2ff1efefe4bb/src/CONST.ts#L322

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

• change the violation value here to

        VIOLATIONS: 'serverViolations',

• also we need to add to the following condition so that the category err is not shown if the receipt is scanning && !isReceiptBeingScanned

https://github.com/Expensify/App/blob/e77b01a5dfd63f473620b33c99837a2eb604e0a7/src/libs/Violations/ViolationsUtils.ts#L148

Optionally

for consistency between the money preview and the money view pages, we can also use the canUseViolations condition in the hasViolations condition inside the preview:

    const hasViolations = TransactionUtils.hasViolation(transaction?.transactionID ?? '', transactionViolations) && !!canUseViolations;

or we can add it already inside the hasViolation function

[bernhardoj]

I don't see the violation disappears anymore, but here is the proposal to not add the violation optimistically when it's a partial transaction

Proposal

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

The violation data is added optimistically when it's a partial transaction.

What is the root cause of that problem?

When we create a new money request, we just push the violation data that we found on the request, https://github.com/Expensify/App/blob/47bba81007ae9a4a5dfd6008c626efecb895153d/src/libs/actions/IOU.ts#L797-L806

in this case, the category is missing. We never check whether the transaction is a partial transaction or not.

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

As pointed out by @cead22, we don't want to add it optimistically when it's a partial transaction, so we can add a new check and only add it when it's not a partial transaction.

const isPartialTransaction = TransactionUtils.isPartialMerchant(transaction.merchant) && transaction.amount === 0;
if (violationsOnyxData && !isPartialTransaction) {
    optimisticData.push(violationsOnyxData);
    failureData.push({
        onyxMethod: Onyx.METHOD.SET,
        key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
        value: [],
    });
}

Also, when we are updating the transaction, we set the violation optimistically without checking whether it's still a partial transaction or not. https://github.com/Expensify/App/blob/d8361eee3486ccb5472060f2af7655e96461471e/src/libs/actions/IOU.ts#L1701-L1718

We need to do the same check as above.

What alternative solutions did you explore? (Optional)

Or we can check whether we are creating a scan request or not by checking the receipt.state value is SCANREADY or any other way if available.

[eh2077]

Reviewing proposals.

[eh2077]

The category violation is no longer being shown. Tested in dev and staging.

I think I also agree with @trjExpensify that the violation should be shown as long as it's required by the workspace, regardless of whether it's scanning or not. @cead22 Do you agree with this?

I think @abzokhattab 's proposal makes sense to me.

[trjExpensify]

I think I also agree with @trjExpensify that the violation should be shown as long as it's required by the workspace, regardless of whether it's scanning or not. @cead22 Do you agree with this?

I caught up with Carlos, and you can ignore me =D. We don't show the violation(s) while the transaction is partial because we don't have all the info yet. So it's right to proceed with a solution that doesn't show them in that state.

[allgandalf]

Adding some more RCA for my proposal:

In MoneyRequestView.tsx, we call getError for the first time we load the page: https://github.com/Expensify/App/blob/a979d7d57d399542853a351db894700e528d8c84/src/components/ReportActionItem/MoneyRequestView.tsx#L378-L379

Now in getError: https://github.com/Expensify/App/blob/a979d7d57d399542853a351db894700e528d8c84/src/components/ReportActionItem/MoneyRequestView.tsx#L229-L232

We set the missing category error immediately as we have the category set as required, but when the OpenReport data comes, the error is set to null as the report is still scanning.

So i guess my main solution to not call the getError if the receipt is still scanning is a good fit as we want that no error should pop up in category until the receipt scan is completed :)

[eh2077]

@trjExpensify Thanks for the update! Reviewing proposals.

[eh2077]

@trjExpensify I want to confirm I understand the expected flow. Let's say we have following preconditions as described in this issue

• User is an employee of Collect workspace.
• The Collect workspace has toggled on "People must categorize expenses" for Categories on Old Dot.

For a scan request without category

1. Should we display the red dot when the receipt is scanning? Currently, there's a red dot shown

2. Should we show category violation when scanning is completed? Currently, the category violation is not shown when scanning is failed.

   

3. What's the expected result after manual editing the failed request? Currently, the category violation is not shown after editing the amount or both amount and merchant

   

[cead22]

2. Currently, the category violation is not shown when scanning is failed.

I think this makes sense since in this case the transaction is still partial because we don't have all merchant and amount for it

3. What's the expected result after manual editing the failed request? Currently, the category violation is not shown after editing the amount or both amount and merchant

If/when the transaction isn't partial, we should show the category violation, probably by triggering a call to getViolationsOnyxData

[JmillsExpensify]

Building on that last point from Carlos

Should we display the red dot when the receipt is scanning?

We shouldn't show the red dot on a scanning receipt, as that's a partial transaction.

[bernhardoj]

What's the expected result after manual editing the failed request? Currently, the category violation is not shown after editing the amount or both amount and merchant

I tested this and the violation shows as we already added the violation when editing the transaction. https://github.com/Expensify/App/blob/d8361eee3486ccb5472060f2af7655e96461471e/src/libs/actions/IOU.ts#L1701-L1718

I have updated my proposal to make sure the violation is added when editing only when it's not a partial transaction.

[eh2077]

What's the expected result after manual editing the failed request? Currently, the category violation is not shown after editing the amount or both amount and merchant

I tested this and the violation shows as we already added the violation when editing the transaction.

@bernhardoj Thanks for your comment. I'm not following you about this. I just tested it in staging and didn't see category violation after updating amount and merchant.

[bernhardoj]

That's weird. Is the account has access to the beta and is it a collect policy?

Actually, if both is satisfied, won't you get the violation right away after creating the request? From my testing:

1. Create a new scan request. Violation shows
2. Scan failed. Violation still shows

If I fixed the optimistic violation:

1. Create a new scan request. No violation
2. Scan failed. No violation
3. Edit the amount. Violation shows.

https://github.com/Expensify/App/assets/50919443/5adf8aee-72c2-4ad0-8de3-0b3c058a0817

[eh2077]

@bernhardoj It's a collective policy but my account hasn't been added to the violations beta. I can reproduce what you shared if I overwrite Permissions.canUseAllBetas locally to return true.

[allgandalf]

@eh2077 won't it be okay to hold calling get error if we have a scanning transaction?

[eh2077]

Thank you all for your proposals.

@GandalfGwaihir Your idea to display violation according to isScanning has limitation when receipt scanning is failed, see https://github.com/Expensify/App/issues/38131#issuecomment-2019305656

@abzokhattab I believe you would be able to provide a better proposal if you know that there's a violations beta to access the feature. I also overlooked this before @bernhardoj 's clarification. You can apply it like link or just overwrite it locally.

I think we can go with @bernhardoj 's proposal as their solution follows the expected flow as discussed https://github.com/Expensify/App/issues/38131#issuecomment-2010830030 and https://github.com/Expensify/App/issues/38131#issuecomment-2018233259.

@cead22 All yours.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @cead22 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[allgandalf]

Currently, the category violation is not shown when scanning is failed.

A little clarification please, this was not showing on your side because beta was not enabled for your account right :thinking: