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] Split- 'Guest' is counted twice in Public room #43290

Closed
5 of 6 tasks
lanitochka17 opened this issue Jun 7, 2024 · 54 comments
Closed
5 of 6 tasks

[$250] Split- 'Guest' is counted twice in Public room #43290

lanitochka17 opened this issue Jun 7, 2024 · 54 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Hot Pick Ready for an engineer to pick up and run with Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 7, 2024

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: 1.4.80-9
**Reproducible in staging?:**Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4605118
Issue reported by: Applause - Internal Team

Action Performed:

Admin (User A):

'Guest' (User B):

  • Go to an incognito window
  • paste the room URL
  • DO NOT SIGN IN
  • Click the URLs in the chat
  • Verify you need to sign in when you interact with the room

Admin (User A):

  • Check the Room Members to see that there's a Guest

'Guest' (User B):

  • Sign in to the room through the incognito window

Admin (user A):

  • refresh the browser
  • Notice there is still a 'Guest' in addition to the new user

Expected Result:

The 'guest' should be removed as a member of the room since the new user is signed in

Actual Result:

Both the 'guest' and the new user show as members

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

2024-06-26_14-47-50.mp4
20240627_035355.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0175e97fd87039315b
  • Upwork Job ID: 1800039533475604449
  • Last Price Increase: 2024-07-01
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 7, 2024
Copy link

melvin-bot bot commented Jun 7, 2024

