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.58k stars 2.92k forks source link

[$250] Scan - Receipt scanning shown on LHN after scanning is done #51251

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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: 9.0.52-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): nathanmulugetatesting+1867@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Sign up with a new gmail account
  3. Start a chat with another user
  4. Send a scan receipt
  5. Wait for it until it gets done (without navigating to another chat)
  6. After the scanning is done navigate to your chat (Which must be unread since it is a new account)

Expected Result:

The LHN subtitle shows the amount of the scanned receipt

Actual Result:

The LHN subtitle shows "Receipt scanning..." even if the receipt is done scanning

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/753ff05a-47c1-483f-83b5-ee50f22ffff6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021851023250294310857
  • Upwork Job ID: 1851023250294310857
  • Last Price Increase: 2024-11-25
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 1 month ago

@strepanier03 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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 4 weeks ago

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

melvin-bot[bot] commented 4 weeks ago

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

strepanier03 commented 4 weeks ago

Yup, reproducible.

image
FitseTLT commented 3 weeks ago

This is BE bug, it is setting receipt state to SCAN_READY instead of SCAN_COMPLETE

jacobkim9881 commented 3 weeks ago

Proposal

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

When clicking to navigate to other chat rooms after scanning is finished, the title of scanned report on LHN is changed back to "Receipt scanning..." not "xxx owes x$".

What is the root cause of that problem?

When onyx data of scanned receipt is given("Action Performed" 5), the onyx data aren't added to queuedOnyxUpdates:


data: Array (3)
0 {key: "transactions_8985775263575639680", onyxMethod: "merge", value: Object}
1 {key: "report_5903074309041026", onyxMethod: "merge", value: Object}
2 {key: "reportNameValuePairs_5903074309041026", onyxMethod: "merge", value: {type: "iou"}}

When scanning is finished("Action Performed" 5), GetMissingOnyxMessages lets the client App know the receipt is scanned. So the client App updates onyx data by executing subscribeToUserEvents(). In the function, Onyx.update() is executed and then the title of LHN is changed into "xxx owes x$". So far so good, however when clicking another title on LHN to navigate to other room, then queuedOnyxUpdates are updated at once by SequentialQueue.flush(). "Xxx owes x$" isn't added into queuedOnyxUpdates array, so the title changes into default title "Receipt Scanning...". It is because the onyx data of scanned receipt aren't added into queuedOnyxUpdates array but executed at Onyx.update(). So w/o the added onyx data the title of LHN shows former title "Receipt Scanning...".

There is a reason why data from GetMissingOnyxMessages can't be added into queuedOnyxUpdates. Usually requests with "write" request type only can be in queuedOnyxUpdates. There is a condition that only "write" request type data can be added. So data of requests can be updated by queuedOnyxUpdates. queuedOnyxUpdates array are made here:

https://github.com/Expensify/App/blob/66195ad78fd1fab200ee82e2d530ec38e2947587/src/libs/actions/OnyxUpdates.ts#L32

However GetMissingOnyxMessages doesn't have "write" type and data. It only gets 2 update ID for checking whether it's latest one. The updated onyx data is given by Pusher as an update. This is the log that Pusher sent an update after knowing update is behind latest one:


[info] [Report] Handled multipleEvents event sent by Pusher - 
{"updates":[{"data":[{"key":"transactions_8503838541148848104",
"onyxMethod":"merge","value":{"modifiedAmount":300,
"modifiedCreated":"2017-04-07","modifiedCurrency":"KRW",
"modifiedMerchant":"Unknown Merchant",
"receipt":{"receiptID":8716085149354104,"source":"https://www.expensify.com/receipts/w_b747533cfa6161c6f5b025f03b5107b661fcb0c6.png","state":"SCANCOMPLETE"}}},
{"key":"transactionViolations_","onyxMethod":"mergecollection","value":
{"transactionViolations_8503838541148848104":null}}],"eventType":"onyxApiUpdate"}],
"lastUpdateID":2686094501,"previousUpdateID":2686094474}""

queuedOnyxUpdates is updated once by SequentialQueue.flush(). It is an array of updated onyx data. Whenever certain requests are executed results are stacked in the array. When all updates should be updated then SequentialQueue.flush() does its job.

By adding requests in queuedOnyxUpdates array, there is a point all is done in this issue. Function SequentialQueue.flush() update all array into onyx. In this case, it is executed by clicking another title to navigate to other report. Like this issue, similar onyx updates that aren't added into queuedOnyxUpdates array can be rolled back.

For this issue, Submit expense with "Scan" did money request("write" type) once, and then "GetMissingOnyxMessages" done as a 2nd. However "GetMissingOnyxMessages" gives information which update is needed without having data like "lastUpdateID":2686094501, "previousUpdateID":2686094474. So the data of scanned receipt aren't added into queuedOnyxUpdates. The data only be updated by Handled multiple events by pusher.

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

We should add a condition to add the updates given by Pusher into QueuedOnyxUpdates array and QueuedOnyxUpdates.queueOnyxUpdates should be executed here:

https://github.com/Expensify/App/blob/e7532e972c28bad5fda2994435b5a37e9aed153b/src/libs/actions/OnyxUpdates.ts#L69-L81

It does Onyx.update() for data given by Pusher. Like syncing with other device or getting updates from BE API can be passed here above. I am not sure all the cases of https responses so adding conditions should be edited by helps from internal engineers. Updates passed here above are usually actions which are done in report screen. So by opening the report, openReport API can load all latest updates of the report. For this issue we should add particular conditions. We may execute QueuedOnyxUpdates.queueOnyxUpdates(updates.data) for example:

function applyPusherOnyxUpdates(updates: OnyxUpdateEvent[]) {
condition ? QueuedOnyxUpdates.queueOnyxUpdates(updates[0].data) : null;
      pusherEventsPromise = pusherEventsPromise.then(() => {
          console.debug('[OnyxUpdateManager] Applying pusher update');
      });

What alternative solutions did you explore? (Optional)

We can add a key for updates to pass a condition. With a flag we can easily pass a condition to add the updates into queueOnyxUpdates array. Currently the update shapes like this:

Array 
0 Object
data: Array (3)
0 {key: "transactions_8503838541148848104", onyxMethod: "merge", value: Object}
1 {key: "report_5967872421914163", onyxMethod: "merge", value: Object}
2 {key: "reportNameValuePairs_5967872421914163", onyxMethod: "merge", value: {type: "iou"}}
eventType: "onyxApiUpdate"

We may add a key like shouldAddIntoQueueOnyxUpdates : true. And pass a condition:

function applyPusherOnyxUpdates(updates: OnyxUpdateEvent[]) {
shouldAddIntoQueueOnyxUpdates ? QueuedOnyxUpdates.queueOnyxUpdates(updates[0].data) : null;
      pusherEventsPromise = pusherEventsPromise.then(() => {
          console.debug('[OnyxUpdateManager] Applying pusher update');
      });
melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

Ollyws commented 3 weeks ago

Will get to this one tomorrow.

muttmuure commented 3 weeks ago

IOU bug, moving to MEDIUM

melvin-bot[bot] commented 2 weeks ago

@strepanier03 @Ollyws 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!

Ollyws commented 2 weeks ago

This is BE bug, it is setting receipt state to SCAN_READY instead of SCAN_COMPLETE

@FitseTLT, where are you seeing this? I'm recieving a pusher event with the state SCANCOMPLETE.

FitseTLT commented 2 weeks ago

This is BE bug, it is setting receipt state to SCAN_READY instead of SCAN_COMPLETE

@FitseTLT, where are you seeing this? I'm recieving a pusher event with the state SCANCOMPLETE.

The LHN changes to receipt scanning when it's receipt state is changed back to SCAN_READY so there is no other source than the BE that would change the value. That's how I inferred.

jacobkim9881 commented 2 weeks ago

There is a Pusher giving the transaction merged. But it isn't added into queuedOnyxUpdates array. I think there are some similar issues. Like for a while data are missed.

jacobkim9881 commented 2 weeks ago

https://github.com/Expensify/App/issues/51785

melvin-bot[bot] commented 2 weeks ago

@strepanier03, @Ollyws Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 weeks 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 1 week ago

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

Ollyws commented 1 week ago

Will get to this one asap.

jacobkim9881 commented 1 week ago

I think this issue should get help from BE guys or needs discussion on slack to solve because I don't know enough cases of http requests for this app.

melvin-bot[bot] commented 1 week 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 1 week ago

@strepanier03 @Ollyws this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 1 week ago

@strepanier03, @Ollyws Eep! 4 days overdue now. Issues have feelings too...

Ollyws commented 5 days ago

Still don't think we've got the correct RCA on this one...

jacobkim9881 commented 5 days ago

@Ollyws Could you review my proposal?

Ollyws commented 5 days ago

Ah apologies missed that @jacobkim9881 But I can no longer reproduce this on the latest main. Could anyone else confirm?

https://github.com/user-attachments/assets/440641f0-b750-4a84-a83a-8cfd0a28119c

mvtglobally commented 3 days ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 20 hours ago

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

Ollyws commented 19 hours ago

Given the above I think we can close this one.