[$500] Scan - "Receipt scanning failed. Please enter the details manually" is visible to requestee

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.3.97-1 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:

[User A] Go to chat with User B > + > Request money > Scan
[User A] Create a Scan request with a receipt that will fail the smartscanning
[User B] Open the Scan request details page when the smartscanning fails Note that User B can see the Receipt scanning failed message which also asks User A to enter the details manually

Expected Result:

"Receipt scanning failed. Please enter the details manually" message will not appear to User B

Actual Result:

"Receipt scanning failed. Please enter the details manually" message appears to User B. It is giving the impression that User B is asked to enter the details manually, when the message is aimed at User A

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

Bug6270183_1699546745794!IMG_0726

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d0904e9e5de86b0d
Upwork Job ID: 1722652686187794432
Last Price Increase: 2023-11-30

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

SrR0b0 commented 10 months ago

Proposal

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

The message shown to the author of scan request when the scan fails also appears to the other users who are assigned in the request. The message prompts the user to upload a new image to be scanned but it doesn't make sense to prompt other users than the author of the request for such a thing.

What is the root cause of that problem?

The root cause of the problems is in the logic right where the MoneyRequestConfirmationList component decides whether to show the "iou.receiptScanningFailedMessage" error message.

https://github.com/Expensify/App/blob/5cff53ba5b71bc328d359dbc447cf7dc9157ddb1/src/components/MoneyRequestConfirmationList.js#L275-L278

The problem is that it is not checked whether the user session belongs to the author of the request.

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

Simply adding another message in the translation files for the case the error is being rendered for some user that's not the author of the request and adding a conditional logic in the MoneyRequestConfirmationList component to render the proper message would be enough and quite certainly wouldn't introduce any regressions.

What alternative solutions did you explore? (Optional)

ikevin127 commented 10 months ago

Debugging observations:

when the scan request fails the notifications@expensify.com's message is only shown by re-visiting the report or refreshing the page, I'm assuming this is pusher event related
iou.receiptScanningFailedMessage: 'Receipt scanning failed. Enter the details manually.' is different than the notifications@expensify.com's message Receipt scanning failed. Please enter the details manually. which comes from the back-end after request scan failure, not from the front-end translation

considering these observations I think this issue should be fixed from the back-end where the pusher event notification for when a receipt scan failed should only be sent to the requestor. If a message is needed on the requestee side too, then the message should differ from the one sent to the requestor.

It should be fixed from the back-end because on the front-end side there's no solid way to check for the notifications@expensify.com's scan failed message and hide it both from the report threads preview and and also from within the thread itself.

dylanexpensify commented 9 months ago

confirming something here!

trjExpensify commented 9 months ago

👋 Coupla' things..

The system message is added to the transaction thread visible for all to see as part of the audit history, it's not a whisper message just for the requestor. This part my might be pending wave6 updates but; nobody is subscribed to the transaction thread by default, so they don't get notified about the sys message when added, and I believe this will be made an @-mention for the submitter too with wave6 so they continue to be when receipt scanning fails (CC: @puneetlath @mountiny)
The RBR; so the red dot on the request row when it appears in the LHN as "receipt missing details" and the in-line error messages on the merchant, date and amount rows in error on the request detailed view are only visible for the submitter of the request as it's an action for them to take.
When the hold project is done, a failed receipt scan should automatically go on hold and will be visible to all as being on hold, but that's a wave7 initiative. (CC: @robertjchen @JmillsExpensify).
The Expensify user is in the process of being given a display name and Expensify iconmark, the issue for that is here. (CC: @Gonals).

As for these:

when the scan request fails the notifications@expensify.com's message is only shown by re-visiting the report or refreshing the page, I'm assuming this is pusher event related

Agreed, it shouldn't require a refresh or navigating away/back for the sys message to appear in the transaction thread when the receipt scan fails.

iou.receiptScanningFailedMessage: 'Receipt scanning failed. Enter the details manually.' is different than the notifications@expensify.com's message Receipt scanning failed. Please enter the details manually. which comes from the back-end after request scan failure, not from the front-end translation

I agree the FE/BE error messages should be the same. The one from the doc is Receipt scanning failed. Please enter the details manually for ref, but I actually don't mind dropping the "please" for brevity and standardising on that. Ah actually, I bet the FE error was added and only used in the split case for the "read-only" confirmation page in the RHP which is a little bit more like a form error atop a button to action on trying to recreate the split after fixing the errors as there is no transaction thread here to add a system message from Expensify. (CC: @youssef-lr).

melvin-bot[bot] commented 9 months ago

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

DylanDylann commented 9 months ago

Proposal

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

We have inconsistency between the message from FE and system message From FE (using in EditingSplitBillPage: 'Receipt scanning failed. Enter the details manually.' System Message (using in the transaction thread): 'Receipt scanning failed. Please enter the details manually'

What is the root cause of that problem?

We have inconsistency between the message from FE and system message From FE (using in EditingSplitBillPage: 'Receipt scanning failed. Enter the details manually.' System Message (using in the transaction thread): 'Receipt scanning failed. Please enter the details manually'

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

We need to update the message in FE to match the system message (Spanish too). Or we also update the new message and use it in both FE and BE for consistency

What alternative solutions did you explore? (Optional)

dylanexpensify commented 9 months ago

thanks @trjExpensify for the notes! Closing

DylanDylann commented 9 months ago

@dylanexpensify What do you think about this point in @trjExpensify's comment

I agree the FE/BE error messages should be the same. The one from the doc is Receipt scanning failed. Please enter the details manually for ref, but I actually don't mind dropping the "please" for brevity and standardising on that

trjExpensify commented 9 months ago

Yeah, to be clear I think there appear to be two things uncovered from this issue to work on:

It shouldn't require a refresh or navigating away/back for the sys message to appear in the transaction thread when the receipt scan fails. What's going on with that pusher event? (CC: @Gonals here)
The FE/BE error messages are using slightly different copy. Even though they are used in different places, let's standardise. Meaning, update the sys message one to Receipt scanning failed. Enter the details manually. dropping the "please".

mountiny commented 9 months ago

Reopening as it seems like we are not done here based on the latest comments

dylanexpensify commented 9 months ago

Good call, reviewing today!

dylanexpensify commented 9 months ago

Agree with @trjExpensify's comments - @DylanDylann do you have a proposal that would tackle those two points he brought up? cc @eVoloshchak

DylanDylann commented 9 months ago

@dylanexpensify My proposal will not fix 2 above bugs, we need to fix on BE side

melvin-bot[bot] commented 9 months ago

@eVoloshchak @dylanexpensify 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!

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

@eVoloshchak, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

eVoloshchak commented 9 months ago

To summarize, we have 3 issues than need to be resolved

Update the system message to Receipt scanning failed. Enter the details manually dropping the "please". - needs to be done on back end
The refresh or navigating away/back is required for the system message to appear in the transaction thread when the receipt scan fails. Adding to this, until you open a transaction on User A's device, it will not be marked as "scan failed" for User B
The system message is added to the transaction thread visible for all to see as part of the audit history - looks like it will be resolved in wave6

@DylanDylann, can item 2 be resolved on the front end?

DylanDylann commented 9 months ago

@eVoloshchak I tested again and saw that If User A requests money by scanning image to User B, User B sees this request only when it is a success or failure. If the receipt is scanning, It doesn't display on User B

In case the scanning fail, the error message will display immediately in transaction thread (note that: transaction thread only be accessed when request fail)

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

@eVoloshchak, @dylanexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

dylanexpensify commented 9 months ago

@eVoloshchak mind giving an update

melvin-bot[bot] commented 9 months ago

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

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 9 months ago

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

dylanexpensify commented 9 months ago

bump @eVoloshchak

dylanexpensify commented 8 months ago

bump @eVoloshchak

evoL commented 8 months ago

@dylanexpensify It's possible that they didn't receive the notification because, well, I did. I'd recommend reaching out to them through different means.

I've already reported this issue to GitHub, no response yet.

melvin-bot[bot] commented 8 months ago

📣 @evoL! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

eVoloshchak commented 8 months ago

@dylanexpensify, apologies for the delay There are two sub-tasks left for this:

Update the system message to Receipt scanning failed. Enter the details manually dropping the "please". - needs to be done on back end
The system message is added to the transaction thread visible for all to see as part of the audit history - looks like https://github.com/Expensify/App/issues/31140#issuecomment-1810047413 in wave6

This needs an internal engineer to make a small change on the back end, after which we might put this on hold for wave6

melvin-bot[bot] commented 8 months ago

@eVoloshchak, @dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

dylanexpensify commented 8 months ago

Perfect, will get a volunteer/tribute!

melvin-bot[bot] commented 8 months ago

@eVoloshchak, @dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 8 months ago

@eVoloshchak, @dylanexpensify Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 8 months ago

@eVoloshchak, @dylanexpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 8 months ago

@eVoloshchak, @dylanexpensify 12 days overdue. Walking. Toward. The. Light...

eVoloshchak commented 8 months ago

Not overdue, https://github.com/Expensify/App/issues/31140#issuecomment-1864142734

dylanexpensify commented 8 months ago

still asking around

dylanexpensify commented 7 months ago

Woot! @rlinoz how's this coming along?

rlinoz commented 7 months ago

Opening a PR today to avoid the need to refresh the page to get the scan failed message.

dylanexpensify commented 7 months ago

Wooot!

rlinoz commented 7 months ago

PR is merged, waiting for deploy 😄

dylanexpensify commented 7 months ago

Woooot!

rlinoz commented 7 months ago

This has been deployed to production!

Can we close it?

Expensify / App