Skip to content

[WOOMOB-1722] - Properly clean the Bookings table for Today/Upcoming tabs' first page load #15022

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 27, 2025
+439 −77
Merged

[WOOMOB-1722] - Properly clean the Bookings table for Today/Upcoming tabs' first page load #15022

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6cd96c
Prefer BookingFilters.EMPTY over nullable property
AdamGrzybkowski Nov 25, 2025
98c6ec8
Extract Today/Upcoming DateRange creation for reuse in other places
AdamGrzybkowski Nov 25, 2025
99c3735
Properly clear the Booking table for Today/Upcoming tabs
AdamGrzybkowski Nov 25, 2025
5ef2be3
Make sure that only date range matching today/upcoming is applied
AdamGrzybkowski Nov 26, 2025
e19f6d1
Improve BookingsDateRangePresets
AdamGrzybkowski Nov 27, 2025
c2cf448
Wrap delete and insert in one DB transaction
AdamGrzybkowski Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingsRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class BookingsRepository @Inject constructor(
page: Int,
perPage: Int,
query: String? = null,
filters: BookingFilters? = null,
filters: BookingFilters = BookingFilters.EMPTY,
order: BookingsOrderOption
): Result<FetchResult> {
val result = bookingsStore.fetchBookings(
Expand Down
29 changes: 5 additions & 24 deletions ...rce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListFiltersBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,19 @@
package com.woocommerce.android.ui.bookings.list

import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingsDateRangePresets
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingsFilterOption
import java.time.Clock
import java.time.LocalDate
import java.time.LocalTime
import java.time.ZoneOffset
import javax.inject.Inject

class BookingListFiltersBuilder @Inject constructor(
private val clock: Clock
) {
/**
* Returns a [BookingsFilterOption.DateRange] based on the selected [BookingListTab].
*
* We use UTC for the API calls, as the API stores the dates without timezone information, which means that
* when comparing dates, they are treated as UTC times.
* See p1759398245019489-slack-C09FHQNQERG
*/
fun BookingListTab.asDateRangeFilter(): BookingsFilterOption.DateRange? {
fun todayAtMidnight() = LocalDate.now(clock).atTime(LocalTime.MIDNIGHT).atOffset(ZoneOffset.UTC).toInstant()
fun todayAtEndOfDay() = LocalDate.now(clock).atTime(LocalTime.MAX).atOffset(ZoneOffset.UTC).toInstant()

return when (this) {
BookingListTab.Today -> BookingsFilterOption.DateRange(
before = todayAtEndOfDay(),
after = todayAtMidnight()
)

BookingListTab.Upcoming -> BookingsFilterOption.DateRange(
before = null,
after = todayAtEndOfDay()
)

BookingListTab.All -> null
}
fun BookingListTab.asDateRangeFilter(): BookingsFilterOption.DateRange? = when (this) {
BookingListTab.Today -> BookingsDateRangePresets.today(clock)
BookingListTab.Upcoming -> BookingsDateRangePresets.upcoming(clock)
BookingListTab.All -> null
}
}
4 changes: 2 additions & 2 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class BookingListHandler @Inject constructor(
private val canLoadMore = AtomicBoolean(false)

private val searchQuery = MutableStateFlow<String?>(null)
private val filters = MutableStateFlow<BookingFilters?>(null)
private val filters = MutableStateFlow(BookingFilters.EMPTY)
private val sortBy = MutableStateFlow(BookingListSortOption.NewestToOldest)

private val searchResults = MutableStateFlow(emptyList<Booking>())
Expand All @@ -57,7 +57,7 @@ class BookingListHandler @Inject constructor(

suspend fun loadBookings(
searchQuery: String? = null,
filters: BookingFilters? = null,
filters: BookingFilters = BookingFilters.EMPTY,
sortBy: BookingListSortOption
): Result<Unit> = mutex.withLock {
// Reset pagination attributes
Expand Down
47 changes: 47 additions & 0 deletions ...in/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingsDateRangePresets.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings

import java.time.Clock
import java.time.LocalDate
import java.time.LocalTime
import java.time.ZoneOffset

/**
* Factory for canonical booking date range presets used across app and data layers.
*
* Notes about time zone:
* The WC Bookings API stores dates without timezone information. To ensure
* consistent comparisons and filtering on both client and server, we construct
* ranges in UTC.
* See p1759398245019489-slack-C09FHQNQERG
*/
object BookingsDateRangePresets {
/**
* Returns a DateRange that spans the current day in UTC, inclusive of the
* full day when interpreted by the API (midnight to end-of-day).
*/
fun today(clock: Clock): BookingsFilterOption.DateRange {
val start = LocalDate.now(clock)
.atTime(LocalTime.MIDNIGHT)
.atOffset(ZoneOffset.UTC)
.toInstant()
val end = LocalDate.now(clock)
.atTime(LocalTime.MAX)
.atOffset(ZoneOffset.UTC)
.toInstant()
return BookingsFilterOption.DateRange(before = end, after = start)
}

/**
* Returns a DateRange that includes bookings strictly after the end of the
* current day in UTC (i.e., upcoming bookings from tomorrow onwards).
*
* before = null and after = end-of-today.
*/
fun upcoming(clock: Clock): BookingsFilterOption.DateRange {
val endOfToday = LocalDate.now(clock)
.atTime(LocalTime.MAX)
.atOffset(ZoneOffset.UTC)
.toInstant()
return BookingsFilterOption.DateRange(before = null, after = endOfToday)
}
}
35 changes: 31 additions & 4 deletions ...c/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingsStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import org.wordpress.android.fluxc.store.WCOrderStore
import org.wordpress.android.fluxc.tools.CoroutineEngine
import org.wordpress.android.fluxc.utils.HeadersParser
import org.wordpress.android.util.AppLog
import java.time.Clock
import javax.inject.Inject
import javax.inject.Singleton

Expand All @@ -26,13 +27,14 @@ class BookingsStore @Inject internal constructor(
private val bookingDtoMapper: BookingDtoMapper,
private val headersParser: HeadersParser,
private val coroutineEngine: CoroutineEngine,
private val clock: Clock,
) {
suspend fun fetchBookings(
site: SiteModel,
perPage: Int = BookingsRestClient.DEFAULT_PER_PAGE,
page: Int = 1,
query: String? = null,
filters: BookingFilters?,
filters: BookingFilters,
order: BookingsOrderOption
): WooResult<BookingsFetchResult> {
return coroutineEngine.withDefaultContext(AppLog.T.API, this, "fetchBookings") {
Expand All @@ -54,9 +56,28 @@ class BookingsStore @Inject internal constructor(
)
}
}
if (page == 1 && filters == BookingFilters.EMPTY && query.isNullOrEmpty()) {
// Clear existing bookings and insert new ones when fetching the first page
bookingsDao.replaceAllForSite(site.localId(), entities)
// Clear existing bookings when fetching the first page.
// If filters are applied, only clear entries that match the applied filters,
// otherwise (no filters) clear all entries for the site.
Comment on lines +59 to +61
Copy link
Contributor

@irfano irfano Nov 26, 2025

Choose a reason for hiding this comment

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

The comment here is not entirely accurate. We say, If filters are applied, only clear entries that match the applied filters, but this is only true for date filters. The issue still exists for other filter types. For example, I filtered “No-show” bookings, trashed one, refreshed the list, and it was still visible.
If we want to address this for all cases, we’ll need to always delete cached entities when any filter is applied. Since it already works for the date filter, we could turn that into a more general solution. WDYT?

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since it already works for the date filter, we could turn that into a more general solution. WDYT?

I initially did that, but then I thought that there was a reason why we added this check in the first place, so I decided to strictly add this logic for Today and Upcoming filter.

I'm open to changing that. I wonder if we had similar problems with Products or Orders, and how they were solved there?

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

Hmm thinking about it, I'm not sure we need it. We can only apply other filters in the All tab. This tab, without filters applied, will delete or bookings for a selected site on the initial load. So the problems should be less visible and we won't be clearing the data that we would get on next page loads.

Copy link
Contributor

@irfano irfano Nov 27, 2025

Choose a reason for hiding this comment

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

True, the issue is less visible for the All tab, but the All filter is saved, and it's possible that the user prefers to always keep a certain filter enabled. In that case, it becomes the same situation as the Today and Upcoming tabs.

Copy link
Contributor

@irfano irfano Nov 27, 2025

Choose a reason for hiding this comment

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

I'm open to changing that. I wonder if we had similar problems with Products or Orders, and how they were solved there?

I've just tested the Products tab, and the issue exists there as well.

if (page == 1 && query.isNullOrEmpty()) {
when {
filters == BookingFilters.EMPTY -> {
bookingsDao.replaceAllForSite(site.localId(), entities)
}
filters.dateRange != null && isTodayOrUpcoming(filters.dateRange) -> {
// Delete only the rows that match the applied date range filter for Today/Upcoming
bookingsDao.deleteForSiteWithDateRangeFilter(
site.localId(),
filters.dateRange,
entities.map { it.id.value }
)
bookingsDao.insertOrReplace(entities)
Copy link
Contributor

@irfano irfano Nov 26, 2025

Choose a reason for hiding this comment

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

There is also an alternative way to handle removed bookings. We can compare the new entities with the cached bookings and detect the removed ones instead of removing all cached data. I know replaceAllForSite() is what we already use, but removing only trashed bookings seems like a better approach to me.

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

Could you clarify? The current implementation only removes the local entities if they are not present in the response from the server. I'm not clearing all cached data here.

Copy link
Contributor

@irfano irfano Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

cachedFilteredBookings = [A, B, C]
remoteFilteredBookings = [B, C, D] (A is removed)

// Current implementation
runNewDBTransaction {
    remove(A, B, C)
    insertOrReplace(B, C, D)
}

// My suggestion
runNewDBTransaction {
    remove(A)
    insertOrReplace(B, C, D)
}

In the current implementation, we remove the entire cachedFilteredBookings, right?

This is a minor suggestion and it won’t affect the behavior. It will make the DB work a smaller task, but we’ll still need to find the removed booking (A). Still, I think reducing the number of DB transactions is the better approach.

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

In the current implementation, we remove the entire cachedFilteredBookings, right?

No. We pass the IDs of the fetched bookings and have this SQL query condition AND ((:idsSize = 0) OR id NOT IN (:ids)) to make sure we only delete the ones that are not in the list. It's exactly as in your suggestion.

Copy link
Contributor

@irfano irfano Nov 27, 2025

Choose a reason for hiding this comment

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

Oh, I missed that in the SQL. Then it’s okay. Thanks!

My last feedback: can we unify deleteForSiteWithDateRangeFilter() and insertOrReplace() into a single DB transaction? Having two separate transactions might trigger additional observations in the view models. I fixed a similar issue before here: #14800.

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

In this case, this is not a problem, because we are only removing items that should be deleted. The other issue was created because we were deleting and then inserting the same items.

So in a scenario from #14800 the flickering would still occur, because the item will actually be deleted, no matter if it's wrapped in one transaction or not.

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

I will wrap them in one transaction because it won't hurt us, but this wouldn't trigger the same issue mentioned above.

Copy link
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Nov 27, 2025

Choose a reason for hiding this comment

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

@irfano Fixed in c2cf448

Copy link
Contributor

@irfano irfano Nov 27, 2025

Choose a reason for hiding this comment

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

Yes, I noticed it currently doesn’t cause any issues, but it could still lead to a hard-to-diagnose problem in the future. Thanks for making the change. 👍🏻

}
else -> {
// For any other filters, avoid deletions to prevent removing unrelated cached items
bookingsDao.insertOrReplace(entities)
}
}
} else {
bookingsDao.insertOrReplace(entities)
}
Expand All @@ -77,6 +98,12 @@ class BookingsStore @Inject internal constructor(
}
}

private fun isTodayOrUpcoming(dateRange: BookingsFilterOption.DateRange): Boolean {
val today = BookingsDateRangePresets.today(clock)
val upcoming = BookingsDateRangePresets.upcoming(clock)
return dateRange == today || dateRange == upcoming
}

fun observeBookings(
site: SiteModel,
limit: Int? = null,
Expand Down
33 changes: 33 additions & 0 deletions libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/BookingsDao.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import androidx.room.Transaction
import kotlinx.coroutines.flow.Flow
import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingFilters
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingsFilterOption
import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingsOrderOption
import org.wordpress.android.fluxc.persistence.entity.BookingEntity
import org.wordpress.android.fluxc.persistence.entity.BookingResourceEntity
Expand Down Expand Up @@ -70,6 +71,38 @@ interface BookingsDao {
insertOrReplace(entities)
}

@Suppress("LongParameterList")
@Query(
"""
DELETE FROM Bookings
WHERE localSiteId = :localSiteId
AND (:startDateBefore IS NULL OR start <= :startDateBefore)
AND (:startDateAfter IS NULL OR start >= :startDateAfter)
AND ((:idsSize = 0) OR id NOT IN (:ids))
"""
)
suspend fun deleteForSiteWithDateRangeFilter(
localSiteId: LocalId,
startDateBefore: Long?,
startDateAfter: Long?,
ids: List<Long>,
idsSize: Int,
)

suspend fun deleteForSiteWithDateRangeFilter(
localSiteId: LocalId,
dateRange: BookingsFilterOption.DateRange,
ids: List<Long>
) {
deleteForSiteWithDateRangeFilter(
localSiteId = localSiteId,
startDateBefore = dateRange.before?.epochSecond,
startDateAfter = dateRange.after?.epochSecond,
ids = ids,
idsSize = ids.size,
)
}

fun observeBookings(
localSiteId: LocalId,
limit: Int? = null,
Expand Down
Loading
Loading