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

Added multiple emails invite support to workspace invite page #4068

Merged
merged 6 commits into from
Jul 21, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 15, 2021

Details

#4029 (comment)

Fixed Issues

$ #4029

Tests | QA Steps

1 Login to expensify.cash using an account under freePlan beta
2. Create a workspace via Global create (i.e. the green + icon)
3. Go to Settings and access the Workspace
4. Go to People tab
5. Select "Invite"
6. Attempt to invite multiple valid email addresses and separate them by comma
7. You should be able to invite multiple persons as once.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

invite-w.mp4

Mobile Web

invite-m.mp4

Desktop

iOS

Android

invite-a.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner July 15, 2021 05:03
@MelvinBot MelvinBot requested review from timszot and removed request for a team July 15, 2021 05:04
@parasharrajat
Copy link
Member Author

Some open questions #4029 (comment)

@parasharrajat parasharrajat changed the title [WIP] Added multiple emails invite support to workspace invite page Added multiple emails invite support to workspace invite page Jul 17, 2021
@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 17, 2021

Questions:
When any of the input logins is invalid phone backend throws an error but it does not tell which login so we have to delete all the entered logins.

What would be the correct behavior here?

  1. Remove the wrong login only. Need backend changes. I think the backend does not add even the correct logins if any one of them is wrong.
  2. Or remove all invited as currently backend does.

@parasharrajat parasharrajat changed the title Added multiple emails invite support to workspace invite page [WIP] Added multiple emails invite support to workspace invite page Jul 17, 2021
@timszot
Copy link
Contributor

timszot commented Jul 19, 2021

cc @TomatoToaster since you're working on the OG issue with proposed changes and there are some questions here.

@TomatoToaster
Copy link
Contributor

TomatoToaster commented Jul 20, 2021

@parasharrajat goood catch and good question!

I think removing all of them is fine for this PR In the future, I can imagine we might eventually modify the backend to fix the does not add even the correct logins if any one of them is wrong, but it feels like it wouldn't be a common issue. I'll bring it up though and maybe we'll create a follow up for this.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@TomatoToaster
Copy link
Contributor

Oh dang it, I bet this happens now because we changed the Repo name. I guess we better get this out of the way.

@TomatoToaster
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA

@parasharrajat
Copy link
Member Author

@TomatoToaster Ok. I will finalize the PR for review.

@TomatoToaster
Copy link
Contributor

recheck

@parasharrajat parasharrajat changed the title [WIP] Added multiple emails invite support to workspace invite page Added multiple emails invite support to workspace invite page Jul 20, 2021
@parasharrajat
Copy link
Member Author

Ready for review.

@timszot timszot merged commit 0c532ed into Expensify:main Jul 21, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico
Copy link

isagoico commented Jul 24, 2021

iOS - Invite people - Comma not displayed in keyboard to send multiple invites

Action Performed

  1. Launch the app and login with expensifail account
  2. Tap + > New workspace
  3. Enter name for workspace , tap Get started
    4.Tap People > Invite
  4. Enter email

Expected Result

Comma displayed to invite multiple email or email and phone number at once

Actual Result

No comma displayed in keyboard. Reproducible in iOS only. Android is working as expected.

Notes/Images/Video
Device and OS: iPhone Xs / 14.6 and iPhone 6s

comma.mp4

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label Jul 24, 2021
@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 25, 2021

This can be fixed by changing the type of keyboard keyboardType={getEmailKeyboardType()} to keeping default for all platforms. But I would like to ask if this is how to want to tackle it. We already have the validations in place.

@TomatoToaster
Copy link
Contributor

@parasharrajat I think that's a good idea. You're right that since we have the email validation already in place, we don't need to restrict the keyboard.

@TomatoToaster
Copy link
Contributor

Removing the deploy blocker since this is still a beta feature and I've got a PR to fix it up.

@TomatoToaster TomatoToaster removed the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat parasharrajat deleted the work-invite branch November 20, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants