Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e1aab9d
First approach to cache
toupper Aug 29, 2025
3f9d131
Implement cache and use it in view model
toupper Aug 29, 2025
a77834d
Remove comments
toupper Aug 29, 2025
659ad10
Show the cached content right away
toupper Aug 29, 2025
4a5f936
Clear cache when leaving POS
toupper Aug 29, 2025
2bc7118
Add max size for orders cache
toupper Aug 29, 2025
0a349b7
Add tests for cache
toupper Aug 29, 2025
7f2acde
Bring data source back to the orders package
toupper Aug 29, 2025
fe1c4b5
WIP: Merge branch 'feat/WOOMOB-1145-pos-historical-orders-fetching' i…
toupper Sep 2, 2025
d4d8169
Merge branch 'feat/WOOMOB-1145-pos-historical-orders-fetching' into f…
toupper Sep 2, 2025
2539408
Merge branch 'feat/WOOMOB-1145-pos-historical-orders-fetching' into f…
toupper Sep 4, 2025
a8ef33d
Merge branch 'feat/WOOMOB-1145-pos-historical-orders-fetching' into f…
toupper Sep 4, 2025
96c3a09
Fix test
toupper Sep 4, 2025
a57f5d7
Remove non necessary suspend
toupper Sep 4, 2025
333effb
Move file to its right package
toupper Sep 4, 2025
60d1cf9
Fix detekt
toupper Sep 4, 2025
69cc4dd
fix lint and simplify test
toupper Sep 5, 2025
e0e5325
Improve POS Orders cache
toupper Sep 5, 2025
0cadb0c
Remove non necessary code
toupper Sep 5, 2025
fe798f6
Move file to its right package
toupper Sep 5, 2025
10c135c
Revert change
toupper Sep 5, 2025
eab3bc7
Add unit test for clearing cache on splash screen
toupper Sep 5, 2025
1d91751
Add test
toupper Sep 5, 2025
02fa76f
Simplify cache
toupper Sep 5, 2025
a57941c
Make cache a singleton
toupper Sep 5, 2025
9720e3c
Fallback string not necessary
toupper Sep 5, 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
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.woocommerce.android.di

