Don't use proxy models for the Condition types

Like with the benefit types stop implementing the condition business
logic as subclasses from the condition model.

src/oscar/apps/offer/abstract_models.py

@@ -16,7 +16,7 @@
 from django.utils.translation import ugettext_lazy as _
 from django.conf import settings
 
-from oscar.apps.offer import benefits, results, utils
+from oscar.apps.offer import benefits, conditions, results, utils
 from oscar.apps.offer.managers import ActiveOfferManager
 from oscar.core.compat import AUTH_USER_MODEL
 from oscar.core.loading import get_model, get_class
@@ -207,13 +207,13 @@ def is_available(self, user=None, test_date=None):
         return self.get_max_applications(user) > 0
 
     def is_condition_satisfied(self, basket):
-        return self.condition.proxy().is_satisfied(self, basket)
+        return self.condition.is_satisfied(self, basket)
 
     def is_condition_partially_satisfied(self, basket):
-        return self.condition.proxy().is_partially_satisfied(self, basket)
+        return self.condition.is_partially_satisfied(self, basket)
 
     def get_upsell_message(self, basket):
-        return self.condition.proxy().get_upsell_message(self, basket)
+        return self.condition.get_upsell_message(self, basket)
 
     def apply_benefit(self, basket):
         """
@@ -513,12 +513,10 @@ class AbstractCondition(models.Model):
     """
     COUNT, VALUE, COVERAGE = ("Count", "Value", "Coverage")
     TYPE_CHOICES = (
-        (COUNT, _("Depends on number of items in basket that are in "
-                  "condition range")),
-        (VALUE, _("Depends on value of items in basket that are in "
-                  "condition range")),
-        (COVERAGE, _("Needs to contain a set number of DISTINCT items "
-                     "from the condition range")))
+        (COUNT, conditions.CountCondition.help_text),
+        (VALUE, conditions.ValueCondition.help_text),
+        (COVERAGE, conditions.CoverageCondition.help_text)
+    )
     range = models.ForeignKey(
         'offer.Range', verbose_name=_("Range"), null=True, blank=True)
     type = models.CharField(_('Type'), max_length=128, choices=TYPE_CHOICES,
@@ -546,23 +544,28 @@ def proxy(self):
             self.VALUE: conditions.ValueCondition,
             self.COVERAGE: conditions.CoverageCondition
         }
-        # Short-circuit logic if current class is already a proxy class.
-        if self.__class__ in klassmap.values():
-            return self
-
-        field_dict = dict(self.__dict__)
-        for field in list(field_dict.keys()):
-            if field.startswith('_'):
-                del field_dict[field]
 
         if self.proxy_class:
             klass = utils.load_proxy(self.proxy_class)
-            # Short-circuit again.
-            if self.__class__ == klass:
-                return self
-            return klass(**field_dict)
+
+            if isinstance(klass, self.__class__):
+                warnings.warn(
+                    _("proxy classes should not extend benefit model"),
+                    DeprecationWarning, stacklevel=2)
+
+                field_dict = dict(self.__dict__)
+                for field in list(field_dict.keys()):
+                    if field.startswith('_'):
+                        del field_dict[field]
+
+                # Short-circuit again.
+                if self.__class__ == klass:
+                    return self
+                return klass(**field_dict)
+            return klass(self)
+
         if self.type in klassmap:
-            return klassmap[self.type](**field_dict)
+            return klassmap[self.type](self)
         raise RuntimeError("Unrecognised condition type (%s)" % self.type)
 
     def __str__(self):
