[Due for payment 2025-02-27] [$250] Workspace - Admin-users are displayed in the #Admins room members list after being deleted

[IuliiaHerets]

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: v9.0.95-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Mac 15.2/Safari
App Component: Workspace Settings

Action Performed:

1. Navigate to https://staging.new.expensify.com
2. Create a workspace, navigate to members
3. Invite members
4. Set some members as admins
5. Click on the top checkbox to select all
6. Click on the top dropdown menu > remove option
7. Click on "Remove"
8. Verify the users are removed from the workspace
9. Navigate to the #Admins room
10. Click on the header and navigate to Members

Expected Result:

Admin-users are not displayed in the #Admins room members list after being deleted from the workspace

Actual Result:

Admin-users are displayed in the #Admins room members list after being deleted from the workspace

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6735801_1738893869509.Recording__580.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021889445311434361818
• Upwork Job ID: 1889445311434361818
• Last Price Increase: 2025-02-11

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

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

[bernhardoj]

Proposal

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

Removed admin is still displayed on the admins room members.

What is the root cause of that problem?

When we remove the member from workspace, the API still return the removed user as the member of the admins report. This is a BE issue. However, on the FE, we also don't optimistically remove the user from the admins room.

We currently only optimistically remove the user from the announce rooms.

App/src/libs/actions/Policy/Member.ts

Line 329 in 33a03bf

const announceRoomMembers = removeOptimisticAnnounceRoomMembers(policy?.id, policy?.name ?? '', accountIDs);

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

This should be fixed on the BE. For the optimistic part, we can create a new function called removeOptimisticAdminRoomMembers similar to removeOptimisticAnnounceRoomMembers. The logic will be the same as removeOptimisticAnnounceRoomMembers except we will get the admin room.

App/src/libs/actions/Policy/Member.ts

Line 245 in 33a03bf

const announceReport = ReportUtils.getRoom(CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE, policyID);

To use removeOptimisticAdminRoomMembers, we will pass a filter accountIDs that only contains admin IDs.

const adminRoomMembers = removeOptimisticAdminRoomMembers(policy?.id, policy?.name ?? '', accountIDs.filter(accountID => {
    const login = allPersonalDetails?.[accountID]?.login;
    return login && policy?.employeeList?.[login]?.role !== CONST.POLICY.ROLE.USER;
}));

This will optimistically mark the member as removed. We will need to add this when removing members (removeMembers) and updating the role.

For role update, we will only remove if we update the role to user/member.

if (newRole === CONST.POLICY.ROLE.USER) {
    const adminRoomMembers = removeOptimisticAdminRoomMembers(policy?.id, policy?.name ?? '', accountIDs.filter(accountID => {
        const login = allPersonalDetails?.[accountID]?.login;
        return login && policy?.employeeList?.[login]?.role !== CONST.POLICY.ROLE.USER;
    }));
    optimisticData.push(...adminRoomMembers.onyxOptimisticData);
    successData.push(...adminRoomMembers.onyxSuccessData);
    failureData.push(...adminRoomMembers.onyxFailureData);
}

I notice that we also don't optimistically add the admin to the admins room. If we would like to do it too, we can again follow a similar logic to buildAnnounceRoomMembersOnyxData. We will need to add this when adding new members and updating the role (admin/auditor).

App/src/libs/actions/Policy/Member.ts

Line 678 in 33a03bf

const announceRoomMembers = buildAnnounceRoomMembersOnyxData(policyID, accountIDs);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can test removeMembers and assert that when removing admin, the admin is removed from the admin room. If we want to add optimistic data when adding admin too, then we need to test addMembersToWorkspace too.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[s77rt]

@bernhardoj Thanks for the proposal. After a member is removed, the BE API response returns the correct report.participants (i.e. the removed members are removed correctly) however since the Onyx method is MERGE, the participants objects are merged and the old values persist. Do you know if this problem occurs on rooms / group chats too? We can follow the same solution

[bernhardoj]

the BE API response returns the correct report.participants (i.e. the removed members are removed correctly)

