Skip to content

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Aug 13, 2025

Closes: AINFRA-552
Relates To: #14453


Description

This PR migrates WCGlobalAttributeModel from WellSQL to Room database.

FYI: This is a follow-up to #14453 and the removal of WCAttributeTermModel. This follow-up also removes the termsId column from the WCGlobalAttributeModel table before the actual migration starts (6e07007).

I actually lost more time trying to connect the dots rather than migrating/removing this table... 😭

I now have a hypothesis about whether this WCAttributeTermModel table, the termsId column from the WCGlobalAttributeModel table, and all the unnecessary logic within the corresponding Store/Rest/SQL classes (which btw, now got removed: 2f1b0e7). My hypothesis is, as part of this FluxC#1803 PR, especially after seeing this commit, Thomaz made all attribute related CRUD operations available no matter if there were to be used (or not). Most probably the plan was to use them at some point, but, that never actually happened. Thus, all this logic ended-up being unused, and at best only used within FluxC's example app.


Testing information

  1. Go to the Products screen.
  2. Find a Variable product or change a Physical product to a Variable product.
  3. Click on Add variations to navigate to the Add attribute screen.
  4. Tap to select an existing attribute (like Color or Size) and see the list of option of that attribute.
  5. Tap on a couple of options and then click NEXT twice, then press the GENERATE VARIATION button and select Generate all variations, following by OK when the dialog appears.
  6. Click on Add variations again to navigate to the Variations screen where you'll see the variations you added.
  7. Go back and click the SAVE button to have these changes apply to the product.

EXTRA

You might also want to add a new attribute on your store and see that everything is working as expected. To do so, follow the below instructions:

  1. Go to your store's wp-admin.
  2. Find the Products section, and within it, click on the Attributes sub-section.
  3. Add a new attribute, following the instructions. For example, you could add a new attribute named Material with the the-material-of-the-product slug.
  4. Click on Configure terms for that newly added attributed on that table, and then add a few terms for that attribute, following the instructions. For example, you could add a new term named Cotton with the cotton-product-material slug, etc. Add a few of those.
  5. Now, open the mobile app and follow the above instructions once more. Making sure that this newly added attribute, along with all its terms are shown and usable.

Images/gif

image
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

FYI: Similarly to this 364c18d change,
which completely removed everything 'WCAttributeTermModel', this related
'termsId' column/code is also unnecessary and can be safely removed from
its associated 'WCGlobalAttributeModel' table.
FYI: This is done in preparation to the subsequent WellSQL to Room
table migration.
FYI: This is done in preparation to the subsequent WellSQL to Room
table migration.
FYI: This change includes but is not limited to the:
- Entity Introduction
- DAO Introduction (With Test)
- WCAndroidDatabase Migration
- Code Adjustments (Mapper, Store)
- Test Adjustments (Mapper, Store)
PS: It is quite hard to make sense of all those chained scope function
calls.
@ParaskP7 ParaskP7 added this to the 23.1 milestone Aug 13, 2025
@ParaskP7 ParaskP7 requested a review from Copilot August 13, 2025 13:18
@ParaskP7 ParaskP7 added the type: technical debt Represents or solves tech debt of the project. label Aug 13, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 13, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

Copilot

This comment was marked as outdated.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 13, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit29a7d1d
Direct Downloadwoocommerce-wear-prototype-build-pr14464-29a7d1d.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 13, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit29a7d1d
Direct Downloadwoocommerce-prototype-build-pr14464-29a7d1d.apk

@ParaskP7 ParaskP7 marked this pull request as ready for review August 13, 2025 14:03
@ParaskP7 ParaskP7 requested review from Copilot, a team and malinajirka and removed request for a team August 13, 2025 14:03
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 0% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.92%. Comparing base (4e56f2c) to head (29a7d1d).
⚠️ Report is 38 commits behind head on trunk.

Files with missing lines Patch % Lines
...ress/android/fluxc/store/WCGlobalAttributeStore.kt 0.00% 14 Missing ⚠️
...oocommerce/android/model/ProductGlobalAttribute.kt 0.00% 10 Missing ⚠️
...luxc/model/attribute/terms/WCAttributeTermModel.kt 0.00% 8 Missing ⚠️
...id/fluxc/model/attribute/WCGlobalAttributeModel.kt 0.00% 7 Missing ⚠️
...droid/fluxc/persistence/dao/GlobalAttributesDao.kt 0.00% 4 Missing ⚠️
...rdpress/android/fluxc/persistence/WellSqlConfig.kt 0.00% 4 Missing ⚠️
...d/fluxc/model/attribute/WCGlobalAttributeMapper.kt 0.00% 2 Missing ⚠️
...oid/ui/products/details/ProductDetailRepository.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #14464      +/-   ##
============================================
+ Coverage     37.86%   37.92%   +0.06%     
  Complexity     9262     9262              
============================================
  Files          2008     2008              
  Lines        113100   112905     -195     
  Branches      14950    14916      -34     
============================================
  Hits          42822    42822              
+ Misses        66379    66184     -195     
  Partials       3899     3899              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@Copilot Copilot AI left a 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 WCGlobalAttributeModel from WellSQL to Room database as part of a broader effort to modernize the data persistence layer. The migration involves converting the model to a Room entity, creating a new DAO for database operations, updating the store to use Room instead of WellSQL utilities, and cleaning up unused attribute-related functionality.

Key Changes:

  • Migrates WCGlobalAttributeModel from WellSQL to Room with new entity structure
  • Removes unused attribute CRUD operations and related test files
  • Updates database version and adds migration to drop old WellSQL table

Reviewed Changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
WellSqlConfig.kt Increments database version and adds migration to drop WCGlobalAttributeModel table
WCGlobalAttributeModel.kt Converts from WellSQL model to Room entity with updated field types
GlobalAttributesDao.kt New Room DAO for global attributes database operations
WCGlobalAttributeStore.kt Updated to use Room DAO instead of WellSQL utilities
WCAndroidDatabase.kt Adds new entity and DAO to Room database configuration
Various test files Removes unused test fixtures and updates existing tests for Room migration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ParaskP7
Copy link
Contributor Author

I swear @malinajirka, with Wojtek AFK from tomorrow, I just used @woocommerce/android-developers and it randomly assigned you... 😊 😅 🫣

@malinajirka
Copy link
Contributor

I swear @malinajirka, with Wojtek AFK from tomorrow, I just used @woocommerce/android-developers and it randomly assigned you... 😊 😅 🫣

Haha 🤣 , no worries at all. I'm adding it to my todo list, but since I was AFK yesterday, there is quite a lot there. I'll review it by the end of tomorrow the latest.

@ParaskP7
Copy link
Contributor Author

Haha 🤣 , no worries at all.

🤣

I'm adding it to my todo list, but since I was AFK yesterday, there is quite a lot there.

I bet @malinajirka , hoping all is good! 🫂

I'll review it by the end of tomorrow the latest.

This is not urgent, not at all, please review it when you get some extra space to breath... 🙏

@malinajirka malinajirka self-assigned this Aug 14, 2025
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ParaskP7! Great job, I've reviewed the code and tested the changes.

I've encountered a small issue one time, but it's very likely unrelated to these changes and I haven't been able to reproduce it since. When I created a new variable product and added 3 attributes, the generate variations button would tell me there are no attributes to generate the combinations from. However, when I left that screen and entered it again, I could generate them without issues. I tried to replicate it multiple times without success.

I believe this is good to merge :shipit:. Leaving it open in case the above steps would give you some idea.

@ParaskP7
Copy link
Contributor Author

👋 @malinajirka and thanks for reviewing/testing the changes! 🙇 ❤️ 🚀

I've encountered a small issue one time, but it's very likely unrelated to these changes and I haven't been able to reproduce it since. When I created a new variable product and added 3 attributes, the generate variations button would tell me there are no attributes to generate the combinations from. However, when I left that screen and entered it again, I could generate them without issues. I tried to replicate it multiple times without success.

Yea, I think I know what you mean, not sure if that's the same, but me not being experienced enough with this variable product flow overall, I too got confused many time. I went back and forward testing the flows on trunk and on this branch many times, just to make sure I am not introducing (somehow) any regressions.

On that specific flow what I noticed (and might hep) is the below:

  1. When you change the product type from the usual Physical product to a Variable product and then go through the Add variations flow, at the very end, after clicking NEXT a couple of time, you get to the GENERATE VARIATION screen, very intuitive.
  2. At this point the Add variations screen get transformed to another screen, the Variations attributes screen.
  3. Now, if you go and try the same flows as above (1.) you no longer have any NEXT buttons when you. add a new attribute, you can only go back on the product's main page
  4. Now, if you click the SAVE button, only then you could Generate all variations via the Variations screen. Otherwise, if you don't save, and although you have already added the new variations attributes, when you try and Generate all variations via the Variations screen, you'll get the No variations to generate result.

Maybe this is what you've experienced above, you just forgot to click SAVE, wdyt? 🤷


Btw, I like the fact that we're talking about this, I wanted to, because this might be an opportunity to further discuss this specific flow. I am pretty sure I can't be the only one who's confused by how this works, and I think that our users, unless they too tried it a few time, they'll be confused as well. 🤔

Let me know your thoughts! 🙏

I believe this is good to merge :shipit:. Leaving it open in case the above steps would give you some idea.

:shipit:

…to migrate/wc-global-attribute-model-to-room

# Conflicts:
#	libs/fluxc-plugin/schemas/org.wordpress.android.fluxc.persistence.WCAndroidDatabase/56.json
#	libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/di/WCDatabaseModule.kt
#	libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt
@malinajirka
Copy link
Contributor

Btw, I like the fact that we're talking about this, I wanted to, because this might be an opportunity to further discuss this specific flow. I am pretty sure I can't be the only one who's confused by how this works, and I think that our users, unless they too tried it a few time, they'll be confused as well. 🤔

Haha, I actually raised this discussion earlier this morning when I saw a user review - I pinged you on Slack.

Maybe this is what you've experienced above, you just forgot to click SAVE, wdyt? 🤷

Yep, I'm almost certain this was it. Thanks for the clear steps!

@ParaskP7
Copy link
Contributor Author

Haha, I actually raised this discussion earlier this morning when I saw a user review - I pinged you on Slack.

Awesome, thanks a lot! 🙇 💯 🥇

Yep, I'm almost certain this was it. Thanks for the clear steps!

Thank YOU! ❤️

Due to cbac12f a merge conflict
appeared which got solved via 07b967a,
with the main 'WCAndroidDatabase' including:

- const val WC_DATABASE_VERSION = 57
- AutoMigration(from = 56, to = 57),

As well as accepting the current version of '56.json' and creating this
new one, the '57.json' because the database version for this exact
change has now been updated to '57' (from '56').
@ParaskP7 ParaskP7 merged commit c797d6b into trunk Aug 18, 2025
17 checks passed
@ParaskP7 ParaskP7 deleted the migrate/wc-global-attribute-model-to-room branch August 18, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: technical debt Represents or solves tech debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants