[Code Review]Fixes the invalid item amount bug with PayFlow Express UK #300

Merged
merged 3 commits into from May 16, 2012

Conversation

Projects
None yet
4 participants

An invalid item amount error is thrown by paypal. This only occurs with PayflowExpress UK Gateway. This error is generated when an order has a discount included, taxes are included in the price, there is free shipping and the express checkout option is selected at the end of the transaction

Please Review
@odorcicd @jduff

Dennis O'Connor Changed L_COSTn to L_AMT_n in PayFlowExpressHelper. Added corner case…
… to remote test for payflow_express and payflow_express_uk.
8b1ca6f

@odorcicd odorcicd commented on an outdated diff Mar 12, 2012

test/remote/gateways/remote_payflow_express_test.rb
+ :subtotal=>25.18.to_money,
+ :items=> [
+ {:name=>"test4",
+ :description=>"test4",
+ :quantity=>2,
+ :amount=>13.99.to_money,
+ :url=>"http://localhost:3000/products/test4"},
+ {:name=>"Discount",
+ :quantity=>1,
+ :amount=>-2.80.to_money}],
+ :shipping=>0,
+ :handling=>0,
+ :tax=>0,
+ :no_shipping=>true,
+ :locale=>"US"}
+ response = @gateway.setup_authorization(amount, options)
@odorcicd

odorcicd Mar 12, 2012

Contributor

indentation here and below

@odorcicd odorcicd and 1 other commented on an outdated diff Mar 12, 2012

test/remote/gateways/remote_payflow_express_test.rb
@@ -47,4 +47,69 @@ def test_set_express_purchase
assert response.test?
assert !response.params['token'].blank?
end
+
+ def test_setup_authorization_discount_taxes_included_free_shipping
+ amount = 2518
+ options = {:ip=>"127.0.0.1",
+ :return_url=>"http://localhost:3000/orders/1/8e06ea26f8add7608671d433f13c2193/commit_paypal?buyer_accepts_marketing=true&utm_nooverride=1",
+ :cancel_return_url=>"http://localhost:3000/orders/1/8e06ea26f8add7608671d433f13c2193/commit_paypal?utm_nooverride=1",
+ :customer=>"test6@test.com",
+ :email=>"test6@test.com",
+ :order_id=>"#1092",
+ :currency=>"USD",
+ :disable_review=>true,
@odorcicd

odorcicd Mar 12, 2012

Contributor

where are disable_review, subtotal, shipping, handling, tax, and locale used as options? anywhere?

@dennisfoconnor

dennisfoconnor Mar 13, 2012

This is the data sent from Shopify which was generating the bug. I'll trim out the unused options.

@dennisfoconnor

dennisfoconnor Mar 14, 2012

Subtotal is now being used.

Dennis O'Connor added some commits Mar 13, 2012

Dennis O'Connor Remvoed the L_AMT fix because it might break other things, added DISC…
…OUNT field instead. Added more remote tests to RemotePayflowExpressTest.
80b7048
Dennis O'Connor Using subtotal as Item amt field. a3e5c66

@jduff jduff commented on the diff Mar 14, 2012

lib/active_merchant/billing/gateways/payflow_express.rb
@@ -169,7 +170,7 @@ def add_pay_data(xml, money, options)
items = options[:items] || []
items.each_with_index do |item, index|
xml.tag! 'ExtData', 'Name' => "L_DESC#{index}", 'Value' => item[:description]
- xml.tag! 'ExtData', 'Name' => "L_COST#{index}", 'Value' => amount(item[:amount])
+ xml.tag! 'ExtData', 'Name' => "L_COST#{}{index}", 'Value' => amount(item[:amount])
@jduff

jduff Mar 14, 2012

Contributor

Looks like there is a typo here, I don't think this line should be changed at all.

@jduff jduff commented on the diff Mar 14, 2012

lib/active_merchant/billing/gateways/payflow_express.rb
@@ -177,10 +178,11 @@ def add_pay_data(xml, money, options)
end
if items.any?
xml.tag! 'ExtData', 'Name' => 'CURRENCY', 'Value' => options[:currency] || currency(money)
- xml.tag! 'ExtData', 'Name' => "ITEMAMT", 'Value' => amount(money)
+ xml.tag! 'ExtData', 'Name' => "ITEMAMT", 'Value' => amount(options[:subtotal])
@jduff

jduff Mar 14, 2012

Contributor

This is changing current behaviour. You probably want to do:

amount(options[:subtotal] || money)

@odorcicd odorcicd commented on the diff Mar 15, 2012

test/remote/gateways/remote_payflow_express_test.rb
+ :order_id=>"#1092",
+ :currency=>"USD",
+ :subtotal=>2798,
+ :items => [
+ {:name => "test4",
+ :description => "test4",
+ :quantity=>2 ,
+ :amount=> 1399 ,
+ :url=>"http://localhost:3000/products/test4"}],
+ :discount=>280,
+ :no_shipping=>true}
+ response = @gateway.setup_authorization(amount, options)
+ assert response.success?
+ end
+
+ def test_setup_authorization_with_discount_taxes_and_shipping_addtiional
@odorcicd

odorcicd Mar 15, 2012

Contributor

typo

Contributor

ntalbott commented Apr 25, 2012

What's the status of this PR? Can it be merged?

Contributor

jduff commented Apr 25, 2012

I'm going to test it out with a PayPal UK test account and make sure
everything is working as expected. Once i've tested it out I'll merge
this in.

@jduff jduff merged commit a3e5c66 into master May 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment