-
Notifications
You must be signed in to change notification settings - Fork 133
[WOOMOB-1346] Booking details - payment section #14666
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
base: trunk
Are you sure you want to change the base?
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 adds a payment section to the booking details screen with payment information display and action buttons for marking payments as paid/refunded and viewing the associated order.
Key changes:
- Added payment section UI component with payment breakdown (service, tax, discount, total)
- Implemented "View order" navigation functionality
- Added "Mark as paid" and "Mark as refunded" buttons with state management
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
BookingPaymentSection.kt | New composable component for displaying payment details and action buttons |
BookingDetailsViewState.kt | Added payment model and callback functions for payment actions |
BookingDetailsViewModel.kt | Implemented payment status update logic and callback handling |
BookingDetailsScreen.kt | Integrated payment section into booking details UI |
BookingDetailsFragment.kt | Added navigation to order details screen |
nav_graph_bookings.xml | Added navigation action to order details |
strings.xml | Added payment section related string resources |
BookingDetailsViewModelTest.kt | Updated test to use new state structure |
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
Show resolved
Hide resolved
...ommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/BookingPaymentSection.kt
Outdated
Show resolved
Hide resolved
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
if (paymentStatus == BookingPaymentStatus.PAID) { | ||
WCOutlinedButton( | ||
onClick = onMarkAsRefunded, | ||
colors = ButtonDefaults.outlinedButtonColors( | ||
contentColor = MaterialTheme.colorScheme.onSurface | ||
), | ||
text = stringResource(id = R.string.booking_payment_mark_as_refunded), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) | ||
} else { | ||
WCColoredButton( | ||
onClick = onMarkAsPaid, | ||
text = stringResource(id = R.string.booking_payment_mark_as_paid), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) | ||
} | ||
WCOutlinedButton( | ||
onClick = onViewOrder, | ||
colors = ButtonDefaults.outlinedButtonColors( | ||
contentColor = MaterialTheme.colorScheme.onSurface | ||
), | ||
text = stringResource(id = R.string.booking_payment_view_order), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) |
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.
Those buttons are in a Column spread by 16.dp. The thing is, it appears as if there's more space because the WCButton
has extra space around to ensure the min height of 48.dp.
I've looked at other screens, and this is how the buttons in a column look, so I kept it like this. If you think we should align with the design, we could:
- Remove the
spacedBy
from the column and handle spacing between children individually. - Set the min height to 48.dp for the buttons and keep the
spacedBy
The above will align with the design, but will misalign with some other places in the app with two buttons in a column.
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 was in the same situation before. 😄 When adding buttons in RemoveShipmentBottomSheet, I set 8.dp
between the buttons, which results in a total of 16.dp
.
I don’t have a strong opinion, but my preference would be to do the same as I did in RemoveShipmentBottomSheet
and keep it aligned with the design. I’ll leave the decision to you.
thickness = 0.5.dp, | ||
modifier = Modifier.padding(start = 16.dp) | ||
) | ||
if (paymentStatus == BookingPaymentStatus.PAID) { |
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.
Not 100% this is the correct logic. Should we also show the Mark as refunded
if the booking status is completed
?
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 asked about it in Slack (p1759326540162229-slack-C09FHQNQERG). If we don’t get a reply, we can add a TODO comment to update this logic later and merge the PR now.
📲 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 #14666 +/- ##
============================================
- Coverage 38.41% 38.39% -0.03%
Complexity 9823 9823
============================================
Files 2090 2091 +1
Lines 116568 116700 +132
Branches 15592 15611 +19
============================================
+ Hits 44785 44803 +18
- Misses 67625 67739 +114
Partials 4158 4158 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a4e6bd3
to
bdcebd8
Compare
bdcebd8
to
874459f
Compare
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.
LGTM! I don't have any blocking feedback, so I'm approving, but please review my comments before merging. 🚀
data class BookingPaymentDetailsModel( | ||
val service: String, | ||
val tax: String, | ||
val discount: String, | ||
val total: String | ||
) |
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.
Suggestion: I think it would be better approach to have this BookingPaymentDetailsModel
in the view state file, which in this case is BookingDetailsViewState.kt
. Another suggestion is rename it to BookingPaymentDetailsUiState
to better reflect its responsibility.
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 we do that, we should also move all other models from other sections. I'm not saying no, but I think it would be better to be consistent and this new section follows the previous ones.
verticalArrangement = Arrangement.spacedBy(16.dp), | ||
modifier = Modifier | ||
.background(color = MaterialTheme.colorScheme.surfaceContainer) | ||
.padding(vertical = 16.dp) |
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 set 16.dp
here for horizontal as well. I see that all of this Column
's children have .padding(horizontal = 16.dp)
. So, we can set it once here instead of repeating it four times below.
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't because the HorizontalDivider
only has the startPadding
.
if (paymentStatus == BookingPaymentStatus.PAID) { | ||
WCOutlinedButton( | ||
onClick = onMarkAsRefunded, | ||
colors = ButtonDefaults.outlinedButtonColors( | ||
contentColor = MaterialTheme.colorScheme.onSurface | ||
), | ||
text = stringResource(id = R.string.booking_payment_mark_as_refunded), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) | ||
} else { | ||
WCColoredButton( | ||
onClick = onMarkAsPaid, | ||
text = stringResource(id = R.string.booking_payment_mark_as_paid), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) | ||
} | ||
WCOutlinedButton( | ||
onClick = onViewOrder, | ||
colors = ButtonDefaults.outlinedButtonColors( | ||
contentColor = MaterialTheme.colorScheme.onSurface | ||
), | ||
text = stringResource(id = R.string.booking_payment_view_order), | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(horizontal = 16.dp) | ||
) |
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 was in the same situation before. 😄 When adding buttons in RemoveShipmentBottomSheet, I set 8.dp
between the buttons, which results in a total of 16.dp
.
I don’t have a strong opinion, but my preference would be to do the same as I did in RemoveShipmentBottomSheet
and keep it aligned with the design. I’ll leave the decision to you.
thickness = 0.5.dp, | ||
modifier = Modifier.padding(start = 16.dp) | ||
) | ||
if (paymentStatus == BookingPaymentStatus.PAID) { |
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 asked about it in Slack (p1759326540162229-slack-C09FHQNQERG). If we don’t get a reply, we can add a TODO comment to update this logic later and merge the PR now.
Closes WOOMOB-1346
Description
This PR adds the payment section to the
BookingDetailsScreen
with a workingView order
button and not fully implementedmark as paid/mark as refunded
buttons.There are two extra commits that move the view model callback to the view state to limit the growing number of parameters - fc1ae2f and a4e6bd3.
Steps to reproduce
View order
Testing information
You can verify with different font/view scaling and light/dark theme.
Tapping on the
Mark as paid
/Mark as refunded
should also update the temp in-memory state.The tests that have been performed
The above.
Images/gif
Screen_recording_20250930_115742.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.