`WCOrderSummary` to `Room` #14315

wzieba · 2025-07-11T12:25:55Z

Closes: AINFRA-558

Description

This PR migrates WCOrderSummary from WellSql to Room

Steps to reproduce

Testing information

Create a new Jurassic test site - check "Smooth Generator" option for WooCommerce
Open the site on web, go to Tools > Smooth generator
Generate some products and a great number of orders, e.g. 1000 (it takes a few minutes)
Open the mobile app and smoke test browsing the list of orders

Images/gif

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.

…ansaction

…ansaction for a possible small performance gain The need for "chunk" is an implementation constraint from SQL - it shouldn't leak to consumer of dao. Therefore this change name from "how" to simple "what".

dangermattic · 2025-07-11T12:27:13Z

	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

wpmobilebot · 2025-07-11T12:36:36Z

📲 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
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`806e440`
	Direct Download	`woocommerce-wear-prototype-build-pr14315-806e440.apk`

It doesn't need it. Make `dateModified` nullable, as this is more correct interpretation of "no value" state than an empty string.

wpmobilebot · 2025-07-11T12:43:49Z

📲 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
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`806e440`
	Direct Download	`woocommerce-prototype-build-pr14315-806e440.apk`

codecov-commenter · 2025-07-11T13:14:00Z

Codecov Report

Attention: Patch coverage is 3.84615% with 50 lines in your changes missing coverage. Please review.

Project coverage is 37.69%. Comparing base (ba6df7d) to head (806e440).
Report is 311 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../org/wordpress/android/fluxc/store/WCOrderStore.kt	0.00%	21 Missing ⚠️
.../android/ui/orders/list/OrderListItemDataSource.kt	0.00%	10 Missing ⚠️
...uxc/network/rest/wpcom/wc/order/OrderRestClient.kt	0.00%	6 Missing ⚠️
...rdpress/android/fluxc/model/WCOrderSummaryModel.kt	0.00%	5 Missing ⚠️
...s/android/fluxc/persistence/dao/OrderSummaryDao.kt	0.00%	4 Missing ⚠️
...rdpress/android/fluxc/persistence/WellSqlConfig.kt	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk   #14315   +/-   ##
=========================================
  Coverage     37.69%   37.69%           
+ Complexity     9054     9053    -1     
=========================================
  Files          1979     1980    +1     
  Lines        110994   110984   -10     
  Branches      14602    14602           
=========================================
- Hits          41837    41836    -1     
+ Misses        65336    65326   -10     
- Partials       3821     3822    +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.

Copilot

Pull Request Overview

This PR migrates the WCOrderSummary data flow from WellSql into Room.

Introduces a new OrderSummaryDao, Room entity (WCOrderSummaryModel), and database migration (version bump to 54).
Refactors stores and clients (WCOrderStore, OrderUpdateStore, OrderRestClient, etc.) to use the new DAO instead of OrderSqlUtils.
Removes WellSql-based OrderSqlUtils methods and updates tests to target the new DAO.

Reviewed Changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WellSqlConfig.kt	Bumped WellSql DB version and dropped old table.
libs/fluxc-plugin/src/main/kotlin/.../persistence/dao/OrderSummaryDao.kt	New DAO with chunked queries and upsert methods.
libs/fluxc-plugin/src/main/kotlin/.../model/WCOrderSummaryModel.kt	Converted model from WellSql annotations to Room.
libs/fluxc-plugin/src/main/kotlin/.../WCAndroidDatabase.kt	Added `WCOrderSummaryModel` and DAO to Room DB.
libs/fluxc-plugin/src/main/kotlin/.../store/WCOrderStore.kt	Updated store to call DAO instead of WellSql.
libs/fluxc-plugin/src/main/kotlin/.../store/OrderUpdateStore.kt	Swapped `OrderSqlUtils` for DAO calls.
libs/fluxc-plugin/src/test/java/.../persistence/dao/OrderSummaryDaoTest.kt	New tests covering basic DAO operations.
WooCommerce/src/main/kotlin/.../OrderListItemDataSource.kt	Adapted UI data source to fetch via DAO.

