-
Notifications
You must be signed in to change notification settings - Fork 134
[WellSQL Migration] Remove Unnecessary WCAttributeTermModel
Table/Code from wp-fluxc
#14453
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
[WellSQL Migration] Remove Unnecessary WCAttributeTermModel
Table/Code from wp-fluxc
#14453
Conversation
FYI: The 'WCAttributeTermModel' class is kept on purpose and so as to reduce the amount of code change this would produce, should that "domain" related class were also deleted.
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 removes the unnecessary WCAttributeTermModel
table and related code from the FluxC library as part of a WellSQL migration. The change eliminates database table creation, persistence logic, and related functionality while keeping the model class as a simple data class to minimize code changes.
- Increments database version and adds migration to drop the
WCAttributeTermModel
table - Removes database persistence functionality from
WCAttributeTermModel
and converts it to a plain data class - Eliminates term-related methods and imports from store and utility classes
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
WellSqlConfig.kt | Increments DB version to 227 and adds migration to drop WCAttributeTermModel table |
WCGlobalAttributeStoreTest.kt | Removes WCAttributeTermModel from test configuration |
WCGlobalAttributeStore.kt | Removes term insertion logic and related imports |
WCGlobalAttributeSqlUtils.kt | Removes all term-related database operations and utility methods |
WCAttributeTermModel.kt | Converts from WellSQL table model to simple data class |
WCGlobalAttributeModel.kt | Removes term-related functionality and product attribute conversion methods |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCGlobalAttributeStore.kt
Show resolved
Hide resolved
...plugin/src/main/kotlin/org/wordpress/android/fluxc/model/attribute/WCGlobalAttributeModel.kt
Show resolved
Hide resolved
📲 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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #14453 +/- ##
============================================
+ Coverage 38.00% 38.01% +0.01%
Complexity 9229 9229
============================================
Files 1993 1993
Lines 112560 112506 -54
Branches 14853 14843 -10
============================================
Hits 42773 42773
+ Misses 65891 65837 -54
Partials 3896 3896 ☔ 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.
Looks good! Even better than migration 🙂
FYI: The WCAttributeTermModel class is kept on purpose and so as to reduce the amount of code change this would produce, should that "domain" related class were also deleted.
Do we plan to track it somewhere?
Thanks for the review @wzieba ! 🙇 ❤️ 🚀
💯 😄
How do you mean tracking it? To be honest, I just mentioned it without necessarily having an associated TODO with that specific FYI, just because I don't think it's worth revisiting this for what we're trying to do here, the overall migration work, just so we avoid deep dives that could make us lose focus from our ultimate goal, removing the WellSQL dependency. Wdyt? 🤔 |
I understood that the only reason to keep |
Fare enough @wzieba ! 👍
Done: AINFRA-1123 (created as a sub-task of AINFRA-549 itself) |
Thanks! |
FYI: Just to be on the 100% safe side of thing, before I merge this, I'll ping a product engineer to give us an extra 👍 on this and the subsequent change I am working on atm (AINFRA-552 - removing @malinajirka can you skim review this and give us a 👍 on it, maybe even connect the dots on how come we had this |
Thanks for the ping @ParaskP7 ! I don't have any previous context and unfortunately, I wasn't able to find any explanation during my quick research. Either case, I agree it seems it's not used + Claude also agrees + Wojtek agrees + you agree, so I think we are good to merge this 🥳! |
Thanks for the extra 👍 on this @malinajirka , much appreciated! 🙇 ❤️
🥳 🚀 |
…el-table-from-wp-fluxc
Closes: AINFRA-549
Description
What started with migrating this
WCAttributeTermModel
table, ended-up with removing it instead. This was done because apart from deleting and inserting attribute terms into a table (writing), see usage of insertAttributeTermsFromScratch(...), no flows or code was reading from that table, nor any table was depending on this table, its column are any of its stored values (ie.WCGlobalAttributeModel
).I tried to understand why this table existed in the first place, and whether some flow/code removal just didn't completely cleaned this up (in the past), following a few FluxC related PRs, but couldn't connect the dots, eventually assuming that this table was only used with FluxC's example app and not within WCAndroid itself at all, see PRs:
I actually lost more time trying to connect the dots rather than migrating/removing this table... 😭
FYI: The
WCAttributeTermModel
class is kept on purpose and so as to reduce the amount of code change this would produce, should that "domain" related class were also deleted.PS: As part of AINFRA-552 I'll also remove this
termsId
column fromWCGlobalAttributeModel
, not seeing it being used as well.Testing information
Products
screen.Variable product
or change aPhysical product
to aVariable product
.Add variations
to navigate to theAdd attribute
screen.Color
orSize
) and see the list of option of that attribute.NEXT
twice, then press theGENERATE VARIATION
button and selectGenerate all variations
, following byOK
when the dialog appears.Add variations
again to navigate to theVariations
screen where you'll see the variations you added.SAVE
button to have these changes apply to the product.EXTRA
If you want you could test this flow above on
trunk
and see thisWCAttributeTermModel
table being populated with (Color
orSize
) attribute values and various options. You will also notice that, for a particular attribute, previous options are deleted as new options are being shown on screen. This means that only the new options ending-up being saved on that table, which is weird by itself.RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.