Split generation of packages based on ProductPackages

Previously, product with associated product packages were computed twice
(once based on weight and added in the general packages) and once for
each product_packages. This commit aims to split the generation so
product are added once based on the existence of associated
product_packages

app/models/spree/content_item_decorator.rb

@@ -0,0 +1,7 @@
+module Spree
+  module Stock
+  	ContentItem.class_eval do
+		delegate :has_product_packages?, to: :variant, prefix: true
+	end
+  end
+end

app/models/spree/package_builder.rb

@@ -15,7 +15,7 @@ def process(solidus_package)
       # We compute and set the max_weight once, at the beginning of the process
       @max_weight = get_max_weight(solidus_package)
 
-      packages(solidus_package)
+      to_packages(solidus_package)
     end
 
     # Configuration 
@@ -37,8 +37,8 @@ def max_weight_per_package
 
     private
 
-    def convert_package_to_weights_array(solidus_package)
-      weights = solidus_package.contents.map do |content_item|
+    def convert_package_to_weights_array(product_with_product_packages)
+      weights = product_with_product_packages.map do |content_item|
         item_weight = content_item.weight
         item_weight = default_weight if item_weight <= 0
         item_weight *= multiplier
@@ -61,10 +61,10 @@ def convert_package_to_dimensions_array(_package)
       []
     end
 
-    def convert_package_to_item_packages_array(solidus_package)
+    def convert_package_to_item_packages_array(product_with_no_product_packages)
       packages = []
 
-      solidus_package.contents.each do |content_item|
+      product_with_no_product_packages.each do |content_item|
         variant  = content_item.variant
         quantity = content_item.quantity
         product  = variant.product
@@ -84,14 +84,27 @@ def convert_package_to_item_packages_array(solidus_package)
     end
 
     # Generates an array of Package objects based on the quantities and weights of the variants in the line items
-    def packages(solidus_package)
+    def to_packages(solidus_package)
       active_shipping_packages = []
-      weights = convert_package_to_weights_array(solidus_package)
+
+      product_with_product_packages = []
+      product_with_no_product_packages = []
+      # Product with no associated product packages will be combined in packages based on weight
+      # Product with associated product packages will be added individually based on their attributes
+      solidus_package.contents.each do |content_item|
+        if content_item.variant_has_product_packages?
+          product_with_product_packages << content_item
+        else
+          product_with_no_product_packages << content_item
+        end
+      end
+
+      weights = convert_package_to_weights_array(product_with_no_product_packages)
       dimensions = convert_package_to_dimensions_array(solidus_package)
-      item_specific_packages = convert_package_to_item_packages_array(solidus_package)
+      item_specific_packages = convert_package_to_item_packages_array(product_with_product_packages)
 
       if max_weight <= 0
-        active_shipping_packages << ::ActiveShipping::Package.new(weights.sum, dimensions, units: units)
+        active_shipping_packages << ::ActiveShipping::Package.new(weights.sum, dimensions, units: units) unless weights.empty?
       else
         package_weight = 0
         weights.each do |content_weight|
@@ -108,16 +121,15 @@ def packages(solidus_package)
       item_specific_packages.each do |product_package|
         active_shipping_packages << ::ActiveShipping::Package.new(product_package.weight * multiplier, [product_package.length, product_package.width, product_package.height], units: units)
       end
-
+      
       active_shipping_packages
     end
 
     def get_max_weight(solidus_package)
       order = solidus_package.order
-      order_ship_address_country = order.ship_address.country
 
       # Default value from calculator
-      max_weight = max_weight_for_country(order_ship_address_country)
+      max_weight = max_weight_for_country(order.ship_address.country)
 
       # If max_weight is zero or max_weight_per_package is less than max_weight
       # We use the max_weight_per_package instead

app/models/spree/product_decorator.rb

@@ -3,4 +3,8 @@
   has_many :product_packages, :dependent => :destroy
 
   accepts_nested_attributes_for :product_packages, :allow_destroy => true, :reject_if => lambda { |pp| pp[:weight].blank? or Integer(pp[:weight]) < 1 }
+
+  def has_product_packages?
+  	product_packages.any?
+  end
 end

app/models/spree/variant_decorator.rb

@@ -0,0 +1,3 @@
+Spree::Variant.class_eval do
+	delegate :has_product_packages?, to: :product
+end

spec/models/package_builder_spec.rb

@@ -1,61 +1,219 @@
 require 'spec_helper'
 
 describe Spree::PackageBuilder do
+  include_context 'Canada stock location'
   include PackageHelper
-
-  let(:address) { FactoryGirl.create(:address) }
-  let(:order) { build(:order, ship_address: address) }
 
-  let(:shipping_calculator) { Spree::Calculator::Shipping::ActiveShipping::BogusCalculator.new }
-  let(:package_builder) { Spree::PackageBuilder.new(shipping_calculator) }
-
-  let(:product_package) { mock_model(Spree::ProductPackage, length: 12, width: 24, height: 47, weight: 36) }
-
-  let(:product_with_packages) do
-    build(
-      :variant,
-      weight: 29.0,
-      product: build(
-        :product,
-        product_packages: [product_package]
-      )
-    )
+  let(:shipping_calculator) do
+    Spree::Calculator::Shipping::ActiveShipping::BogusCalculator.new
   end
 
-  let(:package) do
-    double(Spree::Stock::Package,
-           order: order,
-           contents: [build_content_items(product_with_packages, 10, order),
-                      build_content_items(product_with_packages, 4, order)].flatten)
+  let(:package_builder) { Spree::PackageBuilder.new(shipping_calculator) }
+
+  before do
+    # We disable the multiplier so it's simpler to test
+    allow(package_builder).to receive(:multiplier).and_return(1)
   end
 
-  it "has a a unit_multiplier value" do
+  it 'has a a unit_multiplier value' do
     expect(package_builder.multiplier).not_to be_nil
   end
 
-  it "has a a units value" do
+  it 'has a a units value' do
     expect(package_builder.units).not_to be_nil
   end
 
-  it "has a a default_weight value" do
+  it 'has a a default_weight value' do
     expect(package_builder.default_weight).not_to be_nil
   end
 
-  it "has a a max_weight_per_package value" do
+  it 'has a a max_weight_per_package value' do
     expect(package_builder.max_weight_per_package).not_to be_nil
   end
-
+
+  # We make an exception and tests this the private method because max_weight values
+  # are difficult to tests conclusively through the
+  describe 'get_max_weight' do
+    include_context 'package setup'
+
+    context 'when the max_weight from the calculator is non-zero and max_weight_per_package is zero' do
+      before do
+        allow(shipping_calculator).to receive(:max_weight_for_country).and_return(1)
+        allow(package_builder).to receive(:max_weight_per_package).and_return(0)
+      end
+
+      it 'uses the max_weight_for_country as a max_weight' do
+        expect(package_builder.send(:get_max_weight, package)).to eq shipping_calculator.send(:max_weight_for_country)
+      end
+    end
+
+    context 'when the max_weight from the calculator is zero and max_weight_per_package is non-zero' do
+      before do
+        allow(shipping_calculator).to receive(:max_weight_for_country).and_return(0)
+        allow(package_builder).to receive(:max_weight_per_package).and_return(1)
+      end
+
+      it 'uses the max_weight_per_package as a max_weight' do
+        expect(package_builder.send(:get_max_weight, package)).to eq package_builder.max_weight_per_package
+      end
+    end
+
+    context 'when the max_weight from the calculator is non-zero and max_weight_per_package is non-zero' do
+      before do
+        allow(package_builder).to receive(:max_weight_per_package).and_return(SecureRandom.random_number(19) + 1)
+        allow(shipping_calculator).to receive(:max_weight_for_country).and_return(SecureRandom.random_number(19) + 1)
+      end
+
+      it 'uses the lesser one of the two values' do
+        min = [shipping_calculator.send(:max_weight_for_country), package_builder.max_weight_per_package].min
+        expect(package_builder.send(:get_max_weight, package)).to eq min
+      end
+    end
+
+    context 'when the max_weight is zero and max_weight_per_package is zero' do
+      it 'uses 0 as a max_eight' do
+        expect(package_builder.send(:get_max_weight, package)).to be_zero
+      end
+    end
+  end
+
   describe 'process' do
-    let(:legacy_package) { shipping_calculator.send :packages, package }
+    include_context 'package setup'
 
     subject { package_builder.process(package) }
 
-    it 'compute the the same weight as the ActiveShipping::Base.packages method' do
-      expect(subject.map { |package| package.weight.amount }).to eq legacy_package.map { |package| package.weight.amount }
+    it 'returns an array of ActiveShipping::Package' do
+      expect(subject.map(&:class).uniq).to match_array([ActiveShipping::Package])
+    end
+
+    it 'uses the unit multiplier in the calculations' do
+      allow(package_builder).to receive(:multiplier).and_return(2)
+      expect(subject.sum(&:weight)).to eq (package.weight * 2)
+    end
+
+    context 'with an order containing only products with associated product_packages' do
+      let(:product_weight) { 20 }
+      let(:product_package1) { FactoryGirl.create(:product_package, weight: product_weight) }
+      let(:product_package2) { FactoryGirl.create(:product_package, weight: product_weight) }
+      let(:product_with_packages) do
+        build(
+          :variant,
+          weight: product_weight,
+          product: build(
+            :product,
+            product_packages: [product_package1, product_package2]
+          )
+        )
+      end
+
+      let(:package) do
+        build(:stock_package,
+              stock_location: stock_location,
+              contents: [build_content_items(product_with_packages, 2, order),
+                         build_content_items(product_with_packages, 1, order)].flatten)
+      end
+
+      context 'when there is non-zero max_weight' do
+        context 'when the product_package weight exceeds the allowed max_weight' do
+          before do
+            allow(package_builder).to receive(:max_weight_per_package).and_return(0)
+            allow(shipping_calculator).to receive(:max_weight_for_country).and_return(1)
+          end
+
+          it 'raise a Spree::Shipping error' do
+            expect { subject }.to raise_error(Spree::ShippingError)
+          end
+        end
+
+        context 'when the product_packages weight does not exceed the allowed max_weight' do
+          before do
+            allow(shipping_calculator).to receive(:max_weight_for_country).and_return(product_weight + 1000)
+          end
+
+          it 'return a ActiveShipping::Package for each ProductPackage of each ContentItem' do
+            # Each individual item in the Order as a 1-1 association with a ContentItem
+            expected_size = package.contents.sum { |item| item.variant.product.product_packages.size }
+            expect(subject.size).to eq expected_size
+          end
+        end
+      end
+
+      context 'when there is no max weight' do
+        before do
+          allow(package_builder).to receive(:get_max_weight).and_return(0)
+        end
+
+        it 'return a ActiveShipping::Package for each ProductPackage of each ContentItem' do
+          # Each individual item in the Order as a 1-1 association with a ContentItem
+          expected_size = package.contents.sum { |item| item.variant.product.product_packages.size }
+          expect(subject.size).to eq expected_size
+        end
+      end
     end
 
-    it 'use the same weight unit as the ActiveShipping::Base.packages method' do
-      expect(subject.map { |package| package.weight.unit }).to eq legacy_package.map { |package| package.weight.unit }
+    context 'with an order containing only products without any associated product_packages' do
+      include_context 'package setup'
+
+      context 'when there is non-zero max_weight' do
+        it 'will combine items into the same package if their combined weight is lower than the max_weight' do
+          allow(package_builder).to receive(:get_max_weight).and_return(package.weight + 1)
+          expect(subject.size).to eq 1
+        end
+
+        it 'will split items into multiple packages if their combined weight is higher than the max_weight' do
+          allow(package_builder).to receive(:get_max_weight).and_return(package.weight - 1)
+          expect(subject.size).to eq 2
+        end
+
+        it 'will raise an error if a single item weight is higher than the max_weight' do
+          allow(package_builder).to receive(:get_max_weight).and_return(variant_2.weight - 1)
+          expect { subject }.to raise_error(Spree::ShippingError)
+        end
+      end
+
+      context 'when there is no max_weight' do
+        before do
+          allow(package_builder).to receive(:get_max_weight).and_return(0)
+        end
+
+        it 'will combine all items into the same package' do
+          expect(subject.size).to eq 1
+        end
+      end
+    end
+
+    context 'with an order containing some products with product_packages and some products without' do
+      let(:product_weight) { 20 }
+      let(:product_package1) { FactoryGirl.create(:product_package, weight: product_weight) }
+      let(:product_package2) { FactoryGirl.create(:product_package, weight: product_weight) }
+
+      let(:product_no_packages) { FactoryGirl.create(:variant, weight: 5) }
+      let(:product_with_packages) do
+        build(
+          :variant,
+          weight: product_weight,
+          product: build(
+            :product,
+            product_packages: [product_package1, product_package2]
+          )
+        )
+      end
+
+      let(:package) do
+        build(:stock_package,
+              stock_location: stock_location,
+              contents: [build_content_items(product_with_packages, 1, order),
+                         build_content_items(product_no_packages, 1, order),
+                         build_content_items(product_no_packages, 1, order)].flatten)
+      end
+
+      it 'products with product_packages will not be combined with product with no packages' do
+        allow(package_builder).to receive(:get_max_weight).and_return(1000)
+        active_shipping_packages = subject
+        # First package in the array is the "default package" who should
+        # only include product_with_no_packages x2
+        expect(active_shipping_packages[0].weight).to eq (product_no_packages.weight * 2)
+      end
     end
   end
 end