auto discounts not found, if discount is applied to categories/subcategories

Issue #1352 open
Former user created an issue

Bugfix in products/utils.py to find autodiscounts for products, where the autodiscount is applied to any of it's parent categories.

Added a new function needed to find "all" auto-discounted products, which is quite useful if someone wants to have a view with all auto-discounted-products listed.

{{{

!diff

diff --git a/satchmo/apps/product/utils.py b/satchmo/apps/product/utils.py
--- a/satchmo/apps/product/utils.py
+++ b/satchmo/apps/product/utils.py
@@ -1,299 +1,39 @@
from decimal import Decimal
from django.contrib.sites.models import Site
from django.db.models import Q
from livesettings import config_value
from l10n.utils import moneyfmt
from product.models import Option, ProductPriceLookup, OptionGroup, Discount, Product, split_option_unique_id
from satchmo_utils.numbers import round_decimal
+from product.models import Discount, Product
import datetime
import logging
import types
import string
+import operator

+# this is good for getting all discounted products in e.g. disocunts.html
+def get_all_products_for_autodiscounts():
+ today = datetime.date.today()
+ auto_discounts = Discount.objects.filter(automatic=True, active=True, startDatelte=today, endDategt=today)
+ prods_slug_set = set()
+ for disc in auto_discounts:
+ prods_slug_set = get_all_product_slugs_for_discount(disc)
+ return Product.objects.filter(slug__in=prods_slug_set)

def find_auto_discounts(product):
if not type(product) in (types.ListType, types.TupleType):
product = (product,)
today = datetime.date.today()
discs = Discount.objects.filter(automatic=True, active=True, startDatelte=today, endDategt=today)
- return discs.filter(Q(valid_products__in=product) | Q(allValid=True)).order_by('-percentage')
+ disc_list = []
+ slugs = set([p.slug for p in product])
+ for disc in discs:
+ disc_slugs = get_all_product_slugs_for_discount(disc)
+ if len(slugs.intersection(disc_slugs)):
+ disc_list.append(disc)
+ disc_list.sort(key=operator.attrgetter('percentage'))
+ disc_list.reverse()
+ return disc_list

+def get_all_product_slugs_for_discount(disc):
+ set1 = set([p.slug for p in disc.valid_products.all()])
+ set2 = disc._valid_products_in_categories()
+ return set1.union(set2)
+
def find_best_auto_discount(product):
discs = find_auto_discounts(product)
if len(discs) > 0:
return discs[0]
else:
return None

}}}

