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 2 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]


22.6
Expand Down
35 changes: 14 additions & 21 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,19 @@ 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] = []

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

/// OrderItemsRefund Count
///
Expand All @@ -166,19 +158,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 @@ -1202,6 +1188,13 @@ extension OrderDetailsDataSource {
products: products,
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 is this products different from the products below in L1195? Shall we use the same products in the beginning of the 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.

Good point - addressed in dc7f0dc.

productVariations: resultsControllers.productVariations
)
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.


var sections = buildStaticSections().compactMap { $0 }
let paymentSection = await createPaymentSection()
Expand Down