-
Notifications
You must be signed in to change notification settings - Fork 136
[CIAB] Implement mapping of payment details for Bookings and improve address formatting #14746
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
| } ?: this.orderAddressToString() | ||
| } | ||
|
|
||
| fun getFullAddress(name: String, address: String, country: String): 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.
I opted to remove this function, I don't know the history about it, but the fact that the function accepted the details made it look like a utility function that don't belong here in the model.
So instead, I updated the function not to require any arguments, which simplified its usage everywhere. For better context about the change, check the commit f383c32 individually.
a44014b to
d152d74
Compare
| val email: String?, | ||
| val phone: String?, | ||
| val billingAddressLines: List<String>, | ||
| val billingAddress: 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.
I updated this to be a single String over a list, I did this because the existing API for Address formatting returns a single String, so instead of splitting it, we show it in a single Text. If you see any downsides with this change, please let me know.
| val (country, state) = withContext(Dispatchers.IO) { | ||
| getLocations(countryCode, billingState.orEmpty()) | ||
| } |
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.
As we discussed before, this follows what we use in OrderMapper. If we want to avoid doing this in the mapper (personally don't see an issue with having this in the mapper), we'll have to do something like the following patch to move the getLocations call to the consumer, let me know if you prefer the other approach.
Subject: [PATCH] Changes
---
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt (revision 6fb8c209cfc832aeb9f9445bff7415af171deafb)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt (date 1760374703373)
@@ -2,7 +2,9 @@
import com.woocommerce.android.extensions.isNotEqualTo
import com.woocommerce.android.model.Address
+import com.woocommerce.android.model.AmbiguousLocation
import com.woocommerce.android.model.GetLocations
+import com.woocommerce.android.model.Location
import com.woocommerce.android.ui.bookings.compose.BookingAppointmentDetailsModel
import com.woocommerce.android.ui.bookings.compose.BookingAttendanceStatus
import com.woocommerce.android.ui.bookings.compose.BookingCustomerDetailsModel
@@ -11,8 +13,6 @@
import com.woocommerce.android.ui.bookings.compose.BookingSummaryModel
import com.woocommerce.android.ui.bookings.list.BookingListItem
import com.woocommerce.android.util.CurrencyFormatter
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.withContext
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingCustomerInfo
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingPaymentInfo
import org.wordpress.android.fluxc.persistence.entity.BookingEntity
@@ -67,14 +67,17 @@
)
}
- suspend fun BookingCustomerInfo?.toCustomerDetailsModel(): BookingCustomerDetailsModel {
+ fun BookingCustomerInfo?.toCustomerDetailsModel(
+ state: AmbiguousLocation?,
+ country: Location?
+ ): BookingCustomerDetailsModel {
if (this == null) return BookingCustomerDetailsModel.EMPTY
return BookingCustomerDetailsModel(
name = fullName(),
email = billingEmail,
phone = billingPhone,
- billingAddress = address()?.getFullAddress()
+ billingAddress = address(state, country)?.getFullAddress()
)
}
@@ -106,18 +109,19 @@
return "${billingFirstName.orEmpty()} ${billingLastName.orEmpty()}".trim().ifEmpty { null }
}
- private suspend fun BookingCustomerInfo.address(): Address? {
- val countryCode = billingCountry ?: return null
- val (country, state) = withContext(Dispatchers.IO) {
- getLocations(countryCode, billingState.orEmpty())
- }
+ private fun BookingCustomerInfo.address(
+ state: AmbiguousLocation?,
+ country: Location?
+ ): Address? {
+ if (country == null) return null
+
return Address(
company = billingCompany.orEmpty(),
firstName = billingFirstName.orEmpty(),
lastName = billingLastName.orEmpty(),
phone = billingPhone.orEmpty(),
country = country,
- state = state,
+ state = state ?: AmbiguousLocation.Defined(Location.EMPTY),
address1 = billingAddress1.orEmpty(),
address2 = billingAddress2.orEmpty(),
city = billingCity.orEmpty(),
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt (revision 6fb8c209cfc832aeb9f9445bff7415af171deafb)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt (date 1760374630741)
@@ -4,6 +4,7 @@
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.asLiveData
import com.woocommerce.android.R
+import com.woocommerce.android.model.GetLocations
import com.woocommerce.android.ui.bookings.Booking
import com.woocommerce.android.ui.bookings.BookingMapper
import com.woocommerce.android.ui.bookings.BookingsRepository
@@ -23,6 +24,7 @@
resourceProvider: ResourceProvider,
bookingsRepository: BookingsRepository,
private val bookingMapper: BookingMapper,
+ private val getLocations: GetLocations
) : ScopedViewModel(savedState) {
private val navArgs: BookingDetailsFragmentArgs by savedState.navArgs()
@@ -59,16 +61,22 @@
private suspend fun BookingMapper.buildBookingUiState(
booking: Booking,
attendanceStatus: BookingAttendanceStatus?
- ): BookingUiState = BookingUiState(
- bookingSummary = booking.toBookingSummaryModel().let {
- if (attendanceStatus != null) {
- it.copy(attendanceStatus = attendanceStatus)
- } else {
- it
- }
- },
- bookingsAppointmentDetails = booking.toAppointmentDetailsModel(),
- bookingCustomerDetails = booking.order.customerInfo.toCustomerDetailsModel(),
- bookingPaymentDetails = booking.order.paymentInfo?.toPaymentDetailsModel(booking.currency)
- )
+ ): BookingUiState {
+ val (country, state) = booking.order.customerInfo?.let {
+ getLocations(it.billingCountry.orEmpty(), it.billingState.orEmpty())
+ } ?: Pair(null, null)
+
+ return BookingUiState(
+ bookingSummary = booking.toBookingSummaryModel().let {
+ if (attendanceStatus != null) {
+ it.copy(attendanceStatus = attendanceStatus)
+ } else {
+ it
+ }
+ },
+ bookingsAppointmentDetails = booking.toAppointmentDetailsModel(),
+ bookingCustomerDetails = booking.order.customerInfo.toCustomerDetailsModel(state, country),
+ bookingPaymentDetails = booking.order.paymentInfo?.toPaymentDetailsModel(booking.currency)
+ )
+ }
}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 for the explanation @hichamboushaba - my preference would be to go with the patch, especially considering the thread switching within the mapper. I won't block the PR, though.
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 Adam, after all, I decided to stick with what I have now, the nullability handling when moving getLocations call to the ViewModel is not easy to follow, and given we follow the same approach in OrderMapper, we stay consistent too.
| val discount = total - subtotal | ||
| return BookingPaymentDetailsModel( | ||
| service = currencyFormatter.formatCurrency(subtotal, currency), // Pre-discount subtotal | ||
| tax = currencyFormatter.formatCurrency(totalTax, currency), // Tax on total after discount | ||
| discount = if (discount.isNotEqualTo(BigDecimal.ZERO)) { | ||
| "- ${currencyFormatter.formatCurrency(discount.abs(), currency)}" | ||
| } else { | ||
| "-" | ||
| }, | ||
| total = currencyFormatter.formatCurrency(total + totalTax, currency) // Total including tax |
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.
Let me know if you have any questions about the mapping here, I can share details on the role of the fields in the API and how Core handles the calculations.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 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 #14746 +/- ##
=========================================
Coverage 38.36% 38.36%
- Complexity 10002 10004 +2
=========================================
Files 2115 2115
Lines 118023 118014 -9
Branches 15749 15758 +9
=========================================
+ Hits 45274 45280 +6
+ Misses 68550 68535 -15
Partials 4199 4199 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AdamGrzybkowski
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.
| val (country, state) = withContext(Dispatchers.IO) { | ||
| getLocations(countryCode, billingState.orEmpty()) | ||
| } |
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 for the explanation @hichamboushaba - my preference would be to go with the patch, especially considering the thread switching within the mapper. I won't block the PR, though.
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsScreen.kt
Good eye, I'm not sure how I missed this. And this is a result of this PR, so it's good that we caught this, I didn't know |



Closes WOOMOB-1503
Description
This PR adds two main changes:
BookingPaymentInfoto the UI modelBookingPaymentDetailsModel. For this change I made the propertybookingPaymentDetailsnullable inBookingUiState, because if we don't have an order, it will be null, a case that we handle in the DB even though it shouldn't happen in production.Addressmodel as an intermediary for the formatting.I'll leave some comments regarding some decisions below.
Steps to reproduce
Testing information
The tests that have been performed
The above.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.