-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD] [$500] Workspace - WS member list when inviting users to workspace and room appears different #31374
Comments
Triggered auto assignment to @stephanieelliott ( |
Job added to Upwork: https://www.upwork.com/jobs/~0174fd74e2f19eea53 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Proposalfrom: @unicorndev-727 Please re-state the problem that we are trying to solve in this issue.The list of members in the workspace when inviting members to the workspace and to the room should be the same What is the root cause of that problem?The root cause is that RoomMembers filter report.participantAccountIDs and Workspaces filter policyMembers. App/src/pages/RoomMembersPage.js Line 188 in 48782c7
What changes do you think we should make in order to solve the problem?We need to adjust these two values. What alternative solutions did you explore? (Optional)N/A |
📣 @unicorndev-727! 📣
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Workspace - WS member list when inviting users to workspace and room appears different What is the root cause of that problem?In WorkspaceMembersPage and RoomInvitePage we did the different ways to get the personalDetails In
we create the new Otherwise, In App/src/pages/RoomInvitePage.js Line 97 in 0a4ca7b
inviteOptions.personalDetails is already sorted by name What changes do you think we should make in order to solve the problem?For usersToInvite we should did the same as
for personalDetails and selectedOptions, we should do the same as RoomInvitePage to preserve the order Detail implementation - const [userToInvite, setUserToInvite] = useState(null);
+ const [userToInvite, setUserToInvite] = useState([]);
...
useEffect(() => {
+ const newUsersToInviteDict = {};
const inviteOptions = OptionsListUtils.getMemberInviteOptions(props.personalDetails, props.betas, searchTerm, excludedUsers);
// Update selectedOptions with the latest personalDetails information
const detailsMap = {};
_.forEach(inviteOptions.personalDetails, (detail) => (detailsMap[detail.login] = OptionsListUtils.formatMemberForList(detail, false)));
const newSelectedOptions = [];
_.forEach(selectedOptions, (option) => {
newSelectedOptions.push(_.has(detailsMap, option.login) ? {...detailsMap[option.login], isSelected: true} : option);
});
+ const userToInvite = inviteOptions.userToInvite;
+ // Only add the user to the invites list if it is valid
+ if (userToInvite) {
+ newUsersToInviteDict[userToInvite.accountID] = userToInvite;
+ }
+ setUserToInvite(_.values(newUsersToInviteDict));
setPersonalDetails(inviteOptions.personalDetails);
setSelectedOptions(newSelectedOptions);
// eslint-disable-next-line react-hooks/exhaustive-deps -- we don't want to recalculate when selectedOptions change
}, [props.personalDetails, props.betas, searchTerm, excludedUsers]);
because we change userToInvite from object to list -> we need to update everywhere we use userToInvite Apply the same logic for WorkspaceInvitePage ResultScreen.Recording.2023-12-06.at.18.21.10.mov |
Contributor details |
|
ProposalPlease re-state the problem that we are trying to solve in this issue.The list of users looks different when inviting users to a workspace vs inviting users to a room. They are sorted differently What is the root cause of that problem?https://github.com/Expensify/App/pull/29044/files This PR introduced a way to invite multiple users to a workspace. By doing that, in changed the way the list of users is handled: // Only add the user to the invites list if it is valid
if (userToInvite) {
newUsersToInviteDict[userToInvite.accountID] = userToInvite;
}
// Add all personal details to the new dict
_.each(inviteOptions.personalDetails, (details) => {
newPersonalDetailsDict[details.accountID] = details;
});
// Add all selected options to the new dict
_.each(newSelectedOptions, (option) => {
newSelectedOptionsDict[option.accountID] = option;
});
...
...
...
setPersonalDetails(_.values(newPersonalDetailsDict)); When the users are pushed into RoomInvitePage.js does not do that, although it also allow to invite multiple users. What changes do you think we should make in order to solve the problem?We have to lists that does pretty much the exact same thing with duplicate logic: Both allow to invite multiple users but the implementation varies. My proposal is to extract the logic into a shared hook and use it in both invite pages so that logic is shared and we avoid this type of problem in the future. What alternative solutions did you explore? (Optional)This is mostly a sorting issue. Another approach is to explicitly sort the list by name/email/telephone so that regardless of the logic, the list would still look the same. This approach would be the easiest and less risky. Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
|
Contributor details |
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@stephanieelliott Can you assign a new C+ here? Unassigning myself due to low bandwidth. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
@mallenexpensify is doing an overhaul of this invite component, so I suggest we pause this. |
Let's pause work on this until we settle having a unified design here: https://expensify.slack.com/archives/C01GTK53T8Q/p1703557793897069?thread_ts=1703360093.158969&cid=C01GTK53T8Q |
Updated title to reflect this is on hold |
This is still paused while we settle on a unified design. |
This issue has not been updated in over 15 days. @cead22, @barros001, @mananjadhav, @stephanieelliott, @tienifr 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! |
@stephanieelliott Any updates on the unified design? |
The actual design has not yet been settled, but we have agreed to standardize to a singular Workspace > Members design that will apply basically everywhere -- when creating a split or group, adding/removing members from a workspace, room, group or thread, etc. That Workspace > Members design is going to be implemented as part of Wave 8: Simplified Collect (internal doc link). So seeing as this is going to be overhauled soon anyway, I think we should close this issue and not make any changes to the current behavior in the meantime. @cead22 what do you think, do you agree? |
Yes |
@stephanieelliott In cases like this, both contributor (in this case myself and @tienifr) and the C+ (@mananjadhav) are still eligible for payment for the work completed. This [have happened to me before(https://github.com//issues/29972) and according to this comment, that's what the procedure says. Can you look into this please? |
Reopening, pending discussion of potential compensation. Internal discussion. |
@mallenexpensify My recommendation is we payout for the issue. We did review the proposals, went through some discussions both the contributors engaged, and also created the PR. I did partial review of the PR. It was put on hold before I could finish the review, and hence I didn't work on the checklist. |
Thanks for raising this @mananjadhav and @barros001 (and @mallenexpensify for kickstarting the discussion). Closing this without issuing payment was an oversight, and I agree that this should be paid as normal due to the amount of work that was done. This is also in line with our internal documentation on contributor payment structure here. |
No worries @stephanieelliott. Would you be able to post the payout summary? For the contributors, the amount needs to be split between @barros001 and @tienifr. thank you 🙌 |
Yep! Summarizing payment on this issue:
Upwork job is here: https://www.upwork.com/jobs/~019b0016e1e6bb4d02 |
$500 approved for @mananjadhav based on summary above. |
@stephanieelliott accepted, thank you! |
All paid, thanks! |
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: 1.3.99-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Note that the list in these two invite pages is different
Expected Result:
The list of members in the workspace when inviting members to the workspace and to the room should be the same, and in both, members should be displayed in alphabetical order
Actual Result:
The list of members in the workspace when inviting members to the workspace and to the room is different
The list when inviting members to the workspace used to appear the same as the list when inviting members to room
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6277597_1700057363166.20231115_135525.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: