-
Notifications
You must be signed in to change notification settings - Fork 136
Remove unused theme infrastructure from FluxC #15011
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
Conversation
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.
|
It's unused in Woo
It's unused in Woo
Woo uses "fetch wpcom themes" action only
They're not used in Woo
`resultsLimit` is always specified in `ThemeRepository#fetchThemes`
It was used only in tests
It was used only in tests
63da930 to
68ed834
Compare
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 unused theme infrastructure from FluxC to simplify the codebase, retaining only the 4 theme actions needed by the Woo mobile app: fetch WP.com themes, fetch current theme, activate theme, and install theme.
Key changes:
- Removed starter designs functionality and related API endpoints
- Removed installed themes management for Jetpack sites
- Simplified ThemeModel by removing unused fields (description, slug, version, author, screenshot URL, pricing, etc.)
- Cleaned up database operations and removed delete/remove theme actions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ThemeStore.java | Removed unused payload classes, event handlers, and public methods for starter designs, installed themes, and theme deletion |
| ThemeSqlUtils.java | Removed database operations for installed themes management and mobile-friendly theme queries |
| ThemeRestClient.java | Removed API methods for starter designs, Jetpack installed themes, and theme deletion; simplified theme model creation |
| ThemeModel.java | Removed unused fields and simplified constructors to only include essential properties |
| ThemeAction.java | Removed action enums for starter designs, installed themes, theme deletion, and site themes removal |
| StarterDesignsResponse.kt | Deleted entire file as starter designs feature is removed |
| ThemeStoreUnitTest.java | Updated tests to use parameterized methods and direct SQL access where needed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/fluxc/src/main/java/org/wordpress/android/fluxc/model/ThemeModel.java
Show resolved
Hide resolved
|
hi @JorgeMucientes 👋 I'm trying to round-robin review requests that, I think, are closer to Kiwi's scope. Are you available for doing reviews, or should I request other team members? :) |
It's not used anywhere
|
Hey @wzieba feel free to add me to PR reviews whenever. I may just not be as fast as other folks to get the review done, but I'm definitely part of the pool of reviewers :). |
JorgeMucientes
left a comment
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.
Thanks for working on this code clean up @wzieba.
Removed code looks safe and theme feature works as expected! ![]()
| final SiteModel site = SiteUtils.generateWPComSite(); | ||
| TestSiteSqlUtils.INSTANCE.getSiteSqlUtils().insertOrUpdateSite(site); | ||
| assertNull(mThemeStore.getActiveThemeForSite(site)); | ||
| assertEquals(0, ThemeSqlUtils.getActiveThemeForSite(site).size()); |
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.
Super minor np, a potentially a matter of preference but I think this style is more readable?
| assertEquals(0, ThemeSqlUtils.getActiveThemeForSite(site).size()); | |
| assertTrue(ThemeSqlUtils.getActiveThemeForSite(site).isEmpty()); |
No need to change :)
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.
It is, thanks! Actually, I like assertj assertions the most, so I went with
assertThat(ThemeSqlUtils.getActiveThemeForSite(site)).isEmpty();in 66cad90 🙂
Related to: AINFRA-548: Migrate
ThemeModelDescription
Woo mobile app only uses 4 theme actions (fetch WP.com themes, fetch current theme, activate, install). This PR removes the unused theme infrastructure including starter designs, installed themes management, and database operations that aren't needed by the app and were inherited with FluxC vendoring.
Test Steps
Smoke test theme functionality:
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.