Django-oscar: Products with options can be oversold

[ad-65]

When a product is configured with class or product options (not attributes) and tracks stock it possible to oversell the product. This behaviour is visible in the sandbox and in Oscar 1.5.

To reproduce:

1. Create a product option, for example, a message to print on a t-shirt.
2. Create a product type that uses this option in the dashboard.
3. Buy the product multiple times with different messages.
4. Edit the basket, changing the quantities to more than are available.
5. Stock validation will allow product to be oversold.

If I am not mistaken this is because the availability validation happens at the basket line level and does not take the overall basket state into account. The is_purchase_permitted() check passes for each line item but should not pass as a whole because there are only 10 t-shirts available for purchase but 13 in the basket.

The options force the same product to be a separate line item (as it should be) but this means that line item validation will not work because there can be multiple lines with the same product. I am not sure the best way to fix this as it requires basket level validation and I had a quick look at the code but did not see a hook for adding this. Any suggestions?

[solarissmoke]

We could refactor Basket.is_quantity_allowed to accept a line argument which can then be used to check the counts across multiple lines that share the same stock record.

It gets a bit tricky when you consider that it is possible to have different purchase strategies for different lines - I think we would have to make some assumptions about this and make it easy to override them.

[ad-65]

Thanks for the response.

Yeah, that could work. The basket would need to be checked in a number of places to prevent basket errors:

1. When adding a new product to the basket from the catalogue pages. This is already covered because form validation uses Basket.product_quantity() which sums up all line quantities with the same product.
2. When editing the basket line items. The user can be making changes to multiple lines with the same product, so we have to get the basket quantities, the form quantities, and the availability. For example, a product may have 10 items available and the user currently has 5 with "Message one" and 5 with "Message two". The basket could be edited to contain 4 with "Message one" and 6 with "Message two" and should validate properly.
3. At all checkout stages to make sure the product is still available in case any stock was consumed since the product was added to the basket.

Basket.is_quantity_allowed() isn't currently called anywhere except during the re-order view and during the individual basket form clean_quantity(). I think adding a line argument to Basket.is_quantity_allowed() and then making sure it is called during form/formset cleaning would work for items 1 & 2.

For item 3 it gets a bit tricky as you said. Would the validation have to rely on the availability policy, i.e., StockRequired.is_purchase_permitted() or is there a good place to call Basket.is_quantity_allowed()? Maybe inCheckoutSessionMixin.check_basket_is_valid()?

[ad-65]

Not sure if this is the proper way but I am putting my solution here in case anybody else runs into this problem. What I ended up doing was taking the suggestion of using Basket.is_quantity_allowed() to check the basket totals by passing a line argument:

class Basket(AbstractBasket):

    def is_quantity_allowed(self, qty, line=None):
        """Override to check basket level validation.

        Add some validation to make sure the same products on different lines
        are not oversold.
        """

        #Call the original and stop if there are any problems found
        is_allowed, reason = super(Basket, self).is_quantity_allowed(qty)
        if not is_allowed:
            return is_allowed, reason

        #Check the basket totals
        if line:
            is_permitted, reason = line.purchase_info.availability.is_purchase_permitted(self.basket_quantity(line) + qty)
            if not is_permitted:
                reason = _(
                    "{reason} (there may be others in your basket)"
                ).format(
                    reason=reason,
                )
                return False, reason

        return True, None

    def basket_quantity(self, line):
        """Return the quantity of similar lines in the basket.

        The basket can contain multiple lines with the same product and
        stockrecord, but different options. Those quantities are summed up.
        """
        matching_lines = self.lines.filter(stockrecord=line.stockrecord)
        quantity = matching_lines.aggregate(Sum('quantity'))['quantity__sum']
        return quantity or 0

This has to be called somewhere during the BasketLineForm cleaning with the line argument passed:

class BasketLineForm(CoreBasketLineForm):

    def check_max_allowed_quantity(self, qty):
        """Override to pass the line.
        """
        qty_delta = qty - self.instance.quantity
        is_allowed, reason = self.instance.basket.is_quantity_allowed(qty_delta, line=self.instance)
        if not is_allowed:
            raise forms.ValidationError(reason)

I also added a similar check to CheckoutSessionMixin.check_basket_is_valid():

def check_basket_is_valid(self, request):
        """Checks to maker sure the basket is still valid. Called at each
        checkout step. 

        Add some validation to make sure the same products on different lines
        are not oversold.
        """
        super(CheckoutSessionMixin, self).check_basket_is_valid(request)
        messages = []
        strategy = request.strategy

        for line in request.basket.all_lines():
            result = strategy.fetch_for_line(line)
            basket_quantity = request.basket.basket_quantity(line)
            is_permitted, reason = result.availability.is_purchase_permitted(basket_quantity)
            if not is_permitted:
                msg = _(
                    "'{title}' is no longer available to buy (there are {basket_quantity} in your basket - {reason}). "
                    "Please adjust your basket to continue"
                ).format(
                    title=line.product.get_title(),
                    basket_quantity=basket_quantity,
                    reason=reason,
                )
                messages.append(msg)

        if messages:
            raise exceptions.FailedPreCondition(url=reverse("basket:summary"), messages=messages)

This will make sure the basket is valid at checkout.

Once again, not sure if this is the best way but it should work until this gets addressed in subsequent versions.

[sasha0]

@ad-65 would you submit a proper PR and include unit tests? Thanks.

[pantlavanya]

Is this fixed ?

Metadata-Version: 2.1
Name: django-oscar
Version: 3.0