Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 8 tasks
lanitochka17 opened this issue Oct 22, 2024 · 35 comments
Open
1 of 8 tasks

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

lanitochka17 opened this issue Oct 22, 2024 · 35 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 22, 2024

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: 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): [email protected]
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?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6642015_1729600729210.bandicam_2024-10-22_15-29-26-242.mp4

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-12-02
Issue OwnerCurrent Issue Owner: @Ollyws
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

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
Copy link
Author

@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 melvin-bot bot added the Overdue label Oct 24, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

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

@strepanier03 strepanier03 added the External Added to denote the issue can be worked on by a contributor label Oct 28, 2024
Copy link

melvin-bot bot commented Oct 28, 2024

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

@melvin-bot melvin-bot bot changed the title Scan - Receipt scanning shown on LHN after scanning is done [$250] Scan - Receipt scanning shown on LHN after scanning is done Oct 28, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 28, 2024
Copy link

melvin-bot bot commented Oct 28, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Oct 28, 2024
@strepanier03
Copy link
Contributor

Yup, reproducible.

image

@FitseTLT
Copy link
Contributor

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

@jacobkim9881
Copy link
Contributor

jacobkim9881 commented Oct 31, 2024

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:

const updateHandler: (updates: OnyxUpdate[]) => Promise<unknown> = request?.data?.apiRequestType === CONST.API_REQUEST_TYPE.WRITE ? QueuedOnyxUpdates.queueOnyxUpdates : Onyx.update;

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:

function applyPusherOnyxUpdates(updates: OnyxUpdateEvent[]) {
pusherEventsPromise = pusherEventsPromise.then(() => {
console.debug('[OnyxUpdateManager] Applying pusher update');
});
pusherEventsPromise = updates
.reduce((promise, update) => promise.then(() => PusherUtils.triggerMultiEventHandler(update.eventType, update.data)), pusherEventsPromise)
.then(() => {
console.debug('[OnyxUpdateManager] Done applying Pusher update');
});
return pusherEventsPromise;
}

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 melvin-bot bot added the Overdue label Oct 31, 2024
Copy link

melvin-bot bot commented Nov 1, 2024

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

Copy link

melvin-bot bot commented Nov 4, 2024

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

@Ollyws
Copy link
Contributor

Ollyws commented Nov 4, 2024

Will get to this one tomorrow.

@melvin-bot melvin-bot bot removed the Overdue label Nov 4, 2024
@muttmuure
Copy link
Contributor

IOU bug, moving to MEDIUM

@muttmuure muttmuure moved this from CRITICAL to MEDIUM in [#whatsnext] #quality Nov 5, 2024
Copy link

melvin-bot bot commented Nov 5, 2024

@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
Copy link
Contributor

Ollyws commented Nov 5, 2024

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
Copy link
Contributor

FitseTLT commented Nov 5, 2024

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
Copy link
Contributor

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
Copy link
Contributor

#51785

Copy link

melvin-bot bot commented Nov 11, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Nov 11, 2024
Copy link

melvin-bot bot commented Nov 11, 2024

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

Copy link

melvin-bot bot commented Nov 13, 2024

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

@Ollyws
Copy link
Contributor

Ollyws commented Nov 13, 2024

Will get to this one asap.

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2024
@jacobkim9881
Copy link
Contributor

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.

Copy link

melvin-bot bot commented Nov 18, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Nov 18, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

Copy link

melvin-bot bot commented Nov 19, 2024

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

@Ollyws
Copy link
Contributor

Ollyws commented Nov 20, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2024
@jacobkim9881
Copy link
Contributor

@Ollyws Could you review my proposal?

@Ollyws
Copy link
Contributor

Ollyws commented Nov 20, 2024

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

Screen.Recording.2024-11-20.at.23.13.03.mov

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Nov 23, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

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

@Ollyws
Copy link
Contributor

Ollyws commented Nov 25, 2024

Given the above I think we can close this one.

@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@melvin-bot melvin-bot bot added the Overdue label Nov 29, 2024
Copy link

melvin-bot bot commented Nov 29, 2024

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

Copy link

melvin-bot bot commented Dec 2, 2024

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

Copy link

melvin-bot bot commented Dec 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue
Projects
Development

No branches or pull requests

7 participants