Order Details: Load simplified objects for products

[itsmeichigo]

Part of WOOMOB-619

Description

Xcode Organizer has been reporting Product.toReadOnly as the main source of hangs in recent app versions, especially on the order details screen:

This PR attempts to improve performance when loading data on the order details screen by introducing a lightweight version of readonly product objects. Detailed changes:

• Adds a new protocol ListItemConvertible to convert storage objects to simple immutable objects that are suitable for displaying on list views.
• Create a new type ProductListItem to include only basic properties for displaying product lists.
• Make Storage.Product conform to ListItemConvertible to return list items/
• Update order details screen to work with ProductListItem instead of the full Product objects.
• Update order details results controller to only trigger fetched items initially and when there are changes in storage.
• Update the product fetch on order details to only relevant items to the order.

I'll replace the redundant use of the full Product objects in other parts of the app on subsequent PRs.

Testing steps

1. Clean build the app.
2. Navigate to the orders tab and open any order.
3. Confirm that order details are still correct (products, shipping labels, refunds, add-ons etc.)

For some reason I still see the purple warning at dequeueReusableCell line in OrderDetailsDataSource. I suppose this is an issue with Xcode and hope to see more accurate reports in Xcode Organizer's Hang section if this code gets merged.

Testing information

Tested order details screen on both simulator and iPhone 16 Pro.

Screenshots

No UI changes.

---

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

[wpmobilebot]

📲 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 Name	WooCommerce iOS Prototype
	Build Number	pr15959-d27eb7b
	Version	22.9
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	d27eb7b
	Installation URL	46bbqsq9olk88

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

[RafaelKayumov]

looks like AggregateOrderItem assembly logic is similar for .product and .productListItem cases. How about unifing it and making reusable:

...
case let .productListItem(item, orderItem, name):
    return aggregateOrderItem(
        quantity: quantity,
        price: item.price,
        images: item.images,
        productID: item.productID,
        sku: orderItem?.sku ?? item.sku,
        orderItem: orderItem,
        name: name
    )
 case .product(let product, let orderItem, let name):
    return aggregateOrderItem(
        quantity: quantity,
        price: product.price,
        images: product.images,
        productID: product.productID,
        sku: orderItem?.sku ?? product.sku,
        orderItem: orderItem,
        name: name
    )
...

func aggregateOrderItem(
        quantity: Int,
        price: String,
        images: [ProductImage],
        productID: Int64,
        sku: String?,
        orderItem: OrderItem?,
        name: String
    ) -> AggregateOrderItem {
        let itemID = orderItem?.itemID.description ?? "0"
        let productName = orderItem?.name ?? name
        let price = orderItem?.price ??
            currencyFormatter.convertToDecimal(price) ?? 0
        let totalPrice = price.multiplying(by: .init(decimal: Decimal(quantity)))
        let imageURL: URL?
        if let encodedImageURLString = images.first?.src.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) {
            imageURL = URL(string: encodedImageURLString)
        } else {
            imageURL = nil
        }
        return .init(itemID: itemID,
                     productID: productID,
                     variationID: 0,
                     name: productName,
                     price: price,
                     quantity: Decimal(quantity),
                     sku: orderItem?.sku ?? sku,
                     total: totalPrice,
                     imageURL: imageURL,
                     attributes: orderItem?.attributes ?? [],
                     addOns: orderItem?.addOns ?? [],
                     parent: orderItem?.parent)
    }

[itsmeichigo]

It looks like the .product case is no longer used, so I removed it in 0f31c1d.

[RafaelKayumov]

I have a concern about the calculated listItemObjects: [T.ListItemType] since it performs re-initialization of the each ListConvertible instance on every access. For example if we need to reload just one cell or do some other minor check - the whole collection will be recreated.

I think it's fine to keep this approach in scope of this PR since ResultsController.fetchedObjects does the same thing. It re-creates the readonly objects every time when accessed. But generally I think we should maintain the readOnly/ListItem objects cache within ResultsController instance and update it correspondingly (on data change callbacks) to keep the relevant snapshot of readOnly / List item objects. WDYT?

I also have an assumption that the actual hang culprit might be not the heavy relationships in readOnly object creation, but the amount of calls. I made a dummy print log before mutableObject.toListItem() and before mutableObject.toReadOnly(). I get hundreds of logs even if I just open an order details screen.

[itsmeichigo]

I think it's fine to keep this approach in scope of this PR since ResultsController.fetchedObjects does the same thing. It re-creates the readonly objects every time when accessed.

Thanks for sharing this point of view - I agree with you. I think this comes down to the limitation of using extensions. listItemObjects is forced to be computed variables as it's only a part of ResultsController's extension and cannot be stored properties itself.

We can mitigate this limitation by avoiding calling fetchedObjects and listItemObjects in computed variables. Unfortunately I don't think there's a way to assert that in code (that I know of). I added comments in f2fadad.

But generally I think we should maintain the readOnly/ListItem objects cache within ResultsController instance and update it correspondingly (on data change callbacks) to keep the relevant snapshot of readOnly / List item objects. WDYT?

I think keeping all the objects in the memory could have its drawbacks as well for devices with low RAM capacity. The idea of keeping cache of a cache sounds odd to me as well.

I also have an assumption that the actual hang culprit might be not the heavy relationships in readOnly object creation, but the amount of calls. I made a dummy print log before mutableObject.toListItem() and before mutableObject.toReadOnly(). I get hundreds of logs even if I just open an order details screen.

I'd say that the hang is a combination of both the storage object conversions and the number of calls. I added changes in cfaaff8 to avoid triggering the conversions on every reload for order details. I believe this would reduce the number of calls - would you mind check again to confirm please @RafaelKayumov? 🙏

[itsmeichigo]

I also added a change in 98f92c7 to fetch only relevant products in order details.

[itsmeichigo]

Thank you Rafael for the reviews. I'll proceed to merge this PR later today if I don't receive comments from other folks by the end of my day.

[jaclync]

👋 Taking a look today, I think ResultsController could support a more generic output type that doesn't have to be either T.ReadOnlyType or the new T.ListItemType in the PR. I will experiment a bit.

[itsmeichigo]

Thank you @jaclync for the suggestions! I merged changes in your branch and added improvements following your feedback. Please take another look at this PR when you have the time!

[jaclync]

Thanks a lot for the updates, look great to me! Tested the Orders and Products tabs, PTR and checking the order/product details, and didn't see the Product.toReadOnly warnings that originated from OrderDetailsDataSource anymore. Now I see 3 hangs warnings, and 2 of them on Product.toReadOnly I believe you're addressing in #15966:

• ProductAttribute.toReadOnly() ... Product.toReadOnly() from ReviewsDashboardCardViewModel:

• ProductAttribute.toReadOnly() ... Product.toReadOnly() from BlazeCampaignDashboardViewModel:

• After visiting the Orders tab > order details: OrderDetailsDataSource.tableView(_:cellForRowAt:):

[jaclync]

nit: this seems to be used only in unit tests, maybe it can be moved to the tests module?

[jaclync]

super nit:

Suggested change

	/// Represents a Product Entity with basic details to display in the product list.
	/// Represents a Product Entity with basic details to display in the order details product list.

[jaclync]

nit: any reasons having two of the same lines?

Suggested change

	self.productResultsController = createProductResultsController()
	self.productResultsController = createProductResultsController()
	self.productResultsController = createProductResultsController()

[itsmeichigo]

Thank you, this was added by mistake.

[itsmeichigo]

@jaclync While trying to update #15966 following changes in this PR, I found limitations with our current solution as the product list needs the simplified objects to display on the list and the full objects for bulk editing.

I restored our original implementation of ResultsController in 7e65cbb while adding the options to fetch special transformed objects as generic functions instead. This gives more flexibility when using ResultsController. WDYT?

[jaclync]

While trying to update #15966 following changes in this PR, I found limitations with our current solution as the product list needs the simplified objects to display on the list and the full objects for bulk editing.

Could you share the code ref for the use case where the full Product is needed for bulk editing? I thought the bulk editing is mostly for making an API request to update multiple products, which probably just requires a product ID - just my impression though.

[itsmeichigo]

@jaclync Here you go:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Products/ProductsListViewModel.swift

Line 153 in fe2f5ac

