Multiply charges when exceeding top bands upper limit

This approach imitates sending multiple parcels for a defined cost, and
hopefully approximates reality much better.

Note we still make some assumptions about the cost structure to avoid
having to solve an NP hard problem.

This commit also removes the unused max_upper_limit property and
replaces it by a more useful top_band property.

docs/source/releases/v0.8.rst

@@ -64,23 +64,12 @@ Customisation just got easier!
 Reworked shipping app
 ~~~~~~~~~~~~~~~~~~~~~
 
-Several parts of the shipping app have been altered The most important change is a
+Several parts of the shipping app have been altered. The most important change is a
 to the API of shipping methods to avoid a potential thread safety issue.
 Any existing Oscar sites with custom shipping methods will need to adjust them
-to confirm to the new API.
+to confirm to the new API. The new API and the other changes are detailed below.
 
-Other changes to the shipping app include:
-
-* All shipping models now have abstract base classes, similar to
-  the rest of Oscar, allowing them to be customised in the standard way.
-
-* The ``WeightBand.upper_limit`` model field is now a ``DecimalField``, just like the other
-  weight-related fields.
-
-* The Django admin interface for the ``WeightBased`` shipping method has been
-  made slightly more useful. 
-
-See the 
+See the
 :ref:`backwards incompatible changes <incompatible_shipping_changes_in_0.8>` 
 for the shipping app and the 
 :doc:`guide to configuring shipping </howto/how_to_configure_shipping>` 
@@ -154,10 +143,6 @@ Cleanup around shipping methods
 * ``WeightBand.upper_limit`` is now a ``DecimalField``, just like the other
   weight-related fields.
 
-* The Django admin interface for the ``WeightBased`` shipping method has been
-  made slightly more useful. Contributions for a dedicated dashboard app are
-  most welcome!
-
 .. _minor_changes_in_0.8:
 
 Minor changes
@@ -273,6 +258,11 @@ Note that shipping address should be passed around as instances not classes.
 
 Other potentially breaking changes related to shipping include:
 
+* Weight based shipping methods used to have an ``upper_charge`` field which was
+  returned if no weight band matched. That doesn't work very well in practice,
+  and has been removed. Instead, charges from bands are now added together to
+  match the weight of the basket.
+
 * The :class:`~oscar.apps.order.utils.OrderCreator` class no longer defaults to
   free shipping: a shipping method and charge have to be explicitly passed in.
 

oscar/apps/shipping/abstract_models.py

@@ -107,13 +107,7 @@ class AbstractWeightBased(AbstractBase):
         scale = Scale(attribute_code=self.weight_attribute,
                       default_weight=self.default_weight)
         weight = scale.weigh_basket(basket)
-        band = self.get_band_for_weight(weight)
-        if band is not None:
-            charge = band.charge
-        elif self.bands.all().exists() and self.upper_charge:
-            charge = self.upper_charge
-        else:
-            charge = D('0.00')
+        charge = self.get_charge(weight)
 
         # Zero tax is assumed...
         return prices.Price(
@@ -121,30 +115,57 @@ class AbstractWeightBased(AbstractBase):
             excl_tax=charge,
             incl_tax=charge)
 
+    def get_charge(self, weight):
+        """
+        Calculates shipping charges for a given weight.
+
+        If there is one or more matching weight band for a given weight, the
+        charge of the closest matching weight band is returned.
+
+        If the weight exceeds the top weight band, the top weight band charge
+        is added until a matching weight band is found. This models the concept
+        of "sending as many of the large boxes as needed".
+
+        Please note that it is assumed that the closest matching weight band
+        is the most cost-effective one, and that the top weight band is more
+        cost effective than e.g. sending out two smaller parcels.
+        Without that assumption, determining the cheapest shipping solution
+        becomes an instance of the bin packing problem. The bin packing problem
+        is NP-hard and solving it is left as an exercise to the reader.
+        """
+        weight = D(weight)  # weight really should be stored as a decimal
+        if not weight or not self.bands.exists():
+            return D('0.00')
+
+        top_band = self.top_band
+        if weight < top_band.upper_limit:
+            band = self.get_band_for_weight(weight)
+            return band.charge
+        else:
+            quotient, remaining_weight = divmod(weight, top_band.upper_limit)
+            remainder_band = self.get_band_for_weight(remaining_weight)
+            return quotient * top_band.charge + remainder_band.charge
+
     def get_band_for_weight(self, weight):
         """
-        Return the weight band for a given weight
+        Return the closest matching weight band for a given weight.
         """
-        bands = self.bands.filter(
-            upper_limit__gte=weight).order_by('upper_limit')[:1]
-        # Query return only one row, so we can evaluate it
-        if not bands:
-            # No band for this weight
+        try:
+            return self.bands.filter(
+                upper_limit__gte=weight).order_by('upper_limit')[0]
+        except IndexError:
             return None
-        return bands[0]
 
     @property
     def num_bands(self):
-        return self.bands.all().count()
+        return self.bands.count()
 
     @property
-    def max_upper_limit(self):
-        """
-        Return the max upper_limit from this method's weight bands
-        """
-        bands = self.bands.all().order_by('-upper_limit')
-        if bands:
-            return bands[0].upper_limit
+    def top_band(self):
+        try:
+            return self.bands.order_by('-upper_limit')[0]
+        except IndexError:
+            return None
 
 
 class AbstractWeightBand(models.Model):

tests/integration/shipping/model_method_tests.py

@@ -175,3 +175,15 @@ class WeightBasedMethodTests(TestCase):
         charge = self.standard.calculate(basket)
 
         self.assertEqual(expected_charge, charge.excl_tax)
+
+    def test_overflow_shipping_cost_scenario_handled_correctly(self):
+        basket = newfactories.BasketFactory()
+        product_attribute_value = newfactories.ProductAttributeValueFactory(
+            value_float=2.5)
+        basket.add_product(product_attribute_value.product)
+
+        self.standard.bands.create(upper_limit=1, charge=D('1.00'))
+        self.standard.bands.create(upper_limit=2, charge=D('2.00'))
+        charge = self.standard.calculate(basket)
+
+        self.assertEqual(D('1.00') + D('2.00'), charge.excl_tax)