From ceab1fe16af4dbe66386f8833d1f004411b356b5 Mon Sep 17 00:00:00 2001
From: Rob Harrington <oeoeaio@gmail.com>
Date: Thu, 1 Feb 2018 10:25:58 +1100
Subject: [PATCH] Reduce cognitive complexity of StandingOrderPaymentUpdater

---
 app/jobs/standing_order_confirm_job.rb        | 10 +-----
 .../standing_order_payment_updater.rb         | 32 ++++++++++++-------
 spec/jobs/standing_order_confirm_job_spec.rb  | 28 ----------------
 .../standing_order_payment_updater_spec.rb    |  4 +--
 4 files changed, 24 insertions(+), 50 deletions(-)

diff --git a/app/jobs/standing_order_confirm_job.rb b/app/jobs/standing_order_confirm_job.rb
index 07515232aa..21f25e5626 100644
--- a/app/jobs/standing_order_confirm_job.rb
+++ b/app/jobs/standing_order_confirm_job.rb
@@ -45,11 +45,7 @@ class StandingOrderConfirmJob
   end
 
   def update_payment!
-    result = payment_updater.new(@order).update!
-    case result
-    when :no_card
-      @order.errors.add(:base, :no_card)
-    end
+    OpenFoodNetwork::StandingOrderPaymentUpdater.new(@order).update!
   end
 
   def send_confirm_email
@@ -61,8 +57,4 @@ class StandingOrderConfirmJob
     record_and_log_error(:failed_payment, @order)
     StandingOrderMailer.failed_payment_email(@order).deliver
   end
-
-  def payment_updater
-    OpenFoodNetwork::StandingOrderPaymentUpdater
-  end
 end
diff --git a/lib/open_food_network/standing_order_payment_updater.rb b/lib/open_food_network/standing_order_payment_updater.rb
index dc9352de1c..14e44eacb1 100644
--- a/lib/open_food_network/standing_order_payment_updater.rb
+++ b/lib/open_food_network/standing_order_payment_updater.rb
@@ -5,25 +5,26 @@ module OpenFoodNetwork
     end
 
     def update!
-      create_payment if payment.blank?
+      create_payment
+      ensure_payment_source
+      return if order.errors.any?
 
-      if card_required? && !card_set?
-        return :no_card unless ensure_credit_card
-      end
-
-      payment.update_attributes(amount: @order.outstanding_balance)
+      payment.update_attributes(amount: order.outstanding_balance)
     end
 
     private
 
+    attr_reader :order
+
     def payment
-      @payment ||= @order.pending_payments.last
+      @payment ||= order.pending_payments.last
     end
 
     def create_payment
-      @payment = @order.payments.create(
-        payment_method_id: @order.standing_order.payment_method_id,
-        amount: @order.outstanding_balance
+      return if payment.present?
+      @payment = order.payments.create(
+        payment_method_id: order.standing_order.payment_method_id,
+        amount: order.outstanding_balance
       )
     end
 
@@ -35,13 +36,22 @@ module OpenFoodNetwork
       payment.source is_a? Spree::CreditCard
     end
 
+    def ensure_payment_source
+      return unless card_required? && !card_set?
+      ensure_credit_card || order.errors.add(:base, :no_card)
+    end
+
     def ensure_credit_card
       return false if saved_credit_card.blank?
       payment.update_attributes(source: saved_credit_card)
     end
 
     def saved_credit_card
-      @order.standing_order.credit_card
+      order.standing_order.credit_card
+    end
+
+    def errors_present?
+      order.errors.any?
     end
   end
 end
diff --git a/spec/jobs/standing_order_confirm_job_spec.rb b/spec/jobs/standing_order_confirm_job_spec.rb
index e82377a58d..3725e1648b 100644
--- a/spec/jobs/standing_order_confirm_job_spec.rb
+++ b/spec/jobs/standing_order_confirm_job_spec.rb
@@ -101,34 +101,6 @@ describe StandingOrderConfirmJob do
     end
   end
 
-  describe "updating the payment" do
-    let(:order) { create(:order) }
-    let(:payment_updater_mock) { double(:payment_updater) }
-
-    before do
-      job.instance_variable_set(:@order, order)
-      allow(OpenFoodNetwork::StandingOrderPaymentUpdater).to receive(:new) { payment_updater_mock }
-    end
-
-    context "when the updater returns true" do
-      before { expect(payment_updater_mock).to receive(:update!) { true } }
-
-      it "does nothing" do
-        job.send(:update_payment!)
-        expect(order.errors).to be_empty
-      end
-    end
-
-    context "when the updater returns an error code" do
-      before { expect(payment_updater_mock).to receive(:update!) { :no_card } }
-
-      it "adds and error to the order" do
-        expect{ job.send(:update_payment!) }.to change(order.errors, :count).from(0).to(1)
-        expect(order.errors.full_messages).to include I18n.t("activerecord.errors.models.spree/order.no_card")
-      end
-    end
-  end
-
   describe "processing an order" do
     let(:shop) { create(:distributor_enterprise) }
     let(:order_cycle1) { create(:simple_order_cycle, coordinator: shop) }
diff --git a/spec/lib/open_food_network/standing_order_payment_updater_spec.rb b/spec/lib/open_food_network/standing_order_payment_updater_spec.rb
index 7b8f40772f..e932e4e682 100644
--- a/spec/lib/open_food_network/standing_order_payment_updater_spec.rb
+++ b/spec/lib/open_food_network/standing_order_payment_updater_spec.rb
@@ -99,9 +99,9 @@ module OpenFoodNetwork
             context "and no credit card is available on the standing order" do
               before { expect(updater).to receive(:ensure_credit_card) { false } }
 
-              it "returns an error code and does not update the payment" do
+              it "adds an error to the order and does not update the payment" do
                 expect(payment).to_not receive(:update_attributes)
-                expect(updater.update!).to eq :no_card
+                expect{ updater.update! }.to change(order.errors[:base], :count).from(0).to(1)
               end
             end