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

BUG: Rooms should not show split bill #22283

Closed kbecciv closed 1 year ago

kbecciv 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:

  1. Login with any account
  2. Create a workspace
  3. Create a room
  4. Click on plus option

Expected Result:

Split bill option should not appear

Actual Result:

The split bill option appears

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.36-2 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

Outdated https://github.com/Expensify/App/assets/93399543/882654a9-110f-4671-becc-fb3adc6de798

Screencast from 04-07-2023 19_08_57.webm

Expensify/Expensify Issue URL: Issue reported by: @dukenv0307 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688472505039959

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0102e4e3f83c0c685e
  • Upwork Job ID: 1678602294688821248
  • Last Price Increase: 2023-07-11
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)

thienlnam commented 1 year ago

https://expensify.slack.com/archives/C049HHMV9SM/p1688579070022409?thread_ts=1688472505.039959&cid=C049HHMV9SM

This may need to be internal

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

Was able to reproduce, added the Internal label, per Jack's rec

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

Adding Engineering otherwise I don't think an engineer would see it.

mallenexpensify commented 1 year ago

@Santhosh-Sellavel can you confirm you're able to reproduce? If so, what are you thoughts on next steps @neil-marcellini ?

Santhosh-Sellavel commented 1 year ago

@mallenexpensify You confirmed here already

mallenexpensify commented 1 year ago

Yeah, I was able to reproduce @Santhosh-Sellavel , for some reason I wanted bonus 👀 , can't remember exactly why.

Santhosh-Sellavel commented 1 year ago

@mallenexpensify Yeah I can confirm able to reproduce when login via phone number

Screenshot 2023-07-19 at 7 10 27 AM

But this can be reproduced when accessing the public room via email itself

https://github.com/Expensify/App/assets/85645967/b90bdba6-3aff-43fd-92e2-91491fe8881a

melvin-bot[bot] commented 1 year ago

@mallenexpensify @neil-marcellini @Santhosh-Sellavel this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

neil-marcellini commented 1 year ago

I'm working on a few other internal issues right now, so I'll have to get to this later.

neil-marcellini commented 1 year ago

There are still other higher priority issues that I'm working on instead of this.

neil-marcellini commented 1 year ago

I took a bit of a look today. Yep @dukenv0307 is right that the back end is returning report.participants as an empty array when you're logged in with a phone number. If you're logged in with an email then it returns a list of participant logins.

Next time I'll look into OpenReport and see what's going on. I think we should probably be using participantAccountIDs instead due to the refactor to hide login info. cc @Beamanator @puneetlath do we have any other plans to fix this elsewhere / do you guys have any advice. I want to make sure I'm not working on a duplicate issue.

Santhosh-Sellavel commented 1 year ago

@neil-marcellini Check here I have logged in with email also facing same issue

neil-marcellini commented 1 year ago

Oh interesting. I don't have that problem but maybe because I'm logging in with an email using the account that created the room. The bug has nothing to do with a mobile number then. Please investigate further if you can and nail down the root of the problem.

Santhosh-Sellavel commented 1 year ago

I've logged in with a new account that has nothing to do with the room, nor the account interacted in the room

melvin-bot[bot] commented 1 year ago

@mallenexpensify @neil-marcellini @Santhosh-Sellavel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

neil-marcellini commented 1 year ago

Heads up, I'm going to be OOO Monday and Tuesday next week. I'll ask in Slack if anyone can take over these issues while I'm out.

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @neil-marcellini, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 1 year ago

@neil-marcellini is out today/tomorrow, he'll look into once back

melvin-bot[bot] commented 1 year ago

@mallenexpensify @neil-marcellini @Santhosh-Sellavel this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

puneetlath commented 1 year ago

cc @Beamanator @puneetlath do we have any other plans to fix this elsewhere / do you guys have any advice. I want to make sure I'm not working on a duplicate issue.

Not that I'm aware of. We are working on removing report.participants so that only report.participantAccountIDs remains. And I believe that for public rooms, the participantAccountIDs array will always be empty for non-admins (just as an FYI).

neil-marcellini commented 1 year ago

Thanks for checking and letting me know.

I believe that for public rooms, the participantAccountIDs array will always be empty for non-admins

Oh interesting. Why? The split bill option only appears when there are multiple participants I believe, so how should we fix that? If there's some reason we can't return the the participantAccountIDs array, then maybe we need to return some other data that provides a count of the number of participants.

puneetlath commented 1 year ago

Oh interesting. Why?

Because people in public rooms aren't supposed to know who else is in the room for privacy reasons. I believe only admins of the workspace (or maybe all workspace members) are allowed to know that. So for anyone else, the participantAccountIDs array will be empty.

Out of curiosity, what purpose does the split bill option serve in rooms? Perhaps an alternate approach would be to have it be there in DMs/Group-DMs and just leave that out of rooms altogether?

neil-marcellini commented 1 year ago

Out of curiosity, what purpose does the split bill option serve in rooms? Perhaps an alternate approach would be to have it be there in DMs/Group-DMs and just leave that out of rooms altogether?

Yes great point, it doesn't really make sense for rooms. When you split a bill with a group it automatically splits it among all members. I don't think that's really the desired behavior for rooms, so I agree let's only add the split bill menu item for groups. I don't think it makes sense for DMs because there's no one to split with.

puneetlath commented 1 year ago

I think DMs still make sense. You and I could still split a bill (e.g. I pay for lunch, scan the receipt, and you owe me for half).

I agree on rooms though. Maybe we should validate in #product just to be sure?

neil-marcellini commented 1 year ago

haven't had time

neil-marcellini commented 1 year ago

I haven't had time. I'm prioritizing distance requests above this.

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @neil-marcellini, @Santhosh-Sellavel 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

mallenexpensify commented 1 year ago

@neil-marcellini will be looking at soon

neil-marcellini commented 1 year ago

I might have time for this today.

neil-marcellini commented 1 year ago

I've been focused on distance requests trying to get it done before the deadline.

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @neil-marcellini, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

mallenexpensify commented 1 year ago

@neil-marcellini will address once distance requests is wrapped

neil-marcellini commented 1 year ago

I updated the issue title and description based on the direction we're going for.

neil-marcellini commented 1 year ago

I put up a draft PR. A few more tweaks and it will be ready.

neil-marcellini commented 1 year ago

The PR is up for review, but we're still debating whether it's the right approach in Slack.

neil-marcellini commented 1 year ago

We're going to merge the split bill option into request money eventually, with teachers unite I think, and once that happens split bill won't show for public rooms and this will be solved. Also this is super edge case and we don't need to worry about it for now.

dukenv0307 commented 1 year ago

@neil-marcellini Should we have a payment for the reporter here?