Refactor product view to use class-based views

[lorin]

This is a proposed refactoring of the product view code to use class-based views. There are no (intentional) changes to the behavior in this commit.

The motivation behind this proposed change is to make it easier for users to extend Cartridge through subclassing. For example, if a Cartridge user wanted to be able to use a subclass of the Product class in their template, all they would need to do is:

1. subclass the proposed ProductDetailView class
2. Override the get_object method in the subclass
3. Add a line in urls.py in their project to use the subclass

The main change is replacing the cartridge.shop.views.product function with the cartridge.shop.views.ProductDetailView class.

The only other required change was the cartridge.shop.forms.AddProductForm.__init__ method. This change was needed because the Django class-based views don't support passing a positional argument to the __init__ method of forms. However, the Django class-based view code automatically passes request.POST as an argument called data (see django.views.generic.edit.FormMixin.get_form_kwargs), so I made a change to use data as a keyword argument as a replacement for the positional argument.

[lorin]

Note that this pull request includes #81.

[stephenmcd]

Hey Lorin,

To be honest, I'm personally really against class based views and purposely try to avoid them wherever possible (as you might be able to tell from their total absence in Cartridge and Mezzanine). I know there's a big move in the Django community to use them for everything, but I think it's a bad one and they end up sacrificing the clarity of what a view does. Of course it would make things more customisable, but I think it comes at the cost of anyone (particularly newcomers) trying to understand the view code.

I really would like for things to be more customisable, so it makes this a really hard position for me to take on the CBVs, but also I think the branch around subclassing models and managing them the same way Mezzanine pages work will lend itself towards that a great deal.

Also, at the end of the day, it is actually possible to implement a custom view (class based as you've done, or otherwise) in your project and use that instead of Cartridge's. Yes some code will be repeated, but I think it's OK compared to my (very subjective) opinion of the cost in clarity using CBVs.

Sorry I know this isn't what you want to see, and I really don't want to discourage your effort, but it's something I feel pretty strongly about. I'll leave the pull request open for now for if there's more discussion to be had, but my feeling is not to merge it.

More than happy to include the new tests and the form changes as needed. I think there might be a bit of overlap with some of the existing tests still, but we can fix that up easily enough.

PS: There's a really good post from one of the core Django developers, Luke Plant (@spookylukey), that articulates the feeling around CBVs that I have, have a read of that too:

http://lukeplant.me.uk/blog/posts/djangos-cbvs-were-a-mistake/

[lorin]

Yeah, I agree that it's easier to read the old-style function-based views compared to class-based views. However, I think there's a tradeoff between readbility and extensibility.

For example, as you mentioned, we can implement a custom view in our project by re-implementing the view function with our customizations. In our particular use case, we want the product view to return a subclass of Product. To achieve this currently, the only way I can see to do it is to copy and paste the entire 50-line cartridge.shop.views.product function in order to do the equivalent of modifying a single line of code:

def product(request, slug, template="shop/product.html"):
    published_products = Product.objects.published(for_user=request.user)\
                                .select_subclasses()
    product = get_object_or_404(published_products, slug=slug)
    fields = [f.name for f in ProductVariation.option_fields()]
    variations = product.variations.all()
    variations_json = simplejson.dumps([dict([(f, getattr(v, f))
                                        for f in fields + ["sku", "image_id"]])
                                        for v in variations])
    to_cart = (request.method == "POST" and
               request.POST.get("add_wishlist") is None)
    initial_data = {}
    if variations:
        initial_data = dict([(f, getattr(variations[0], f)) for f in fields])
    initial_data["quantity"] = 1
    add_product_form = AddProductForm(request.POST or None, product=product,
                                      initial=initial_data, to_cart=to_cart)
    if request.method == "POST":
        if add_product_form.is_valid():
            if to_cart:
                quantity = add_product_form.cleaned_data["quantity"]
                request.cart.add_item(add_product_form.variation, quantity)
                recalculate_discount(request)
                info(request, _("Item added to cart"))
                return redirect("shop_cart")
            else:
                skus = request.wishlist
                sku = add_product_form.variation.sku
                if sku not in skus:
                    skus.append(sku)
                info(request, _("Item added to wishlist"))
                response = redirect("shop_wishlist")
                set_cookie(response, "wishlist", ",".join(skus))
                return response
    context = {
        "product": product,
        "editable_obj": product,
        "images": product.images.all(),
        "variations": variations,
        "variations_json": variations_json,
        "has_available_variations": any([v.has_price() for v in variations]),
        "related_products": product.related_products.published(
                                                      for_user=request.user),
        "add_product_form": add_product_form
    }

    templates = []
    # Check for a template matching the page's content model.
    if product.content_model is not None:
        templates.append(u"shop/products/%s.html" % product.content_model)
    templates.append(template)

    return render(request, templates, context)

On the other hand, if cartridge used a class-based view here, writing a custom product view that downcasts would look like this (assuming use of the inheritance manager)

class CustomProductDetailView(ProductDetailView):
    def get_queryset(self):
        return super(CustomProductDetailView, self).get_queryset().select_subclasses()

In addition, consider what happens when a cartridge user has written a custom product view and the product view implementation changes in a future version of cartridge. With a function-based view, the user has to identify that a change has occurred in the cartridge code and then update their custom product view code accordingly. It seems to me that you'd be much less likely that the user would have to change their custom class-based view.

[stephenmcd]

Thanks for the thorough reply - I can't argue with anything you've said :-)

I think we both agree on the pros and cons of either argument, and just value each side differently - you as a coder using the library who wants flexibility, and me as a coder maintaining the library (and teaching it to other users) who wants clarity in the code. There's also an undeniable argument to be made that without you as a user of the code, the code serves no purpose, so flexibility should trump clarity, so objectively speaking, we really should merge this in.

I'd still like to let it sit for a bit though, let me explain why - as you know, we have another discussion going on in Mezzanine about page subclassing and automatic downcasting. There's also discussion around custom product subclasses, and having that work the same way as Mezzanine pages do to some extent. So I think that if Cartridge moves forward in a way where custom product subclasses are better supported, there may be much less of a need to customise the product view, which would give heavier weight to my desire to leave it as a function based view. Specifically the product view itself may contain the support for working with a product subclass.

So let's leave this open for now - if we end up implementing better support for product subclasses, it'll make more of a case for leaving the view as is. If we don't (and I can't see any reason why we wouldn't, other than finding the time to make it happen), then there's a pretty clear case for changing to a CBV.

[stephenmcd]

In the mean time, I don't think it would hurt either way to reduce the size of the view a bit.

Perhaps the JSON stuff could go into the manager for the variations as an as_json method.

And all the stuff in the block if add_product_form.is_valid() could go into a method on the form that returns the redirect (response object) to use.

What do you think of that idea? I think it'd help with the function based view (making it easier to implement your own version) and also it'd help if this were a CBV (the clarity of what's happening across it would be increased with reduction of code in it).

[lorin]

Sounds like a plan. I do think that refactoring the current view methods by pulling out chunks into helper methods would go a long way towards reducing the effort required to write and maintain custom view code. I also think this would improve testability, since it's easier to write unit tests against the helper methods compared to writing them against a monolithic view method.

[auvipy]

@stephenmcd I do belive using cbv and cbgv is good where necessary. and we can use fbv or cbv both where necessary. even django core team is planning to upgrade its contrib apps using cbv where necessary :)

[auvipy]

and for lukeplants post he had many arguments which are not valid now.

Update: 2013-03-09

My opinions have changed slightly since I wrote the above, due to comments below and other helpful blog posts. I'd summarise by saying:

There are some places with CBVs really shine, especially when you are writing many similar simple views.
You can avoid some of the problems I mentioned by using your own class hierarchy, that you control completely, and making it flat, and specific to your needs.
I still prefer to write function based views. I've spent too many hours debugging class hierarchies of different kinds, and seen too many view functions that started out fitting into the kind of patterns that CBVs provide, and then breaking them in big ways.

[spookylukey]

For what it's worth, I just blogged about my current practice about CBV: http://lukeplant.me.uk/blog/posts/my-approach-to-class-based-views/