No, it's not removed correctly. The participant still contains the removed member. That's why BE fix is needed too.

web.mp4

The correct way to remove it is by sending a null data, so it's removed from the onyx.

In a group chat, the API doesn't return the removed member, but it doesn't set it to null either, so it completely relies on the optimistic data.

[s77rt]

@bernhardoj Sending null in OpenReport response does not feel right as the BE will have to keep a track of removed members. The optimistic data is not enough as it only works for the user that initiated the remove call. There should be another mechanism (assuming this works well on group chat).

Can you please try the following:

• Have a group with 3 members at least

1. Login as user A in device A
2. Login as user B in device B
3. As user A, remove user C
4. As user A, verify that member C is removed <-- this should work due to optimistic data
5. As user B, verity that member C is removed <-- this should help us find a solution based on what happens on this case

[bernhardoj]

Sending null in OpenReport response does not feel right as the BE will have to keep a track of removed members

I mean the remove member API.

As user B, verity that member C is removed <-- this should help us find a solution based on what happens on this case

This is handled by the pusher data.

But let say the user isn't active, it will be handled by ReconnectApp.

[s77rt]

🎀 👀 🎀 Internal action required

After removing an admin either by removing them from the workspace DeleteMembersFromWorkspace or changing their role UpdateWorkspaceMembersRole, we should:

1. Return onyx data that clears the removed admins from the #admins room (update participants object)
2. Send push events to other members (same onyx data ^)

This flow is also missing optimistic data, let's assign @bernhardoj for his proposal to fix that.

[melvin-bot]

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

[s77rt]

@iwiznia Users that miss the onyx data that would be sent by the pusher will always get it in the ReconnectApp phase, right?

[iwiznia]

Users that miss the onyx data that would be sent by the pusher will always get it in the ReconnectApp phase, right?

Not quite, if you miss an update, the app will detect it later when it receives any other update and realizes the previousUpdateID returned in the update is higher than the last updateID the app applied.

[aldo-expensify]

I added an admin (the account didn't exist before), then removed it. The request DeleteMembersFromWorkspace returned and error:

{
    "jsonCode": 404,
    "message": "404 Account not found",
    "source": "auth-via-api",
    "onyxData": [],
    "requestID": "9117b026aead4905-YVR"
}

Maybe the bug is related to this? Logs:

https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=10000&query=request_id%3A%229117b026aead4905-YVR%22%20AND%20timestamp%3A%5B2025-02-13T00%3A00%20TO%202025-02-14T23%3A59%5D

If I logout and login, I don't see the removed admin in #admins

[iwiznia]

Hmmmm odd. I tested locally and the command worked, but the user was not unshared from the admins room

[aldo-expensify]

hmm maybe a different bug. Did you add an existing account as an admin?

[FitseTLT]

This is a dupe

[aldo-expensify]

From looking at #53390, it sounds like we are missing an onyx update to remove the admin from #admins?

by "missing" I mean that we are not creating it

[iwiznia]

This is a dupe

I think not? At least from the tests above and the ones I did in dev, the user is never removed from the room.

[aldo-expensify]

So.. from testing a bit more, I see the following:

1. If you remove the admin, you still see it in #admins, but if you log out and log back in, it is not there anymore. This makes me think it is an onyx update problem and the admin is being removed correctly from everywhere.
2. If you look into the onyx updates returned by DeleteMembersFromWorkspace, there is an onyx update removing the admin from the policy, but no onyx update removing the admin from #admins

Onyx updates response from DeleteMembersFromWorkspace

{
    "jsonCode": 200,
    "requestID": "9117d2c50d084905-YVR",
    "onyxData": [
        {
            "key": "report_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "19147050": {
                        "notificationPreference": "always"
                    },
                    "19147090": {
                        "notificationPreference": "always"
                    }
                }
            }
        },
        {
            "key": "personalDetailsList",
            "onyxMethod": "merge",
            "value": {
                "19147090": {
                    "accountID": 19147090,
                    "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_11.png",
                    "displayName": "aldo+testingadmininvite1@expensifail.com",
                    "firstName": "",
                    "lastName": "",
                    "login": "aldo+testingadmininvite1@expensifail.com",
                    "phoneNumber": "",
                    "pronouns": "",
                    "status": null,
                    "timezone": {
                        "automatic": true,
                        "selected": "America\/Vancouver"
                    },
                    "validated": true
                }
            }
        },
        {
            "key": "report_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "lastReadTime": "2025-02-13 21:13:36.959"
            }
        },
        {
            "key": "reportActions_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "9113622837979361014": {
                    "actionName": "CLOSED",
                    "actorAccountID": 19147090,
                    "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_11.png",
                    "created": "2025-02-13 21:13:36.957",
                    "lastModified": "2025-02-13 21:13:36.957",
                    "message": [
                        {
                            "style": "normal",
                            "text": "aldo+testingadmininvite1@expensifail.com closed this report",
                            "type": "TEXT"
                        }
                    ],
                    "originalMessage": {
                        "isNewDot": true,
                        "lastModified": "2025-02-13 21:13:36.957",
                        "policyName": "Expensifail's Workspace",
                        "reason": "removedFromPolicy"
                    },
                    "person": [
                        {
                            "style": "strong",
                            "text": "aldo+testingadmininvite1@expensifail.com",
                            "type": "TEXT"
                        }
                    ],
                    "reportActionID": "9113622837979361014",
                    "shouldShow": true
                }
            }
        },
        {
            "key": "report_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "private_isArchived": "2025-02-13 21:13:36.959"
            }
        },
        {
            "key": "reportNameValuePairs_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "private_isArchived": "2025-02-13 21:13:36.959"
            }
        },
        {
            "key": "reportDraftComment_999762946624637",
            "onyxMethod": "merge",
            "value": null
        },
        {
            "key": "reportDraftCommentNumberOfLines_999762946624637",
            "onyxMethod": "merge",
            "value": null
        },
        {
            "key": "report_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "oldPolicyName": "Expensifail's Workspace"
            }
        },
        {
            "key": "policy_D7846317A49F5F42",
            "onyxMethod": "merge",
            "value": {
                "employeeList": {
                    "aldo+testingadmininvite1@expensifail.com": null
                }
            }
        },
        {
            "key": "report_4488477575519198",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "19147050": {
                        "notificationPreference": "always"
                    }
                }
            }
        },
        {
            "key": "personalDetailsList",
            "onyxMethod": "merge",
            "value": {
                "19147050": {
                    "accountID": 19147050,
                    "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_19.png",
                    "displayName": "aldo+testingadmininvite@expensifail.com",
                    "firstName": "",
                    "lastName": "",
                    "login": "aldo+testingadmininvite@expensifail.com",
                    "phoneNumber": "",
                    "pronouns": "",
                    "status": null,
                    "timezone": {
                        "automatic": true,
                        "selected": "America\/Vancouver"
                    },
                    "validated": true
                }
            }
        },
        {
            "key": "report_4488477575519198",
            "onyxMethod": "merge",
            "value": {
                "lastReadTime": "2025-02-13 21:13:36.965"
            }
        },
        {
            "key": "report_4488477575519198",
            "onyxMethod": "merge",
            "value": {
                "chatType": "policyAdmins",
                "description": "",
                "lastActorAccountID": 19147050,
                "lastMessageText": "removed admin aldo+testingadmininvite1@expensifail.com",
                "lastVisibleActionCreated": "2025-02-13 21:13:36.962",
                "managerID": null,
                "ownerAccountID": 0,
                "parentReportActionID": null,
                "parentReportID": null,
                "policyID": "D7846317A49F5F42",
                "reportID": "4488477575519198",
                "reportName": "#admins",
                "state": "OPEN",
                "stateNum": 0,
                "statusNum": 0,
                "type": "chat",
                "visibility": null
            }
        },
        {
            "key": "reportActions_4488477575519198",
            "onyxMethod": "merge",
            "value": {
                "6125216547896740044": {
                    "actionName": "POLICYCHANGELOG_DELETE_EMPLOYEE",
                    "actorAccountID": 19147050,
                    "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_19.png",
                    "created": "2025-02-13 21:13:36.962",
                    "lastModified": "2025-02-13 21:13:36.962",
                    "message": [
                        {
                            "html": "",
                            "text": "",
                            "type": "COMMENT",
                            "whisperedTo": []
                        }
                    ],
                    "originalMessage": {
                        "accountID": "19147090",
                        "email": "aldo+testingadmininvite1@expensifail.com",
                        "isNewDot": true,
                        "lastModified": "2025-02-13 21:13:36.962",
                        "name": "",
                        "role": "admin"
                    },
                    "person": [
                        {
                            "style": "strong",
                            "text": "aldo+testingadmininvite@expensifail.com",
                            "type": "TEXT"
                        }
                    ],
                    "reportActionID": "6125216547896740044",
                    "shouldShow": true
                }
            }
        },
        {
            "key": "report_8463558605722124",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "19147050": {
                        "notificationPreference": "always"
                    },
                    "19147087": {
                        "notificationPreference": "always"
                    },
                    "19147090": null
                }
            }
        },
        {
            "key": "report_999762946624637",
            "onyxMethod": "merge",
            "value": {
                "oldPolicyName": "Expensifail's Workspace",
                "stateNum": 0,
                "statusNum": 0
            }
        },
        {
            "key": "report_4488477575519198",
            "onyxMethod": "merge",
            "value": {
                "lastActorAccountID": 19147050,
                "lastMentionedTime": null,
                "lastMessageText": "removed admin aldo+testingadmininvite1@expensifail.com",
                "lastReadTime": "2025-02-13 21:13:36.962",
                "lastVisibleActionCreated": "2025-02-13 21:13:36.962",
                "maxSequenceNumber": null,
                "reportID": "4488477575519198"
            }
        },
        {
            "key": "reportActions_4488477575519198",
            "onyxMethod": "merge",
            "shouldNotify": false,
            "value": {
                "6125216547896740044": {
                    "actionName": "POLICYCHANGELOG_DELETE_EMPLOYEE",
                    "actorAccountID": 19147050,
                    "automatic": false,
                    "avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/default-avatar_19.png",
                    "created": "2025-02-13 21:13:36.962",
                    "isAttachment": false,
                    "lastModified": "2025-02-13 21:13:36.962",
                    "message": [
                        {
                            "deleted": "",
                            "html": "removed admin aldo+testingadmininvite1@expensifail.com",
                            "isDeletedParentAction": false,
                            "isEdited": false,
                            "reactions": [],
                            "text": "removed admin aldo+testingadmininvite1@expensifail.com",
                            "type": "COMMENT",
                            "whisperedTo": []
                        }
                    ],
                    "originalMessage": {
                        "accountID": "19147090",
                        "email": "aldo+testingadmininvite1@expensifail.com",
                        "isNewDot": true,
                        "lastModified": "2025-02-13 21:13:36.962",
                        "name": "",
                        "role": "admin"
                    },
                    "person": [
                        {
                            "style": "strong",
                            "text": "aldo+testingadmininvite@expensifail.com",
                            "type": "TEXT"
                        }
                    ],
                    "reportActionID": "6125216547896740044",
                    "reportActionTimestamp": 1739481216962,
                    "sequenceNumber": null,
                    "shouldShow": true,
                    "timestamp": 1739481216,
                    "whisperedToAccountIDs": []
                }
            }
        },
        {
            "key": "policy_D7846317A49F5F42",
            "onyxMethod": "merge",
            "value": {
                "employeeList": {
                    "aldo+testingadmininvite1@expensifail.com": null
                }
            }
        },
        {
            "key": "account",
            "onyxMethod": "merge",
            "value": {
                "guideCalendarLink": null
            }
        }
    ],
    "previousUpdateID": 4191716640,
    "lastUpdateID": 4191719647
}

Unrelated bug: If you have never logged into the admin account, and you remove it, the DeleteMembersFromWorkspace command throws an error, but still the admin is removed from the policy. This is an unrelated bug in my opinion. I don't see it happening if I log into the admin's account before kicking them out of the policy.

I have only tested in staging, not dev for now.

[iwiznia]

I now see why my tests were wrong (had a problem in my VM).
Let's go with @bernhardoj's proposal and we will work on updating the backend.

[iwiznia]

hmmmm I think the problem is that we are using merge instead of set?
For example I just removed accountID 5 of report 7822338802779550 and I do see this update:

        {
            "key": "report_7822338802779550",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "4": {
                        "notificationPreference": "always"
                    }
                }
            }
        },

But that won't ever remove the previous participant.
Does this sound right @aldo-expensify?

[iwiznia]

I think the code is here and since here we get all the participants, what we should do is change it to set instead of merge...
But I am not entirely sure this is the code I want because those updates are only queued for the user's account channel... 😕

[iwiznia]

ok yeah, that was wrong. I am almost sure the code is here which is quite weird as this is very high in the call chain.

[iwiznia]

Oh no, that's for the announce room (that I assume has the same bug, but will test)

[iwiznia]

PR to fix the backend here https://github.com/Expensify/Auth/pull/14185

