-
Notifications
You must be signed in to change notification settings - Fork 136
[Products] Extract subscriptions outside of the Product DB table - Part 1 #12766
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
Conversation
6f3c9f5 to
16587ff
Compare
Generated by 🚫 Danger |
Project dependencies changesThe following changes in project dependencies were detected (configuration listtree +--- androidx.datastore:datastore-preferences:1.1.0
| \--- androidx.datastore:datastore-preferences-android:1.1.0
| \--- androidx.datastore:datastore:1.1.0
| \--- androidx.datastore:datastore-android:1.1.0
| \--- androidx.datastore:datastore-core:1.1.0
| \--- androidx.datastore:datastore-core-android:1.1.0
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25
-| +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-| \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.25
-| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.24
+| +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.24
+| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.24 -> 1.9.25 (*)
-+--- org.wordpress:fluxc:2.99.0
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
-| | +--- 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 -> 1.9.25 (*)
-| | +--- 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.0 (*)
-| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-| +--- org.wordpress.fluxc:fluxc-annotations:2.99.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.0 (*)
-| | | \--- 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.7.0 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.7.0 (*)
-| | \--- 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 (*)
++--- org.wordpress:fluxc:trunk-7475ad2070b90ab63276656225890fd312188011 FAILED
-+--- org.wordpress.fluxc.plugins:woocommerce:2.99.0
-| +--- org.wordpress:fluxc:2.99.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.99.0
-| +--- androidx.room:room-ktx:2.6.1 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
++--- org.wordpress.fluxc.plugins:woocommerce:trunk-7475ad2070b90ab63276656225890fd312188011 FAILED
+--- org.wordpress:login:1.18.0
| +--- com.github.bumptech.glide:glide:4.12.0 -> 4.16.0
-| | \--- androidx.exifinterface:exifinterface:1.3.6 (*)
+| | \--- androidx.exifinterface:exifinterface:1.3.6
+| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-| \--- com.google.dagger:dagger:2.47 -> 2.51.1 (*)
+| \--- com.google.dagger:dagger:2.47 -> 2.51.1
+| \--- javax.inject:javax.inject:1
+--- project :libs:cardreader
| +--- com.stripe:stripeterminal-localmobile:3.7.1
-| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
| | +--- com.stripe:stripeterminal-external:3.7.1
-| | | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| | | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
| | | \--- com.stripe:stripeterminal-internal-models:3.7.1
-| | | \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| | | \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
| | \--- com.stripe:stripeterminal-internal-common:3.7.1
-| | \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| | \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
| +--- com.stripe:stripeterminal-core:3.7.1
-| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
-| | +--- androidx.security:security-crypto:1.1.0-alpha03 (*)
+| | +--- androidx.security:security-crypto:1.1.0-alpha03
+| | | +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
+| | | +--- com.google.crypto.tink:tink-android:1.5.0
+| | | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
-| | \--- androidx.room:room-ktx: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 -> 1.9.25 (*)
+| | +--- 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)
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.24 (*)
+--- com.automattic:about:0.0.6
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.5.31 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.5.31 -> 1.9.24 (*)
+--- org.wordpress:mediapicker:0.3.1
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
| \--- org.wordpress.mediapicker:domain:0.3.1
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
+--- org.wordpress.mediapicker:source-wordpress:0.3.1
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.24 (*)
+--- io.coil-kt:coil-compose:2.1.0
| \--- io.coil-kt:coil-compose-base:2.1.0
| \--- io.coil-kt:coil-base:2.1.0
-| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.9.25 (*)
+| \--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.9.24 (*)
-\--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.25 (*)
+\--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.22 -> 1.9.24 (*) |
16587ff to
bd5764f
Compare
bd5764f to
9eafa13
Compare
📲 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.
|
e5e9332 to
06f0775
Compare
056fa19 to
d5c3475
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #12766 +/- ##
============================================
+ Coverage 40.83% 40.84% +0.01%
- Complexity 5739 5753 +14
============================================
Files 1236 1237 +1
Lines 69703 69725 +22
Branches 9668 9673 +5
============================================
+ Hits 28460 28479 +19
- Misses 38618 38621 +3
Partials 2625 2625 ☔ View full report in Codecov by Sentry. |
JorgeMucientes
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.
Excellent work @hichamboushaba the code changes look good and everything I tested worked well, including updating from older version of the app to a new one with subscription products loaded into the DB.
| val remoteId: Long | ||
| get() = product.remoteId | ||
|
|
||
| val hasShipping: Boolean |
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've noticed a bug around this. But i could confirm is not related to the changes from this PR so I created a GH issue to address it.
# Conflicts: # build.gradle
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Part of: #12752
Depends on wordpress-mobile/WordPress-FluxC-Android#3106
Note: I want to apologize first for the size of the PR, I tried to split the work, but it's not straightforward to get a compiling working state without changing all parts at once.
I think the code review could still be straightforward, half of the changes are in the
ProductDetailCardBuilderclass, and most changes there are just because we started usingProductAggregateinstead ofProduct, so a quick skim through the file would be enough.Description
This PR is the first part in a refactor to align with the above FluxC changes that removed the
METADATAcolumn from the products table, it adds the needed changes to load the subscription details from the MetaData table, and shows them in the product details.Saving is not handled yet, it will be added in the next PR.
Before working on this, I looked into the different options to approach it, and I considered two options:
subscriptionproperty inside the modelProduct, this would have required to create a special mapper that aggregates the data from both tables (WCProductModelandMetaDataEntity), this mapper would replace the extension functiontoAppModel.subscriptionproperty from the modelProductand use a separate class that aggregates the product with the subscription details.I went with option 2, while both approaches have their pros and cons, I think approach 1 has bigger downsides:
This PR implements the approach 2.
Steps to reproduce
Testing information
The tests that have been performed
^
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:
- The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
- Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
- Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.
t