import com.woocommerce.android.ui.woopos.common.data.WooPosOrdersInMemoryCache
import com.woocommerce.android.ui.woopos.common.data.WooPosProductsCache
import com.woocommerce.android.ui.woopos.common.data.WooPosProductsInMemoryCache
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosOrdersCache
import dagger.Binds
import dagger.Module
import dagger.hilt.InstallIn
Expand All @@ -14,4 +16,8 @@ abstract class WooPosModule {
@Binds
@Singleton
abstract fun provideWooPosProductsCache(implementation: WooPosProductsInMemoryCache): WooPosProductsCache

@Binds
@Singleton
abstract fun provideWooPosOrdersCache(implementation: WooPosOrdersInMemoryCache): WooPosOrdersCache
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package com.woocommerce.android.ui.woopos.common.data
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to place it into woopos.orders. The product-related code here is because it's used in the list and the search.


import com.woocommerce.android.model.Order
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosOrdersCache
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosOrdersCache.Companion.MAX_CACHE_SIZE
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import javax.inject.Inject

class WooPosOrdersInMemoryCache @Inject constructor() : WooPosOrdersCache {
private val mutex = Mutex()

companion object {
private const val INITIAL_CAPACITY = MAX_CACHE_SIZE
Copy link
Contributor

@kidinov kidinov Sep 5, 2025

Choose a reason for hiding this comment

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

As the goal is to have this equal to the page size, I think we should reference the same constant used in WooPosOrdersDataSource and here.

Maybe adding const val ORDERS_PAGE_SIZE = 25 to WooPosOrdersDataSource.kt and then

private const val INITIAL_CAPACITY = ORDERS_PAGE_SIZE

?

private const val LOAD_FACTOR = 0.75f
}

private val ordersCache = LinkedHashMap<Long, Order>(INITIAL_CAPACITY, LOAD_FACTOR, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason to use hash map here? I think we can have just a list of orders here


override suspend fun addAll(orders: List<Order>) = mutex.withLock {
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on the call, this should probably be setAll that just replaces all you have in the cache with the new list.

addAllInternal(orders)
}

override suspend fun getAll(): List<Order> = mutex.withLock {
ordersCache.values.toList()
}

override suspend fun clear() = mutex.withLock {
ordersCache.clear()
}

private fun addAllInternal(orders: List<Order>) {
orders.forEach { order ->
ordersCache[order.id] = order
if (ordersCache.size > MAX_CACHE_SIZE) {
val keysToRemove = ordersCache.keys.take(ordersCache.size - MAX_CACHE_SIZE)
keysToRemove.forEach { ordersCache.remove(it) }
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.woocommerce.android.ui.woopos.common.data.searchbyidentifier

import com.woocommerce.android.model.Order

interface WooPosOrdersCache {
Copy link
Contributor

Choose a reason for hiding this comment

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

np: I think we don't really need an interface here. For the products, it was introduced because we were planning to migrate to persistent storage; therefore, we declared one interface with potential for two implementations. Here, at least for now, there are no plans for that, so maybe keeping it simple would be easier.

companion object {
const val MAX_CACHE_SIZE = 25
}

suspend fun addAll(orders: List<Order>)

suspend fun getAll(): List<Order>

suspend fun clear()
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package com.woocommerce.android.ui.woopos.orders
import com.woocommerce.android.model.Order
import com.woocommerce.android.model.OrderMapper
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosOrdersCache
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
import org.wordpress.android.fluxc.network.rest.wpcom.wc.order.OrderRestClient
import org.wordpress.android.fluxc.network.rest.wpcom.wc.order.OrderRestClient.OrderBy
import org.wordpress.android.fluxc.persistence.entity.OrderEntity
Expand All @@ -17,8 +20,12 @@ class WooPosOrdersDataSource @Inject constructor(
private val restClient: OrderRestClient,
private val selectedSite: SelectedSite,
private val orderMapper: OrderMapper,
private val ordersCache: WooPosOrdersCache
) {
suspend fun loadOrders(): LoadOrdersResult {
fun loadOrders(): Flow<LoadOrdersResult> = flow {
val cached = ordersCache.getAll()
emit(LoadOrdersResult.Success(cached))

val result = restClient.fetchOrders(
site = selectedSite.get(),
count = 25,
Expand All @@ -29,14 +36,16 @@ class WooPosOrdersDataSource @Inject constructor(
createdVia = "pos-rest-api"
)

return if (result.isError) {
LoadOrdersResult.Error(result.error.message)
if (result.isError) {
emit(LoadOrdersResult.Error(result.error.message))
} else {
LoadOrdersResult.Success(result.orders.toAppModels())
val mapped = result.orders.toAppModels()
ordersCache.addAll(mapped)
emit(LoadOrdersResult.Success(result.orders.toAppModels()))
}
}

private suspend fun List<OrderEntity>.toAppModels(): List<Order> = map {
private suspend fun List<OrderEntity>?.toAppModels(): List<Order> = this?.map {
Copy link
Contributor

@kidinov kidinov Sep 5, 2025

Choose a reason for hiding this comment

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

I’m a bit confused about this change. I thought the list here was always non-nullable, right?

orderMapper.toAppModel(it)
}
} ?: emptyList()
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,28 @@ class WooPosOrdersViewModel @Inject constructor(
viewModelScope.launch {
_state.update { it.copy(isLoading = true, error = null) }

when (val result = ordersDataSource.loadOrders()) {
is LoadOrdersResult.Error -> {
_state.update {
it.copy(
isLoading = false,
error = result.message ?: "Unknown error"
)
ordersDataSource.loadOrders().collect { result ->
when (result) {
is LoadOrdersResult.Error -> {
_state.update {
it.copy(
isLoading = false,
error = result.message ?: "Unknown error"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just keep in mind to not show this error to a user, as this won't be localized

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and btw message is never null so ?: "Unknown error" not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I removed it 👍

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

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
)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ import androidx.core.view.WindowInsetsCompat
import com.woocommerce.android.ui.woopos.cardreader.WooPosCardReaderFacade
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.data.searchbyidentifier.WooPosOrdersCache
import com.woocommerce.android.ui.woopos.home.items.coupons.creation.WooPosCouponCreationFacade
import com.woocommerce.android.ui.woopos.support.WooPosGetSupportFacade
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker
import com.woocommerce.android.ui.woopos.util.ext.isGestureNavigation
import dagger.hilt.android.AndroidEntryPoint
import kotlinx.coroutines.runBlocking
import javax.inject.Inject

@AndroidEntryPoint
Expand All @@ -35,6 +37,9 @@ class WooPosActivity : AppCompatActivity() {
@Inject
lateinit var wooPosCouponCreationFacade: WooPosCouponCreationFacade

@Inject
lateinit var ordersCache: WooPosOrdersCache

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
WindowCompat.setDecorFitsSystemWindows(window, false)
Expand All @@ -53,6 +58,13 @@ class WooPosActivity : AppCompatActivity() {
}
}
}

override fun onDestroy() {
runBlocking {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that for 2 reasons:

  • You are blocking the main thread here
  • Overall, are you sure that we want to clean the cache here? Keep in mind that on Android, activity can be destroyed on configuration change. I wouldn't do that at all, especially since currently there are up to 25 orders in memory

Copy link
Contributor Author

@toupper toupper Sep 5, 2025

Choose a reason for hiding this comment

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

Thanks for the comment @kidinov!

You are blocking the main thread here

If we do it asynchronously, we lose the reference to the cache, so we end up not deleting the cache.

Overall, are you sure that we want to clean the cache here? Keep in mind that on Android, activity can be destroyed on configuration change. I wouldn't do that at all, especially since currently there are up to 25 orders in memory

What other place would be more suitable for this action?

ordersCache.clear()
}
super.onDestroy()
}
}

@Composable
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package com.woocommerce.android.ui.woopos.common.data

import com.woocommerce.android.ui.orders.OrderTestUtils
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
import kotlinx.coroutines.test.runTest
import org.assertj.core.api.Assertions.assertThat
import org.junit.Before
import org.junit.Test

@ExperimentalCoroutinesApi
class WooPosOrdersInMemoryCacheTest {

private lateinit var cache: WooPosOrdersInMemoryCache

@Before
fun setup() {
cache = WooPosOrdersInMemoryCache()
}

@Test
fun `when cache is empty, then getAll returns empty list`() = runTest {
// WHEN
val result = cache.getAll()

// THEN
assertThat(result).isEmpty()
}

@Test
fun `when orders are added, then getAll returns all orders`() = runTest {
// GIVEN
val orders = listOf(OrderTestUtils.generateTestOrder(1), OrderTestUtils.generateTestOrder(2))

// WHEN
cache.addAll(orders)
val result = cache.getAll()

// THEN
assertThat(result).hasSize(2)
assertThat(result).containsExactlyInAnyOrderElementsOf(orders)
}

@Test
fun `when cache is cleared, then getAll returns empty list`() = runTest {
// GIVEN
val orders = listOf(OrderTestUtils.generateTestOrder(1), OrderTestUtils.generateTestOrder(2))
cache.addAll(orders)

// WHEN
cache.clear()
val result = cache.getAll()

// THEN
assertThat(result).isEmpty()
}

@Test
fun `when adding order with existing id, then it should replace old order`() = runTest {
// GIVEN
val order1 = OrderTestUtils.generateTestOrder(1)
val order1Updated = OrderTestUtils.generateTestOrder(1).copy(currency = "TEST")

// WHEN
cache.addAll(listOf(order1))
cache.addAll(listOf(order1Updated))
val result = cache.getAll().first { it.id == 1L }

// THEN
assertThat(result).isEqualTo(order1Updated)
assertThat(result.currency).isEqualTo("TEST")
}

@Test
fun `when adding more orders than max cache size, then oldest orders should be removed`() = runTest {
// GIVEN
val orders = (1..10005L).map { OrderTestUtils.generateTestOrder(it) }

// WHEN
cache.addAll(orders)
val firstOrder = cache.getAll().find { it.id == 1L }
val lastOrder = cache.getAll().find { it.id == 10005L }

// THEN
assertThat(firstOrder).isNull()
assertThat(lastOrder).isNotNull
assertThat(cache.getAll()).hasSize(25)
}

@Test
fun `when multiple threads access cache concurrently, then data remains consistent`() = runTest {
// GIVEN
val initialOrders = (1..100L).map { OrderTestUtils.generateTestOrder(it) }
cache.addAll(initialOrders)

// WHEN - simulate concurrent access
val concurrentOperations = (101..200L).map { id ->
async {
val order = OrderTestUtils.generateTestOrder(id)
cache.addAll(listOf(order))
cache.getAll().find { it.id == id }
}
}

// THEN
val results = concurrentOperations.awaitAll()
assertThat(results).hasSize(100)
assertThat(results).doesNotContainNull()

val allCachedOrders = cache.getAll()
// Only MAX_CACHE_SIZE should remain
assertThat(allCachedOrders).hasSize(25)

// The most recent 25 orders should be present
val expectedIds = (176L..200L).toList()
assertThat(allCachedOrders.map { it.id }).containsExactlyInAnyOrderElementsOf(expectedIds)
}
}
Loading
Loading