diff --git a/CHANGELOG.md b/CHANGELOG.md index ff8709e1a24..338e1d1b614 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/api/spec/controllers/spree/api/checkouts_controller_spec.rb b/api/spec/controllers/spree/api/checkouts_controller_spec.rb index dfd9d15e563..572ee48bd70 100644 --- a/api/spec/controllers/spree/api/checkouts_controller_spec.rb +++ b/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, diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 4b49c4c70d3..8f635c001f8 100644 --- a/core/app/models/spree/shipment.rb +++ b/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 diff --git a/core/app/models/spree/stock/coordinator.rb b/core/app/models/spree/stock/coordinator.rb index f7e64b5db9b..38ce54db247 100644 --- a/core/app/models/spree/stock/coordinator.rb +++ b/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 diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb index a9806b5110a..19d22fa61dd 100644 --- a/core/app/models/spree/stock/estimator.rb +++ b/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] 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 diff --git a/core/app/models/spree/stock/package.rb b/core/app/models/spree/stock/package.rb index 9d658bd7fbe..870c5ed34f5 100644 --- a/core/app/models/spree/stock/package.rb +++ b/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] 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 diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 54d1b7e666c..93d3d217252 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -175,7 +175,7 @@ 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) @@ -183,7 +183,7 @@ 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 diff --git a/core/spec/models/spree/stock/estimator_spec.rb b/core/spec/models/spree/stock/estimator_spec.rb index 736f972f681..8eca05b8f29 100644 --- a/core/spec/models/spree/stock/estimator_spec.rb +++ b/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) diff --git a/core/spec/models/spree/stock/package_spec.rb b/core/spec/models/spree/stock/package_spec.rb index f09b1c85d54..fbb6a9569c6 100644 --- a/core/spec/models/spree/stock/package_spec.rb +++ b/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