-
Notifications
You must be signed in to change notification settings - Fork 134
WCProductTagModel
to Room
#14003
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
WCProductTagModel
to Room
#14003
Conversation
Introduce ProductTagsDao, replace SELECT queries with the new Dao.
…fluxc-plugin:assembleDebug` successful
…biDebug` task pass
…ProductSqlUtilsTest there
`:libs:fluxc-plugin:testDebugUnitTest` 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.
|
📲 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 #14003 +/- ##
============================================
+ Coverage 38.43% 38.44% +0.01%
Complexity 9606 9606
============================================
Files 2126 2127 +1
Lines 116952 116916 -36
Branches 15027 15028 +1
============================================
+ Hits 44952 44953 +1
+ Misses 67882 67846 -36
+ Partials 4118 4117 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR migrates the WCProductTagModel from a legacy WellSql-based implementation to a modern Room-based architecture. Key changes include converting WCProductTagModel into a Room @entity data class, adding a ProductTagsDao with associated database updates and tests, and updating repository and store methods to use suspend functions.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
libs/fluxc-plugin/src/testFixtures/kotlin/org/wordpress/android/fluxc/wc/product/ProductTestUtils.kt | Updated test utility to use the WCProductTagModel constructor. |
libs/fluxc-plugin/src/test/java/org/wordpress/android/fluxc/leaderboards/* | Updated tests to include productTagsDao where needed. |
libs/fluxc-plugin/src/test/java/org/wordpress/android/fluxc/persistence/dao/ProductTagsDaoTest.kt | Added new tests for the ProductTagsDao. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt | Refactored product tag methods to use DAO methods with suspend functions. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt | Updated the database version and added the productTagsDao. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCProductTagModel.kt | Converted to a data class with Room annotations. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/products/tags/ProductTagsRepository.kt | Adjusted repository methods to suspend functions. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailViewModel.kt | Updated to use suspend functions; however, a runBlocking call was introduced. |
@@ -2404,7 +2405,7 @@ class ProductDetailViewModel @Inject constructor( | |||
fun onProductTagAdded(tagName: String) { | |||
// verify if the entered tagName exists for the site | |||
// It so, the tag should be added to the product directly | |||
productTagsRepository.getProductTagByName(tagName)?.let { | |||
runBlocking { productTagsRepository.getProductTagByName(tagName) }?.let { |
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.
Using runBlocking in a ViewModel's UI method may block the main thread. Consider launching a coroutine within the ViewModel's scope instead of using runBlocking.
Copilot uses AI. Check for mistakes.
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.
In previous scenarios and other migrations, I leaned over persisting this behavior (even if it meant blocking a thread), as I didn't want to introduce yet another variable to the migration process. But in this case, I think it's indeed quite safe. I tested it and it looks fine. Suggestion addressed in: befc13c
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCProductTagModel.kt
Show resolved
Hide resolved
In this case, it seems completely safe to use `viewModelScope` right away
…erce/woocommerce-android into migrate_product_tag_to_room
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.
👋 @wzieba !
I have reviewed and tested this PR as per the instructions, everything works as expected (ish), one more migration (almost) done! ✅ 💪 🚀
Warning (WCProductTagModel
table being dropped from the wp-fluxc
database (just like it was done here as an example).
WellSqlConfig.kt
open class WellSqlConfig : DefaultWellConfig {
...
override fun getDbVersion(): Int {
return 211
}
override fun onUpgrade(...) {
...
210 -> migrateAddOn(ADDON_WOOCOMMERCE, version) {
db.execSQL("DROP TABLE IF EXISTS WCProductTagModel")
}
...
}
...
}
PS: The 211
db version takes into account that a similar 210
migration will happen as part of #13996 (see
I have left one warning (
FYI: As soon as these #14027 and #13966 PRs gets reviewed, tested and merged, my plan is to proceed with these changes myself, unless of course you've already returned from AFK by then. 😊 🤷
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCProductTagModel.kt
Outdated
Show resolved
Hide resolved
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCProductTagModel.kt
Show resolved
Hide resolved
...fluxc-plugin/src/test/java/org/wordpress/android/fluxc/persistence/dao/ProductTagsDaoTest.kt
Outdated
Show resolved
Hide resolved
...mmerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailViewModel.kt
Show resolved
Hide resolved
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/ProductTagsDao.kt
Outdated
Show resolved
Hide resolved
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt
Outdated
Show resolved
Hide resolved
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt
Outdated
Show resolved
Hide resolved
Update DAOs methods signatures to not duplicate information
It has been migrated to Room
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.
👋 @wzieba , I did another review on this after those new commits you pushed today and everything LGTM, I also tested the migration/app once more and (obviously) it works as expected. Thanks a lot! 🙇 ❤️ 🚀
It is a ✅ from my side, so I am approving this. I am not sure if you also want someone from the product team to do some more testing on this change before merging. I'll leave that up to you. 👍
Closes: AINFRA-127
Description
This PR migrates
WCProductTagModel
to Room.Steps to reproduce
Testing information
Smoke test any feature that uses
WCProductTagModel
.The tags are under
+ ADD MORE DETAILS
button on the details of the product:Images/gif
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: