-
Notifications
You must be signed in to change notification settings - Fork 121
[Shipping Labels] Fix missing origin address for previously purchased shipping labels #15872
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
Conversation
|
|
| selectedOrigins = try? container.decodeIfPresent( | ||
| WooShippingLabelAddressMap.self, | ||
| forKey: CodingKeys.selectedOrigin | ||
| ) |
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.
Please consider applying the same fix as suggested here to support the array case.
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: [Shipping Labels] Handle address array in selected_destination and selected_origin
| /// Fallback to `originAddress` in case if `originAddressLines` are missing | ||
| [viewModel.originAddress] |
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.
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?
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.
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?
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.
I'm good with having a blank space, it's easier to know that there's an issue than having incorrect data.
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.
Done in 548904a
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.
Thanks for the update. Just a note: when both addresses cannot be found, I see that the layout becomes horizontal:
I hope the workaround in #15876 would prevent this case to happen.
Generated by 🚫 Danger |

WOOMOB-746
Description
This is a follow up PR for the [Shipping Labels] Fix purchased labels not presenting destination address - addresses the "editable" origin address line in "Shipment details" bottom sheet for purchased labels. The line shouldn't be editable in case if a label is purchased.
The issue cause is the same as for the missing destination address. The
selected_originwasn't requested and therefore mapped from theconfig/label-purchase/{order_id}endpoint.config.shippingLabelData.storedData.selected_originrequest parameter for theconfig/label-purchase/{order_id}endpoint.selected_origincontent intoShippingLabels following the same logic implemented in #15866WooShippingRemoteTests.swiftto cover origin address mapping.Adds a [just in case for safety] fallback inWooShippingCreateLabelsViewto present the view model origin address in case if origin address lines from shipping labels are missing.Steps to reproduce
config/label-purchase/{order_id}endpoint request sends theconfig.shippingLabelData.storedData.selected_originargument. Make sure the response containsselected_originpayload.Testing information
WooShippingRemoteTestsextended to cover origin address mapping.Screenshots
RELEASE-NOTES.txtif necessary.