Skip to content
This repository has been archived by the owner on Jun 13, 2018. It is now read-only.

Clean up tests and formatting for Location #446

Merged
merged 4 commits into from
Dec 14, 2016
Merged

Clean up tests and formatting for Location #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
548065d
Clean up tests and formatting for Location
kmcphillips Dec 13, 2016
c4aa010
Code review: Refactor alias lookups and add tests for .from
kmcphillips Dec 14, 2016
8e01d09
Code review: Simplify zip regex
kmcphillips Dec 14, 2016
68b519c
Code review: Unnecessary hash check
kmcphillips Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 63 additions & 46 deletions lib/active_shipping/location.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,21 @@ module ActiveShipping #:nodoc:
class Location
ADDRESS_TYPES = %w(residential commercial po_box)

ATTRIBUTE_ALIASES = {
name: [:name],
country: [:country_code, :country],
postal_code: [:postal_code, :zip, :postal],
province: [:province_code, :state_code, :territory_code, :region_code, :province, :state, :territory, :region],
city: [:city, :town],
address1: [:address1, :address, :street],
address2: [:address2],
address3: [:address3],
phone: [:phone, :phone_number],
fax: [:fax, :fax_number],
address_type: [:address_type],
company_name: [:company, :company_name],
}.freeze

attr_reader :options,
:country,
:postal_code,
Expand All @@ -24,9 +39,12 @@ class Location
alias_method :company, :company_name

def initialize(options = {})
@country = (options[:country].nil? or options[:country].is_a?(ActiveUtils::Country)) ?
options[:country] :
ActiveUtils::Country.find(options[:country])
@country = if options[:country].nil? || options[:country].is_a?(ActiveUtils::Country)
options[:country]
else
ActiveUtils::Country.find(options[:country])
end

@postal_code = options[:postal_code] || options[:postal] || options[:zip]
@province = options[:province] || options[:state] || options[:territory] || options[:region]
@city = options[:city]
Expand All @@ -42,48 +60,53 @@ def initialize(options = {})
end

def self.from(object, options = {})
return object if object.is_a? ActiveShipping::Location
attr_mappings = {
:name => [:name],
:country => [:country_code, :country],
:postal_code => [:postal_code, :zip, :postal],
:province => [:province_code, :state_code, :territory_code, :region_code, :province, :state, :territory, :region],
:city => [:city, :town],
:address1 => [:address1, :address, :street],
:address2 => [:address2],
:address3 => [:address3],
:phone => [:phone, :phone_number],
:fax => [:fax, :fax_number],
:address_type => [:address_type],
:company_name => [:company, :company_name]
}
return object if object.is_a?(ActiveShipping::Location)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something to consider for a future PR: perhaps this should dupe the object? I feel like a from method should return something I can mutate without having side effects somewhere else.


attributes = {}

hash_access = begin
object[:some_symbol]
true
rescue
false
end
attr_mappings.each do |pair|
pair[1].each do |sym|
if value = (object[sym] if hash_access) || (object.send(sym) if object.respond_to?(sym) && (!hash_access || !Hash.public_instance_methods.include?(sym.to_s)))
attributes[pair[0]] = value
break

ATTRIBUTE_ALIASES.each do |attribute, aliases|
aliases.detect do |sym|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little weird in that detect we don't care about the return value, but we do want it to exit early. Test fails if you change it to each.

value = if hash_access
object[sym]
elsif object.respond_to?(sym)
object.send(sym)
end

attributes[attribute] = value if value
end
end

attributes.delete(:address_type) unless ADDRESS_TYPES.include?(attributes[:address_type].to_s)

new(attributes.update(options))
end

def country_code(format = :alpha2)
@country.nil? ? nil : @country.code(format).value
end

def residential?; @address_type == 'residential' end
def commercial?; @address_type == 'commercial' end
def po_box?; @address_type == 'po_box' end
def unknown?; country_code == 'ZZ' end
def residential?
@address_type == 'residential'
end

def commercial?
@address_type == 'commercial'
end

def po_box?
@address_type == 'po_box'
end

def unknown?
country_code == 'ZZ'
end

def address_type=(value)
return unless value.present?
Expand All @@ -93,18 +116,18 @@ def address_type=(value)

def to_hash
{
:country => country_code,
:postal_code => postal_code,
:province => province,
:city => city,
:name => name,
:address1 => address1,
:address2 => address2,
:address3 => address3,
:phone => phone,
:fax => fax,
:address_type => address_type,
:company_name => company_name
country: country_code,
postal_code: postal_code,
province: province,
city: city,
name: name,
address1: address1,
address2: address2,
address3: address3,
phone: phone,
fax: fax,
address_type: address_type,
company_name: company_name
}
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify this, I believe;

def zip_plus_4
  "#{$1}-#{2} if /(\d{5})-?(\d{4})/ =~ @postal_code
end

Tested it out a bit:

irb(main):001:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "12345-1234"
=> "12345-1234"
irb(main):002:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "123451234"
=> "12345-1234"
irb(main):003:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "K2P 1L4"
=> nil


Expand All @@ -128,13 +151,7 @@ def inspect

# Returns the postal code as a properly formatted Zip+4 code, e.g. "77095-2233"
def zip_plus_4
if /(\d{5})(\d{4})/ =~ @postal_code
return "#{$1}-#{$2}"
elsif /\d{5}-\d{4}/ =~ @postal_code
return @postal_code
else
nil
end
"#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ @postal_code
end

def address2_and_3
Expand Down
Loading