-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Woo POS][Non-Simple Products] Handle empty state on variation screen #13209
Conversation
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13209 +/- ##
============================================
- Coverage 40.80% 40.79% -0.02%
- Complexity 6411 6415 +4
============================================
Files 1353 1353
Lines 77679 77733 +54
Branches 10692 10696 +4
============================================
+ Hits 31698 31709 +11
- Misses 43172 43215 +43
Partials 2809 2809 ☔ View full report in Codecov by Sentry. |
…ull to be shown as well
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
@@ -89,5 +89,5 @@ sealed class FetchResult<out T> { | |||
} | |||
|
|||
private fun List<ProductVariation>.applyFilter(): List<ProductVariation> { | |||
return filter { it.price != null && !it.isDownloadable } | |||
return filter { it.price != null && !it.isDownloadable && it.isPurchasable } |
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.
👋 @AnirudhBhat, during engineering sync call today, @malinajirka mentioned we should not rely on isPurchasable
field. Wdut?
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.
For context, and future reference:
- Purchasable for simple products and variations means not free, and published
- Grouped products are never purchasable – when we start supporting them in the POS, we'll want to show them.
Bottom line is, that it might have different meanings for different types, and it doesn't say whether the product should or shouldn't be displayed in the POS.
Based on the discussion here p2-pdfdoF-62l#comment-7224, we decided to show free products, so right now that's the main reason, we likely want to update this PR and remove the purchasable check.
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 bringing this up, @samiuelson, and for the clarification, @malinajirka. I’ve been using this flag primarily to filter out variations that are created but not enabled. It seems the isVisible
flag already provides information on whether a variation is enabled and should be displayed.
Given this, I suggest we ask the backend team to consider including this flag in the variations endpoint. This would allow us to eliminate client-side filtering for this purpose. What do you think?
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.
Sounds good to me, let's try to remove all client side filtering.
# Conflicts: # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsDataSourceTest.kt # gradle/libs.versions.toml
Project dependencies changesThe following changes in project dependencies were detected (configuration list
tree-+--- org.wordpress:fluxc:2.102.0
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25 -> 2.0.21
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
-| | \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:2.0.21
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
-| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.8.1 (*)
-| | +--- com.google.crypto.tink:tink-android:1.5.0
-| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
-| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
-| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
-| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-| +--- org.apache.commons:commons-text:1.10.0 (*)
-| +--- androidx.room:room-runtime:2.6.1 (*)
-| +--- androidx.room:room-ktx:2.6.1
-| | +--- androidx.room:room-common:2.6.1 (*)
-| | +--- androidx.room:room-runtime:2.6.1 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 2.0.21 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
-| | +--- androidx.room:room-common:2.6.1 (c)
-| | \--- androidx.room:room-runtime:2.6.1 (c)
-| +--- com.google.dagger:dagger:2.51.1 -> 2.53.1
-| | +--- jakarta.inject:jakarta.inject-api:2.0.1
-| | +--- javax.inject:javax.inject:1
-| | \--- org.jspecify:jspecify:1.0.0
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
-| +--- org.wordpress:wellsql:2.0.0
-| | +--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
-| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-| +--- org.wordpress.fluxc:fluxc-annotations:2.102.0
-| +--- org.greenrobot:eventbus:3.3.1 (*)
-| +--- org.greenrobot:eventbus-java:3.3.1
-| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| +--- com.android.volley:volley:1.1.1 -> 1.2.0
-| +--- androidx.paging:paging-runtime:2.1.2
-| | +--- androidx.paging:paging-common:2.1.2
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.8.1 (*)
-| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.8.7 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.8.7 (*)
-| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
-| +--- com.goterl:lazysodium-android:5.0.2
-| +--- net.java.dev.jna:jna:5.5.0
-| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*)
++--- org.wordpress:fluxc:trunk-0264533ce612f74e1ae6fcbaefeb69b252163774
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25 -> 2.0.21
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
+| | \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:2.0.21
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
+| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
+| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
+| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+| | +--- androidx.annotation:annotation:1.1.0 -> 1.8.1 (*)
+| | +--- com.google.crypto.tink:tink-android:1.5.0
+| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
+| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
+| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
+| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+| +--- org.apache.commons:commons-text:1.10.0 (*)
+| +--- androidx.room:room-runtime:2.6.1 (*)
+| +--- androidx.room:room-ktx:2.6.1
+| | +--- androidx.room:room-common:2.6.1 (*)
+| | +--- androidx.room:room-runtime:2.6.1 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 2.0.21 (*)
+| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
+| | +--- androidx.room:room-common:2.6.1 (c)
+| | \--- androidx.room:room-runtime:2.6.1 (c)
+| +--- com.google.dagger:dagger:2.51.1 -> 2.53.1
+| | +--- jakarta.inject:jakarta.inject-api:2.0.1
+| | +--- javax.inject:javax.inject:1
+| | \--- org.jspecify:jspecify:1.0.0
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+| +--- org.wordpress:wellsql:2.0.0
+| | +--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
+| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+| +--- org.wordpress.fluxc:fluxc-annotations:trunk-0264533ce612f74e1ae6fcbaefeb69b252163774
+| +--- org.greenrobot:eventbus:3.3.1 (*)
+| +--- org.greenrobot:eventbus-java:3.3.1
+| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| +--- com.android.volley:volley:1.1.1 -> 1.2.0
+| +--- androidx.paging:paging-runtime:2.1.2
+| | +--- androidx.paging:paging-common:2.1.2
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.8.1 (*)
+| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.8.7 (*)
+| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.8.7 (*)
+| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
+| +--- com.goterl:lazysodium-android:5.0.2
+| +--- net.java.dev.jna:jna:5.5.0
+| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:2.102.0
- +--- org.wordpress:fluxc:2.102.0 (*)
- +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
- +--- com.google.dagger:dagger:2.51.1 -> 2.53.1 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
- +--- androidx.room:room-runtime:2.6.1 (*)
- +--- org.wordpress:wellsql:2.0.0 (*)
- +--- org.wordpress.fluxc:fluxc-annotations:2.102.0
- +--- androidx.room:room-ktx:2.6.1 (*)
- \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:trunk-0264533ce612f74e1ae6fcbaefeb69b252163774
+ +--- org.wordpress:fluxc:trunk-0264533ce612f74e1ae6fcbaefeb69b252163774 (*)
+ +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+ +--- com.google.dagger:dagger:2.51.1 -> 2.53.1 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+ +--- androidx.room:room-runtime:2.6.1 (*)
+ +--- org.wordpress:wellsql:2.0.0 (*)
+ +--- org.wordpress.fluxc:fluxc-annotations:trunk-0264533ce612f74e1ae6fcbaefeb69b252163774
+ +--- androidx.room:room-ktx:2.6.1 (*)
+ \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*) |
Version |
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!
# Conflicts: # gradle/libs.versions.toml
Please review this fluxc PR before merging this PR and also ensure to update the fluxc has before merging this PR
Closes: #13206
Description
This PR
Decision: pdfdoF-62l-p2#comment-7224
Steps to reproduce
Variable products with no variations published
Variable products with no variations
The tests that have been performed
Tested the above scenario by disabling variations both from the app and from the admin page.
Images/gif
variations_empty_state.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: