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 for payment 2021-11-04] Allow inviting multiple users to a workspace by reusing the OptionsSelector component #4775

Closed
marcaaron opened this issue Aug 21, 2021 · 56 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Planning Changes still in the thought process Weekly KSv2

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Aug 21, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

When accessing the "Invite People" page we should be able to choose users from a list of our contacts (personalDetails) similar to how the Bill Split page works maybe?

2021-08-20_14-37-22

I'm not totally sure how we want this design to work yet and could use some feedback from design.

cc @JmillsExpensify @MitchExpensify

Actual Result:

We can only add one user at a time.

Screen Shot 2021-08-20 at 2 32 05 PM

Workaround:

Can invite multiple people in the current design by separating the emails by a comma. This feature is not very clear as documented by @Santhosh-Sellavel in #4668

Platform:

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on GitHub

@marcaaron marcaaron added Planning Changes still in the thought process Design labels Aug 21, 2021
@MelvinBot
Copy link

Triggered auto assignment to @michelle-thompson (Design), see these Stack Overflow questions for more details.

@Santhosh-Sellavel
Copy link
Collaborator

Just commenting here to follow this discussion!

@kakajann
Copy link
Contributor

I can work on this if you decide how the user interface should look. I have experience with OptionsSelector and OptionsListUtils.js in #2818

@MitchExpensify
Copy link
Contributor

Big fan of being consistent and using the same invite design as "New Group" for workspace invites. One small note is that you can invite multiple people in the current design by separating the emails by a comma so the "workaround" and "actual result" sections are a little off. I still think we should 100% make this change

@MitchExpensify
Copy link
Contributor

this seems like a good external candidate. Please share any proposals here and we'll get you hired for the Upwork job cc @kakajann @Santhosh-Sellavel

@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Sep 6, 2021
@MelvinBot MelvinBot added the Weekly KSv2 label Sep 6, 2021
@MitchExpensify
Copy link
Contributor

MitchExpensify commented Sep 6, 2021

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 6, 2021
@MelvinBot
Copy link

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

@Santhosh-Sellavel
Copy link
Collaborator

Initial Proposal

We could reuse the code from IOUParticipantsSplit.js

In WorkspaceInvitePage which is currently.

Need to modify few lines as per our need here.

Let's pause here.

@MitchExpensify
We need to clear few things before a head start:

  1. What's the flow now. The invite page is a single or multiple page?
  2. Need a design with copies to understand the flow required.
  3. Is there any limit to the total no of workspace users?

@Santhosh-Sellavel
Copy link
Collaborator

One clarification will I get a bonus for this since issue #4668 which I raised was closed. And we are proceeding with this one. And changes I suggest in there are already implemented now.
Screenshot 2021-09-07 at 8 44 19 AM

Thanks!

@Julesssss
Copy link
Contributor

We need to await the design before accepting any proposals here. CC @michelle-thompson

One problem I see is that our previous usages of the OptionsSelector Component extend to the full page. In this case, we might need to reduce the height to include the personal message field. Another problem is that the component will extend infinitely as the user selects users to invite, further hiding the personal message field.


@Santhosh-Sellavel

  1. What's the flow now. The invite page is a single or multiple page?
  2. Need a design with copies to understand the flow required.
  3. Is there any limit to the total no of workspace users?

Thanks for the clarifying questions. There is no workspace user limit. We'll need to hold on to answering the other questions until the design is finalized.

@Julesssss
Copy link
Contributor

Julesssss commented Sep 7, 2021

One clarification will I get a bonus for this since issue #4668 which I raised was closed. And we are proceeding with this one.

@Santhosh-Sellavel I think you have a reasonable claim to that bonus, but I don't think this is the right issue to claim the bonus on. Your initial proposal was to include a hint in the input field and as this was added as a part of this issue I think you should raise this point there instead.

@Julesssss Julesssss removed the Monthly KSv2 label Sep 7, 2021
@shawnborton
Copy link
Contributor

Before we get this on Upwork, I think we should nail down the design first. Does anyone agree?

@Julesssss Julesssss removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 7, 2021
@kakajann
Copy link
Contributor

kakajann commented Sep 7, 2021

Just a suggestion. 2 screens would be good to solve this issue.

1st screen:

  • OptionsSelector to select users.
  • Next button.

2nd screen

  • Preview selected Users.
  • Personal message input.
  • Invite button.

@shawnborton
Copy link
Contributor

I can grab this one quickly @michelle-thompson

I think it would be better if we could still keep the message and the user selection all on one screen... that being said, not sure if this feels a bit awkward but here is what we would have:
image

Ideally the textarea would grow/shrink depending on the message in it too to save some space. Thoughts?

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Sep 9, 2021

Hey @marcaaron - it looks like this issue is related to what was initially reported here by Santhosh.

Can you confirm I have this correct, this issue relates to fixing what was originally reported by Santhosh?

If that's the case, then we'll want to pay Santhosh for originally reporting this issue (I will start that process). Thanks!

@trjExpensify
Copy link
Contributor

@trjExpensify Thanks for the points. I am currently following the design attached but I will update it, if needed.

Yeah, cool. Jules and I chatted about this briefly and here's where I think we've landed:

  • Filter the list to not show already invited workspace members (and therefore the need for the error message becomes obsolete).
  • Further, if the user happens to type the full email address/number of an invited workspace member it returns no search results (like typing your own email address in split bills today).
  • the privacy policy link needs to be added back as per that internal thread. Where should that go, underneath the optional message box still? CC: @shawn
  • inviting members from here when offline works pretty nice today in the app and we should retain that (let's make sure to test). The one issue today is that the invite page doesn't close when clicking invite and the lack of feedback makes it a tad confusing. With this change, I suspect we're returning to the members list after clicking invite, right? So that's good upside to take care of that one.

@shawnborton
Copy link
Contributor

Ugh that privacy link... maybe it can go under the button so it's the bottom-most thing on the page? I'm not quite sure what else to do with it...

@parasharrajat
Copy link
Member

Is it decided to place the Privacy link below the button? If so, Will it be left-aligned? cc: @shawnborton .

And, please do look at the PR #5726 and let me know if you feel some improvements are needed.

@Julesssss
Copy link
Contributor

@parasharrajat would you mind placing the privacy link left-aligned under the button and sharing a screenshot?

@parasharrajat
Copy link
Member

Will be done ASAP.

@Julesssss
Copy link
Contributor

@shawnborton, would you mind reviewing the screenshot from the PR here which shows the privacy link, thanks!

@parasharrajat
Copy link
Member

Also, Can some send me the offer for this job? The job was closed a few days back.

@Julesssss
Copy link
Contributor

Also, Can some send me the offer for this job? The job was closed a few days back.

CC @MitchExpensify

@mallenexpensify
Copy link
Contributor

Reposted the job
https://www.upwork.com/jobs/~01efc6bdfcc984aaa3
@parasharrajat can you apply and confirm here once ya have?

@parasharrajat
Copy link
Member

@mallenexpensify Done.

@parasharrajat
Copy link
Member

Well. there is an issue with my PR. It is not excluding all the emails.

const EXPENSIFY_EMAILS = [
    CONST.EMAIL.CONCIERGE,
    CONST.EMAIL.CONTRIBUTORS,
    CONST.EMAIL.FIRST_RESPONDER,
    CONST.EMAIL.HELP,
    CONST.EMAIL.QA,
    CONST.EMAIL.CHRONOS,
    CONST.EMAIL.RECEIPTS,
    CONST.EMAIL.BILLS,
    CONST.EMAIL.STUDENT_AMBASSADOR,
    CONST.EMAIL.QA_TRAVIS,
    CONST.EMAIL.SVFG,
];

const EXCLUDED_GROUP_EMAILS = [
    CONST.EMAIL.CONTRIBUTORS,
    CONST.EMAIL.FIRST_RESPONDER,
    CONST.EMAIL.HELP,
    CONST.EMAIL.QA,
    CONST.EMAIL.CHRONOS,
    CONST.EMAIL.BILLS,
    CONST.EMAIL.STUDENT_AMBASSADOR,
    CONST.EMAIL.QA_TRAVIS,
    CONST.EMAIL.SVFG,
];

const EXCLUDED_IOU_EMAILS = [
    CONST.EMAIL.CONCIERGE,
    CONST.EMAIL.CONTRIBUTORS,
    CONST.EMAIL.FIRST_RESPONDER,
    CONST.EMAIL.HELP,
    CONST.EMAIL.QA,
    CONST.EMAIL.CHRONOS,
    CONST.EMAIL.RECEIPTS,
    CONST.EMAIL.BILLS,
    CONST.EMAIL.STUDENT_AMBASSADOR,
    CONST.EMAIL.QA_TRAVIS,
    CONST.EMAIL.SVFG,
    CONST.EMAIL.INTEGRATION_TESTING_CREDS,
];

There are multiple sets here. Which one do I need to exclude? @marcaaron

@marcaaron
Copy link
Contributor Author

😦 hmm I am not too sure. But I think mainly you should not be able to invite the EXPENSIFY_EMAILS

It is not excluding all the emails.

What do you mean by this? Which emails are not being excluded?

@parasharrajat
Copy link
Member

CONCIERGE , it was not part of EXCLUDED_GROUP_EMAILS. Ok, I will send follow-up.

@marcaaron
Copy link
Contributor Author

Ok I'm also not sure what EXCLUDED_GROUP_EMAILS is used for... 😬 it looks very similar but I will review the PR

@mallenexpensify
Copy link
Contributor

Hired @parasharrajat in Upwork

@MitchExpensify
Copy link
Contributor

thanks for grabbing while I was at SuiteWorld @mallenexpensify! I can pick this up again from here

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 28, 2021
@botify botify changed the title Allow inviting multiple users to a workspace by reusing the OptionsSelector component [HOLD for payment 2021-11-04] Allow inviting multiple users to a workspace by reusing the OptionsSelector component Oct 28, 2021
@botify
Copy link

botify commented Oct 28, 2021

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

If no regressions arise, payment will be issued on 2021-11-04. 🎊

@Christinadobrzyn
Copy link
Contributor

Since this PR is in prod for 7 days and we can pay now - I paid Santosh for his proposal of this issue per this GH #4668

@parasharrajat
Copy link
Member

Ping for
image

@mallenexpensify
Copy link
Contributor

Paid @parasharrajat in Upwork, closed the job, closing this now too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Planning Changes still in the thought process Weekly KSv2
Projects
None yet
Development

No branches or pull requests