Skip to content
New issue

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

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

Already on GitHub? # to your account

[Woo POS][Coupons] Refactor WooPosProductsDataSource to Introduce WooPosBaseDataSource for Improved Maintainability #13764

Open
wants to merge 25 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d1d8ea4
Add new model class to hold the fetched result
AnirudhBhat Mar 14, 2025
98c8548
Add a new data class to hold all fetch options
AnirudhBhat Mar 14, 2025
7c7a706
Add a new interface contract for fetching data from the source
AnirudhBhat Mar 14, 2025
24b0e37
Add a new BaseDataSource class that defines how the data must be fetc…
AnirudhBhat Mar 14, 2025
5adc288
Extend WooPosProductsDataSource from BaseDataSource and implement rel…
AnirudhBhat Mar 14, 2025
747e86c
Update WooPosProductsDataSourceTest.kt and make the tests pass
AnirudhBhat Mar 14, 2025
def5811
Update WooPosItemsViewModel to integrate necessary changes from WooPo…
AnirudhBhat Mar 14, 2025
2e1d3ac
Update WooPosItemsViewModelTest.kt and make the tests pass
AnirudhBhat Mar 14, 2025
88370e3
Update WooPosSplashViewModel
AnirudhBhat Mar 14, 2025
36ff06c
Update WooPosSplashViewModelTest
AnirudhBhat Mar 14, 2025
e1b379b
Fix imports
AnirudhBhat Mar 14, 2025
18b2635
Fix imports
AnirudhBhat Mar 14, 2025
2df2d8f
Merge branch 'trunk' into issue/13746-refactor-data-source-business-l…
AnirudhBhat Mar 14, 2025
35e8718
Fix merge conflicts
AnirudhBhat Mar 14, 2025
2c6d30a
Make failing tests pass
AnirudhBhat Mar 14, 2025
75ff8e1
Add WooPosBaseDataSourceTest
AnirudhBhat Mar 14, 2025
c8f64bd
Add test to verify cached data and remote failure is emitted
AnirudhBhat Mar 14, 2025
98385d4
Add test to verify force refresh fetches new data from remote
AnirudhBhat Mar 14, 2025
a572dbe
Remove unnecessary method
AnirudhBhat Mar 14, 2025
a689dde
Add test to verify given no cached data and remote failure when fetch…
AnirudhBhat Mar 14, 2025
3049c64
Renamed BaseDataSource.kt to WooPosBaseDataSource
AnirudhBhat Mar 14, 2025
9ee1b5e
Refactor WooPosBaseDataSource to pass in FetchOptions for abstract me…
AnirudhBhat Mar 14, 2025
f949718
Merge branch 'trunk' into issue/13746-refactor-data-source-business-l…
AnirudhBhat Mar 31, 2025
6715443
Fix merge conflicts and failing tests
AnirudhBhat Mar 31, 2025
7f7782f
Remove blank line
AnirudhBhat Mar 31, 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
Expand Up @@ -11,6 +11,8 @@ import com.woocommerce.android.ui.woopos.featureflags.WooPosIsProductsSearchEnab
import com.woocommerce.android.ui.woopos.home.ChildToParentEvent
import com.woocommerce.android.ui.woopos.home.WooPosChildrenToParentEventSender
import com.woocommerce.android.ui.woopos.home.items.WooPosItemNavigationData.VariableProductData
import com.woocommerce.android.ui.woopos.home.items.common.FetchOptions
import com.woocommerce.android.ui.woopos.home.items.common.FetchResult
import com.woocommerce.android.ui.woopos.home.items.navigation.WooPosItemsNavigator
import com.woocommerce.android.ui.woopos.home.items.navigation.WooPosItemsNavigator.WooPosItemsScreenNavigationEvent
import com.woocommerce.android.ui.woopos.home.items.navigation.WooPosItemsNavigator.WooPosItemsScreenNavigationEvent.NavigateToVariationsScreen
Expand Down Expand Up @@ -197,18 +199,20 @@ class WooPosItemsViewModel @Inject constructor(
WooPosItemsViewState.Loading(withCart = withCart)
}

