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

Deselect attendees when splitting bill #3881

Merged
merged 34 commits into from
Aug 3, 2021
Merged

Deselect attendees when splitting bill #3881

merged 34 commits into from
Aug 3, 2021

Conversation

rushatgabhane
Copy link
Member

@rushatgabhane rushatgabhane commented Jul 3, 2021

Progress so far.

  • Toggle between selected and unselected participants.
  • The amount is split between selected participants only.
  • Disable button when no participants is selected.
  • New group shouldn't be created for selected participants.
  • (This will be handled by API layer. See Reference) Add the split comment to the group DM with selected participants only.

Details

Comment
To check if a split happening from a group DM, props.route.params.reportID is checked.

Fixed Issues

$ #3841

Tests / QA

  1. Login to E.cash
  2. Go to a group DM
  3. Click on +, then split bill.
  4. Enter amount and go next.
  5. Verify that you can unselect 'who was there'.
  6. Verify that the amount is split correctly.
  7. After splitting bill. Verify that bill is split only among the selected participants.
  8. Verify that no new group is created.
  9. Verify that split comment in group DM only lists the selected participants.

Tested On

Don't have a Mac to test iOS, Desktop.

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

newWebVideo.mp4

Mobile Web

Desktop

iOS

Android

newAndroid.mp4

@rushatgabhane rushatgabhane marked this pull request as ready for review July 7, 2021 07:51
@rushatgabhane rushatgabhane requested a review from a team as a code owner July 7, 2021 07:51
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team July 7, 2021 07:51
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.

This looks really good 👍

Great work so far, and I'm eager to discuss this solution with you further in the comments.

src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Show resolved Hide resolved
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.

This is looking great! I know we've done several rounds of reviews, but I think it's really gonna show in the end. Thanks for hanging in there!

src/components/IOUConfirmationList.js Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
@rushatgabhane
Copy link
Member Author

This is looking great! I know we've done several rounds of reviews, but I think it's really gonna show in the end. Thanks for hanging in there!

Actually, thanks for your time!
Yes.

@Luke9389
Copy link
Contributor

OK @rushatgabhane, go ahead and test this branch on Android, Web, and Mobile Web. I'll test Desktop and iOS on your behalf. Though, I do highly recommend you set up a mac vm to test iOS changes in the future. The chance of us hiring contributors who do not have an iOS testing environment in the future will continue to decrease.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Just passing through to leave some quick style notes.

src/pages/iou/IOUModal.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved
@rushatgabhane
Copy link
Member Author

OK @rushatgabhane, go ahead and test this branch on Android, Web, and Mobile Web. I'll test Desktop and iOS on your behalf. Though, I do highly recommend you set up a mac vm to test iOS changes in the future. The chance of us hiring contributors who do not have an iOS testing environment in the future will continue to decrease.

Yes, thank you!
Will setup a Mac VM for my next PRs for testing.

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Jul 13, 2021

I've tested this branch. And updated the screenshots for web, android, and mWeb.

@Luke9389
Copy link
Contributor

OK! Just tested on iOS and desktop. Lookin' good. 👍

Luke9389
Luke9389 previously approved these changes Jul 13, 2021
@Luke9389
Copy link
Contributor

It looks like there are now merge conflicts here. I've approved the code here, and I'll be out of office next week, so perhaps @marcaaron can give the final approve and merge once the conflicts are addressed.

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Jul 24, 2021

Apologies for the forced-push! I had messed up the commit history.

Merge conflicts are fixed and tested ✔

@Luke9389 Luke9389 removed the request for review from marcaaron August 3, 2021 00:13
@Luke9389 Luke9389 merged commit 22e7f0a into Expensify:main Aug 3, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Aug 3, 2021

✋ 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

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 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.

4 participants