Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

pmusolino · 2025-01-28T15:54:57Z

Description

We observed that while opening product details, it's easy to see the following warning, generated by multiple reads on storage (Core Data). Also, this issue sometimes generate a lag opening the Order Detail screen.

This warning is occurring because we're making a lot of different fetch requests when loading the details, causing the hang issue.

I solved the issue by removing two fetches from OrderDetailsResultsControllers:

updating the feeLines property to return the relationship from order.fees instead, of using a Results Controller to fetch the order fees.
I added the shippingLabels property to Order which was connected in Storage with a relationship, but not present inside the Networking model.
- Update Networking.generated.swift, Models+Copiable.generated.swift, and Order.swift to include shippingLabels property in Order struct.
- Update OrderDetailsResultsControllers.swift by removing shippingLabelResultsController and using order.shippingLabels instead.
- Update various files to incorporate shippingLabels property in Order creation and manipulation.
- Add Sendable conformance to ShippingLabel, ShippingLabelAddress, ShippingLabelRefund, ShippingLabelRefundStatus, and ShippingLabelStatus structs and enums for concurrency safety.
- Update tests in OrderDetailsDataSourceTests.swift to handle shippingLabels within Order instances.

Those were the only relationships with Order. In my opinion, we could potentially improve performance further by handling some fetches externally (e.g., fetching all order statuses). However, the current implementation is structured such that each view/model doesn't have visibility beyond the order itself. While this approach is possible, we need to discuss how deep we want to go with such changes. The current solution, however, does address the I/O problem effectively.

Steps to reproduce

Open multiple times different Order Details. You can notice a purple warning mentioning Performing 1/0 on the main thread by reading or writing to a database can cause slow launches. in ResultsController.

Testing information

You should not be able anymore to replicate the warning after these changes.

Make sure you are able to see “Custom amounts” section adding a fee, and that editing the fee update the fee title/amount.
Test that Shipping Labels in Order Details works like before if the plugin is installed: fetching/creation/update.
- Ensure the "Create Shipping Label" button is visible when the order is eligible for shipping label creation and has no existing labels.
- Ensure the "Create Shipping Label" button is not visible when the order is eligible but already has shipping labels.
- Ensure the "More" button is visible in the product section when the order is eligible and has no refunded shipping labels.
- Ensure the "More" button is visible in the product section when the order is eligible and has refunded shipping labels.
- Ensure the "More" button is not visible in the product section when the order is not eligible for shipping label creation.
- Ensure that refunded shipping labels are correctly identified and handled, and that the UI reflects this state appropriately.
- Confirm that the order is correctly marked as eligible for shipping label creation based on predefined criteria (e.g., order status, payment status).
- Ensure that any changes to the shipping labels (e.g., addition, refund) are immediately reflected in the UI without requiring a manual refresh.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…oved `feeLinesResultsController` and related logic

…ge, but not present inside the Networking model. Doing this to reduce number of `ResultsController` from `OrderDetailsResultsControllers`: - Update `Networking.generated.swift`, `Models+Copiable.generated.swift`, and `Order.swift` to include `shippingLabels` property in `Order` struct - Update `OrderDetailsResultsControllers.swift` by removing `shippingLabelResultsController` and using `order.shippingLabels` instead - Update various files to incorporate `shippingLabels` property in Order creation and manipulation - Add `Sendable` conformance to `ShippingLabel`, `ShippingLabelAddress`, `ShippingLabelRefund`, `ShippingLabelRefundStatus`, and `ShippingLabelStatus` structs and enums for concurrency safety - Update tests in `OrderDetailsDataSourceTests.swift` to handle `shippingLabels` within `Order` instances

…ch-fetching-in-order-details

wpmobilebot · 2025-01-28T16:08:40Z

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

	App Name	WooCommerce iOS
	Build Number	pr14999-85c0115
	Version	21.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`85c0115`
	App Center Build	WooCommerce - Prototype Builds #12813

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

selanthiraiyan · 2025-01-29T05:45:01Z

Thanks for working on this, @pmusolino!

Not directly related to this PR: While attempting to test this PR I noticed that the "Add Extension To Store" from the following screen doesn't work. I also noticed that the code is not linked to any action. Code link

Video recording:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-29.at.11.13.48.mp4

selanthiraiyan · 2025-01-29T05:55:43Z

I noticed one more issue that is probably not related to this PR.

I have installed WooCommerce Shipping on my JN store, but I still see the "Get WooCommerce Shipping" banner in the Order Details screen.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-29.at.11.22.44.mp4

I have tried restarting the app (stop and run again from Xcode), but the banner doesn't go away.

selanthiraiyan

The following issue happens in trunk as well and I don't think this is related to this PR. IMO, this looks related to this CUW post - peaMlT-Yo-p2

After creating a shipping label on my JN store I started facing issues with loading orders.

I tried emptying the orders creating Woo Smooth Generator from my JN store and kept only the order that I manually created from the mobile app. But still the mobile app fails to load that order and shows the following decoding error in logs.

⛔️ Error synchronizing orders: typeMismatch(Swift.Int64, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "data", intValue: nil), _JSONKey(stringValue: "Index 4", intValue: 4), CodingKeys(stringValue: "line_items", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "taxes", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "id", intValue: nil)], debugDescription: "Expected to decode Int64 but found a string instead.", underlyingError: nil))

My JN site has the following plugins installed.

Please let me know if you need an invitation to my JN site.

rachelmcr · 2025-01-29T10:56:42Z

Not directly related to this PR: While attempting to test this PR I noticed that the "Add Extension To Store" from the following screen doesn't work. I also noticed that the code is not linked to any action.

FWIW the "Get WooCommerce Shipping" banner with this install prompt is behind the shippingLabelsOnboardingM1 feature flag. I believe that was an older Kiwi project that never shipped; to avoid confusion it's probably worth removing the code or entirely disabling that feature flag.

selanthiraiyan · 2025-01-30T06:42:03Z

@pmusolino While trying to Refund a shipping label, I found that the "Request refund" button doesn't do anything.

selanthiraiyan

@pmusolino 👋

Quoting the following from the "Testing Information" in the PR description

Ensure that any changes to the shipping labels (e.g., addition, refund) are immediately reflected in the UI without requiring a manual refresh.

I noticed that the order detail page doesn't reflect the newly purchased shipping label unless I pull down to refresh. Even after I pull to refresh, I see the "Create Shipping Label" button, and it goes away after another pull down to refresh.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-30.at.12.13.22.mp4

pmusolino · 2025-01-30T12:01:11Z

I noticed one more issue that is probably not related to this PR.
I have installed WooCommerce Shipping on my JN store, but I still see the "Get WooCommerce Shipping" banner in the Order Details screen.

I filled an issue here about removing the feature flag.

pmusolino · 2025-01-30T12:03:07Z

@pmusolino While trying to Refund a shipping label, I found that the "Request refund" button doesn't do anything.

This doesn't work in trunk either because I think it's still a WIP under a feature flag. cc @rachelmcr

pmusolino · 2025-01-30T12:26:05Z

I noticed that the order detail page doesn't reflect the newly purchased shipping label unless I pull down to refresh. Even after I pull to refresh, I see the "Create Shipping Label" button, and it goes away after another pull down to refresh.

I tested it by creating the shipping label from the web, and it works; it's visible in the app when opening the order. I also tried creating the shipping label from the app. In this specific case, the refresh does not work as you mentioned, but it also doesn't refresh in the trunk branch. I've asked in C02KUCFCSFP/p1738239651479579 if this is part of a new flow, since I remember the UI for creating the shipping label was different a while back. In any case, it's not something related to this PR. 👍

selanthiraiyan · 2025-01-31T05:33:27Z

Networking/Networking/Model/Order.swift

@@ -239,6 +245,9 @@ public struct Order: Decodable, Sendable, GeneratedCopiable, GeneratedFakeable {
            return OrderAttributionInfo(metaData: allOrderMetaData)
        }()

+        // Shipping labels
+        let shippingLabels = try container.decodeIfPresent([ShippingLabel].self, forKey: .shippingLabels) ?? []


👋 @pmusolino Do we receive this shipping_labels key in orders response? Based on my testing in a Woo store with shipping labels, I don't see it being sent in wc/v3/orders.

If we do receive the shipping_labels, should we update the mock JSON and add unit tests to ensure that shipping_labels are mapped correctly?

Good question, @selanthiraiyan. In ShippingLabelStore, the method upsertShippingLabels connects the fetched shipping labels to the specific order, so the order doesn't return specific shipping labels, making it unnecessary to decode the Shipping Labels in the Order model. However, declaring the property (without decoding) in the networking model seems necessary to access shippingLabels, unless you have another solution in mind.

Yes, we could access the shipping labels from storage, but that's what we're trying to avoid. Do you think I can leave the shippingLabels property in Order.swift but remove the decoding? This way, by default, it would be an empty array.

Thanks, @pmusolino! I don't have a solution in mind. But I think we should remove the decoding, as it will be misleading.

