Merge pull request #965 from jordan-brough/add-rates-to-shipments-ear…

…lier

Connect shipping rates to shipments earlier in Stock::Coordinator

CHANGELOG.md

@@ -1,5 +1,15 @@
 ## Solidus 1.3.0 (unreleased)
 
+*   Changes to Spree::Stock::Estimator
+
+    * The package passed to Spree::Stock::Estimator#shipping_rates must have its
+      shipment assigned and that shipment must have its order assigned. This
+      is needed for some upcoming tax work in to calculate taxes correctly.
+    * Spree::Stock::Estimator.new no longer accepts an order argument. The order
+      will be fetched from the shipment.
+
+    https://github.com/solidusio/solidus/pull/965
+
 *   Made Spree::Order validate :store_id
 
     All orders created since Spree v2.4 should have a store assigned. We want to build more

api/spec/controllers/spree/api/checkouts_controller_spec.rb

@@ -136,7 +136,7 @@ module Spree
 
       it "can update shipping method and transition from delivery to payment" do
         order.update_column(:state, "delivery")
-        shipment = create(:shipment, order: order)
+        shipment = create(:shipment, order: order, address: order.ship_address)
         shipment.refresh_rates
         shipping_rate = shipment.shipping_rates.where(selected: false).first
         api_put :update, id: order.to_param, order_token: order.guest_token,

core/app/models/spree/shipment.rb

@@ -178,7 +178,7 @@ def refresh_rates
       # StockEstimator.new assigment below will replace the current shipping_method
       original_shipping_method_id = shipping_method.try!(:id)
 
-      new_rates = Spree::Config.stock.estimator_class.new(order).shipping_rates(to_package)
+      new_rates = Spree::Config.stock.estimator_class.new.shipping_rates(to_package)
 
       # If one of the new rates matches the previously selected shipping
       # method, select that instead of the default provided by the estimator.
@@ -259,6 +259,7 @@ def tax_total
 
     def to_package
       package = Stock::Package.new(stock_location)
+      package.shipment = self
       inventory_units.includes(:variant).joins(:variant).group_by(&:state).each do |state, state_inventory_units|
         package.add_multiple state_inventory_units, state.to_sym
       end

core/app/models/spree/stock/coordinator.rb

@@ -10,9 +10,7 @@ def initialize(order, inventory_units = nil)
       end
 
       def shipments
-        packages.map do |package|
-          package.to_shipment.tap { |s| s.address = order.ship_address }
-        end
+        packages.map(&:shipment)
       end
 
       private
@@ -21,6 +19,9 @@ def packages
         packages = build_location_configured_packages
         packages = build_packages(packages)
         packages = prioritize_packages(packages)
+        packages.each do |package|
+          package.shipment = package.to_shipment
+        end
         packages = estimate_packages(packages)
         validate_packages(packages)
         packages
@@ -120,9 +121,9 @@ def prioritize_packages(packages)
       end
 
       def estimate_packages(packages)
-        estimator = Spree::Config.stock.estimator_class.new(order)
+        estimator = Spree::Config.stock.estimator_class.new
         packages.each do |package|
-          package.shipping_rates = estimator.shipping_rates(package)
+          package.shipment.shipping_rates = estimator.shipping_rates(package)
         end
         packages
       end

core/app/models/spree/stock/estimator.rb

@@ -1,13 +1,8 @@
 module Spree
   module Stock
     class Estimator
-      attr_reader :order, :currency
-
-      # @param order [Spree::Order] the order whose shipping rates to estimate
-      def initialize(order)
-        @order = order
-        @currency = order.currency
-      end
+      class ShipmentRequired < StandardError; end
+      class OrderRequired < StandardError; end
 
       # Estimate the shipping rates for a package.
       #
@@ -17,6 +12,9 @@ def initialize(order)
       # @return [Array<Spree::ShippingRate>] the shipping rates sorted by
       #   descending cost, with the least costly marked "selected"
       def shipping_rates(package, frontend_only = true)
+        raise ShipmentRequired if package.shipment.nil?
+        raise OrderRequired if package.shipment.order.nil?
+
         rates = calculate_shipping_rates(package)
         rates.select! { |rate| rate.shipping_method.frontend? } if frontend_only
         choose_default_shipping_rate(rates)
@@ -41,12 +39,15 @@ def calculate_shipping_rates(package)
               # If the rate's zone matches the order's zone, a positive adjustment will be applied.
               # If the rate is from the default tax zone, then a negative adjustment will be applied.
               # See the tests in shipping_rate_spec.rb for an example of this.d
-              rate.zone == order.tax_zone || rate.zone.default_tax?
+              rate.zone == package.shipment.order.tax_zone || rate.zone.default_tax?
             end
           end
 
           if cost
-            rate = shipping_method.shipping_rates.new(cost: cost)
+            rate = shipping_method.shipping_rates.new(
+              cost: cost,
+              shipment: package.shipment
+            )
             rate.tax_rate = tax_rate if tax_rate
           end
 
@@ -56,14 +57,14 @@ def calculate_shipping_rates(package)
 
       def shipping_methods(package)
         package.shipping_methods
