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

Updated logic to not allow attendee deselection from bill split confirmation page. #5895

Merged
merged 3 commits into from
Oct 18, 2021

Conversation

PrashantMangukiya
Copy link
Contributor

@Julesssss pr is ready for review.

Details

If bill split flow initiated via FAB, we will Not Allow attendee deselection on bill split confirmation page.
If bill split flow initiated from Group Chat we will Allow attendee deselection on bill split confirmation page.

Discussion #5295 (comment) and #5295 (comment)
Confirmation: #5295 (comment)

Fixed Issues

$ #5295

Tests | QA Steps

QA Test1: Bill-split init via FAB

  1. Click FAB + > Split bill
  2. Enter amount and click next
  3. It will show attendee select screen, So select two or more attendees and click next
  4. You will land at confirmation page.
    So in this case it Should Not Allow to deselect attendees on confirmation page.

QA Test2: Bill-split init via Group Chat

  1. Select Group Chat,
  2. Click + button at left of text box in group chat.
  3. Select Split Bill (for Group chat it shows Split Bill. We need to test Split Bill for group chat).
  4. Enter amount and click next
  5. It will directly land at confirmation page.
    So in this case it Should Allow to deselect attendees.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Web.mov

Mobile Web

MobileWeb.mov

Desktop

Desktop.mov

iOS

iOS.mov

Android

Android.mov

@PrashantMangukiya PrashantMangukiya requested a review from a team as a code owner October 15, 2021 18:29
@MelvinBot MelvinBot requested review from Julesssss and removed request for a team October 15, 2021 18:29
@parasharrajat
Copy link
Member

@shawnborton A question. Do we really need to show the checkboxes on the confirmation page if we can't deselect the users in the FAB flow?

@@ -65,6 +65,9 @@ const propTypes = {
phoneNumber: PropTypes.string,
})).isRequired,

/** Amount split belongs to group or not */
Copy link
Member

Choose a reason for hiding this comment

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

NAB: It can be improved. e.g. Whether this is an IOU split and belongs to a group report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat thanks, I will update instruction as per your suggestion asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes done. thanks.

parasharrajat
parasharrajat previously approved these changes Oct 15, 2021
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM & tests well.

cc: @Julesssss ready for final review and merge when n6-hold is lifted.

@@ -40,6 +40,9 @@ const propTypes = {

/** IOU type */
iouType: PropTypes.string,

/** Amount split belongs to group or not */
Copy link
Member

Choose a reason for hiding this comment

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

This can be changed similarly to the above comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also changed.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM. Tests well.

Ready for final review and merge when n6 is lifted. cc: @Julesssss

@Julesssss Julesssss merged commit d7edd80 into Expensify:main Oct 18, 2021
@Julesssss
Copy link
Contributor

Julesssss commented Oct 18, 2021

@parasharrajat FYI, N6 hold is lifted now

@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 by @Julesssss in version: 1.1.8-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

4 participants