-
Notifications
You must be signed in to change notification settings - Fork 136
[WOOMOB-1511] - Booking details cancel flow #14752
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a booking cancellation flow for the WooCommerce mobile app, removing the cancelled status from the attendance bottom sheet and adding a confirmation dialog with loading indicator.
- Removes the cancelled attendance status from the bottom sheet options
- Adds a confirmation dialog for booking cancellation with formatted message
- Implements loading state with progress indicator during cancellation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| BookingDetailsViewModelTest.kt | Adds test coverage for cancel dialog functionality and message formatting |
| BookingMapperTest.kt | Tests cancel dialog message generation with customer name fallbacks |
| strings.xml | Adds new strings for cancel dialog UI and removes cancelled status description |
| ic_attendance_cancelled.xml | Removes unused cancelled attendance icon |
| BookingDetailsViewState.kt | Adds cancel dialog state and CancelState sealed interface |
| BookingDetailsViewModel.kt | Implements cancel flow logic with dialog state management |
| BookingDetailsScreen.kt | Integrates cancel dialog component into UI |
| CancelBookingDialog.kt | New dialog component for booking cancellation confirmation |
| BookingAttendanceStatusBottomSheet.kt | Removes cancelled status from attendance options |
| BookingAppointmentDetails.kt | Adds loading indicator and disabled state for cancel button |
| BookingMapper.kt | Adds cancel dialog message formatting logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...merce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewState.kt
Outdated
Show resolved
Hide resolved
| status.iconRes?.let { iconRes -> | ||
| Icon( | ||
| painter = painterResource(iconRes), | ||
| contentDescription = status.text(), | ||
| tint = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| modifier = Modifier.size(24.dp) | ||
| ) | ||
| } |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The nullable icon handling suggests that some attendance statuses may not have icons. Consider adding a consistent icon for all statuses or documenting why certain statuses lack visual representation.
| BookingAttendanceStatus.NO_SHOW -> R.string.booking_attendance_status_no_show_desc | ||
| }.let { stringResource(it) } | ||
| else -> null | ||
| }.let { it?.let { id -> stringResource(id) } ?: "" } |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested let expressions with nullable handling make this code harder to read. Consider simplifying to: ?.let { stringResource(it) } ?: \"\"
| }.let { it?.let { id -> stringResource(id) } ?: "" } | |
| }?.let { stringResource(it) } ?: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the code, AS is wrong here so updated the code.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
26ba718 to
bf3935b
Compare
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14752 +/- ##
============================================
+ Coverage 38.27% 38.30% +0.02%
- Complexity 10010 10019 +9
============================================
Files 2118 2118
Lines 118488 118558 +70
Branches 15824 15831 +7
============================================
+ Hits 45357 45412 +55
- Misses 68917 68928 +11
- Partials 4214 4218 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hichamboushaba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @AdamGrzybkowski 👏
I left one remark about potentially using DialogState for managing the dialog state instead of the custom one we have, please check it out. But I won't block the PR because of it, so I'm pre-approving.
| enabled = model.cancelButtonEnabled, | ||
| ) { | ||
| Text(text = stringResource(R.string.booking_details_cancel_booking_button)) | ||
| if (model.cancelInProgressShown) { | ||
| CircularProgressIndicator( | ||
| color = LocalContentColor.current, | ||
| modifier = Modifier.size(24.dp) | ||
| ) | ||
| } else { | ||
| Text(text = stringResource(R.string.booking_details_cancel_booking_button)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor remark, WDYT about updating WCOutlinedButton to accept a loading parameter like what we do with WCColoredButton here, and move this logic to the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do, please also make sure to update WCColoredButton's implementation to pass enabled = enabled && !loading, to make sure we disable button during loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind, I would prefer to open a separate PR with it just to isolate the change that goes beyond Booking Details screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is here #14768
| val onDismissCancelDialog: () -> Unit = {}, | ||
| val onConfirmCancelBooking: () -> Unit = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using the existing common DialogState here, I mean something like this:
data class BookingDetailsViewState(
val toolbarTitle: String = "",
val bookingUiState: BookingUiState? = null,
val loadingState: BookingDetailsLoadingState = BookingDetailsLoadingState.Idle,
val onCancelBooking: () -> Unit = {},
val onAttendanceStatusSelected: (BookingAttendanceStatus) -> Unit = { _ -> },
val dialogState: DialogState? = null,
val onRefresh: () -> Unit = {},
) {
val shouldShowSkeleton: Boolean = bookingUiState == null && loadingState == BookingDetailsLoadingState.Refreshing
}Then, on the screen, we would use something like the following:
viewState.dialogState?.Render()And we can then remove completely CancelBookingDialog.
One additional advantage of the above is that dialogState will be generic enough so that it can be used for any flow that requires a modal dialog in the details screen.
Also, DialogState accepts UiString, so you won't have to use ResourceProvider:
fun buildCancelDialogMessage(booking: Booking, resourceProvider: ResourceProvider): UiString {
val customerName = booking.order.customerInfo?.fullName()
?: resourceProvider.getString(R.string.customer_detail_guest_customer)
val serviceName = booking.order.productInfo?.name ?: "-"
val date = detailsDateFormatter.format(booking.start)
val time = timeRangeFormatter.format(booking.start)
return UiString.UiStringRes(
R.string.booking_cancel_dialog_message,
listOf(
UiString.UiStringText(customerName),
UiString.UiStringText(serviceName),
UiString.UiStringText(date),
UiString.UiStringText(time)
)
)
}BTW, ResourceProvider doesn't play well with configuration changes, as it uses the app context behind the scenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed the buttons use custom colors in the dialog, so this changes things a bit, but personally I would still vote to use DialogState, we can add a parameter to DialogButton to set the text color, or even a whole ButtonColors, or we can update the Render function to allow customizing the button colors if we want to keep the color logic in the UI.
Also, if we want to match the other dialogs in the app, the dismiss color should use the primary color and not onSurface.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea, I was not aware of the DialogState. The migration is here d0c3f31
I kept the standard colors for now.
| booking, | ||
| bookingUiStateFlow, | ||
| loadingState, | ||
| cancelBookingDialogState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was becoming quite big, so I extracted some of those to separate Flows (booking Ui state, and cancel booking dialog state)
hichamboushaba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AdamGrzybkowski for the changes, nice work 💯
| private val cancelStatusState = MutableStateFlow<CancelStatus>(CancelStatus.Idle) | ||
| private val showCancelBookingDialog = MutableStateFlow(false) | ||
|
|
||
| val cancelBookingDialogState = combine( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this and bookingUiStateFlow private.
| val loadingState: BookingDetailsLoadingState = BookingDetailsLoadingState.Idle, | ||
| val onCancelBooking: () -> Unit = {}, | ||
| val onAttendanceStatusSelected: (BookingAttendanceStatus) -> Unit = { _ -> }, | ||
| val cancelBookingDialogState: DialogState? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just sharing a thought here, no need to change it now if you prefer to keep it for later: I think using a generic name like dialogState here would be more future proof, as if we need any other AlertDialog in the screen, we can then keep the same state, and also we'll ensure we are not trying to show multiple dialogs at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Code updated.

WOOMOB-1511
Description
This PR implements the booking flow as per the designs lxixW6aBBNkCd9ooBK70Z1-fi-1251_19683.
What's included:
What's not included:
Steps to reproduce
Cancel bookingbuttonTesting information
Test Dark/Ligth theme.
The tests that have been performed
The above
Images/gif
Screen_recording_20251014_163517.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.