[Woo POS][Non-Simple Products] Add downloadable products filter from API and remove client side filtering #13196
Conversation
AnirudhBhat
added
type: task
AnirudhBhat
changed the base branch from
trunk
to
issue/13189-add-tests-for-variation-data-source
Project dependencies changesThe following changes in project dependencies were detected (configuration listtree-+--- org.wordpress:fluxc:2.101.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
-| | \--- javax.inject:javax.inject:1
-| +--- 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.101.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-b329e031481c8653a57c0a41c4dedfc0ba6a510e
+| +--- 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
+| | \--- javax.inject:javax.inject:1
+| +--- 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-b329e031481c8653a57c0a41c4dedfc0ba6a510e
+| +--- 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.101.0
- +--- org.wordpress:fluxc:2.101.0 (*)
- +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
- +--- com.google.dagger:dagger:2.51.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.101.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-b329e031481c8653a57c0a41c4dedfc0ba6a510e
+ +--- org.wordpress:fluxc:trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e (*)
+ +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+ +--- com.google.dagger:dagger:2.51.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-b329e031481c8653a57c0a41c4dedfc0ba6a510e
+ +--- androidx.room:room-ktx:2.6.1 (*)
+ \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*) |
📲 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #13196 +/- ##
=========================================
Coverage 40.59% 40.59%
+ Complexity 6368 6364 -4
=========================================
Files 1345 1345
Lines 77238 77238
Branches 10603 10601 -2
=========================================
Hits 31353 31353
Misses 43128 43128
Partials 2757 2757 ☔ View full report in Codecov by Sentry. |
| filters = mapOf(WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value) | ||
| filters = mapOf( | ||
| WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value, | ||
| WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(), |
There was a problem hiding this comment.
I am thinking if DownloadableOptions actually gives us something here:
- filter typed as
<ProductFilterOption, String> DownloadableOptionsenum has no connections toProductFilterOption, so it doesn't define the API anyhow
It feels like it either should be something changed in the filter type, or we rather pass false/true as strings
Wdyt?
There was a problem hiding this comment.
I see your point. My train of thought was that DownloadableOptions enforces type safety and avoids potential typos (like "flase" or "treu") in the future. Additionally, by using DownloadableOptions, developers are naturally guided toward the enum whenever changes are needed, reducing the risk of inconsistent values.
I do understand this adds a slight abstraction, but I believe the trade-off is worthwhile for the benefits of safety and maintainability. WDYT?
There was a problem hiding this comment.
The thing is that it doesn't enforce anything. It's the string type here. If you want actually to implement type safety, then the type should not be a string but some ProductFilterOptionFilterValue or something like that
Currently, it feels like just an enum for the sake of an enum, and there is no way anyone can figure out/find that they can/should use it as a key to the filter. So to me, it feels that it is either just a string or an actual type should be here
Wdyt?
There was a problem hiding this comment.
I see your point about the current implementation not fully enforcing type safety since the filter type is <ProductFilterOption, String>. You’re right that a more robust approach would involve introducing something like a ProductFilterOptionFilterValue type.
That said, even though DownloadableOptions doesn’t enforce type safety at the API level, I believe it still adds value by reducing human errors like typos ("flase" instead of "false") when setting the filter key. The enum serves as a centralised, predefined set of valid values that developers can rely on.
Additionally, the presence of DownloadableOptions makes it clear to anyone working with the code that there is a defined set of options for the DOWNLOADABLE key. I believe this will be helpful as developers will know to refer to the enum rather than hardcoding strings.
I understand the concern about discoverability. We could potentially improve this by better documenting its usage by adding a comment.
There was a problem hiding this comment.
Hey @AnirudhBhat
That said, even though DownloadableOptions doesn’t enforce type safety at the API level, I believe it still adds value by reducing human errors like typos ("flase" instead of "false") when setting the filter key. The enum serves as a centralized, predefined set of valid values that developers can rely on.
I believe others might not realize that there is an enum, especially since it's not even meant to be used as an enum but rather as a substitute for a String constant, creating more confusion.
I understand the concern about discoverability. We could potentially improve this by better documenting its usage by adding a comment.
That’s maybe another problem on its own 😀 Rather than providing the right solution, adding a comment that may become outdated or incorrect and ultimately create further issues
In any case, I think it's really a small thing, and not worth further discussion
There was a problem hiding this comment.
The code looks good! I left one suggestion though
What I am not sure if the API actually works. It returns me downloadable items in the response. Is this something available from a specific version of WooCore? If it's then I guess we somehow should it handle or...?
Sorry I forgot to mention this in the PR. Yes, the changes in the API are available only from WooCommerce 9.6.0 or from the nightly build (for testing). We are thinking to add a minimum WooCommerce version as a requirement for POS users. |
There was a problem hiding this comment.
I validated that the API works 👍
Do you think we can merge it before uping the minimum supported version? My understanding that we consider that the POS is available to the users, so we must make sure that the min version enforced before this change then, correct?
We’re still considering whether to make this a hard requirement or to implement checks within the app to allow users with WooCommerce versions < 9.6.0 to use POS with only simple products. Thinking more about this, I believe enforcing the check now is the better approach. This ensures compatibility and avoids potential issues, and we can always revisit and adjust this decision later if needed. |
NOTE: Please review and merge this Fluxc PR before reviewing this one. Also, please update the fluxc hash in this PR before merging this PR. Thanks!
Closes: #13195
Description
This PR adds downloadable filtering for products on POS from API instead of client side filtering. The changes remain unaffected for store management.
Steps to reproduce
POS Mode
downloadable=falseis being sent in POS mode for products endpoint.Store management mode
The tests that have been performed
Tested in both Store management and POS mode to verify downloadable filter works as expected.
Images/gif
downloadable_product.mp4
RELEASE-NOTES.txtif 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: