adds address validation to usps.rb #410
Conversation
|
This PR is related to issue #409 |
There was a problem hiding this comment.
Thanks for doing this feature, @pborel! I understand it's an MVP but it looks like you're on the right track here. After addressing some of the comments, I'd love to see some tests verifying the expected behaviour, as well.
Great work thus far ⭐️ !
| xml.AddressValidateRequest('USERID' => @options[:login]) do | ||
| xml.IncludeOptionalElements(true) | ||
| xml.ReturnCarrierRoute(true) | ||
| Array(addresses).each_with_index do |address, id| |
There was a problem hiding this comment.
I don't believe you need to wrap addresses in an Array() constructor; do we anticipate it ever being not-Array-like?
|
|
||
| if success | ||
| addresses = Hash.from_xml(response)["AddressValidateResponse"]["Address"] | ||
| if addresses.is_a?(Array) |
There was a problem hiding this comment.
Took a look at the response, you should be able to do this to avoid the Array check entirely:
locations = [addresses].flatten.map do |address|
Location.new(#etc etc)
end
| def parse_address_standardize_response(response, addresses, options = {}) | ||
| success = true | ||
| message = '' | ||
| address_hash = {} |
| success = true | ||
| message = '' | ||
| address_hash = {} | ||
| locations = [] |
There was a problem hiding this comment.
If you use the map assignment suggestion below, you won't need to initialize an empty array variable.
| Array(addresses).each_with_index do |address, id| | ||
| xml.Address('ID' => id) do | ||
| xml.FirmName(address[:firm_name]) | ||
| xml.Address1(address[:address1]) |
There was a problem hiding this comment.
From the documentation, it looks like Address1 and Address2 are flipped for USPS.
We use Address1 as the street address in ActiveShipping but USPS uses it as the suite number, with Address2 being their street address.
| # The correct addresses returned from USPS | ||
| class AddressStandardizationResponse < Response | ||
|
|
||
| attr_reader :locations |
There was a problem hiding this comment.
You'll need an attr_reader for :addresses too.
|
|
||
| attr_reader :locations | ||
|
|
||
| # Initializes a new AddressStandarizationResponse instance. |
There was a problem hiding this comment.
Indentation a little off here.
| # @param success (see ActiveShipping::Response#initialize) | ||
| # @param message (see ActiveShipping::Response#initialize) | ||
| # @param params (see ActiveShipping::Response#initialize) | ||
| # @option options (see ActiveShipping::Response#initialize) |
There was a problem hiding this comment.
Could summarize it in one line like https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/tracking_response.rb#L59
| super | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
A newline is preferred at the end of all files.
maartenvg
force-pushed
the
master
branch
2 times, most recently
from
6eca161
to
20da555
Compare
To do: