From 6ea4aac361f553b66d62600465b0caf0431bb3cb Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Wed, 9 Nov 2016 10:42:22 +1100 Subject: [PATCH] WIP: Refactoring StandingOrderUpdater into StandingOrderForm Update logic coupled to update action on controller rather than changes to the model --- .../admin/standing_orders_controller.rb | 35 +++---- app/forms/standing_order_form.rb | 92 ++++++++++++++++++ app/models/standing_line_item.rb | 2 +- app/models/standing_order.rb | 6 -- .../standing_order_updater.rb | 58 ------------ .../standing_order_form_spec.rb} | 94 ++++++++++++------- 6 files changed, 171 insertions(+), 116 deletions(-) create mode 100644 app/forms/standing_order_form.rb delete mode 100644 lib/open_food_network/standing_order_updater.rb rename spec/{lib/open_food_network/standing_order_updater_spec.rb => forms/standing_order_form_spec.rb} (61%) diff --git a/app/controllers/admin/standing_orders_controller.rb b/app/controllers/admin/standing_orders_controller.rb index d2ed37031d..937b0a6448 100644 --- a/app/controllers/admin/standing_orders_controller.rb +++ b/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 diff --git a/app/forms/standing_order_form.rb b/app/forms/standing_order_form.rb new file mode 100644 index 0000000000..855ca889d9 --- /dev/null +++ b/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 diff --git a/app/models/standing_line_item.rb b/app/models/standing_line_item.rb index 70d0eebf82..7e72dc6f41 100644 --- a/app/models/standing_line_item.rb +++ b/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}) diff --git a/app/models/standing_order.rb b/app/models/standing_order.rb index 29db805d57..c32f5627d9 100644 --- a/app/models/standing_order.rb +++ b/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 diff --git a/lib/open_food_network/standing_order_updater.rb b/lib/open_food_network/standing_order_updater.rb deleted file mode 100644 index 1c34084614..0000000000 --- a/lib/open_food_network/standing_order_updater.rb +++ /dev/null @@ -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 diff --git a/spec/lib/open_food_network/standing_order_updater_spec.rb b/spec/forms/standing_order_form_spec.rb similarity index 61% rename from spec/lib/open_food_network/standing_order_updater_spec.rb rename to spec/forms/standing_order_form_spec.rb index 35645fef47..00e59b3324 100644 --- a/spec/lib/open_food_network/standing_order_updater_spec.rb +++ b/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