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.36k stars 2.78k forks source link

[HOLD] [$500] [MEDIUM] Split bill – When Smartscan fails in Workspace, the Merchant field is empty and the amount is displayed as $0.00. #36046

Open m-natarajan opened 7 months ago

m-natarajan commented 7 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.38-0 Reproducible in staging?: y Reproducible in production?: y 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:

  1. Go to https://staging.new.expensify.com/
  2. Login
  3. Navigate to the workspace chat
  4. Click on the + button > Split bill
  5. Use a receipt that will not fail the scan
  6. Continue to the final review page
  7. Create the expense
  8. When the scan request is complete click on the split preview to navigate to the split report

Expected Result:

The amount and merchant fields have been updated

Actual Result:

When Smartscan fails in Workspace, the Merchant field is empty and the amount is displayed as $0.00.

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/38435837/f19b6c2b-eef8-493d-9a84-c3bc49423c90

View all open jobs on GitHub

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

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

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

m-natarajan commented 7 months ago

We think that this bug might be related #wave6-collect-submitters CC @greg-schroeder

abdulrahuman5196 commented 7 months ago

No proposals yet

abekkala commented 7 months ago

https://expensify.slack.com/archives/C03UK30EA1Z/p1707851230196889 update: CS unable

melvin-bot[bot] commented 7 months ago

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

abekkala commented 7 months ago

https://expensify.slack.com/archives/C04878MDF34/p1708013831428379

melvin-bot[bot] commented 7 months ago

@abekkala, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

abekkala commented 7 months ago

https://app.slack.com/client/E047TPA624F/C04878MDF34

melvin-bot[bot] commented 7 months ago

Upwork job price has been updated to $1000

brunovjk commented 7 months ago

Proposal

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

Split bill – Merchant field is empty, amount is $0.00 when Smartscan fails in WS:

  1. Splitbill receipt does not carry value and merchant correctly.
  2. When Splitbill receipt does not load value and merchant correctly, the same fields continue with 0.00 and empty merchant

What is the root cause of that problem?

  1. I believe it is a backend issue, I tried several different receipts, and none of them managed to extract the value correctly.
  2. In startSplitBill and completeSplitBill we hardcode the value of amount and merchant to 0 and "none": https://github.com/Expensify/App/blob/4ae7b293018a7cd3d41a265cdca34f1dd8417552/src/libs/actions/IOU.ts#L2000 https://github.com/Expensify/App/blob/4ae7b293018a7cd3d41a265cdca34f1dd8417552/src/libs/actions/IOU.ts#L2007 https://github.com/Expensify/App/blob/4ae7b293018a7cd3d41a265cdca34f1dd8417552/src/libs/actions/IOU.ts#L2325 And if the value is not loaded correctly at the time of scanning, 0.00 and "" remains.

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

  1. Check at BE why we haven't received the correct amount on the receipt.
  2. Update completeSplitBill to show another message/value, we removed the TDB, if the scan is unsuccessful, instead of leaving 0 and "".

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 7 months ago

@abekkala @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

kmbcook commented 7 months ago

I don't think this issue is described correctly. In the video, a receipt is used which FAILS, which is the opposite of what step 5 describes. Which is correct?

Expected Result doesn't match the video.

Actual Result is missing.

trjExpensify commented 7 months ago

@m-natarajan - please can you include:

  1. the account email from the above
  2. the receiptID (open the expense in OldDot and type this into the JS console: g_megaEdit.expense.receiptID)
  3. the file used for upload

Here’s going through splitting a receipt with a workspace that I know won’t fail:

https://github.com/Expensify/App/assets/16232057/3e96acfe-08e9-4f58-ae60-820019bba908

abekkala commented 7 months ago

Holding as I think this may not be an issue. The failed smartscan outcome above is expected. Once I get the info above, I can look into the receipt ID, etc

m-natarajan commented 7 months ago

@trjExpensify Tester could not find the failed scan expense in the OD. This the failed receipt report id from ND https://staging.new.expensify.com/w/0E2C8C1A7530F0D2/r/8639230505798598/split/9055791033890449916

Email used is applausetester+jp_e_category@applause.expensifail.com

abekkala commented 7 months ago

I don't see a report under that ID: 8639230505798598

If the smartscan failed and provided a red "Merchant" and $0.00 - that is expected and not a bug

rojiphil commented 7 months ago

@abekkala I think the issue here is that there is no violation message displayed for amount even though amount is 0 and the smart scan has failed to process the amount. However, violation message is displayed for missing merchant.

And I think the root cause for this is that we are not handling receipt state of DELETED sent by BE (refer below). To resolve this in FE, we can add DELETED to RECEIPT_STATE here and include this state in the condition here. Alternatively, if it was not intentional to send DELETED state, BE can also send SCANFAILED state which will resolve the problem here. But if we are doing this in FE, I can put this in proposal format.

I just had a look at this issue. But it looks like the party's already over here. Maybe not! Do you think this can be reopened? cc @trjExpensify @abdulrahuman5196

Screenshot 2024-02-22 at 9 46 02 PM

kbecciv commented 7 months ago

I'm reopening the issue for a second review. Previously, there was an error next to the Amount, Merchant, and Split button. Screenshot 2024-02-27 at 22 05 33 (2)

rojiphil commented 7 months ago

Converting my comment here into proposal format. What are your thoughts on this? cc @abdulrahuman5196

Proposal

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

The issue here is that there is no violation message displayed for amount even though amount is 0 and the smart scan has failed to process the amount. However, violation message is displayed for missing merchant.

What is the root cause of that problem?

The root cause for this is that we are not handling receipt state of DELETED sent by BE (refer below).

Screenshot 2024-02-22 at 9 46 02 PM

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

To resolve this in FE, we can add DELETED to RECEIPT_STATE here and include this state in the condition here. Alternatively, if it was not intentional to send DELETED state, BE can also send SCANFAILED state which will resolve the problem here.

What alternative solutions did you explore? (Optional)

trjExpensify commented 7 months ago

Sorry @kbecciv but for us to actually triage this issue and look at evaluate proposals accurately the OP needs to be improved a lot. For example:

Actual result: Describe what actually happened

  1. On the scan option, upload a digital receipt that will not fail the scan

Contrast that to the title which alludes to "when SmartScan fails in WS".

When the scan request is complete click on the split preview to navigate to the report conversation

Overall, this is a pretty poor issue. Can we start over and clean this up to then revaluate accurately, please? Thanks!

kbecciv commented 7 months ago

@trjExpensify Apologies for the oversight. I've made the necessary corrections in the OP based on your suggestion.

abekkala commented 7 months ago

@kbecciv I still need some clarification here:

You state :

  1. Use a receipt that will not fail the scan

yet, in your video I'm not seeing that SmartScan has been given the time to actually complete and be considered a successful smartscanned receipt.

Screenshot 2024-02-27 at 3 24 26 PM

Often, SmartScan does NOT complete in a matter of 2-3 seconds.

Have you tested this with a SmartScanned receipt that actually completes a successful smartscan .

From what I can tell you're attempting to split a receipt that does not have a successful SmartScan and that outcome is expected.

abekkala commented 7 months ago

And going back to your title: When Smartscan fails in Workspace, the Merchant field is empty and the amount is displayed as $0.00

This is expected and how SmartScan operates

melvin-bot[bot] commented 7 months ago

Upwork job price has been updated to $500

abekkala commented 7 months ago

I've changed this back to $500 as I prematurely increased as I misunderstood the original issue as originally provided.

