Skip to content

[POS Historical Orders] Pull to Refresh #14578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Sep 10, 2025
+456 −154
Merged

[POS Historical Orders] Pull to Refresh #14578

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a890474
Enhance orders state and adapt clients
toupper Sep 4, 2025
cc911fc
Implement Pull To Refresh
toupper Sep 4, 2025
73ba760
Add unit tests
toupper Sep 4, 2025
313a64c
Remove non necessary comment
toupper Sep 4, 2025
b0f9a13
Merge branch 'feat/WOOMOB-1145-pos-historical-orders-in-memory-cache'…
toupper Sep 5, 2025
3cabebf
Merge branch 'feat/WOOMOB-1145-pos-historical-orders-in-memory-cache'…
toupper Sep 5, 2025
0df5676
Fix tests
toupper Sep 5, 2025
ba95772
Fix Detekt
toupper Sep 5, 2025
9713b6d
add missing import
toupper Sep 5, 2025
b70396c
Refactor to better readability
toupper Sep 5, 2025
020078a
Simplify state
toupper Sep 5, 2025
48dc8e5
Fix detekt
toupper Sep 5, 2025
e4d24ab
Refactor for better readability
toupper Sep 9, 2025
3292780
Remove elvis operator applied to a non-nullable type
toupper Sep 9, 2025
4340083
Revert to a sealed class
toupper Sep 9, 2025
6c9aa20
Use logic from view model
toupper Sep 9, 2025
a5378e8
Add information about the pull refresh enabled state
toupper Sep 9, 2025
73bec2d
Simplify view model
toupper Sep 9, 2025
2894ae4
Fix test
toupper Sep 9, 2025
7cf9f93
Keep one source of truth for refreshing
toupper Sep 10, 2025
ac705b3
Rename to follow convention,
toupper Sep 10, 2025
6f414cc
Better naming
toupper Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions ...mmerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +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 class LoadOrdersResult {
data class Success(val orders: List<Order>) : LoadOrdersResult()
data class Error(val message: String) : LoadOrdersResult()
sealed interface LoadOrdersResult {
Copy link
Contributor

@kidinov kidinov Sep 8, 2025

Choose a reason for hiding this comment

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

Any reason specifically for interface here?

Copy link
Contributor Author

@toupper toupper Sep 9, 2025

Choose a reason for hiding this comment

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

Not a big reason, changed back to sealed class as that's the convention in our code 4340083

data class SuccessCache(val orders: List<Order>) : LoadOrdersResult
data class SuccessRemote(val orders: List<Order>) : LoadOrdersResult
data class Error(val message: String) : LoadOrdersResult
}

class WooPosOrdersDataSource @Inject constructor(
Expand All @@ -26,7 +27,7 @@ class WooPosOrdersDataSource @Inject constructor(
}
fun loadOrders(): Flow<LoadOrdersResult> = flow {
val cached = ordersCache.getAll()
emit(LoadOrdersResult.Success(cached))
emit(LoadOrdersResult.SuccessCache(cached))

val result = restClient.fetchOrders(
site = selectedSite.get(),
Expand All @@ -43,10 +44,12 @@ class WooPosOrdersDataSource @Inject constructor(
} else {
val mapped = result.orders.toAppModels()
ordersCache.setAll(mapped)
emit(LoadOrdersResult.Success(result.orders.toAppModels()))
emit(LoadOrdersResult.SuccessRemote(result.orders.toAppModels()))
}
}

fun clearCache() = ordersCache.clear()

Copy link
Contributor

@kidinov kidinov Sep 8, 2025

Choose a reason for hiding this comment

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

Not related to the PR: on line 55 there is still ?: applied to a non-nullable type
image

Copy link
Contributor Author

@toupper toupper Sep 9, 2025

Choose a reason for hiding this comment

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

True, removed in 3292780

private suspend fun List<OrderEntity>.toAppModels(): List<Order> = map {
orderMapper.toAppModel(it)
} ?: emptyList()
Expand Down
190 changes: 106 additions & 84 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -27,16 +32,16 @@ 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
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,
Expand All @@ -48,6 +53,7 @@ fun WooPosOrdersScreen(
BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) }

Row(modifier = Modifier.fillMaxSize()) {
// Left pane
Copy link
Contributor

@kidinov kidinov Sep 8, 2025

Choose a reason for hiding this comment

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

NP: Instead of a comment, it is better to have a composable with a nice name that represents the left and right panes.

Copy link
Contributor

@kidinov kidinov Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The same applies to other comments here; it appears to be a workaround for a decent composition of the code. And some maybe don't have value, e.g.

              // PTR indicator at the top of the list area
                PullRefreshIndicator(

I think that's a redundant comment

Copy link
Contributor Author

@toupper toupper Sep 9, 2025

Choose a reason for hiding this comment

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

Thanks for the hints. Done in e4d24ab

Column(
modifier = Modifier
.weight(0.3f)
Expand All @@ -59,59 +65,98 @@ fun WooPosOrdersScreen(
onBackClicked = onBackClicked,
)

when {
state.isLoading -> {
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 (val s = state) {
Copy link
Contributor

@kidinov kidinov Sep 8, 2025

Choose a reason for hiding this comment

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

It’s best to keep the logic separate from the view. The view should simply display the data without any complex calculations, the logic should be in the VM. Also, you seem to ignore disabled state here.

So maybe something like that would be cleaner:

            val pullRefreshState = rememberPullRefreshState(
                refreshing = state.pullToRefreshState == Refreshing,
                onRefresh = { viewModel.refresh() },
            )

            Box(
                modifier = Modifier
                    .fillMaxSize()
                    .pullRefresh(
                        pullRefreshState,
                        enabled = state.pullToRefreshState != WooPosPullToRefreshState.Disabled
                    )
            ) {

@Immutable
sealed class WooPosOrdersState(
    open val pullToRefreshState: WooPosPullToRefreshState,
) {
    @Immutable
    data class Content(
        val items: List<OrderItemViewState>,
        override val pullToRefreshState: WooPosPullToRefreshState,
        val paginationState: WooPosPaginationState,
        val selectedOrderId: Long?
    ) : WooPosOrdersState(pullToRefreshState)

    @Immutable
    data class Error(
        val message: String,
        override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled,
    ) : WooPosOrdersState(pullToRefreshState)

    @Immutable
    data class Loading(
        override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled,
    ) : WooPosOrdersState(pullToRefreshState)

    @Immutable
    data class Empty(
        override val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled,
    ) : WooPosOrdersState(pullToRefreshState)
}

Copy link
Contributor Author

@toupper toupper Sep 9, 2025

Choose a reason for hiding this comment

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

Enabled state in a5378e8

is WooPosOrdersState.Content -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing
is WooPosOrdersState.Error -> s.pullToRefreshState == WooPosPullToRefreshState.Refreshing
is WooPosOrdersState.Empty -> false
is WooPosOrdersState.Loading -> false
}
state.error != null -> {
Column(
modifier = Modifier.fillMaxSize(),
horizontalAlignment = Alignment.CenterHorizontally
) {
WooPosText(
text = state.error ?: stringResource(R.string.error_generic),
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)
)
}
}
}
state.orders.isEmpty() -> {
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 = 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.Content -> {
WooPosOrdersListPaneScreen(
items = s.items,
selectedOrderId = s.selectedOrderId,
onOrderSelected = viewModel::onOrderSelected,
modifier = Modifier.fillMaxSize()
)
}
}
else -> {
WooPosOrdersListPaneScreen(
orders = state.orders,
selectedOrderId = state.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
)
}
}

// 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()
Expand All @@ -122,7 +167,7 @@ fun WooPosOrdersScreen(

@Composable
fun WooPosOrdersListPaneScreen(
orders: List<Order>,
items: List<OrderItemViewState>,
selectedOrderId: Long?,
onOrderSelected: (Long) -> Unit,
modifier: Modifier = Modifier
Expand All @@ -131,14 +176,13 @@ 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 {
MaterialTheme.colorScheme.surface
}

val foreground = if (isSelected) {
MaterialTheme.colorScheme.onPrimaryContainer
} else {
Expand All @@ -150,32 +194,21 @@ 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,
vertical = WooPosSpacing.Medium.value
),
verticalAlignment = Alignment.Top
) {
Column(
verticalArrangement = Arrangement.spacedBy(WooPosSpacing.XSmall.value)
) {
WooPosText(
"Order #${order.number}",
style = WooPosTypography.BodyMedium
)
WooPosText(
text = order.dateCreated.formatToDDMMMYYYY(),
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 = "${order.total} ${order.currency}",
text = item.total,
style = WooPosTypography.BodyMedium,
modifier = Modifier.alignByBaseline()
)
Expand All @@ -186,29 +219,22 @@ fun WooPosOrdersListPaneScreen(

@Composable
fun WooPosOrdersDetailPaneScreen(
order: Order?,
selected: OrderItemViewState?,
modifier: Modifier = Modifier
) {
Column(
modifier = modifier.fillMaxSize()
) {
Column(modifier = modifier.fillMaxSize()) {
WooPosToolbar(
modifier = Modifier
.fillMaxWidth(),
titleText = "Order #${order?.number ?: "--"}",
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
)
Expand All @@ -219,9 +245,5 @@ fun WooPosOrdersDetailPaneScreen(
@WooPosPreview
@Composable
fun WooPosOrdersScreenPreview() {
WooPosTheme {
WooPosOrdersScreen(
onNavigationEvent = {}
)
}
WooPosTheme { WooPosOrdersScreen(onNavigationEvent = {}) }
}
49 changes: 39 additions & 10 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,42 @@
package com.woocommerce.android.ui.woopos.orders

import com.woocommerce.android.model.Order

data class WooPosOrdersState(
val orders: List<Order> = 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<OrderItemViewState>,
val pullToRefreshState: WooPosPullToRefreshState,
val paginationState: WooPosPaginationState,
val selectedOrderId: Long?
) : WooPosOrdersState()

@Immutable
data class Error(
val message: String,
val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled,
) : WooPosOrdersState()

@Immutable
data object Loading : WooPosOrdersState() {
val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Disabled
}

@Immutable
data object Empty : WooPosOrdersState() {
val pullToRefreshState: WooPosPullToRefreshState = WooPosPullToRefreshState.Enabled
}
}
Loading