Comments suppressed due to low confidence (1)

libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/OrderSummaryDao.kt:24

The chunking logic splits IDs into subsets of CHUNK_SIZE—it would be prudent to add a test case that verifies getOrderSummaries handles lists larger than CHUNK_SIZE correctly.

        return remoteOrderIds.chunked(CHUNK_SIZE)

Copilot · 2025-07-11T15:48:59Z

libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCOrderSummaryModel.kt

+) {
+    @Ignore var dateModified: String? = null
 }


The dateModified field is marked with @Ignore and won't be persisted, so its value is always null when fetching. You likely need to include it in the entity (remove @Ignore and add it to the primary constructor) so that Room stores and retrieves the modified date correctly.

Suggested change

) {

@Ignore var dateModified: String? = null

}

val dateModified: String? = null // ISO 8601-formatted date in UTC, e.g. 1955-11-05T14:15:00Z

)

Not really - the previous WellSql implementation didn't had this field in db table as well:

woocommerce-android/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WellSqlConfig.kt

Lines 877 to 882 in c68a743

db.execSQL(

"CREATE TABLE WCOrderSummaryModel (LOCAL_SITE_ID INTEGER,REMOTE_ORDER_ID INTEGER," +

"DATE_CREATED TEXT NOT NULL,_id INTEGER PRIMARY KEY AUTOINCREMENT," +

"FOREIGN KEY(LOCAL_SITE_ID) REFERENCES SiteModel(_id) ON DELETE CASCADE," +

"UNIQUE (REMOTE_ORDER_ID, LOCAL_SITE_ID) ON CONFLICT REPLACE)"

)

so I guess it's intentional

Copilot · 2025-07-11T15:49:00Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/OrderListItemDataSource.kt

