search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$1000] [MEDIUM] Scan - No red dot for transaction thread in LHN when scanning fails #34827

Closed lanitochka17 closed 9 months ago

lanitochka17 commented 10 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.28-0 Reproducible in staging?: Y Reproducible in production?: N Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/31448

Action Performed:

  1. Go to workspace chat
  2. Create a scan request with receipt that will fail the scanning
  3. Wait for the scanning to fail
  4. Click on the preview to open report page
  5. Click on the preview to open details page

Expected Result:

Actual Result:

Both expense report and details page do not show red dot in LHN

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/de240dda-8f67-4d86-babe-83e5914a0238

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e2581b13bb117eee
  • Upwork Job ID: 1757711391539257344
  • Last Price Increase: 2024-02-16
melvin-bot[bot] commented 10 months ago

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

github-actions[bot] commented 10 months ago

:wave: 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[bot] commented 10 months ago

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

lanitochka17 commented 10 months ago

We think that this bug might be related to #wave6. CC @greg-schroeder

mountiny commented 10 months ago

We should fix this but I dont think its that major to be a deploy blocker now cc @cead22

cead22 commented 10 months ago

Agreed, let's HOLD this one too, since I think it may be fixed by a fix for a bug we just found in the main violations PR -- context here https://expensify.slack.com/archives/C01GTK53T8Q/p1705693401154309?thread_ts=1705692222.233999&cid=C01GTK53T8Q

laurenreidexpensify commented 10 months ago

on hold for bug mentioned above, not overdue

marcochavezf commented 9 months ago

Hi @cead22, this is the PR we were holding for this issue, correct?

cead22 commented 9 months ago

It looks like it, so we can remove the HOLD here if this is still happening

Both expense report and details page will show red dot in LHN

That said, the expected result is that

melvin-bot[bot] commented 9 months ago

@marcochavezf @laurenreidexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

marcochavezf commented 9 months ago

I'm already assigned to other wave issues. Dropping it in wave6 channel https://expensify.slack.com/archives/C02MW39LT9N/p1707498241279089

laurenreidexpensify commented 9 months ago

Just to be clear - is this one internal @marcochavezf @cead22 ?

cead22 commented 9 months ago

This one's external

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

@allroundexperts @laurenreidexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 9 months ago

Current assignee @allroundexperts is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 9 months ago

Upwork job price has been updated to $1000

laurenreidexpensify commented 9 months ago

increased bounty

bernhardoj commented 9 months ago

Proposal

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

Based on the new expected result, I see only the last one that doesn't match, that is the red dot shown in the expense report.

What is the root cause of that problem?

We have a logic here to get the report errors that include expense/IOU reports which will show the red dot error if the user is the owner. https://github.com/Expensify/App/blob/20af9e52db6031dea6d0086a6692682eb15e422a/src/libs/OptionsListUtils.ts#L487-L490

This was first added in https://github.com/Expensify/App/pull/27494.

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

If we want to change it, then we can simply remove the condition or just the expense report if we want to keep the IOU one.

(IOU report only) Related to the red dot, I see that the red dot is shown in the report preview, DM chat LHN, and the request preview even though they are not the owner. I think the red dot shouldn't be shown if they aren't the owner because they can't edit it. We need to check the owner, but I didn't include the details here as it could be expected and probably out of scope

Tony-MK commented 9 months ago

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?

In trying to reproduce this bug, the following expectations from https://github.com/Expensify/App/issues/34827#issuecomment-1928050071 were fulfilled:

However the third expectation, "The expense report should not have the RBR on the LHN for either user", is not fulfilled.

https://github.com/Expensify/App/blob/91ea9792fe54b795115106965b2682dd1ef36847/src/libs/OptionsListUtils.ts#L481-L493

The third expectation is caused by the third condition in the getAllReportErrors function since the function does not consider report?.ownerAccountID === currentUserAccountID.

Therefore, the third condition will pass because the ReportUtils.hasSmartscanError returned true for the reportActions.

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

In the third condition, check if the report is an expense report, to prevent reportActionErrors.smartscan to be being set to expense reports that didn't meet the report?.ownerAccountID === currentUserAccountID conditions.

Hence, let's use the ReportUtils.isExpenseReport function as such:

else if (!ReportUtils.isExpenseReport(report) && ReportUtils.hasSmartscanError(Object.values(reportActions ?? {})))

https://github.com/Expensify/App/blob/91ea9792fe54b795115106965b2682dd1ef36847/src/libs/OptionsListUtils.ts#L491-L493

laurenreidexpensify commented 9 months ago

@allroundexperts couple of proposals to review ^^

allroundexperts commented 9 months ago

Still reviewing these. Need a little more time. Will provide an update shortly.

allroundexperts commented 9 months ago

Can we confirm if even for IOU reports, we don't want to show the RBR in LHN if there is a smart scan error? @marcochavezf @cead22

allroundexperts commented 9 months ago

@bernhardoj Removing the second condition won't work in this case since the third condition would simply kick in, causing the RBR to show up.

@Tony-MK Removing only the last condition would result in showing of an RBR for the expense report if the user is admin which we don't want. As such, I don't think that your proposal would work.

allroundexperts commented 9 months ago

Still looking for a more accurate proposal.

bernhardoj commented 9 months ago

The third condition won't kick in because it will return false if it's not a report preview action. The condition is used to show the RBR on the chat report if there is a split bill or report preview error. https://github.com/Expensify/App/blob/0797c0ac556e9d3822f6121e4a9fd3943d990a2c/src/libs/ReportUtils.ts#L4767-L4770

allroundexperts commented 9 months ago

Ah, I did not notice this condition. Given this, your proposal should be good.

We just need to confirm if we need to show the RBR for IOU report or not.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @amyevans, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

cead22 commented 9 months ago

If by IOU report you mean reports holding p2p request between users and outside of a workspace, then those won't get violations. Only money requests on workspaces get violations. Let me know if that answers your question

Tony-MK commented 9 months ago

@allroundexperts, I think you might be confusing the two proposals.

My proposal did not suggest removing any conditions.

Kindly, could you re-review my proposal?

Thanks

Tony-MK commented 9 months ago

Hi @cead22, just a quick question.

In a p2p request, is it expected to show the RBR of IOUs for only the requestor to be notified? 🤔

Thank you.

cead22 commented 9 months ago
mvtglobally commented 9 months ago

Issue not reproducible during KI retests. (First week)

laurenreidexpensify commented 9 months ago

I am closing based on the above comment that this is no longer reproducible

eVoloshchak commented 8 months ago

Hey folks, this is still reproducible in https://github.com/Expensify/App/issues/37044 Should we handle it there or re-open this issue?

allroundexperts commented 8 months ago

We should re-open this. cc @laurenreidexpensify