[1/2] WCProductVariationModel to Room: working app
#13972
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`:libs:fluxc-plugin:assembleDebug` passes
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
Base automatically changed from
migrate_tests_to_products_room
to
products_to_room
April 25, 2025 08:16
…e to app model mapper, make query methods suspendable
To reduce chance of similar mistake in the future, I propose using `LocalId/RemoteId` wrappers.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
To fix lint task
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the WCProductVariationModel implementation to use Room for local data persistence and updates related DAO and store methods. Key changes include:
- Replacing legacy WellSql operations with Room DAO methods and converting model instantiations from mutable apply blocks to immutable copy constructors.
- Updating several functions to suspend functions and incrementing the database version to accommodate the WCProductVariationModel entity.
- Removing obsolete ProductSqlUtils variation methods and updating network mappers and repositories to align with the new model.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/fluxc-plugin/src/testFixtures/kotlin/org/wordpress/android/fluxc/wc/product/ProductTestUtils.kt | Replaced legacy model instantiation with constructor parameters. |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt | Updated variation access methods to suspend functions, replacing legacy SQL utils with DAO calls. |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/ProductVariationsDao.kt | Introduced a new Room DAO for product variations. |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt | Added WCProductVariationModel as an entity and incremented the database version. |
| Multiple network and repository files | Updated model instantiation and copy patterns to ensure immutability and consistency. |
Comments suppressed due to low confidence (2)
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt:79
- Ensure that the migration from database version 39 to 40 is thoroughly tested, especially with regard to the new WCProductVariationModel schema and data integrity.
const val WC_DATABASE_VERSION = 40
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt:828
- Confirm that all call sites for variation-related methods are updated to handle the new suspend signatures to prevent unintended blocking or concurrency issues.
suspend fun getVariationByRemoteId(
...erce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailCardBuilder.kt
Show resolved
Hide resolved
wzieba
commented
Apr 28, 2025
Comment on lines
+153
to
+163
| fun attributesToJson(): String { | ||
| val jsonArray = JsonArray() | ||
| attributes.forEach { variantOption -> | ||
| JsonObject().apply { | ||
| addProperty("id", variantOption.id) | ||
| addProperty("name", variantOption.name) | ||
| addProperty("option", variantOption.option) | ||
| }.also { jsonArray.add(it) } | ||
| } | ||
| return jsonArray.toString() | ||
| } |
There was a problem hiding this comment.
this replaces addVariant method. I like it doesn't need gson object (we shouldn't use gson inside models, nor create instance of gson per model), but we also loose synchronization of fields with ProductVariantOption. I think it's acceptable, because I think it's unlikely this model will change.
wzieba
commented
Apr 28, 2025
| } | ||
| @Entity( | ||
| tableName = "ProductVariationEntity", | ||
| primaryKeys = ["localSiteId", "remoteProductId", "remoteVariationId"], |
There was a problem hiding this comment.
This primaryKeys introduces a new behavior of UNIQUE constraint, which wasn't present before, yet I think it's expected.
In case when `jacocoTestReport` task fails, our script was not moving test results to the correct directory, as it was exiting on error before collection.
…erage only on success tests This will reduce chances of `testJalapenoDebugUnitTest` task being not up to date when running `jacocoTestReport`, which caused running unit tests again, without any reason.
To address issues like: Reason: Task ':jacocoTestReport' uses this output of task ':libs:apifaker:testDebugUnitTest' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
There are two reasons for this: - with the current setup, we were opening "Copying test logs for test collector" if tests failed, because this was the last collapsed regular group (docs: https://buildkite.com/docs/pipelines/configure/managing-log-output#collapsing-output) and it was confusing: tests did fail, not copying test logs - developers are much more interested in details of `Testing` step than (empty) details of "Copying test logs" so it makes sense to open former by default
wzieba
changed the title
WCProductVariationModel to Room: working appWCProductVariationModel to Room: working app
4 tasks
…coco_fail Fix Buildkite test collector support when tests execution fail
…_variation_to_room
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WCProductVariationModelto Room: working app #13972WCProductVariationModelto Room: unit tests adjustments #13982Description
This PR migrates
WCProductVariationModelfrom WellSql to Room. It's similar to #13935, but this time the migration has smaller scope, asWCProductVariationModelis not used in that many places.+343/-396, excluding JSON schema fileWCProductVariationModelto a data class with immutable membersTesting information
Please test using #13982
Images/gif
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: