-
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
[Shipping Labels] Fix purchased labels not presenting destination address #15866
Conversation
|
|
Generated by 🚫 Danger |
itsmeichigo
left a comment
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.
Thank you for fixing this. I tested and confirmed that the fix works with labels created with the correct shipment IDs for hazmat and customs.
The workaround for labels created with invalid shipment IDs didn't seem to work (please see a comment below).
I also see that the origin address is still editable - we would need to use the same solution to parse selected_origin as well. Please feel free to handle that in a separate PR if you prefer.
| /// shipment ID to set for hazmat and customs form | ||
| var formattedShipmentID: String { | ||
| Values.shipmentIDPrefix + shipmentID | ||
| return WooShippingShipmentIDFormatter.formattedShipmentID(shipmentID) |
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.shipmentIDPrefix is no longer used and should be removed from this file.
| } | ||
|
|
||
| public extension WooShippingLabelData { | ||
| typealias WooShippingLabelDestinations = [String: WooShippingAddress] |
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.
❓ 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.
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.
@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.
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.
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?
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.
Is it reproducible
Yep. Check out the Order #1854 in our shared testing store. Addresses there contain both UUID and numeric keys.
| // Then | ||
| let config = try XCTUnwrap(result.get()) | ||
| let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first) | ||
| XCTAssertTrue(label.destinationAddress.isEmpty) |
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.
With the workaround in mapDestinations, we should expect destinationAddress to be not empty.
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.
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.
|
I removed the redundant |
itsmeichigo
left a comment
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.
As mentioned in a comment here, I cannot reproduce the case with destinations returned as a dictionary with numeric keys. But I left a suggestion for decoding selected destination addresses as an array, it works from my testing. Please consider applying that as a workaround for labels created in the previous version.
| self.selectedDestinations = try? container.decodeIfPresent( | ||
| WooShippingLabelDestinations.self, | ||
| forKey: CodingKeys.selectedDestination | ||
| ) |
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.
💡 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:
| 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 | |
| ) | |
| } | |
| }() |
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.
Implemented in a separate PR: #15876

Fix: Display destination address for purchased shipping labels
WOOMOB-746
Description
This pull request fixes a bug where the destination address for previously purchased shipping labels was not being displayed to the user.
The root cause was that the
config/label-purchase/{order_id}endpoint returns shipping label objects and their corresponding destination addresses in separate parts of the JSON payload. TheShippingLabelmodel itself did not contain the address, and the app was not performing the necessary association between the two datasets.This PR introduces a client-side solution to correctly populate the address information:
selected_destinationdictionary from theconfig.shippingLabelData.storedDatapath in the API response. Theconfig.shippingLabelData.storedData.selected_destinationrequest parameter was added to obtain theselected_destinationpayload.currentOrderLabelsand injects the correct destination address from theselected_destinationdictionary.shipment_<id>format for the dictionary key. A newWooShippingShipmentIDFormatterutility was created to handle this formatting consistently. It was extracted from the recently added in for reusabilityWooShippingPackagePurchase.swiftshipmentID.Testing steps
Testing information
This PR is thoroughly covered by new automated tests to ensure the mapping logic is correct and to prevent regressions.
WooShippingShipmentIDFormatterTests: A new test file was created to unit test the ID formatting logic, covering various input scenarios.WooShippingRemoteTests: The existing remote tests were refactored to use the establishedwaitForandsimulateResponsepattern. The new tests validate the end-to-end flow by mocking theloadConfigAPI call with different JSON fixtures:wooshipping-config-with-destinations.json: Simulates a response containing destination addresses and asserts that they are correctly mapped to the label objects.wooshipping-config-without-destinations.json: Ensures the app handles responses without destination data gracefully.wooshipping-config-with-invalid-destinations.json: Confirms that a malformedselected_destinationobject (i.e., an array instead of a dictionary) does not cause a crash.RELEASE-NOTES.txtif necessary.