IF this moves forward (although I'm still not convinced this is a real issue) this can go back to the original price after discussions have been complete to pinpoint IF there is a real issue and that issue should be stated correctly and in detail.

brunovjk commented 7 months ago

@abekkala, @trjExpensify Can you please tell me what should happen "When Smartscan fails in Workspace"? The Merchant field should be empty and the amount should be $0.00? Thank you :D

abekkala commented 7 months ago

Yes, if a smartscan fails - the merchant name is left blank and the amount is $0.00. A failed SmartScan requires the user to manually input those details.

abekkala commented 7 months ago

yes, here is an example from a failed smartscan just yesterday:

Screenshot 2024-02-27 at 3 55 50 PM
brunovjk commented 7 months ago

Great! Thank you. What about the amount failure message? image

Here looks like we have, but I can't reproduce this.

abekkala commented 7 months ago

A successful SmartScan requires at least an amount & date. Without either one of those two things, the SmartScan will fail every time. For context, if there is not a merchant, or it cannot be clearly read, “Unknown Merchant” will be entered (non parsed receipt).

@trjExpensify in the case of requesting money I believe we expect something different, correct? Meaning smartscanning an expense receipt (for reporting purposes) means something different than than when smartscanning for an IOU. Based on this convo that you and Jason participated in.

brunovjk commented 7 months ago

Are those related https://github.com/Expensify/App/issues/34916#issuecomment-1905293484? Thank yoiu.

trjExpensify commented 7 months ago

@trjExpensify in the case of requesting money I believe we expect something different, correct? Meaning smartscanning an expense receipt (for reporting purposes) means something different than than when smartscanning for an IOU. Based on this convo that you and Jason participated in.

That hasn't been implemented yet for requests that go on iouReports (i.e "P2P, requests to friends"), so that wouldn't factor in here. It's being progressed by @youssef-lr here (internal repo). But yes, at some point in the future iouReport'd requests won't show a merchant field unless you scan a receipt successfully.

For context, if there is not a merchant, or it cannot be clearly read, “Unknown Merchant” will be entered (non parsed receipt).

Yeah, I think it's still expected in NewDot that:

  1. "Unknown merchant" to be populated when the merchant field is deemed not clear on the receipt but amount and/or date are, such that it's a partial receipt. In this case, you would not see the Enter a merchant name error message. (CC: @Gonals)
  2. The merchant field is blank when a receipt upload is marked as illegible (i.e say a random picture with no merchant, amount or date readable).

What about the amount failure message?

  1. The amount field should be blank and has the error Enter an amount when the receipt is marked illegible.
  2. The amount field has $0.00 and has the Enter an amount error message when the receipt is deemed partial.

CC: @mountiny as well for a fact check on these, as I know you've worked on this a bit at some point in the last few months. It's pretty easy to get turned around!

JmillsExpensify commented 7 months ago

Agreed, this is tricky. That sounds right to me though.

rojiphil commented 7 months ago

Based on the following expected behavior mentioned here

1 The amount field has $0.00 and has the Enter an amount error message when the receipt is deemed partial.

and, the following comment here from the testing team (Also please note the attached image there)

I'm reopening the issue for a second review. Previously, there was an error next to the Amount, Merchant, and Split button.

it seems to me that one issue here is that the violation message is not displayed for amount. Looks like there is consensus coming out on this. right?

rojiphil commented 7 months ago

1 "Unknown merchant" to be populated when the merchant field is deemed not clear on the receipt but amount and/or date are, such that it's a partial receipt. In this case, you would not see the Enter a merchant name error message. (CC: @Gonals) 2 The merchant field is blank when a receipt upload is marked as illegible (i.e say a random picture with no merchant, amount or date readable).

And this looks a little confusing to me. Especially since there is no reference to usage of Unknown Merchant in code in FE. Or maybe the BE should have populated the merchant text with Unknown Merchant in which case FE would have naturally not shown the error message. Looking forward to authentic feedback from @mountiny @gonals.

abekkala commented 7 months ago

When I tested in staging if I prematurely clicked Split prior to letting the SmartScan complete (it's still in progress) an Amount error appears:

Screenshot 2024-02-29 at 5 30 42 PM

I let this SmartScan complete (it failed - not sure why as the example receipt does show a date, merchant, amount 🤷‍♀️) In any case, I am provided the red dot 'error' once I expand the receipt there is no error and it only appears once I click Split 2024-02-29_17-35-50 (1)

rojiphil commented 7 months ago

I let this SmartScan complete (it failed - not sure why as the example receipt does show a date, merchant, amount 🤷‍♀️) In any case, I am provided the red dot 'error' once I expand the receipt there is no error and it only appears once I click Split

@abekkala Exactly. And, that I think is the issue here. Earlier it was showing error message but now it does not. On clicking on Split button though, the validation will anyway fail and show the error as expected.

melvin-bot[bot] commented 6 months ago

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

abekkala commented 6 months ago

CC: @mountiny or @Gonals would you be able to fact check the above?

melvin-bot[bot] commented 6 months ago

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

mountiny commented 6 months ago

Sorry, lots of comments in this issue. @rojiphil or someone else would you mind summarizing what are the questions here please?

youssef-lr commented 6 months ago

I can help investigating this one on Monday @mountiny

rojiphil commented 6 months ago

Sorry, lots of comments in this issue.  @rojiphil or someone else would you mind summarizing what are the questions here please?

@mountiny Agree. Quite a lot of discussions happened here. Not sure of others, but to me, it looks like there is no ambiguity about the amount issue. However, regarding Unknown merchant, the following comment here seems like inconsistent with the current FE.

1 "Unknown merchant" to be populated when the merchant field is deemed not clear on the receipt but amount and/or date are, such that it's a partial receipt. In this case, you would not see the Enter a merchant name error message. (CC: @Gonals)

Currently, there is no reference to the usage of Unknown Merchant in code in FE. So, it makes me wonder if the BE should have populated the merchant text with Unknown Merchant in which case FE would have naturally not shown the error message. Or is this a recent change that needs to be implemented in FE? cc @youssef-lr

youssef-lr commented 6 months ago

Going to run some tests and get back to you @rojiphil by EOD.

melvin-bot[bot] commented 6 months ago

@youssef-lr, @abekkala, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

abekkala commented 6 months ago

@youssef-lr is there any update for @rojiphil on this one?

youssef-lr commented 6 months ago

I think the summary is still this and it sounds good to me as well. I think we just need to implement it? This will need backend changes I think because the frontend has no way of knowing if the receipt is marked as illegible.

abekkala commented 6 months ago

not overdue - still on hold