-                    summaries.filter { cachedOrders.containsKey(it.orderId) }
-                } else {
-                    summaries
+        val orderSummaries = runBlocking {


Using runBlocking in a UI data source can block the main thread and cause jank. Consider making this method suspendable or launching a coroutine in an appropriate scope instead of using runBlocking.

Well known comment. As always - it's intentional, as these migrations are not meant to resolve threading issues (sometimes they do when I'm sure there are no side effects)

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions (almost), everything works as expected, good job! 🌟 x 🌟 ^ 🌟

I have left one question (❓), one suggestions (💡) and one minor (🔍) comment for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

EXTRAS

Copilot wrote this and it made me 🤔 a bit: The chunking logic splits IDs into subsets of CHUNK_SIZE—it would be prudent to add a test case that verifies getOrderSummaries handles lists larger than CHUNK_SIZE correctly.
On this second step of testing information 2. Open the site on web, go to Tools > Smooth generator, I could proceed as I was getting this critical error below:

Any ideas, did that happen to you too? 🤔

FYI: I tried that 3 times already, 3 different sites... 🤷

libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCOrderSummaryModel.kt

ParaskP7 · 2025-07-14T15:46:30Z

libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/WCOrderSummaryModel.kt

-        "UNIQUE (REMOTE_ORDER_ID, LOCAL_SITE_ID) ON CONFLICT REPLACE"
+@Entity(
+    tableName = "OrderSummaryEntity",
+    primaryKeys = ["siteId", "orderId"]


Info (ℹ️): Just an FYI that after I migrated I noticed that, when navigating to the Orders screen, the list of orders was empty and wouldn't auto-refresh. I then had to manually trigger pull-to-refresh to get the orders to show up.

Could you please confirm that it doesn't happen on trunk?

👋 @wzieba , how do you suggest I confirm that this doesn't happen on trunk? 🤔

FYI: To my understanding, I should uninstall/install the app to get a similar behavior, an empty WCOrderSummaryModel table, correct? And, as far as I remember, whenever I uninstall/install the app from scratch, when navigating to the Orders screen, the list of orders gets auto-populated for me, no need for a manual pull-to-refresh from my side.

Ah sorry, I understand now. I wonder, we can either:

leave it as it is (one time pull-to-refresh shouldn't be bothering)

move the data from WellSql to Room database before removing the table (maybe worth experimenting with a solution for this as we'll have to do it anyway for SiteModel)

request summaries fetch if database has orders, but not summaries (it'll be one-time case, only after initial run after this migration)

I'll check how time-consuming would be the second point .

Thanks for the reply @wzieba ! 🙏

leave it as it is (one time pull-to-refresh shouldn't be bothering)

Can't disagree too much, it shouldn't be too bothering indeed.

Then again, in trying to think as a non-tech user, I wonder if they'll get scared not having any orders shown after an app update... And how quickly would they find pull-to-refresh to get their soul back into their body... 😅 🤷

FYI: I think personally this was they first time I used pull-to-refresh on this screen, but can't be too sure, thinking that the order screen got auto-refreshed for me every time I visit this screen, again, not sure... 🤔

move the data from WellSql to Room database before removing the table (maybe worth experimenting with a solution for this as we'll have to do it anyway for SiteModel)

Maybe this is indeed worth experimenting and get some initial feedback on the process... 🤔

request summaries fetch if database has orders, but not summaries (it'll be one-time case, only after initial run after this migration)

Interesting idea, maybe that would work... 🤔

I'll check how time-consuming would be the second point .

👍 and btw, maybe it's a good time to ask someone from the product team for some extra feedback on that, wdyt? 💭

.../fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/OrderSummaryDao.kt

...luxc-plugin/src/test/java/org/wordpress/android/fluxc/persistence/dao/OrderSummaryDaoTest.kt

ParaskP7 · 2025-07-14T16:17:03Z

Any ideas, did that happen to you too? 🤔

Btw @wzieba , maybe this here and me being just unlucky: p1752505897072599-slack-C02TDTD1GCX 🤷

PS: Will try again tomorrow... 🤞

… is moved to Room

wzieba · 2025-07-15T20:31:40Z

Copilot wrote this and it made me 🤔 a bit: The chunking logic splits IDs into subsets of CHUNK_SIZE—it would be prudent to add a test case that verifies getOrderSummaries handles lists larger than CHUNK_SIZE correctly.

I was thinking about test for this method, but rather to verify if it really runs in a transaction - I wasn't sure that @Transaction annotation was enough to make it happen. So I experimented with a test, but eventually decided to not add it as I thought it's too complex. But this comment made me think that this could be actually useful, so here it is: fe950c8

…es in `OrderSummaryDao`

ParaskP7 · 2025-07-16T08:25:57Z

👋 @wzieba !

I was thinking about test for this method, but rather to verify if it really runs in a transaction - I wasn't sure that @transaction annotation was enough to make it happen. So I experimented with a test, but eventually decided to not add it as I thought it's too complex. But this comment made me think that this could be actually useful, so here it is: fe950c8

I just reviewed this OrderSummaryDaoChunksTest, thanks for doing that, an awesome way you found there! 🥇

Question (❓): Is this assertThat(normalizedLogs).contains(buildString { ... } really needed? I am just trying to understand why you added it. Isn't this testing implementation details of how Room does it? Wouldn't just testing that adding 250 order summaries and then retrieving them be enough? Understand that you also want to test the chunked functionality, and if want to keep that please do, I was just not expecting that when writing/pointing to that comment. 🤔
Suggestion (💡): Just to keep this test simpler, I would extract the whole chucked test login into its own assert method, maybe that will further help:

    @Test
    fun `given more than 200 order IDs when getOrderSummaries is called then it chunks input and returns all results in a single transaction`() =
        runTest {
            val orderSummaries = (1..250).asOrderSummaries(siteId)
            dao.upsertOrderSummaries(orderSummaries)

            val remoteIds = orderSummaries.map(WCOrderSummaryModel::orderId)

            // Call your method that chunks with a for loop and is annotated with @Transaction
            val result = dao.getOrderSummaries(siteId, remoteIds)

            assertChunked()
            assertThat(result).containsExactlyInAnyOrderElementsOf(orderSummaries)
        }

    private fun assertChunked() {
        val expectedFirstChunk = createExpectedQuery(200)
        val expectedSecondChunk = createExpectedQuery(50)

        val normalizedLogs = logs.lines()
            .map { it.trim() }
            .filter { it.isNotEmpty() }
            .joinToString("\n")
        assertThat(normalizedLogs).contains(
            buildString {
                appendLine(expectedFirstChunk)
                appendLine(expectedSecondChunk)
                appendLine("TRANSACTION SUCCESSFUL")
                appendLine("END TRANSACTION")
            }
        )
    }

wzieba added 4 commits July 10, 2025 13:48

Migrate WCOrderSummaryModel to Room

01ba165

Adjust and migrate OrderSummary unit tests

041ca33

Rename getOrderSummariesChunked to getOrderSummaries. Make it @tr…

6a3aa05

…ansaction for a possible small performance gain The need for "chunk" is an implementation constraint from SQL - it shouldn't leak to consumer of dao. Therefore this change name from "how" to simple "what".

Drop WCOrderSummaryModel from WellSql

2d8dc77

Remove default parameters from WCOrderSummaryModel

76b280e

It doesn't need it. Make `dateModified` nullable, as this is more correct interpretation of "no value" state than an empty string.

Base automatically changed from migrate_refund to trunk July 11, 2025 15:47

wzieba added the type: technical debt Represents or solves tech debt of the project. label Jul 11, 2025

wzieba added this to the 22.9 milestone Jul 11, 2025

wzieba requested a review from Copilot July 11, 2025 15:47

Copilot AI reviewed Jul 11, 2025

View reviewed changes

wzieba requested a review from ParaskP7 July 14, 2025 08:04

wzieba marked this pull request as ready for review July 14, 2025 08:04

ParaskP7 approved these changes Jul 14, 2025

View reviewed changes

wzieba added 5 commits July 15, 2025 22:16

Introduce OrderSummaryDaoChunksTest

fe950c8

Remove redundant unit test

9a4ede8

Improve dao formatting

cdb80ff

Remove debug println from OrderSummaryDaoChunksTest

a07082b

Add a todo comment to bring foreign key constraint when SiteModel…

0302bab

… is moved to Room

Adjust OrderSummaryDaoChunksTest test after recent formatting chang…

806e440

…es in `OrderSummaryDao`

	db.execSQL(
	"CREATE TABLE WCOrderSummaryModel (LOCAL_SITE_ID INTEGER,REMOTE_ORDER_ID INTEGER," +
	"DATE_CREATED TEXT NOT NULL,_id INTEGER PRIMARY KEY AUTOINCREMENT," +
	"FOREIGN KEY(LOCAL_SITE_ID) REFERENCES SiteModel(_id) ON DELETE CASCADE," +
	"UNIQUE (REMOTE_ORDER_ID, LOCAL_SITE_ID) ON CONFLICT REPLACE)"
	)

WCOrderSummary to Room #14315

Are you sure you want to change the base?

WCOrderSummary to Room #14315

Uh oh!

Conversation

wzieba commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to reproduce

Testing information

Images/gif

Uh oh!

dangermattic commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

wzieba Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

wzieba Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

ParaskP7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ParaskP7 Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

wzieba Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

wzieba Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ParaskP7 commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzieba commented Jul 15, 2025

Uh oh!

ParaskP7 commented Jul 16, 2025

Uh oh!

Uh oh!

`WCOrderSummary` to `Room` #14315

`WCOrderSummary` to `Room` #14315

wzieba commented Jul 11, 2025 •

edited

Loading

dangermattic commented Jul 11, 2025 •

edited

Loading

wpmobilebot commented Jul 11, 2025 •

edited

Loading

wpmobilebot commented Jul 11, 2025 •

edited

Loading

codecov-commenter commented Jul 11, 2025 •

edited

Loading

wzieba Jul 11, 2025 •

edited

Loading

ParaskP7 left a comment •

edited

Loading

ParaskP7 commented Jul 14, 2025 •

edited

Loading