-          .available_for_address(order.ship_address)
+          .available_for_address(package.shipment.address)
           .includes(:calculator, tax_category: :tax_rates)
           .to_a
           .select do |ship_method|
           calculator = ship_method.calculator
           calculator.available?(package) &&
             (calculator.preferences[:currency].blank? ||
-             calculator.preferences[:currency] == currency)
+             calculator.preferences[:currency] == package.shipment.order.currency)
         end
       end
     end

core/app/models/spree/stock/package.rb

@@ -2,14 +2,13 @@ module Spree
   module Stock
     class Package
       attr_reader :stock_location, :contents
-      attr_accessor :shipping_rates
+      attr_accessor :shipment
 
       # @param stock_location [Spree::StockLocation] the stock location this package originates from
       # @param contents [Array<Spree::Stock::ContentItem>] the contents of this package
       def initialize(stock_location, contents = [])
         @stock_location = stock_location
         @contents = contents
-        @shipping_rates = []
       end
 
       # Adds an inventory unit to this package.
@@ -123,8 +122,9 @@ def to_shipment
         contents.each { |content_item| content_item.inventory_unit.state = content_item.state.to_s }
 
         Spree::Shipment.new(
+          order: order,
+          address: order.ship_address,
           stock_location: stock_location,
-          shipping_rates: shipping_rates,
           inventory_units: contents.map(&:inventory_unit)
         )
       end

core/spec/models/spree/shipment_spec.rb

@@ -175,15 +175,15 @@
       before { allow(shipment).to receive(:can_get_rates?){ true } }
 
       it 'should request new rates, and maintain shipping_method selection' do
-        expect(Spree::Stock::Estimator).to receive(:new).with(shipment.order).and_return(mock_estimator)
+        expect(Spree::Stock::Estimator).to receive(:new).with(no_args).and_return(mock_estimator)
         allow(shipment).to receive_messages(shipping_method: shipping_method2)
 
         expect(shipment.refresh_rates).to eq(shipping_rates)
         expect(shipment.reload.selected_shipping_rate.shipping_method_id).to eq(shipping_method2.id)
       end
 
       it 'should handle no shipping_method selection' do
-        expect(Spree::Stock::Estimator).to receive(:new).with(shipment.order).and_return(mock_estimator)
+        expect(Spree::Stock::Estimator).to receive(:new).with(no_args).and_return(mock_estimator)
         allow(shipment).to receive_messages(shipping_method: nil)
         expect(shipment.refresh_rates).to eq(shipping_rates)
         expect(shipment.reload.selected_shipping_rate).not_to be_nil
@@ -222,6 +222,10 @@
           expect(package.on_hand.count).to eq 1
           expect(package.backordered.count).to eq 1
         end
+
+        it 'should set the shipment to itself' do
+          expect(shipment.to_package.shipment).to eq(shipment)
+        end
       end
     end
   end

core/spec/models/spree/stock/estimator_spec.rb

@@ -5,11 +5,15 @@ module Stock
     describe Estimator, type: :model do
       let(:shipping_rate) { 4.00 }
       let!(:shipping_method) { create(:shipping_method, cost: shipping_rate, currency: currency) }
-      let(:package) { build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(i) }) }
+      let(:package) do
+        build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(i) }).tap do |p|
+          p.shipment = p.to_shipment
+        end
+      end
       let(:order) { create(:order_with_line_items, shipping_method: shipping_method) }
       let(:inventory_units) { order.inventory_units }
 
-      subject { Estimator.new(order) }
+      subject { Estimator.new }
 
       context "#shipping rates" do
         before(:each) do
@@ -18,6 +22,24 @@ module Stock
 
         let(:currency) { "USD" }
 
+        context 'without a shipment' do
+          before { package.shipment = nil }
+          it 'raises an error' do
+            expect {
+              subject.shipping_rates(package)
+            }.to raise_error(Spree::Stock::Estimator::ShipmentRequired)
+          end
+        end
+
+        context 'without an order' do
+          before { package.shipment.order = nil }
+          it 'raises an error' do
+            expect {
+              subject.shipping_rates(package)
+            }.to raise_error(Spree::Stock::Estimator::OrderRequired)
+          end
+        end
+
         shared_examples_for "shipping rate matches" do
           it "returns shipping rates" do
             shipping_rates = subject.shipping_rates(package)

core/spec/models/spree/stock/package_spec.rb

@@ -10,7 +10,7 @@ module Stock
       subject { Package.new(stock_location) }
 
       def build_inventory_unit
-        build(:inventory_unit, variant: variant)
+        build(:inventory_unit, variant: variant, order: order)
       end
 
       it 'calculates the weight of all the contents' do
@@ -97,9 +97,9 @@ def build_inventory_unit
         subject.add build_inventory_unit, :backordered
 
         shipping_method = build(:shipping_method)
-        subject.shipping_rates = [Spree::ShippingRate.new(shipping_method: shipping_method, cost: 10.00, selected: true)]
 
         shipment = subject.to_shipment
+        shipment.shipping_rates = [Spree::ShippingRate.new(shipping_method: shipping_method, cost: 10.00, selected: true)]
         expect(shipment.stock_location).to eq subject.stock_location
         expect(shipment.inventory_units.size).to eq 3
 
@@ -113,6 +113,7 @@ def build_inventory_unit
         expect(last_unit.state).to eq 'backordered'
 
         expect(shipment.shipping_method).to eq shipping_method
+        expect(shipment.address).to eq order.ship_address
       end
 
       it 'does not add an inventory unit to a package twice' do