WIP: Refactoring StandingOrderUpdater into StandingOrderForm

Update logic coupled to update action on controller rather than changes to the model

app/controllers/admin/standing_orders_controller.rb

@@ -8,16 +8,6 @@ module Admin
     before_filter :wrap_nested_attrs, only: [:create, :update]
     respond_to :json
 
-    respond_override create: { json: {
-      success: lambda { render_as_json @standing_order, fee_calculator: fee_calculator },
-      failure: lambda { render json: { errors: json_errors }, status: :unprocessable_entity }
-    } }
-
-    respond_override update: { json: {
-      success: lambda { render_as_json @standing_order, fee_calculator: fee_calculator },
-      failure: lambda { render json: { errors: json_errors }, status: :unprocessable_entity }
-    } }
-
     def index
       respond_to do |format|
         format.html
@@ -30,6 +20,24 @@ module Admin
       @standing_order.ship_address = Spree::Address.new
     end
 
+    def create
+      form = StandingOrderForm.new(@standing_order, params[:standing_order])
+      if form.save
+        render_as_json @standing_order, fee_calculator: fee_calculator
+      else
+        render json: { errors: form.json_errors }, status: :unprocessable_entity
+      end
+    end
+
+    def update
+      form = StandingOrderForm.new(@standing_order, params[:standing_order])
+      if form.save
+        render_as_json @standing_order, fee_calculator: fee_calculator
+      else
+        render json: { errors: form.json_errors }, status: :unprocessable_entity
+      end
+    end
+
     private
 
     def permissions
@@ -64,13 +72,6 @@ module Admin
       OpenFoodNetwork::EnterpriseFeeCalculator.new(shop, next_oc)
     end
 
-    def json_errors
-      @object.errors.messages.inject({}) do |errors, (k,v)|
-        errors[k] = v.map{ |msg| @object.errors.full_message(k,msg) }
-        errors
-      end
-    end
-
     # Wrap :standing_line_items_attributes in :standing_order root
     def wrap_nested_attrs
       if params[:standing_line_items].is_a? Array

app/forms/standing_order_form.rb

@@ -0,0 +1,92 @@
+class StandingOrderForm
+  include ActiveModel::Naming
+  include ActiveModel::Conversion
+  include ActiveModel::Validations
+
+  attr_accessor :standing_order, :params
+
+  delegate :orders, :order_cycles, :bill_address, :ship_address, :standing_line_items, to: :standing_order
+  delegate :shop, :shop_id, :customer, :customer_id, to: :standing_order
+  delegate :shipping_method, :shipping_method_id, :payment_method, :payment_method_id, to: :standing_order
+  delegate :shipping_method_id_changed?, :shipping_method_id_was, :payment_method_id_changed?, :payment_method_id_was, to: :standing_order
+
+  def initialize(standing_order, params)
+    @standing_order = standing_order
+    @params = params
+  end
+
+  def save
+    @standing_order.transaction do
+      @standing_order.assign_attributes(params)
+
+      initialise_orders!
+
+      orders.update_all(customer_id: customer_id, email: customer.andand.email, distributor_id: shop_id)
+
+      orders.each do |order|
+        if shipping_method_id_changed?
+          shipment = order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id_was).last
+          if shipment
+            shipment.update_attributes(shipping_method_id: shipping_method_id)
+            order.update_attribute(:shipping_method_id, shipping_method_id)
+          end
+        end
+
+        if payment_method_id_changed?
+          payment = order.payments.with_state('checkout').where(payment_method_id: payment_method_id_was).last
+          if payment
+            payment.andand.void_transaction!
+            create_payment_for(order)
+          end
+        end
+      end
+
+      standing_order.save
+    end
+  end
+
+  def json_errors
+    @standing_order.errors.messages.inject({}) do |errors, (k,v)|
+      errors[k] = v.map{ |msg| @standing_order.errors.full_message(k,msg) }
+      errors
+    end
+  end
+
+  private
+
+  def future_and_undated_orders
+    orders.joins(:order_cycle).merge(OrderCycle.not_closed)
+  end
+
+  def create_order_for(order_cycle_id)
+    order = Spree::Order.create!({
+      customer_id: customer_id,
+      email: customer.email,
+      order_cycle_id: order_cycle_id,
+      distributor_id: shop_id,
+      shipping_method_id: shipping_method_id,
+    })
+    standing_line_items.each do |sli|
+      order.line_items.create(variant_id: sli.variant_id, quantity: sli.quantity)
+    end
+    order.update_attributes(bill_address: bill_address.dup, ship_address: ship_address.dup)
+    order.update_distribution_charge!
+    create_payment_for(order)
+
+    order
+  end
+
+  def create_payment_for(order)
+    order.payments.create(payment_method_id: payment_method_id, amount: order.reload.total)
+  end
+
+  def initialise_orders!
+    uninitialised_order_cycle_ids.each do |order_cycle_id|
+      orders << create_order_for(order_cycle_id)
+    end
+  end
+
+  def uninitialised_order_cycle_ids
+    order_cycles.pluck(:id) - orders.map(&:order_cycle_id)
+  end
+end

app/models/standing_line_item.rb

@@ -10,7 +10,7 @@ class StandingLineItem < ActiveRecord::Base
   validates :variant, presence: true
   validates :quantity, { presence: true, numericality: { only_integer: true } }
 
-  before_save :update_line_items! # In OpenFoodNetwork::StandingLineItemUpdater
+  # before_save :update_line_items! # In OpenFoodNetwork::StandingLineItemUpdater
 
   def available_from?(shop, schedule)
     Spree::Variant.joins(exchanges: { order_cycle: :schedules})

app/models/standing_order.rb

@@ -1,10 +1,4 @@
-require 'open_food_network/standing_order_updater'
-
 class StandingOrder < ActiveRecord::Base
-  include OpenFoodNetwork::StandingOrderUpdater
-
-  before_save :update_orders!
-
   belongs_to :shop, class_name: 'Enterprise'
   belongs_to :customer
   belongs_to :schedule

lib/open_food_network/standing_order_updater.rb

@@ -1,58 +0,0 @@
-module OpenFoodNetwork
-  module StandingOrderUpdater
-
-    def update_orders!
-      uninitialised_order_cycle_ids.each do |order_cycle_id|
-        orders << create_order_for(order_cycle_id)
-      end
-
-      orders.update_all(customer_id: customer_id, email: customer.email, distributor_id: shop_id, shipping_method_id: shipping_method_id)
-
-      orders.each do |order|
-        if shipping_method_id_changed?
-          shipment = order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id_was).last
-          shipment.andand.update_attributes(shipping_method_id: shipping_method_id)
-        end
-        if payment_method_id_changed?
-          payment = order.payments.with_state('checkout').where(payment_method_id: payment_method_id_was).last
-          if payment
-            payment.andand.void_transaction!
-            create_payment_for(order)
-          end
-        end
-      end
-    end
-
-    def future_and_undated_orders
-      orders.joins(:order_cycle).merge(OrderCycle.not_closed)
-    end
-
-    private
-
-    def create_order_for(order_cycle_id)
-      order = Spree::Order.create!({
-        customer_id: customer_id,
-        email: customer.email,
-        order_cycle_id: order_cycle_id,
-        distributor_id: shop_id,
-        shipping_method_id: shipping_method_id,
-      })
-      standing_line_items.each do |sli|
-        order.line_items.create(variant_id: sli.variant_id, quantity: sli.quantity)
-      end
-      order.update_attributes(bill_address: bill_address.dup, ship_address: ship_address.dup)
-      order.update_distribution_charge!
-      create_payment_for(order)
-
-      order
-    end
-
-    def create_payment_for(order)
-      order.payments.create(payment_method_id: payment_method_id, amount: order.reload.total)
-    end
-
-    def uninitialised_order_cycle_ids
-      order_cycles.pluck(:id) - orders.map(&:order_cycle_id)
-    end
-  end
-end

spec/forms/standing_order_form_spec.rb

@@ -1,7 +1,5 @@
-require 'open_food_network/standing_order_updater'
-
 module OpenFoodNetwork
-  describe StandingOrderUpdater do
+  describe StandingOrderForm do
     describe "creating a new standing order" do
       let!(:shop) { create(:distributor_enterprise) }
       let!(:customer) { create(:customer, enterprise: shop) }
@@ -22,24 +20,29 @@ module OpenFoodNetwork
       let!(:payment_method) { create(:payment_method, distributors: [shop]) }
       let!(:shipping_method) { create(:shipping_method, distributors: [shop]) }
       let!(:address) { create(:address) }
+      let(:standing_order) { StandingOrder.new }
 
-      let!(:standing_order) { StandingOrder.create({
-        shop: shop,
-        customer: customer,
-        schedule: schedule,
-        bill_address: address.clone,
-        ship_address: address.clone,
-        payment_method: payment_method,
-        shipping_method: shipping_method,
+      let!(:params) { {
+        shop_id: shop.id,
+        customer_id: customer.id,
+        schedule_id: schedule.id,
+        bill_address_attributes: address.clone.attributes,
+        ship_address_attributes: address.clone.attributes,
+        payment_method_id: payment_method.id,
+        shipping_method_id: shipping_method.id,
         begins_at: 2.weeks.ago,
         standing_line_items_attributes: [
-          {variant: variant1, quantity: 1},
-          {variant: variant2, quantity: 2},
-          {variant: variant3, quantity: 3}
+          {variant_id: variant1.id, quantity: 1},
+          {variant_id: variant2.id, quantity: 2},
+          {variant_id: variant3.id, quantity: 3}
         ]
-      }) }
+      } }
+
+      let(:form) { StandingOrderForm.new(standing_order, params) }
 
       it "creates orders for each order cycle in the schedule" do
