Repository: django-oscar/django-oscar

Issue:
Offer Tracking for Stacked Offers
I was testing out the stacked offer tracking that will be included in v1.6 and came across what may be a problem. It looks like the `LineOfferConsumer` class might be tracking the applied offers incorrectly.

As an example, in the basket app when  `AbstractLine.consume()` runs it should mark a line as consumed using `LineOfferConsumer.consume()` and passing the applicable offer. I think it should be storing the offer that consumed the items regardless of if the offer was exclusive or not. However, if the offer is exclusive it doesn't store the offer that consumed the line items.

This is for a test basket with an offer that should be applied to all three items in the line:

```python
class Line(AbstractLine):

    def consume(self, quantity, offer=None):
        super(Line, self).consume(quantity, offer)
        print "offer------------------------->", offer
        print "exclusive--------------------->", offer.exclusive
        print "has_discount------------------>", self.has_discount
        print "quantity_with_discount-------->", self.quantity_with_discount
        print "has_offer_discount------------>", self.has_offer_discount(offer)
        print "quantity_with_offer_discount-->", self.quantity_with_offer_discount(offer)
```

Output:

```
offer-------------------------> Discount 123456
exclusive---------------------> True
has_discount------------------> True
quantity_with_discount--------> 3
has_offer_discount------------> False
quantity_with_offer_discount--> 0
```

If the offer is set to non-exclusive then the behavior is as expected:

```
offer-------------------------> Discount 123456
exclusive---------------------> False
has_discount------------------> True
quantity_with_discount--------> 3
has_offer_discount------------> True
quantity_with_offer_discount--> 3
```

I don't actually need stacked offers per se, I just need to know what offer was applied to the line as a whole. Maybe this is stored somewhere else when a whole line is consumed by the same offer?


Discussion:
I'll take a look at this asap.

The LineOfferConsumer interface could do with more extensive tests.

Fix patch:
diff --git a/src/oscar/apps/basket/abstract_models.py b/src/oscar/apps/basket/abstract_models.py
--- a/src/oscar/apps/basket/abstract_models.py
+++ b/src/oscar/apps/basket/abstract_models.py
@@ -714,7 +714,7 @@ def consume(self, quantity, offer=None):
 
         Consumed items are no longer available to be used in offers.
         """
-        self.consumer.consume(quantity, offer=offer)
+        return self.consumer.consume(quantity, offer=offer)
 
     def get_price_breakdown(self):
         """
diff --git a/src/oscar/apps/basket/utils.py b/src/oscar/apps/basket/utils.py
--- a/src/oscar/apps/basket/utils.py
+++ b/src/oscar/apps/basket/utils.py
@@ -82,18 +82,19 @@ class LineOfferConsumer(object):
     """
 
     def __init__(self, line):
-        self.__line = line
-        self.__offers = dict()
-        self.__affected_quantity = 0
-        self.__consumptions = defaultdict(int)
+        self._line = line
+        self._offers = dict()
+        self._affected_quantity = 0
+        self._consumptions = defaultdict(int)
 
-    # private
-    def __cache(self, offer):
-        self.__offers[offer.pk] = offer
+    def _cache(self, offer):
+        self._offers[offer.pk] = offer
 
-    def __update_affected_quantity(self, quantity):
-        available = int(self.__line.quantity - self.__affected_quantity)
-        self.__affected_quantity += min(available, quantity)
+    def _update_affected_quantity(self, quantity):
+        available = int(self._line.quantity - self._affected_quantity)
+        num_consumed = min(available, quantity)
+        self._affected_quantity += num_consumed
+        return num_consumed
 
     # public
     def consume(self, quantity: int, offer=None):
@@ -103,16 +104,22 @@ def consume(self, quantity: int, offer=None):
         :param int quantity: the number of items on the line affected
         :param offer: the offer to mark the line
         :type offer: ConditionalOffer or None
+        :return: the number of items actually consumed
+        :rtype: int
 
         if offer is None, the specified quantity of items on this
         basket line is consumed for *any* offer, else only for the
         specified offer.
         """
-        self.__update_affected_quantity(quantity)
         if offer:
-            self.__cache(offer)
+            self._cache(offer)
             available = self.available(offer)
-            self.__consumptions[offer.pk] += min(available, quantity)
+
+        num_consumed = self._update_affected_quantity(quantity)
+        if offer:
+            num_consumed = min(available, quantity)
+            self._consumptions[offer.pk] += num_consumed
+        return num_consumed
 
     def consumed(self, offer=None):
         """
@@ -129,12 +136,12 @@ def consumed(self, offer=None):
 
         """
         if not offer:
-            return self.__affected_quantity
-        return int(self.__consumptions[offer.pk])
+            return self._affected_quantity
+        return int(self._consumptions[offer.pk])
 
     @property
     def consumers(self):
-        return [x for x in self.__offers.values() if self.consumed(x)]
+        return [x for x in self._offers.values() if self.consumed(x)]
 
     def available(self, offer=None) -> int:
         """
@@ -145,7 +152,7 @@ def available(self, offer=None) -> int:
         :return: the number of items available for offer
         :rtype: int
         """
-        max_affected_items = self.__line.quantity
+        max_affected_items = self._line.quantity
 
         if offer and isinstance(offer, ConditionalOffer):
 
@@ -160,6 +167,12 @@ def available(self, offer=None) -> int:
             if offer.exclusive and len(applied):
                 return 0
 
+            # check for applied offers allowing restricted combinations
+            for x in applied:
+                check = offer.combinations.count() or x.combinations.count()
+                if check and offer not in x.combined_offers:
+                    return 0
+
             # respect max_affected_items
             if offer.benefit.max_affected_items:
                 max_affected_items = min(offer.benefit.max_affected_items, max_affected_items)
diff --git a/src/oscar/apps/dashboard/offers/forms.py b/src/oscar/apps/dashboard/offers/forms.py
--- a/src/oscar/apps/dashboard/offers/forms.py
+++ b/src/oscar/apps/dashboard/offers/forms.py
@@ -36,7 +36,7 @@ class Meta:
         fields = ('start_datetime', 'end_datetime',
                   'max_basket_applications', 'max_user_applications',
                   'max_global_applications', 'max_discount',
-                  'priority', 'exclusive')
+                  'priority', 'exclusive', 'combinations')
 
     def clean(self):
         cleaned_data = super().clean()
@@ -45,8 +45,35 @@ def clean(self):
         if start and end and end < start:
             raise forms.ValidationError(_(
                 "The end date must be after the start date"))
+        exclusive = cleaned_data['exclusive']
+        combinations = cleaned_data['combinations']
+        if exclusive and combinations:
+            raise forms.ValidationError(_('Exclusive offers cannot be combined'))
         return cleaned_data
 
