Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Aug 6, 2025

For WOOMOB-981

Just one review is required.

Description

Updates OrdersRemote.loadAllOrders and OrdersRemote.searchOrders methods to use modern async/await interface instead of completion handlers. This change improves response handling by ensuring it doesn't occur on the main thread and follows current Swift concurrency best practices. These two methods are the only ones in OrdersRemote that fetch and parse a list of orders, which include meta_data field that can include any number of values resulting in a large response size.

The updated methods now:

  • Use async throws instead of completion handlers which parse the response on the main thread
  • Integrate with existing async/await patterns in the codebase

All call sites have been updated to maintain compatibility:

  • OrderStore methods use Task blocks with @MainActor for thread safety
  • Tests have been converted to async/await syntax
  • UI components maintain their existing interfaces

Steps to reproduce

  • Feel free to put a breakpoint in Order.swift init(from:)
  • Go to the Orders tab --> the breakpoint in Order.swift should not be on the main thread
  • Disable the breakpoint
  • Pull to refresh --> the UI should be smooth while orders are being loaded in the background
  • Search for orders --> the UI should be smooth while orders are being loaded in the background

Testing information

I tested on iPad A16 iOS 18.4 simulator.


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

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 6, 2025

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

App NameWooCommerce iOS Prototype
Build Numberpr15982-8377aba
Version22.9
Bundle IDcom.automattic.alpha.woocommerce
Commit8377aba
Installation URL7n513n1naqp7g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync added type: task An internally driven task. feature: order list Related to the order list. labels Aug 6, 2025
@jaclync jaclync added this to the 23.0 milestone Aug 6, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 6, 2025

1 Warning
⚠️ This PR is assigned to the milestone 23.0. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@jaclync jaclync changed the title Update OrdersRemote methods to use async/await interface Update OrdersRemote with async/throws network methods to parse orders response in a background thread Aug 6, 2025
@iamgabrielma iamgabrielma self-assigned this Aug 6, 2025
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

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

Works great! Thanks for your work modernizing those.

I left a couple of nits, feel free to address them or not 👍

switch result {
case .success(let orders):
self?.upsertStoredOrdersInBackground(readOnlyOrders: orders) {
Task { @MainActor in
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The alignment of these props seems to be off!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the alignment in 8377aba.

/// Verifies that loadAllOrders properly parses the `orders-load-all` sample response.
///
func testLoadAllOrdersProperlyReturnsParsedOrders() throws {
func testLoadAllOrdersProperlyReturnsParsedOrders() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since we're updating the test signature we can update the test name to use snake case as well 🐍 , ideally with when/then too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated in aa00e4c.

@jaclync jaclync force-pushed the backlog/WOOMOB-981-orders-remote-async-await branch from e9e1be6 to aa00e4c Compare August 7, 2025 05:57
@jaclync jaclync enabled auto-merge August 7, 2025 06:19
@jaclync jaclync merged commit 24ed98a into trunk Aug 7, 2025
13 checks passed
@jaclync jaclync deleted the backlog/WOOMOB-981-orders-remote-async-await branch August 7, 2025 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: order list Related to the order list. type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants