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

[HOLD] [$500] Workspace - WS member list when inviting users to workspace and room appears different #31374

Closed
6 tasks done
lanitochka17 opened this issue Nov 15, 2023 · 66 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 15, 2023

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:

  1. Go to staging.new.expensify.com
  2. Go to Settings > Workspaces > any workspace > Members
  3. Click Invite
  4. Go to any room
  5. Click on the room header > Members
  6. Click Invite
    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?

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

Screenshots/Videos

Bug6277597_1700057363150!bandicam_2023-11-15_22-05-36-493

Bug6277597_1700057363166.20231115_135525.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0174fd74e2f19eea53
  • Upwork Job ID: 1724806336848719872
  • Last Price Increase: 2023-11-22
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 15, 2023
@melvin-bot melvin-bot bot changed the title Workspace - WS member list when inviting users to workspace and room appears different [$500] Workspace - WS member list when inviting users to workspace and room appears different Nov 15, 2023
Copy link

melvin-bot bot commented Nov 15, 2023

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Nov 15, 2023

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

Copy link

melvin-bot bot commented Nov 15, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2023
Copy link

melvin-bot bot commented Nov 15, 2023

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

@unidev727
Copy link
Contributor

unidev727 commented Nov 15, 2023

Proposal

from: @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.

_.each(props.report.participantAccountIDs, (accountID) => {

_.each(props.policyMembers, (policyMember, accountIDKey) => {

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

Copy link

melvin-bot bot commented Nov 15, 2023

📣 @unicorndev-727! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@tienifr
Copy link
Contributor

tienifr commented Nov 15, 2023

Proposal

Please 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

setPersonalDetails(_.values(newPersonalDetailsDict));

we create the new newPersonalDetailsDict object that stores personalDetails but it's key is accountID so personalDetails is sorted by accountID

Otherwise, In

setPersonalDetails(inviteOptions.personalDetails);

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

setUsersToInvite(_.values(newUsersToInviteDict));
to support multiple emails as we did in #29044

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

Result

Screen.Recording.2023-12-06.at.18.21.10.mov

@unidev727
Copy link
Contributor

unidev727 commented Nov 15, 2023

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0184e107e8ee954e0d

Copy link

melvin-bot bot commented Nov 15, 2023

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

@barros001
Copy link
Contributor

Proposal

Please 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 newPersonalDetailsDict and later _.values(newPersonalDetailsDict) is called, it sorts the list of users differently.

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.

Copy link

melvin-bot bot commented Nov 15, 2023

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

@unidev727
Copy link
Contributor

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0184e107e8ee954e0d

Copy link

melvin-bot bot commented Nov 15, 2023

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

@unidev727
Copy link
Contributor

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0184e107e8ee954e0d

Copy link

melvin-bot bot commented Nov 15, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Nov 15, 2023

@stephanieelliott Can you assign a new C+ here? Unassigning myself due to low bandwidth.

@Santhosh-Sellavel Santhosh-Sellavel removed their assignment Nov 15, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2023
@stephanieelliott stephanieelliott added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Nov 20, 2023
Copy link

melvin-bot bot commented Nov 20, 2023

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2023
@quinthar
Copy link
Contributor

@mallenexpensify is doing an overhaul of this invite component, so I suggest we pause this.

@quinthar
Copy link
Contributor

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

@cead22 cead22 changed the title [$500] Workspace - WS member list when inviting users to workspace and room appears different [HOLD] [$500] Workspace - WS member list when inviting users to workspace and room appears different Dec 26, 2023
@cead22
Copy link
Contributor

cead22 commented Dec 26, 2023

Updated title to reflect this is on hold

@stephanieelliott
Copy link
Contributor

This is still paused while we settle on a unified design.

Copy link

melvin-bot bot commented Feb 5, 2024

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!

@barros001
Copy link
Contributor

@stephanieelliott Any updates on the unified design?

@stephanieelliott
Copy link
Contributor

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?

@cead22
Copy link
Contributor

cead22 commented Feb 9, 2024

Yes

@barros001
Copy link
Contributor

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

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Feb 12, 2024

Reopening, pending discussion of potential compensation. Internal discussion.

@mananjadhav
Copy link
Collaborator

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

@stephanieelliott
Copy link
Contributor

stephanieelliott commented Feb 12, 2024

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.

@mananjadhav
Copy link
Collaborator

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 🙌

@stephanieelliott
Copy link
Contributor

Yep! Summarizing payment on this issue:

Upwork job is here: https://www.upwork.com/jobs/~019b0016e1e6bb4d02

@JmillsExpensify
Copy link

$500 approved for @mananjadhav based on summary above.

@barros001
Copy link
Contributor

@stephanieelliott accepted, thank you!

@quinthar quinthar moved this from CRITICAL to HIGH in [#whatsnext] #vip-vsb Feb 13, 2024
@stephanieelliott
Copy link
Contributor

All paid, thanks!

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. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests