Closed lazybird closed 10 years ago
More improvement mentioned on comments from Issue #10:
ImproperlyConfigured
when neither product_model
nor queryset
are set or product_model
can't be imported.product_queryset
.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:
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 cartmodel
nor queryset
are specified and if CART_PRODUCT_MODEL
is not defined, ImproperlyConfigured
should be raised.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.
queryset
as an optional argument to {% get_cart %}
template tag.queryset = None
class attribute from Cart
if subclassed need it, they can be define it.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.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.
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.
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?
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.
There are currently several ways to inform the cart object about the product model and the product queryset.
Product model can be passed in the cart init method
Product model dotted path can be set in settings
Product queryset can be set as an attribute on a cart subclass
Product queryset can be customized by redefining a method on a cart subclass
I'm wondering if this can be improved, simplified, be more consistent.
Issues:
__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.