I'm tagging @itsmeichigo to check whether she has any other solutions.

Thanks for the ping Sharma!

I think if the key for shipping labels is non-existent in the order response, keeping the property an empty array should be the way to go. There's no need to waste more resources to look for such a key to decode. Please also leave a comment in the code to explain why the value is empty by default, and when it's going to be updated.

However, I'm concerned about this part:

In ShippingLabelStore, the method upsertShippingLabels connects the fetched shipping labels to the specific order

In the order details screen, after syncing shipping labels, we'd need to ensure that the order in the view model is updated with the fetched label(s), just so they can be displayed on the UI.

Thank you both! I updated the decoding here 85c0115 the comment should be clear enough.

In the order details screen, after syncing shipping labels, we'd need to ensure that the order in the view model is updated with the fetched label(s), just so they can be displayed on the UI.

Yes, this already happens from my testing. If you create or update a shipping label, or open an order detail with an existing shipping label that needs to be fetched remotely, you will see the order detail UI correctly updated.

wpmobilebot · 2025-01-31T12:41:42Z

Version 21.6 has now entered code-freeze, so the milestone of this PR has been updated to 21.7.

- Modify `Order.swift` to initialize `shippingLabels` as an empty array by default. - Update RELEASE-NOTES.txt to correctly place and describe the improved data fetching change in version 21.7.

selanthiraiyan

@pmusolino I cannot see the purchased shipping label in the order detail screen. (Even after multiple pull to refresh attempts)

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-02-06.at.11.47.11.mp4

I tested by changing the Xcode scheme Build configuration into "Release" to ensure that we test the shipping labels work in production. (And not the WIP shipping labels project which is behind a feature flag)

pmusolino · 2025-02-06T12:00:49Z

@selanthiraiyan I was blocked from testing that flow because of this new issue #15078 but I believe you didn't encounter this issue.

pmusolino · 2025-02-06T14:10:23Z

@selanthiraiyan it seems to be an issue in trunk as well, and it also depends on which button you press at the end of the flow. If you press Print Shipping Label, the shipping label will appear in the order detail and on the web. However, if you press Save for later, I can't see or print the shipping label on mobile or on the web. The only thing visible on the web is the shipment tracking. In my tests, I always pressed "Print Shipping Label", but "Save for Later" seems to have a different behavior. I've pinged Eagle to understand if this is normal or an issue. C02KUCFCSFP/p1738850660636149

itsmeichigo · 2025-02-07T03:18:18Z

👋 @pmusolino - jumping in because I have a question regarding this line:

Those were the only relationships with Order.

I also found refunds as a relationship with order, maybe it should be cleaned up in this PR as well?

pmusolino · 2025-02-07T11:14:04Z

@itsmeichigo Order Refund has not been cleaned up, because the refunds relationship in Order detail is of type OrderRefundCondensed while the expected one in OrderDetailsResultsControllers is of type Refund, which includes additional fields like orderID, dateCreated, refundedByUserID, isAutomated, etc… OrderRefundCondensed is suitable for scenarios where only basic refund info is needed. So, we still need to fetch the refunds from Core Data.

rachelmcr · 2025-02-07T12:12:53Z

@pmusolino This isn't working well for me when I test with opening the order details for an order with a shipping label purchased on the web. Here's the scenario:

I've opened the order in the app before (so it exists in local storage).
I purchased a shipping label on the web.
I open the order in the app again and expect the shipping label to be there.

In trunk this works as expected:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-07.at.12.07.37.mp4

But in this branch, the shipping label shows up and then disappears. It only reappears after I pull to refresh:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-07.at.12.04.21.mp4

rachelmcr · 2025-02-07T12:16:02Z

If you press Print Shipping Label, the shipping label will appear in the order detail and on the web. However, if you press Save for later, I can't see or print the shipping label on mobile

FWIW I can't reproduce this behavior on my test store. Even if I select "Save for later," I see the purchased shipping label in order details in the app when I test in trunk:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-07.at.12.17.45.mp4

I tested the same steps in this branch and it also worked for me there.

wpmobilebot · 2025-02-07T13:24:24Z

Version 21.7 has now entered code-freeze, so the milestone of this PR has been updated to 21.8.

pmusolino added 4 commits January 27, 2025 17:47

update: feeLines property to return order.fees instead, while rem…

19def60

…oved `feeLinesResultsController` and related logic

Merge commit 'f12dbd8aeff195fe6da618d23550f9d8d6d24b7f' into feat/bat…

ec15b19

…ch-fetching-in-order-details

update: relese-notes