productsDataSource.loadSimpleProducts(forceRefreshProducts = forceRefreshProducts).collect { result ->
productsDataSource.fetchData(
fetchOptions = FetchOptions(forceRefresh = forceRefreshProducts),
).collect { result ->
when (result) {
is WooPosProductsDataSource.ProductsResult.Cached -> {
if (result.products.isNotEmpty()) {
_viewState.value = result.products.toContentState()
is FetchResult.Cached -> {
if (result.data.isNotEmpty()) {
_viewState.value = result.data.toContentState()
}
}

is WooPosProductsDataSource.ProductsResult.Remote -> {
is FetchResult.Remote -> {
_viewState.value = when {
result.productsResult.isSuccess -> {
val products = result.productsResult.getOrThrow()
result.result.isSuccess -> {
val products = result.result.getOrThrow()
if (products.isNotEmpty()) {
products.toContentState(
paginationState = if (loadMoreProductsJob?.isActive == true) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.woocommerce.android.ui.woopos.home.items.common

import kotlinx.coroutines.flow.Flow

interface FetchDataSource<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I'm trying to figure out where the interface would be used. Did you have any specific use case in mind, like DI, testing, etc. If we provide base class, maybe interface is not needed?

fun fetchData(
fetchOptions: FetchOptions
): Flow<FetchResult<T>>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.woocommerce.android.ui.woopos.home.items.common

data class FetchOptions(
val productId: Long? = null,
val forceRefresh: Boolean = false,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.woocommerce.android.ui.woopos.home.items.common

sealed class FetchResult<T> {
data class Cached<T>(val data: List<T>) : FetchResult<T>()
data class Remote<T>(val result: Result<List<T>>) : FetchResult<T>()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.woocommerce.android.ui.woopos.home.items.common

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOn

abstract class WooPosBaseDataSource<T> : FetchDataSource<T> {

protected abstract suspend fun fetchFromCache(fetchOptions: FetchOptions): List<T>
protected abstract suspend fun fetchFromRemote(
fetchOptions: FetchOptions,
): Result<List<T>>
protected abstract suspend fun updateCache(fetchOptions: FetchOptions, data: List<T>)

override fun fetchData(
fetchOptions: FetchOptions
): Flow<FetchResult<T>> = flow {
if (fetchOptions.forceRefresh) {
updateCache(fetchOptions, data = emptyList())
}
Comment on lines +16 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider mutex or other concurrency safeguards for force-refresh cache clearing.

When fetchOptions.forceRefresh is true, the existing cache is cleared without an apparent concurrency lock around updateCache. If multiple coroutines call fetchData simultaneously, there's a risk of overwriting each other's cache updates. To ensure thread safety, consider using a mutex or synchronized approach, similar to how other data sources might guard shared state.


val cachedData = fetchFromCache(fetchOptions)
emit(FetchResult.Cached(cachedData))

val remoteResult = fetchFromRemote(fetchOptions)
if (remoteResult.isSuccess) {
val remoteData = remoteResult.getOrThrow()
updateCache(fetchOptions = fetchOptions, data = remoteData)
emit(FetchResult.Remote(Result.success(remoteData)))
} else {
emit(
FetchResult.Remote(
Result.failure(remoteResult.exceptionOrNull() ?: Exception("Unknown error"))
)
)
}
}.flowOn(Dispatchers.IO)
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@ package com.woocommerce.android.ui.woopos.home.items.products
import com.woocommerce.android.model.Product
import com.woocommerce.android.ui.products.ProductStatus
import com.woocommerce.android.ui.products.selector.ProductListHandler
import com.woocommerce.android.ui.woopos.home.items.common.FetchOptions
import com.woocommerce.android.ui.woopos.home.items.common.WooPosBaseDataSource
import com.woocommerce.android.util.WooLog
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.take
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import kotlinx.coroutines.withContext
Expand All @@ -20,46 +18,13 @@ import javax.inject.Singleton
@Singleton
class WooPosProductsDataSource @Inject constructor(
private val handler: ProductListHandler,
) {
) : WooPosBaseDataSource<Product>() {
private var productCache: List<Product> = emptyList()
private val cacheMutex = Mutex()

val hasMorePages: Boolean
get() = handler.canLoadMore.get()

fun loadSimpleProducts(forceRefreshProducts: Boolean): Flow<ProductsResult> = flow {
if (forceRefreshProducts) {
updateProductCache(emptyList())
}

emit(ProductsResult.Cached(productCache))

val result = handler.loadFromCacheAndFetch(
forceRefresh = forceRefreshProducts,
searchType = ProductListHandler.SearchType.DEFAULT,
includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable),
filters = mapOf(
WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(),
)
)

if (result.isSuccess) {
val remoteProducts = handler.productsFlow.first()
updateProductCache(remoteProducts)
emit(ProductsResult.Remote(Result.success(productCache)))
} else {
result.logFailure()
emit(
ProductsResult.Remote(
Result.failure(
result.exceptionOrNull() ?: Exception("Unknown error")
)
)
)
}
}.flowOn(Dispatchers.IO).take(2)

suspend fun loadMore(): Result<List<Product>> = withContext(Dispatchers.IO) {
val result = handler.loadMore(
includeTypes = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable),
Expand Down Expand Up @@ -88,4 +53,29 @@ class WooPosProductsDataSource @Inject constructor(
data class Cached(val products: List<Product>) : ProductsResult()
data class Remote(val productsResult: Result<List<Product>>) : ProductsResult()
}

override suspend fun fetchFromCache(fetchOptions: FetchOptions): List<Product> {
return productCache
}

override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> {
val result = handler.loadFromCacheAndFetch(
forceRefresh = true,
includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable),
searchType = ProductListHandler.SearchType.DEFAULT,
filters = mapOf(
WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(),
)
)
return if (result.isSuccess) {
Result.success(handler.productsFlow.first())
} else {
Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products"))
}
}
Comment on lines +61 to +76
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remote fetch strategy.

Using handler.loadFromCacheAndFetch(forceRefresh = true, ...) always forces a refresh from the server. If partial or non-forced fetch is required, passing fetchOptions.forceRefresh might be more flexible.

-val result = handler.loadFromCacheAndFetch(
-    forceRefresh = true,
-    ...
-)
+val result = handler.loadFromCacheAndFetch(
+    forceRefresh = fetchOptions.forceRefresh,
+    ...
+)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> {
val result = handler.loadFromCacheAndFetch(
forceRefresh = true,
includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable),
searchType = ProductListHandler.SearchType.DEFAULT,
filters = mapOf(
WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(),
)
)
return if (result.isSuccess) {
Result.success(handler.productsFlow.first())
} else {
Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products"))
}
}
override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> {
val result = handler.loadFromCacheAndFetch(
forceRefresh = fetchOptions.forceRefresh,
includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable),
searchType = ProductListHandler.SearchType.DEFAULT,
filters = mapOf(
WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(),
)
)
return if (result.isSuccess) {
Result.success(handler.productsFlow.first())
} else {
Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products"))
}
}


override suspend fun updateCache(fetchOptions: FetchOptions, data: List<Product>) {
updateProductCache(data)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package com.woocommerce.android.ui.woopos.splash

import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.woocommerce.android.ui.woopos.home.items.common.FetchOptions
import com.woocommerce.android.ui.woopos.home.items.common.FetchResult
import com.woocommerce.android.ui.woopos.home.items.products.WooPosProductsDataSource
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.Loaded
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker
Expand All @@ -23,11 +25,13 @@ class WooPosSplashViewModel @Inject constructor(

init {
viewModelScope.launch {
productsDataSource.loadSimpleProducts(forceRefreshProducts = true)
productsDataSource.fetchData(
fetchOptions = FetchOptions(forceRefresh = true)
)
.collect { result ->
when (result) {
is WooPosProductsDataSource.ProductsResult.Cached -> {}
is WooPosProductsDataSource.ProductsResult.Remote -> {
is FetchResult.Cached -> {}
is FetchResult.Remote -> {
_state.value = WooPosSplashState.Loaded
trackPosLoaded()
}
Expand Down
Loading
Loading