Merge pull request #35 from StemboltHQ/bug-auth-completion

Create payment in auth notification

Author: Dkendal

app/controllers/spree/adyen_redirect_controller.rb

@@ -7,7 +7,17 @@ class AdyenRedirectController < StoreController
 
     # This is the entry point after an Adyen HPP payment is completed
     def confirm
-      source = Adyen::HppSource.new(source_params(params))
+      if @order.complete?
+        confirm_order_already_completed
+      else
+        confirm_order_incomplete
+      end
+    end
+
+    private
+
+    def confirm_order_incomplete
+      source = Adyen::HppSource.new(source_params)
 
       unless source.authorised?
         flash.notice = Spree.t(:payment_processing_failed)
@@ -18,40 +28,39 @@ def confirm
       # payment is created in a 'checkout' state so that the payment method
       # can attempt to auth it. The payment of course is already auth'd and
       # adyen hpp's authorize implementation just returns a dummy response.
-      payment =
-        @order.payments.create!(
-          amount: @order.total,
-          payment_method: @payment_method,
-          source: source,
-          response_code: params[:pspReference],
-          state: "checkout",
-          # Order is explicitly defined here because as of writing the
-          # Order -> Payments association does not have the inverse of defined
-          # when we call `order.complete` below payment.order will still
-          # refer to a previous state of the record.
-          #
-          # If the payment is auto captured only then the payment will completed
-          # in `process_outstanding!`, and because Payment calls
-          # .order.update_totals after save the order is saved with its
-          # previous values, causing payment_state and shipment_state to revert
-          # to nil.
-          order: @order
-        )
+      @order.payments.create!(
+        amount: @order.total,
+        payment_method: @payment_method,
+        source: source,
+        response_code: psp_reference,
+        state: "checkout"
+      )
 
       if @order.complete
-        # We may have already recieved the authorization notification, so process
-        # it now
-        Spree::Adyen::NotificationProcessor.process_outstanding!(payment)
-
-        flash.notice = Spree.t(:order_processed_successfully)
-        redirect_to order_path(@order)
+        redirect_to_order
       else
         #TODO void/cancel payment
         redirect_to checkout_state_path(@order.state)
       end
     end
 
-    private
+    # If an authorization notification is received before the redirection the
+    # payment is created there.In this case we just need to assign the addition
+    # parameters received about the source.
+    #
+    # We do this because there is a chance that we never get redirected back
+    # so we need to make sure we complete the payment and order.
+    def confirm_order_already_completed
+      payment = @order.payments.find_by!(response_code: psp_reference)
+      payment.source.update(source_params)
+
+      redirect_to_order
+    end
+
+    def redirect_to_order
+      flash.notice = Spree.t(:order_processed_successfully)
+      redirect_to order_path(@order)
+    end
 
     def check_signature
       unless ::Adyen::Form.redirect_signature_check(params, @payment_method.shared_secret)
@@ -71,11 +80,10 @@ def restore_session
 
       @order =
         Spree::Order.
-        incomplete.
         find_by!(guest_token: cookies.signed[:guest_token])
     end
 
-    def source_params params
+    def source_params
       params.permit(
         :authResult,
         :pspReference,
@@ -86,5 +94,9 @@ def source_params params
         :shopperLocale,
         :merchantReturnData)
     end
+
+    def psp_reference
+      params[:pspReference]
+    end
   end
 end

app/models/spree/adyen/notification_processor.rb

@@ -1,11 +1,21 @@
 module Spree
   module Adyen
+    # Class responsible for taking in a notification from Adyen and applying
+    # some form of modification to the associated payment.
+    #
+    # I would in the future like to refactor this by breaking this into
+    # separate classes that are only aware of how to process specific kinds of
+    # notifications (auth, capture, refund, etc.).
     class NotificationProcessor
       attr_accessor :notification, :payment
 
       def initialize(notification, payment = nil)
         self.notification = notification
         self.payment = payment ? payment : notification.payment
+
+        if self.payment.nil?
+          self.payment = create_missing_payment
+        end
       end
 
       # for the given payment, process all notifications that are currently
@@ -97,6 +107,34 @@ def complete_payment!
         payment.update!(amount: payment.captured_amount)
         payment.complete!
       end
+
+      # At this point the auth was received before the redirect, we create
+      # the payment here with the information we have available so that if
+      # the user is not redirected to back for some reason we still have a
+      # record of the payment.
+      def create_missing_payment
+        order = notification.order
+
+        source = Spree::Adyen::HppSource.new(
+          auth_result: "unknown",
+          order: order,
+          payment_method: notification.payment_method,
+          psp_reference: notification.psp_reference
+        )
+
+        payment = order.payments.create!(
+          amount: notification.money.dollars,
+          # We have no idea what payment method they used, this will be
+          # updated when/if they get redirected
+          payment_method: Spree::Gateway::AdyenHPP.last,
+          response_code: notification.psp_reference,
+          source: source,
+          order: order
+        )
+
+        order.complete
+        payment
+      end
     end
   end
 end

lib/spree/adyen/version.rb

@@ -1,5 +1,5 @@
 module Spree
   module Adyen
-    VERSION = "0.1.3"
+    VERSION = "0.2.0"
   end
 end

spec/controllers/spree/adyen_notifications_controller_spec.rb

@@ -5,7 +5,6 @@
 
   routes { Spree::Core::Engine.routes }
 
-  let(:order) { create :order }
   let(:params) do
     { "pspReference" => reference,
       "eventDate" => "2013-10-21T14:45:45.93Z",
@@ -22,12 +21,14 @@
       "live" => "false" }
   end
 
+  let!(:order) { create :completed_order_with_totals }
+
   let!(:payment) do
-    create :payment, response_code: reference,
-      payment_method: payment_method
+    create :hpp_payment, response_code: reference,
+      payment_method: payment_method, order: order
   end
 
-  let(:payment_method) { create :hpp_gateway }
+  let!(:payment_method) { create :hpp_gateway }
 
   let(:reference) { "8513823667306210" }
 

spec/controllers/spree/adyen_redirect_controller_spec.rb

@@ -12,8 +12,8 @@
     )
   end
 
-  let(:store) { Spree::Store.default }
-  let(:gateway) { create :hpp_gateway }
+  let!(:store) { Spree::Store.default }
+  let!(:gateway) { create :hpp_gateway }
 
   before do
     allow(controller).to receive(:check_signature)
@@ -24,6 +24,7 @@
 
     let(:psp_reference) { "8813824003752247" }
     let(:payment_method) { "amex" }
+    let(:merchantReturnData) { "#{order.guest_token}|#{gateway.id}" }
     let(:params) do
       { merchantReference: order.number,
         skinCode: "xxxxxxxx",
@@ -35,28 +36,20 @@
         merchantReturnData: merchantReturnData
       }
     end
-    let(:merchantReturnData) { [order.guest_token, gateway.id].join("|") }
-
-    shared_examples "payments are pending" do
-      it "has pending payments" do
-        expect(order.payments).to all be_pending
-      end
-    end
 
     shared_examples "payment is successful" do
       it "changes the order state to completed" do
-        expect { subject }.
-          to change { order.reload.state }.
-          from("payment").
-          to("complete").
-
-          and change { order.payment_state }.
-          from(nil).
-          to("balance_due").
-
-          and change { order.shipment_state }.
-          from(nil).
-          to("pending")
+        subject
+        order.reload
+        expect(order).to have_attributes(
+          state: "complete",
+          payment_state: "balance_due",
+          shipment_state: "pending"
+        )
+      end
+
+      it "has pending payments" do
+        expect(order.payments).to all be_pending
       end
 
       it "redirects to the order complete page" do
@@ -65,10 +58,8 @@
       end
 
       it "creates a payment" do
-        expect{ subject }.
-          to change{ order.payments.count }.
-          from(0).
-          to(1)
+        subject
+        expect(order.reload.payments.count).to eq 1
       end
 
       context "and the order cannot complete" do
@@ -80,9 +71,20 @@
       end
     end
 
+    shared_examples "payment is not successful" do
+      it "does not change order state" do
+        expect{ subject }.to_not change{ order.state }
+      end
+
+      it "redirects to the order payment page" do
+        is_expected.to have_http_status(:redirect).
+          and redirect_to checkout_state_path("payment")
+      end
+    end
+
     context "when the payment is AUTHORISED" do
       include_examples "payment is successful"
-      include_examples "payments are pending"
+
       let(:auth_result) { "AUTHORISED" }
 
       context "and the authorisation notification has already been received" do
@@ -91,56 +93,40 @@
         let(:notification) do
           create(
             :notification,
-            notification_type,
+            :auth,
+            processed: true,
             psp_reference: psp_reference,
             merchant_reference: order.number)
         end
 
-        shared_examples "auth received" do
-          include_examples "payment is successful"
+        # there will already be a payment and source created at this point
+        before do
+          source =
+            create(:hpp_source, psp_reference: psp_reference, order: order)
 
-          it "processes the notification" do
-            expect { subject }.
-              to change { notification.reload.processed }.
-              from(false).
-              to(true)
-          end
-        end
+          create(:hpp_payment, source: source, order: order)
 
-        context "and payment method is sofort" do
-          let(:notification_type) { :sofort_auth }
-          include_examples "auth received"
+          order.complete
         end
 
-        context "and payment method is ideal" do
-          let(:notification_type) { :ideal_auth }
-          include_examples "auth received"
-        end
+        it { expect { subject }.to_not change { order.payments.count }.from 1 }
 
-        context "and payment method is credit" do
-          let(:notification_type) { :auth }
-          include_examples "auth received"
+        it "updates the source" do
+          expect(order.payments.last.source).to have_attributes(
+            auth_result: "AUTHORISED",
+            skin_code: "XXXXXXXX"
+          )
         end
+
+        include_examples "payment is successful"
       end
     end
 
     context "when the payment is PENDING" do
       include_examples "payment is successful"
-      include_examples "payments are pending"
       let(:auth_result) { "PENDING" }
     end
 
-    shared_examples "payment is not successful" do
-      it "does not change order state" do
-        expect{ subject }.to_not change{ order.state }
-      end
-
-      it "redirects to the order payment page" do
-        is_expected.to have_http_status(:redirect).
-          and redirect_to checkout_state_path("payment")
-      end
-    end
-
     context "when the payment is CANCELLED" do
       include_examples "payment is not successful"
       let(:auth_result) { "CANCELLED" }

spec/factories/spree_payment.rb

@@ -6,6 +6,7 @@
 
     before :create do |record, _|
       # these associations/keys are awful and are making this difficult
+      record.response_code = record.source.psp_reference
       record.source.order = record.order
       record.source.merchant_reference = record.order.number
     end