Comments (6)

  1. Hynek Cernoch

    It is not good to enumerate all discountable products in the store if it is searched only for discounts for one product or small number of products. The main purpose is to find the best discount for products in the cart.

    The related issue https://bitbucket.org/chris1610/satchmo/issue/1381/discount-system-brainstorm, paragraph find_best_auto_discount introduces a new problem which causes that function find_auto_discounts will be unimportant soon.

  2. Hynek Cernoch

    The original function "find_auto_discounts" has been implemented much better than the suggested one. It was only implemented long ago before relation m2m discount categories bdef44d9a355.

    The new replacement of function "find_auto_discounts" should return not only list of discounts but list of tuples (discount, product). I suggest a name "find_auto_discount_map".

    The title about "subcategories" is wrong. It is not implemented neither can be implemented effectively.

  3. Andrea de la Huerta

    @Hynek Schlawack, I'm might be misunderstanding you again. So these are my answers, and I hope I understood you right :-)

    The fix in the commit bdef44d9a355 looks good but, if it was merged on March 2010, then something's not right, as I surely used a satchmo version, which should include this changeset. In the changeset it says clearly "apply discounts to categories and child categories" so it's probably not working(?) I definitely had the problem, that products on child categories of a category with an auto-discount applied wouldn't get the discount. You wrote: The title about "subcategories" is wrong. It is not implemented neither can be implemented effectively. So what do you mean? The changeset isn't supposed to work on products of child-categories? If so, then it's really confusing, and I'm sure lot's of programmers/users would expect that Discounts applied to parent categories also apply to the products of their child categories.

    To your comments: It is not good to enumerate all discountable products in the store if it is searched only for discounts for one product or small number of products. The main purpose is to find the best discount for products in the cart. -> I agree. But the function find_auto_discounts doesn't get _all_ discountable products, but only the ones of the discounts applied to the product/list-of-products passed to the function AND the ones also found on the valid-categories for that discount. And then makes an intersection of this sets... but this is of course getting lot's of duplicated products before the intersection. But I couldn't figure out a better way of getting _all_ valid products for a discount.... And I needed that, as my client uses "10% off for all products of category xy"-discounts or better said "promotions", as proposed in the disocunts-brainstorm, and almost all categories (actually all categories on his shop) have child categories, LOTS of child categories. So without this fix, the discounts applied through categories to a product would never be found, whether on the cart nor on the category listings, making the whole discounts-applied-to-categories-issue quite useless for us.

    The related issue https://bitbucket.org/chris1610/satchmo/issue/1381/discount-system-brainstorm, paragraph find_best_auto_discount introduces a new problem which causes that function find_auto_discounts will be unimportant soon. -> Yes, I see the discounts issue is going to be a very large one, with tons of aspects to discuss and to figure out. And I'm looking forward to contribute with both, ideas and hopefully usable code :-)

    The new replacement of function "find_auto_discounts" should return not only list of discounts but list of tuples (discount, product). I suggest a name "find_auto_discount_map". -> I agree too. Before changing this, I guess we should wait a little bit to see, which course the discussion of the brainstorming will take.

  4. Hynek Cernoch
    • changed status to open

    @Andrea: Yes, all products in a discounted category should be automatically discounted and are not now, which is a bug. I disagree with the way of fix. Some errors were mistakes from the beginning by low-quality code. This error was caused by a later extension which has not been implemented enough. The code was optimal and it is a loss to rewrite it completely.

    Please verify he following patch:

    diff -r 1a425a1160bb satchmo/apps/product/utils.py
    --- a/satchmo/apps/product/utils.py	Tue Jan 03 19:25:35 2012 -0600
    +++ b/satchmo/apps/product/utils.py	Tue Jan 03 16:45:49 2012 +0100
    @@ -28,7 +28,8 @@
             product = (product,)
         today = datetime.date.today()
         discs = Discount.objects.filter(automatic=True, active=True, startDate__lte=today, endDate__gt=today)
    -    return discs.filter(Q(valid_products__in=product) | Q(allValid=True)).order_by('-percentage')
    +    return discs.filter(Q(valid_products__in=product) | Q(allValid=True) |
    +            Q(valid_categories__product__in=product, valid_categories__is_active=True)).order_by('-percentage')
     
     def find_best_auto_discount(product):
         discs = find_auto_discounts(product)
    
    

    Subcategories - no. If a discounted category has subcategories it can not be applied effectively automatically. Therefore if you need it, you must propagate the discount from the parent category to subcategories by some batch script or manually, but not automatically calculate everything recursive at runtime. Same categoy with high discount can have a subcategory with a low discount intentionally etc.

    I think that your solution require to get all products from the database and then they are filtered by python even if you need only info obout one product. Exactly spoken you cycle over all discounts and for every discount you cycle over all valid products, which is even less effective than database fullscan over all products with discount. In a wholesale store: every product has some discount for some group of big clients. Number of discounted products can be many tens thousand and you can get also memory problems etc.

    The main challenge of https://bitbucket.org/chris1610/satchmo/issue/1381/discount-system-brainstorm is to think about a signal which permits a rich customization of discounts.

    The new replacement of function "find_auto_discounts" should return not only list of discounts but list of tuples (discount, product). I suggest a name "find_auto_discount_map".
    -> This function should more likely return tuples (discount value, discount type, product). Discount type I mean product discount, category discount or allValid. Some stores can want precedence of higher discount, other want precedence of product discount over category discount even if it is lover. The main data acquisition should be done by a good implemented queryset with sufficient selectivity. Then it should be customizable by signals. Constant number of queries is better then looping many queries over all discounts.

  5. Hynek Cernoch

    The previous fix is a proof of concept. Does it what is expected? Please feedback.

    If so, I rewrite it for big databases to work better. It looks that the database optitimizer can produce very poor query plan now.

  6. Andrea de la Huerta

    @Hynek Schlawack, thanks for your patch. I'll try to test it soon, but I need to make a test repository-yet. And I don't know if it would fix our problem, as in this shop, there are only products inside subcategories (always 2 levels)... But I'll try it and give you some feedback. It surely looks very clean and slim.

    My patch has been for some months under production already without any issues. Our database has also more than 3000 products, so at least so far it's having a good performance. And the function is being used intensively, as it is used to find auto-discounts applied to products on every product-listing, which is quite often. I'm looking forward to see, if there is a performance boost with your new patch.

    Here some comments and thoughts on your other answer above:

    Subcategories - no. If a discounted category has subcategories it can not be applied effectively automatically. Therefore if you need it, you must propagate the discount from the parent category to subcategories by some batch script or manually, but not automatically calculate everything recursive at runtime. Same categoy with high discount can have a subcategory with a low discount intentionally etc.

    -> ok, this recursively "activating" of the discount would have to be done with a signal, after applying the discount on the admin, and then if deleting or deactivating it, it would have to be deactivated also recursively. Right? So, some thoughts to it:

    - There could be a checkbox (default yes) to propagate a given auto-discount to child-categories.

    - There could also be a checkbox to deactivate all auto-discounts for a given category + AND also subcategories.

    - I think the normal behaviour of an "auto-disocount" as in a "promotion" should imply inheritance as in any OOP, so e.g. lets say I have the category shirts, with t-shirts and long-sleeve-shirts on it. I set an auto-discount 20% on all shirts during all summer, and 40% on t-shirts but only on June... I would actually expect, that the subcategory discount would override it's parent discount, and of course, a product would override it's category discount. So when the 40% discount runs out, it would automatically inherit it's parent's 20% discount... With a non-runtime propagation method, it feels a little bit more "obscure" for me, imagining some "not-correctly-removed" discounts or "priority-problems" like, which discount had which priority, and so on. So this definitely needs more thinking ...

    - With an inheritance/override behaviour the only "issue" could be, that if I set t-shirts with 10% discount, and the parent has 20%, then the "find the best discount" would not get the 20% but the 10%... but this might be for many people a desired behaviour. And a good way to say, All shirts (except long-sleeve-shirts) have 30% off during September. I think discount-inheritance "feels" like a natural behaviour of "real"-life discounts. At least here, in germany and in spain or mexico that would be the normal behaviour. And having thousands of products, "discounts" on category level with inheritance it is the most effective way to do this for the store-owners.

    But I find this quite more complicated to implement if we apply everything recursively instead of making it more like an inheritance process. I mean runtime-lookups of the parent-categories. Of course, the performance would increase, not having to query the parent categories. --- ohh, and of course, a product can belong to more than one category... I had forgotten that, that makes it more complicated... mhhh. There are lot's of aspects to think of... I guess I'll have to order my thoughts and my next answer should go directly into the brainstorm-issue.

    I think that your solution require to get all products from the database and then they are filtered by python even if you need only info obout one product. Exactly spoken you cycle over all discounts and for every discount you cycle over all valid products, which is even less effective than database fullscan over all products with discount. In a wholesale store: every product has some discount for some group of big clients. Number of discounted products can be many tens thousand and you can get also memory problems etc.

    -> Yes, we have no wholesale store, so we don't have that problem and are not having any performance issues so far. But of course THE solution should be a more generic, global approach for everyone, without any performance issues.

    Maybe you can close this issue (after I get to test your patch) and just make sure, that people "know" that applying a discount to a category WON'T include subcategories. If someone, like in our case, needs another behaviour, and can't wait until THE new discounts are finished, then this can be easily reached in the meantime with local functions, e.g. under localsite/product/utils.py (which is actually the real location of my patched version, as I left the core untouched). After that, just import the function as needed on the files which use the function. By us it would mean: templatetags (satchmo_discounts.py <- which we had already overriden locally) and localsite/payment/listeners.py (in _find_sale).

  7. Log in to comment