+        form.save
+
         expect(standing_order.orders.count).to be 3
 
         # Add line items for variants that aren't yet available from the order cycle
@@ -58,24 +61,38 @@ module OpenFoodNetwork
 
     describe "changing the shipping method" do
       let(:standing_order) { create(:standing_order_with_items) }
-      let!(:order) { standing_order.orders.first }
-      let!(:new_shipping_method) { create(:shipping_method, distributors: [standing_order.shop]) }
+      let(:shipping_method) { standing_order.shipping_method }
+      let(:new_shipping_method) { create(:shipping_method, distributors: [standing_order.shop]) }
+      let(:order) { standing_order.orders.first }
+      let(:params) { { shipping_method_id: new_shipping_method.id } }
+      let(:form) { StandingOrderForm.new(standing_order, params) }
 
       context "when the shipping method on an order is the same as the standing order" do
+        before { form.send(:initialise_orders!) }
+
         it "updates the shipping_method on the order and on shipments" do
-          standing_order.update_attributes(shipping_method: new_shipping_method)
-          expect(order.reload.shipping_method).to eq new_shipping_method
-          expect(order.reload.shipments.first.shipping_method).to eq new_shipping_method
+          expect(order.shipments.first.shipping_method).to eq shipping_method
+          form.save
+          expect(order.shipping_method).to eq new_shipping_method
+          expect(order.shipments.first.shipping_method).to eq new_shipping_method
         end
       end
 
       context "when the shipping method on a shipment is not the same as the standing order" do
-        let!(:changed_shipping_method) { create(:shipping_method) }
-        before { order.shipments.first.update_attributes(shipping_method: changed_shipping_method) }
+        let(:changed_shipping_method) { create(:shipping_method) }
 
-        it "updates the shipping_method on the order but not on pre-altered shipments" do
-          standing_order.update_attributes(shipping_method: new_shipping_method)
-          expect(order.reload.shipping_method).to eq new_shipping_method
+        before do
+          form.send(:initialise_orders!)
+          # Updating the shipping method on a shipment updates the shipping method on the order,
+          # and vice-versa via logic in Spree's shipments controller. So updating both here mimics that
+          # behaviour.
+          order.shipments.first.update_attributes(shipping_method_id: changed_shipping_method.id)
+          order.update_attributes(shipping_method_id: changed_shipping_method.id)
+          form.save
+        end
+
+        it "does not update the shipping_method on the standing order or on the pre-altered shipment" do
+          expect(order.reload.shipping_method).to eq changed_shipping_method
           expect(order.reload.shipments.first.shipping_method).to eq changed_shipping_method
         end
       end
@@ -83,13 +100,18 @@ module OpenFoodNetwork
 
     describe "changing the payment method" do
       let(:standing_order) { create(:standing_order_with_items) }
-      let!(:order) { standing_order.orders.first }
-      let!(:payment_method) { standing_order.payment_method }
-      let!(:new_payment_method) { create(:payment_method, distributors: [standing_order.shop]) }
+      let(:order) { standing_order.orders.first }
+      let(:payment_method) { standing_order.payment_method }
+      let(:new_payment_method) { create(:payment_method, distributors: [standing_order.shop]) }
+      let(:params) { { payment_method_id: new_payment_method.id } }
+      let(:form) { StandingOrderForm.new(standing_order, params) }
 
       context "when the payment method on an order is the same as the standing order" do
-        it "updates the payment_method on payments" do
-          standing_order.update_attributes(payment_method: new_payment_method)
+        before { form.send(:initialise_orders!) }
+
+        it "voids existing payments and creates a new payment with the relevant payment method" do
+          expect(order.payments.reload.first.payment_method).to eq payment_method
+          form.save
           payments = order.reload.payments
           expect(payments.count).to be 2
           expect(payments.with_state('void').count).to be 1
@@ -100,11 +122,15 @@ module OpenFoodNetwork
       end
 
       context "when the payment method on a payment is not the same as the standing order" do
-        let!(:changed_payment_method) { create(:payment_method) }
-        before { order.payments.first.update_attribute(:payment_method, changed_payment_method) }
+        let(:changed_payment_method) { create(:payment_method) }
 
-        it "does not update the payment_method on pre-altered payments" do
-          standing_order.update_attributes(payment_method: new_payment_method)
+        before do
+          form.send(:initialise_orders!)
+          order.payments.first.update_attribute(:payment_method_id, changed_payment_method.id)
+          form.save
+        end
+
+        it "keeps pre-altered payments" do
           payments = order.reload.payments
           expect(payments.count).to be 1
           expect(payments.first.payment_method).to eq changed_payment_method