@@ -584,57 +587,19 @@ def description(self):
         A description of the condition.
         Defaults to the name. May contain HTML.
         """
-        return self.name
+        return self.proxy().name
 
     def consume_items(self, offer, basket, affected_lines):
-        pass
+        return self.proxy().consume_items(offer, basket, affected_lines)
+
+    def get_upsell_message(self, offer, basket):
+        return self.proxy().get_upsell_message(offer, basket)
 
     def is_satisfied(self, offer, basket):
-        """
-        Determines whether a given basket meets this condition.  This is
-        stubbed in this top-class object.  The subclassing proxies are
-        responsible for implementing it correctly.
-        """
-        return False
+        return self.proxy().is_satisfied(offer, basket)
 
     def is_partially_satisfied(self, offer, basket):
-        """
-        Determine if the basket partially meets the condition.  This is useful
-        for up-selling messages to entice customers to buy something more in
-        order to qualify for an offer.
-        """
-        return False
-
-    def get_upsell_message(self, offer, basket):
-        return None
-
-    def can_apply_condition(self, line):
-        """
-        Determines whether the condition can be applied to a given basket line
-        """
-        if not line.stockrecord_id:
-            return False
-        product = line.product
-        return (self.range.contains_product(product)
-                and product.get_is_discountable())
-
-    def get_applicable_lines(self, offer, basket, most_expensive_first=True):
-        """
-        Return line data for the lines that can be consumed by this condition
-        """
-        line_tuples = []
-        for line in basket.all_lines():
-            if not self.can_apply_condition(line):
-                continue
-
-            price = utils.unit_price(offer, line)
-            if not price:
-                continue
-            line_tuples.append((price, line))
-        key = operator.itemgetter(0)
-        if most_expensive_first:
-            return sorted(line_tuples, reverse=True, key=key)
-        return sorted(line_tuples, key=key)
+        return self.proxy().is_partially_satisfied(offer, basket)
 
 
 @python_2_unicode_compatible

src/oscar/apps/offer/conditions.py

@@ -1,26 +1,88 @@
+import operator
 from decimal import Decimal as D, ROUND_UP
 
 from django.utils import six
 from django.utils.translation import ungettext, ugettext_lazy as _
 
 from oscar.apps.offer import utils
-from oscar.core.loading import get_model
 from oscar.templatetags.currency_filters import currency
 
-
-Condition = get_model('offer', 'Condition')
-
 __all__ = [
     'CountCondition', 'CoverageCondition', 'ValueCondition'
 ]
 
 
-class CountCondition(Condition):
+class ConditionImplementation(object):
+
+    def __init__(self, instance):
+        self.instance = instance
+
+    @property
+    def value(self):
+        return self.instance.value
+
+    @property
+    def range(self):
+        return self.instance.range
+
+    def is_satisfied(self, offer, basket):
+        """
+        Determines whether a given basket meets this condition.  This is
+        stubbed in this top-class object.  The subclassing proxies are
+        responsible for implementing it correctly.
+        """
+        return False
+
+    def is_partially_satisfied(self, offer, basket):
+        """
+        Determine if the basket partially meets the condition.  This is useful
+        for up-selling messages to entice customers to buy something more in
+        order to qualify for an offer.
+        """
+        return False
+
+    def get_upsell_message(self, offer, basket):
+        return None
+
+    def can_apply_condition(self, line):
+        """
+        Determines whether the condition can be applied to a given basket line
+        """
+        if not line.stockrecord_id:
+            return False
+        product = line.product
+        return (self.range.contains_product(product)
+                and product.get_is_discountable())
+
+    def get_applicable_lines(self, offer, basket, most_expensive_first=True):
+        """
+        Return line data for the lines that can be consumed by this condition
+        """
+        line_tuples = []
+        for line in basket.all_lines():
+            if not self.can_apply_condition(line):
+                continue
+
+            price = utils.unit_price(offer, line)
+            if not price:
+                continue
+            line_tuples.append((price, line))
+        key = operator.itemgetter(0)
+        if most_expensive_first:
+            return sorted(line_tuples, reverse=True, key=key)
+        return sorted(line_tuples, key=key)
+
+
+class CountCondition(ConditionImplementation):
     """
     An offer condition dependent on the NUMBER of matching items from the
     basket.
     """
     _description = _("Basket includes %(count)d item(s) from %(range)s")
+    help_text = _("Depends on number of items in basket that are in "
+                  "condition range")
+    verbose_name = _("Count condition")
+    verbose_name_plural = _("Count conditions")
 
     @property
     def name(self):
@@ -34,12 +96,6 @@ def description(self):
             'count': self.value,
             'range': utils.range_anchor(self.range)}
 
-    class Meta:
-        app_label = 'offer'
-        proxy = True
-        verbose_name = _("Count condition")
-        verbose_name_plural = _("Count conditions")
-
     def is_satisfied(self, offer, basket):
         """
         Determines whether a given basket meets this condition
@@ -103,13 +159,17 @@ def consume_items(self, offer, basket, affected_lines):
                 break
 
 
-class CoverageCondition(Condition):
+class CoverageCondition(ConditionImplementation):
     """
     An offer condition dependent on the number of DISTINCT matching items from
     the basket.
     """
     _description = _("Basket includes %(count)d distinct item(s) from"
                      " %(range)s")
+    help_text = _("Needs to contain a set number of DISTINCT items "
+                  "from the condition range")
+    verbose_name = _("Coverage Condition")
+    verbose_name_plural = _("Coverage Conditions")
 
     @property
     def name(self):
@@ -123,12 +183,6 @@ def description(self):
             'count': self.value,
             'range': utils.range_anchor(self.range)}
 
-    class Meta:
-        app_label = 'offer'
-        proxy = True
-        verbose_name = _("Coverage Condition")
-        verbose_name_plural = _("Coverage Conditions")
-
     def is_satisfied(self, offer, basket):
         """
         Determines whether a given basket meets this condition
@@ -208,12 +262,16 @@ def get_value_of_satisfying_items(self, offer, basket):
         return value
 
 
-class ValueCondition(Condition):
+class ValueCondition(ConditionImplementation):
     """
     An offer condition dependent on the VALUE of matching items from the
     basket.
     """
     _description = _("Basket includes %(amount)s from %(range)s")
+    help_text = _("Depends on value of items in basket that are in "
+                  "condition range")
+    verbose_name = _("Value condition")
+    verbose_name_plural = _("Value conditions")
 
     @property
     def name(self):
@@ -227,12 +285,6 @@ def description(self):
             'amount': currency(self.value),
             'range': utils.range_anchor(self.range)}
 
-    class Meta:
-        app_label = 'offer'
-        proxy = True
-        verbose_name = _("Value condition")
-        verbose_name_plural = _("Value conditions")
-
     def is_satisfied(self, offer, basket):
         """
         Determine whether a given basket meets this condition

tests/integration/offer/absolute_benefit_tests.py

@@ -16,7 +16,7 @@ def setUp(self):
         self.condition_product = factories.ProductFactory()
         condition_range = factories.RangeFactory()
         condition_range.add_product(self.condition_product)
-        self.condition = models.CountCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=condition_range,
             type=models.Condition.COUNT,
             value=2)
@@ -62,7 +62,7 @@ class TestAnAbsoluteDiscountAppliedWithCountCondition(TestCase):
     def setUp(self):
         range = models.Range.objects.create(
             name="All products", includes_all_products=True)
-        self.condition = models.CountCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=range,
             type=models.Condition.COUNT,
             value=2)
@@ -150,7 +150,7 @@ class TestAnAbsoluteDiscount(TestCase):
     def setUp(self):
         range = models.Range.objects.create(
             name="All products", includes_all_products=True)
-        self.condition = models.CountCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=range,
             type=models.Condition.COUNT,
             value=2)
@@ -180,7 +180,7 @@ class TestAnAbsoluteDiscountWithMaxItemsSetAppliedWithCountCondition(TestCase):
     def setUp(self):
         range = models.Range.objects.create(
             name="All products", includes_all_products=True)
-        self.condition = models.CountCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=range,
             type=models.Condition.COUNT,
             value=2)
@@ -225,7 +225,7 @@ class TestAnAbsoluteDiscountAppliedWithValueCondition(TestCase):
     def setUp(self):
         range = models.Range.objects.create(
             name="All products", includes_all_products=True)
-        self.condition = models.ValueCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=range,
             type=models.Condition.VALUE,
             value=D('10.00'))
@@ -276,7 +276,7 @@ class TestAnAbsoluteDiscountWithMaxItemsSetAppliedWithValueCondition(TestCase):
     def setUp(self):
         range = models.Range.objects.create(
             name="All products", includes_all_products=True)
-        self.condition = models.ValueCondition.objects.create(
+        self.condition = models.Condition.objects.create(
             range=range,
             type=models.Condition.VALUE,
             value=D('10.00'))

tests/integration/offer/combination_tests.py

@@ -12,7 +12,7 @@ class TestACountConditionWithPercentageDiscount(TestCase):
     def setUp(self):
         range = models.Range(
             name="All products", includes_all_products=True)
-        condition = models.CountCondition(
+        condition = models.Condition(
             range=range,
             type=models.Condition.COUNT,
             value=3)