Changed so only Cart state is saved to session, not the whole cart object.

[sect2k]

Store result of Cart.items_serializable in session instead of the whole cart object, then rebuild Cart._items_dict on initialization.

Need to set either CART_PRODUCT_MODEL in settings.py or pass model as part of cart initialization. The former is required for "get_cart" template tag to work out of the box.

Other than that, user facing API did not change, so existing code using the app should work without modification.

Still need to update tests and the example app, but thought I'd share what I've done so far, to get your feedback.

[lazybird]

Thanks a lot for contributing, this is very much appreciated. I hope we can release a new version including your changes and solve the Django 1.6 compatibility issues. Please feel free to contact me directly by email if you want to discuss more of the implementation details. Thanks again!

[sect2k]

Currently testing django-carton with my modifications on a Django 1.6 project I'm working on and there seem to be no more compatibility issues, all seems to we working fine now. If no issues come up during the next day or two, we can update the docs, tests and example and then you can make a new release.

I'll let you know how the testing goes.

[lazybird]

Thank you, Ok, I will make a release with these updates. I have also committed some changes on top of your contribution. It would be great for both of us to test them as well.

Thanks. On Feb 10, 2014 9:25 AM, "Mitja Pagon" notifications@github.com wrote:

Currently testing django-carton with my modifications on a Django 1.6 project I'm working on and there seem to be no more compatibility issues, all seems to we working fine now. If no issues come up during the next day or two, we can update the docs, tests and example and then you can make a new release.

I'll let you know how the testing goes.

Reply to this email directly or view it on GitHubhttps://github.com/lazybird/django-carton/pull/10#issuecomment-34608729 .

[sect2k]

Couple of comments:

1. import_by_path is only available in Django 1.6, it's not available in previous releases and is depreciated in Django 1.7+, we should avoid using it, the function that was in place before should work with Django 1.2+.
2. Splitting just one function (get_product_model) into a separate module/file (module_loading.py in this case) is overkill, especially as it will cause unnecessary overhead in form of I/O when loading the module, for a small project like this it would be optimal if there was just one file, personally I would kill the settings.py file as well and just put everything in cart.py (except for test and the example of course).
3. Like the way that dicts are now stored in session, makes lookups easier, but I would keep the get_queryset method, it allows the Cart object to be easily subclassed in case one needs more control over which Products are "valid". For example on the project I'm currently using django-carton, I need to show only products that are active (Product.is_active=True) and are of certain status (Product.status = X), without the ability to customize the queryset, I would need to override the init method.

Maybe we could do it similar to how class based ListView does it, by having a queryset property on the class and also a get_queryset function that can be subclassed if needed. Something like this.

class Cart(object):
    queryset = None

    def __init__(self, session, session_key=None, model=None):
        self._items_dict = {}
        self.session = session
        self.session_key = session_key or carton_settings.CART_SESSION_KEY
        self.model = model or get_product_model()
        if self.session_key in self.session:
            cart_representation = self.session[self.session_key]
            for product in self.get_queryset(cart_representation.keys()):
                item = cart_representation[product.pk]
                self._items_dict[product.pk] = CartItem(product, item['quantity'], Decimal(item['price']))

    def get_queryset(self, ids_in_cart ):
        if ids_in_cart:
            queryset = self.queryset or self.model.objects.all()
            return queryset.filter(pk__in=ids_in_cart)
        return self.model.objects.none()

Thoughts?

[lazybird]

Thanks for these valuable inputs.

1. About using import_by_path: yes, we should change that. I think can use import_string instead. Using import_string would break compatibility with older Django version. If breaking compatibility is an issue, we could look at releasing a separate version - support for Django-1.4 would be enough I think.
2. About splitting settings and module_loading into separate modules. In general, I like having separate modules even if there isn't much code in there. Lets say it's an organization preference. I completely understand your preference about not splitting into too many files.
3. About the ability to customize the product queryset: I agree that it's a useful feature. I'm think having get_queryset like you initially had it was a good idea. May be the filtering by IDs should be done on the init method instead of the get_queryset.

Thanks.

[sect2k]

1. import_string is currently only avaliable in dev version of Django (1.7), I think for the time being my original get_product_model function code is most convenient way to go, as it allows for Django 1.4+ compatibility, it's basically a verbatim copy of django.contrib.auth.get_user_model.
2. No problem, it doesn't really affect anything.
3. I agree, keep the get_queryset, but do filtering in the __init__ method, something like this:

def __init__(self, session, session_key=None, model=None):
    ...
    cart_representation = self.session[self.session_key]
    ids_in_cart = cart_representation.keys()
    if ids_in_cart:
        queryset = self.get_queryset().filter(pk__in=ids_in_cart)
        for product in queryset:
            item = cart_representation[product.pk]
    ...

def get_queryset(self):
    return self.queryset or self.model.objects.all()

[lazybird]

I have made some modifications and I have tagged a new version that's also on PyPi. It would be great to get your feedback - don't hesitate to contact me here or by email.

[sect2k]

There was a bug in carton_tags, it was still sending model instead of product_model to Cart init, pushed a fix. I also made some minor changes, to allow queryset to be set directly on the Cart subclass without overriding get_queryset. Also in case queryset is set, there is no need to set product_model.

One thing thought get_product_model does not work, it raises AttibuteError: 'module' object has no attribute 'Product'. I tried both full path to the model (myapp.products.Product) or app.Model (products.Product) as CART_PRODUCT_MODEL, neither work.

Could also do with some configuration validation, like raising ImproperlyConfigured in case when neither product_model nor queryset are set or product_model can't be imported.

Finally we could also pass queryset as an (optional) argument to Cart init, as an alternate way of setting it and since model is now product_model for consistency maybe we should call it product_queryset although it just makes for longer writing.

[lazybird]

I have open separate issues for the previous comments.

The setting for CART_PRODUCT_MODEL is the full dotted path to the module:

    CART_PRODUCT_MODEL = 'products.models.Product'

Issue #12: Improve the way product model and queryset are handled Issue #11: Missing documentation on CART_PRODUCT_MODEL setting

Let me know if I have missed anything.