Triggered auto assignment to @Christinadobrzyn (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

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

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-split

@devguest07
Copy link
Contributor

Proposal

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

"Guest" appears as a participant when splitting in a public room after logging in via a public room.

What is the root cause of that problem?

When a public room is opened without any logged account, the API sends a report with an extra user ID, labeled as Guest. This Guest becomes part of the report participants and remains even when the user logs in.

A check on the network request reveals an extra user ID in the participantsList from the API.

image

But why is it not displayed as hidden?

When the app is opened and the user is not logged in, the API includes this Guest in the personalDetailsList, making the new Guest a part of the user's contacts. Consequently, it will not be displayed as hidden.

image

As mentioned by anitochka17 in the expected result section, "there will not be an extra 'Guest' member in the participant list". The issue is that this Guest user is not filtered out when there's a split request.

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

The main idea is to filter out the Guest on the MoneyRequestConfirmationList page, preventing this type of user from being part of the split.

This filter can be implemented in various parts of the app, as the process of building the list of split users involves many functions, starting from the creation of the new transaction when clicking on Split Expenses.

I can suggest a way to do this, but I believe the Expensify team may lead this better.

const selectedParticipants = useMemo(() => selectedParticipantsProp.filter((participant) => participant.selected), [selectedParticipantsProp]);

In the selectedParticipants function, we can filter out any Guest user to ensure they are not part of the split. The data for this user is as follows:

image

We can use the firstName: "Guest" and the login ending with expensify.anon to filter out this type of user.

What alternative solutions did you explore?

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Jun 10, 2024
@melvin-bot melvin-bot bot changed the title Split- "Guest" appears as participant when splitting in public room after logging in via public room [$250] Split- "Guest" appears as participant when splitting in public room after logging in via public room Jun 10, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 10, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@Christinadobrzyn

This comment was marked as outdated.

@dominictb
Copy link
Contributor

dominictb commented Jun 10, 2024

Proposal

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

When splitting expense in public room after logging in via public room, there is an extra "Guest" member in participant list

What is the root cause of that problem?

When the user views the public room when logged out, that one will be considered an "anonymous" user from the back-end. And the data related to that anonymous user will be present in personalDetailList and in the report's participants.

However, when the user logs in as a real user, we're not clearing those anonymous client-side only data from Onyx in successData, so the anonymous user still shows up in the split.

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

When the user was anonymous then logs in as a real user, we should clear anonymous client-side only data (personalDetailList, public room participants, ...) from Onyx in successData (or optimisticData)

when the user logs in

There's a couple of endpoints that need the change, depending on if we sign up or sign in, like SignUpUser, SigninUser. ie. in SignUpUser here, first we check if the current user is anonymous by isAnonymousUser(). If the user is anonymous, get the current anonymous user account id by getCurrentUserAccountID, and use it to clear the anonymous account data in personalDetailList, public room participants, ... accordingly.

What alternative solutions did you explore? (Optional)

NA

@Christinadobrzyn
Copy link
Contributor

hi @allroundexperts can you check these proposals when you have a moment?

@Christinadobrzyn Christinadobrzyn removed their assignment Jun 12, 2024
@Christinadobrzyn Christinadobrzyn added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

Triggered auto assignment to @twisterdotcom (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.

@Christinadobrzyn
Copy link
Contributor

Just a heads up - I'm going to be ooo until June 24th so going to assign a teammate to watch this while I'm away

@twisterdotcom proposals are being reviewed

cc @allroundexperts

@Christinadobrzyn Christinadobrzyn self-assigned this Jun 12, 2024
@allroundexperts
Copy link
Contributor

Reviewing today!

@allroundexperts
Copy link
Contributor

When should "guest" be used and not the email address? I'm a little confused on when we name a member "Guest" in a public room. @allroundexperts let me know if that's important for this and if you know (or I should ask)

My understanding is that we show the name as guest until the guest sends a message in the channel. Once the message is there, we show the user name. @twisterdotcom can you confirm this for us? Thanks!

@Christinadobrzyn
Copy link
Contributor

I will ask @allroundexperts I'm not 100% sure either. Asking in our project channel - https://expensify.slack.com/archives/C05RECHFBEW/p1718247545080749

@twisterdotcom could you keep an eye on a response and add it here? TY!

@dominictb
Copy link
Contributor

My understanding is that we show the name as guest until the guest sends a message in the channel. Once the message is there, we show the user name.

@allroundexperts I don't think this is the case, after the user logged in, she's no longer "guest". The "Guest" user is a residual from the anonymous session earlier, as explained in detail in my proposal.

You can validate this by try logging out and logging in again without sending anything to the public room. You'll see the "Guest" user will disappear, although nothing is different about "until the guest sends a message in the channel", no message was sent.

@melvin-bot melvin-bot bot added the Overdue label Jun 14, 2024
@Christinadobrzyn
Copy link
Contributor

@allroundexperts would you be able to take a peek at @devguest07's proposal?

Maybe also checking out this one too to see if it will work with the changed objective

@allroundexperts
Copy link
Contributor

@devguest07 I don't think your proposal would work. You're suggesting to filtering out the guest from the split money expense participant list. In reality, we don't want to show the guest even in the members page of the room (once the guest signs in).

I think this requires a fix from the backend.

@devguest07
Copy link
Contributor

@allroundexperts The issue with guest in the members page of the room has been fixed in the latest main branch. However, there is still a problem with the split functionality, where you will find a guest among the possible split members.

@Christinadobrzyn
Copy link
Contributor

I think this requires a fix from the backend.

@allroundexperts let me know if you think this needs to be an internal label.

@allroundexperts
Copy link
Contributor

@allroundexperts The issue with guest in the members page of the room has been fixed in the latest main branch. However, there is still a problem with the split functionality, where you will find a guest among the possible split members.

Even so, filtering shouldn't be the solution here. We should remove the guest entirely once he signs in.

@allroundexperts
Copy link
Contributor

@allroundexperts let me know if you think this needs to be an internal label.

I think that is the best course here.

@melvin-bot melvin-bot bot added the Overdue label Jul 1, 2024
Copy link

melvin-bot bot commented Jul 1, 2024

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

@Christinadobrzyn Christinadobrzyn added Internal Requires API changes or must be handled by Expensify staff Hot Pick Ready for an engineer to pick up and run with and removed External Added to denote the issue can be worked on by a contributor labels Jul 1, 2024
@Christinadobrzyn
Copy link
Contributor

Sounds good @allroundexperts - I added the Internal label and I'll work on finding a volunteer.

@melvin-bot melvin-bot bot removed the Overdue label Jul 1, 2024
@Christinadobrzyn
Copy link
Contributor

waiting for a volunteer

@Christinadobrzyn
Copy link
Contributor

Looking for a volunteer

@melvin-bot melvin-bot bot added the Overdue label Jul 5, 2024
@Christinadobrzyn
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Jul 8, 2024
@Christinadobrzyn
Copy link
Contributor

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Jul 12, 2024
Copy link

melvin-bot bot commented Jul 15, 2024

@allroundexperts, @Christinadobrzyn Eep! 4 days overdue now. Issues have feelings too...

@Christinadobrzyn
Copy link
Contributor

I'll try to retest this today to see if it's still happening unless you have time to do that @allroundexperts

@melvin-bot melvin-bot bot removed the Overdue label Jul 15, 2024
@Christinadobrzyn
Copy link
Contributor

I'm not going to get to this retest today, I will do it tomorrow

@Christinadobrzyn
Copy link
Contributor

Testing again - I think this is resolved - @allroundexperts would be able to check and see if you're still setting a Guest? I'm not seeing a guest anymore.

@Christinadobrzyn
Copy link
Contributor

I'm going to ask QA if they can retest - https://expensify.slack.com/archives/C9YU7BX5M/p1721241144696699

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

1721031579106.bandicam_2024-07-15_11-15-19-949.mp4

@Christinadobrzyn
Copy link
Contributor

Let's close this - the issue seems to be resolved.

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 Help Wanted Apply this label when an issue is open to proposals by contributors Hot Pick Ready for an engineer to pick up and run with Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

9 participants