From 92d91250ef1560abbb56d9c18fd39779268d31b3 Mon Sep 17 00:00:00 2001 From: Cody Fauser Date: Thu, 26 Mar 2009 10:21:43 -0400 Subject: [PATCH 1/2] Improve FedEx handling of some error conditions --- CHANGELOG | 1 + .../shipping/carriers/fedex.rb | 18 +++++- test/fixtures/xml/fedex/empty_response.xml | 1 + .../invalid_recipient_country_response.xml | 1 + test/remote/fedex_test.rb | 60 ++++++++++++++++--- test/unit/carriers/fedex_test.rb | 32 +++++++++- 6 files changed, 100 insertions(+), 13 deletions(-) create mode 100644 test/fixtures/xml/fedex/empty_response.xml create mode 100644 test/fixtures/xml/fedex/invalid_recipient_country_response.xml diff --git a/CHANGELOG b/CHANGELOG index 273015c2e..a8b023097 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,4 @@ +* Improve FedEx handling of some error conditions [cody] * Add support for validating credentials to Shipwire [cody] * Add support for ssl_get to PostsData. Update Carriers to use PostsData module. Turn on retry safety for carriers [cody] * Add support for Shipwire Shipping Rate API [cody] diff --git a/lib/active_shipping/shipping/carriers/fedex.rb b/lib/active_shipping/shipping/carriers/fedex.rb index e454829d1..39120f0c6 100644 --- a/lib/active_shipping/shipping/carriers/fedex.rb +++ b/lib/active_shipping/shipping/carriers/fedex.rb @@ -156,7 +156,7 @@ def build_location_node(name, location) location_node = XmlNode.new(name) do |xml_node| xml_node << XmlNode.new('StateOrProvinceCode', location.state) xml_node << XmlNode.new('PostalCode', location.postal_code) - xml_node << XmlNode.new("CountryCode", location.country_code(:alpha2)) unless location.country_code(:alpha2).blank? + xml_node << XmlNode.new("CountryCode", location.country_code(:alpha2)) end end @@ -169,9 +169,11 @@ def parse_rate_response(origin, destination, packages, response, options) xml_hash = Hash.from_xml(response)['FDXRateAvailableServicesReply'] success = response_hash_success?(xml_hash) message = response_hash_message(xml_hash) + if success entries << xml_hash['Entry'] entries.flatten! + entries.compact! end entries.each do |rated_shipment| @@ -183,6 +185,11 @@ def parse_rate_response(origin, destination, packages, response, options) :packages => packages) end + if rate_estimates.empty? + success = false + message = "No shipping rates could be found for the destination address" if message.blank? + end + RateResponse.new(success, message, xml_hash, :rates => rate_estimates, :xml => response, :request => last_request, :log_xml => options[:log_xml]) end @@ -239,7 +246,8 @@ def parse_tracking_response(response, options) :request => last_request, :shipment_events => shipment_events, :destination => destination, - :tracking_number => tracking_number) + :tracking_number => tracking_number + ) end def response_hash_success?(xml_hash) @@ -247,7 +255,11 @@ def response_hash_success?(xml_hash) end def response_hash_message(xml_hash) - response_hash_success?(xml_hash) ? '' : "FedEx Error Code: #{xml_hash['Error']['Code'] || xml_hash['SoftError']['Code']}: #{xml_hash['Error']['Message'] || xml_hash['SoftError']['Message']}" + if error = xml_hash['Error'] || xml_hash['SoftError'] + "FedEx Error Code: #{error['Code']}: #{error['Message']}" + else + '' + end end def commit(request, test = false) diff --git a/test/fixtures/xml/fedex/empty_response.xml b/test/fixtures/xml/fedex/empty_response.xml new file mode 100644 index 000000000..26d65816b --- /dev/null +++ b/test/fixtures/xml/fedex/empty_response.xml @@ -0,0 +1 @@ +?xml version="1.0" encoding="UTF-8"?> \ No newline at end of file diff --git a/test/fixtures/xml/fedex/invalid_recipient_country_response.xml b/test/fixtures/xml/fedex/invalid_recipient_country_response.xml new file mode 100644 index 000000000..3845daeb0 --- /dev/null +++ b/test/fixtures/xml/fedex/invalid_recipient_country_response.xml @@ -0,0 +1 @@ +0161451Invalid recipient country \ No newline at end of file diff --git a/test/remote/fedex_test.rb b/test/remote/fedex_test.rb index ff71e8a89..95b428bc8 100644 --- a/test/remote/fedex_test.rb +++ b/test/remote/fedex_test.rb @@ -7,15 +7,14 @@ def setup @locations = TestFixtures.locations @carrier = FedEx.new(fixtures(:fedex)) end - + def test_us_to_canada response = nil assert_nothing_raised do response = @carrier.find_rates( @locations[:beverly_hills], @locations[:ottawa], - @packages.values_at(:wii), - :test => true + @packages.values_at(:wii) ) assert !response.rates.blank? response.rates.each do |rate| @@ -25,14 +24,62 @@ def test_us_to_canada end end + def test_zip_to_zip_fails + begin + @carrier.find_rates( + Location.new(:zip => 40524), + Location.new(:zip => 40515), + @packages[:wii] + ) + rescue ResponseError => e + assert_equal 'FedEx Error Code: 64354: Required element OriginAddress/CountryCode missing.', e.message + end + end + + # FedEx requires a valid origin and destination postal code + def test_rates_for_locations_with_only_zip_and_country + response = @carrier.find_rates( + @locations[:bare_beverly_hills], + @locations[:bare_ottawa], + @packages.values_at(:wii) + ) + + assert response.rates.size > 0 + end + + def test_rates_for_location_with_only_country_code + begin + response = @carrier.find_rates( + @locations[:bare_beverly_hills], + Location.new(:country => 'CA'), + @packages.values_at(:wii), + :items => @items + ) + rescue ResponseError => e + assert_equal 'FedEx Error Code: 61530: Invalid Destination/Address/PostalCode.', e.message + end + end + + def test_invalid_recipient_country + begin + response = @carrier.find_rates( + @locations[:bare_beverly_hills], + Location.new(:country => 'JP', :zip => '108-8361'), + @packages.values_at(:wii), + :items => @items + ) + rescue ResponseError => e + assert_equal 'FedEx Error Code: 61451: Invalid recipient country', e.message + end + end + def test_canada_to_us response = nil assert_nothing_raised do response = @carrier.find_rates( @locations[:ottawa], @locations[:beverly_hills], - @packages.values_at(:wii), - :test => true + @packages.values_at(:wii) ) assert !response.rates.blank? response.rates.each do |rate| @@ -48,8 +95,7 @@ def test_ottawa_to_beverly_hills response = @carrier.find_rates( @locations[:ottawa], @locations[:beverly_hills], - @packages.values_at(:book, :wii), - :test => true + @packages.values_at(:book, :wii) ) assert !response.rates.blank? response.rates.each do |rate| diff --git a/test/unit/carriers/fedex_test.rb b/test/unit/carriers/fedex_test.rb index aba4f7602..91e1c5e18 100644 --- a/test/unit/carriers/fedex_test.rb +++ b/test/unit/carriers/fedex_test.rb @@ -1,8 +1,6 @@ require File.dirname(__FILE__) + '/../../test_helper' class FedExTest < Test::Unit::TestCase - include ActiveMerchant::Shipping - def setup @packages = TestFixtures.packages @locations = TestFixtures.locations @@ -21,9 +19,37 @@ def test_initialize_options_requirements assert_nothing_raised { FedEx.new(:login => '999999999', :password => '7777777')} end + def test_invalid_recipient_country + @carrier.expects(:commit).returns(xml_fixture('fedex/invalid_recipient_country_response')) + + begin + @carrier.find_rates( + @locations[:ottawa], + @locations[:beverly_hills], + @packages.values_at(:book, :wii) + ) + rescue ResponseError => e + assert_equal "FedEx Error Code: 61451: Invalid recipient country", e.message + end + end + + def test_no_rates_response + @carrier.expects(:commit).returns(xml_fixture('fedex/empty_response')) + + begin + response = @carrier.find_rates( + @locations[:ottawa], + @locations[:beverly_hills], + @packages.values_at(:book, :wii) + ) + rescue ResponseError => e + assert_equal "No shipping rates could be found for the destination address", e.message + end + end + def test_find_tracking_info_should_return_a_tracking_response @carrier.expects(:commit).returns(@tracking_response) - assert_equal 'ActiveMerchant::Shipping::TrackingResponse', @carrier.find_tracking_info('077973360390581').class.name + assert_instance_of ActiveMerchant::Shipping::TrackingResponse, @carrier.find_tracking_info('077973360390581') end def test_find_tracking_info_should_parse_response_into_correct_number_of_shipment_events From e270b17678fd8ac6e538b24b290ec915f02c8584 Mon Sep 17 00:00:00 2001 From: Cody Fauser Date: Thu, 26 Mar 2009 10:22:57 -0400 Subject: [PATCH 2/2] Add name to shipwire class --- CHANGELOG | 1 + lib/active_shipping/shipping/carriers/shipwire.rb | 3 +++ 2 files changed, 4 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index a8b023097..9a1a64aa3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,4 @@ +* Add name to Shipwire class [cody] * Improve FedEx handling of some error conditions [cody] * Add support for validating credentials to Shipwire [cody] * Add support for ssl_get to PostsData. Update Carriers to use PostsData module. Turn on retry safety for carriers [cody] diff --git a/lib/active_shipping/shipping/carriers/shipwire.rb b/lib/active_shipping/shipping/carriers/shipwire.rb index 8b3816415..658cc1bd5 100644 --- a/lib/active_shipping/shipping/carriers/shipwire.rb +++ b/lib/active_shipping/shipping/carriers/shipwire.rb @@ -6,6 +6,9 @@ module Shipping class Shipwire < Carrier self.retry_safe = true + cattr_reader :name + @@name = "Shipwire" + URL = 'https://www.shipwire.com/exec/RateServices.php' SCHEMA_URL = 'http://www.shipwire.com/exec/download/RateRequest.dtd' WAREHOUSES = { 'CHI' => 'Chicago',