Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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,5 @@
package com.woocommerce.android.ui.woopos.common.data

import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.model.toAppModel
import com.woocommerce.android.tools.SelectedSite
import kotlinx.coroutines.Dispatchers.IO
import kotlinx.coroutines.withContext
Expand All @@ -12,7 +10,8 @@ class WooPosGetVariationById @Inject constructor(
private val store: WCProductStore,
private val site: SelectedSite,
) {
suspend operator fun invoke(productId: Long, variationId: Long): ProductVariation? = withContext(IO) {
store.getVariationByRemoteId(site.getOrNull()!!, productId, variationId)?.toAppModel()
suspend operator fun invoke(productId: Long, variationId: Long): WooPosVariation? = withContext(IO) {
val siteModel = site.getOrNull() ?: return@withContext null
store.getVariationByRemoteId(siteModel, productId, variationId)?.toWooPosVariation()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package com.woocommerce.android.ui.woopos.common.data

import com.google.gson.Gson
import com.google.gson.JsonSyntaxException
import com.google.gson.reflect.TypeToken
import com.woocommerce.android.R
import com.woocommerce.android.model.Product
import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.viewmodel.ResourceProvider
import org.wordpress.android.fluxc.model.WCProductVariationModel
import org.wordpress.android.fluxc.persistence.entity.pos.WCPosVariationModel
import java.math.BigDecimal

/**
* POS-specific product variation model containing only the fields needed for POS functionality.
* This model provides better performance and cleaner separation compared to the general ProductVariation model.
*/
data class WooPosVariation(
val remoteVariationId: Long,
val remoteProductId: Long,
val globalUniqueId: String,
val price: BigDecimal?,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 This is technically enough for now, but we'll likely need to have access to both sale and regular prices quite soon, so I'd consider including them similarly to how we do it in the product model. Having said that, feel free to leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave these props for now and update the model when we need them.

val image: WooPosVariationImage?,
val attributes: List<WooPosVariationAttribute>,
val isVisible: Boolean,
val isDownloadable: Boolean
) {

data class WooPosVariationImage(
val source: String
)

data class WooPosVariationAttribute(
val id: Long?,
val name: String?,
val option: String?
)
}

fun ProductVariation.toWooPosVariation(): WooPosVariation {
return WooPosVariation(
remoteVariationId = remoteVariationId,
remoteProductId = remoteProductId,
globalUniqueId = globalUniqueId,
price = price,
image = image?.let { WooPosVariation.WooPosVariationImage(it.source) },
attributes = attributes.map {
WooPosVariation.WooPosVariationAttribute(
id = it.id,
name = it.name,
option = it.option
)
},
isVisible = isVisible,
isDownloadable = isDownloadable
)
}

@Suppress("SwallowedException")
fun WCProductVariationModel.toWooPosVariation(): WooPosVariation {
val attributesList = attributeList?.map { attribute ->
WooPosVariation.WooPosVariationAttribute(
id = attribute.id,
name = attribute.name,
option = attribute.option
)
} ?: emptyList()

val imageModel = try {
if (image.isNotEmpty()) getImageModel() else null
} catch (e: JsonSyntaxException) {
null
}

return WooPosVariation(
remoteVariationId = remoteVariationId.value,
remoteProductId = remoteProductId.value,
globalUniqueId = globalUniqueId,
price = price.toBigDecimalOrNull(),
image = imageModel?.src?.let { WooPosVariation.WooPosVariationImage(it) },
attributes = attributesList,
isVisible = status == "publish",
isDownloadable = downloadable
)
}

@Suppress("SwallowedException")
fun WCPosVariationModel.toWooPosVariation(): WooPosVariation {
val attributesList = try {
if (attributesJson.isNotEmpty()) {
parseAttributesJson(attributesJson)
} else {
emptyList()
}
} catch (e: JsonSyntaxException) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ I'd consider logging an error into the local log.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realized this is related to the below comment - since we are doing the parsing within the model, not in a separate class, we don't have access to the logger. I'd personally vote for moving the parsing to a mapper so we can log issues and re-use a single instance of gson.

Copy link
Contributor Author

@samiuelson samiuelson Sep 5, 2025

Choose a reason for hiding this comment

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

Yes, parsing logic is now in the WooPosVariationMapper.kt file.

emptyList()
}

return WooPosVariation(
remoteVariationId = remoteVariationId.value,
remoteProductId = remoteProductId.value,
globalUniqueId = globalUniqueId,
price = price.toBigDecimalOrNull(),
image = if (imageUrl.isNotEmpty()) WooPosVariation.WooPosVariationImage(imageUrl) else null,
attributes = attributesList,
isVisible = status == "publish",
isDownloadable = downloadable
)
}

private val gson by lazy { Gson() }
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Having an instance of gson per instance feels a bit off. Have you considered creating a separate mapper class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, done: abba849


@Suppress("SwallowedException")
private fun parseAttributesJson(attributesJson: String): List<WooPosVariation.WooPosVariationAttribute> {
return try {
val type = object : TypeToken<List<AttributeJsonItem>>() {}.type
val items: List<AttributeJsonItem> = gson.fromJson(attributesJson, type)
items.map { item ->
WooPosVariation.WooPosVariationAttribute(
id = item.id,
name = item.name,
option = item.option
)
}
} catch (e: JsonSyntaxException) {
emptyList()
}
}

private data class AttributeJsonItem(
val id: Long?,
val name: String?,
val option: String?
)

fun WooPosVariation.getNameForPOS(
parentProduct: Product? = null,
resourceProvider: ResourceProvider,
): String {
return parentProduct?.variationEnabledAttributes?.joinToString(", ") { attribute ->
val option = attributes.firstOrNull { it.name == attribute.name }
if (option?.option != null) {
"${attribute.name}: ${option.option}"
} else {
resourceProvider.getString(
R.string.woopos_variations_any_variation,
attribute.name
)
}
} ?: attributes.joinToString(", ") { attribute ->
when {
attribute.option != null && attribute.name != null -> "${attribute.name}: ${attribute.option}"
attribute.option != null -> attribute.option
attribute.name != null -> resourceProvider.getString(
R.string.woopos_variations_any_variation,
attribute.name
)
else -> ""
}
}
}

fun WooPosVariation.getName(parentProduct: Product? = null): String {
return parentProduct?.variationEnabledAttributes?.joinToString(" - ") { attribute ->
val option = attributes.firstOrNull { it.name == attribute.name }
option?.option ?: "Any ${attribute.name}"
} ?: attributes.mapNotNull { it.option }.joinToString(" - ")
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.woocommerce.android.ui.woopos.common.data.searchbyidentifier

import com.woocommerce.android.model.Product
import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.ui.woopos.common.data.WooPosProductsTypesFilterConfig
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import com.woocommerce.android.ui.woopos.common.data.WooPosVariationsTypesFilterConfig
import com.woocommerce.android.ui.woopos.common.util.WooPosLogWrapper
import org.wordpress.android.fluxc.store.WCProductStore
Expand Down Expand Up @@ -75,7 +75,7 @@ class WooPosSearchByIdentifier @Inject constructor(
}
}

private fun meetsVariationFilterRequirements(variation: ProductVariation): Boolean {
private fun meetsVariationFilterRequirements(variation: WooPosVariation): Boolean {
val requiredStatus = variationFilterConfig.filters[VariationFilterOption.STATUS]
val hasValidStatus = when (requiredStatus) {
WooPosVariationsTypesFilterConfig.VARIATION_STATUS_PUBLISH -> variation.isVisible
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package com.woocommerce.android.ui.woopos.common.data.searchbyidentifier

import com.woocommerce.android.model.Product
import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation

sealed class WooPosSearchByIdentifierResult {
data class Success(val product: Product) : WooPosSearchByIdentifierResult()
data class VariationSuccess(val variation: ProductVariation, val parentProduct: Product) :
data class VariationSuccess(val variation: WooPosVariation, val parentProduct: Product) :
WooPosSearchByIdentifierResult()

data class Failure(val error: Error) : WooPosSearchByIdentifierResult()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package com.woocommerce.android.ui.woopos.common.data.searchbyidentifier

import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.model.toAppModel
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import com.woocommerce.android.ui.woopos.common.data.toWooPosVariation
import com.woocommerce.android.ui.woopos.home.items.variations.WooPosVariationsLRUCache
import org.wordpress.android.fluxc.store.WCProductStore
import javax.inject.Inject
Expand All @@ -14,7 +15,7 @@ class WooPosSearchByIdentifierVariationFetch @Inject constructor(
private val errorMapper: WooPosSearchByIdentifierProductErrorMapper
) {
sealed class VariationFetchResult {
data class Success(val variation: ProductVariation) : VariationFetchResult()
data class Success(val variation: WooPosVariation) : VariationFetchResult()
data class Failure(val error: WooPosSearchByIdentifierResult.Error) : VariationFetchResult()
}

Expand All @@ -34,7 +35,7 @@ class WooPosSearchByIdentifierVariationFetch @Inject constructor(
selectedSite.get(),
parentId,
variationId
)?.toAppModel()
)?.toAppModel()?.toWooPosVariation()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Seems like we are mapping it to appModel and then to POS model - can we skip the first mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in abba849


return if (variation != null) {
variationsCache.add(parentId, variation)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ import androidx.lifecycle.map
import androidx.lifecycle.viewModelScope
import com.woocommerce.android.R
import com.woocommerce.android.model.Product
import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.ui.woopos.common.composeui.modifier.BarcodeInputDetector
import com.woocommerce.android.ui.woopos.common.data.WooPosGetCouponById
import com.woocommerce.android.ui.woopos.common.data.WooPosGetProductById
import com.woocommerce.android.ui.woopos.common.data.WooPosGetVariationById
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import com.woocommerce.android.ui.woopos.common.data.getNameForPOS
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosSearchByIdentifier
import com.woocommerce.android.ui.woopos.common.data.searchbyidentifier.WooPosSearchByIdentifierResult
import com.woocommerce.android.ui.woopos.common.util.WooPosLogWrapper
Expand All @@ -28,7 +29,6 @@ import com.woocommerce.android.ui.woopos.home.cart.WooPosCartStatus.CHECKOUT
import com.woocommerce.android.ui.woopos.home.cart.WooPosCartStatus.EDITABLE
import com.woocommerce.android.ui.woopos.home.cart.WooPosCartStatus.EMPTY
import com.woocommerce.android.ui.woopos.home.items.WooPosItemsViewModel
import com.woocommerce.android.ui.woopos.home.items.variations.getNameForPOS
import com.woocommerce.android.ui.woopos.util.WooPosGetCachedStoreCurrency
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.BackToCartTapped
Expand Down Expand Up @@ -589,7 +589,7 @@ class WooPosCartViewModel @Inject constructor(
imageUrl = firstImageUrl,
)

private suspend fun ProductVariation.toCartListItem(
private suspend fun WooPosVariation.toCartListItem(
itemNumber: Int,
product: Product
): WooPosCartItemViewState.Product.Variation =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package com.woocommerce.android.ui.woopos.home.items.variations

import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.ui.products.variations.selector.VariationListHandler
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import com.woocommerce.android.ui.woopos.common.data.WooPosVariationsTypesFilterConfig
import com.woocommerce.android.ui.woopos.common.data.toWooPosVariation
import com.woocommerce.android.util.WooLog
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.Flow
Expand All @@ -20,11 +22,11 @@ class WooPosVariationsDataSource @Inject constructor(
private val variationCache: WooPosVariationsLRUCache,
private val variationFilterConfig: WooPosVariationsTypesFilterConfig
) {
private suspend fun getCachedVariations(productId: Long): List<ProductVariation> {
private suspend fun getCachedVariations(productId: Long): List<WooPosVariation> {
return variationCache.get(productId) ?: emptyList()
}

private suspend fun updateCache(productId: Long, variations: List<ProductVariation>) {
private suspend fun updateCache(productId: Long, variations: List<WooPosVariation>) {
variationCache.put(productId, variations)
}

Expand Down Expand Up @@ -55,7 +57,9 @@ class WooPosVariationsDataSource @Inject constructor(
filterOptions = variationFilterConfig.filters
)
if (result.isSuccess) {
val remoteVariations = handler.getVariationsFlow(productId).firstOrNull()?.applyFilter() ?: emptyList()
val remoteVariations = handler.getVariationsFlow(productId).firstOrNull()?.applyFilter()?.map {
it.toWooPosVariation()
} ?: emptyList()
updateCache(productId, remoteVariations)
emit(FetchResult.Remote(Result.success(remoteVariations)))
} else {
Expand All @@ -69,13 +73,15 @@ class WooPosVariationsDataSource @Inject constructor(
}
}.flowOn(Dispatchers.IO)

suspend fun loadMore(productId: Long): Result<List<ProductVariation>> = withContext(Dispatchers.IO) {
suspend fun loadMore(productId: Long): Result<List<WooPosVariation>> = withContext(Dispatchers.IO) {
val result = handler.loadMore(
productId,
filterOptions = variationFilterConfig.filters
)
if (result.isSuccess) {
val fetchedVariations = handler.getVariationsFlow(productId).first().applyFilter()
val fetchedVariations = handler.getVariationsFlow(
productId
).first().applyFilter().map { it.toWooPosVariation() }
Result.success(fetchedVariations)
} else {
result.logFailure()
Expand All @@ -93,8 +99,8 @@ private fun Result<Unit>.logFailure() {
}

sealed class FetchResult {
data class Cached(val data: List<ProductVariation>) : FetchResult()
data class Remote(val result: Result<List<ProductVariation>>) : FetchResult()
data class Cached(val data: List<WooPosVariation>) : FetchResult()
data class Remote(val result: Result<List<WooPosVariation>>) : FetchResult()
}

private fun List<ProductVariation>.applyFilter(): List<ProductVariation> {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package com.woocommerce.android.ui.woopos.home.items.variations

import android.util.LruCache
import com.woocommerce.android.model.ProductVariation
import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import javax.inject.Inject
Expand All @@ -13,22 +13,22 @@ class WooPosVariationsLRUCache @Inject constructor() {
private const val VARIATION_CACHE_MAX_SIZE = 50
}

private val cache = LruCache<Long, List<ProductVariation>>(VARIATION_CACHE_MAX_SIZE)
private val cache = LruCache<Long, List<WooPosVariation>>(VARIATION_CACHE_MAX_SIZE)
private val mutex = Mutex()

suspend fun get(key: Long): List<ProductVariation>? {
suspend fun get(key: Long): List<WooPosVariation>? {
return mutex.withLock {
cache.get(key)
}
}

suspend fun put(key: Long, value: List<ProductVariation>) {
suspend fun put(key: Long, value: List<WooPosVariation>) {
mutex.withLock {
cache.put(key, value)
}
}

suspend fun add(key: Long, value: ProductVariation) {
suspend fun add(key: Long, value: WooPosVariation) {
mutex.withLock {
val list = cache.get(key)
if (list != null) {
Expand All @@ -48,7 +48,7 @@ class WooPosVariationsLRUCache @Inject constructor() {
}
}

suspend fun getAll(): List<ProductVariation> {
suspend fun getAll(): List<WooPosVariation> {
return mutex.withLock {
cache.snapshot().values.flatten()
}
Expand Down
Loading