-
Notifications
You must be signed in to change notification settings - Fork 136
[POS - Historical Orders] In-memory Cache #14569
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.
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
[POS - Historical Orders] In-memory Cache #14569
Conversation
…nto feat/WOOMOB-1145-pos-historical-orders-in-memory-cache # 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
…eat/WOOMOB-1145-pos-historical-orders-in-memory-cache # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt
…eat/WOOMOB-1145-pos-historical-orders-in-memory-cache # 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
…eat/WOOMOB-1145-pos-historical-orders-in-memory-cache
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
...c/test/kotlin/com/woocommerce/android/ui/woopos/common/data/WooPosOrdersInMemoryCacheTest.kt
Fixed
Show fixed
Hide fixed
| private val mutex = Mutex() | ||
|
|
||
| companion object { | ||
| private const val INITIAL_CAPACITY = MAX_CACHE_SIZE |
There was a problem hiding this comment.
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
?
| @@ -0,0 +1,41 @@ | |||
| package com.woocommerce.android.ui.woopos.common.data | |||
There was a problem hiding this comment.
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.
|
|
||
| private val ordersCache = LinkedHashMap<Long, Order>(INITIAL_CAPACITY, LOAD_FACTOR, true) | ||
|
|
||
| override suspend fun addAll(orders: List<Order>) = mutex.withLock { |
There was a problem hiding this comment.
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.
| private const val LOAD_FACTOR = 0.75f | ||
| } | ||
|
|
||
| private val ordersCache = LinkedHashMap<Long, Order>(INITIAL_CAPACITY, LOAD_FACTOR, true) |
There was a problem hiding this comment.
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 fun onDestroy() { | ||
| runBlocking { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| _state.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| error = result.message ?: "Unknown error" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I removed it 👍
| } | ||
|
|
||
| private suspend fun List<OrderEntity>.toAppModels(): List<Order> = map { | ||
| private suspend fun List<OrderEntity>?.toAppModels(): List<Order> = this?.map { |
There was a problem hiding this comment.
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?
|
|
||
| import com.woocommerce.android.model.Order | ||
|
|
||
| interface WooPosOrdersCache { |
There was a problem hiding this comment.
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.
kidinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a few comments; please take a look!
|
Version |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14569 +/- ##
=========================================
Coverage 37.99% 37.99%
- Complexity 9450 9453 +3
=========================================
Files 2040 2041 +1
Lines 114311 114328 +17
Branches 15162 15162
=========================================
+ Hits 43430 43443 +13
- Misses 66900 66902 +2
- Partials 3981 3983 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Closes WOOMOB-1145
With this PR, we add an in-memory cache when fetching the POS Orders. The cache content is removed when leaving POS.
Steps to reproduce
Testing information
Tested on Samsung Tab A8 Android 14
Images/gif
Screen_Recording_20250904_120742_Woo.Dev.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.