Expensify
[$250] Workspace - Non-admin is not instantly removed from #admins member list after being demoted
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.69-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team
Action Performed:
- Go to staging.new.expensify.com
- Go to workspace settings > Members.
- Invite a member and promote the member to admin.
- Go to #admins room.
- Click on the chat header > Members.
- Note that the new admin is in the member list.
- Go to workspace settings > Members.
- Demote the admin to member.
- Go to #admins room.
- Click on the chat header > Members.
Expected Result:
The non-admin should now be removed from #admins room and will not appear in member list in #admins room.
Actual Result:
The non-admin still appears in member list in #admins room after being demoted to member. After resetting the app in Troubleshoot (or relogin), the non-admin is removed from member list in #admins.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [x] Android: Standalone
- [x] Android: HybridApp
- [x] Android: mWeb Chrome
- [x] iOS: Standalone
- [x] iOS: HybridApp
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
https://github.com/user-attachments/assets/d88dba00-38bd-4d8d-b391-7bdc0d06c04b
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~021866294583392794294
- Upwork Job ID: 1866294583392794294
- Last Price Increase: 2024-12-10
Issue Owner
Current Issue Owner: @allgandalf
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.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspace - Non-admin is not instantly removed from #admins member list after being demoted
What is the root cause of that problem?
We are neither adding a user to the admin room nor removing them when setting or unsetting a user's role to admin optimistically here
https://github.com/Expensify/App/blob/64829131fe962681626b4b016140521d05f7fe52/src/libs/actions/Policy/Member.ts#L452-L466
and although the BE return participants removing the key for the user onyx merge will not be able to remove the existing key for the member hence we need to clear cache to see the member removed from the members of the admin room
What changes do you think we should make in order to solve the problem?
We will retrieve the admin room of the policy from allReports with the condition that a report is isAdminRoom and the policyID is the same as the current policy and then update the report.participants list accordingly by removing the user-account ids (passed to updateWorkspaceMembersRole function) who's roles are being removed from admin (or vice versa when it is being upgraded to admin role)
We will get the current role from the policy and then compare it with the role being set and determine whether it is being set as admin or being removed from admin role (when being set to admin we might not need to know the prev role) and then update report.participants object of the admin room accordingly : the structure something similar to
"user_accountId": {
"notificationPreference": "always"
}
on the removing case we should specifically set the participants key for the member to null so that onyx merge will successfully remove it.
And we will revert on failureData
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
For tests: we currently have tests for Member.updateWorkspaceMembersRole here
https://github.com/Expensify/App/blob/64829131fe962681626b4b016140521d05f7fe52/tests/actions/PolicyMemberTest.ts#L107-L117
but we are only asserting for the employeeList of the policy.
Now we can create a policy admin room report (setting explicitly the policyID, 'participantsandchatTypesome how similar to [here](https://github.com/FitseTLT/App/blob/64829131fe962681626b4b016140521d05f7fe52/tests/actions/PolicyMemberTest.ts#L36-L39)) and set in onyx and then we will add more assertions asserting that the admin room of the policy has the correctparticipants` list accordingly for both cases where admin role is set and unset
What alternative solutions did you explore? (Optional)
Proposal
Please re-state the problem that we are trying to solve in this issue.
suggestions for the code that might help address the issue where a non-admin is not immediately removed from the #admins room member list after being demoted.
What is the root cause of that problem?
A lack of immediate UI synchronization after the user's role is updated in the backend.
-
Backend Delay in Propagation: When a user is demoted, the backend may update the user's role, but the changes are not reflected immediately in the UI (specifically in the #admins room member list). This could be due to the frontend not receiving the updated data after the role change.
-
UI State Not Refreshing: The member list UI does not automatically refresh or update when the user's role is changed. This could be because the frontend is not actively fetching or listening for the updated role list or because the UI is not reacting to role change events in real-time.
-
Missing Real-Time Update Mechanism: If the app does not have a real-time synchronization mechanism (such as WebSockets or a polling mechanism) to notify the frontend about the role change, the frontend remains unaware of the updated role until a more substantial update (like a reset or relogin) occurs.
using useState and useEffect
import { useEffect, useState } from 'react';
// State to manage member list
const [adminMembers, setAdminMembers] = useState([]);
// This function would be called when a user's role is changed
const handleRoleChange = (userId, newRole) => {
// Perform the role change logic (API call, etc.)
api.demoteUser(userId, newRole)
.then(() => {
// After demotion, update the member list
refreshAdminList(); // Re-fetch the list of admins
})
.catch((error) => console.error("Error demoting user:", error));
};
// Fetch updated admin list after role change
const refreshAdminList = async () => {
const updatedMembers = await api.getAdminMembers(); // Replace with actual API call
setAdminMembers(updatedMembers);
};
// Effect to initialize admin members on mount
useEffect(() => {
refreshAdminList();
}, []);
return (
<div>
<h3>Admin Members</h3>
<ul>
{adminMembers.map(member => (
<li key={member.id}>{member.name}</li>
))}
</ul>
</div>
);
Use of WebSockets or Event Listeners for Real-Time Updates
// Event listener for role change
socket.on('roleChanged', (userId, newRole) => {
if (newRole !== 'admin') {
// Remove the user from the admin list UI
setAdminMembers(prevMembers => prevMembers.filter(member => member.id !== userId));
} else {
// Add the user to the admin list UI
fetchUpdatedAdminList();
}
});
Triggering UI Refresh on Role Change (In Vanilla JavaScript or jQuery)
// Function to handle role change
function demoteUser(userId) {
// Assuming a function that performs the role demotion (API call)
api.demoteUser(userId)
.then(() => {
// Refresh the member list after demotion
refreshAdminList();
})
.catch((error) => {
console.error("Failed to demote user:", error);
});
}
// Function to refresh the admin member list
function refreshAdminList() {
api.getAdminMembers().then((updatedMembers) => {
const adminListContainer = document.getElementById('adminList');
adminListContainer.innerHTML = ''; // Clear current list
updatedMembers.forEach(member => {
const memberItem = document.createElement('li');
memberItem.textContent = member.name;
adminListContainer.appendChild(memberItem);
});
});
}
// Trigger UI refresh after role change
document.getElementById('demoteBtn').addEventListener('click', () => {
const userId = getUserIdToDemote(); // Fetch the user ID to demote
demoteUser(userId);
});
Handle Immediate Role Removal from UI (For Example with API Polling)
// Poll for updates every 5 seconds (or longer)
setInterval(async () => {
const members = await api.getAdminMembers();
updateAdminListUI(members);
}, 5000);
// Function to update the UI
function updateAdminListUI(members) {
const listElement = document.getElementById('adminList');
listElement.innerHTML = ''; // Clear current list
members.forEach(member => {
const li = document.createElement('li');
li.textContent = member.name;
listElement.appendChild(li);
});
}
Backend Example (Node.js with Socket.IO):
socket.on('demoteUser', async (userId) => {
try {
// Perform the role demotion in your backend
await demoteUserRole(userId);
// Emit an event to notify all connected clients of the role change
io.emit('roleChanged', userId, 'member'); // Replace with the actual role
} catch (error) {
console.error("Error demoting user:", error);
}
});
@strepanier03 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Job added to Upwork: https://www.upwork.com/jobs/~021866294583392794294
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allgandalf (External)
@strepanier03, @allgandalf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@strepanier03 @allgandalf 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!
okay @FitseTLT your solutions seems plausible, but before moving forward i would love to test it a bit, can you please share a test branch?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@strepanier03, @allgandalf Whoops! This issue is 2 days overdue. Let's get this updated quick!
@strepanier03, @allgandalf 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@strepanier03, @allgandalf Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!
@strepanier03 @allgandalf this issue is now 4 weeks old, please consider:
- Finding a contributor to fix the bug
- Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
- If you have any questions, don't hesitate to start a discussion in #expensify-open-source
Thanks!
@strepanier03, @allgandalf 12 days overdue. Walking. Toward. The. Light...
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Proposal from @FitseTLT LGTM! there RCA is correct and solution would work
Their test branch is here, just in case you want to see the changes @mjasikowski 🎀👀🎀 C+ reviewed
Triggered auto assignment to @mjasikowski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
This issue has not been updated in over 15 days. @mjasikowski, @strepanier03, @FitseTLT, @allgandalf eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
@allgandalf Bump on this one and @mjasikowski Can you help turn the label back to weekly? Thx
Reviewing label has been removed, please complete the "BugZero Checklist".