+    def save(self, *args, **kwargs):
+        """Store the offer combinations.
+
+        Also, and make sure the combinations are stored on the combine-able
+        offers as well.
+        """
+        instance = super().save(*args, **kwargs)
+        if instance.id:
+            instance.combinations.clear()
+            for offer in self.cleaned_data['combinations']:
+                if offer != instance:
+                    instance.combinations.add(offer)
+
+            combined_offers = instance.combined_offers
+            for offer in combined_offers:
+                if offer == instance:
+                    continue
+                for otheroffer in combined_offers:
+                    if offer == otheroffer:
+                        continue
+                    offer.combinations.add(otheroffer)
+        return instance
+
 
 class ConditionForm(forms.ModelForm):
     custom_condition = forms.ChoiceField(
diff --git a/src/oscar/apps/dashboard/offers/views.py b/src/oscar/apps/dashboard/offers/views.py
--- a/src/oscar/apps/dashboard/offers/views.py
+++ b/src/oscar/apps/dashboard/offers/views.py
@@ -111,9 +111,15 @@ def _store_form_kwargs(self, form):
 
         # Adjust kwargs to avoid trying to save the range instance
         form_data = form.cleaned_data.copy()
-        range = form_data.get('range', None)
-        if range is not None:
-            form_data['range'] = range.id
+        product_range = form_data.get('range')
+        if product_range is not None:
+            form_data['range'] = product_range.id
+
+        combinations = form_data.get('combinations')
+        if combinations is not None:
+            form_data['combination_ids'] = [x.id for x in combinations]
+            del form_data['combinations']
+
         form_kwargs = {'data': form_data}
         json_data = json.dumps(form_kwargs, cls=DjangoJSONEncoder)
 
@@ -136,7 +142,17 @@ def _store_object(self, form):
         # We don't store the object instance as that is not JSON serialisable.
         # Instead, we save an alternative form
         instance = form.save(commit=False)
-        json_qs = serializers.serialize('json', [instance])
+        fields = form.fields.keys()
+        safe_fields = ['custom_benefit', 'custom_condition']
+        # remove fields that do not exist (yet) on the uncommitted instance, i.e. m2m fields
+        # unless they are 'virtual' fields as listed in 'safe_fields'
+        cleanfields = {x: hasattr(instance, x) for x in fields}
+        cleanfields.update({x: True for x in fields if x in safe_fields})
+        cleanfields = [
+            x[0] for x in cleanfields.items() if x[1]
+        ]
+
+        json_qs = serializers.serialize('json', [instance], fields=tuple(cleanfields))
 
         session_data[self._key(is_object=True)] = json_qs
         self.request.session.save()
@@ -152,10 +168,10 @@ def _fetch_object(self, step_name, request=None):
             return deserialised_obj[0].object
 
     def _fetch_session_offer(self):
-        """
-        Return the offer instance loaded with the data stored in the
-        session.  When updating an offer, the updated fields are used with the
-        existing offer data.
+        """Return the offer instance loaded with the data stored in the session.
+
+        When updating an offer, the updated fields are used with the existing
+        offer data.
         """
         offer = self._fetch_object('metadata')
         if offer is None and self.update:
diff --git a/src/oscar/apps/offer/abstract_models.py b/src/oscar/apps/offer/abstract_models.py
--- a/src/oscar/apps/offer/abstract_models.py
+++ b/src/oscar/apps/offer/abstract_models.py
@@ -119,6 +119,13 @@ class AbstractConditionalOffer(models.Model):
         help_text=_("Exclusive offers cannot be combined on the same items"),
         default=True
     )
+    combinations = models.ManyToManyField(
+        'offer.ConditionalOffer',
+        help_text=_('Select other non-exclusive offers that this offer can be combined with on the same items'),
+        related_name='in_combination',
+        limit_choices_to={'exclusive': False},
+        blank=True,
+    )
 
     # We track a status variable so it's easier to load offers that are
     # 'available' in some sense.
@@ -443,6 +450,14 @@ def products(self):
         return queryset.filter(is_discountable=True).exclude(
             structure=Product.CHILD)
 
+    @cached_property
+    def combined_offers(self):
+        return self.__class__.objects.filter(
+            models.Q(pk=self.pk)
+            | models.Q(pk__in=self.combinations.values_list("pk", flat=True))
+            | models.Q(pk__in=self.in_combination.values_list("pk", flat=True))
+        ).distinct()
+
 
 class AbstractBenefit(BaseOfferMixin, models.Model):
     range = models.ForeignKey(
diff --git a/src/oscar/apps/offer/conditions.py b/src/oscar/apps/offer/conditions.py
--- a/src/oscar/apps/offer/conditions.py
+++ b/src/oscar/apps/offer/conditions.py
@@ -92,13 +92,9 @@ def consume_items(self, offer, basket, affected_lines):
         if to_consume == 0:
             return
 
-        for __, line in self.get_applicable_lines(offer, basket,
-                                                  most_expensive_first=True):
-            quantity_to_consume = min(
-                line.quantity_without_offer_discount(offer), to_consume
-            )
-            line.consume(quantity_to_consume, offer=offer)
-            to_consume -= quantity_to_consume
+        for __, line in self.get_applicable_lines(offer, basket, most_expensive_first=True):
+            num_consumed = line.consume(to_consume, offer=offer)
+            to_consume -= num_consumed
             if to_consume == 0:
                 break
 
diff --git a/src/oscar/apps/offer/migrations/0010_conditionaloffer_combinations.py b/src/oscar/apps/offer/migrations/0010_conditionaloffer_combinations.py
new file mode 100644
--- /dev/null
+++ b/src/oscar/apps/offer/migrations/0010_conditionaloffer_combinations.py
@@ -0,0 +1,18 @@
+# Generated by Django 3.0.9 on 2020-08-28 09:03
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('offer', '0009_auto_20200801_0817'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='conditionaloffer',
+            name='combinations',
+            field=models.ManyToManyField(blank=True, help_text='Select other non-exclusive offers that this offer can be combined with on the same items', limit_choices_to={'exclusive': False}, related_name='in_combination', to='offer.ConditionalOffer'),
+        ),
+    ]


Test patch:
diff --git a/tests/integration/basket/test_utils.py b/tests/integration/basket/test_utils.py
--- a/tests/integration/basket/test_utils.py
+++ b/tests/integration/basket/test_utils.py
@@ -118,7 +118,7 @@ def test_consume(self, filled_basket):
         line.consume(99)
         assert line.quantity_with_discount == 10
 
-    def test_consumed_with_exclusive_offer(self, filled_basket):
+    def test_consumed_with_exclusive_offer_1(self, filled_basket):
         offer1 = ConditionalOfferFactory(name='offer1')
         offer2 = ConditionalOfferFactory(name='offer2')
         offer3 = ConditionalOfferFactory(name='offer3')
@@ -132,7 +132,9 @@ def test_consumed_with_exclusive_offer(self, filled_basket):
 
         line1, line2 = list(filled_basket.all_lines())
 
+        # exclusive offer consumes one item on line1
         line1.consumer.consume(1, offer1)
+
         # offer1 is exclusive so that blocks other offers
         assert line1.is_available_for_offer_discount(offer2) is False
 
@@ -153,9 +155,126 @@ def test_consumed_with_exclusive_offer(self, filled_basket):
         # but still room for offer3!
         assert line2.is_available_for_offer_discount(offer3) is True
 
+    def test_consumed_with_exclusive_offer_2(self, filled_basket):
+        offer1 = ConditionalOfferFactory(name='offer1')
+        offer2 = ConditionalOfferFactory(name='offer2')
+        offer3 = ConditionalOfferFactory(name='offer3')
+        offer1.exclusive = True
+        offer2.exclusive = False
+        offer3.exclusive = False
+
+        for line in filled_basket.all_lines():
+            assert line.consumer.consumed(offer1) == 0
+            assert line.consumer.consumed(offer2) == 0
+
+        line1, line2 = list(filled_basket.all_lines())
+
+        # exclusive offer consumes one item on line1
+        line1.consumer.consume(1, offer1)
+        remaining1 = line1.quantity - 1
+
+        assert line1.quantity_with_offer_discount(offer1) == 1
+        assert line1.quantity_with_offer_discount(offer2) == 0
+        assert line1.quantity_with_offer_discount(offer3) == 0
+
+        assert line1.quantity_without_offer_discount(offer1) == remaining1
+        assert line1.quantity_without_offer_discount(offer2) == 0
+        assert line1.quantity_without_offer_discount(offer3) == 0
+
+        # exclusive offer consumes all items on line1
+        line1.consumer.consume(remaining1, offer1)
+        assert line1.quantity_with_offer_discount(offer1) == line1.quantity
+        assert line1.quantity_with_offer_discount(offer2) == 0
+        assert line1.quantity_with_offer_discount(offer3) == 0
+
+        assert line1.quantity_without_offer_discount(offer1) == 0
+        assert line1.quantity_without_offer_discount(offer2) == 0
+        assert line1.quantity_without_offer_discount(offer3) == 0
+
+        # non-exclusive offer consumes one item on line2
+        line2.consumer.consume(1, offer2)
+        remaining2 = line2.quantity - 1
+
+        assert line2.quantity_with_offer_discount(offer1) == 0
+        assert line2.quantity_with_offer_discount(offer2) == 1
+        assert line2.quantity_with_offer_discount(offer3) == 0
+
+        assert line2.quantity_without_offer_discount(offer1) == 0
+        assert line2.quantity_without_offer_discount(offer2) == remaining2
+        assert line2.quantity_without_offer_discount(offer3) == line2.quantity
+
+        # non-exclusive offer consumes all items on line2
+        line2.consumer.consume(remaining2, offer2)
+
+        assert line2.quantity_with_offer_discount(offer1) == 0
+        assert line2.quantity_with_offer_discount(offer2) == line2.quantity
+        assert line2.quantity_with_offer_discount(offer3) == 0
+
+        assert line2.quantity_without_offer_discount(offer1) == 0
+        assert line2.quantity_without_offer_discount(offer2) == 0
+        assert line2.quantity_without_offer_discount(offer3) == line2.quantity
+
     def test_consumed_by_application(self, filled_basket, single_offer):
         basket = filled_basket
         Applicator().apply(basket)
         assert len(basket.offer_applications.offer_discounts) == 1
-
         assert [x.consumer.consumed() for x in basket.all_lines()] == [1, 0]
+
+    def test_consumed_with_combined_offer(self, filled_basket):
+        offer1 = ConditionalOfferFactory(name='offer1')
+        offer2 = ConditionalOfferFactory(name='offer2')
+        offer3 = ConditionalOfferFactory(name='offer3')
+        offer4 = ConditionalOfferFactory(name='offer4')
+        offer1.exclusive = True
+        offer2.exclusive = False
+        offer3.exclusive = False
+        offer4.exclusive = False
+        offer2.combinations.add(offer3)
+        assert offer3 in offer2.combined_offers
+        assert offer2 in offer3.combined_offers
+
+        for line in filled_basket.all_lines():
+            assert line.consumer.consumed(offer1) == 0
+            assert line.consumer.consumed(offer2) == 0
+            assert line.consumer.consumed(offer3) == 0
+
+        line1 = filled_basket.all_lines()[0]
+
+        # combinable offer consumes one item of line1
+        line1.consumer.consume(1, offer2)
+        remaining1 = line1.quantity - 1
+
+        assert line1.quantity_with_offer_discount(offer1) == 0
+        assert line1.quantity_with_offer_discount(offer2) == 1
+        assert line1.quantity_with_offer_discount(offer3) == 0
+        assert line1.quantity_with_offer_discount(offer4) == 0
+
+        assert line1.quantity_without_offer_discount(offer1) == 0
+        assert line1.quantity_without_offer_discount(offer2) == remaining1
+        assert line1.quantity_without_offer_discount(offer3) == line1.quantity
+        assert line1.quantity_without_offer_discount(offer4) == 0
+
+        # combinable offer consumes one item of line1
+        line1.consumer.consume(1, offer3)
+        assert line1.quantity_with_offer_discount(offer1) == 0
+        assert line1.quantity_with_offer_discount(offer2) == 1
+        assert line1.quantity_with_offer_discount(offer3) == 1
+        assert line1.quantity_with_offer_discount(offer4) == 0
+
+        assert line1.quantity_without_offer_discount(offer1) == 0
+        assert line1.quantity_without_offer_discount(offer2) == remaining1
+        assert line1.quantity_without_offer_discount(offer3) == remaining1
+        assert line1.quantity_without_offer_discount(offer4) == 0
+
+        # combinable offer consumes all items of line1
+        line1.consumer.consume(remaining1, offer2)
+
+        assert line1.quantity_with_offer_discount(offer1) == 0
+        assert line1.quantity_with_offer_discount(offer2) == line1.quantity
+        assert line1.quantity_with_offer_discount(offer3) == 1
+        assert line1.quantity_with_offer_discount(offer4) == 0
+
+        assert line1.quantity_without_offer_discount(offer1) == 0
+        assert line1.quantity_without_offer_discount(offer2) == 0
+        assert line1.quantity_without_offer_discount(offer3) == remaining1
+        assert line1.quantity_without_offer_discount(offer4) == 0