[WOOMOB-1400] - Observe booking object from DB on the details screen #14682
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements BookingRepository in BookingDetailsViewModel to observe booking objects from the database, allowing real-time updates of booking data in the details screen. It also introduces a centralized BookingMapper class to handle all booking-related model mappings and refactors the existing code to use this mapper.
Key Changes
- Added database observation capability for individual bookings through new DAO and repository methods
- Created a centralized
BookingMapperclass to handle all booking model transformations - Refactored the booking details screen to use observed data instead of hardcoded values
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
BookingsDao.kt |
Added observeBooking method to query and observe individual booking entities |
BookingsStore.kt |
Added observeBooking method to expose booking observation through the store layer |
BookingsRepository.kt |
Added observeBooking method to provide booking observation at the repository level |
BookingMapper.kt |
New centralized mapper class for all booking-related model transformations |
BookingDetailsViewModel.kt |
Updated to observe booking data from repository and use mapper for transformations |
BookingDetailsViewState.kt |
Refactored state structure to use nullable BookingUiState instead of hardcoded values |
BookingDetailsScreen.kt |
Updated to handle nullable booking state and display content conditionally |
BookingListViewModel.kt |
Updated to use the new centralized BookingMapper |
BookingListViewState.kt |
Removed inline mapping functions that were moved to BookingMapper |
| Test files | Updated and added tests to support the new mapper and repository integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt
Show resolved
Hide resolved
Generated by 🚫 Danger |
| data class BookingUiState( | ||
| val bookingSummary: BookingSummaryModel, | ||
| val bookingsAppointmentDetails: BookingAppointmentDetailsModel, | ||
| val bookingCustomerDetails: BookingCustomerDetailsModel, | ||
| val bookingPaymentDetails: BookingPaymentDetailsModel, | ||
| ) |
There was a problem hiding this comment.
All those models would be nullable after this PR, so it's handy to wrap them in a single class to avoid multiple null checks.
| private val currencyFormatter: CurrencyFormatter, | ||
| ) { | ||
| private val summaryDateFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedDateTime( | ||
| FormatStyle.MEDIUM, | ||
| FormatStyle.SHORT | ||
| ).withZone(ZoneOffset.UTC) |
There was a problem hiding this comment.
cc @hichamboushaba I've used the UTC based on your discussion.
📲 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.
|
AdamGrzybkowski
force-pushed
the
issue/WOOMOB-1400_observe_booking
branch
from
7c821d7 to
4533d98
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14682 +/- ##
============================================
+ Coverage 38.37% 38.38% +0.01%
- Complexity 9842 9855 +13
============================================
Files 2097 2098 +1
Lines 116949 116974 +25
Branches 15651 15653 +2
============================================
+ Hits 44876 44900 +24
- Misses 67908 67909 +1
Partials 4165 4165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@hichamboushaba I pushed one extra commit with a one-liner that I forgot to do after rebasing with trunk. 509794d |
| toolbarTitle = resourceProvider.getString(R.string.booking_details_title, navArgs.bookingId), | ||
| ) | ||
| } | ||
| observeBooking(navArgs.bookingId) |
There was a problem hiding this comment.
Before approving this, I just want to discuss one problem with this approach and decide whether we are OK with it or not.
With this approach, we observe the DB regardless of whether the app is in the foreground or not (this is part of what we discussed before on whether we should expose flows or LiveData).
I think a better approach (IMO 🙂) would be to have a cold flow exposed as LiveData, so the cold flow won't start until the screen reaches the Started lifecycle state, and to achieve it, we can think of something like the following patch:
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 509794d7b5731f41a1b4ea3847de4599bcf8a3f5)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt (date 1759502430662)
@@ -4,7 +4,6 @@
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.asLiveData
import com.woocommerce.android.R
-import com.woocommerce.android.ui.bookings.Booking
import com.woocommerce.android.ui.bookings.BookingMapper
import com.woocommerce.android.ui.bookings.BookingsRepository
import com.woocommerce.android.ui.bookings.compose.BookingAttendanceStatus
@@ -15,69 +14,41 @@
import com.woocommerce.android.viewmodel.navArgs
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.flow.MutableStateFlow
-import kotlinx.coroutines.flow.launchIn
-import kotlinx.coroutines.flow.onEach
-import kotlinx.coroutines.flow.update
+import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.filterNotNull
import javax.inject.Inject
@HiltViewModel
class BookingDetailsViewModel @Inject constructor(
savedState: SavedStateHandle,
resourceProvider: ResourceProvider,
- private val bookingsRepository: BookingsRepository,
+ bookingsRepository: BookingsRepository,
private val bookingMapper: BookingMapper,
) : ScopedViewModel(savedState) {
private val navArgs: BookingDetailsFragmentArgs by savedState.navArgs()
- private val _state = MutableStateFlow(
- BookingDetailsViewState(
- onCancelBooking = ::onCancelBooking,
- onAttendanceStatusSelected = ::onAttendanceStatusSelected,
- )
- )
- val state: LiveData<BookingDetailsViewState> = _state.asLiveData()
-
- init {
- _state.update {
- it.copy(
- toolbarTitle = resourceProvider.getString(R.string.booking_details_title, navArgs.bookingId),
- )
- }
- observeBooking(navArgs.bookingId)
- }
-
- private fun onAttendanceStatusSelected(status: BookingAttendanceStatus) {
- val bookingState = _state.value.bookingUiState
- if (bookingState != null) {
- _state.update { current ->
- current.copy(
- bookingUiState = bookingState.copy(
- bookingSummary = bookingState.bookingSummary.copy(attendanceStatus = status)
- )
- )
- }
- }
- }
-
- private fun onCancelBooking() {
- // TODO Add logic to Cancel booking
- }
-
- private fun observeBooking(bookingId: Long) {
- bookingsRepository.observeBooking(bookingId)
- .onEach { booking ->
- booking?.let { updateStateWithBooking(it) }
- }
- .launchIn(this)
- }
-
- private fun updateStateWithBooking(booking: Booking) = with(bookingMapper) {
- _state.update { current ->
- current.copy(
+ private val booking = bookingsRepository.observeBooking(navArgs.bookingId)
+
+ // If we need this, then using `savedState.getStateFlow` would be better here to preserve state across process death,
+ private val bookingAttendanceStatus = MutableStateFlow<BookingAttendanceStatus?>(null)
+
+ val state: LiveData<BookingDetailsViewState> = combine(
+ booking.filterNotNull(),
+ bookingAttendanceStatus
+ ) { booking, attendanceStatus ->
+ with(bookingMapper) {
+ BookingDetailsViewState(
+ toolbarTitle = resourceProvider.getString(R.string.booking_details_title, booking.id),
orderId = booking.orderId,
bookingUiState = BookingUiState(
- bookingSummary = booking.toBookingSummaryModel(),
+ bookingSummary = booking.toBookingSummaryModel().let {
+ if (attendanceStatus != null) {
+ it.copy(attendanceStatus = attendanceStatus)
+ } else {
+ it
+ }
+ },
bookingsAppointmentDetails = booking.toAppointmentDetailsModel(),
bookingCustomerDetails = BookingCustomerDetailsModel(
name = "Margarita Nikolaevna",
@@ -96,7 +67,24 @@
total = "$59.50"
)
),
+ onCancelBooking = ::onCancelBooking,
+ onAttendanceStatusSelected = ::onAttendanceStatusSelected
)
}
+ }.asLiveData()
+
+ private fun onAttendanceStatusSelected(status: BookingAttendanceStatus) {
+ // Whether we'll need the `bookingAttendanceStatus` property or not depending
+ // on whether we'll have a "save" button for the whole screen or we'll update the status
+ // as soon as the user selects it from the bottom sheet.
+ // Personally I think we should update it immediately, and if we do, then we don't need the property
+ // we just need to manage the loading state, and when the API call is done, the booking will be updated accordingly
+ // via the `booking` flow.
+ // But this is a product decision. And this patch just preserves the current behavior.
+ bookingAttendanceStatus.value = status
+ }
+
+ private fun onCancelBooking() {
+ // TODO Add logic to Cancel booking
}
}The patch also makes the whole screen reactive, so if during the implementation we find that we need the Order (which most probably will happen), then we can chain the Flows to observe also the Order from the DB, and then the DB will be the single source of truth, and drives the screen.
Please check it out, and share your thoughts. While I think observing the DB when the app is in the background should be avoided, if you don't agree with the above approach, I'm OK with accepting the PR as it's.
There was a problem hiding this comment.
Please check it out, and share your thoughts. While I think observing the DB when the app is in the background should be avoided, if you don't agree with the above approach, I'm OK with accepting the PR as it's.
Thanks @hichamboushaba! You're obviously correct. I think I was working on an autopilot, not being used to LiveData 🤦
I will update the PR on Monday!
hichamboushaba
left a comment
hichamboushaba
left a comment
There was a problem hiding this comment.
Nice work @AdamGrzybkowski 💯, works well, I left two remarks, but they are not blockers.
| FormatStyle.SHORT | ||
| ).withZone(ZoneOffset.UTC) | ||
|
|
||
| private val detailsDateFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("EEEE, dd MMM yyyy") |
There was a problem hiding this comment.
IMO we should avoid hardcording formatting pattens, I'd use a localized format instead. I know it's used in the app is some spots, but IMO it's not something we should follow.
| private val detailsDateFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("EEEE, dd MMM yyyy") | |
| private val detailsDateFormatter: DateTimeFormatter = DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL) |
The above will update the text from: Monday, 06 Oct 2025 to Monday, 6 October 2025 for UK english set as the locale.
Another approach if we are sure we want to keep the shorter month name is to use something like:
private val detailsDateFormatter: DateTimeFormatter = DateTimeFormatterBuilder()
.appendPattern("EEEE, ")
.append(DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM))
.toFormatter()
.withZone(ZoneOffset.UTC)But personally I think using FormatStyle.FULL is better in this context, WDYT?
There was a problem hiding this comment.
Makes sense, I did focus on the format I saw in the Figma, but this will work better across different locales. Thanks!
| ) | ||
| } | ||
|
|
||
| fun Booking.toUiModel(): BookingListItem { |
There was a problem hiding this comment.
np, just to make the role a bit clearer, I'd rename this to toListItem(), WDYT?
WOOMOB-1400
Description
The main goal of this PR is to implement
BookingRepositoryin theBookingDetailsViewModelto observe the booking object in the DB.This only observes the local object. We will have to fetch more information anyway, so that's where I think we can add refreshing of the booking model as well (if we think it's necessary).
Additionally, since we have to handle object mapping, I went ahead and created a mapper class
BookingMapper, for all booking-related models. In one of the commits, I have added this mapper to theBookingListViewModelas well.Steps to reproduce
Testing information
No UI changes. The above should be enough.
The tests that have been performed
The above
Images/gif
Screen_recording_20251002_171432.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.