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

Calculates UPS delivery date for rate estimates #38

Closed
wants to merge 3 commits into from
Closed

Calculates UPS delivery date for rate estimates #38

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
119f55a
Calculates UPS delivery date for rate estimates
Jan 6, 2012
e000e6a
Modified ups_test for readability. Ups create a rate_estimate with a …
Jan 9, 2012
f089d01
Fixed removed if block from test.
Jan 18, 2012
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
6 changes: 5 additions & 1 deletion lib/active_shipping/shipping/carriers/ups.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,16 @@ def parse_rate_response(origin, destination, packages, response, options={})

xml.elements.each('/*/RatedShipment') do |rated_shipment|
service_code = rated_shipment.get_text('Service/Code').to_s
days_to_delivery = rated_shipment.get_text('GuaranteedDaysToDelivery').to_s.to_i
delivery_date = days_to_delivery >= 1 ? days_to_delivery.days.from_now : nil

rate_estimates << RateEstimate.new(origin, destination, @@name,
service_name_for(origin, service_code),
:total_price => rated_shipment.get_text('TotalCharges/MonetaryValue').to_s.to_f,
:currency => rated_shipment.get_text('TotalCharges/CurrencyCode').to_s,
:service_code => service_code,
:packages => packages)
:packages => packages,
:delivery_date => delivery_date)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to set :delivery_range => [delivery_date] here instead, that way both delivery_range and delivery_date will be set on the estimate. I think it's fine for :delivery_range to only have one entry and having both set will make the api less confusing (do I need to call delivery_date or delivery_range? Am I Fedex or UPS?)

end
end
RateResponse.new(success, message, Hash.from_xml(response).values.first, :rates => rate_estimates, :xml => response, :request => last_request)
Expand Down
2 changes: 1 addition & 1 deletion lib/active_shipping/shipping/rate_estimate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def initialize(origin, destination, carrier, service_name, options={})
@total_price = Package.cents_from(options[:total_price])
@currency = options[:currency]
@delivery_range = options[:delivery_range] ? options[:delivery_range].map { |date| date_for(date) }.compact : []
@delivery_date = @delivery_range.last
@delivery_date = options[:delivery_date] ? options[:delivery_date] : @delivery_range.last
Copy link
Contributor

Choose a reason for hiding this comment

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

With @jduff's suggestion, this change isn't needed.

Copy link
Member

Choose a reason for hiding this comment

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

Can refactor this into:

@delivery_date = options[:delivery_date] || @delivery_range.last

end

def total_price
Expand Down
2 changes: 2 additions & 0 deletions test/unit/carriers/ups_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ def test_response_parsing
"UPS Next Day Air Early A.M.",
"UPS Next Day Air"], response.rates.map(&:service_name)
assert_equal [992, 2191, 3007, 5509, 9401, 6124], response.rates.map(&:price)
assert_equal [nil, 3, 2, 1, 1, 1].map{|days| days ? days.days.from_now.strftime("%Y-%m-%d"): days},
response.rates.map{|rate| rate.delivery_date ? rate.delivery_date.strftime("%Y-%m-%d") : rate.delivery_date}
Copy link
Member

Choose a reason for hiding this comment

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

I would split this up into multiple lines, kind of hard to parse with my old eyes :P

Copy link
Contributor

Choose a reason for hiding this comment

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

rate.delivery_date should then already be in DateTime format or nil, I think this can just be response.rates.map(&:delivery_date)

end

def test_maximum_weight
Expand Down