Improve the way product model and queryset are handled

[lazybird]

There are currently several ways to inform the cart object about the product model and the product queryset.

1. Product model can be passed in the cart init method

   cart(session, product_model=Product)
   

2. Product model dotted path can be set in settings

   CART_PRODUCT_MODEL = 'products.models.Product' 
   

3. Product queryset can be set as an attribute on a cart subclass

   Class MyCart(Cart):
       queryset = Product.objects.published()
   

4. Product queryset can be customized by redefining a method on a cart subclass

   Class MyCart(Cart):
       def get_queryset(self):
           return Product.objects.published()
   

I'm wondering if this can be improved, simplified, be more consistent.

Issues:

1. We have the ability to pass the product model via the cart __init__. That's not the case for the product queryset which seems inconsistent. We could have both passed in __init__ or have none of them there.
2. The product model can be defined without the need to subclass the cart. That's done by defining the dotted path in a setting. We don't have a similar approach for the queryset. Currently, you have to subclass the cart to get a chance to customize the queryset.

[lazybird]

More improvement mentioned on comments from Issue #10:

• Configuration validation: Raising ImproperlyConfigured when neither product_model nor queryset are set or product_model can't be imported.
• Naming: For consistency maybe the queryset references we should be named product_queryset.

[sect2k]

Hey,

sorry for the late reply, hard disk in my macbook failed last week and it took while to get back on track.

How about this:

1. We allow both model or queryset to be passed via __init__. This should cover most of the cases, where some advanced filtering is required, for more complex cases Cart can be subclassed and get_queryset() overridden. This should limit the need to subclass the cart
2. If neither model nor queryset are specified and if CART_PRODUCT_MODEL is not defined, ImproperlyConfiguredshould be raised.
3. To simplify filtering products, we could add a new setting CART_PRODUCT_LOOKUP that would be a dictionary with QuerySet lookup filter, something like

CART_PRODUCT_LOOKUP = {
    'is_active': True,
    'status': 'A',
    ... 
} 

This would the be applied in the default implementation of get_querysey() and should be enough for a lot of cases. Another benefit is that with this approach {% get_cart %} template tag works out of the box with filtered products.

1. Add querysetas an optional argument to {% get_cart %} template tag.
2. Remove queryset = None class attribute from Cartif subclassed need it, they can be define it.
3. Pass request to __init__ not just request.session, and set it to self.request. While not needed in base Cart implementation, it could be handy when subclassing (I have a use case for my project) and it would diminish the need to override __init__ in such cases. Since Cart is no longer stored in session as a whole, there is no downside to doing this.
4. This might be more of a personal preference, but I would name product_model just model and keep queryset, there are no other models or querysets in Cart and it would save some space in code as product_model and product_querysey are rather long, but as I said it's just a preference.

Let me know what you think.

Also I found a small bug in current release, I'll open a separate issue and push a fix.

[lazybird]

I have made some changes in the product-model branch. May be we can work on that branch and merge it when it looks good.

Thanks.

[sect2k]

Sure, I had a look at the branch, looks OK, my comments.

1. Agreed, that there is no need to pass model and queryset to __init__, subclasses can do that if needed.

2. CART_PRODUCT_LOOKUP should support passing in callables, making this possible:

CART_PRODUCT_LOOKUP = {
     'status: 'A',
     'date_valid_from__gte': datetime.datetime.now
}

3. Alternatively (instead of having CART_PRODUCT_LOOKUP), we could allow users to set CART_PRODUCT_MANAGER_METHOD (defaults to 'carton') which would then get called on Product._default_manager instead of all() if it exists. The Cart.get_queryset would then look something like this:

def get_queryset(self):
    product_model = self.get_product_model()
    if hasattr(product_model._default_manager, CART_PRODUCT_MANAGER_METHOD):
        queryset = getattr(product_model._default_manager, CART_PRODUCT_MANAGER_METHOD)()
    else:
        queryset = product_model._default_manager.all()
    return queryset

Not sure which approach is better, but the latter (3) has an advantage that reduces the need for subclassing to practically zero, which in turn solves the problem of having to write custom get_cart template tag when you subclass the Cart.

Your thoughts?

[lazybird]

Hey sect2k,
I'm coming back to this old discussion about improving the way we get the product model.

I have merge the change into master and tagged a new release.
I have also opened a new issue with your comment about adding support for callables in CART_PRODUCT_LOOKUP. Issue #15.

Concerning point 3 on the alternative to having CART_PRODUCT_LOOKUP I will leave that for now. If you want to open an issue about it we can look at it for sure.

Thanks again for you help on this and I hope we can continue testing and improving the app.