let batchAction = ProductAction.updateProducts(siteID: siteID, products: updatedProducts) { result in

This goes all the way to our implementation in the Networking layer, so it would require a lot of changes if we want to change the type sent to bulk editing. I've updated #15966 to keep everything as-is on the product list and only update the transform type for display items.

[jaclync]

Here you go:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Products/ProductsListViewModel.swift

Line 153 in fe2f5ac

let batchAction = ProductAction.updateProducts(siteID: siteID, products: updatedProducts) { result in

This goes all the way to our implementation in the Networking layer, so it would require a lot of changes if we want to change the type sent to bulk editing. I've updated #15966 to keep everything as-is on the product list and only update the transform type for display items.

Thanks for sharing this. The current batch products update remote implementation to send all product fields for each product seems not so performant, as the relationship fields can have an unlimited number of objects that are unrelated to the batch update. From the API doc on products batch update, it seems like just the updated field needs to be set in the request body. Personally, I'd fix this use case first: passing a list of product IDs and the one field to update (could be an enum) with testing.

The additional transformedObjects/transformedObject approach works, but the sections is still using the full objects and it's easy to use the full objects (with a note in the comment). Trading a more generic API for a use case that could be refactored feels not so ideal, but just my personal thoughts. Please feel free to move forward as you/Kiwi think best. @joshheald is back from AFK today, if he has some time to take a look.

[itsmeichigo]

The current batch products update remote implementation to send all product fields for each product seems not so performant, as the relationship fields can have an unlimited number of objects that are unrelated to the batch update. From the API doc on products batch update, it seems like just the updated field needs to be set in the request body. Personally, I'd fix this use case first: passing a list of product IDs and the one field to update (could be an enum) with testing.

I agree that this would make it better for the feature, but let's try to focus on the problem we want to solve first. If you need, we can add a backlog issue to optimize batch product update, but that's irrelevant for the issue we're working on.

The additional transformedObjects/transformedObject approach works, but the sections is still using the full objects and it's easy to use the full objects (with a note in the comment). Trading a more generic API for a use case that could be refactored feels not so ideal

With this solution, the results controller still works the way it always does before, so I'd say this is expected. The transformedObject function is only there to help if we don't want to work with the full objects. When we need to work with sections without full objects, we can always add another helper method for sections of transformed objects.

@jaclync How about restoring your solution but keeping these transformed object helpers as a workaround for the product list? That way we can avoid having to do the refactoring for batch product update and can keep the results controller generic for other use cases.

[dangermattic]

	2 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	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

[itsmeichigo]

☝️ I reverted the change to get this PR done and make everyone happy. I'll merge this once I get a nod from @joshheald.

[RafaelKayumov]

LGTM

I think we could take more advantage of Swift generics here. When we use a generic GenericResultsController - we are supposed to specify the Core Data type and an immutable / lightweight type as generic constraints.
The lightweight type can always be matched to a corresponding Core Data object type. For example if we fetch OrderDetailsProduct, the Core Data type will certainly be the Storage.Product and so on... Therefore I think we could improve these points:

• GenericResultsController requires to specify 2 generic constraints
• GenericResultsController allows to specify 2 unrelated types as generic constraints
• We manually pass the transform logic

The approach I'm proposing:

/// The read-only convertibly Core Data object type
public typealias ResultsControllerMutableType = NSManagedObject & ReadOnlyConvertible

/// The lightweight object protocol where the associated type is the Core Data object declared above
public protocol MutableResultMappable {
    associatedtype MutableResultType: ResultsControllerMutableType
    
    /// Initializer from the Core Data object
    init(mutableObject: MutableResultType) 
}

public class GenericResultsController<T: MutableResultMappable> {
    ...
}

This is the example of "lightweight" object definition that conforms to the MutableResultMappable and defines the assiciated "Core Data" object type:

extension OrderDetailsProduct: MutableResultMappable {
    typealias MutableResultType = StorageProduct
    ...
}

With such approach we could use the GenericResultsController just with one constraint: GenericResultsController<OrderDetailsProduct>(...). We could also omit passing the manual transform for each case and utilize init(mutableObject: MutableResultType) which is common for all "lightweight" MutableResultMappable objects.

Though I haven't yet figured out how to use this to declare ResultsController by subclassing the GenericResultsController. We could look into it if the approach looks valid. WDYT?

[itsmeichigo]

Thank you @RafaelKayumov for the suggestion.

Looking at the protocol MutableResultMappable, it requires the associated type to conform to ReadOnlyConvertible. If I understand correctly, this means OrderDetailsProduct would need to be in charge of the conversion from the storage objects to the read-only Product objects.

This responsibility currently belongs to the storage objects, so if we go ahead with this, would we need to refactor the conformance? Would we need to do the same for other lightweight types of Product?

[RafaelKayumov]

Thank you @RafaelKayumov for the suggestion.

Looking at the protocol MutableResultMappable, it requires the associated type to conform to ReadOnlyConvertible. If I understand correctly, this means OrderDetailsProduct would need to be in charge of the conversion from the storage objects to the read-only Product objects.

This responsibility currently belongs to the storage objects, so if we go ahead with this, would we need to refactor the conformance? Would we need to do the same for other lightweight types of Product?

No, the associated type in that case would be the Storage.Product and that's already conforms to ReadOnlyConvertible

[itsmeichigo]

@RafaelKayumov Thanks for the clarification. I tried out your suggestion and got stuck with declaring ResultsController for backward compatibility too.

For now let's keep the current solution and revise it when we stumble upon any limitation while using the generic results controller.

[RafaelKayumov]

For now let's keep the current solution and revise it when we stumble upon any limitation

Sounds good

[itsmeichigo]

I'm merging since we've got 2 approvals already and we're approaching the code freeze.

[joshheald]

@itsmeichigo I'm catching up, so sorry this is after the merge. Kudos for addressing the hangs.

With this approach, will we ever write the lightweight products to the database? If it does, we should avoid that, because Core Data won't catch the fact that we're missing a load of fields and it'll lead to strange behaviour at different times in the app.

If it won't ever do that, no concerns at all

[itsmeichigo]

With this approach, will we ever write the lightweight products to the database? If it does, we should avoid that, because Core Data won't catch the fact that we're missing a load of fields and it'll lead to strange behaviour at different times in the app.

Hi @joshheald - the current design and implementation use custom models created on the UI layer to transform data from the storage objects. They are read-only and not used to update the database. The database is only updated as part of upsertion operations in the Yosemite layer like it has always been. Thank you for sharing the concern.