Skip to content

Order details: Fix crash when reloading orders #15764

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 19, 2025
Merged

Order details: Fix crash when reloading orders #15764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
530c10f
Freeze all data from results controller before reloading order details
itsmeichigo Jun 17, 2025
a308394
Update release notes
itsmeichigo Jun 17, 2025
dc7f0dc
Freeze product variations
itsmeichigo Jun 18, 2025
1737b8e
Merge branch 'trunk' into fix/woomob-604-crash-in-order-details-order…
itsmeichigo Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

22.7
-----
- [*] Order Details: Fix crash when reloading data [https://github.com/woocommerce/woocommerce-ios/pull/15764]
- [*] Shipping Labels: Improved shipment management UI by hiding remove/merge options instead of disabling them, hiding merge option for orders with 2 or fewer unfulfilled shipments, and hiding the ellipsis menu when no remove/merge actions are available [https://github.com/woocommerce/woocommerce-ios/pull/15760]
- [**] POS: a POS tab in the tab bar is now available in the app for stores eligible for Point of Sale, instead of the previous entry point in the Menu tab. [https://github.com/woocommerce/woocommerce-ios/pull/15766]

Expand Down
44 changes: 21 additions & 23 deletions WooCommerce/Classes/ViewModels/Order Details/OrderDetailsDataSource.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,27 +136,23 @@ final class OrderDetailsDataSource: NSObject {

/// Order shipment tracking list
///
var orderTracking: [ShipmentTracking] {
return resultsControllers.orderTracking
}
var orderTracking: [ShipmentTracking] = []

/// Order statuses list
///
var currentSiteStatuses: [OrderStatus] {
return resultsControllers.currentSiteStatuses
}
var currentSiteStatuses: [OrderStatus] = []

/// Products from an Order
///
var products: [Product] {
return resultsControllers.products
}
var products: [Product] = []

/// Product variations from an order
///
var productVariations: [ProductVariation] = []

/// Custom amounts (fees) from an Order
///
var customAmounts: [OrderFeeLine] {
return resultsControllers.feeLines
}
var customAmounts: [OrderFeeLine] = []

/// OrderItemsRefund Count
///
Expand All @@ -166,19 +162,13 @@ final class OrderDetailsDataSource: NSObject {

/// Refunds on an Order
///
var refunds: [Refund] {
return resultsControllers.refunds
}
var refunds: [Refund] = []

var addOnGroups: [AddOnGroup] {
resultsControllers.addOnGroups
}
var addOnGroups: [AddOnGroup] = []

/// Shipping Methods list
///
var siteShippingMethods: [ShippingMethod] {
resultsControllers.siteShippingMethods
}
var siteShippingMethods: [ShippingMethod] = []

/// Shipping Labels for an Order
///
Expand Down Expand Up @@ -1171,7 +1161,7 @@ extension OrderDetailsDataSource {
}

private func lookUpProductVariation(productID: Int64, variationID: Int64) -> ProductVariation? {
return resultsControllers.productVariations.filter({ $0.productID == productID && $0.productVariationID == variationID }).first
return productVariations.filter({ $0.productID == productID && $0.productVariationID == variationID }).first
}

func lookUpRefund(by refundID: Int64) -> Refund? {
Expand All @@ -1195,12 +1185,20 @@ extension OrderDetailsDataSource {
@MainActor
func reloadSections() async {
// Freezes any data that require lookup after the sections are reloaded, in case the data from a ResultsController changes before the next reload.
refunds = resultsControllers.refunds
customAmounts = resultsControllers.feeLines
orderTracking = resultsControllers.orderTracking
currentSiteStatuses = resultsControllers.currentSiteStatuses
products = resultsControllers.products
addOnGroups = resultsControllers.addOnGroups
siteShippingMethods = resultsControllers.siteShippingMethods
Comment on lines +1188 to +1194
Copy link
Contributor

@jaclync jaclync Jun 18, 2025

Choose a reason for hiding this comment

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

How about also freezing order (and its items among other properties used in the sections) in case the order changes in the middle?

Copy link
Contributor

@jaclync jaclync Jun 18, 2025

Choose a reason for hiding this comment

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

This probably requires some refactoring, but I was wondering if a crash could still happen when there are two reloadSections at the same time, and because they're async, the second call can overwrite the variables and the first call can use the updated values later on? If we want to freeze the values, it might be better passing them to functions called in reloadSections that depend on them instead of saving them as variables so that these values aren't mutable for the whole reload function.

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jun 18, 2025

Choose a reason for hiding this comment

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

I'm struggling to understand your suggestions here, so it'd be nice if you could clarify your suggestions further.

From my understanding, it'd be simpler if the reloadSections method were synchronous - that way, every reload would be subsequent and more predictable. Another refactoring idea is to udpate createPaymentSection to be synchronous and adjust the payment section when the receipt row is available. That could be simpler - WDYT?

Copy link
Contributor Author

@itsmeichigo itsmeichigo Jun 18, 2025

Choose a reason for hiding this comment

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

If we want to freeze the values, it might be better passing them to functions called in reloadSections that depend on them instead of saving them as variables so that these values aren't mutable for the whole reload function.

The variables are also accessed when configuring cells, so it cannot be a simple as passing them through to determine sections.

productVariations = resultsControllers.productVariations
shippingLabels = resultsControllers.shippingLabels
shippingLabelOrderItemsAggregator = AggregatedShippingLabelOrderItems(
shippingLabels: shippingLabels,
orderItems: items,
products: products,
productVariations: resultsControllers.productVariations
productVariations: productVariations
)

var sections = buildStaticSections().compactMap { $0 }
Expand Down