-
Notifications
You must be signed in to change notification settings - Fork 121
[Shipping Labels] Fix purchased labels not presenting destination address #15866
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.
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
Changes from 10 commits
c9ef314
191af69
c51e221
ed2c3d4
1219856
1b7e03e
2bcdb3f
d2e473f
bb40a05
a19efa9
0290290
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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] | ||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Regarding the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep. Check out the |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in a separate PR: #15876 |
||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| 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 |
|---|---|---|
| @@ -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 |
|---|---|---|
| @@ -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) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the workaround in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
|
||
| 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 | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
Values.shipmentIDPrefixis no longer used and should be removed from this file.