[CIAB] BookingListViewModel refactoring and some fixes #14780
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,17 +12,20 @@ import com.woocommerce.android.viewmodel.ScopedViewModel | |
| import com.woocommerce.android.viewmodel.getNullableStateFlow | ||
| import com.woocommerce.android.viewmodel.getStateFlow | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import kotlinx.coroutines.FlowPreview | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.flow.MutableSharedFlow | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.collectLatest | ||
| import kotlinx.coroutines.flow.combine | ||
| import kotlinx.coroutines.flow.debounce | ||
| import kotlinx.coroutines.flow.flatMapLatest | ||
| import kotlinx.coroutines.flow.map | ||
| import kotlinx.coroutines.flow.onStart | ||
| import kotlinx.coroutines.flow.withIndex | ||
| import kotlinx.coroutines.launch | ||
| import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingFilters | ||
| import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingsFilterOption | ||
| import javax.inject.Inject | ||
|
|
||
|
|
@@ -42,6 +45,7 @@ class BookingListViewModel @Inject constructor( | |
| clazz = String::class.java, | ||
| key = "searchQuery" | ||
| ) | ||
| private val refreshTrigger = MutableSharedFlow<Unit>(extraBufferCapacity = 1) | ||
|
|
||
| private val sortOption = savedStateHandle.getStateFlow(viewModelScope, BookingListSortOption.NewestToOldest) | ||
|
|
||
|
|
@@ -59,7 +63,7 @@ class BookingListViewModel @Inject constructor( | |
| BookingListContentState( | ||
| bookings = bookings, | ||
| loadingState = loadingState, | ||
| onRefresh = { refreshTrigger.tryEmit(Unit) }, | ||
| onLoadMore = ::loadMore, | ||
| onBookingClick = ::onBookingClick | ||
| ) | ||
|
|
@@ -112,40 +116,70 @@ class BookingListViewModel @Inject constructor( | |
| monitorSearchAndFilterChanges() | ||
| } | ||
|
|
||
| @OptIn(FlowPreview::class, ExperimentalCoroutinesApi::class) | ||
| private fun monitorSearchAndFilterChanges() { | ||
| launch { | ||
| var lastFetchParams: FetchParams? = null | ||
| val queryFlow = searchQuery | ||
| .withIndex() | ||
| .debounce { (index, query) -> | ||
| // Skip debounce for the initial value or when the query is cleared | ||
| if (index == 0 || query.isNullOrEmpty()) 0L else AppConstants.SEARCH_TYPING_DELAY_MS | ||
| } | ||
| .map { it.value } | ||
|
|
||
| combine( | ||
| selectedTab, | ||
| queryFlow, | ||
| sortOption, | ||
| bookingFilterRepository.bookingFiltersFlow | ||
| ) { tab, query, sort, filters -> | ||
| FetchParams( | ||
| searchQuery = query, | ||
| sortOption = sort, | ||
| selectedTab = tab, | ||
| filters = filters | ||
| ) | ||
| }.flatMapLatest { fetchParams -> | ||
| refreshTrigger.map { true } | ||
| .onStart { emit(false) } | ||
| .map { isRefreshing -> Pair(fetchParams, isRefreshing) } | ||
| }.collectLatest { (fetchParams, isRefreshing) -> | ||
| // Cancel any ongoing fetch or load more operations | ||
| bookingsFetchJob?.cancel() | ||
| bookingsLoadMoreJob?.cancel() | ||
|
|
||
| val initialLoadingState = if (isRefreshing) { | ||
| BookingListLoadingState.Refreshing | ||
| } else { | ||
| lastFetchParams.let { lastFetchParams -> | ||
| if (lastFetchParams != null && lastFetchParams.sortOption != fetchParams.sortOption) { | ||
| // When sort option changes, force refreshing state to indicate data reload | ||
| BookingListLoadingState.Refreshing | ||
| } else { | ||
| BookingListLoadingState.Loading | ||
| } | ||
| } | ||
| } | ||
|
|
||
| lastFetchParams = fetchParams | ||
| fetchBookings( | ||
| initialLoadingState = initialLoadingState, | ||
| fetchParams = fetchParams | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private suspend fun fetchBookings( | ||
| initialLoadingState: BookingListLoadingState, | ||
| fetchParams: FetchParams, | ||
| ) { | ||
|
Comment on lines
+174
to
+177
There was a problem hiding this comment. The |
||
| loadingState.value = initialLoadingState | ||
| bookingListHandler.loadBookings( | ||
| searchQuery = fetchParams.searchQuery, | ||
| filters = fetchParams.prepareFilters(), | ||
| sortBy = fetchParams.sortOption | ||
| ).onFailure { | ||
| triggerEvent(MultiLiveEvent.Event.ShowSnackbar(R.string.bookings_fetch_error)) | ||
| } | ||
|
|
@@ -192,17 +226,24 @@ class BookingListViewModel @Inject constructor( | |
| triggerEvent(NavigateToFilters) | ||
| } | ||
|
|
||
| private fun FetchParams.prepareFilters(): List<BookingsFilterOption> = with(filtersBuilder) { | ||
| when (selectedTab) { | ||
| BookingListTab.Today, | ||
| BookingListTab.Upcoming -> listOfNotNull( | ||
| selectedTab.asDateRangeFilter() | ||
| ) | ||
|
|
||
| BookingListTab.All -> filters.asList() | ||
| } | ||
| } | ||
|
|
||
| private data class FetchParams( | ||
| val searchQuery: String?, | ||
| val sortOption: BookingListSortOption, | ||
| val selectedTab: BookingListTab, | ||
| val filters: BookingFilters | ||
| ) | ||
|
|
||
| data class NavigateToBookingDetails(val bookingId: Long) : MultiLiveEvent.Event() | ||
| object NavigateToFilters : MultiLiveEvent.Event() | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import com.woocommerce.android.ui.bookings.BookingsRepository | |
| import com.woocommerce.android.util.InlineClassesAnswer | ||
| import com.woocommerce.android.viewmodel.BaseUnitTest | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import kotlinx.coroutines.delay | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.first | ||
| import kotlinx.coroutines.flow.flowOf | ||
|
|
@@ -21,6 +22,7 @@ import org.mockito.kotlin.given | |
| import org.mockito.kotlin.mock | ||
| import org.mockito.kotlin.never | ||
| import org.mockito.kotlin.verify | ||
| import org.mockito.kotlin.willSuspendableAnswer | ||
| import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId | ||
| import org.wordpress.android.fluxc.model.LocalOrRemoteId.RemoteId | ||
| import org.wordpress.android.fluxc.network.rest.wpcom.wc.bookings.BookingOrderInfo | ||
|
|
@@ -202,6 +204,87 @@ class BookingListHandlerTest : BaseUnitTest() { | |
| assertThat(bookings).hasSize(BookingListHandler.PAGE_SIZE * 2) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given search, when refreshing, then keep previous search results during the fetch`() = testBlocking { | ||
| val searchQuery = "Sample" | ||
| val firstResults = List(BookingListHandler.PAGE_SIZE) { getSampleBooking(it) } | ||
| val refreshedResults = List(BookingListHandler.PAGE_SIZE - 2) { getSampleBooking(it) } | ||
| given(bookingsRepository.fetchBookings(any(), any(), eq(searchQuery), any(), any())) | ||
| .willReturn( | ||
| Result.success( | ||
| BookingsRepository.FetchResult(firstResults, false) | ||
| ) | ||
| ) | ||
| .willSuspendableAnswer { | ||
| delay(10) // To allow checking intermediate state | ||
| Result.success( | ||
| BookingsRepository.FetchResult(refreshedResults, false) | ||
| ) | ||
| } | ||
|
|
||
| bookingListHandler.loadBookings( | ||
| searchQuery = searchQuery, | ||
| sortBy = BookingListSortOption.NewestToOldest | ||
| ) | ||
| val bookingsAfterFirstLoad = bookingListHandler.bookingsFlow.first() | ||
| val refreshJob = launch { | ||
| bookingListHandler.loadBookings( | ||
| searchQuery = searchQuery, | ||
| sortBy = BookingListSortOption.NewestToOldest | ||
| ) | ||
| } | ||
| val bookingsDuringRefresh = bookingListHandler.bookingsFlow.first() | ||
| refreshJob.join() | ||
| val bookingsAfterRefresh = bookingListHandler.bookingsFlow.first() | ||
|
|
||
| assertThat(bookingsAfterFirstLoad).isEqualTo(firstResults) | ||
| assertThat(bookingsDuringRefresh).isEqualTo(firstResults) | ||
| assertThat(bookingsAfterRefresh).isEqualTo(refreshedResults) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given search, when changing search query, then reset search results`() = testBlocking { | ||
| val initialQuery = "Initial" | ||
| val newQuery = "New" | ||
|
|
||
| val firstResults = List(BookingListHandler.PAGE_SIZE) { getSampleBooking(it) } | ||
| val newResults = List(BookingListHandler.PAGE_SIZE - 3) { getSampleBooking(it) } | ||
|
|
||
| given(bookingsRepository.fetchBookings(any(), any(), eq(initialQuery), any(), any())) | ||
| .willReturn( | ||
| Result.success( | ||
| BookingsRepository.FetchResult(firstResults, false) | ||
| ) | ||
| ) | ||
| given(bookingsRepository.fetchBookings(any(), any(), eq(newQuery), any(), any())) | ||
| .willSuspendableAnswer { | ||
| delay(10) // To allow checking intermediate state | ||
| Result.success( | ||
| BookingsRepository.FetchResult(newResults, false) | ||
| ) | ||
| } | ||
|
|
||
| bookingListHandler.loadBookings( | ||
| searchQuery = initialQuery, | ||
| sortBy = BookingListSortOption.NewestToOldest | ||
| ) | ||
| val bookingsAfterInitialSearch = bookingListHandler.bookingsFlow.first() | ||
|
|
||
| val refreshJob = launch { | ||
| bookingListHandler.loadBookings( | ||
| searchQuery = newQuery, | ||
| sortBy = BookingListSortOption.NewestToOldest | ||
| ) | ||
| } | ||
| val bookingsDuringNewSearch = bookingListHandler.bookingsFlow.first() | ||
| refreshJob.join() | ||
| val bookingsAfterNewSearch = bookingListHandler.bookingsFlow.first() | ||
|
|
||
| assertThat(bookingsAfterInitialSearch).isEqualTo(firstResults) | ||
| assertThat(bookingsDuringNewSearch).isEmpty() | ||
| assertThat(bookingsAfterNewSearch).isEqualTo(newResults) | ||
| } | ||
|
|
||
| private fun getSampleBooking(id: Int) = BookingEntity( | ||
| id = RemoteId(id.toLong()), | ||
| localSiteId = LocalId(1), | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is the main part of the second change mentioned in the description, it fixes an issue where pull-to-refresh would result in displaying the Empty state during the fetch. I added two unit tests to confirm the behavior too.
Check the below recordings to see the diffference:
Screen_recording_20251017_181115.mp4
Screen_recording_20251017_181007.mp4