0311bd9

pmusolino added the feature: order details Related to order details. label Jan 28, 2025

pmusolino added this to the 21.6 milestone Jan 28, 2025

pmusolino requested review from selanthiraiyan and hichamboushaba January 28, 2025 15:54

update: add PR link to release-notes

0ef427b

selanthiraiyan self-assigned this Jan 29, 2025

selanthiraiyan reviewed Jan 29, 2025

View reviewed changes

pmusolino requested review from toupper and rachelmcr January 29, 2025 09:13

selanthiraiyan removed their assignment Jan 29, 2025

selanthiraiyan reviewed Jan 30, 2025

View reviewed changes

selanthiraiyan self-assigned this Jan 30, 2025

pmusolino mentioned this pull request Jan 30, 2025

Remove shippingLabelsOnboardingM1 feature flag #15019

Open

Merge branch 'trunk' into feat/batch-fetching-in-order-details

22d0a31

selanthiraiyan reviewed Jan 31, 2025

View reviewed changes

wpmobilebot modified the milestones: 21.6, 21.7 Jan 31, 2025

pmusolino added 3 commits February 3, 2025 15:55

Merge branch 'trunk' into feat/batch-fetching-in-order-details

e2d583f

Merge branch 'trunk' into feat/batch-fetching-in-order-details

bfc3695

Update Order.swift and RELEASE-NOTES.txt

85c0115

- Modify `Order.swift` to initialize `shippingLabels` as an empty array by default. - Update RELEASE-NOTES.txt to correctly place and describe the improved data fetching change in version 21.7.

selanthiraiyan reviewed Feb 6, 2025

View reviewed changes

wpmobilebot modified the milestones: 21.7, 21.8 Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

pmusolino commented Jan 28, 2025

wpmobilebot commented Jan 28, 2025 •

edited

Loading

selanthiraiyan commented Jan 29, 2025

selanthiraiyan commented Jan 29, 2025 •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

rachelmcr commented Jan 29, 2025

selanthiraiyan commented Jan 30, 2025

selanthiraiyan left a comment •

edited

Loading

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

selanthiraiyan Jan 31, 2025

pmusolino Feb 3, 2025

selanthiraiyan Feb 4, 2025

itsmeichigo Feb 4, 2025 •

edited

Loading

pmusolino Feb 5, 2025

wpmobilebot commented Jan 31, 2025

selanthiraiyan left a comment

pmusolino commented Feb 6, 2025

pmusolino commented Feb 6, 2025 •

edited

Loading

itsmeichigo commented Feb 7, 2025

pmusolino commented Feb 7, 2025

rachelmcr commented Feb 7, 2025 •

edited

Loading

rachelmcr commented Feb 7, 2025 •

edited

Loading

wpmobilebot commented Feb 7, 2025

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Are you sure you want to change the base?

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Conversation

pmusolino commented Jan 28, 2025

Description

Steps to reproduce

Testing information

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Jan 28, 2025 • edited Loading

selanthiraiyan commented Jan 29, 2025

selanthiraiyan commented Jan 29, 2025 • edited Loading

selanthiraiyan left a comment • edited Loading

Choose a reason for hiding this comment

rachelmcr commented Jan 29, 2025

selanthiraiyan commented Jan 30, 2025

selanthiraiyan left a comment • edited Loading

Choose a reason for hiding this comment

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

selanthiraiyan Jan 31, 2025

Choose a reason for hiding this comment

pmusolino Feb 3, 2025

Choose a reason for hiding this comment

selanthiraiyan Feb 4, 2025

Choose a reason for hiding this comment

itsmeichigo Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

pmusolino Feb 5, 2025

Choose a reason for hiding this comment

wpmobilebot commented Jan 31, 2025

selanthiraiyan left a comment

Choose a reason for hiding this comment

pmusolino commented Feb 6, 2025

pmusolino commented Feb 6, 2025 • edited Loading

itsmeichigo commented Feb 7, 2025

pmusolino commented Feb 7, 2025

rachelmcr commented Feb 7, 2025 • edited Loading

rachelmcr commented Feb 7, 2025 • edited Loading

wpmobilebot commented Feb 7, 2025

wpmobilebot commented Jan 28, 2025 •

edited

Loading

selanthiraiyan commented Jan 29, 2025 •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

itsmeichigo Feb 4, 2025 •

edited

Loading

pmusolino commented Feb 6, 2025 •

edited

Loading

rachelmcr commented Feb 7, 2025 •

edited

Loading

rachelmcr commented Feb 7, 2025 •

edited

Loading