From 9e986f25f1f3afb249241352b9459419ec72a259 Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 16 Dec 2016 14:43:14 +1100 Subject: [PATCH] Orders are not marked as problematic if the changed value matches the new value --- app/forms/standing_order_form.rb | 22 +++-- spec/forms/standing_order_form_spec.rb | 112 ++++++++++++++++++------- 2 files changed, 98 insertions(+), 36 deletions(-) diff --git a/app/forms/standing_order_form.rb b/app/forms/standing_order_form.rb index 65f0ebc53c..17ba4663f5 100644 --- a/app/forms/standing_order_form.rb +++ b/app/forms/standing_order_form.rb @@ -3,7 +3,7 @@ class StandingOrderForm include ActiveModel::Conversion include ActiveModel::Validations - attr_accessor :standing_order, :params, :fee_calculator, :problematic_orders + attr_accessor :standing_order, :params, :fee_calculator, :order_update_issues delegate :orders, :order_cycles, :bill_address, :ship_address, :standing_line_items, to: :standing_order delegate :shop, :shop_id, :customer, :customer_id, :begins_at, :ends_at, :proxy_orders, to: :standing_order @@ -14,7 +14,7 @@ class StandingOrderForm @standing_order = standing_order @params = params @fee_calculator = fee_calculator - @problematic_orders = [] + @order_update_issues = {} end def save @@ -56,7 +56,10 @@ class StandingOrderForm if line_item.quantity == sli.quantity_was line_item.update_attributes(quantity: sli.quantity, skip_stock_check: true) else - @problematic_orders |= [order] + unless line_item.quantity == sli.quantity + product_name = "#{line_item.product.name} - #{line_item.full_name}" + add_order_update_issue(order, product_name) + end end end @@ -81,7 +84,9 @@ class StandingOrderForm payment.andand.void_transaction! order.payments.create(payment_method_id: payment_method_id, amount: order.reload.total) else - @problematic_orders |= [order] + unless order.payments.with_state('checkout').where(payment_method_id: payment_method_id).any? + add_order_update_issue(order, I18n.t('admin.payment_method')) + end end end @@ -91,7 +96,9 @@ class StandingOrderForm shipment.update_attributes(shipping_method_id: shipping_method_id) order.update_attribute(:shipping_method_id, shipping_method_id) else - @problematic_orders |= [order] + unless order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id).any? + add_order_update_issue(order, I18n.t('admin.shipping_method')) + end end end @@ -151,4 +158,9 @@ class StandingOrderForm fees = fee_calculator.indexed_fees_for(variant) (variant.price + fees).to_d end + + def add_order_update_issue(order, issue) + order_update_issues[order.id] ||= [] + order_update_issues[order.id] << issue + end end diff --git a/spec/forms/standing_order_form_spec.rb b/spec/forms/standing_order_form_spec.rb index 237eff0fc6..74a3c89764 100644 --- a/spec/forms/standing_order_form_spec.rb +++ b/spec/forms/standing_order_form_spec.rb @@ -121,22 +121,41 @@ describe StandingOrderForm do 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) } + context "when the shipping method on a shipment is not the same as the original shipping method on the standing order" do + context "when the shipping method on a shipment is the same as the new shipping method on the standing order" do + before do + # 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: new_shipping_method.id) + order.update_attributes(shipping_method_id: new_shipping_method.id) + expect(form.save).to be true + end - before do - # 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) - expect(form.save).to be true + 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 new_shipping_method + expect(order.reload.shipments.first.shipping_method).to eq new_shipping_method + expect(form.order_update_issues[order.id]).to be nil + end 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 - expect(form.problematic_orders).to include order + context "when the shipping method on a shipment is not the same as the new shipping method on the standing order" do + let(:changed_shipping_method) { create(:shipping_method) } + + before do + # 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) + expect(form.save).to be true + 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 + expect(form.order_update_issues[order.id]).to include "Shipping Method" + end end end end @@ -163,18 +182,34 @@ describe StandingOrderForm do 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) } + context "when the payment method on a payment is the same as the original payment method on the standing order" do + before do + order.payments.first.update_attribute(:payment_method_id, new_payment_method.id) + expect(form.save).to be true + end - before do - order.payments.first.update_attribute(:payment_method_id, changed_payment_method.id) - expect(form.save).to be true + it "keeps pre-altered payments and doesn't add an issue to order_update_issues" do + payments = order.reload.payments + expect(payments.count).to be 1 + expect(payments.first.payment_method).to eq new_payment_method + expect(form.order_update_issues[order.id]).to be nil + end 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 - expect(form.problematic_orders).to include order + context "when the payment method on a shipment is not the same as the original payment method on the standing order" do + let(:changed_payment_method) { create(:payment_method) } + + before do + order.payments.first.update_attribute(:payment_method_id, changed_payment_method.id) + expect(form.save).to be true + end + + it "keeps pre-altered payments and adds an issue to order_update_issues" do + payments = order.reload.payments + expect(payments.count).to be 1 + expect(payments.first.payment_method).to eq changed_payment_method + expect(form.order_update_issues[order.id]).to include "Payment Method" + end end end end @@ -207,7 +242,7 @@ describe StandingOrderForm do before { variant.update_attribute(:count_on_hand, 2) } - context "when quantity is less than available stock" do + context "when quantity is within available stock" do let(:params) { { standing_line_items_attributes: [ { id: sli.id, quantity: 2} ] } } let(:form) { StandingOrderForm.new(standing_order, params) } @@ -238,15 +273,30 @@ describe StandingOrderForm do let(:form) { StandingOrderForm.new(standing_order, params) } let(:changed_line_item) { order.line_items.find_by_variant_id(sli.variant_id) } - before { changed_line_item.update_attributes(quantity: 2) } + before { variant.update_attribute(:count_on_hand, 3) } - it "does not change the quantity, and adds the order to the problematic_orders list" do - expect(order.reload.total.to_f).to eq 79.96 - expect(form.save).to be true - line_items = Spree::LineItem.where(order_id: standing_order.orders, variant_id: sli.variant_id) - expect(line_items.map(&:quantity)).to eq [2] - expect(order.reload.total.to_f).to eq 79.96 - expect(form.problematic_orders).to include order + context "when the changed line_item quantity matches the new quantity on the standing line item" do + before { changed_line_item.update_attributes(quantity: 3) } + + it "does not change the quantity, and doesn't add the order to order_update_issues" do + expect(order.reload.total.to_f).to eq 99.95 + expect(form.save).to be true + expect(changed_line_item.reload.quantity).to eq 3 + expect(order.reload.total.to_f).to eq 99.95 + expect(form.order_update_issues[order.id]).to be nil + end + end + + context "when the changed line_item quantity doesn't match the new quantity on the standing line item" do + before { changed_line_item.update_attributes(quantity: 2) } + + it "does not change the quantity, and adds the order to order_update_issues" do + expect(order.reload.total.to_f).to eq 79.96 + expect(form.save).to be true + expect(changed_line_item.reload.quantity).to eq 2 + expect(order.reload.total.to_f).to eq 79.96 + expect(form.order_update_issues[order.id]).to include "#{changed_line_item.product.name} - #{changed_line_item.full_name}" + end end end end