[aldo-expensify]

hmmmm I think the problem is that we are using merge instead of set?
For example I just removed accountID 5 of report 7822338802779550 and I do see this update:

I would have expected that we would push a merge with:

        {
            "key": "report_7822338802779550",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "5": null
                }
            }
        },

to specifically remove the participant.5. Sounds more like weight that pushing a SET with all participants, no?

[iwiznia]

🤔 that would make sense, but that's not what we do at all....

[aldo-expensify]

🤔 that would make sense, but that's not what we do at all....

Well, if you want to explore that, I think a good place to create the update setting the participant to null is here: https://github.com/Expensify/Auth/pull/14185#pullrequestreview-2618889965

[bernhardoj]

FE PR is ready

cc: @s77rt

[iwiznia]

ok my fix was totally wrong, so back to the drawing board.
I am quite lost on all the updates we queue in this flow. Like I would assume all report related updates when sharing/unsharing would be queued at the lowest level, which would be in queueShareReportOnyxUpdates, but all I see there are updates for the accountID the report is being shared/unshared from, which does not make sense to me because:

1. All users in the report need updates
2. That means that the updates for other users are being queued somewhere else OR totally broken (could be the latter and this is the issue signifying that)

Looking more at the code, I see some updates are here but that's only for the announce room, which I don't see why should have any special logic (ie: this should've been done in queueShareReportOnyxUpdates not in the command).

Now the question is: should I add the update to the lowest level queueShareReportOnyxUpdates and try to remove the ones I see duplicated like the one above for announce room?

[iwiznia]

I see that this was originally added here and it indeed has a question from @flodnv questioning why this is only for announce and not in the lowest level. Seems like the reasoning was: I want a smaller change rather than a big one. @marcaaron what do you think? Should we undo that and move this to queueShareReportOnyxUpdates?

[flodnv]

@marcaaron is on parental leave until April 1st 🙃 I still think it makes more sense to have this at the deepest level.

[iwiznia]

The more I look into the code, the more confused I am. I just discovered Report::queueOnyxUpdatesForInvitedMembers, which also seems to be doing something similar and also not called in the lowest level.

[iwiznia]

I had this PR before I discovered that queueOnyxUpdatesForInvitedMembers but queueOnyxUpdatesForInvitedMembers does a lot more logic, so not sure how to proceed.

[iwiznia]

ReportAction::buildOnyxUpdatesForReportParticipants and Report::getReportParticipantsOnyxUpdate also look quite similar and could be unified somehow.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Optimistically add and remove members from admins room #56895

If no regressions arise, payment will be issued on 2025-02-27. 🎊

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@s77rt @bfitzexpensify @s77rt The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]