Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- [*][Woo POS] Automatically scroll to newly created orders when returning to the Orders screen [https://github.com/woocommerce/woocommerce-android/pull/14990]
- [Internal] Restore coupon drawable files mistakenly removed in previous release [https://github.com/woocommerce/woocommerce-android/pull/15005]
- [*] Fixed a rare crash that occurred when displaying shipping rates for a selected package [https://github.com/woocommerce/woocommerce-android/pull/15032]
- [*][Woo POS] Prepopulate orders cache after opening POS to improve performance [https://github.com/woocommerce/woocommerce-android/pull/15066]

23.7
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ import com.woocommerce.android.ui.woopos.home.ParentToChildrenEvent.SearchEvent.
import com.woocommerce.android.ui.woopos.home.ParentToChildrenEvent.SearchEvent.RecentSearchSelected
import com.woocommerce.android.ui.woopos.home.WooPosHomeState.DialogState
import com.woocommerce.android.ui.woopos.home.WooPosHomeState.ScreenPositionState
import com.woocommerce.android.ui.woopos.orders.WooPosOrdersDataSource
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.BackToCartTapped
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker
import com.woocommerce.android.viewmodel.getStateFlow
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.SharedFlow
import kotlinx.coroutines.flow.StateFlow
Expand All @@ -31,8 +33,13 @@ class WooPosHomeViewModel @Inject constructor(
private val parentToChildrenEventSender: WooPosParentToChildrenEventSender,
private val analyticsTracker: WooPosAnalyticsTracker,
private val soundHelper: WooPosSoundHelper,
private val ordersDataSource: WooPosOrdersDataSource,
savedStateHandle: SavedStateHandle,
) : ViewModel() {
companion object {
private const val ORDERS_CACHE_WARMUP_DELAY_MS = 5000L
}

private val _state = savedStateHandle.getStateFlow(
scope = viewModelScope,
key = "home_state",
Expand All @@ -54,6 +61,7 @@ class WooPosHomeViewModel @Inject constructor(
viewModelScope.launch {
soundHelper.preloadChaChing()
}
prepopulateOrdersCache()
}

override fun onCleared() {
Expand Down Expand Up @@ -255,6 +263,15 @@ class WooPosHomeViewModel @Inject constructor(
}
}

private fun prepopulateOrdersCache() {
viewModelScope.launch {
delay(ORDERS_CACHE_WARMUP_DELAY_MS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 The delay is added to relieve resources on POS start-up, because there are other network calls started immediately after POS launches (loading products in case the hybrid storage is used).

ordersDataSource.loadOrders(
Copy link
Contributor

Choose a reason for hiding this comment

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

Orders prepopulation feels like something that completely has nothing to do with the home screen and its view model. Configuration (delay) also feels not related to the home screen. Have you considered calling it from from WooPosSplashViewModel, but on global coroutine scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to splash and also removed delay - simply calling prepopulation after all the calls in splash complete: be96ae1

pageSize = WooPosOrdersDataSource.POS_ORDERS_CACHE_PREPOPULATION_SIZE
).collect {}
}
}

private fun handleOrderCreated(event: ChildToParentEvent.OrderCreated) {
sendEventToChildren(
OrderCreated(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,18 @@ class WooPosOrdersDataSource @Inject constructor(

companion object {
const val POS_ORDERS_PAGE_SIZE = 25
const val POS_ORDERS_CACHE_PREPOPULATION_SIZE = 5
private const val UNKNOWN_ERROR = "Unknown error"
}

fun loadOrders(): Flow<LoadOrdersResult> = flow {
fun loadOrders(pageSize: Int = POS_ORDERS_PAGE_SIZE): Flow<LoadOrdersResult> = flow {
Copy link
Contributor

Choose a reason for hiding this comment

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

All these changes related to loading 5 instead of 25, in my opinion, are not worth the effect - we are "lazy" loading 5kb

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shorter page makes the request itself faster, which was my main motivation. But that's probably not worth complicating the codebase. Simplified in be96ae1

val cached = ordersCache.getAll()
if (cached.isNotEmpty()) {
val cachedWithRefunds = fetchRefundsForOrders(cached)
emit(LoadOrdersResult.SuccessCache(cachedWithRefunds))
}

val result = loadFirstPage()
val result = loadFirstPage(pageSize = pageSize)
result.onSuccess { orders ->
ordersCache.setAll(orders)
val ordersWithRefunds = fetchRefundsForOrders(orders)
Expand Down Expand Up @@ -123,18 +124,24 @@ class WooPosOrdersDataSource @Inject constructor(
ordersCache.setAll(updated)
}

private suspend fun loadFirstPage(searchQuery: String? = null): Result<List<Order>> {
private suspend fun loadFirstPage(
searchQuery: String? = null,
pageSize: Int = POS_ORDERS_PAGE_SIZE
): Result<List<Order>> {
page.set(1)
canLoadMore.set(false)
return fetchAndMap(searchQuery)
return fetchAndMap(searchQuery, pageSize)
}

private suspend fun loadNextPage(searchQuery: String? = null): Result<List<Order>> {
return fetchAndMap(searchQuery)
}

private suspend fun fetchAndMap(searchQuery: String? = null): Result<List<Order>> {
val result = fetchOrdersFromRemote(page.get(), searchQuery)
private suspend fun fetchAndMap(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need any of the changes in this file at all?

searchQuery: String? = null,
pageSize: Int = POS_ORDERS_PAGE_SIZE
): Result<List<Order>> {
val result = fetchOrdersFromRemote(page.get(), searchQuery, pageSize)
return if (result.isError) {
Result.failure(result.error.toThrowable())
} else {
Expand All @@ -146,10 +153,11 @@ class WooPosOrdersDataSource @Inject constructor(

private suspend fun fetchOrdersFromRemote(
page: Int,
searchQuery: String?
searchQuery: String?,
count: Int = POS_ORDERS_PAGE_SIZE
) = restClient.fetchOrders(
site = selectedSite.get(),
count = POS_ORDERS_PAGE_SIZE,
count = count,
page = page,
orderBy = OrderBy.DATE,
sortOrder = OrderRestClient.SortOrder.DESCENDING,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,10 @@ class WooPosOrdersViewModel @Inject constructor(
searchInputState = WooPosSearchInputState.Closed
)
} else {
replaceOrders(result.ordersWithRefunds)
replaceOrders(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this change?

ordersWithRefunds = result.ordersWithRefunds,
paginationState = WooPosPaginationState.Loading
)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@ import com.woocommerce.android.ui.woopos.home.ParentToChildrenEvent.OrderSuccess
import com.woocommerce.android.ui.woopos.home.WooPosHomeUIEvent.ExitPosClicked
import com.woocommerce.android.ui.woopos.home.WooPosHomeUIEvent.SystemBackClicked
import com.woocommerce.android.ui.woopos.home.items.WooPosItemsViewModel.ItemClickedData
import com.woocommerce.android.ui.woopos.orders.WooPosOrdersDataSource
import com.woocommerce.android.ui.woopos.util.WooPosCoroutineTestRule
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.BackToCartTapped
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.ExitConfirmed
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.emptyFlow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.assertj.core.api.Assertions.assertThat
import org.junit.Rule
import org.mockito.kotlin.any
import org.mockito.kotlin.argThat
import org.mockito.kotlin.mock
import org.mockito.kotlin.verify
Expand All @@ -37,6 +40,7 @@ class WooPosHomeViewModelTest {
private val parentToChildrenEventSender: WooPosParentToChildrenEventSender = mock()
private val analyticsTracker: WooPosAnalyticsTracker = mock()
private val soundHelper: WooPosSoundHelper = mock()
private val ordersDataSource: WooPosOrdersDataSource = mock()

@Test
fun `when order created, then pass event to cart`() =
Expand Down Expand Up @@ -342,11 +346,13 @@ class WooPosHomeViewModelTest {
}

private fun createViewModel(): WooPosHomeViewModel {
whenever(ordersDataSource.loadOrders(any())).thenReturn(emptyFlow())
return WooPosHomeViewModel(
childrenToParentEventReceiver,
parentToChildrenEventSender,
analyticsTracker,
soundHelper,
ordersDataSource,
SavedStateHandle()
)
}
Expand Down