Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ private struct WooShippingConfigMapperEnvelope: Decodable {
extension WooShippingConfigMapper {
/// Load only the relevant fields from remote
///
static let fieldsToLoad = "config.shipments, config.shippingLabelData.currentOrderLabels"
static let fieldsToLoad = "config.shipments, config.shippingLabelData.currentOrderLabels, config.shippingLabelData.storedData.selected_destination"
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ extension WooShippingPackagePurchase {

/// shipment ID to set for hazmat and customs form
var formattedShipmentID: String {
Values.shipmentIDPrefix + shipmentID
return WooShippingShipmentIDFormatter.formattedShipmentID(shipmentID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Values.shipmentIDPrefix is no longer used and should be removed from this file.

}
}

Expand Down Expand Up @@ -219,6 +219,5 @@ extension WooShippingPackagePurchase: Encodable {
static let adult = "adult"
static let signatureRequired = "signatureRequired"
static let adultSignatureRequired = "adultSignatureRequired"
static let shipmentIDPrefix = "shipment_"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,60 @@ public struct WooShippingLabelData: Decodable, Equatable {
/// Labels purchased for the current order
public let currentOrderLabels: [ShippingLabel]

public init(currentOrderLabels: [ShippingLabel]) {
/// Contains destination addresses
public let storedData: StoredData?

public init(
currentOrderLabels: [ShippingLabel],
storedData: StoredData? = nil
) {
self.currentOrderLabels = currentOrderLabels
self.storedData = storedData
}

public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
let currentOrderLabels = try container.decodeIfPresent([ShippingLabel].self, forKey: .currentOrderLabels) ?? []
self.init(currentOrderLabels: currentOrderLabels)

let storedData = try container.decodeIfPresent(StoredData.self, forKey: .storedData)
let decodedOrderLabels = try container.decodeIfPresent([ShippingLabel].self, forKey: .currentOrderLabels) ?? []

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

self.init(
currentOrderLabels: orderLabels,
storedData: storedData
)
}

private enum CodingKeys: String, CodingKey {
case currentOrderLabels
case storedData
}
}

public extension WooShippingLabelData {
typealias WooShippingLabelDestinations = [String: WooShippingAddress]
Copy link
Contributor

@itsmeichigo itsmeichigo Jul 4, 2025

Choose a reason for hiding this comment

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

❓ Here the destinations are expected to be a dictionary - but for users who created labels with invalid shipment IDs for hazmat and customs, they get an array instead. Then the parsing for selectedDestinations would fail and the workaround in mapDestinations would not be used. We would need to either parse a fallback property as an array to cover that case, or ignore the case entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmeichigo I didn't implement logic to recover/fallback in case if an array arrives behind selected_destinations. The try?is in the init(from decoder: any Decoder) just to make sure the whole decoding pipeline won't fail. At the moment we're just ignoring selected_destination payload if it's an array.

Regarding the mapDestinations function - the fallback only handles ids without a "shipment_" prefix. I.e. the selected_destination can be a dictionary, but the indexes are just numeric strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. I didn't know there was such case that the destinations are returned as a dictionary with numeric IDs. From my testing, I could only see the array case. Is it reproducible or the workaround in mapDestination is just for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it reproducible

Yep. Check out the Order #1854 in our shared testing store. Addresses there contain both UUID and numeric keys.


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

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

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

Choose a reason for hiding this comment

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

💡 Since you already added a workaround for when the addresses are returned as a dictionary with numeric keys, we can add a fallback for the array case here:

Suggested change
self.selectedDestinations = try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
forKey: CodingKeys.selectedDestination
)
self.selectedDestinations = {
do {
let selectedDestinationsArray = try container.decode([WooShippingAddress].self, forKey: .selectedDestination)
var dictionary: [String: WooShippingAddress] = [:]
for (index, item) in selectedDestinationsArray.enumerated() {
dictionary[index.description] = item
}
return dictionary
} catch {
return try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
forKey: CodingKeys.selectedDestination
)
}
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in a separate PR: #15876

}
}
}

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

extension WooShippingLabelData {
static func mapDestinations(
_ destinations: WooShippingLabelDestinations,
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 {
return label
}

return label.copy(destinationAddress: destinationAddress.toShippingLabelAddress())
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ extension WooShippingAddress: Codable {
// If no name is sent to validation address request, no name will be received in response.
// So make sure to decode it only if it's present.
let name = try container.decodeIfPresent(String.self, forKey: .name) ?? ""
let company = try container.decode(String.self, forKey: .company)
let phone = try container.decode(String.self, forKey: .phone)
let company = try container.decodeIfPresent(String.self, forKey: .company) ?? ""
let phone = try container.decodeIfPresent(String.self, forKey: .phone) ?? ""
let country = try container.decode(String.self, forKey: .country)
let state = try container.decode(String.self, forKey: .state)
let address1 = try container.decodeIfPresent(String.self, forKey: .address1) ?? container.decode(String.self, forKey: .alternateAddress1)
let address2 = try container.decode(String.self, forKey: .address2)
let address2 = try container.decodeIfPresent(String.self, forKey: .address2) ?? ""
let city = try container.decode(String.self, forKey: .city)
let postcode = try container.decode(String.self, forKey: .postcode)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import Foundation

enum WooShippingShipmentIDFormatter {
/// Turns numeric shipment ID into formatted as `shipment_<id>`
/// - Parameter shipmentID: numeric shipment id
/// - Returns: formated id string
static func formattedShipmentID(_ shipmentID: String) -> String {
return isArgumentIDValid(shipmentID) ?
Values.shipmentIDPrefix + shipmentID :
shipmentID
}
}

private extension WooShippingShipmentIDFormatter {
private enum Values {
static let shipmentIDPrefix = "shipment_"
}

/// Make sure we are formatting incoming numeric ID string
private static func isArgumentIDValid(_ argumentID: String) -> Bool {
return Int(argumentID) != nil
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,16 @@ extension ShippingLabelAddress {
init() {
self.init(company: "", name: "", phone: "", country: "", state: "", address1: "", address2: "", city: "", postcode: "")
}

var isEmpty: Bool {
return company.isEmpty &&
name.isEmpty &&
phone.isEmpty &&
country.isEmpty &&
state.isEmpty &&
address1.isEmpty &&
address2.isEmpty &&
city.isEmpty &&
postcode.isEmpty
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import Foundation
import XCTest
@testable import Networking

final class WooShippingShipmentIDFormatterTests: XCTestCase {

// MARK: - formattedShipmentID

func test_formatted_shipment_id_when_id_is_numeric_should_return_formatted_id() {
// Given
let sut = WooShippingShipmentIDFormatter.self
let id = "123456"

// When
let formattedID = sut.formattedShipmentID(id)

// Then
XCTAssertEqual(formattedID, "shipment_123456")
}

func test_formatted_shipment_id_when_id_is_already_formatted_should_return_same_id() {
// Given
let sut = WooShippingShipmentIDFormatter.self
let id = "shipment_123456"

// When
let formattedID = sut.formattedShipmentID(id)

// Then
XCTAssertEqual(formattedID, "shipment_123456")
}

func test_formatted_shipment_id_when_non_numeric_should_return_same_id() {
// Given
let sut = WooShippingShipmentIDFormatter.self
let id = "non-numeric-id"

// When
let formattedID = sut.formattedShipmentID(id)

// Then
XCTAssertEqual(formattedID, id)
}
}
57 changes: 57 additions & 0 deletions Modules/Tests/NetworkingTests/Remote/WooShippingRemoteTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,63 @@ final class WooShippingRemoteTests: XCTestCase {
XCTAssertNotNil(result.failure)
}

// MARK: - Load Config With Destinations

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

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
remote.loadConfig(siteID: self.sampleSiteID, orderID: self.sampleOrderID) { result in
promise(result)
}
}

// Then
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertNotNil(label.destinationAddress)
XCTAssertEqual(label.destinationAddress.address1, "200 N SPRING ST")
}

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

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
remote.loadConfig(siteID: self.sampleSiteID, orderID: self.sampleOrderID) { result in
promise(result)
}
}

// Then
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertTrue(label.destinationAddress.isEmpty)
}

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

// When
let result: Result<WooShippingConfig, Error> = waitFor { promise in
remote.loadConfig(siteID: self.sampleSiteID, orderID: self.sampleOrderID) { result in
promise(result)
}
}

// Then
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertTrue(label.destinationAddress.isEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

With the workaround in mapDestinations, we should expect destinationAddress to be not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned here. The successful parse means ignoring the invalid payload and not failing the whole json decoding. The destinationAddress is intended to be empty since we are ignoring the invalid payload.

}

// MARK: Update shipment

func test_updateShipment_parses_success_response() throws {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{
"data": {
"config": {
"shippingLabelData": {
"storedData": {
"selected_destination": {
"shipment_1": {
"address_1": "200 N SPRING ST",
"city": "LOS ANGELES",
"state": "CA",
"postcode": "90012-4801",
"country": "US"
}
}
},
"currentOrderLabels": [
{
"label_id": 4871,
"tracking": null,
"refundable_amount": 0,
"created": 1742292110723,
"carrier_id": "usps",
"service_name": "USPS - Priority Mail",
"status": "PURCHASE_IN_PROGRESS",
"commercial_invoice_url": "",
"is_commercial_invoice_submitted_electronically": false,
"package_name": "Small Flat Rate Box",
"is_letter": false,
"product_names": [
"BG upload"
],
"product_ids": [
522
],
"id": "1",
"receipt_item_id": -1,
"created_date": 1742292110000
}
]
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"data": {
"config": {
"shippingLabelData": {
"storedData": {
"selected_destination": [
{
"address_1": "200 N SPRING ST"
}
]
},
"currentOrderLabels": [
{
"label_id": 4871,
"tracking": null,
"refundable_amount": 0,
"created": 1742292110723,
"carrier_id": "usps",
"service_name": "USPS - Priority Mail",
"status": "PURCHASE_IN_PROGRESS",
"commercial_invoice_url": "",
"is_commercial_invoice_submitted_electronically": false,
"package_name": "Small Flat Rate Box",
"is_letter": false,
"product_names": [
"BG upload"
],
"product_ids": [
522
],
"id": "1",
"receipt_item_id": -1,
"created_date": 1742292110000
}
]
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"data": {
"config": {
"shippingLabelData": {
"currentOrderLabels": [
{
"label_id": 4871,
"tracking": null,
"refundable_amount": 0,
"created": 1742292110723,
"carrier_id": "usps",
"service_name": "USPS - Priority Mail",
"status": "PURCHASE_IN_PROGRESS",
"commercial_invoice_url": "",
"is_commercial_invoice_submitted_electronically": false,
"package_name": "Small Flat Rate Box",
"is_letter": false,
"product_names": [
"BG upload"
],
"product_ids": [
522
],
"id": "1",
"receipt_item_id": -1,
"created_date": 1742292110000
}
]
}
}
}
}
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +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 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