Skip to content

Conversation

@wzieba
Copy link
Contributor

@wzieba wzieba commented Dec 1, 2025

Related: #15011
Closes: AINFRA-548: Migrate ThemeModel

Description

This PR migrates ThemeModel from WellSql to Room. As always, these PRs focus on migrations, but in this specific case I also decided to migrate a few classes from Java to Kotlin, to be able to run suspend methods.

Test Steps

Smoke test theme functionality:

  1. Open the app and navigate to SettingsSite SettingsThemes
  2. Verify the current theme is displayed correctly
  3. Browse available themes and verify they load with names and preview images
  4. Select a different theme and activate it
  5. Verify the theme activation succeeds and the new theme is shown as active

Images/gif

Before (trunk)

Screenshot 2025-12-05 at 14 04 47

After

Screenshot 2025-12-05 at 13 24 28
  • 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.

wzieba and others added 3 commits December 1, 2025 12:34
- Convert all classes and methods to Kotlin syntax
- Use @JvmField for properties accessed from Java code
- Maintain exact same behavior and API
- All tests passing

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Convert ThemeModel to Room entity with composite primary key (siteId, themeId, isWpComTheme)
- Create ThemeDao with direct @upsert exposure following OrderStatusDao pattern
- Remove ThemeSqlUtils and move business logic to ThemeStore
- Add LocalIdConverter for Room type conversion
- Add comprehensive ThemeDaoTest with 13 test cases
- Rewrite ThemeStoreUnitTest from Java to Kotlin
- Update WPAndroidDatabase to version 31 with AutoMigration
- Use site.localId() instead of LocalId(site.id)
- Use runBlocking without dispatcher to preserve threading behavior

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@dangermattic
Copy link
Collaborator

dangermattic commented Dec 1, 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

wzieba and others added 2 commits December 4, 2025 12:29
Updates ThemeRepository to use the new Room-based ThemeModel constructor
with all required parameters instead of the deprecated default constructor.
This fixes compilation errors in the Wasabi debug unit tests.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The LocalIdConverter was defined in both fluxc and fluxc-plugin modules,
causing a "Type is defined multiple times" error during dex merging.

Since fluxc-plugin already depends on fluxc via api dependency, it
inherits the LocalIdConverter from the fluxc module. Removed the
duplicate from fluxc-plugin to resolve the build error.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 4, 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 NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit3d173e9
Direct Downloadwoocommerce-wear-prototype-build-pr15045-3d173e9.apk

- Remove unused imports from ThemeStore, ThemeDao, and ThemeStoreUnitTest
- Fix import ordering in WPAndroidDatabase (ThemeModel should come before persistence imports)
- Remove unused createSite function from ThemeDaoTest
- Rename test functions to follow detekt naming convention: "when X, then Y"

All 19 detekt issues have been resolved.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 4, 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 NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit3d173e9
Direct Downloadwoocommerce-prototype-build-pr15045-3d173e9.apk

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 0% with 264 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.52%. Comparing base (eda322f) to head (3d173e9).
⚠️ Report is 44 commits behind head on trunk.

Files with missing lines Patch % Lines
...va/org/wordpress/android/fluxc/store/ThemeStore.kt 0.00% 147 Missing ⚠️
.../fluxc/network/rest/wpcom/theme/ThemeRestClient.kt 0.00% 87 Missing ⚠️
...m/woocommerce/android/ui/themes/ThemeRepository.kt 0.00% 18 Missing ⚠️
...va/org/wordpress/android/fluxc/model/ThemeModel.kt 0.00% 8 Missing ⚠️
...ordpress/android/fluxc/persistence/dao/ThemeDao.kt 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #15045      +/-   ##
============================================
- Coverage     38.61%   38.52%   -0.09%     
- Complexity    10311    10312       +1     
============================================
  Files          2163     2167       +4     
  Lines        122674   122936     +262     
  Branches      16934    16945      +11     
============================================
+ Hits          47365    47366       +1     
- Misses        70503    70765     +262     
+ Partials       4806     4805       -1     

☔ 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.

wzieba and others added 2 commits December 4, 2025 17:06
- Move ThemeStoreUnitTest from fluxc-tests to fluxc/store package
- Create ThemeTestUtils with createTestTheme helper function
- Create SiteTestUtils with generateWPComSite helper function
- Update ThemeDaoTest to use shared createTestTheme utility
- Update ThemeStoreUnitTest to use shared utilities

This consolidates theme-related tests in the fluxc module and provides
reusable test helpers for creating test themes and sites.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@wzieba wzieba requested a review from Copilot December 4, 2025 16:45
Copilot finished reviewing on behalf of wzieba December 4, 2025 16:48
Copy link
Contributor

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 ThemeModel from WellSQL to Room database, modernizing the persistence layer for theme management in FluxC. The migration includes converting Java code to Kotlin and updating all related components.

Key Changes

  • Converted ThemeModel from Java/WellSQL to Kotlin data class with Room annotations
  • Migrated persistence from ThemeSqlUtils (WellSQL) to ThemeDao (Room)
  • Converted ThemeStore and ThemeRestClient from Java to Kotlin

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
libs/fluxc/src/main/java/org/wordpress/android/fluxc/model/ThemeModel.kt Converted to Kotlin data class with Room entity annotations and composite primary key
libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/dao/ThemeDao.kt New Room DAO with suspend functions for theme data access
libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/converters/LocalIdConverter.kt Type converter for Room to handle LocalId custom type
libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WPAndroidDatabase.kt Updated database to version 31, added ThemeModel entity and LocalIdConverter
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ThemeStore.kt Converted to Kotlin, integrated Room database with runBlocking for backward compatibility
libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpcom/theme/ThemeRestClient.kt Converted to Kotlin, updated ThemeModel construction to use data class constructor
libs/fluxc/src/test/java/org/wordpress/android/fluxc/utils/ThemeTestUtils.kt New test utility for creating test themes
libs/fluxc/src/test/java/org/wordpress/android/fluxc/utils/SiteTestUtils.kt New test utility for creating test sites
libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/ThemeStoreUnitTest.kt New Room-based unit tests for ThemeStore
libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/ThemeDaoTest.kt Comprehensive unit tests for ThemeDao operations
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/themes/ThemeRepository.kt Updated to use new ThemeModel data class constructor
libs/fluxc/schemas/org.wordpress.android.fluxc.persistence.WPAndroidDatabase/31.json Room database schema for version 31 with ThemeModel table

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +79 to +83
val retrievedTheme = themeStore.getInstalledThemeByThemeId(site, testThemeId)
assertThat(retrievedTheme)
.usingRecursiveComparison()
.ignoringFields("active")
.isEqualTo(testTheme)
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The test comparison is incomplete. The testTheme is created with siteId = LocalId(0) (line 69), but when inserted via setActiveThemeForSite (line 76), the theme is copied with siteId = site.localId() which is LocalId(556) based on the test site. The comparison should either:

  1. Ignore the "siteId" field in addition to "active", or
  2. Create the testTheme with the correct siteId upfront

Currently, this test would fail because the siteId values don't match.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually passes because in ThemeStore#setActiveThemeForSite we set the site id like

val activeTheme = theme.copy(
            siteId = site.localId(),
            isWpComTheme = false,
            active = true
        )

I agree it's not very readable, but themes created in ThemeRestClient always set siteId = LocalId(0),.

I think this part of codebase could use some refactoring to fix this and remove the event bus architecture, but I also want to limit this PR to be focused on database migration.

@wzieba wzieba added the type: technical debt Represents or solves tech debt of the project. label Dec 5, 2025
@wzieba wzieba requested a review from JorgeMucientes December 5, 2025 13:06
@wzieba wzieba added this to the 23.9 milestone Dec 5, 2025
@wzieba wzieba marked this pull request as ready for review December 5, 2025 13:06
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