[HOLD for payment 2022-12-20] Web- Chat - Onyx error is displayed in console when requesting money

[kbecciv]

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

---

Action Performed:

1. Go to staging.new.expensify.com
2. Logged in with applausetester+0901abb@applause.expensifail.com/Feya86Katya
3. Open Console
4. Request money from kbecciv+1103abb1@gmail.com/Feya86Katya

Expected Result:

Onyx error is not displayed in console when requesting money

Actual Result:

Onyx error is displayed in console when requesting money

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.23.7

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Recording.1591.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[johncschuster]

I couldn't reproduce the Onyx error when I followed the repro steps.

Here's what I did:
Go to staging.new.expensify.com
Logged in with applausetester+0901abb@applause.expensifail.com/Feya86Katya
Open Console
Request money from kbecciv+1103abb1@gmail.com/Feya86Katya

And here's the result:

[johncschuster]

@kbecciv Can you try reproducing this again? If you continue to get this behavior, let's see if we can clarify the repro steps.

[kbecciv]

@johncschuster Checking again with latest build, will update you shortly.

[kbecciv]

Issue is still reproducible with build 1.2.23.9

Recording.1612.mp4

[mallenexpensify]

Bumped back to daily @johncschuster , per the new BZ process.

[melvin-bot]

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

[johncschuster]

@amyevans, I'm trying to decide if this can be labeled Internal or External. Do you have a sense of which direction this should go?

[amyevans]

I haven't attempted to reproduce myself yet, but given that it's related to data from the API I think it's best to keep it Internal. Could you try reproducing again @johncschuster and writing up more specific repro steps if successful? I can also take a closer look Wednesday, if not before (but it's getting late for me today and I'm traveling tomorrow, so I don't want to overpromise).

[johncschuster]

Thanks for the buddy check, Amy! I'll label it Internal and will try reproducing the behavior again.

[johncschuster]

I wasn't able to get back to testing this today. I'll carve out some time tomorrow morning.

[amyevans]

I spent some time with this today and can reliably reproduce locally with the following steps:

1. Create two IOU requests
2. Cancel both of them
3. Create another IOU request
4. Observe error in console

To fix the Onyx error (An error occurred while applying merge for key: reportIOUs_XYZ Error: TypeError: Cannot convert undefined or null to object), we need to call the Onyx set method instead of merge in this line. I've opened a PR for that: https://github.com/Expensify/Web-Expensify/pull/35526.

However I'm unsure what to do, if anything, about the second error (Report no longer exists). I see it occurs because we call Report.fetchIOUReportByID() for the optimistic iouReportID which doesn't exist on the server, hence it's expected to be not found (since we're using the other iouReportID that already exists instead). But beyond that level of understanding, I'm getting a bit lost in the code. Hoping maybe @Gonals you can offer an opinion/direction, since it looks like you worked on this refactor originally. Thanks! 🙏

[Gonals]

I haven't dug a lot into the code, but I think the key here is that there's only ever one active IOU report in a chat.
What I believe is happening is that, when we delete the requests, the IOUReport gets deleted in the backend, but the frontend is not informed and keeps trying to use it. This should be the cause of both issues, so I don't think modifying this line is the way to go to fix the underlying problem.

Basically, I believe we are trying to add a Money Request to an IOU report that no longer exists. Chances are we are missing an onyx update the moment we delete the last request

[johncschuster]

Thanks for digging into this @amyevans and @Gonals! In light of what you've found, do you think this is a proper bug in need of fixing?

[Gonals]

I believe so, yes :)

[amyevans]

Yeah, definitely a bug! I'm continuing to dig into it (just didn't have much time on Friday after PR reviews/other chores).

[melvin-bot]

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

[amyevans]

Not overdue, the 2 PRs were just updated and are under review.

[melvin-bot]

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

[mountiny]

Assigned @mananjadhav and @sakluger to manage a payment for this PR which Manan has reviewed and tested. that will be once the PR hits production

[melvin-bot]

@mananjadhav, @sakluger, @amyevans Huh... This is 4 days overdue. Who can take care of this?

[sakluger]

@mountiny @amyevans can you catch me up on who needs to be paid here and how much? I see it's labeled as Internal, so just making sure I handle this properly since I've only ever paid out reporting bonuses on internal issues before.

[mountiny]

@sakluger of course, at the moment we are paying flat rate of $1000 to C+ who review and test our internal PRs. For this issue, we should pay @mananjadhav $1000 for his review and testing 🙇

We just need to make a job for $1000 for Manan, not sure what we usually write into these job descriptions.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.38-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Only generate optimistic IOU report if no existing iouReportID #12759

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter No external issue reporter
• Contributor that fixed the issue Fixed by an internal engineer
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mananjadhav / @amyevans] The PR that introduced the bug has been identified. Link to the PR: Refactor createIOUTransaction to RequestMoney #10843
• [@mananjadhav / @amyevans] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Refactor createIOUTransaction to RequestMoney #10843 (comment)
• [@mananjadhav / @amyevans] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1671123973335579
• [@sakluger] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[sakluger]

@mananjadhav I've created the Upwork post and sent you an offer.

Here's the Upwork post: https://www.upwork.com/ab/applicants/1604898467295248384/job-details (external: https://www.upwork.com/jobs/~011f05def8834dffeb).

[mananjadhav]

Thanks for the invite, accepted the offer

[sakluger]

@amyevans @Gonals do you think this needs a new regression test? And if so, would we add steps to:

• Global Create - Split Bill - IOU Preview test case
• IOU - Cancelling/Declining Requests test case

From reading through the issue, it is an error in the IOU creation, but I think it only happens after you'd deleted a previous request?

[sakluger]

Paid @mananjadhav in Upwork! Just waiting to see if we need to add any new regression tests.

[sakluger]

No regression tests needed since this is just a console error.