Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,10 @@ private struct WooShippingConfigMapperEnvelope: Decodable {
extension WooShippingConfigMapper {
/// Load only the relevant fields from remote
///
static let fieldsToLoad = "config.shipments, config.shippingLabelData.currentOrderLabels, config.shippingLabelData.storedData.selected_destination"
static let fieldsToLoad = [
"config.shipments",
"config.shippingLabelData.currentOrderLabels",
"config.shippingLabelData.storedData.selected_destination",
"config.shippingLabelData.storedData.selected_origin"
].joined(separator: ", ")
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,16 @@ public struct WooShippingLabelData: Decodable, Equatable {

/// Inject destination addresses into labels if present
let orderLabels: [ShippingLabel]
if let destinations = storedData?.selectedDestinations, !destinations.isEmpty {
orderLabels = WooShippingLabelData.mapDestinations(destinations, into: decodedOrderLabels)

let destinations = storedData?.selectedDestinations
let origins = storedData?.selectedOrigins

if destinations?.isEmpty == false || origins?.isEmpty == false {
orderLabels = WooShippingLabelData.mapAddresses(
origins: origins,
destinations: destinations,
into: decodedOrderLabels
)
} else {
orderLabels = decodedOrderLabels
}
Expand All @@ -98,21 +106,27 @@ public struct WooShippingLabelData: Decodable, Equatable {
}

public extension WooShippingLabelData {
typealias WooShippingLabelDestinations = [String: WooShippingAddress]
typealias WooShippingLabelAddressMap = [String: WooShippingAddress]

struct StoredData: Decodable, Equatable {
let selectedDestinations: WooShippingLabelDestinations?
let selectedDestinations: WooShippingLabelAddressMap?
let selectedOrigins: WooShippingLabelAddressMap?

public enum CodingKeys: String, CodingKey {
case selectedDestination = "selected_destination"
case selectedOrigin = "selected_origin"
}

public init(from decoder: any Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
self.selectedDestinations = try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
selectedDestinations = try? container.decodeIfPresent(
WooShippingLabelAddressMap.self,
forKey: CodingKeys.selectedDestination
)
selectedOrigins = try? container.decodeIfPresent(
WooShippingLabelAddressMap.self,
forKey: CodingKeys.selectedOrigin
)
Comment on lines +126 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider applying the same fix as suggested here to support the array case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
import Foundation

extension WooShippingLabelData {
static func mapDestinations(
_ destinations: WooShippingLabelDestinations,
static func mapAddresses(
origins: WooShippingLabelAddressMap?,
destinations: WooShippingLabelAddressMap?,
into labels: [ShippingLabel]
) -> [ShippingLabel] {
return labels.map { label in
guard
let shipmentID = label.shipmentID,
let destinationAddress = destinations[
WooShippingShipmentIDFormatter.formattedShipmentID(shipmentID)
] ?? destinations[shipmentID] /// Fallback for ids previously submitted without `shipment_<id>` formatting
else {
guard let shipmentID = label.shipmentID else {
return label
}

return label.copy(destinationAddress: destinationAddress.toShippingLabelAddress())
let formattedID = WooShippingShipmentIDFormatter.formattedShipmentID(shipmentID)
let originAddress = origins?[formattedID] ?? origins?[shipmentID]
let destinationAddress = destinations?[formattedID] ?? destinations?[shipmentID]

return label.copy(
originAddress: originAddress?.toShippingLabelAddress() ?? .copy,
destinationAddress: destinationAddress?.toShippingLabelAddress() ?? .copy
)
}
}
}
16 changes: 10 additions & 6 deletions Modules/Tests/NetworkingTests/Remote/WooShippingRemoteTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -857,10 +857,10 @@ final class WooShippingRemoteTests: XCTestCase {

// MARK: - Load Config With Destinations

func test_loadConfig_withDestinations_parses_success_response() throws {
func test_load_config_with_addresses_parses_success_response() throws {
// Given
let remote = WooShippingRemote(network: network)
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-with-destinations")
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-with-addresses")

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
Expand All @@ -874,12 +874,14 @@ final class WooShippingRemoteTests: XCTestCase {
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertNotNil(label.destinationAddress)
XCTAssertEqual(label.destinationAddress.address1, "200 N SPRING ST")
XCTAssertNotNil(label.originAddress)
XCTAssertEqual(label.originAddress.address1, "Test origin address line")
}

func test_loadConfig_withoutDestinations_parses_success_response() throws {
func test_load_config_without_addresses_parses_success_response() throws {
// Given
let remote = WooShippingRemote(network: network)
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-without-destinations")
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-without-addresses")

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
Expand All @@ -892,12 +894,13 @@ final class WooShippingRemoteTests: XCTestCase {
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertTrue(label.destinationAddress.isEmpty)
XCTAssertTrue(label.originAddress.isEmpty)
}

func test_loadConfig_withInvalidDestinations_parses_success_response() throws {
func test_load_config_with_invalid_addresses_parses_success_response() throws {
// Given
let remote = WooShippingRemote(network: network)
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-with-invalid-destinations")
network.simulateResponse(requestUrlSuffix: "config/label-purchase/\(sampleOrderID)", filename: "wooshipping-config-with-invalid-addresses")

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
Expand All @@ -910,6 +913,7 @@ final class WooShippingRemoteTests: XCTestCase {
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertTrue(label.destinationAddress.isEmpty)
XCTAssertTrue(label.originAddress.isEmpty)
}

// MARK: Update shipment
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
"postcode": "90012-4801",
"country": "US"
}
},
"selected_origin": {
"shipment_1": {
"address_1": "Test origin address line",
"city": "Test origin city",
"state": "CA",
"postcode": "90012-4801",
"country": "US"
}
}
},
"currentOrderLabels": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
{
"address_1": "200 N SPRING ST"
}
],
"selected_origin": [
{
"address_1": "Test origin address line"
}
]
},
"currentOrderLabels": [
Expand Down
2 changes: 1 addition & 1 deletion RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

22.7
-----
- [*] Shipping Labels: Fixed an issue where the destination address was missing for previously purchased shipping labels. [https://github.com/woocommerce/woocommerce-ios/pull/15866]
- [*] Shipping Labels: Fixed an issue where the origin and destination addresses were missing for previously purchased shipping labels. [https://github.com/woocommerce/woocommerce-ios/pull/15866]
- [*] Shipping Labels: Fixed an issue where the purchase button would display a stale price after changing the origin address. [https://github.com/woocommerce/woocommerce-ios/pull/15795]
- [*] 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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,13 @@ private extension WooShippingCreateLabelsView {
Text(Localization.BottomSheet.shipFrom)
.trackSize(size: $shipmentDetailsShipFromSize)

if viewModel.canViewLabel,
let addressLines = viewModel.originAddressLines {
AddressLinesView(addressLines: addressLines)
.frame(maxWidth: .infinity, alignment: .leading)
if viewModel.canViewLabel {
AddressLinesView(
addressLines: viewModel.originAddressLines ??
/// Fallback to `originAddress` in case if `originAddressLines` are missing
[viewModel.originAddress]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned that this fallback may cause the app to display incorrect data. Do you think we can remove the workaround if we add support for the origin address array as suggested above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong preference here.

It would be either a blank space or the viewModel.originAddress fallback if selected_origin addresses aren't mapped for some reason. And it seems to be super rare for potential edge cases where a fallback wouldn't work.

I checked - web does present just a blank space for this case. But for the destination web does a fallback to the order destination address if the shipping label destination is missing.

@itsmeichigo Are we ok to leave a blank space there?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with having a blank space, it's easier to know that there's an issue than having incorrect data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 548904a

)
.frame(maxWidth: .infinity, alignment: .leading)
} else {
VStack(alignment: .leading) {
Button {
Expand Down