search

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.56k stars 2.9k forks source link

[$1000] Unexpected error while splitting money- After task history- mobile number #22031

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!

Action Performed:

Pre-requisite: There must be a task history over user A and user B. Please assign task from User A to User B and send some messages. User B must be a phone number. 1 Login as User A and click on + button and click on split money 2 Set amount and description and select User B

  1. Split money and verify if you get error

    Expected Result:

    There should be no error in splitting money with registered users.

    Actual Result:

    Got these errors : “There is a previously existing chat between these users.” “Auth GetEmailByAccountID returned an error” and “Unexpected error requesting money, please try again later” I got error with both registered and unregistered mobile numbers

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.35-5 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/7bb244ec-72db-4845-9591-ea47ed8752ed

https://github.com/Expensify/App/assets/43996225/f0dee343-751c-44e7-bd8c-126d53f3ae14

Expensify/Expensify Issue URL: Issue reported by: @avi-shek-jha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688104145295479

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016b1c3dd2f7daeaad
  • Upwork Job ID: 1676297367694114816
  • Last Price Increase: 2023-07-25
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @abdulrahuman5196 (Internal)

mallenexpensify commented 1 year ago

Updated OP to make it clear at the beginning that User B needs to be a phone number. I was able to reproduce,

image

@abdulrahuman5196 , can you also test to see if you're able to reproduce then propose if this should be internal or external? thx

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

Will check and update in the morning

abdulrahuman5196 commented 1 year ago

@mallenexpensify I was able to reproduce this issue. I think there should be some issue in what report Id we are using for split in frontend, since it is not expected to create a new report since we already have a chat with the user created by the task. We can open it for external, there could be some internal work. But the investigation place to start is in frontend.

bernhardoj commented 1 year ago

Same root cause with https://github.com/Expensify/App/issues/22433

melvin-bot[bot] commented 1 year ago

Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

mallenexpensify commented 1 year ago

Made External, dropped a comment on https://github.com/Expensify/App/issues/22433 for Kadie to keep tabs on this, thanks for the comment @bernhardoj

abdulrahuman5196 commented 1 year ago

I think both the issues are related from the root cause from @bernhardoj. I will review the proposal here and re-check if the solution solves both the issues and update back. Meanwhile, @bernhardoj could you kindly post your proposal here as well specific to this issue?

bernhardoj commented 1 year ago

Cross-posting my proposal from https://github.com/Expensify/App/issues/22433 with changes specific to this issue.

Proposal

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

Split bill with a user after assigning a task to that user will fail the split bill. This could happen with request money too.

What is the root cause of that problem?

Assigning a task to a user will create a task report. The task report participants will be a single-item array that contains the assigned user account ID.

When we are trying to split bill, it will iterate over all split bill participants and find the participant report. https://github.com/Expensify/App/blob/a2dc004c6342c3b2e1e9a61bdc40b99da4fb647c/src/libs/actions/IOU.js#L602

The problem lies in getChatByParticipants. It returns the wrong report. https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/libs/ReportUtils.js#L2117-L2125

getChatByParticipants iterates over all reports and compares the participantAccountIDs array. It will find the first occurrences that have the same participantAccountIDs array. Currently, it ignores a thread report and policy room report (isUserCreatedPolicyRoom). However, there is some report that potentially has the same participantAccountIDs, for example, task report.

If you assign user A to a task, the task report participantAccountIDs will be [userAId]. This means, now we have 2 reports with the same participantAccountIDs. The normal (1:1) chat and the task report. In case the task report has a smaller report ID than the 1:1 chat (it's important because report onyx is sorted by id), getChatByParticipants will return the task report instead of the 1:1 chat.

Because the task report does not have iouReportID, it will create a new one. https://github.com/Expensify/App/blob/a2dc004c6342c3b2e1e9a61bdc40b99da4fb647c/src/libs/actions/IOU.js#L608-L615

And because we already have an active IOU, the error is thrown.

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

We should add some exclusion to other reports.

if (... || isTaskReport(report) || isMoneyRequestReport(report) || isChatRoom(report) || isPolicyExpenseChat(report)) return;

This will fix everything that depends on this function, e.g. request, split money, etc.

melvin-bot[bot] commented 1 year ago

@mallenexpensify @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!

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@abdulrahuman5196 , please review @bernhardoj 's proposal above

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@abdulrahuman5196 , please review @bernhardoj 's proposal above, it's been 5 days. https://github.com/Expensify/App/issues/22031#issuecomment-1635998348

abdulrahuman5196 commented 1 year ago

Sorry for the delay. Reviewing the proposal now.

abdulrahuman5196 commented 1 year ago

On @bernhardoj 's proposal here https://github.com/Expensify/App/issues/22031#issuecomment-1635998348

I did a dive deep and I am not able to see getChatByParticipants causing issues in this particular case.

For instance I kept a debugger at getChatByParticipants, it was only called on this line but for that too getChatByParticipants return undefined and was never called again in split money flow. But still the issue occured for me. So I think the root cause is somewhere else for this particular issue in hand.

https://github.com/Expensify/App/blob/a2dc004c6342c3b2e1e9a61bdc40b99da4fb647c/src/libs/actions/IOU.js#L456-L458

https://github.com/Expensify/App/assets/46707890/7a8087f4-a8aa-4ede-a328-50386abb62ca

abdulrahuman5196 commented 1 year ago

We don't have any other suitable proposals yet here.

bernhardoj commented 1 year ago

I see. This issue is probably specific to phone number users where getChatByParticipants fails to find the existing chat, thus returning undefined.

Screenshot 2023-07-20 at 12 35 16

Looking at the image above, the user in LHN and in split bill has a different avatar and display name, which is really weird.

My proposal above will fix the case where getChatByParticipants returns the wrong report, which happens to users with an email. I can't continue investigating the phone number case because every time I start a new chat with a phone number user, BE will always return an empty participant to the report, idk why.

bernhardoj commented 1 year ago

I finally have an explanation for why @abdulrahuman5196 has this issue https://github.com/Expensify/App/issues/22031#issuecomment-1642599910.

As I mentioned in my comment above, the avatar and display name looks different and they indeed have a different account ID. This issue happens when we assign/split bill a task to a user that we never chat before. How? Here is what happens:

  1. Assign a task to user A that we never chat before, let's say the accountID is ID_A. At this point, we don't have their login yet. This is expected as part of the secure login project.
  2. Open Split Bill. The first time, you won't see user A on the list and you need to manually type their login, either the phone number or email.
  3. When you type the email, it will optimistically generate a new accountID, which could be different from the real accountID that user A has, let's say the generated accountID is ID_B.
  4. Confirm the split bill. At this point, getChatByParticipants can't find any chat with ID_B as the participant, thus we create a new report/chat.
  5. Because we actually already have a chat with user A, BE throws us an existing report error

So, yes my proposal is not valid in case of a split bill with a user that we don't have their login yet, which is what this issue is about.

melvin-bot[bot] commented 1 year ago

@mallenexpensify @abdulrahuman5196 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!

abdulrahuman5196 commented 1 year ago

@mallenexpensify I think there is an explanation for why the proposal was not working - https://github.com/Expensify/App/issues/22031#issuecomment-1645159927

So now, we don't have a proposal yet. I think we need to increase the bounty.

@bernhardoj Kindly feel free to propose a different solution if you have already found the cause if possible.

bernhardoj commented 1 year ago

I think it's best for the internal team to handle this one.

Currently, we are filtering out users without login. https://github.com/Expensify/App/blob/82669ae1201f3778986ee26209fb80878f166cbd/src/libs/OptionsListUtils.js#L642-L646

This code is the one that caused the 2nd and 3rd point here

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

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

melvin-bot[bot] commented 1 year ago

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

Thanks!

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

There hasn't been any proposal. Will check on the above comment to see if something could change in internal from backend to fix this.

abdulrahuman5196 commented 1 year ago

@mallenexpensify @bernhardoj 's analysis seems to be correct. We need to check how was case expected to work with the secure login change. Can we involve an internal engineer to check on this query? This is the account information present in onyx

Screenshot 2023-08-01 at 9 48 31 PM

This is the data present in split api call being made, the accountID is different

Screenshot 2023-08-01 at 9 50 04 PM
melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

@mallenexpensify Could you kindly check on this?

abdulrahuman5196 commented 1 year ago

Pinged in slack

abdulrahuman5196 commented 1 year ago

Need an internal engineer on this

🎀 👀 🎀

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

abdulrahuman5196 commented 1 year ago

@tgolen Could you kindly check on this https://github.com/Expensify/App/issues/22031#issuecomment-1660665502?

tgolen commented 1 year ago

Rather than assigning an internal engineer to a GH when you have a question like that, can you please post it in slack in the #expensify-open-source channel and tag @engineering-team, please? That way more eyes can see it and maybe get you an answer faster/better.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@abdulrahuman5196 , can you please post your comment above in the thread for the bug report (and be sure to check the box to share with the room so you get more 👀 ) https://expensify.slack.com/archives/C049HHMV9SM/p1688104145295479

If you don't receive a reply in a couple days, please post the same to #expensify-open-source

Thx

abdulrahuman5196 commented 1 year ago

I had already posted in open source channel days back, missed to paste the reference here https://expensify.slack.com/archives/C01GTK53T8Q/p1692383613454819

But still no update @mallenexpensify

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

mallenexpensify commented 1 year ago

Thanks @abdulrahuman5196 , I posted again and tagged the @engineering-team, to see if that helps. I'm guessing you're unable to. Let's see if we get some 🎣

abdulrahuman5196 commented 1 year ago

This needs to be on hold as discussed here https://expensify.slack.com/archives/C01GTK53T8Q/p1693412993417589?thread_ts=1692383613.454819&cid=C01GTK53T8Q

cc: @puneetlath @mallenexpensify

mallenexpensify commented 1 year ago

Put on Hold, will add issue number once it's dropped in the thread. https://expensify.slack.com/archives/C01GTK53T8Q/p1693818288598599?thread_ts=1692383613.454819&cid=C01GTK53T8Q

mallenexpensify commented 1 year ago

Checking again for issue link https://expensify.slack.com/archives/C01GTK53T8Q/p1694416424585289?thread_ts=1692383613.454819&cid=C01GTK53T8Q