Remove orphaned PayPalExpress payments when processing actual PayPalExpress payment

Fixes both #1074 and #1837

app/models/spree/payment_decorator.rb

@@ -5,6 +5,7 @@ module Spree
     after_save :ensure_correct_adjustment, :update_order
 
     def ensure_correct_adjustment
+      destroy_orphaned_paypal_payments if payment_method.is_a?(Spree::Gateway::PayPalExpress)
       # Don't charge for invalid payments.
       # PayPalExpress always creates a payment that is invalidated later.
       # Unknown: What about failed payments?
@@ -81,5 +82,10 @@ module Spree
       refund_amount.to_f
     end
 
+    def destroy_orphaned_paypal_payments
+      return unless source_type == "Spree::PaypalExpressCheckout"
+      orphaned_payments = order.payments.where(payment_method_id: payment_method_id, source_id: nil)
+      orphaned_payments.each(&:destroy)
+    end
   end
 end

spec/models/spree/payment_spec.rb

@@ -124,5 +124,93 @@ module Spree
         payment.refund!
       end
     end
+
+    describe "applying transaction fees" do
+      let!(:order) { create(:order) }
+      let!(:line_item) { create(:line_item, order: order, quantity: 3, price: 5.00) }
+
+      # Mimicing call from PayPalController#confirm in spree_paypal_express
+      def create_new_paypal_express_payment(order, payment_method)
+        order.payments.create!({
+          :source => Spree::PaypalExpressCheckout.create({
+            :token => "123",
+            :payer_id => "456"
+          }, :without_protection => true),
+          :amount => order.total,
+          :payment_method => payment_method
+        }, :without_protection => true)
+      end
+
+      before do
+        order.update_totals
+      end
+
+      context "for paypal express payments with transaction fees" do
+        let!(:payment_method) { Spree::Gateway::PayPalExpress.create!(name: "PayPalExpress", distributor_ids: [create(:distributor_enterprise).id], environment: Rails.env) }
+        let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) }
+
+        before do
+          payment_method.calculator = calculator
+          payment_method.save!
+        end
+
+        context "when a payment already exists on the order" do
+          let!(:existing_payment) do
+            order.payments.create!(payment_method_id: payment_method.id, amount: order.total)
+          end
+
+          context "and the existing payment uses the same payment method" do
+            context "and the existing payment does not have a source" do
+              it "removes the existing payment (and associated fees)" do
+                new_payment = create_new_paypal_express_payment(order, payment_method)
+
+                order.reload
+                expect(order.payments.count).to eq 1
+                expect(order.payments).to include new_payment
+                expect(order.payments).to_not include existing_payment
+                expect(order.adjustments.payment_fee.count).to eq 1
+                expect(order.adjustments.payment_fee.first.amount).to eq 1.5
+              end
+            end
+
+            context "and the existing payment has a source" do
+              let!(:source) { Spree::PaypalExpressCheckout.create(token: "123") }
+
+              before do
+                existing_payment.source = source
+                existing_payment.save!
+              end
+
+              it "does not remove or invalidate the existing payment" do
+                new_payment = create_new_paypal_express_payment(order, payment_method)
+
+                order.reload
+                expect(order.payments.count).to eq 2
+                expect(order.payments).to include new_payment, existing_payment
+                expect(new_payment.state).to eq "checkout"
+                expect(existing_payment.state).to eq "checkout"
+              end
+            end
+          end
+
+          context "and the existing payment uses a different method" do
+            let!(:another_payment_method) { Spree::Gateway::PayPalExpress.create!(name: "PayPalExpress", distributor_ids: [create(:distributor_enterprise).id], environment: Rails.env) }
+
+            before do
+              existing_payment.update_attributes(payment_method_id: another_payment_method.id)
+            end
+
+            it "does not remove or invalidate the existing payment" do
+              new_payment = create_new_paypal_express_payment(order, payment_method)
+
+              order.reload
+              expect(order.payments).to include new_payment, existing_payment
+              expect(new_payment.state).to eq "checkout"
+              expect(existing_payment.state).to eq "checkout"
+            end
+          end
+        end
+      end
+    end
   end
 end

spec/requests/paypal_confirm_spec.rb

@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe "confirming an order with paypal express payment method", type: :request do
+  include ShopWorkflow
+
+  let!(:address) { create(:address) }
+  let!(:shop) { create(:enterprise) }
+  let!(:shipping_method) { create(:shipping_method, distributor_ids: [shop.id]) }
+  let!(:order) { create(:order, distributor: shop, ship_address: address.dup, bill_address: address.dup) }
+  let!(:shipment) { create(:shipment, order: order, shipping_method: shipping_method) }
+  let!(:line_item) { create(:line_item, order: order, quantity: 3, price: 5.00) }
+  let!(:payment_method) { Spree::Gateway::PayPalExpress.create!(name: "PayPalExpress", distributor_ids: [create(:distributor_enterprise).id], environment: Rails.env) }
+  let(:params) { { token: 'lalalala', PayerID: 'payer1', payment_method_id: payment_method.id } }
+  let(:mocked_xml_response) {
+    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>
+    <Envelope><Body>
+      <GetExpressCheckoutDetailsResponse>
+        <Ack>Success</Ack>
+        <PaymentDetails>Something</PaymentDetails>
+        <DoExpressCheckoutPaymentResponseDetails>
+          <PaymentInfo><TransactionID>s0metran$act10n</TransactionID></PaymentInfo>
+        </DoExpressCheckoutPaymentResponseDetails>
+      </GetExpressCheckoutDetailsResponse>
+    </Body></Envelope>"
+  }
+
+  before do
+    order.reload.update_totals
+    order.shipping_method = shipping_method
+    expect(order.next).to be true # => address
+    expect(order.next).to be true # => delivery
+    expect(order.next).to be true # => payment
+    set_order order
+
+    stub_request(:post, "https://api-3t.sandbox.paypal.com/2.0/")
+    .to_return(:status => 200, :body => mocked_xml_response )
+  end
+
+  context "with a flat percent calculator" do
+    let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) }
+
+    before do
+      payment_method.calculator = calculator
+      payment_method.save!
+      order.payments.create!(payment_method_id: payment_method.id, amount: order.total)
+    end
+
+    it "destroys the old payment and processes the order" do
+      # Sanity check to condition of the order before we confirm the payment
+      expect(order.payments.count).to eq 1
+      expect(order.payments.first.state).to eq "checkout"
+      expect(order.adjustments.payment_fee.count).to eq 1
+      expect(order.adjustments.payment_fee.first.amount).to eq 1.5
+
+      get spree.confirm_paypal_path, params
+
+      # Processing was successful, order is complete
+      expect(response).to redirect_to spree.order_path(order, :token => order.token)
+      expect(order.reload.complete?).to be true
+
+      # We have only one payment, and one transaction fee
+      expect(order.payments.count).to eq 1
+      expect(order.payments.first.state).to eq "completed"
+      expect(order.adjustments.payment_fee.count).to eq 1
+      expect(order.adjustments.payment_fee.first.amount).to eq 1.5
+    end
+  end
+end