[POS Historical Orders] Pull to Refresh #14578

toupper · 2025-09-05T09:18:04Z

Description

With this PR we implement Pull to Refresh on POS Orders.

Steps to reproduce

Open the app
Go to POS
Go to Orders
Pull to Refresh
Verify that we request orders remotely and that we update the list

Testing information

Tested on Samsung Tab A8 Android 14

Images/gif

Screen_Recording_20250904_163607_Woo.Dev.mp4

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

dangermattic · 2025-09-05T09:19:06Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

… into feat/WOOMOB-1149-pos-historical-orders-ptr # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt

wpmobilebot · 2025-09-05T09:29:37Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`6f414cc`
	Direct Download	`woocommerce-wear-prototype-build-pr14578-6f414cc.apk`

wpmobilebot · 2025-09-05T09:40:40Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`6f414cc`
	Direct Download	`woocommerce-prototype-build-pr14578-6f414cc.apk`

codecov-commenter · 2025-09-05T10:07:01Z

Codecov Report

❌ Patch coverage is 87.50000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.04%. Comparing base (9720e3c) to head (6f414cc).
⚠️ Report is 337 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../android/ui/woopos/orders/WooPosOrdersViewModel.kt	80.95%	4 Missing and 4 partials ⚠️
...android/ui/woopos/orders/WooPosOrdersDataSource.kt	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #14578      +/-   ##
============================================
+ Coverage     37.99%   38.04%   +0.05%     
- Complexity     9453     9466      +13     
============================================
  Files          2041     2041              
  Lines        114328   114369      +41     
  Branches      15162    15170       +8     
============================================
+ Hits          43443    43517      +74     
+ Misses        66902    66867      -35     
- Partials       3983     3985       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… into feat/WOOMOB-1149-pos-historical-orders-ptr # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt

kidinov · 2025-09-08T10:04:27Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt

    BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) }

    Row(modifier = Modifier.fillMaxSize()) {
+        // Left pane


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

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

Thanks for the hints. Done in e4d24ab

kidinov · 2025-09-08T10:07:09Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt

    }

+    fun clearCache() = ordersCache.clear()
+


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

True, removed in 3292780

kidinov · 2025-09-08T10:10:55Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt

-sealed class LoadOrdersResult {
-    data class Success(val orders: List<Order>) : LoadOrdersResult()
-    data class Error(val message: String) : LoadOrdersResult()
+sealed interface LoadOrdersResult {


Any reason specifically for interface here?

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

kidinov · 2025-09-08T10:20:18Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt

-                        )
-                    }
+            val refreshing =
+                when (val s = state) {


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) }

Enabled state in a5378e8

kidinov · 2025-09-08T10:25:14Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt

-    init {
-        refreshOrders()
-    }
+    private var orders: List<Order> = emptyList()


Why to save orders in 2 places? The same about selectedId. You have to have 1 source of the truth - _state variable.

It feels that all this code can be simpler:

@HiltViewModel class WooPosOrdersViewModel @Inject constructor( private val ordersDataSource: WooPosOrdersDataSource ) : ViewModel() { private val _state = MutableStateFlow<WooPosOrdersState>(WooPosOrdersState.Loading()) val state: StateFlow<WooPosOrdersState> = _state.asStateFlow() init { loadOrders() } fun onOrderSelected(orderId: Long) { 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 refresh() { 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() } private fun loadOrders() { viewModelScope.launch { ordersDataSource.loadOrders().collect { result -> when (result) { 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 updateContentState(orders: List<Order>) { val currentSelectedId = (_state.value as? WooPosOrdersState.Content)?.selectedOrderId val newSelectedId = currentSelectedId?.takeIf { id -> orders.any { it.id == id } } ?: orders.firstOrNull()?.id _state.value = WooPosOrdersState.Content( 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 ) } }

I think it make sense if WooPosOrdersDataSource won't return SuccessCache if there is nothing in the cache as well:

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

Awesome, done in 73bec2d

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt

kidinov

Thanks @toupper !

I left a few comments! Please take a look. Especially I think we should not have 2 different states in the view model as it source of many bugs having to sync them.

Also, it’s generally not a good idea to use when with else in a sealed class. This can make it harder to catch bugs when a new type is added, as the compiler won't complain

I didn't test the implementation yet, as I want to see what you think about my comments.

kidinov · 2025-09-10T06:17:45Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt

+            state = state,
+            onBackClicked = onBackClicked,
+            onRefresh = viewModel::refresh,
+            isRefreshing = viewModel.isRefreshing(),


The issue here is that isRefreshing is a function call that derives state from the existing state flow, rather than being part of the state itself. This violates the principle of having a single source of truth for UI state. So basically during composition, in some moment you go to the VM and ask for another piece of state, and there is no guarantee that it will still be the same state that you are rendering at the moment, which may end up with inconsistent state being displayed to the user

Something like that would solve this potential issue:

Refactor_pull-to-refresh_logic_in_WooPosOrders_components.patch

Thanks for that @kidinov! I applied it in 7cf9f93

kidinov · 2025-09-10T06:18:59Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt

+        (state.value as? WooPosOrdersState.Content)
+            ?.pullToRefreshState == WooPosPullToRefreshState.Refreshing
+
+    fun refresh() {


np: onRefresh would follow the convention when the functions are called as a reaction to the user’s action

Good point, done in ac705b3

kidinov

LGTM! Before merging, please take a look at the comments I left especially the one about isRefreshing method

toupper added 4 commits September 4, 2025 15:19

Enhance orders state and adapt clients

a890474

Implement Pull To Refresh

cc911fc

Add unit tests

73ba760

Remove non necessary comment

313a64c

toupper marked this pull request as draft September 5, 2025 09:18

Merge branch 'feat/WOOMOB-1145-pos-historical-orders-in-memory-cache'…

b0f9a13

… into feat/WOOMOB-1149-pos-historical-orders-ptr # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt

toupper added 4 commits September 5, 2025 16:44

Fix tests

0df5676

Fix Detekt

ba95772

add missing import

9713b6d

Base automatically changed from feat/WOOMOB-1145-pos-historical-orders-in-memory-cache to trunk September 5, 2025 14:59

toupper added 2 commits September 5, 2025 17:27

Refactor to better readability

b70396c

Simplify state

020078a

toupper requested a review from kidinov September 5, 2025 15:43

toupper changed the title ~~Feat/woomob 1149 pos historical orders ptr~~ [POS Historical Orders] Pull to Refresh Sep 5, 2025

toupper added the type: task An internally driven task. label Sep 5, 2025

toupper added this to the 23.3 milestone Sep 5, 2025

toupper marked this pull request as ready for review September 5, 2025 15:45

Fix detekt

48dc8e5

kidinov self-assigned this Sep 8, 2025

kidinov reviewed Sep 8, 2025

View reviewed changes

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt Outdated Show resolved Hide resolved

kidinov reviewed Sep 8, 2025

View reviewed changes

toupper added 7 commits September 9, 2025 10:04

Refactor for better readability

e4d24ab

Remove elvis operator applied to a non-nullable type

3292780

Revert to a sealed class

4340083

Use logic from view model

6c9aa20

Add information about the pull refresh enabled state

a5378e8

Simplify view model

73bec2d

Fix test

2894ae4

toupper requested a review from kidinov September 9, 2025 13:07

kidinov reviewed Sep 10, 2025

View reviewed changes

kidinov approved these changes Sep 10, 2025

View reviewed changes

toupper added 3 commits September 10, 2025 11:51

Keep one source of truth for refreshing

7cf9f93

Rename to follow convention,

ac705b3

Better naming

6f414cc

toupper merged commit cd0480a into trunk Sep 10, 2025
17 checks passed

toupper deleted the feat/WOOMOB-1149-pos-historical-orders-ptr branch September 10, 2025 10:34

[POS Historical Orders] Pull to Refresh #14578

[POS Historical Orders] Pull to Refresh #14578

Conversation

toupper commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to reproduce

Testing information

Images/gif

Uh oh!

dangermattic commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kidinov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

toupper commented Sep 5, 2025 •

edited

Loading

dangermattic commented Sep 5, 2025 •

edited

Loading

wpmobilebot commented Sep 5, 2025 •

edited

Loading

wpmobilebot commented Sep 5, 2025 •

edited

Loading

codecov-commenter commented Sep 5, 2025 •

edited

Loading

kidinov Sep 8, 2025 •

edited

Loading

kidinov Sep 8, 2025 •

edited

Loading

kidinov left a comment •

edited

Loading

kidinov Sep 10, 2025 •

edited

Loading