[WellSQL Migration] Migrate WCProductVariationModel to Room
#14027
Conversation
`:libs:fluxc-plugin:assembleDebug` passes
…e to app model mapper, make query methods suspendable
To reduce chance of similar mistake in the future, I propose using `LocalId/RemoteId` wrappers.
To fix lint task
2 tests are failing right now
…ven` Room and WellSql have different defaults for sort. In this test, the order doesn't matter as all items have `menuOrder=0`
Don't use incorrect `$original.id` for description: instead, use simple string to avoid any confusion. Close database on test end
…rn default values Previously, functions like `VariationRepository#getProductVariationList` or `ProductDetailRepository#getCachedVariationCount` were *not* suspend. This allowed Mockito to return default values for both of the methods, respectively: empty list and 0. After recent changed, both of this functions were refactored to suspend, which caused Mockito to return `null` for them (see: mockito/mockito-kotlin#342) This comment brings back the previous behavior by mocking mentioned methods to return default values.
…_variation_to_room
…_variation_to_room_tests
…to_room [1/2] `WCProductVariationModel` to Room: working app
…to_room_tests [2/2] `WCProductVariationModel` to Room: unit tests adjustments
…to feature/product_variation_to_room
ParaskP7
added
status: do not merge
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the WCProductVariationModel to use Room for persistence and updates related DAOs, database configuration, model mapping, and tests.
- Introduces a new Room DAO (ProductVariationsDao) and updates WCAndroidDatabase to include WCProductVariationModel.
- Refactors model creation and mapping methods to use Kotlin’s data class immutability via copy.
- Updates tests and dependency injections accordingly to support the new Room-based implementation.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/ProductVariationsDao.kt | New Room DAO for WCProductVariationModel providing CRUD operations. |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt | Updates database version and schema to include WCProductVariationModel. |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCProductVariationModel.kt | Migrates the variation model to a Room entity using immutable data class copy. |
| Various files in network, repository, and tests | Updates existing mapping, repository functions, and tests to work with the new immutable model. |
Comments suppressed due to low confidence (2)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailCardBuilder.kt:522
- Using runBlocking here can block the thread and potentially affect UI responsiveness. Consider refactoring this code to employ a fully suspendable or asynchronous approach to avoid potential deadlocks.
runBlocking { variationRepository.getProductVariationList(product.remoteId).all { ... } }
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/products/ProductTestUtils.kt:142
- [nitpick] Ensure that the toAppModel() conversion retains all required fields and that the hardcoded currency value is appropriate across different locales. Review the conversion logic for consistency with updated Room model semantics.
return WCProductVariationModel(2).copy(...).toAppModel().also { it.priceWithCurrency = "$10.00" }
📲 You can test the changes from this Pull Request in WooCommerce-Wear 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 #14027 +/- ##
============================================
+ Coverage 38.32% 38.34% +0.02%
- Complexity 9546 9566 +20
============================================
Files 2123 2124 +1
Lines 116926 116874 -52
Branches 15009 15003 -6
============================================
+ Hits 44810 44817 +7
+ Misses 68004 67944 -60
- Partials 4112 4113 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
It looks like as part of 1df49aa an oversight occurred where, instead of 'result.remoteProductId', 'payload.remoteProductId' got used. I am not sure if that was deliberate or not, and since there was no specific comment about that, it is better to treat that as an oversight. This commit serves as an explicit nudge to reviewers, and the original auhtor, to verify this change with that extra 'fix' commit.
This follows the same pattern applied to 'WCProductModel' and it seems that this pattern is the one mostly used. This pattern is similar to using a 'value' class to create a more domain-specific type instead of primitive types to help with data class construction clarity. Also, related to: 316ef5f
…to feature/product_variation_to_room
ParaskP7
removed
the
status: do not merge
|
👋 @malinajirka , I am adding you as an extra reviewer to this, additional to myself, me taking this to the finish line for @wzieba (who is currently AFK). I am doing that because I want someone from the product team to do a final review on this, test this as thoroughly as possible and signing this change for merging. With that, I want to give you some extra hints on where I would love you to put some extra focus:
💭 Last but not least, and maybe more importantly, I wanted to clarify the behavior of this migration. I want to clarify that with this kind of migrations we are okay dropping an existing table (wp-fluxc.WCProductVariationModel), along with its entries, and recreating the table (wc-android-database.ProductVariationEntity), but without any entries (empty). With such a migration, all previous table entries will be gone and added back (partially or in full) only when a user navigates to that specific screen.
Once more, thank you so much in advance for all the help and insights on this! 🙏 |
|
Thanks for working on this @ParaskP7! I'll also invite @hichamboushaba and @kidinov since they reviewed the previous PRs so likely know what to look for. Moreover, I'm wrapping up a project with a target date set to next friday and I'm a bit behind, so it might take me a bit before I find time for this review (I'd estimate it to Wednesday the earliest). |
|
Thanks for all the help @malinajirka , the heads-up on this and for inviting others to review this, much appreciated! 🙇 |
…to feature/product_variation_to_room
|
👋 team (@malinajirka @hichamboushaba @kidinov), just a friendly ping as a reminder on this, thank you! 🙏 |
I think this change is fine, but just to double-check my understanding. I believe the remoteProductId between payload and result is always identical, right? Or have you fixed it based on some bug you discovered?
Thanks, that's a nice improvement! The wrappers were introduced as part of the introduction of List Management component in WPAndroid (for posts) and later in WCAndroid (for orders). I'm not aware of any guidelines/best-practices regarding the usage - for models that are using the list management component, this is required, for other models, it's nice to have.
I can confirm the byteBuddy exception is happening on my side as well 🤔. Having said that, it's happening even when I run the command on
Editing products and variations is supported in the app, but we do not support offline => the user needs to explicitly save the changes while being online => AFAIK it's fine to recreate the table. I have tested various flows in the app (variable product creation, editing, order creation with variations in the cart, order detail with variations in the cart, POS) and it all seems to be working as expected. I also tested "migration" - dropping the database after already having some data in there, and I also didn't discover any issues. (I discovered an unrelated issue on Attributes subscreen and reported it to Linear) Great job Wojtek and Petros! Thank you for the super clear description and heads-up about changes that might have a higher potential of introducing regressions! 🎖️ 🙇 |
|
Awesome, thanks for this review and all the extra context you provided as part of it @malinajirka , much appreciated! 🙇 ❤️ 🚀
About the FYI: I (indeed) didn't discover any bug.
Interesting, good to know, didn't have (or forgot) this knowledge about the list management component and all. 🔖
Yea, I have the feeling this all started when #13975 merged to
Good to know! 👍 Btw, of all the "legacy" WellSQL tables we still have to migrate, do you maybe have any idea if any such table would require a more careful migration, like migrating its
Thanks for all the testing! 🙇 🧪 🎉
❤️ |
…to feature/product_variation_to_room
|
👋 @wzieba and just an FYI that I just updated this branch with the latest change from Now that you're back, let me know if you want to take one last time at the changes before merging, or want to take it over from here. 🙏 |
AFAIK, all our tables are cache only, so they should all be safe to drop - I asked for double confirmation here p1747214664229459-slack-C6H8C3G23.
Ohh, I see 🤔. |
Good to know @malinajirka , thanks once more! 🥇 |
All fine on my end, thank you for applying improvements! |
Noted @malinajirka , thanks! 🔖 PS: In the meanwhile, if you think of any other tables that could potentially cause troubles, let us know! 🙏 |
Closes: AINFRA-124
FYI: This is a feature branch of two PRs (context):
WCProductVariationModelto Room: working app #13972WCProductVariationModelto Room: unit tests adjustments #13982Description
This PR migrates
WCProductVariationModelto Room.FYI: For more info see this PR's description as well.
Testing information
Smoke test any feature that uses
WCProductVariationModel.FYI: For more info see this PR's testing information as well.
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: