Fix and deprecate min_child_price_* properties

They are from a time when we had one stockrecord per product, and no
strategy classes. They're used for visual purposes only, and were
forgotten when the codebase was updated. They failed loudly, as reported
in #1592.

We really need to either drop them or add a helper method to the
strategy class, but to ensure backwards-compatibility, I've
re-implemented the naive approach of just sorting by price; blindly
ignoring the many shortcomings.

I also added a deprecation notice.

docs/source/releases/v1.0.1.rst

@@ -7,16 +7,20 @@ This is Oscar 1.0.1, a bug fix release.
 Bug fixes
 =========
 
+* `#1553`_: ``from oscar.apps.partner.models import *`` could lead to the
+  wrong models being imported.
+
 * `#1556`_: Dashboard order table headers shifted
 
 * `#1577`_: The billing address was not being correctly passed through to the
   `place_order` method.
 
-* `#1553`_: ``from oscar.apps.partner.models import *`` could lead to the
-  wrong models being imported.
-
+* `#1592`_: ``Product.min_child_price_[excl|incl]_tax`` were broken and
+  failing loudly. They are not recommended any more, but to ensure
+  backwards-compatibility, they have been fixed.
 
-  .. _#1556: https://github.com/django-oscar/django-oscar/issues/1556
   .. _#1553: https://github.com/django-oscar/django-oscar/issues/1553
+  .. _#1556: https://github.com/django-oscar/django-oscar/issues/1556
   .. _#1577: https://github.com/django-oscar/django-oscar/issues/1577
+  .. _#1592: https://github.com/django-oscar/django-oscar/issues/1592
 

oscar/apps/catalogue/abstract_models.py

@@ -23,12 +23,14 @@ from treebeard.mp_tree import MP_Node
 
 from oscar.core.decorators import deprecated
 from oscar.core.utils import slugify
-from oscar.core.loading import get_classes, get_model
+from oscar.core.loading import get_classes, get_model, get_class
 from oscar.models.fields import NullCharField, AutoSlugField
 
 ProductManager, BrowsableProductManager = get_classes(
     'catalogue.managers', ['ProductManager', 'BrowsableProductManager'])
 
+Selector = get_class('partner.strategy', 'Selector')
+
 
 @python_2_unicode_compatible
 class AbstractProductClass(models.Model):
@@ -480,32 +482,53 @@ class AbstractProduct(models.Model):
         pairs = [attribute.summary() for attribute in attributes]
         return ", ".join(pairs)
 
+    # The two properties below are deprecated because determining minimum
+    # price is not as trivial as it sounds considering multiple stockrecords,
+    # currencies, tax, etc.
+    # The current implementation is very naive and only works for a limited
+    # set of use cases.
+    # At the very least, we should pass in the request and
+    # user. Hence, it's best done as an extension to a Strategy class.
+    # Once that is accomplished, these properties should be removed.
+
     @property
+    @deprecated
     def min_child_price_incl_tax(self):
         """
-        Return minimum child product price including tax
+        Return minimum child product price including tax.
         """
-        return self._min_child_price('price_incl_tax')
+        return self._min_child_price('incl_tax')
 
     @property
+    @deprecated
     def min_child_price_excl_tax(self):
         """
-        Return minimum child product price excluding tax
+        Return minimum child product price excluding tax.
+
+        This is a very naive approach; see the deprecation notice above. And
+        only use it for display purposes (e.g. "new Oscar shirt, prices
+        starting from $9.50").
         """
-        return self._min_child_price('price_excl_tax')
+        return self._min_child_price('excl_tax')
 
-    def _min_child_price(self, property):
+    def _min_child_price(self, prop):
         """
-        Return minimum child product price
+        Return minimum child product price.
+
+        This is for visual purposes only. It ignores currencies, most of the
+        Strategy logic for selecting stockrecords, knows nothing about the
+        current user or request, etc. It's only here to ensure
+        backwards-compatibility; the previous implementation wasn't any
+        better.
         """
-        prices = []
-        for child in self.children.all():
-            if child.has_stockrecords:
-                prices.append(getattr(child.stockrecord, property))
-        if not prices:
-            return None
-        prices.sort()
-        return prices[0]
+        strategy = Selector().strategy()
+
+        children_stock = strategy.select_children_stockrecords(self)
+        prices = [
+            strategy.pricing_policy(child, stockrecord)
+            for child, stockrecord in children_stock]
+        raw_prices = sorted([getattr(price, prop) for price in prices])
+        return raw_prices[0] if raw_prices else None
 
     # The properties below are based on deprecated naming conventions
 

tests/integration/catalogue/product_tests.py

@@ -7,6 +7,7 @@ from oscar.apps.catalogue.models import (Product, ProductClass,
                                          ProductAttribute,
                                          AttributeOption)
 from oscar.test import factories
+from oscar.test.decorators import ignore_deprecation_warnings
 
 
 class ProductTests(TestCase):
@@ -82,8 +83,7 @@ class ChildProductTests(ProductTests):
 
     def test_child_products_dont_need_titles(self):
         Product.objects.create(
-            parent=self.parent, product_class=self.product_class,
-            structure=Product.CHILD)
+            parent=self.parent, structure=Product.CHILD)
 
     def test_child_products_dont_need_a_product_class(self):
         Product.objects.create(parent=self.parent, structure=Product.CHILD)
@@ -103,6 +103,18 @@ class ChildProductTests(ProductTests):
             structure=Product.CHILD)
         self.assertEqual(set([self.parent]), set(Product.browsable.all()))
 
+    @ignore_deprecation_warnings
+    def test_have_a_minimum_price(self):
+        self.assertIsNone(self.parent.min_child_price_excl_tax)
+        factories.ProductFactory(
+            parent=self.parent, structure=Product.CHILD,
+            stockrecords__price_excl_tax=5)
+        self.assertEqual(5, self.parent.min_child_price_excl_tax)
+        factories.ProductFactory(
+            parent=self.parent, structure=Product.CHILD,
+            stockrecords__price_excl_tax=3)
+        self.assertEqual(3, self.parent.min_child_price_excl_tax)
+
 
 class TestAChildProduct(TestCase):