From a89047406358a15a5825536c28d05a9f09ebcd32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Thu, 4 Sep 2025 15:19:57 +0200 Subject: [PATCH 01/20] Enhance orders state and adapt clients --- .../ui/woopos/orders/WooPosOrdersScreen.kt | 53 ++++++++------ .../ui/woopos/orders/WooPosOrdersState.kt | 57 ++++++++++++--- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 70 ++++++++++++++----- 3 files changed, 129 insertions(+), 51 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 0ce21e64bb4f..878abcc4d003 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -27,8 +27,6 @@ import androidx.compose.ui.semantics.semantics import androidx.compose.ui.text.font.FontWeight import androidx.hilt.navigation.compose.hiltViewModel import com.woocommerce.android.R -import com.woocommerce.android.extensions.formatToDDMMMYYYY -import com.woocommerce.android.model.Order import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosText import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosToolbar @@ -59,8 +57,8 @@ fun WooPosOrdersScreen( onBackClicked = onBackClicked, ) - when { - state.isLoading -> { + when (val s = state) { + is WooPosOrdersState.Loading -> { Column( modifier = Modifier.fillMaxSize(), horizontalAlignment = Alignment.CenterHorizontally @@ -73,20 +71,22 @@ fun WooPosOrdersScreen( ) } } - state.error != null -> { + + is WooPosOrdersState.Error -> { Column( modifier = Modifier.fillMaxSize(), horizontalAlignment = Alignment.CenterHorizontally ) { WooPosText( - text = state.error ?: stringResource(R.string.error_generic), + text = s.message, style = WooPosTypography.BodyMedium, color = MaterialTheme.colorScheme.error, modifier = Modifier.padding(WooPosSpacing.Large.value) ) } } - state.orders.isEmpty() -> { + + is WooPosOrdersState.Empty -> { Column( modifier = Modifier.fillMaxSize(), horizontalAlignment = Alignment.CenterHorizontally @@ -99,10 +99,11 @@ fun WooPosOrdersScreen( ) } } - else -> { + + is WooPosOrdersState.Content -> { WooPosOrdersListPaneScreen( - orders = state.orders, - selectedOrderId = state.selectedOrderId, + items = s.items, + selectedOrderId = s.selectedOrderId, onOrderSelected = viewModel::onOrderSelected, modifier = Modifier.fillMaxSize() ) @@ -110,8 +111,14 @@ fun WooPosOrdersScreen( } } + // Right pane + val selectedItem: OrderItemViewState? = when (val s = state) { + is WooPosOrdersState.Content -> s.items.firstOrNull { it.id == s.selectedOrderId } + else -> null + } + WooPosOrdersDetailPaneScreen( - order = state.selectedOrder, + selected = selectedItem, modifier = Modifier .weight(0.7f) .fillMaxHeight() @@ -122,7 +129,7 @@ fun WooPosOrdersScreen( @Composable fun WooPosOrdersListPaneScreen( - orders: List, + items: List, selectedOrderId: Long?, onOrderSelected: (Long) -> Unit, modifier: Modifier = Modifier @@ -131,8 +138,8 @@ fun WooPosOrdersListPaneScreen( modifier = modifier, contentPadding = PaddingValues(vertical = WooPosSpacing.XSmall.value) ) { - items(orders, key = { it.id }) { order -> - val isSelected = order.id == selectedOrderId + items(items, key = { it.id }) { item -> + val isSelected = item.id == selectedOrderId val background = if (isSelected) { MaterialTheme.colorScheme.primaryContainer } else { @@ -150,7 +157,7 @@ fun WooPosOrdersListPaneScreen( .fillMaxWidth() .clip(MaterialTheme.shapes.medium) .background(background) - .clickable { onOrderSelected(order.id) } + .clickable { onOrderSelected(item.id) } .semantics { selected = isSelected } .padding( horizontal = WooPosSpacing.Medium.value, @@ -162,11 +169,12 @@ fun WooPosOrdersListPaneScreen( verticalArrangement = Arrangement.spacedBy(WooPosSpacing.XSmall.value) ) { WooPosText( - "Order #${order.number}", - style = WooPosTypography.BodyMedium + text = item.title, + style = WooPosTypography.BodyMedium, + color = foreground ) WooPosText( - text = order.dateCreated.formatToDDMMMYYYY(), + text = item.date, style = WooPosTypography.BodySmall, color = foreground ) @@ -175,7 +183,7 @@ fun WooPosOrdersListPaneScreen( Spacer(Modifier.weight(1f)) WooPosText( - text = "${order.total} ${order.currency}", + text = item.total, style = WooPosTypography.BodyMedium, modifier = Modifier.alignByBaseline() ) @@ -186,16 +194,15 @@ fun WooPosOrdersListPaneScreen( @Composable fun WooPosOrdersDetailPaneScreen( - order: Order?, + selected: OrderItemViewState?, modifier: Modifier = Modifier ) { Column( modifier = modifier.fillMaxSize() ) { WooPosToolbar( - modifier = Modifier - .fillMaxWidth(), - titleText = "Order #${order?.number ?: "--"}", + modifier = Modifier.fillMaxWidth(), + titleText = selected?.title ?: "--", titleFontWeight = FontWeight.Bold ) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index a10ddfdab633..aeecd122a18e 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -1,13 +1,50 @@ package com.woocommerce.android.ui.woopos.orders -import com.woocommerce.android.model.Order - -data class WooPosOrdersState( - val orders: List = emptyList(), - val selectedOrderId: Long? = null, - val isLoading: Boolean = false, - val error: String? = null -) { - val selectedOrder: Order? - get() = selectedOrderId?.let { id -> orders.firstOrNull { it.id == id } } +import androidx.compose.runtime.Immutable +import com.woocommerce.android.ui.woopos.home.items.WooPosPaginationState +import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState + +@Immutable +data class OrderItemViewState( + val id: Long, + val title: String, + val date: String, + val total: String, + val isSelected: Boolean +) + +@Immutable +sealed class WooPosOrdersState { + + @Immutable + data class Content( + val items: List, + val pullToRefreshState: WooPosPullToRefreshState, + val paginationState: WooPosPaginationState, + val selectedOrderId: Long?, + val listError: String? = null, + ) : WooPosOrdersState() + + @Immutable + data class Error( + val message: String, + val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled, + ) : WooPosOrdersState() { + val items: List = emptyList() + val paginationState: WooPosPaginationState = WooPosPaginationState.None + } + + @Immutable + data object Loading : WooPosOrdersState() { + val items: List = emptyList() + val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled + val paginationState: WooPosPaginationState = WooPosPaginationState.None + } + + @Immutable + data object Empty : WooPosOrdersState() { + val items: List = emptyList() + val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled + val paginationState: WooPosPaginationState = WooPosPaginationState.None + } } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index 5979d290597e..e40555fd21ad 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -2,11 +2,15 @@ package com.woocommerce.android.ui.woopos.orders import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import com.woocommerce.android.extensions.formatToDDMMMYYYY +import com.woocommerce.android.model.Order +import com.woocommerce.android.ui.woopos.home.items.WooPosPaginationState +import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.update +import kotlinx.coroutines.flow.collect import kotlinx.coroutines.launch import javax.inject.Inject @@ -15,41 +19,59 @@ class WooPosOrdersViewModel @Inject constructor( private val ordersDataSource: WooPosOrdersDataSource ) : ViewModel() { - private val _state = MutableStateFlow(WooPosOrdersState()) + private val _state = MutableStateFlow(WooPosOrdersState.Loading) val state: StateFlow = _state.asStateFlow() + // Keep the domain list & selection internally + private var orders: List = emptyList() + private var selectedId: Long? = null + init { refreshOrders() } fun onOrderSelected(orderId: Long) { - _state.update { it.copy(selectedOrderId = orderId) } + selectedId = orderId + val s = _state.value + if (s is WooPosOrdersState.Content) { + _state.value = s.copy( + items = orders.toOrderItems(selectedId), + selectedOrderId = selectedId + ) + } } fun refreshOrders() { viewModelScope.launch { - _state.update { it.copy(isLoading = true, error = null) } + _state.value = WooPosOrdersState.Loading ordersDataSource.loadOrders().collect { result -> when (result) { is LoadOrdersResult.Error -> { - _state.update { - it.copy( - isLoading = false, - error = result.message ?: "Unknown error" - ) - } + // If we already had content (e.g., cache emitted first) keep it and surface an error? + // For now, match current behavior: show an error state. + _state.value = WooPosOrdersState.Error( + message = result.message + ) } is LoadOrdersResult.Success -> { - val list = result.orders - _state.update { prev -> - prev.copy( - isLoading = false, - orders = list, - selectedOrderId = prev.selectedOrderId?.takeIf { id -> - list.any { o -> o.id == id } - } ?: list.firstOrNull()?.id + orders = result.orders + + // Preserve previous selection if still present; otherwise pick first + selectedId = selectedId?.takeIf { id -> orders.any { it.id == id } } + ?: orders.firstOrNull()?.id + + if (orders.isEmpty()) { + _state.value = WooPosOrdersState.Empty + } else { + _state.value = WooPosOrdersState.Content( + items = orders.toOrderItems(selectedId), + selectedOrderId = selectedId, + // PTR/pagination are for later; set sensible defaults + pullToRefreshState = WooPosPullToRefreshState.Enabled, + paginationState = WooPosPaginationState.None, + listError = null ) } } @@ -58,3 +80,15 @@ class WooPosOrdersViewModel @Inject constructor( } } } + +/** Map domain orders to lightweight UI rows, deriving selection from a single source of truth. */ +private fun List.toOrderItems(selectedId: Long?): List = + map { o -> + OrderItemViewState( + id = o.id, + title = "Order #${o.number}", + date = o.dateCreated.formatToDDMMMYYYY(), + total = "${o.total} ${o.currency}", + isSelected = o.id == selectedId + ) + } From cc911fc3c8e104c3f20c13204c9750ab21eac798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Thu, 4 Sep 2025 15:54:45 +0200 Subject: [PATCH 02/20] Implement Pull To Refresh --- .../woopos/orders/WooPosOrdersDataSource.kt | 15 +- .../ui/woopos/orders/WooPosOrdersScreen.kt | 162 ++++++++++-------- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 47 ++--- 3 files changed, 121 insertions(+), 103 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt index 9d35cc160061..6c6330fa9821 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt @@ -21,9 +21,11 @@ class WooPosOrdersDataSource @Inject constructor( private val orderMapper: OrderMapper, private val ordersCache: WooPosOrdersCache ) { - suspend fun loadOrders(): Flow = flow { - val cached = ordersCache.getAll() - emit(LoadOrdersResult.Success(cached)) + suspend fun loadOrders(loadFromCacheFirst: Boolean): Flow = flow { + if (loadFromCacheFirst) { + val cached = ordersCache.getAll() + emit(LoadOrdersResult.Success(cached)) + } val result = restClient.fetchOrders( site = selectedSite.get(), @@ -40,11 +42,10 @@ class WooPosOrdersDataSource @Inject constructor( } else { val mapped = result.orders.toAppModels() ordersCache.addAll(mapped) - emit(LoadOrdersResult.Success(result.orders.toAppModels())) + emit(LoadOrdersResult.Success(mapped)) } } - private suspend fun List?.toAppModels(): List = this?.map { - orderMapper.toAppModel(it) - } ?: emptyList() + private suspend fun List?.toAppModels(): List = + this?.map { orderMapper.toAppModel(it) } ?: emptyList() } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 878abcc4d003..15f970141e6d 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -4,6 +4,7 @@ import androidx.activity.compose.BackHandler import androidx.compose.foundation.background import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.Row @@ -14,6 +15,10 @@ import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.items +import androidx.compose.material.ExperimentalMaterialApi +import androidx.compose.material.pullrefresh.PullRefreshIndicator +import androidx.compose.material.pullrefresh.pullRefresh +import androidx.compose.material.pullrefresh.rememberPullRefreshState import androidx.compose.material3.MaterialTheme import androidx.compose.runtime.Composable import androidx.compose.runtime.collectAsState @@ -33,8 +38,10 @@ import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosToolba import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosSpacing import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTheme import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTypography +import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent +@OptIn(ExperimentalMaterialApi::class) @Composable fun WooPosOrdersScreen( onNavigationEvent: (WooPosNavigationEvent) -> Unit, @@ -46,6 +53,7 @@ fun WooPosOrdersScreen( BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) } Row(modifier = Modifier.fillMaxSize()) { + // Left pane Column( modifier = Modifier .weight(0.3f) @@ -57,57 +65,88 @@ fun WooPosOrdersScreen( onBackClicked = onBackClicked, ) - when (val s = state) { - is WooPosOrdersState.Loading -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = stringResource(R.string.loading), - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(WooPosSpacing.Large.value) - ) - } + // --- Pull-to-refresh wiring (left pane content area) --- + val refreshing = + when (val s = state) { + is WooPosOrdersState.Content -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing + is WooPosOrdersState.Error -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing + is WooPosOrdersState.Empty -> false + is WooPosOrdersState.Loading -> false } - is WooPosOrdersState.Error -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = s.message, - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.error, - modifier = Modifier.padding(WooPosSpacing.Large.value) - ) + val pullRefreshState = rememberPullRefreshState( + refreshing = refreshing, + onRefresh = { viewModel.refresh() } + ) + + Box( + modifier = Modifier + .fillMaxSize() + .pullRefresh(pullRefreshState) + ) { + when (val s = state) { + is WooPosOrdersState.Loading -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = stringResource(R.string.loading), + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(WooPosSpacing.Large.value) + ) + } + } + + is WooPosOrdersState.Error -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = s.message, + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(WooPosSpacing.Large.value) + ) + } + } + + is WooPosOrdersState.Empty -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = "No orders found", + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(WooPosSpacing.Large.value) + ) + } } - } - is WooPosOrdersState.Empty -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = "No Orders Found", - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(WooPosSpacing.Large.value) + is WooPosOrdersState.Content -> { + WooPosOrdersListPaneScreen( + items = s.items, + selectedOrderId = s.selectedOrderId, + onOrderSelected = viewModel::onOrderSelected, + modifier = Modifier.fillMaxSize() ) } } - is WooPosOrdersState.Content -> { - WooPosOrdersListPaneScreen( - items = s.items, - selectedOrderId = s.selectedOrderId, - onOrderSelected = viewModel::onOrderSelected, - modifier = Modifier.fillMaxSize() - ) - } + // PTR indicator at the top of the list area + PullRefreshIndicator( + refreshing = refreshing, + state = pullRefreshState, + modifier = Modifier + .align(Alignment.TopCenter) + .padding(top = WooPosSpacing.XSmall.value), + backgroundColor = MaterialTheme.colorScheme.surface, + contentColor = MaterialTheme.colorScheme.primary + ) } } @@ -145,7 +184,6 @@ fun WooPosOrdersListPaneScreen( } else { MaterialTheme.colorScheme.surface } - val foreground = if (isSelected) { MaterialTheme.colorScheme.onPrimaryContainer } else { @@ -165,23 +203,11 @@ fun WooPosOrdersListPaneScreen( ), verticalAlignment = Alignment.Top ) { - Column( - verticalArrangement = Arrangement.spacedBy(WooPosSpacing.XSmall.value) - ) { - WooPosText( - text = item.title, - style = WooPosTypography.BodyMedium, - color = foreground - ) - WooPosText( - text = item.date, - style = WooPosTypography.BodySmall, - color = foreground - ) + Column(verticalArrangement = Arrangement.spacedBy(WooPosSpacing.XSmall.value)) { + WooPosText(item.title, style = WooPosTypography.BodyMedium, color = foreground) + WooPosText(item.date, style = WooPosTypography.BodySmall, color = foreground) } - Spacer(Modifier.weight(1f)) - WooPosText( text = item.total, style = WooPosTypography.BodyMedium, @@ -197,25 +223,19 @@ fun WooPosOrdersDetailPaneScreen( selected: OrderItemViewState?, modifier: Modifier = Modifier ) { - Column( - modifier = modifier.fillMaxSize() - ) { + Column(modifier = modifier.fillMaxSize()) { WooPosToolbar( modifier = Modifier.fillMaxWidth(), titleText = selected?.title ?: "--", titleFontWeight = FontWeight.Bold ) - Column( modifier = Modifier .fillMaxSize() - .padding( - start = WooPosSpacing.Large.value, - end = WooPosSpacing.Large.value, - ) + .padding(start = WooPosSpacing.Large.value, end = WooPosSpacing.Large.value) ) { WooPosText( - text = "Orders details will be displayed here", + text = "Order details goes here", style = WooPosTypography.BodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant ) @@ -226,9 +246,5 @@ fun WooPosOrdersDetailPaneScreen( @WooPosPreview @Composable fun WooPosOrdersScreenPreview() { - WooPosTheme { - WooPosOrdersScreen( - onNavigationEvent = {} - ) - } + WooPosTheme { WooPosOrdersScreen(onNavigationEvent = {}) } } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index e40555fd21ad..670c4856bab0 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -10,7 +10,6 @@ import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.launch import javax.inject.Inject @@ -22,12 +21,11 @@ class WooPosOrdersViewModel @Inject constructor( private val _state = MutableStateFlow(WooPosOrdersState.Loading) val state: StateFlow = _state.asStateFlow() - // Keep the domain list & selection internally private var orders: List = emptyList() private var selectedId: Long? = null init { - refreshOrders() + loadOrders(loadFromCacheFirst = true) } fun onOrderSelected(orderId: Long) { @@ -41,34 +39,38 @@ class WooPosOrdersViewModel @Inject constructor( } } - fun refreshOrders() { - viewModelScope.launch { - _state.value = WooPosOrdersState.Loading + fun refresh() { + val s = _state.value + _state.value = when (s) { + is WooPosOrdersState.Content -> s.copy(pullToRefreshState = WooPosPullToRefreshState.Refreshing) + else -> WooPosOrdersState.Loading + } + loadOrders(loadFromCacheFirst = false) + } - ordersDataSource.loadOrders().collect { result -> + private fun loadOrders(loadFromCacheFirst: Boolean) { + viewModelScope.launch { + ordersDataSource.loadOrders(loadFromCacheFirst).collect { result -> when (result) { is LoadOrdersResult.Error -> { - // If we already had content (e.g., cache emitted first) keep it and surface an error? - // For now, match current behavior: show an error state. - _state.value = WooPosOrdersState.Error( - message = result.message - ) + _state.value = WooPosOrdersState.Error(message = result.message) } - is LoadOrdersResult.Success -> { - orders = result.orders - - // Preserve previous selection if still present; otherwise pick first - selectedId = selectedId?.takeIf { id -> orders.any { it.id == id } } - ?: orders.firstOrNull()?.id + val newOrders = result.orders + orders = newOrders + selectedId = selectedId?.takeIf { id -> newOrders.any { it.id == id } } + ?: newOrders.firstOrNull()?.id - if (orders.isEmpty()) { - _state.value = WooPosOrdersState.Empty + if (newOrders.isEmpty()) { + if (loadFromCacheFirst) { + _state.value = WooPosOrdersState.Loading + } else { + _state.value = WooPosOrdersState.Empty + } } else { _state.value = WooPosOrdersState.Content( - items = orders.toOrderItems(selectedId), + items = newOrders.toOrderItems(selectedId), selectedOrderId = selectedId, - // PTR/pagination are for later; set sensible defaults pullToRefreshState = WooPosPullToRefreshState.Enabled, paginationState = WooPosPaginationState.None, listError = null @@ -81,7 +83,6 @@ class WooPosOrdersViewModel @Inject constructor( } } -/** Map domain orders to lightweight UI rows, deriving selection from a single source of truth. */ private fun List.toOrderItems(selectedId: Long?): List = map { o -> OrderItemViewState( From 73ba760752d23cf0062875f53e282374bfdfba76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Thu, 4 Sep 2025 16:30:43 +0200 Subject: [PATCH 03/20] Add unit tests --- .../woopos/orders/WooPosOrdersDataSource.kt | 2 +- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 8 +- .../orders/WooPosOrdersDataSourceTest.kt | 154 ++++++++++----- .../orders/WooPosOrdersViewModelTest.kt | 179 ++++++++++++++++++ 4 files changed, 296 insertions(+), 47 deletions(-) create mode 100644 WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt index 6c6330fa9821..2d9edc9553a2 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt @@ -21,7 +21,7 @@ class WooPosOrdersDataSource @Inject constructor( private val orderMapper: OrderMapper, private val ordersCache: WooPosOrdersCache ) { - suspend fun loadOrders(loadFromCacheFirst: Boolean): Flow = flow { + fun loadOrders(loadFromCacheFirst: Boolean): Flow = flow { if (loadFromCacheFirst) { val cached = ordersCache.getAll() emit(LoadOrdersResult.Success(cached)) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index 670c4856bab0..c27c475c4e76 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -50,6 +50,8 @@ class WooPosOrdersViewModel @Inject constructor( private fun loadOrders(loadFromCacheFirst: Boolean) { viewModelScope.launch { + var sawCacheEmission = false + ordersDataSource.loadOrders(loadFromCacheFirst).collect { result -> when (result) { is LoadOrdersResult.Error -> { @@ -62,7 +64,7 @@ class WooPosOrdersViewModel @Inject constructor( ?: newOrders.firstOrNull()?.id if (newOrders.isEmpty()) { - if (loadFromCacheFirst) { + if (loadFromCacheFirst && !sawCacheEmission) { _state.value = WooPosOrdersState.Loading } else { _state.value = WooPosOrdersState.Empty @@ -76,6 +78,10 @@ class WooPosOrdersViewModel @Inject constructor( listError = null ) } + + if (loadFromCacheFirst && !sawCacheEmission) { + sawCacheEmission = true + } } } } diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt index 83e65c3ceee4..0b07eb62e859 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt @@ -45,19 +45,13 @@ class WooPosOrdersDataSourceTest { ) @Test - fun `given cache and successful fetch, when loadOrders collected, then emits cache first then mapped network and stores in cache`() = runTest { - // GIVEN + fun `given loadFromCacheFirst true and cache exists, when fetch succeeds, then emit cache then mapped network and store in cache`() = runTest { val cachedOrder = OrderTestUtils.generateTestOrder() whenever(ordersCache.getAll()).thenReturn(listOf(cachedOrder)) - // Network returns two entities val e1 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 1) val e2 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 2) - val entities = listOf( - e1 to emptyList(), - e2 to emptyList() - ) - + val entities = listOf(e1 to emptyList(), e2 to emptyList()) val firstOrder = OrderTestUtils.generateTestOrder() val secondOrder = OrderTestUtils.generateTestOrder() @@ -68,7 +62,6 @@ class WooPosOrdersDataSourceTest { site = siteModel, ordersWithMeta = entities ) - whenever( orderRestClient.fetchOrders( site = eq(siteModel), @@ -81,20 +74,14 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - // WHEN - val emissions = sut.loadOrders().toList(mutableListOf()) + val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) - // THEN assertThat(emissions).hasSize(2) - // First emission = cache - val first = emissions[0] - assertThat(first).isInstanceOf(LoadOrdersResult.Success::class.java) - assertThat((first as LoadOrdersResult.Success).orders).containsExactly(cachedOrder) + val first = emissions[0] as LoadOrdersResult.Success + assertThat(first.orders).containsExactly(cachedOrder) - // Second emission = network mapped - val second = emissions[1] - assertThat(second).isInstanceOf(LoadOrdersResult.Success::class.java) - assertThat((second as LoadOrdersResult.Success).orders).containsExactly(firstOrder, secondOrder) + val second = emissions[1] as LoadOrdersResult.Success + assertThat(second.orders).containsExactly(firstOrder, secondOrder) verify(selectedSite).get() verify(ordersCache).getAll() @@ -111,8 +98,7 @@ class WooPosOrdersDataSourceTest { } @Test - fun `given cache and fetch error, when loadOrders collected, then emits cache then error without caching`() = runTest { - // GIVEN + fun `given loadFromCacheFirst true and cache exists, when fetch fails, then emit cache then error without storing`() = runTest { val cachedOrder = OrderTestUtils.generateTestOrder() whenever(ordersCache.getAll()).thenReturn(listOf(cachedOrder)) @@ -120,12 +106,10 @@ class WooPosOrdersDataSourceTest { type = WCOrderStore.OrderErrorType.GENERIC_ERROR, message = "generic error" ) - val payload = WCOrderStore.FetchOrdersResponsePayload( error = orderError, site = siteModel ) - whenever( orderRestClient.fetchOrders( site = eq(siteModel), @@ -138,19 +122,14 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - // WHEN - val emissions = sut.loadOrders().toList(mutableListOf()) + val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) - // THEN assertThat(emissions).hasSize(2) + val first = emissions[0] as LoadOrdersResult.Success + assertThat(first.orders).containsExactly(cachedOrder) - val first = emissions[0] - assertThat(first).isInstanceOf(LoadOrdersResult.Success::class.java) - assertThat((first as LoadOrdersResult.Success).orders).containsExactly(cachedOrder) - - val second = emissions[1] - assertThat(second).isInstanceOf(LoadOrdersResult.Error::class.java) - assertThat((second as LoadOrdersResult.Error).message).isEqualTo("generic error") + val second = emissions[1] as LoadOrdersResult.Error + assertThat(second.message).isEqualTo("generic error") verify(ordersCache).getAll() verify(ordersCache, never()).addAll(any()) @@ -166,8 +145,7 @@ class WooPosOrdersDataSourceTest { } @Test - fun `given empty cache, when loadOrders collected, then forwards params including createdVia and emits empty then empty`() = runTest { - // GIVEN + fun `given loadFromCacheFirst true and empty cache, when fetch returns empty, then emit empty then empty`() = runTest { whenever(ordersCache.getAll()).thenReturn(emptyList()) val payload = WCOrderStore.FetchOrdersResponsePayload( @@ -186,19 +164,15 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - // WHEN - val emissions = sut.loadOrders().toList(mutableListOf()) + val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) - // THEN assertThat(emissions).hasSize(2) - val first = emissions[0] - assertThat(first).isInstanceOf(LoadOrdersResult.Success::class.java) - assertThat((first as LoadOrdersResult.Success).orders).isEmpty() + val first = emissions[0] as LoadOrdersResult.Success + assertThat(first.orders).isEmpty() - val second = emissions[1] - assertThat(second).isInstanceOf(LoadOrdersResult.Success::class.java) - assertThat((second as LoadOrdersResult.Success).orders).isEmpty() + val second = emissions[1] as LoadOrdersResult.Success + assertThat(second.orders).isEmpty() verify(selectedSite).get() verify(ordersCache).getAll() @@ -213,4 +187,94 @@ class WooPosOrdersDataSourceTest { createdVia = eq("pos-rest-api") ) } + + @Test + fun `given loadFromCacheFirst false, when fetch succeeds, then emit only mapped network and do not read cache`() = runTest { + val e1 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 11) + val e2 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 22) + val entities = listOf(e1 to emptyList(), e2 to emptyList()) + + val firstOrder = OrderTestUtils.generateTestOrder() + val secondOrder = OrderTestUtils.generateTestOrder() + + whenever(orderMapper.toAppModel(e1)).thenReturn(firstOrder) + whenever(orderMapper.toAppModel(e2)).thenReturn(secondOrder) + + val payload = WCOrderStore.FetchOrdersResponsePayload( + site = siteModel, + ordersWithMeta = entities + ) + whenever( + orderRestClient.fetchOrders( + site = eq(siteModel), + count = eq(25), + page = eq(1), + orderBy = any(), + sortOrder = any(), + statusFilter = anyOrNull(), + createdVia = eq("pos-rest-api") + ) + ).thenReturn(payload) + + val emissions = sut.loadOrders(loadFromCacheFirst = false).toList(mutableListOf()) + + assertThat(emissions).hasSize(1) + val only = emissions.first() as LoadOrdersResult.Success + assertThat(only.orders).containsExactly(firstOrder, secondOrder) + + verify(ordersCache, never()).getAll() + verify(ordersCache).addAll(listOf(firstOrder, secondOrder)) + verify(selectedSite).get() + verify(orderRestClient).fetchOrders( + site = eq(siteModel), + count = eq(25), + page = eq(1), + orderBy = any(), + sortOrder = any(), + statusFilter = anyOrNull(), + createdVia = eq("pos-rest-api") + ) + } + + @Test + fun `given loadFromCacheFirst false, when fetch fails, then emit only error and do not read cache`() = runTest { + val orderError = WCOrderStore.OrderError( + type = WCOrderStore.OrderErrorType.GENERIC_ERROR, + message = "boom" + ) + val payload = WCOrderStore.FetchOrdersResponsePayload( + error = orderError, + site = siteModel + ) + whenever( + orderRestClient.fetchOrders( + site = eq(siteModel), + count = eq(25), + page = eq(1), + orderBy = any(), + sortOrder = any(), + statusFilter = anyOrNull(), + createdVia = eq("pos-rest-api") + ) + ).thenReturn(payload) + + val emissions = sut.loadOrders(loadFromCacheFirst = false).toList(mutableListOf()) + + assertThat(emissions).hasSize(1) + val only = emissions.first() as LoadOrdersResult.Error + assertThat(only.message).isEqualTo("boom") + + verify(ordersCache, never()).getAll() + verify(ordersCache, never()).addAll(any()) + verify(selectedSite).get() + verify(orderRestClient).fetchOrders( + site = eq(siteModel), + count = eq(25), + page = eq(1), + orderBy = any(), + sortOrder = any(), + statusFilter = anyOrNull(), + createdVia = eq("pos-rest-api") + ) + } } diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt new file mode 100644 index 000000000000..01b1dcef9e8f --- /dev/null +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt @@ -0,0 +1,179 @@ +package com.woocommerce.android.ui.woopos.orders + +import com.woocommerce.android.model.Order +import com.woocommerce.android.ui.orders.OrderTestUtils +import com.woocommerce.android.ui.woopos.home.items.WooPosPaginationState +import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState +import com.woocommerce.android.ui.woopos.util.WooPosCoroutineTestRule +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +@OptIn(ExperimentalCoroutinesApi::class) +class WooPosOrdersViewModelTest { + + @Rule + @JvmField + val coroutineTestRule = WooPosCoroutineTestRule() + + private val dataSource: WooPosOrdersDataSource = mock() + + private lateinit var viewModel: WooPosOrdersViewModel + + private fun order(id: Long = 1L): Order = OrderTestUtils.generateTestOrder(orderId = id) + + @Before + fun setUp() { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(1), order(2)))) } + ) + } + + @Test + fun `given cache and network data, when init, then final state shows network content`() = runTest { + val cached = listOf(order(1)) + val network = listOf(order(2), order(3)) + + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { + emit(LoadOrdersResult.Success(cached)) + emit(LoadOrdersResult.Success(network)) + } + ) + + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + val state = viewModel.state.value + assertThat(state).isInstanceOf(WooPosOrdersState.Content::class.java) + val content = state as WooPosOrdersState.Content + assertThat(content.items.map { it.id }).containsExactly(2L, 3L) + assertThat(content.pullToRefreshState).isEqualTo(WooPosPullToRefreshState.Enabled) + assertThat(content.paginationState).isEqualTo(WooPosPaginationState.None) + verify(dataSource).loadOrders(eq(true)) + } + + @Test + fun `given empty cache and non-empty network, when init, then final state shows network content`() = runTest { + val network = listOf(order(10)) + + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { + emit(LoadOrdersResult.Success(emptyList())) // cache + emit(LoadOrdersResult.Success(network)) // network + } + ) + + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + val state = viewModel.state.value + assertThat(state).isInstanceOf(WooPosOrdersState.Content::class.java) + val content = state as WooPosOrdersState.Content + assertThat(content.items.map { it.id }).containsExactly(10L) + } + + @Test + fun `given empty cache and empty network, when init, then final state is Empty`() = runTest { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { + emit(LoadOrdersResult.Success(emptyList())) // cache + emit(LoadOrdersResult.Success(emptyList())) // network + } + ) + + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + val state = viewModel.state.value + assertThat(state).isInstanceOf(WooPosOrdersState.Empty::class.java) + } + + @Test + fun `given data source error, when init, then state is Error`() = runTest { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { emit(LoadOrdersResult.Error("boom")) } + ) + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + val state = viewModel.state.value + assertThat(state).isInstanceOf(WooPosOrdersState.Error::class.java) + val error = state as WooPosOrdersState.Error + assertThat(error.message).isEqualTo("boom") + } + + @Test + fun `given initial content, when refresh, then skip cache and update with network only`() = runTest { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(1)))) } + ) + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + val before = viewModel.state.value as WooPosOrdersState.Content + assertThat(before.items.map { it.id }).containsExactly(1L) + + whenever(dataSource.loadOrders(eq(false))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(5), order(6)))) } + ) + + viewModel.refresh() + advanceUntilIdle() + + val after = viewModel.state.value as WooPosOrdersState.Content + assertThat(after.items.map { it.id }).containsExactly(5L, 6L) + assertThat(after.pullToRefreshState).isEqualTo(WooPosPullToRefreshState.Enabled) + verify(dataSource).loadOrders(eq(false)) + } + + @Test + fun `given orders loaded, when selecting an order, then selected id and flags update`() = runTest { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(1), order(2), order(3)))) } + ) + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + viewModel.onOrderSelected(3L) + advanceUntilIdle() + + val state = viewModel.state.value as WooPosOrdersState.Content + assertThat(state.selectedOrderId).isEqualTo(3L) + val selectedFlags = state.items.associate { it.id to it.isSelected } + assertThat(selectedFlags[1L]).isFalse() + assertThat(selectedFlags[2L]).isFalse() + assertThat(selectedFlags[3L]).isTrue() + } + + @Test + fun `given selection removed after reload, when refreshing, then first item is auto selected`() = runTest { + whenever(dataSource.loadOrders(eq(true))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(100), order(200)))) } + ) + viewModel = WooPosOrdersViewModel(dataSource) + advanceUntilIdle() + + viewModel.onOrderSelected(200L) + advanceUntilIdle() + + whenever(dataSource.loadOrders(eq(false))).thenReturn( + flow { emit(LoadOrdersResult.Success(listOf(order(300), order(400)))) } + ) + + viewModel.refresh() + advanceUntilIdle() + + val state = viewModel.state.value as WooPosOrdersState.Content + assertThat(state.items.map { it.id }).containsExactly(300L, 400L) + assertThat(state.selectedOrderId).isEqualTo(300L) + } +} From 313a64cadea891775b0c1fe4fdde360ba3595760 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Thu, 4 Sep 2025 16:37:47 +0200 Subject: [PATCH 04/20] Remove non necessary comment --- .../woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 15f970141e6d..0289c5d32259 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -65,7 +65,6 @@ fun WooPosOrdersScreen( onBackClicked = onBackClicked, ) - // --- Pull-to-refresh wiring (left pane content area) --- val refreshing = when (val s = state) { is WooPosOrdersState.Content -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing From 0df5676e226f469c9775b4ed1d3903407127beb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 16:56:26 +0200 Subject: [PATCH 05/20] Fix tests --- .../orders/WooPosOrdersDataSourceTest.kt | 150 ++++-------------- .../orders/WooPosOrdersViewModelTest.kt | 92 +++++++---- 2 files changed, 91 insertions(+), 151 deletions(-) diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt index 8801d1b8ab69..4f97d2d9124b 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt @@ -27,8 +27,7 @@ import kotlin.test.Test @OptIn(ExperimentalCoroutinesApi::class) class WooPosOrdersDataSourceTest { - @Rule - @JvmField + @Rule @JvmField val coroutinesTestRule = WooPosCoroutineTestRule() private val orderRestClient: OrderRestClient = mock() @@ -45,18 +44,17 @@ class WooPosOrdersDataSourceTest { ) @Test - fun `given loadFromCacheFirst true and cache exists, when fetch succeeds, then emit cache then mapped network and store in cache`() = runTest { + fun `when cache has data and fetch succeeds, then emit SuccessCache then SuccessRemote and store mapped in cache`() = runTest { val cachedOrder = OrderTestUtils.generateTestOrder() whenever(ordersCache.getAll()).thenReturn(listOf(cachedOrder)) - val e1 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 1) - val e2 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 2) + val e1 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 11) + val e2 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 22) val entities = listOf(e1 to emptyList(), e2 to emptyList()) - val firstOrder = OrderTestUtils.generateTestOrder() - val secondOrder = OrderTestUtils.generateTestOrder() - - whenever(orderMapper.toAppModel(e1)).thenReturn(firstOrder) - whenever(orderMapper.toAppModel(e2)).thenReturn(secondOrder) + val mapped1 = OrderTestUtils.generateTestOrder() + val mapped2 = OrderTestUtils.generateTestOrder() + whenever(orderMapper.toAppModel(e1)).thenReturn(mapped1) + whenever(orderMapper.toAppModel(e2)).thenReturn(mapped2) val payload = WCOrderStore.FetchOrdersResponsePayload( site = siteModel, @@ -65,7 +63,7 @@ class WooPosOrdersDataSourceTest { whenever( orderRestClient.fetchOrders( site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), @@ -74,21 +72,24 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) + // WHEN + val emissions = sut.loadOrders().toList(mutableListOf()) + // THEN assertThat(emissions).hasSize(2) - val first = emissions[0] as LoadOrdersResult.Success + + val first = emissions[0] as LoadOrdersResult.SuccessCache assertThat(first.orders).containsExactly(cachedOrder) - val second = emissions[1] as LoadOrdersResult.Success - assertThat(second.orders).containsExactly(firstOrder, secondOrder) + val second = emissions[1] as LoadOrdersResult.SuccessRemote + assertThat(second.orders).containsExactly(mapped1, mapped2) verify(selectedSite).get() verify(ordersCache).getAll() - verify(ordersCache).setAll(listOf(firstOrder, secondOrder)) + verify(ordersCache).setAll(listOf(mapped1, mapped2)) verify(orderRestClient).fetchOrders( site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), @@ -98,7 +99,7 @@ class WooPosOrdersDataSourceTest { } @Test - fun `given loadFromCacheFirst true and cache exists, when fetch fails, then emit cache then error without storing`() = runTest { + fun `when cache has data and fetch fails, then emit SuccessCache then Error and do not store`() = runTest { val cachedOrder = OrderTestUtils.generateTestOrder() whenever(ordersCache.getAll()).thenReturn(listOf(cachedOrder)) @@ -113,7 +114,7 @@ class WooPosOrdersDataSourceTest { whenever( orderRestClient.fetchOrders( site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), @@ -122,10 +123,13 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) + // WHEN + val emissions = sut.loadOrders().toList(mutableListOf()) + // THEN assertThat(emissions).hasSize(2) - val first = emissions[0] as LoadOrdersResult.Success + + val first = emissions[0] as LoadOrdersResult.SuccessCache assertThat(first.orders).containsExactly(cachedOrder) val second = emissions[1] as LoadOrdersResult.Error @@ -135,7 +139,7 @@ class WooPosOrdersDataSourceTest { verify(ordersCache, never()).setAll(any()) verify(orderRestClient).fetchOrders( site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), @@ -145,7 +149,7 @@ class WooPosOrdersDataSourceTest { } @Test - fun `given loadFromCacheFirst true and empty cache, when fetch returns empty, then emit empty then empty`() = runTest { + fun `when cache empty and fetch returns empty, then emit SuccessCache empty then SuccessRemote empty and store empty`() = runTest { whenever(ordersCache.getAll()).thenReturn(emptyList()) val payload = WCOrderStore.FetchOrdersResponsePayload( @@ -155,7 +159,7 @@ class WooPosOrdersDataSourceTest { whenever( orderRestClient.fetchOrders( site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), @@ -164,14 +168,14 @@ class WooPosOrdersDataSourceTest { ) ).thenReturn(payload) - val emissions = sut.loadOrders(loadFromCacheFirst = true).toList(mutableListOf()) + val emissions = sut.loadOrders().toList(mutableListOf()) assertThat(emissions).hasSize(2) - val first = emissions[0] as LoadOrdersResult.Success + val first = emissions[0] as LoadOrdersResult.SuccessCache assertThat(first.orders).isEmpty() - val second = emissions[1] as LoadOrdersResult.Success + val second = emissions[1] as LoadOrdersResult.SuccessRemote assertThat(second.orders).isEmpty() verify(selectedSite).get() @@ -179,97 +183,7 @@ class WooPosOrdersDataSourceTest { verify(ordersCache).setAll(emptyList()) verify(orderRestClient).fetchOrders( site = eq(siteModel), - count = eq(25), - page = eq(1), - orderBy = any(), - sortOrder = any(), - statusFilter = anyOrNull(), - createdVia = eq("pos-rest-api") - ) - } - - @Test - fun `given loadFromCacheFirst false, when fetch succeeds, then emit only mapped network and do not read cache`() = runTest { - val e1 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 11) - val e2 = OrderEntity(localSiteId = LocalOrRemoteId.LocalId(1), 22) - val entities = listOf(e1 to emptyList(), e2 to emptyList()) - - val firstOrder = OrderTestUtils.generateTestOrder() - val secondOrder = OrderTestUtils.generateTestOrder() - - whenever(orderMapper.toAppModel(e1)).thenReturn(firstOrder) - whenever(orderMapper.toAppModel(e2)).thenReturn(secondOrder) - - val payload = WCOrderStore.FetchOrdersResponsePayload( - site = siteModel, - ordersWithMeta = entities - ) - whenever( - orderRestClient.fetchOrders( - site = eq(siteModel), - count = eq(25), - page = eq(1), - orderBy = any(), - sortOrder = any(), - statusFilter = anyOrNull(), - createdVia = eq("pos-rest-api") - ) - ).thenReturn(payload) - - val emissions = sut.loadOrders(loadFromCacheFirst = false).toList(mutableListOf()) - - assertThat(emissions).hasSize(1) - val only = emissions.first() as LoadOrdersResult.Success - assertThat(only.orders).containsExactly(firstOrder, secondOrder) - - verify(ordersCache, never()).getAll() - verify(ordersCache).addAll(listOf(firstOrder, secondOrder)) - verify(selectedSite).get() - verify(orderRestClient).fetchOrders( - site = eq(siteModel), - count = eq(25), - page = eq(1), - orderBy = any(), - sortOrder = any(), - statusFilter = anyOrNull(), - createdVia = eq("pos-rest-api") - ) - } - - @Test - fun `given loadFromCacheFirst false, when fetch fails, then emit only error and do not read cache`() = runTest { - val orderError = WCOrderStore.OrderError( - type = WCOrderStore.OrderErrorType.GENERIC_ERROR, - message = "boom" - ) - val payload = WCOrderStore.FetchOrdersResponsePayload( - error = orderError, - site = siteModel - ) - whenever( - orderRestClient.fetchOrders( - site = eq(siteModel), - count = eq(25), - page = eq(1), - orderBy = any(), - sortOrder = any(), - statusFilter = anyOrNull(), - createdVia = eq("pos-rest-api") - ) - ).thenReturn(payload) - - val emissions = sut.loadOrders(loadFromCacheFirst = false).toList(mutableListOf()) - - assertThat(emissions).hasSize(1) - val only = emissions.first() as LoadOrdersResult.Error - assertThat(only.message).isEqualTo("boom") - - verify(ordersCache, never()).getAll() - verify(ordersCache, never()).addAll(any()) - verify(selectedSite).get() - verify(orderRestClient).fetchOrders( - site = eq(siteModel), - count = eq(25), + count = eq(WooPosOrdersDataSource.POS_ORDERS_PAGE_SIZE), page = eq(1), orderBy = any(), sortOrder = any(), diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt index 01b1dcef9e8f..278d51fbb317 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt @@ -13,16 +13,14 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Rule import org.junit.Test -import org.mockito.kotlin.eq -import org.mockito.kotlin.mock +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @OptIn(ExperimentalCoroutinesApi::class) class WooPosOrdersViewModelTest { - @Rule - @JvmField + @Rule @JvmField val coroutineTestRule = WooPosCoroutineTestRule() private val dataSource: WooPosOrdersDataSource = mock() @@ -33,79 +31,92 @@ class WooPosOrdersViewModelTest { @Before fun setUp() { - whenever(dataSource.loadOrders(eq(true))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(1), order(2)))) } + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( + flow { emit(LoadOrdersResult.SuccessCache(listOf(order(1), order(2)))) } ) } @Test fun `given cache and network data, when init, then final state shows network content`() = runTest { + // GIVEN val cached = listOf(order(1)) val network = listOf(order(2), order(3)) - - whenever(dataSource.loadOrders(eq(true))).thenReturn( + whenever(dataSource.loadOrders()).thenReturn( flow { - emit(LoadOrdersResult.Success(cached)) - emit(LoadOrdersResult.Success(network)) + emit(LoadOrdersResult.SuccessCache(cached)) + emit(LoadOrdersResult.SuccessRemote(network)) } ) + // WHEN viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() + // THEN val state = viewModel.state.value assertThat(state).isInstanceOf(WooPosOrdersState.Content::class.java) val content = state as WooPosOrdersState.Content assertThat(content.items.map { it.id }).containsExactly(2L, 3L) assertThat(content.pullToRefreshState).isEqualTo(WooPosPullToRefreshState.Enabled) assertThat(content.paginationState).isEqualTo(WooPosPaginationState.None) - verify(dataSource).loadOrders(eq(true)) + verify(dataSource).loadOrders() } @Test fun `given empty cache and non-empty network, when init, then final state shows network content`() = runTest { + // GIVEN val network = listOf(order(10)) - - whenever(dataSource.loadOrders(eq(true))).thenReturn( + whenever(dataSource.loadOrders()).thenReturn( flow { - emit(LoadOrdersResult.Success(emptyList())) // cache - emit(LoadOrdersResult.Success(network)) // network + emit(LoadOrdersResult.SuccessCache(emptyList())) // cache → VM sets Loading + emit(LoadOrdersResult.SuccessRemote(network)) // remote → Content } ) + // WHEN viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() + // THEN val state = viewModel.state.value assertThat(state).isInstanceOf(WooPosOrdersState.Content::class.java) val content = state as WooPosOrdersState.Content assertThat(content.items.map { it.id }).containsExactly(10L) + assertThat(content.pullToRefreshState).isEqualTo(WooPosPullToRefreshState.Enabled) } @Test fun `given empty cache and empty network, when init, then final state is Empty`() = runTest { - whenever(dataSource.loadOrders(eq(true))).thenReturn( + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( flow { - emit(LoadOrdersResult.Success(emptyList())) // cache - emit(LoadOrdersResult.Success(emptyList())) // network + emit(LoadOrdersResult.SuccessCache(emptyList())) // cache → Loading + emit(LoadOrdersResult.SuccessRemote(emptyList())) // remote → Empty } ) + // WHEN viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() + // THEN val state = viewModel.state.value assertThat(state).isInstanceOf(WooPosOrdersState.Empty::class.java) } @Test fun `given data source error, when init, then state is Error`() = runTest { - whenever(dataSource.loadOrders(eq(true))).thenReturn( + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( flow { emit(LoadOrdersResult.Error("boom")) } ) + + // WHEN viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() + // THEN val state = viewModel.state.value assertThat(state).isInstanceOf(WooPosOrdersState.Error::class.java) val error = state as WooPosOrdersState.Error @@ -113,39 +124,49 @@ class WooPosOrdersViewModelTest { } @Test - fun `given initial content, when refresh, then skip cache and update with network only`() = runTest { - whenever(dataSource.loadOrders(eq(true))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(1)))) } + fun `given initial content, when refresh, then clear cache and update with network result`() = runTest { + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( + flow { emit(LoadOrdersResult.SuccessRemote(listOf(order(1)))) } ) viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() val before = viewModel.state.value as WooPosOrdersState.Content assertThat(before.items.map { it.id }).containsExactly(1L) - whenever(dataSource.loadOrders(eq(false))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(5), order(6)))) } + whenever(dataSource.loadOrders()).thenReturn( + flow { + emit(LoadOrdersResult.SuccessCache(emptyList())) + emit(LoadOrdersResult.SuccessRemote(listOf(order(5), order(6)))) + } ) + // WHEN viewModel.refresh() advanceUntilIdle() + // THEN val after = viewModel.state.value as WooPosOrdersState.Content assertThat(after.items.map { it.id }).containsExactly(5L, 6L) assertThat(after.pullToRefreshState).isEqualTo(WooPosPullToRefreshState.Enabled) - verify(dataSource).loadOrders(eq(false)) + verify(dataSource).clearCache() + verify(dataSource, times(2)).loadOrders() // init + refresh } @Test fun `given orders loaded, when selecting an order, then selected id and flags update`() = runTest { - whenever(dataSource.loadOrders(eq(true))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(1), order(2), order(3)))) } + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( + flow { emit(LoadOrdersResult.SuccessRemote(listOf(order(1), order(2), order(3)))) } ) + + // WHEN viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() - viewModel.onOrderSelected(3L) advanceUntilIdle() + // THEN val state = viewModel.state.value as WooPosOrdersState.Content assertThat(state.selectedOrderId).isEqualTo(3L) val selectedFlags = state.items.associate { it.id to it.isSelected } @@ -156,22 +177,27 @@ class WooPosOrdersViewModelTest { @Test fun `given selection removed after reload, when refreshing, then first item is auto selected`() = runTest { - whenever(dataSource.loadOrders(eq(true))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(100), order(200)))) } + // GIVEN + whenever(dataSource.loadOrders()).thenReturn( + flow { emit(LoadOrdersResult.SuccessRemote(listOf(order(100), order(200)))) } ) viewModel = WooPosOrdersViewModel(dataSource) advanceUntilIdle() - viewModel.onOrderSelected(200L) advanceUntilIdle() - whenever(dataSource.loadOrders(eq(false))).thenReturn( - flow { emit(LoadOrdersResult.Success(listOf(order(300), order(400)))) } + whenever(dataSource.loadOrders()).thenReturn( + flow { + emit(LoadOrdersResult.SuccessCache(emptyList())) + emit(LoadOrdersResult.SuccessRemote(listOf(order(300), order(400)))) + } ) + // WHEN viewModel.refresh() advanceUntilIdle() + // THEN val state = viewModel.state.value as WooPosOrdersState.Content assertThat(state.items.map { it.id }).containsExactly(300L, 400L) assertThat(state.selectedOrderId).isEqualTo(300L) From ba95772055639c728814b1519e284ec40a826e58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 16:56:43 +0200 Subject: [PATCH 06/20] Fix Detekt --- .../android/ui/woopos/orders/WooPosOrdersViewModel.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index 0a89a27ef97b..c239e9173f7e 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -60,7 +60,7 @@ class WooPosOrdersViewModel @Inject constructor( is LoadOrdersResult.SuccessCache, is LoadOrdersResult.SuccessRemote -> { val newOrders = when (result) { - is LoadOrdersResult.SuccessCache -> result.orders + is LoadOrdersResult.SuccessCache -> result.orders is LoadOrdersResult.SuccessRemote -> result.orders else -> emptyList() } @@ -94,9 +94,9 @@ class WooPosOrdersViewModel @Inject constructor( } private fun toStateForEmpty(result: LoadOrdersResult): WooPosOrdersState = when (result) { - is LoadOrdersResult.SuccessCache -> WooPosOrdersState.Loading + is LoadOrdersResult.SuccessCache -> WooPosOrdersState.Loading is LoadOrdersResult.SuccessRemote -> WooPosOrdersState.Empty - is LoadOrdersResult.Error -> WooPosOrdersState.Error(result.message) + is LoadOrdersResult.Error -> WooPosOrdersState.Error(result.message) } } From 9713b6d489d698b8380bfcc25627bf1eb07bf1eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 16:58:30 +0200 Subject: [PATCH 07/20] add missing import --- .../android/ui/woopos/orders/WooPosOrdersViewModelTest.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt index 278d51fbb317..0b0b8290a72e 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt @@ -13,6 +13,7 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Rule import org.junit.Test +import org.mockito.Mockito.mock import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever From b70396cb37f1978f30c0c05c1ab81ca6561c0903 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 17:27:28 +0200 Subject: [PATCH 08/20] Refactor to better readability --- .../ui/woopos/orders/WooPosOrdersState.kt | 3 +- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 95 ++++++++++--------- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index aeecd122a18e..6c440a3cd188 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -21,8 +21,7 @@ sealed class WooPosOrdersState { val items: List, val pullToRefreshState: WooPosPullToRefreshState, val paginationState: WooPosPaginationState, - val selectedOrderId: Long?, - val listError: String? = null, + val selectedOrderId: Long? ) : WooPosOrdersState() @Immutable diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index c239e9173f7e..afe885370a22 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -24,14 +24,11 @@ class WooPosOrdersViewModel @Inject constructor( private var orders: List = emptyList() private var selectedId: Long? = null - init { - loadOrders() - } + init { loadOrders() } fun onOrderSelected(orderId: Long) { selectedId = orderId - val s = _state.value - if (s is WooPosOrdersState.Content) { + (state.value as? WooPosOrdersState.Content)?.let { s -> _state.value = s.copy( items = orders.toOrderItems(selectedId), selectedOrderId = selectedId @@ -40,8 +37,7 @@ class WooPosOrdersViewModel @Inject constructor( } fun refresh() { - val s = _state.value - _state.value = when (s) { + _state.value = when (val s = _state.value) { is WooPosOrdersState.Content -> s.copy(pullToRefreshState = WooPosPullToRefreshState.Refreshing) else -> WooPosOrdersState.Loading } @@ -53,50 +49,61 @@ class WooPosOrdersViewModel @Inject constructor( viewModelScope.launch { ordersDataSource.loadOrders().collect { result -> when (result) { - is LoadOrdersResult.Error -> { - _state.value = WooPosOrdersState.Error(message = result.message) - } - - is LoadOrdersResult.SuccessCache, - is LoadOrdersResult.SuccessRemote -> { - val newOrders = when (result) { - is LoadOrdersResult.SuccessCache -> result.orders - is LoadOrdersResult.SuccessRemote -> result.orders - else -> emptyList() - } - - orders = newOrders - selectedId = chooseSelectedId(selectedId, newOrders) - - _state.value = if (newOrders.isEmpty()) { - toStateForEmpty(result) - } else { - WooPosOrdersState.Content( - items = newOrders.toOrderItems(selectedId), - selectedOrderId = selectedId, - pullToRefreshState = WooPosPullToRefreshState.Enabled, - paginationState = WooPosPaginationState.None, - listError = null - ) - } - } + is LoadOrdersResult.Error -> + handleError(result.message) + + is LoadOrdersResult.SuccessCache -> + handleSuccess(result.orders, isCache = true) + + is LoadOrdersResult.SuccessRemote -> + handleSuccess(result.orders, isCache = false) } } } } - private fun chooseSelectedId( - current: Long?, - newOrders: List - ): Long? { - return current?.takeIf { id -> newOrders.any { it.id == id } } - ?: newOrders.firstOrNull()?.id + private fun handleError(message: String) { + _state.value = WooPosOrdersState.Error(message) } - private fun toStateForEmpty(result: LoadOrdersResult): WooPosOrdersState = when (result) { - is LoadOrdersResult.SuccessCache -> WooPosOrdersState.Loading - is LoadOrdersResult.SuccessRemote -> WooPosOrdersState.Empty - is LoadOrdersResult.Error -> WooPosOrdersState.Error(result.message) + private fun handleSuccess(newOrders: List, isCache: Boolean) { + applySelection(newOrders) + + if (newOrders.isEmpty()) { + handleEmpty(isCache) + } else { + setContent(newOrders) + } + } + + private fun handleEmpty(isCache: Boolean) { + if (isCache && isRefreshing() && _state.value is WooPosOrdersState.Content) { + return + } + _state.value = if (isCache) WooPosOrdersState.Loading else WooPosOrdersState.Empty + } + + private fun setContent(newOrders: List) { + _state.value = WooPosOrdersState.Content( + items = newOrders.toOrderItems(selectedId), + selectedOrderId = selectedId, + pullToRefreshState = WooPosPullToRefreshState.Enabled, + paginationState = WooPosPaginationState.None + ) + } + + private fun applySelection(newOrders: List) { + orders = newOrders + selectedId = chooseSelectedId(selectedId, newOrders) + } + + private fun isRefreshing(): Boolean = + (state.value as? WooPosOrdersState.Content) + ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing + + private fun chooseSelectedId(current: Long?, newOrders: List): Long? { + return current?.takeIf { id -> newOrders.any { it.id == id } } + ?: newOrders.firstOrNull()?.id } } From 020078af434e81c778205080eedc190e2f837516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 17:37:03 +0200 Subject: [PATCH 09/20] Simplify state --- .../android/ui/woopos/orders/WooPosOrdersState.kt | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index 6c440a3cd188..c680ef7709b9 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -27,23 +27,17 @@ sealed class WooPosOrdersState { @Immutable data class Error( val message: String, - val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled, + val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled, ) : WooPosOrdersState() { - val items: List = emptyList() - val paginationState: WooPosPaginationState = WooPosPaginationState.None } @Immutable data object Loading : WooPosOrdersState() { - val items: List = emptyList() val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled - val paginationState: WooPosPaginationState = WooPosPaginationState.None } @Immutable data object Empty : WooPosOrdersState() { - val items: List = emptyList() val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled - val paginationState: WooPosPaginationState = WooPosPaginationState.None } } From 48dc8e5efccbdbc346bf40da65e1b1e04994430c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Fri, 5 Sep 2025 17:46:16 +0200 Subject: [PATCH 10/20] Fix detekt --- .../woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index c680ef7709b9..13500e5e09ad 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -28,8 +28,7 @@ sealed class WooPosOrdersState { data class Error( val message: String, val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled, - ) : WooPosOrdersState() { - } + ) : WooPosOrdersState() @Immutable data object Loading : WooPosOrdersState() { From e4d24ab8347ce91ce1a18e9bb3643522620464db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 10:04:15 +0200 Subject: [PATCH 11/20] Refactor for better readability --- .../ui/woopos/orders/WooPosOrdersScreen.kt | 201 ++++++++++-------- 1 file changed, 112 insertions(+), 89 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 0289c5d32259..5262025656eb 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -53,116 +53,139 @@ fun WooPosOrdersScreen( BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) } Row(modifier = Modifier.fillMaxSize()) { - // Left pane - Column( + WooPosOrdersLeftPane( + state = state, + onBackClicked = onBackClicked, + onRefresh = viewModel::refresh, + onOrderSelected = viewModel::onOrderSelected, modifier = Modifier .weight(0.3f) .fillMaxHeight() .background(MaterialTheme.colorScheme.surface) - ) { - WooPosToolbar( - titleText = stringResource(R.string.woopos_orders_title), - onBackClicked = onBackClicked, - ) + ) - val refreshing = - when (val s = state) { - is WooPosOrdersState.Content -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing - is WooPosOrdersState.Error -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing - is WooPosOrdersState.Empty -> false - is WooPosOrdersState.Loading -> false - } + WooPosOrdersRightPane( + state = state, + modifier = Modifier + .weight(0.7f) + .fillMaxHeight() + .background(MaterialTheme.colorScheme.surfaceContainerLow) + ) + } +} - val pullRefreshState = rememberPullRefreshState( - refreshing = refreshing, - onRefresh = { viewModel.refresh() } - ) +@OptIn(ExperimentalMaterialApi::class) +@Composable +private fun WooPosOrdersLeftPane( + state: WooPosOrdersState, + onBackClicked: () -> Unit, + onRefresh: () -> Unit, + onOrderSelected: (Long) -> Unit, + modifier: Modifier = Modifier +) { + Column(modifier = modifier) { + WooPosToolbar( + titleText = stringResource(R.string.woopos_orders_title), + onBackClicked = onBackClicked, + ) - Box( - modifier = Modifier - .fillMaxSize() - .pullRefresh(pullRefreshState) - ) { - when (val s = state) { - is WooPosOrdersState.Loading -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = stringResource(R.string.loading), - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(WooPosSpacing.Large.value) - ) - } - } + val refreshing = when (state) { + is WooPosOrdersState.Content -> state.pullToRefreshState == WooPosPullToRefreshState.Refreshing + is WooPosOrdersState.Error -> state.pullToRefreshState == WooPosPullToRefreshState.Refreshing + is WooPosOrdersState.Empty -> false + is WooPosOrdersState.Loading -> false + } - is WooPosOrdersState.Error -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = s.message, - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.error, - modifier = Modifier.padding(WooPosSpacing.Large.value) - ) - } + val pullRefreshState = rememberPullRefreshState( + refreshing = refreshing, + onRefresh = onRefresh + ) + + Box( + modifier = Modifier + .fillMaxSize() + .pullRefresh(pullRefreshState) + ) { + when (state) { + is WooPosOrdersState.Loading -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = stringResource(R.string.loading), + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(WooPosSpacing.Large.value) + ) } + } - is WooPosOrdersState.Empty -> { - Column( - modifier = Modifier.fillMaxSize(), - horizontalAlignment = Alignment.CenterHorizontally - ) { - WooPosText( - text = "No orders found", - style = WooPosTypography.BodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(WooPosSpacing.Large.value) - ) - } + is WooPosOrdersState.Error -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = state.message, + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(WooPosSpacing.Large.value) + ) } + } - is WooPosOrdersState.Content -> { - WooPosOrdersListPaneScreen( - items = s.items, - selectedOrderId = s.selectedOrderId, - onOrderSelected = viewModel::onOrderSelected, - modifier = Modifier.fillMaxSize() + is WooPosOrdersState.Empty -> { + Column( + modifier = Modifier.fillMaxSize(), + horizontalAlignment = Alignment.CenterHorizontally + ) { + WooPosText( + text = "No orders found", + style = WooPosTypography.BodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(WooPosSpacing.Large.value) ) } } - // PTR indicator at the top of the list area - PullRefreshIndicator( - refreshing = refreshing, - state = pullRefreshState, - modifier = Modifier - .align(Alignment.TopCenter) - .padding(top = WooPosSpacing.XSmall.value), - backgroundColor = MaterialTheme.colorScheme.surface, - contentColor = MaterialTheme.colorScheme.primary - ) + is WooPosOrdersState.Content -> { + WooPosOrdersListPaneScreen( + items = state.items, + selectedOrderId = state.selectedOrderId, + onOrderSelected = onOrderSelected, + modifier = Modifier.fillMaxSize() + ) + } } - } - // Right pane - val selectedItem: OrderItemViewState? = when (val s = state) { - is WooPosOrdersState.Content -> s.items.firstOrNull { it.id == s.selectedOrderId } - else -> null + PullRefreshIndicator( + refreshing = refreshing, + state = pullRefreshState, + modifier = Modifier + .align(Alignment.TopCenter) + .padding(top = WooPosSpacing.XSmall.value), + backgroundColor = MaterialTheme.colorScheme.surface, + contentColor = MaterialTheme.colorScheme.primary + ) } + } +} - WooPosOrdersDetailPaneScreen( - selected = selectedItem, - modifier = Modifier - .weight(0.7f) - .fillMaxHeight() - .background(MaterialTheme.colorScheme.surfaceContainerLow) - ) +@Composable +private fun WooPosOrdersRightPane( + state: WooPosOrdersState, + modifier: Modifier = Modifier +) { + val selectedItem: OrderItemViewState? = when (state) { + is WooPosOrdersState.Content -> state.items.firstOrNull { it.id == state.selectedOrderId } + else -> null } + + WooPosOrdersDetailPaneScreen( + selected = selectedItem, + modifier = modifier.fillMaxSize() + ) } @Composable From 32927808d5320b9bd8162804196d4ce83a59a0c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 10:07:58 +0200 Subject: [PATCH 12/20] Remove elvis operator applied to a non-nullable type --- .../android/ui/woopos/orders/WooPosOrdersDataSource.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt index e043a7a23f0a..c757f1d7b0a4 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt @@ -52,5 +52,5 @@ class WooPosOrdersDataSource @Inject constructor( private suspend fun List.toAppModels(): List = map { orderMapper.toAppModel(it) - } ?: emptyList() + } } From 43400830f82f9cb240ef33530fd419b28144c022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 10:11:40 +0200 Subject: [PATCH 13/20] Revert to a sealed class --- .../android/ui/woopos/orders/WooPosOrdersDataSource.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt index c757f1d7b0a4..e517f7ac2bf4 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt @@ -10,10 +10,10 @@ import org.wordpress.android.fluxc.network.rest.wpcom.wc.order.OrderRestClient.O import org.wordpress.android.fluxc.persistence.entity.OrderEntity import javax.inject.Inject -sealed interface LoadOrdersResult { - data class SuccessCache(val orders: List) : LoadOrdersResult - data class SuccessRemote(val orders: List) : LoadOrdersResult - data class Error(val message: String) : LoadOrdersResult +sealed class LoadOrdersResult { + data class SuccessCache(val orders: List) : LoadOrdersResult() + data class SuccessRemote(val orders: List) : LoadOrdersResult() + data class Error(val message: String) : LoadOrdersResult() } class WooPosOrdersDataSource @Inject constructor( From 6c9aa20c20211e8273cb0f35f22c47d6cbe42f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 10:42:58 +0200 Subject: [PATCH 14/20] Use logic from view model --- .../android/ui/woopos/orders/WooPosOrdersScreen.kt | 14 ++++---------- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 8 ++++---- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 5262025656eb..45c32e77095c 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -38,7 +38,6 @@ import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosToolba import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosSpacing import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTheme import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTypography -import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent @OptIn(ExperimentalMaterialApi::class) @@ -57,6 +56,7 @@ fun WooPosOrdersScreen( state = state, onBackClicked = onBackClicked, onRefresh = viewModel::refresh, + isRefreshing = viewModel.isRefreshing(), onOrderSelected = viewModel::onOrderSelected, modifier = Modifier .weight(0.3f) @@ -80,6 +80,7 @@ private fun WooPosOrdersLeftPane( state: WooPosOrdersState, onBackClicked: () -> Unit, onRefresh: () -> Unit, + isRefreshing: Boolean, onOrderSelected: (Long) -> Unit, modifier: Modifier = Modifier ) { @@ -89,15 +90,8 @@ private fun WooPosOrdersLeftPane( onBackClicked = onBackClicked, ) - val refreshing = when (state) { - is WooPosOrdersState.Content -> state.pullToRefreshState == WooPosPullToRefreshState.Refreshing - is WooPosOrdersState.Error -> state.pullToRefreshState == WooPosPullToRefreshState.Refreshing - is WooPosOrdersState.Empty -> false - is WooPosOrdersState.Loading -> false - } - val pullRefreshState = rememberPullRefreshState( - refreshing = refreshing, + refreshing = isRefreshing, onRefresh = onRefresh ) @@ -160,7 +154,7 @@ private fun WooPosOrdersLeftPane( } PullRefreshIndicator( - refreshing = refreshing, + refreshing = isRefreshing, state = pullRefreshState, modifier = Modifier .align(Alignment.TopCenter) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index afe885370a22..89c87bbecea9 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -45,6 +45,10 @@ class WooPosOrdersViewModel @Inject constructor( loadOrders() } + fun isRefreshing(): Boolean = + (state.value as? WooPosOrdersState.Content) + ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing + private fun loadOrders() { viewModelScope.launch { ordersDataSource.loadOrders().collect { result -> @@ -97,10 +101,6 @@ class WooPosOrdersViewModel @Inject constructor( selectedId = chooseSelectedId(selectedId, newOrders) } - private fun isRefreshing(): Boolean = - (state.value as? WooPosOrdersState.Content) - ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing - private fun chooseSelectedId(current: Long?, newOrders: List): Long? { return current?.takeIf { id -> newOrders.any { it.id == id } } ?: newOrders.firstOrNull()?.id From a5378e88e50e3607234486dc34d07d9a89e5dbd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 11:28:42 +0200 Subject: [PATCH 15/20] Add information about the pull refresh enabled state --- .../android/ui/woopos/orders/WooPosOrdersScreen.kt | 6 +++++- .../android/ui/woopos/orders/WooPosOrdersState.kt | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 45c32e77095c..86663437249c 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -38,6 +38,7 @@ import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosToolba import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosSpacing import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTheme import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTypography +import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent @OptIn(ExperimentalMaterialApi::class) @@ -98,7 +99,10 @@ private fun WooPosOrdersLeftPane( Box( modifier = Modifier .fillMaxSize() - .pullRefresh(pullRefreshState) + .pullRefresh( + pullRefreshState, + enabled = state.pullToRefreshState != WooPosPullToRefreshState.Disabled + ) ) { when (state) { is WooPosOrdersState.Loading -> { diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index 13500e5e09ad..0f1f17c0e2e7 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -15,11 +15,12 @@ data class OrderItemViewState( @Immutable sealed class WooPosOrdersState { + abstract val pullToRefreshState: WooPosPullToRefreshState @Immutable data class Content( val items: List, - val pullToRefreshState: WooPosPullToRefreshState, + override val pullToRefreshState: WooPosPullToRefreshState, val paginationState: WooPosPaginationState, val selectedOrderId: Long? ) : WooPosOrdersState() @@ -27,16 +28,16 @@ sealed class WooPosOrdersState { @Immutable data class Error( val message: String, - val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled, + override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled, ) : WooPosOrdersState() @Immutable data object Loading : WooPosOrdersState() { - val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled + override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled } @Immutable data object Empty : WooPosOrdersState() { - val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled + override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled } } From 73bec2d154eac7213f66d861944d44fd2dac8650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 13:15:07 +0200 Subject: [PATCH 16/20] Simplify view model --- .../woopos/orders/WooPosOrdersDataSource.kt | 4 +- .../ui/woopos/orders/WooPosOrdersState.kt | 7 +- .../ui/woopos/orders/WooPosOrdersViewModel.kt | 125 ++++++++---------- 3 files changed, 64 insertions(+), 72 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt index e517f7ac2bf4..3011c96f2cac 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt @@ -27,7 +27,9 @@ class WooPosOrdersDataSource @Inject constructor( } fun loadOrders(): Flow = flow { val cached = ordersCache.getAll() - emit(LoadOrdersResult.SuccessCache(cached)) + if (cached.isNotEmpty()) { + emit(LoadOrdersResult.SuccessCache(cached)) + } val result = restClient.fetchOrders( site = selectedSite.get(), diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index 0f1f17c0e2e7..3a2b4a21425a 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -37,7 +37,8 @@ sealed class WooPosOrdersState { } @Immutable - data object Empty : WooPosOrdersState() { - override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled - } + data class Empty( + override val pullToRefreshState: WooPosPullToRefreshState = + WooPosPullToRefreshState.Enabled + ) : WooPosOrdersState() } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index 89c87bbecea9..ffea3df1e685 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -21,99 +21,88 @@ class WooPosOrdersViewModel @Inject constructor( private val _state = MutableStateFlow(WooPosOrdersState.Loading) val state: StateFlow = _state.asStateFlow() - private var orders: List = emptyList() - private var selectedId: Long? = null - - init { loadOrders() } + init { + loadOrders() + } fun onOrderSelected(orderId: Long) { - selectedId = orderId - (state.value as? WooPosOrdersState.Content)?.let { s -> - _state.value = s.copy( - items = orders.toOrderItems(selectedId), - selectedOrderId = selectedId + val currentState = _state.value + if (currentState is WooPosOrdersState.Content) { + _state.value = currentState.copy( + items = currentState.items.map { it.copy(isSelected = it.id == orderId) }, + selectedOrderId = orderId ) } } + fun isRefreshing(): Boolean = + (state.value as? WooPosOrdersState.Content) + ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing + fun refresh() { - _state.value = when (val s = _state.value) { - is WooPosOrdersState.Content -> s.copy(pullToRefreshState = WooPosPullToRefreshState.Refreshing) - else -> WooPosOrdersState.Loading + val currentState = _state.value + _state.value = when (currentState) { + is WooPosOrdersState.Content -> currentState.copy( + pullToRefreshState = WooPosPullToRefreshState.Refreshing + ) + + is WooPosOrdersState.Empty -> currentState.copy( + pullToRefreshState = WooPosPullToRefreshState.Refreshing + ) + + is WooPosOrdersState.Error -> currentState + is WooPosOrdersState.Loading -> currentState } + ordersDataSource.clearCache() loadOrders() } - fun isRefreshing(): Boolean = - (state.value as? WooPosOrdersState.Content) - ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing - private fun loadOrders() { viewModelScope.launch { ordersDataSource.loadOrders().collect { result -> when (result) { - is LoadOrdersResult.Error -> - handleError(result.message) - - is LoadOrdersResult.SuccessCache -> - handleSuccess(result.orders, isCache = true) - - is LoadOrdersResult.SuccessRemote -> - handleSuccess(result.orders, isCache = false) + is LoadOrdersResult.Error -> { + _state.value = WooPosOrdersState.Error( + message = result.message, + pullToRefreshState = WooPosPullToRefreshState.Disabled + ) + } + + is LoadOrdersResult.SuccessCache -> { + updateContentState(result.orders) + } + + is LoadOrdersResult.SuccessRemote -> { + if (result.orders.isEmpty()) { + _state.value = WooPosOrdersState.Empty() + } else { + updateContentState(result.orders) + } + } } } } } - private fun handleError(message: String) { - _state.value = WooPosOrdersState.Error(message) - } - - private fun handleSuccess(newOrders: List, isCache: Boolean) { - applySelection(newOrders) - - if (newOrders.isEmpty()) { - handleEmpty(isCache) - } else { - setContent(newOrders) - } - } - - private fun handleEmpty(isCache: Boolean) { - if (isCache && isRefreshing() && _state.value is WooPosOrdersState.Content) { - return - } - _state.value = if (isCache) WooPosOrdersState.Loading else WooPosOrdersState.Empty - } + private fun updateContentState(orders: List) { + val currentSelectedId = (_state.value as? WooPosOrdersState.Content)?.selectedOrderId + val newSelectedId = currentSelectedId?.takeIf { id -> orders.any { it.id == id } } + ?: orders.firstOrNull()?.id - private fun setContent(newOrders: List) { _state.value = WooPosOrdersState.Content( - items = newOrders.toOrderItems(selectedId), - selectedOrderId = selectedId, + items = orders.map { order -> + OrderItemViewState( + id = order.id, + title = "Order #${order.number}", + date = order.dateCreated.formatToDDMMMYYYY(), + total = "${order.total} ${order.currency}", + isSelected = order.id == newSelectedId + ) + }, + selectedOrderId = newSelectedId, pullToRefreshState = WooPosPullToRefreshState.Enabled, paginationState = WooPosPaginationState.None ) } - - private fun applySelection(newOrders: List) { - orders = newOrders - selectedId = chooseSelectedId(selectedId, newOrders) - } - - private fun chooseSelectedId(current: Long?, newOrders: List): Long? { - return current?.takeIf { id -> newOrders.any { it.id == id } } - ?: newOrders.firstOrNull()?.id - } } - -private fun List.toOrderItems(selectedId: Long?): List = - map { o -> - OrderItemViewState( - id = o.id, - title = "Order #${o.number}", - date = o.dateCreated.formatToDDMMMYYYY(), - total = "${o.total} ${o.currency}", - isSelected = o.id == selectedId - ) - } From 2894ae4b608779a9f4e9e65819e15ce094a9ba4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Tue, 9 Sep 2025 13:21:33 +0200 Subject: [PATCH 17/20] Fix test --- .../ui/woopos/orders/WooPosOrdersDataSourceTest.kt | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt index 4f97d2d9124b..f8cc11a1a32b 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSourceTest.kt @@ -149,7 +149,7 @@ class WooPosOrdersDataSourceTest { } @Test - fun `when cache empty and fetch returns empty, then emit SuccessCache empty then SuccessRemote empty and store empty`() = runTest { + fun `when cache empty and fetch returns empty, then SuccessRemote empty and store empty`() = runTest { whenever(ordersCache.getAll()).thenReturn(emptyList()) val payload = WCOrderStore.FetchOrdersResponsePayload( @@ -170,14 +170,11 @@ class WooPosOrdersDataSourceTest { val emissions = sut.loadOrders().toList(mutableListOf()) - assertThat(emissions).hasSize(2) + assertThat(emissions).hasSize(1) - val first = emissions[0] as LoadOrdersResult.SuccessCache + val first = emissions[0] as LoadOrdersResult.SuccessRemote assertThat(first.orders).isEmpty() - val second = emissions[1] as LoadOrdersResult.SuccessRemote - assertThat(second.orders).isEmpty() - verify(selectedSite).get() verify(ordersCache).getAll() verify(ordersCache).setAll(emptyList()) From 7cf9f9380dffd2b9e5e531eb349b3b0c710da29b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Wed, 10 Sep 2025 11:51:34 +0200 Subject: [PATCH 18/20] Keep one source of truth for refreshing --- .../android/ui/woopos/orders/WooPosOrdersScreen.kt | 2 +- .../android/ui/woopos/orders/WooPosOrdersState.kt | 5 +++-- .../android/ui/woopos/orders/WooPosOrdersViewModel.kt | 9 +-------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 86663437249c..5a04bd774b09 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -57,7 +57,7 @@ fun WooPosOrdersScreen( state = state, onBackClicked = onBackClicked, onRefresh = viewModel::refresh, - isRefreshing = viewModel.isRefreshing(), + isRefreshing = state.pullToRefreshState == WooPosPullToRefreshState.Refreshing, onOrderSelected = viewModel::onOrderSelected, modifier = Modifier .weight(0.3f) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt index 3a2b4a21425a..9187893c00dd 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt @@ -28,8 +28,9 @@ sealed class WooPosOrdersState { @Immutable data class Error( val message: String, - override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled, - ) : WooPosOrdersState() + ) : WooPosOrdersState() { + override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled + } @Immutable data object Loading : WooPosOrdersState() { diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index ffea3df1e685..03a08b58617b 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -35,10 +35,6 @@ class WooPosOrdersViewModel @Inject constructor( } } - fun isRefreshing(): Boolean = - (state.value as? WooPosOrdersState.Content) - ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing - fun refresh() { val currentState = _state.value _state.value = when (currentState) { @@ -63,10 +59,7 @@ class WooPosOrdersViewModel @Inject constructor( ordersDataSource.loadOrders().collect { result -> when (result) { is LoadOrdersResult.Error -> { - _state.value = WooPosOrdersState.Error( - message = result.message, - pullToRefreshState = WooPosPullToRefreshState.Disabled - ) + _state.value = WooPosOrdersState.Error(message = result.message) } is LoadOrdersResult.SuccessCache -> { From ac705b3e7f718a538aeaa02e6a88e07f62b6ed97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Wed, 10 Sep 2025 11:53:37 +0200 Subject: [PATCH 19/20] Rename to follow convention, --- .../android/ui/woopos/orders/WooPosOrdersScreen.kt | 2 +- .../android/ui/woopos/orders/WooPosOrdersViewModel.kt | 2 +- .../android/ui/woopos/orders/WooPosOrdersViewModelTest.kt | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index 5a04bd774b09..d35636f182c0 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -56,7 +56,7 @@ fun WooPosOrdersScreen( WooPosOrdersLeftPane( state = state, onBackClicked = onBackClicked, - onRefresh = viewModel::refresh, + onRefresh = viewModel::onRefresh, isRefreshing = state.pullToRefreshState == WooPosPullToRefreshState.Refreshing, onOrderSelected = viewModel::onOrderSelected, modifier = Modifier diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt index 03a08b58617b..c616922c995c 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt @@ -35,7 +35,7 @@ class WooPosOrdersViewModel @Inject constructor( } } - fun refresh() { + fun onRefresh() { val currentState = _state.value _state.value = when (currentState) { is WooPosOrdersState.Content -> currentState.copy( diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt index 0b0b8290a72e..295e69086985 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt @@ -143,7 +143,7 @@ class WooPosOrdersViewModelTest { ) // WHEN - viewModel.refresh() + viewModel.onRefresh() advanceUntilIdle() // THEN @@ -195,7 +195,7 @@ class WooPosOrdersViewModelTest { ) // WHEN - viewModel.refresh() + viewModel.onRefresh() advanceUntilIdle() // THEN From 6f414cc5040a2947f55adaebf3b1c7e6627079f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ce=CC=81sar=20Vargas=20Casaseca?= Date: Wed, 10 Sep 2025 11:56:46 +0200 Subject: [PATCH 20/20] Better naming --- .../android/ui/woopos/orders/WooPosOrdersScreen.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt index d35636f182c0..c023859bb830 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt @@ -53,7 +53,7 @@ fun WooPosOrdersScreen( BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) } Row(modifier = Modifier.fillMaxSize()) { - WooPosOrdersLeftPane( + OrdersList( state = state, onBackClicked = onBackClicked, onRefresh = viewModel::onRefresh, @@ -65,7 +65,7 @@ fun WooPosOrdersScreen( .background(MaterialTheme.colorScheme.surface) ) - WooPosOrdersRightPane( + OrderDetails( state = state, modifier = Modifier .weight(0.7f) @@ -77,7 +77,7 @@ fun WooPosOrdersScreen( @OptIn(ExperimentalMaterialApi::class) @Composable -private fun WooPosOrdersLeftPane( +private fun OrdersList( state: WooPosOrdersState, onBackClicked: () -> Unit, onRefresh: () -> Unit, @@ -171,7 +171,7 @@ private fun WooPosOrdersLeftPane( } @Composable -private fun WooPosOrdersRightPane( +private fun OrderDetails( state: WooPosOrdersState, modifier: Modifier = Modifier ) {