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

Exclude concierge from iou #2740

Merged
merged 5 commits into from
May 11, 2021
Merged

Exclude concierge from iou #2740

merged 5 commits into from
May 11, 2021

Conversation

tugbadogan
Copy link
Contributor

@tugbadogan tugbadogan commented May 7, 2021

@trjExpensify

Details

Exclude Concierge from IOU participant selection and remove IOU menu items from chat with Concierge.

Fixed Issues

Fixes #2698

Tests

Tested manually and made sure that users cannot initiate IOU request with Concierge.

QA Steps

- Open 1:1 and group chat with Concierge
- Press `+` icon next to chat composer
- Observe that there is no `Request Money` or `Split Bill` option on menu
- Open 1:1 and group chat without Concierge
- Press `+` icon next to chat composer
- Observe that there is a `Request Money` or `Split Bill` option on menu
- Press big green `+` icon on the sidebar
- Select `Request Money` or `Split Bill`
- Enter a random amount and press `Next`
- Search Concierge
- Observe that Concierge doesn't appear among results

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Desktop

Screenshot 2021-05-07 at 17 13 19 | Screenshot 2021-05-07 at 17 13 50
Screenshot 2021-05-07 at 17 14 23 | Screenshot 2021-05-07 at 17 14 59
Screenshot 2021-05-07 at 17 15 43 | Screenshot 2021-05-07 at 17 16 20

@tugbadogan tugbadogan requested a review from a team as a code owner May 7, 2021 17:13
@MelvinBot MelvinBot requested review from cead22 and removed request for a team May 7, 2021 17:13
@trjExpensify trjExpensify requested a review from Luke9389 May 7, 2021 17:49
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tugbadogan. Would you mind also adding this functionality to the unit tests for getNewGroupOptions? Otherwise lookin' great 👍

@tugbadogan
Copy link
Contributor Author

Hi @tugbadogan. Would you mind also adding this functionality to the unit tests for getNewGroupOptions? Otherwise lookin' great 👍

Hi, I added unit tests for getNewChatOptions() and getNewGroupOptions() 👍

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! This is lookin' great!
Thanks for adding in these tests @tugbadogan.

Just a couple of questions, and a few NABs (Not a Blocker).
Comment back on whether you want to address the review comments, excluding default values is not something that I'd require (since they make the code a tad more informative). Still though, I wouldn't fault you if you were to take them out.

I do however need an answer on whether we default excludeConcierge to true. I see that you're using some syntactic sugar when you pass it to getNewGroupOptions and I just want to be sure I'm reading that right.

Thanks!

@tugbadogan
Copy link
Contributor Author

Thanks for the quick review @Luke9389 , I've addressed the comments and updated the PR

@Luke9389
Copy link
Contributor

Dang! Super speedy 🏎

@Luke9389
Copy link
Contributor

Luke9389 commented May 11, 2021

Obligatory question after review rounds: you've tested this on all 5 platforms?
I see screenshots for Desktop but not for the others.

@tugbadogan
Copy link
Contributor Author

Obligatory question after review rounds: you've tested this on all 5 platforms?
I see screenshots for Desktop but not for the others.

Yes, I tested this on all platforms. I didn't put screenshots for other platforms since there's no UI element or platform specific change here.

@Luke9389
Copy link
Contributor

Ok cool 👍

I've got some internal stuff to do before I can merge this but I'll be back later today. Thanks for all the quick response times here!

@Luke9389 Luke9389 merged commit 90b5942 into Expensify:main May 11, 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.42-1🚀

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

@Jag96
Copy link
Contributor

Jag96 commented May 12, 2021

It looks like this PR caused a regression here: #2809

Reverting this PR in #2834 to unblock the deploy, and reopening the associated issue

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

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

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.

[IOU] Users are able to send payment requests and split bills with Concierge
5 participants