[HOLD for payment 2024-06-13] [$500] Reddot pinned chat appears for approver for submitter's failed scanned receipt

[m-natarajan]

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.43-3
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708448202824579

Action Performed:

1. Create a collect policy in OD as account A
2. Invite user B as employee
3. Log in as user B in NewDot
4. Create a expense report with a invalid receipt to fail the scanning
5. Log in to user A in NewDot

Expected Result:

No RBR pinned chat appear in the LHN for the error message

Actual Result:

Seeing red dot pinned chat in LHN for submitter's workspace chat as approver

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0158b53b3b70327b50
• Upwork Job ID: 1760383519930896384
• Last Price Increase: 2024-04-08
• Automatic offers:
• dukenv0307 | Contributor | 0

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0158b53b3b70327b50

[melvin-bot]

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

[wildan-m]

Proposal

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

What is the root cause of that problem?

Currently errors are not distinguished by owner, so they will show to all users who have access.

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

If the red dot should only be visible to the owner, we can relocate this line:

App/src/libs/SidebarUtils.ts

Line 238 in d33bf6e

result.brickRoadIndicator = hasErrors || hasViolations ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';

below this code:

App/src/libs/SidebarUtils.ts

Lines 359 to 361 in d33bf6e

	result.isIOUReportOwner = ReportUtils.isIOUOwnedByCurrentUser(result as Report);

And adjust the logic to:

    result.isIOUReportOwner = ReportUtils.isIOUOwnedByCurrentUser(result as Report);
    result.brickRoadIndicator = (hasErrors || hasViolations) && result.isIOUReportOwner ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';

If this condition also applies to the workspace switcher, we can place the check here:

    if (doesReportContainErrors && ReportUtils.isIOUOwnedByCurrentUser(report)) {
        return CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
    }

If we also want to ignore CONST.BRICK_ROAD_INDICATOR_STATUS.INFO in the workspace switcher, we can modify getBrickRoadForPolicy to:

const getBrickRoadForPolicy = (report: Report, shouldOnlyShowOwnedReport = true): BrickRoad => {
    if(shouldOnlyShowOwnedReport && !ReportUtils.isIOUOwnedByCurrentUser(report))
    {
        return undefined;
    }

What alternative solutions did you explore? (Optional)

[wildan-m]

@eVoloshchak proposal updated.

Add an optional condition if we also want to apply the same rule for the workspace switcher.

[isabelastisser]

Hey @eVoloshchak, can you please review the proposal? Thanks!

[brandonhenry]

Proposal

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

In the current implementation, when a submitter creates an expense report with an invalid receipt that fails to scan, RBR
appears in the LHN only for the submitter of the report.

The expected behavior is:

• The RBR on the LHN should only show for the user that made the request
• The RBRs on the report preview, money request preview, and the ones in the money request view, should show to every user that has access to them

What is the root cause of that problem?

The getAllReportErrors function currently does not distinguish between showing RBRs on the LHN vs showing them on report/request previews and the money request view.

App/src/libs/OptionsListUtils.ts

Line 472 in 690174f

function getAllReportErrors(report: OnyxEntry<Report>, reportActions: OnyxEntry<ReportActions>): OnyxCommon.Errors {

Specifically, the ReportUtils.hasSmartscanError check is used to determine if a smartscan error RBR should be shown:

} else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {
    reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');
}

App/src/libs/OptionsListUtils.ts

Lines 492 to 493 in 690174f

	} else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {
	reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');

This adds the RBR for any user that has access to the report actions, regardless of if they are the submitter or not.

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

To meet the requirements of only showing the RBR on the LHN for the submitter, while still showing it for all users on report/request previews and the money request view, I propose:

1. Create a new function shouldShowLHNErrorForMissingSmartscanFields that checks if the current user is the submitter before calling the existing hasMissingSmartscanFields:

function shouldShowLHNErrorForMissingSmartscanFields(iouReportID) {
  const reportActions = ReportActionsUtils.getAllReportActions(iouReportID);
  const submitterReportActions = _.filter(reportActions, reportAction => 
    reportAction.actorAccountID === currentUserAccountID
  );

  return ReportUtils.hasMissingSmartscanFields(iouReportID);
}

2. Update getAllReportErrors to use this new function for determining LHN errors:

} else if (shouldShowLHNErrorForMissingSmartscanFields(report?.reportID ?? '')) {
    reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');
}

3. Leave hasMissingSmartscanFields as is, so it continues to be used for showing RBRs on report/request previews and money request view for all users with access.

This separates the logic for showing LHN errors vs preview/money request view errors, to meet the different requirements for each.

[dukenv0307]

Proposal

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

• Reddot pinned chat appears for approver for submitter's failed scanned receipt

What is the root cause of that problem?

• When displaying a RBR, we do not check if the current user is requestor or not, which leads to the bug:
  

  App/src/libs/OptionsListUtils.ts

  Lines 504 to 506 in ddf9e7a

  	} else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {
  	reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');
  	}

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

• Removed

What alternative solutions did you explore? (Optional)

• We can create a util function to check if the current user is the requestor or not:

     const isRequestor: (reportAction) => currentUserAccountID === reportAction?.actorAccountID

• Then, we need to fix the RBR LHN in the transaction thread report, expense report and the chat report.
• The first one is transaction thread report, we need to update this one:
  

  App/src/libs/OptionsListUtils.ts

  Line 482 in 690174f

  if (parentReportAction?.actorAccountID === currentUserAccountID && ReportActionUtils.isTransactionThread(parentReportAction)) {

  
  to:

    if (isRequestor(parentReportAction) && ReportActionUtils.isTransactionThread(parentReportAction)) {

• The second one is expense report, we need to update hasMissingSmartscanFields in this one:
  

  App/src/libs/OptionsListUtils.ts

  Line 489 in 690174f

  if (ReportUtils.hasMissingSmartscanFields(report?.reportID ?? '') && !ReportUtils.isSettled(report?.reportID)) {

  
  to:

function hasMissingSmartscanFields(iouReportID: string): boolean {
    const reportActions = Object.values(ReportActionsUtils.getAllReportActions(iouReportID));
    return reportActions.some((action)=>{
        if(!ReportActionsUtils.isMoneyRequestAction(action)){
            return false
        }
        const transaction = TransactionUtils.getLinkedTransaction(action)
        return TransactionUtils.hasMissingSmartscanFields(transaction) && isRequestor(action)
    })
}

• The last one is the chat report. We do not need to update anything in this one:
  

  App/src/libs/OptionsListUtils.ts

  Line 492 in 690174f

  } else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {

  
  because the above change we made for hasMissingSmartscanFields is enough

• Additionally, we also need to fix the MoneyRequestView based on the comment "The RBRs on the report preview, money request preview, and the ones in the money request view, should show to every user that has access to them.". Do it by remove canEdit in here

• We can create a additional param in hasMissingSmartscanFields, named isLHN preview to only apply the change with LHN preview message

[Tony-MK]

Proposal

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

Scan - No red dot for transaction thread in LHN when scanning fails

What is the root cause of that problem?

The second condition block, in the getAllReportErrors function, won't be executed because on the approver's app, the report?.ownerAccountID === currentUserAccountID condition will fail.

App/src/libs/OptionsListUtils.ts

Lines 487 to 489 in 91ea979

	} else if ((ReportUtils.isIOUReport(report) || ReportUtils.isExpenseReport(report)) && report?.ownerAccountID === currentUserAccountID) {
	if (ReportUtils.hasMissingSmartscanFields(report?.reportID ?? '') && !ReportUtils.isSettled(report?.reportID)) {
	reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');

Hence, the getAllReportErrors function will continue to the third and last condition which will succeed.

This root cause of this issue is caused by the third condition in the getAllReportErrors function since the function does not consider that previous condition failed because report?.ownerAccountID !== currentUserAccountID.

App/src/libs/OptionsListUtils.ts

Lines 491 to 492 in 91ea979

	} else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {
	reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');

Therefore, the third condition will pass because the ReportUtils.hasSmartscanError returned true. After all, the reportActions contain scan errors because the REPORT_PREVIEW action has to display the RBR in this state.

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

In the third condition, we should check if the report action was acted by the current user to prevent scan errors from the reportActions getting assigned to reportActionErrors.smartscan when expense reports do not meet the report?.ownerAccountID === currentUserAccountID condition.

Old Condition

App/src/libs/OptionsListUtils.ts

Lines 491 to 493 in 91ea979

	} else if (ReportUtils.hasSmartscanError(Object.values(reportActions ?? {}))) {
	reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');
	}

New Condition

...
else if (
   ReportUtils.hasSmartscanError(
     Object.values(
      reportActions.filter((reportAction) => reportAction.actorAccountId === currentUserId) ?? {}
      )
   )
)
...

[Tony-MK]

This issue was meant to be fixed at #34827.

The expected result was explained in detail in this #34827 (comment) and #34827 (comment) by @cead22.

The issue was closed despite the third expected result is not fulfilled: The expense report should not have the RBR on the LHN for either user

[eVoloshchak]

@Tony-MK, thank you for the added context!
Asking how we should proceed in #34827 (comment)

[dukenv0307]

@eVoloshchak Also, maybe we have another expected behavior that we need to consider here #27333 (comment)

[cead22]

@JmillsExpensify do we wanna show RBR on the LHN when receipt scanning fails only to the user requesting money? I think so, and if that's the case, @brandonhenry 's and @dukenv0307 's root cause is accurate

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[isabelastisser]

Not overdue. bump @JmillsExpensify on Carlo's question above ^^

[isabelastisser]

I DM'd Mills.

[JmillsExpensify]

@JmillsExpensify do we wanna show RBR on the LHN when receipt scanning fails only to the user requesting money? I think so, and if that's the case, @brandonhenry 's and @dukenv0307 's root cause is accurate

Yes, that's correct. If the receipt scan fails, then we only RBR the LHN of the person that created the request.

[Tony-MK]

Updated Proposal

Proposal Link: #37044 (comment)

[dukenv0307]

@eVoloshchak PR #39970 is opened

[melvin-bot]

This issue has not been updated in over 15 days. @cead22, @eVoloshchak, @isabelastisser, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[isabelastisser]

@dukenv0307 @eVoloshchak, any updates?

[eVoloshchak]

@isabelastisser, PR is in review, should be merged fairly soon

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.79-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix Reddot pinned chat appears for approver after failed scanned #39970

If no regressions arise, payment will be issued on 2024-06-13. 🎊

For reference, here are some details about the assignees on this issue:

• @eVoloshchak requires payment through NewDot Manual Requests
• @dukenv0307 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eVoloshchak / @dukenv0307] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak / @dukenv0307] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@eVoloshchak / @dukenv0307] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@eVoloshchak / @dukenv0307] Determine if we should create a regression test for this bug.
• [@eVoloshchak / @dukenv0307] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[isabelastisser]

@eVoloshchak, can you please complete the BZ list above? Thanks!

[isabelastisser]

Payment summary:

@eVoloshchak C+ requires payment through NewDot Manual Requests PENDING $500
@dukenv0307 requires payment automatic offer (Contributor) - paid in Upwork. $500

[isabelastisser]

Bump @eVoloshchak to complete the BZ list before I can close the issue. I will also DM you for visibility. Thanks!

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: N/A, this was not a bug, rather a change in expected behavior
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: looks like internal discussion has already took place, which has resulted in [HOLD for payment 2024-06-13] [$500] Reddot pinned chat appears for approver for submitter's failed scanned receipt  #37044 (comment)
• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? No
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? Yes

[eVoloshchak]

Regression Test Proposal

Test case 1 (to make sure RBR on the LHN is only shown for the user that made the request):

1. Create a collect policy in OD as account A
2. Invite user B as employee
3. Log in as user B in NewDot
4. Create an expense report with an invalid receipt to fail the scanning
5. Log in to user A in NewDot
6. No RBR pinned chat appears in the LHN for the error message
7. Log in to user B in NewDot
8. Verify there is RBR pinned chat appearing in the LHN for the error message

Test case 2 (to make sure the RBRs on the report preview, money request preview, and the ones in the money request view are shown to every user that has access to them):

1. Create a collect policy in OD as account A
2. Invite user B as employee
3. Log in as user B in NewDot
4. Create an expense report with an invalid receipt to fail the scanning
5. Log in to user A in NewDot
6. Verify the RBRs on the report preview, money request preview, and the ones in the money request view are shown
7. Log in to user B in NewDot
8. Verify the RBRs on the report preview, money request preview, and the ones in the money request view are shown

Do we agree 👍 or 👎

[JmillsExpensify]

$500 approved for @eVoloshchak

[isabelastisser]

All set!