From 1fdef1d9252aaf8dc3809cb20a28f1efe466b36c Mon Sep 17 00:00:00 2001 From: Sofia Besenski Date: Tue, 30 Jun 2026 13:36:38 -0700 Subject: [PATCH] Move create_proposed_shipments to OrderShipping Spree::OrderShipping is a configurable class, which increases the extensibility for this system behaviour. The Spree::Order model also shouldn't need to be responsible for the creation of its own shipments. Separating this concern will allow us to refactor the underlying stock coordination implementation without introducing additional churn on the already heavy Spree::Order class. Co-authored-by: Adam Mueller --- core/app/models/spree/order.rb | 19 ++++---- core/app/models/spree/order_shipping.rb | 17 +++++++ core/spec/models/spree/order_shipping_spec.rb | 44 +++++++++++++++++++ core/spec/models/spree/order_spec.rb | 43 ------------------ 4 files changed, 69 insertions(+), 54 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index d1bc0cc54f6..be11aa9c1f2 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -307,6 +307,14 @@ def all_inventory_units_returned? inventory_units.reload.all?(&:returned?) end + # Since this method is called from within the order state machine, we cannot + # simply delegate this to :shipping. The state_machines gem uses the arity + # of the method to determine if it should pass in the transition as an + # argument. The arity of the method is masked when using delegates. + def create_proposed_shipments + shipping.create_proposed_shipments + end + def contents @contents ||= Spree::Config.order_contents_class.new(self) end @@ -501,17 +509,6 @@ def billing_address_required? Spree::Config.billing_address_required end - def create_proposed_shipments - if completed? - raise CannotRebuildShipments.new(I18n.t("spree.cannot_rebuild_shipments_order_completed")) - elsif shipments.any? { |shipment| !shipment.pending? } - raise CannotRebuildShipments.new(I18n.t("spree.cannot_rebuild_shipments_shipments_not_pending")) - else - shipments.destroy_all - shipments.push(*Spree::Config.stock.coordinator_class.new(self).shipments) - end - end - def create_shipments_for_line_item(line_item) units = Spree::Config.stock.inventory_unit_builder_class.new(self).missing_units_for_line_item(line_item) diff --git a/core/app/models/spree/order_shipping.rb b/core/app/models/spree/order_shipping.rb index a60a86d6e7f..03cbfce61e9 100644 --- a/core/app/models/spree/order_shipping.rb +++ b/core/app/models/spree/order_shipping.rb @@ -8,6 +8,23 @@ def initialize(order) @order = order end + # A method that destroys the current shipments on an order, creates new + # shipments for the line items, and assigns them to the order. + # + # @return [Array] The created shipments + # @raise [Spree::Order::CannotRebuildShipments] raised if order is completed + # or there are any non-pending shipments. + def create_proposed_shipments + if @order.completed? + raise Spree::Order::CannotRebuildShipments.new(I18n.t("spree.cannot_rebuild_shipments_order_completed")) + elsif @order.shipments.any? { |shipment| !shipment.pending? } + raise Spree::Order::CannotRebuildShipments.new(I18n.t("spree.cannot_rebuild_shipments_shipments_not_pending")) + else + @order.shipments.destroy_all + @order.shipments.push(*Spree::Config.stock.coordinator_class.new(@order).shipments) + end + end + # A shortcut method that ships *all* inventory units in a shipment in a single # carton. See also {#ship}. # diff --git a/core/spec/models/spree/order_shipping_spec.rb b/core/spec/models/spree/order_shipping_spec.rb index e6f98184d95..caa245c6ac9 100644 --- a/core/spec/models/spree/order_shipping_spec.rb +++ b/core/spec/models/spree/order_shipping_spec.rb @@ -53,6 +53,50 @@ def emails end end + describe "#create_proposed_shipments" do + subject(:order) { create(:order) } + + it "assigns the coordinator returned shipments to its shipments" do + shipment = build(:shipment) + allow_any_instance_of(Spree::Stock::SimpleCoordinator).to receive(:shipments).and_return([shipment]) + order.shipping.create_proposed_shipments + expect(order.shipments).to eq [shipment] + end + + it "raises an error if any shipments are ready" do + shipment = create(:shipment, order:, state: "ready") + + expect { + expect { + order.shipping.create_proposed_shipments + }.to raise_error(Spree::Order::CannotRebuildShipments) + }.not_to change { order.reload.shipments.pluck(:id) } + + expect { shipment.reload }.not_to raise_error + end + + it "raises an error if any shipments are shipped" do + shipment = create(:shipment, order:, state: "shipped") + expect { + expect { + order.shipping.create_proposed_shipments + }.to raise_error(Spree::Order::CannotRebuildShipments) + }.not_to change { order.reload.shipments.pluck(:id) } + + expect { shipment.reload }.not_to raise_error + end + + context "when the order is already completed" do + let(:order) { create(:completed_order_with_pending_payment) } + + it "raises an error" do + expect { + order.shipping.create_proposed_shipments + }.to raise_error(Spree::Order::CannotRebuildShipments) + end + end + end + describe "#ship" do subject do order.shipping.ship( diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 9ca1d93c319..6dec982e7b4 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1247,49 +1247,6 @@ def call end end - describe "#create_proposed_shipments" do - subject(:order) { create(:order) } - it "assigns the coordinator returned shipments to its shipments" do - shipment = build(:shipment) - allow_any_instance_of(Spree::Stock::SimpleCoordinator).to receive(:shipments).and_return([shipment]) - subject.create_proposed_shipments - expect(subject.shipments).to eq [shipment] - end - - it "raises an error if any shipments are ready" do - shipment = create(:shipment, order: subject, state: "ready") - - expect { - expect { - subject.create_proposed_shipments - }.to raise_error(Spree::Order::CannotRebuildShipments) - }.not_to change { subject.reload.shipments.pluck(:id) } - - expect { shipment.reload }.not_to raise_error - end - - it "raises an error if any shipments are shipped" do - shipment = create(:shipment, order: subject, state: "shipped") - expect { - expect { - subject.create_proposed_shipments - }.to raise_error(Spree::Order::CannotRebuildShipments) - }.not_to change { subject.reload.shipments.pluck(:id) } - - expect { shipment.reload }.not_to raise_error - end - - context "when the order is already completed" do - let(:order) { create(:completed_order_with_pending_payment) } - - it "raises an error" do - expect { - order.create_proposed_shipments - }.to raise_error(Spree::Order::CannotRebuildShipments) - end - end - end - describe "#all_inventory_units_returned?" do let(:order) { create(:order_with_line_items, line_items_count: 3) }