Closed lanitochka17 closed 1 month ago
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.
@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
We think that this bug might be related to #vip-vsp
Unable to remove qa.guide@team.expensify.com from workspace chat members.
The BE gives us a successful response when removing it, but shows again after refresh.
I think we should either disable it or not show the qa.guide@team.expensify.com at all like on the workspace members page, https://github.com/Expensify/App/blob/0cb2dc052f01b6a024ac872ad8bf628159fe7873/src/pages/workspace/WorkspaceMembersPage.tsx#L325-L332
but with an additional check of the owner of the report.
if (!PolicyUtils.isExpensifyTeam(reportOwner) && !PolicyUtils.isExpensifyTeam(policyOwner) && !PolicyUtils.isExpensifyTeam(currentUserPersonalDetails.login)) {
If we want to disable it, we can use the condition and apply it here https://github.com/Expensify/App/blob/0cb2dc052f01b6a024ac872ad8bf628159fe7873/src/pages/RoomMembersPage.tsx#L214
If we don't want to show it, we need to use the condition in ReportUtils.getVisibleChatMemberAccountIDs.
so it will affect the member count too.
Unable to remove qa.guide@team.expensify.com from members list in a workspace. The same is the case with removing myself from my workspace. I see this behaviour for any user removal.
Behaviour seen in main
branch HEAD
.
OpenReport
API returns list of members.
Though the RemoveFromRoom
API returns 200 response, refresh of page returns back the removed user in the list.
This needs to be fixed in the backend.
However, proposing frontend fixes below.
Similar the first user in the screenshot below (with a disabled checkbox), owner of the account and qa.guide@team.expensify.com users can be marked disabled, if they are not to be removable.
This is a call the product has to take. Accordingly frontend changes can be implemented.
@Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Hum, what I'm able to reproduce is slightly different - the member doesn't always appear again in the chat but the member count is off. Asking QA to confirm what they see. https://expensify.slack.com/archives/C9YU7BX5M/p1717483892088549
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989
Tester is seeing the same result as in the OP.
https://github.com/Expensify/App/assets/43996225/e9073000-aee4-4b88-b4a9-9be396c607f3
Job added to Upwork: https://www.upwork.com/jobs/~01008c963ec6d29d78
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani (External
)
I think this might need to be internal but we'll start with external. I think this is vsp
I think for the consistency we can go with what @bernhardoj suggest Of not to display the user if it consist Expensify Team's email domain, just like how we did on the workspace members page, but let's see what Internal engineer thinks
@bernhardoj 's Proposal looks good here
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@jayeshmangwani / @Christinadobrzyn : as mentioned in my proposal, this is not limited to qa.guide@team.expensify.com. I see this behaviour for all users. The issue is coming from backend as I have explained.
@jayeshmangwani : i am not sure if your conclusion is final. But the selected solution will not solve this issue.
I see this behaviour for all users
I am not getting this behavior with any other users except the Expensify domain's users, can you add test steps/video of the reproduction for all users ?
i am not sure if your conclusion is final. But the selected solution will not solve this issue.
No, final call will be of @techievivek, And How selected solution will not solve this ? We have added same logic to WorkspaceMembersPage And that's working
Quick question regarding the bug: Are we unable to remove only the team Expensify users and the owner, or can we not remove anyone from the workspace?
You can't actually remove team Expensify users since they are usually assigned for customer support. Similarly, owners can't be removed since they own the policy. If the bug is for these 2 categories of users, then we can just hide them in the front end.
Are we unable to remove only the team Expensify users and the owner, or can we not remove anyone from the workspace?
@techievivek For me, the first one is happening, I am not able to remove the Expensify users.
But @kpadmanabhan mentioned here that I see this behaviour for all users.
, but that is not happening for me, @kpadmanabhan can you add a video of the all users that you can not remove ?
@techievivek Now I am able to reproduce this issue for all the other users too, we are passing the targetAccountIDs to RemoveFromRoom
API and it gives 200 response, then again when we are calling the OpenRoomMembersPage
API, then removed account id still there, First we need to Fix the Issue from BE where OpenRoomMembersPage
should not return the deleted account id then we can go with the hiding the Expensify users from the FE
This is a dupe of https://github.com/Expensify/App/issues/42140, that issue deals with the qa.guide
email showing up in various places and the fact that we should hide it.
Let's reopen the original issue to address this.
cc @techievivek @Christinadobrzyn
@tienifr can you address this comment on the PR https://github.com/Expensify/App/pull/42380#issuecomment-2152149097, its waiting for your action
This is a dupe of https://github.com/Expensify/App/issues/42140, that issue deals with the qa.guide email showing up in various places and the fact that we should hide it.
@techievivek We can move on with this issue, as other issue was for the #announce room and thats not the reproducible right now so that issue was closed and This issue is with workspace chat and #admins room, IMO we should proceed with the selected proposal, what your opinion on this ?
@techievivek We can move on with this issue, as other issue was for the #announce room and thats not the reproducible right now so that issue was closed
@jayeshmangwani IMO that would be unfair because both issues have the same root cause, in the proposals in the original issue, people also highlighted other places in the app where qa.guide
also shows and suggested to apply the same fix to those places.
The root cause and solution are the same, this issue is only another place where the root cause causes problems. Also the solution being accepted here is a dupe of proposals in the other issue. Even if we decide to fix this here, proposals from the other issue should be given priority IMO because it was way before this issue was created.
Also we have more history and context on the original issue.
Let's wait for @techievivek's thoughts.
@jayeshmangwani, @techievivek, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@techievivek what's your suggestion for moving forward with this issue? Should we go with the above proposal, or should we open other issue and close this one ?
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.
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.
@strepanier03 - we are working on this decision - https://github.com/Expensify/App/issues/42928#issuecomment-2159173365
The root cause and solution are the same, this issue is only another place where the root cause causes problems.
Based on this comment: https://github.com/Expensify/App/issues/42928#issuecomment-2151882601, I think we can see this as a new issue where removing any user from the workspace is not working as expected. The concern with the qa.guide@team.expensify.com
and other team Expensify users can be tackled in the other GH that @tienifr has pointed out since it was first reported there.
Since this is now a backend issue, I can look into fixing that, thanks.
@strepanier03 @jayeshmangwani @techievivek @Christinadobrzyn 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!
Issue seems BE and will be handled internally
@strepanier03, @jayeshmangwani, @techievivek, @Christinadobrzyn Huh... This is 4 days overdue. Who can take care of this?
@strepanier03, @jayeshmangwani, @techievivek, @Christinadobrzyn 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
Updated the labels to Internal.
@strepanier03, @jayeshmangwani, @techievivek, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!
@techievivek can you prove an update for us? TY!
I don't think we can allow admins to remove users from workspace chat since it is tied to the workspace and each users on the workspace needs to have a workspace chat.
Backend handles this correctly.
So we can probably hide the remove button in the frontend? what do you think @jayeshmangwani?
Hmm, but even workspace chats have the type set to chat
.
And then I see this comment in the code for RemoveFromRoom
.
// For policy expense chats, do not allow the removal of people who the chat was originally auto-shared with
// (i.e. policy admins/auditors and default approvers)
So based on above I think we should allow removing user members from the workspace chat.
I looked into it further and realized we have a special case for handling the removal of the owner from their workspace chat, as seen in this GitHub PR. However, we don't disable the select box for the owner. Additionally, in the backend, since the command supports removing multiple users from the report, we don't throw an error but instead return a 200 status.
To address this, I suggest disabling the option to remove users from workspace chats in the frontend. In the backend, we already prevent the removal of the workspace chat owner, admin, and auditor. So, let's implement the same restriction in the frontend. Since users in this chat can be either the owner, admin, or auditor, we should altogether disable the remove button for this chat.
CC @jasperhuangg since you worked on the backend changes for this.
@bernhardoj @kpadmanabhan @tienifr Can you please add an updated proposal based on above comments, thanks.
I see that we already ~enable~ disable removing the policy admin here.
That is for preventing the removal no?
Ah yes, sorry, I mean we already disable the removal of policy admin.
Ok, thanks, but we will need to also disable the remove of the workspace chat owner(the user for whom the workspace chat was created).
we need to disable the remove of the workspace chat owner(the user for whom the workspace chat was created).
In https://github.com/Expensify/App/blob/a613c26c0b6af8bb08b6f69a99fc765e7cb897e8/src/pages/RoomMembersPage.tsx#L188, we don't disable the workspace chat owner
Disable the workspace chat owner here by adding
|| (details.accountID && details.accountID === report.ownerAccountID)
If this additional condition should only apply to policy expense chat, we can add && ReportUtils.isPolicyExpenseChat(report)
Since users in this chat can be either the owner, admin, or auditor, we should altogether disable the remove button for this chat.
If we need to go as far as this, we can add || ReportUtils.isPolicyExpenseChat(report)
to here (or can avoid displaying the Button
altogether). If we do this we should also disable all the checkboxes by adding || ReportUtils.isPolicyExpenseChat(report)
here, there's no use for checking the boxes if removal is not possible.
Proposal updated with alternative solution
@techievivek You can still invite people to the workspace chat who weren't in it by default (i.e. you can invite people other than the owner, admins, and auditors). So we still want the ability to remove the people who weren't in the workspace chat by default.
I think @tienifr's proposal to disable the option if we see the accountID matching the workspace chat's owner should suffice.
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.78-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
User should be able to remove a member from the workspace The member count should match the number of members
Actual Result:
When refreshing the browser the user isn't removed from the Members list The member count isn't accurate
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/906149cf-779d-4b36-941e-fc57936a01f7
https://github.com/Expensify/App/assets/51066321/efab0564-171d-4456-ba8d-135b82650f4c
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Christinadobrzyn / @ChristinadobrzynIssue Owner
Current Issue Owner: @strepanier03 / @Christinadobrzyn