Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Using a dynamically generated form for cart view in order to validate in... #114

Merged
merged 7 commits into from

2 participants

@mbrochh
Owner

Currently, if you enter bullshit into the quantity fields in the cart view, the app crashes.
I think we could use a dynamically generated form and make use of Django's nice form validation features...

I guess this would not be backwards compatible. How can we handle this? The change is quite simple, anyone could update their cart.html easily.

If you guys think this is good, I will add a patch with tests, updated changelog and maybe some docs if necessary.

shop/templates/shop/cart.html
@@ -16,13 +16,14 @@
</thead>
<tbody>
- {% for item in cart_items %}
+ {% for field in form %}
@mbrochh Owner
mbrochh added a note

this is the minimal change that would have to be done in the template... iterate over form instead cart_items and call field.field.cart_item instead of just item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
shop/views/cart.py
((6 lines not shown))
and QTY is quantity to set.
"""
- field_prefix = 'update_item-'
- cart_item_fields = [k for k in self.request.POST.keys()
- if k.startswith(field_prefix)]
- cart_object = get_or_create_cart(self.request)
- for key in cart_item_fields:
- id = key[len(field_prefix):]
- cart_object.update_quantity(id, int(self.request.POST[key]))
- return self.put_success()
+ context = self.get_context_data(**kwargs)
+ form = CartDetailsForm(context['cart'], data=self.request.POST)
+ if form.is_valid():
+ form.save()
+ return self.put_success()
+ context.update({'form': form, })
+ return self.render_to_response(context)
@mbrochh Owner
mbrochh added a note

I don't know... did I miss anything obvious? I think this is saxay.. no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh
Owner

|zeus| suggested to use a Formset instead. If no one has objections, I will try to cook a new patch tonight using formsets.

@chrisglass
Owner

Sure, formsets were an option, but I think I was lazy at the time... :p

Thanks for tackling on the problem!

@mbrochh mbrochh commented on the diff
shop/forms.py
((5 lines not shown))
+ payment_method = forms.ChoiceField(choices=get_billing_backends_choices())
+
+
+class CartItemModelForm(forms.ModelForm):
@mbrochh Owner
mbrochh added a note

As suggested by |zeus| I tried to use modelformsets now. Usually things should be extremely simple, as we just have to tell the modelformset_factory which Model we want to use. Unfortunately we can't do that here because we don't want to simply save the model, we want to call the update_quantity method of the Cart object instead.

Therefore we need this CartItemModelForm which overrides the save() method. Besides it's a cool way to tell the bloody form that negative values for quantity are not welcomed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
shop/forms.py
((13 lines not shown))
+ class Meta:
+ model = CartItem
+ fields = ('quantity', )
+
+ def save(self, *args, **kwargs):
+ """
+ We don't save the model using the regular way here because the
+ Cart's ``update_quantity()`` method already takes care of deleting
+ items from the cart when the quantity is set to 0.
+ """
+ quantity = self.cleaned_data['quantity']
+ self.instance.cart.update_quantity(self.instance.id, quantity)
+
+
+def get_cart_item_formset(cart_items=None, data=None):
@mbrochh Owner
mbrochh added a note

Usually in our view we would just do something like this:
CartItemFormSet = modelformset_factory(CartItem)
formset = CartItemFormSet(queryset=foobar)

However, I don't like the idea that where-ever we want to reuse that formset we have to remember to hand over the correct queryset (which is quite important because otherwise the user would see ALL cart items, from all carts).

So I created this convenience method that returns our desired form_set. Turns out that django's modelforms does lot's of wicked things and seem to pull the model out of the DB again, even though I just gave it a nice queryset. As a result, line_subtotal and line_total was always 0....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/forms.py
((29 lines not shown))
+ Returns a CartItemFormSet which can be used in the CartDetails view.
+
+ :param cart_items: The queryset to be used for this formset. This should
+ be the list of updated cart items of the current cart.
+ :param data: Optional POST data to be bound to this formset.
+ """
+ assert(cart_items is not None)
+ CartItemFormSet = modelformset_factory(CartItem, form=CartItemModelForm,
+ extra=0)
+ kwargs = {'queryset': cart_items, }
+ form_set = CartItemFormSet(data, **kwargs)
+
+ # The Django ModelFormSet pulls the item out of the database again and we
+ # would lose the updated line_subtotals
+ for form in form_set:
@mbrochh Owner
mbrochh added a note

This hack is to make sure that the updated CartItems are in form.instance. Unfortunately Django's ModelForms do weird stuff and pull the instances out of the database again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/templates/shop/cart.html
@@ -5,6 +5,10 @@
<form method="post" action="{% url cart_update %}">
{% csrf_token %}
+ {{ formset.management_form }}
@mbrochh Owner
mbrochh added a note

Using a formset makes things a bit more complicated in the template...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/templates/shop/cart.html
@@ -5,6 +5,10 @@
<form method="post" action="{% url cart_update %}">
{% csrf_token %}
+ {{ formset.management_form }}
+ {% for form in formset %}
@mbrochh Owner
mbrochh added a note

Since we are using an ugly table for our form further down, I cannot insert the form's id properly. I think inserting it up here next to the management_form stuff is nice enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
shop/templates/shop/cart.html
@@ -16,22 +20,25 @@
</thead>
<tbody>
- {% for item in cart_items %}
- <tr>
- <td>{{item.product.name}}</td>
- <td>{{item.product.get_price}}</td>
- <td><input type="text" name="update_item-{{ item.id }}"
- value="{{item.quantity}}"></td>
- <td>{{item.line_subtotal}}</td>
- </tr>
- {% for extra_price_field in item.extra_price_fields %}
+ {% for form in formset %}
+ {% for field in form.visible_fields %}
@mbrochh Owner
mbrochh added a note

this loop is a hack as well... we KNOW that the form only has one field. But with modelforms each form has an additional hidden ID field. If we looped over form.fields, we would get an unwanted row in our table. That's why I only call form.visible_fields here.

Eventually we shouldn't loop here at all but do something like

{% with form.fields.quantity as field %}
...
{% endwith %}

Yea I think that is better. I will submit another patch once you guys gave some feedback on this patch.

UPDATE: New patch submitted and got rid of this iteration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/views/cart.py
((6 lines not shown))
and QTY is quantity to set.
"""
- field_prefix = 'update_item-'
- cart_item_fields = [k for k in self.request.POST.keys()
- if k.startswith(field_prefix)]
- cart_object = get_or_create_cart(self.request)
- for key in cart_item_fields:
- id = key[len(field_prefix):]
- cart_object.update_quantity(id, int(self.request.POST[key]))
- return self.put_success()
+ context = self.get_context_data(**kwargs)
@mbrochh Owner
mbrochh added a note

The usage of all this stuff in our view is actually quite straightforward and elegant, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh
Owner

Okay submitted a new patch. Please review. If there are no objections in the next 48 hours I will create tests and merge this.

I still need some opinions about backwards-compatibility. People will need to update their cart.html template at least.

@chrisglass
Owner

Yeah, the backwards compatibility will break. But I guess it's worth it to have actual forms (that all django developers except me should know how to handle).

I'll keep reviewing during the day, but I guess it LGTM.

@mbrochh
Owner

Just submitted a new patch with a minor tweak in cart.html. Nothing serious.

@mbrochh mbrochh commented on the diff
shop/forms.py
((10 lines not shown))
+
+ quantity = forms.IntegerField(min_value=0)
+
+ class Meta:
+ model = CartItem
+ fields = ('quantity', )
+
+ def save(self, *args, **kwargs):
+ """
+ We don't save the model using the regular way here because the
+ Cart's ``update_quantity()`` method already takes care of deleting
+ items from the cart when the quantity is set to 0.
+ """
+ quantity = self.cleaned_data['quantity']
+ instance = self.instance.cart.update_quantity(self.instance.id,
@mbrochh Owner
mbrochh added a note

As you will see later, I changed update_quantity() so that it returns the updated (or deleted) instance, because Django encourages to always return the instance that has just been saved here.

Question: How do we handle the case when quantity is 0. update_quantity() will call cart_item.delete(). It might be a bit misleading that we still get the isntance instead of None.

On the other hand, if someone overrides our save() method maybe he can still make some good use of the instance that has been returned even though it has just been removed from the database. I vote for just leaving it as it is and just return the 'ghost' instance in this case.

@chrisglass Owner

Hum, we should return something explicit... Maybe None is actually he right thing to return?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/models/defaults/bases.py
@@ -153,6 +154,7 @@ class BaseCart(models.Model):
cart_item.quantity = quantity
cart_item.save()
self.save()
+ return cart_item
@mbrochh Owner
mbrochh added a note

I'm returning the just changed cart_item here so that we can return it in our CartItemModelForm.save() method.
As I said, we might decide to return None in case of quantity = 0 but I think this is OK as well.

@chrisglass Owner

Yeah this makes sense.

Still not sure, maybe we should return None?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/models/defaults/bases.py
((15 lines not shown))
@property
def amount_payed(self):
"""
The amount payed is the sum of related orderpayments
"""
from shop.models import OrderPayment
- sum = OrderPayment.objects.filter(order=self).aggregate(sum=Sum('amount'))
- result = sum.get('sum')
+ sum_ = OrderPayment.objects.filter(order=self).aggregate(
@mbrochh Owner
mbrochh added a note

My flake8 script warned me that we are redefining the builtin sum here, so I renamed this variable..

@chrisglass Owner

Ohhh good catch. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/tests/forms.py
@@ -0,0 +1,51 @@
+# -*- coding: utf-8 -*-
@mbrochh Owner
mbrochh added a note

Here are the new tests...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/tests/views.py
@@ -163,7 +163,13 @@ class CartViewTestCase(TestCase):
self.add_product_to_cart(self.product)
cart = self.get_cart()
- post = { 'update_item-%d' % cart.items.all()[0].pk : '5' }
+ cart_item_id = cart.items.all()[0].pk
@mbrochh Owner
mbrochh added a note

Modelformsets complicate things a bit for tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/models/defaults/bases.py
((6 lines not shown))
# That will hold things like tax totals or total discount
self.subtotal_price = Decimal('0.0')
self.total_price = Decimal('0.0')
self.current_total = Decimal('0.0') # used by cart modifiers
self.extra_price_fields = [] # List of tuples (label, value)
+ self._updated_cart_items = None
@mbrochh Owner
mbrochh added a note

My flake8 script warned me that we are defining self.update_cart_items outside of __init_ and indeed, defining it here is good for intellisense in IDEs and being transparent about our intentions with this class...

@chrisglass Owner

Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh mbrochh commented on the diff
shop/models/defaults/bases.py
@@ -169,8 +171,8 @@ class BaseCart(models.Model):
Returns updated cart items after update() has been called and
cart modifiers have been processed for all cart items.
"""
- assert hasattr(self, '_updated_cart_items'),\
- "Cart needs to be updated before calling get_updated_cart_items."
+ assert self._updated_cart_items is not None, ('Cart needs to be'
@mbrochh Owner
mbrochh added a note

Because I initiated self.updated_cart_items in the __init_ method, this assertions had to be slightly changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mbrochh
Owner

OK I'm done here. ./runtests doesn't fail, we have new tests and I fixed a lot of unrelated PEP8 errors. Sorry for that but I couldn't resist.

@chrisglass
Owner

It's nice. Sorry for being so long to fix stuff, but I still don't have a proper connection - I'm trying to do some of that stuff at work but can't really spend that much time on it. I should have a nice and quiet weekend though, so I'll be reviewing and fixing stuff :)

@mbrochh mbrochh merged commit ba2ac3e into divio:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  .gitignore
@@ -19,3 +19,4 @@ database.db
*.swp
*coverage.xml
.idea
+.ropeproject
View
8 CHANGELOG.rst
@@ -1,3 +1,11 @@
+Version 0.0.12
+==============
+
+* Using a dynamically generated form for the cart now to validate user input.
+ This will break your cart.html template. Please refer to the changes in
+ cart.html shipped by the shop to see how you can update your own template.
+ Basically you need to iterate over a formset now instead of cart_items.
+
Version 0.0.11
==============
View
51 shop/forms.py
@@ -1,6 +1,11 @@
#-*- coding: utf-8 -*-
+"""Forms for the django-shop app."""
from django import forms
+from django.forms.models import modelformset_factory
+
from shop.backends_pool import backends_pool
+from shop.models.cartmodel import CartItem
+
def get_shipping_backends_choices():
shipping_backends = backends_pool.get_shipping_backends_list()
@@ -16,4 +21,48 @@ class BillingShippingForm(forms.Form):
in settings.SHOP_SHIPPING_BACKENDS and settings.SHOP_PAYMENT_BACKENDS)
"""
shipping_method = forms.ChoiceField(choices=get_shipping_backends_choices())
- payment_method = forms.ChoiceField(choices=get_billing_backends_choices())
+ payment_method = forms.ChoiceField(choices=get_billing_backends_choices())
+
+
+class CartItemModelForm(forms.ModelForm):
@mbrochh Owner
mbrochh added a note

As suggested by |zeus| I tried to use modelformsets now. Usually things should be extremely simple, as we just have to tell the modelformset_factory which Model we want to use. Unfortunately we can't do that here because we don't want to simply save the model, we want to call the update_quantity method of the Cart object instead.

Therefore we need this CartItemModelForm which overrides the save() method. Besides it's a cool way to tell the bloody form that negative values for quantity are not welcomed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ """A form for the CartItem model. To be used in the CartDetails view."""
+
+ quantity = forms.IntegerField(min_value=0)
+
+ class Meta:
+ model = CartItem
+ fields = ('quantity', )
+
+ def save(self, *args, **kwargs):
+ """
+ We don't save the model using the regular way here because the
+ Cart's ``update_quantity()`` method already takes care of deleting
+ items from the cart when the quantity is set to 0.
+ """
+ quantity = self.cleaned_data['quantity']
+ instance = self.instance.cart.update_quantity(self.instance.id,
@mbrochh Owner
mbrochh added a note

As you will see later, I changed update_quantity() so that it returns the updated (or deleted) instance, because Django encourages to always return the instance that has just been saved here.

Question: How do we handle the case when quantity is 0. update_quantity() will call cart_item.delete(). It might be a bit misleading that we still get the isntance instead of None.

On the other hand, if someone overrides our save() method maybe he can still make some good use of the instance that has been returned even though it has just been removed from the database. I vote for just leaving it as it is and just return the 'ghost' instance in this case.

@chrisglass Owner

Hum, we should return something explicit... Maybe None is actually he right thing to return?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ quantity)
+ return instance
+
+
+def get_cart_item_formset(cart_items=None, data=None):
+ """
+ Returns a CartItemFormSet which can be used in the CartDetails view.
+
+ :param cart_items: The queryset to be used for this formset. This should
+ be the list of updated cart items of the current cart.
+ :param data: Optional POST data to be bound to this formset.
+ """
+ assert(cart_items is not None)
+ CartItemFormSet = modelformset_factory(CartItem, form=CartItemModelForm,
+ extra=0)
+ kwargs = {'queryset': cart_items, }
+ form_set = CartItemFormSet(data, **kwargs)
+
+ # The Django ModelFormSet pulls the item out of the database again and we
+ # would lose the updated line_subtotals
+ for form in form_set:
@mbrochh Owner
mbrochh added a note

This hack is to make sure that the updated CartItems are in form.instance. Unfortunately Django's ModelForms do weird stuff and pull the instances out of the database again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ for cart_item in cart_items:
+ if form.instance.id == cart_item.id:
+ form.instance = cart_item
+ return form_set
View
90 shop/models/defaults/bases.py
@@ -23,34 +23,34 @@ class BaseProduct(PolymorphicModel):
Most of the already existing fields here should be generic enough to reside
on the "base model" and not on an added property
"""
-
+
name = models.CharField(max_length=255, verbose_name=_('Name'))
slug = models.SlugField(verbose_name=_('Slug'), unique=True)
active = models.BooleanField(default=False, verbose_name=_('Active'))
-
+
date_added = models.DateTimeField(auto_now_add=True, verbose_name=_('Date added'))
last_modified = models.DateTimeField(auto_now=True, verbose_name=_('Last modified'))
-
+
unit_price = CurrencyField(verbose_name=_('Unit price'))
-
+
class Meta(object):
abstract = True
app_label = 'shop'
verbose_name = _('Product')
verbose_name_plural = _('Products')
-
+
def __unicode__(self):
return self.name
-
+
def get_absolute_url(self):
return reverse('product_detail', args=[self.slug])
-
+
def get_price(self):
"""
Return the price for this item (provided for extensibility)
"""
return self.unit_price
-
+
def get_name(self):
"""
Return the name of this Product (provided for extensibility)
@@ -81,12 +81,13 @@ class Meta(object):
verbose_name_plural = _('Carts')
def __init__(self, *args, **kwargs):
- super(BaseCart, self).__init__(*args,**kwargs)
+ super(BaseCart, self).__init__(*args, **kwargs)
# That will hold things like tax totals or total discount
self.subtotal_price = Decimal('0.0')
self.total_price = Decimal('0.0')
self.current_total = Decimal('0.0') # used by cart modifiers
self.extra_price_fields = [] # List of tuples (label, value)
+ self._updated_cart_items = None
@mbrochh Owner
mbrochh added a note

My flake8 script warned me that we are defining self.update_cart_items outside of __init_ and indeed, defining it here is good for intellisense in IDEs and being transparent about our intentions with this class...

@chrisglass Owner

Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def add_product(self, product, quantity=1, merge=True, queryset=None):
"""
@@ -153,6 +154,7 @@ def update_quantity(self, cart_item_id, quantity):
cart_item.quantity = quantity
cart_item.save()
self.save()
+ return cart_item
@mbrochh Owner
mbrochh added a note

I'm returning the just changed cart_item here so that we can return it in our CartItemModelForm.save() method.
As I said, we might decide to return None in case of quantity = 0 but I think this is OK as well.

@chrisglass Owner

Yeah this makes sense.

Still not sure, maybe we should return None?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def delete_item(self, cart_item_id):
"""
@@ -169,8 +171,8 @@ def get_updated_cart_items(self):
Returns updated cart items after update() has been called and
cart modifiers have been processed for all cart items.
"""
- assert hasattr(self, '_updated_cart_items'),\
- "Cart needs to be updated before calling get_updated_cart_items."
+ assert self._updated_cart_items is not None, ('Cart needs to be'
@mbrochh Owner
mbrochh added a note

Because I initiated self.updated_cart_items in the __init_ method, this assertions had to be slightly changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 'updated before calling get_updated_cart_items.')
return self._updated_cart_items
def update(self, state=None):
@@ -189,21 +191,21 @@ def update(self, state=None):
"purchase" button was pressed)
"""
from shop.models import CartItem, Product
-
+
# This is a ghetto "select_related" for polymorphic models.
items = CartItem.objects.filter(cart=self)
product_ids = [item.product_id for item in items]
products = Product.objects.filter(id__in=product_ids)
products_dict = dict([(p.id, p) for p in products])
-
+
self.extra_price_fields = [] # Reset the price fields
self.subtotal_price = Decimal('0.0') # Reset the subtotal
# This will hold extra information that cart modifiers might want to pass
# to each other
if state == None:
- state = {}
-
+ state = {}
+
# This calls all the pre_process_cart methods (if any), before the cart
# is processed. This allows for data collection on the cart for example)
for modifier in cart_modifiers_pool.get_modifiers_list():
@@ -212,15 +214,15 @@ def update(self, state=None):
for item in items: # For each CartItem (order line)...
item.product = products_dict[item.product_id] #This is still the ghetto select_related
self.subtotal_price = self.subtotal_price + item.update(state)
-
+
self.current_total = self.subtotal_price
# Now we have to iterate over the registered modifiers again (unfortunately)
# to pass them the whole Order this time
for modifier in cart_modifiers_pool.get_modifiers_list():
modifier.process_cart(self, state)
-
+
self.total_price = self.current_total
-
+
# This calls the post_process_cart method from cart modifiers, if any.
# It allows for a last bit of processing on the "finished" cart, before
# it is displayed
@@ -265,7 +267,7 @@ class Meta(object):
def __init__(self, *args, **kwargs):
# That will hold extra fields to display to the user
# (ex. taxes, discount)
- super(BaseCartItem, self).__init__(*args,**kwargs)
+ super(BaseCartItem, self).__init__(*args, **kwargs)
self.extra_price_fields = [] # list of tuples (label, value)
# These must not be stored, since their components can be changed between
# sessions / logins etc...
@@ -282,7 +284,7 @@ def update(self, state):
# We now loop over every registered price modifier,
# most of them will simply add a field to extra_payment_fields
modifier.process_cart_item(self, state)
-
+
self.line_total = self.current_total
return self.line_total
@@ -292,17 +294,17 @@ def update(self, state):
#==============================================================================
-
+
class BaseOrder(models.Model):
"""
A model representing an Order.
-
+
An order is the "in process" counterpart of the shopping cart, which holds
stuff like the shipping and billing addresses (copied from the User profile)
when the Order is first created), list of items, and holds stuff like the
status, shipping costs, taxes, etc...
"""
-
+
PROCESSING = 1 # New order, no shipping/payment backend chosen yet
PAYMENT = 2 # The user is filling in payment information
CONFIRMED = 3 # Chosen shipping/payment backend, processing payment
@@ -318,17 +320,17 @@ class BaseOrder(models.Model):
(SHIPPED, _('Shipped')),
(CANCELLED, _('Cancelled')),
)
-
+
# If the user is null, the order was created with a session
user = models.ForeignKey(User, blank=True, null=True,
verbose_name=_('User'))
-
+
status = models.IntegerField(choices=STATUS_CODES, default=PROCESSING,
verbose_name=_('Status'))
-
+
order_subtotal = CurrencyField(verbose_name=_('Order subtotal'))
order_total = CurrencyField(verbose_name='Order total')
-
+
shipping_address_text = models.TextField(_('Shipping address'), blank=True, null=True)
billing_address_text = models.TextField(_('Billing address'), blank=True, null=True)
@@ -337,7 +339,7 @@ class BaseOrder(models.Model):
verbose_name=_('Created'))
modified = models.DateTimeField(auto_now=True,
verbose_name=_('Updated'))
-
+
class Meta(object):
abstract = True
app_label = 'shop'
@@ -349,34 +351,36 @@ def __unicode__(self):
def get_absolute_url(self):
return reverse('order_detail', kwargs={'pk': self.pk })
-
+
def is_payed(self):
"""Has this order been integrally payed for?"""
return self.amount_payed == self.order_total
-
+
def is_completed(self):
return self.status == self.COMPLETED
-
+
@property
def amount_payed(self):
"""
The amount payed is the sum of related orderpayments
"""
from shop.models import OrderPayment
- sum = OrderPayment.objects.filter(order=self).aggregate(sum=Sum('amount'))
- result = sum.get('sum')
+ sum_ = OrderPayment.objects.filter(order=self).aggregate(
@mbrochh Owner
mbrochh added a note

My flake8 script warned me that we are redefining the builtin sum here, so I renamed this variable..

@chrisglass Owner

Ohhh good catch. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ sum=Sum('amount'))
+ result = sum_.get('sum')
if not result:
result = Decimal('-1')
return result
-
+
@property
def shipping_costs(self):
from shop.models import ExtraOrderPriceField
- sum = Decimal('0.0')
- cost_list = ExtraOrderPriceField.objects.filter(order=self).filter(is_shipping=True)
+ sum_ = Decimal('0.0')
+ cost_list = ExtraOrderPriceField.objects.filter(order=self).filter(
+ is_shipping=True)
for cost in cost_list:
- sum = sum + cost.value
- return sum
+ sum_ += cost.value
+ return sum_
def set_billing_address(self, billing_address):
"""
@@ -388,7 +392,7 @@ def set_billing_address(self, billing_address):
if hasattr(billing_address, 'as_text'):
self.billing_address_text = billing_address.as_text()
self.save()
-
+
def set_shipping_address(self, shipping_address):
"""
Process shipping_address trying to get as_text method from address
@@ -411,10 +415,10 @@ class BaseOrderItem(models.Model):
"""
A line Item for an order.
"""
-
+
order = models.ForeignKey(get_model_string('Order'), related_name='items',
verbose_name=_('Order'))
-
+
product_reference = models.CharField(max_length=255,
verbose_name=_('Product reference'))
product_name = models.CharField(max_length=255, null=True, blank=True,
@@ -422,10 +426,10 @@ class BaseOrderItem(models.Model):
product = models.ForeignKey(get_model_string('Product'), verbose_name=_('Product'), null=True, blank=True, **f_kwargs)
unit_price = CurrencyField(verbose_name=_('Unit price'))
quantity = models.IntegerField(verbose_name=_('Quantity'))
-
+
line_subtotal = CurrencyField(verbose_name=_('Line subtotal'))
line_total = CurrencyField(verbose_name=_('Line total'))
-
+
class Meta(object):
abstract = True
app_label = 'shop'
View
35 shop/templates/shop/cart.html
@@ -5,6 +5,10 @@
<form method="post" action="{% url cart_update %}">
{% csrf_token %}
+ {{ formset.management_form }}
@mbrochh Owner
mbrochh added a note

Using a formset makes things a bit more complicated in the template...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {% for form in formset %}
@mbrochh Owner
mbrochh added a note

Since we are using an ugly table for our form further down, I cannot insert the form's id properly. I think inserting it up here next to the management_form stuff is nice enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {{ form.id }}
+ {% endfor %}
<table border="1">
<thead>
<tr>
@@ -16,22 +20,25 @@
</thead>
<tbody>
- {% for item in cart_items %}
- <tr>
- <td>{{item.product.name}}</td>
- <td>{{item.product.get_price}}</td>
- <td><input type="text" name="update_item-{{ item.id }}"
- value="{{item.quantity}}"></td>
- <td>{{item.line_subtotal}}</td>
- </tr>
- {% for extra_price_field in item.extra_price_fields %}
+ {% for form in formset %}
+ {% with form.quantity as field %}
<tr>
- <td colspan="2">&nbsp;</td>
- <td>{{ extra_price_field.0 }}</td>
- <td>{{ extra_price_field.1 }}</td>
+ <td>{{ form.instance.product.name }}</td>
+ <td>{{ form.instance.product.get_price }}</td>
+ <td>
+ {{ field.errors }}
+ {{ field }}</td>
+ <td>{{ form.instance.line_subtotal }}</td>
</tr>
- {% endfor %}
- <tr><td colspan="2">&nbsp;</td><td>{% trans "Line Total" %}:</td><td>{{item.line_total}}</td></tr>
+ {% for extra_price_field in form.instance.extra_price_fields %}
+ <tr>
+ <td colspan="2">&nbsp;</td>
+ <td>{{ extra_price_field.0 }}</td>
+ <td>{{ extra_price_field.1 }}</td>
+ </tr>
+ {% endfor %}
+ <tr><td colspan="2">&nbsp;</td><td>{% trans "Line Total" %}:</td><td>{{ form.instance.line_total }}</td></tr>
+ {% endwith %}
{% endfor %}
</tbody>
View
26 shop/tests/__init__.py
@@ -1,34 +1,38 @@
from api import ShopApiTestCase
from cart import CartTestCase
-from cart_modifiers import (
+from cart_modifiers import (
CartModifiersTestCase,
TenPercentPerItemTaxModifierTestCase,
)
-from order import (
- OrderConversionTestCase,
+from order import (
+ OrderConversionTestCase,
OrderPaymentTestCase,
- OrderTestCase,
+ OrderTestCase,
OrderUtilTestCase,
)
+from forms import (
+ CartItemModelFormTestCase,
+ GetCartItemFormsetTestCase,
+)
from payment import PayOnDeliveryTestCase, GeneralPaymentBackendTestCase
from product import ProductTestCase, ProductStatisticsTestCase
from shipping import (
FlatRateShippingTestCase,
- GeneralShippingBackendTestCase,
- ShippingApiTestCase,
+ GeneralShippingBackendTestCase,
+ ShippingApiTestCase,
)
from templatetags import ProductsTestCase
from util import (
AddressUtilTestCase,
- CartUtilsTestCase,
- CurrencyFieldTestCase,
+ CartUtilsTestCase,
+ CurrencyFieldTestCase,
LoaderTestCase,
)
-from views import (
+from views import (
CartDetailsViewTestCase,
- CartViewTestCase,
+ CartViewTestCase,
OrderListViewTestCase,
- ProductDetailViewTestCase,
+ ProductDetailViewTestCase,
)
from views_checkout import (
CheckoutCartToOrderTestCase,
View
51 shop/tests/forms.py
@@ -0,0 +1,51 @@
+# -*- coding: utf-8 -*-
@mbrochh Owner
mbrochh added a note

Here are the new tests...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+"""Tests for the forms of the django-shop app."""
+from django.contrib.auth.models import User
+from django.test import TestCase
+
+from shop.forms import CartItemModelForm, get_cart_item_formset
+from shop.models.cartmodel import Cart, CartItem
+from shop.models.productmodel import Product
+
+
+class BaseCartItemFormsTestCase(TestCase):
+ """Base class for tests related to ``CartItem`` forms and formsets."""
+
+ def setUp(self):
+ self.user = User.objects.create(username="test",
+ email="test@example.com",
+ first_name="Test",
+ last_name = "Tester")
+ self.cart = Cart.objects.create()
+ self.product = Product.objects.create(unit_price=123)
+ self.item = CartItem.objects.create(cart=self.cart, quantity=2,
+ product=self.product)
+
+
+
+class CartItemModelFormTestCase(BaseCartItemFormsTestCase):
+ """Tests for the ``CartItemModelForm`` form class."""
+
+ def test_setting_quantity_to_0_removes_cart_item(self):
+ data = {
+ 'quantity': '0',
+ }
+ form = CartItemModelForm(instance=self.item, data=data)
+ self.assertEqual(len(form.errors), 0)
+ form.save()
+ self.assertEqual(0, CartItem.objects.all().count())
+
+
+class GetCartItemFormsetTestCase(BaseCartItemFormsTestCase):
+ """Tests for the ``get_cart_item_formset()`` method."""
+
+ def test_should_return_formset(self):
+ items = CartItem.objects.all()
+ formset = get_cart_item_formset(cart_items=items)
+ self.assertTrue('quantity' in formset.forms[0].fields)
+
+ def test_cart_items_should_have_updated_values(self):
+ self.cart.update()
+ items = self.cart.get_updated_cart_items()
+ formset = get_cart_item_formset(cart_items=items)
+ self.assertEqual(formset.forms[0].instance.line_subtotal, 246)
View
74 shop/tests/views.py
@@ -16,48 +16,48 @@
class ProductDetailViewTestCase(TestCase):
def setUp(self):
-
+
self.product = Product()
self.product.name = 'test'
self.product.short_description = 'test'
self.product.long_description = 'test'
self.product.unit_price = Decimal('1.0')
self.product.save()
-
+
self.view = ProductDetailView(kwargs={'pk':self.product.id})
-
+
def test_get_product_returns_correctly(self):
setattr(self.view, 'object', None)
obj = self.view.get_object()
- inst = isinstance(obj,Product)
+ inst = isinstance(obj, Product)
self.assertEqual(inst, True)
-
+
def test_get_templates_return_expected_values(self):
self.view = ProductDetailView()
setattr(self.view, 'object', None)
tmp = self.view.get_template_names()
self.assertEqual(len(tmp), 1)
-
+
class CartDetailsViewTestCase(TestCase):
def setUp(self):
- self.user = User.objects.create(username="test",
+ self.user = User.objects.create(username="test",
email="test@example.com",
first_name="Test",
last_name = "Tester")
-
+
self.cart = Cart.objects.create()
- self.product= Product.objects.create()
- self.item = CartItem.objects.create(cart=self.cart, quantity=1,
+ self.product = Product.objects.create()
+ self.item = CartItem.objects.create(cart=self.cart, quantity=1,
product=self.product)
-
-
+
+
def test_get_context_data_works(self):
request = Mock()
setattr(request, 'user', self.user)
view = CartDetails(request=request)
ret = view.get_context_data()
- self.assertNotEqual(ret,None)
-
+ self.assertNotEqual(ret, None)
+
def test_context_has_as_many_items_as_cart(self):
self.cart.user = self.user
self.cart.save()
@@ -65,55 +65,55 @@ def test_context_has_as_many_items_as_cart(self):
setattr(request, 'user', self.user)
view = CartDetails(request=request)
ret = view.get_context_data()
- self.assertNotEqual(ret,None)
- self.assertEqual(len(ret['cart_items']),1)
+ self.assertNotEqual(ret, None)
+ self.assertEqual(len(ret['cart_items']), 1)
self.assertEqual(ret['cart_items'][0], self.item)
-
+
def test_calling_post_redirects_properly(self):
self.cart.user = self.user
self.cart.save()
-
+
request = Mock()
setattr(request, 'is_ajax', lambda :False)
setattr(request, 'user', self.user)
- post={
+ post = {
'add_item_id':self.product.id,
'add_item_quantity':1,
}
setattr(request, 'POST', post)
-
+
view = CartDetails(request=request)
ret = view.post()
- self.assertTrue(isinstance(ret,HttpResponseRedirect))
-
+ self.assertTrue(isinstance(ret, HttpResponseRedirect))
+
ret = view.get_context_data()
- self.assertNotEqual(ret,None)
- self.assertEqual(len(ret['cart_items']),1)
-
+ self.assertNotEqual(ret, None)
+ self.assertEqual(len(ret['cart_items']), 1)
+
self.assertEqual(ret['cart_items'][0], self.item)
self.assertEqual(ret['cart_items'][0].quantity, 2)
-
+
def test_calling_ajax_post_returns_content(self):
self.cart.user = self.user
self.cart.save()
-
+
request = Mock()
setattr(request, 'is_ajax', lambda :True)
setattr(request, 'user', self.user)
- post={
+ post = {
'add_item_id':self.product.id,
'add_item_quantity':1,
}
setattr(request, 'POST', post)
-
+
view = CartDetails(request=request)
ret = view.post()
- self.assertTrue(isinstance(ret,HttpResponse))
-
+ self.assertTrue(isinstance(ret, HttpResponse))
+
ret = view.get_context_data()
self.assertNotEqual(ret,None)
self.assertEqual(len(ret['cart_items']),1)
-
+
self.assertEqual(ret['cart_items'][0], self.item)
self.assertEqual(ret['cart_items'][0].quantity, 2)
@@ -163,7 +163,13 @@ def test_cart_update(self):
self.add_product_to_cart(self.product)
cart = self.get_cart()
- post = { 'update_item-%d' % cart.items.all()[0].pk : '5' }
+ cart_item_id = cart.items.all()[0].pk
@mbrochh Owner
mbrochh added a note

Modelformsets complicate things a bit for tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ post = {
+ 'form-TOTAL_FORMS': '1',
+ 'form-INITIAL_FORMS': '1',
+ 'form-MAX_NUM_FORMS': '',
+ 'form-0-id': str(cart_item_id),
+ 'form-0-quantity': '5' }
response = self.client.post(reverse("cart_update"), post)
self.assertEqual(response.status_code, 302)
self.assertCartHasItems(5)
@@ -175,7 +181,7 @@ def test_cart_item_update(self):
cart_item_id = cart.items.all()[0].pk
url = reverse('cart_item', kwargs={'id': cart_item_id})
post = { 'item_quantity': "5" }
- response = self.client.post(url, post,
+ response = self.client.post(url, post,
HTTP_X_REQUESTED_WITH='XMLHttpRequest')
self.assertEqual(response.status_code, 200)
self.assertCartHasItems(5)
View
49 shop/views/cart.py
@@ -1,6 +1,8 @@
# -*- coding: utf-8 -*-
from django.core.urlresolvers import reverse
from django.http import HttpResponse, HttpResponseRedirect
+
+from shop.forms import get_cart_item_formset
from shop.models.productmodel import Product
from shop.util.cart import get_or_create_cart
from shop.views import ShopView, ShopTemplateResponseMixin
@@ -28,37 +30,37 @@ def dispatch(self, request, *args, **kwargs):
self.args = args
self.kwargs = kwargs
return handler(request, *args, **kwargs)
-
+
def post(self, request, *args, **kwargs):
"""
Update one of the cartItem's quantities. This requires a single 'item_quantity'
POST parameter, but should be posted to a properly RESTful URL (that should
contain the item's ID):
-
+
http://example.com/shop/cart/item/12345
"""
cart_object = get_or_create_cart(self.request)
item_id = self.kwargs.get('id')
# NOTE: it seems logic to be in POST but as tests client shows
- #with PUT request, data is in GET variable
+ # with PUT request, data is in GET variable
# TODO: test in real client
- #quantity = self.request.POST['item_quantity']
+ # quantity = self.request.POST['item_quantity']
quantity = self.request.POST['item_quantity']
cart_object.update_quantity(item_id, int(quantity))
return self.put_success()
-
+
def delete(self, request, *args, **kwargs):
"""
- Deletes one of the cartItems. This should be posted to a properly
+ Deletes one of the cartItems. This should be posted to a properly
RESTful URL (that should contain the item's ID):
-
+
http://example.com/shop/cart/item/12345
"""
cart_object = get_or_create_cart(self.request)
item_id = self.kwargs.get('id')
cart_object.delete_item(item_id)
return self.delete_success()
-
+
# success hooks
def success(self):
"""
@@ -71,19 +73,19 @@ def success(self):
def post_success(self, product, cart_item):
"""
- Post success hook"
+ Post success hook
"""
return self.success()
def delete_success(self):
"""
- Post delete hook"
+ Post delete hook
"""
return self.success()
def put_success(self):
"""
- Post put hook"
+ Post put hook
"""
return self.success()
@@ -94,7 +96,7 @@ class CartDetails(ShopTemplateResponseMixin, CartItemDetail):
This is the actual "cart" view, that answers to GET and POST requests like
a normal view (and returns HTML that people can actually see)
"""
-
+
template_name = 'shop/cart.html'
action = None
@@ -106,7 +108,6 @@ def get_context_data(self, **kwargs):
cart_object.update(state)
ctx.update({'cart': cart_object})
ctx.update({'cart_items': cart_object.get_updated_cart_items()})
-
return ctx
def get(self, request, *args, **kwargs):
@@ -115,11 +116,13 @@ def get(self, request, *args, **kwargs):
this only extends the mixin and not templateview.
"""
context = self.get_context_data(**kwargs)
+ formset = get_cart_item_formset(cart_items=context['cart_items'])
+ context.update({'formset': formset, })
return self.render_to_response(context)
def post(self, *args, **kwargs):
"""
- This is to *add* a new item to the cart. Optionally, you can pass it a
+ This is to *add* a new item to the cart. Optionally, you can pass it a
quantity parameter to specify how many you wish to add at once (defaults
to 1)
"""
@@ -145,14 +148,14 @@ def put(self, *args, **kwargs):
"""
Update shopping cart items quantities.
- Data should be in update_item-ID=QTY form, where ID is id of cart item
+ Data should be in update_item_ID=QTY form, where ID is id of cart item
and QTY is quantity to set.
"""
- field_prefix = 'update_item-'
- cart_item_fields = [k for k in self.request.POST.keys()
- if k.startswith(field_prefix)]
- cart_object = get_or_create_cart(self.request)
- for key in cart_item_fields:
- id = key[len(field_prefix):]
- cart_object.update_quantity(id, int(self.request.POST[key]))
- return self.put_success()
+ context = self.get_context_data(**kwargs)
@mbrochh Owner
mbrochh added a note

The usage of all this stuff in our view is actually quite straightforward and elegant, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ formset = get_cart_item_formset(cart_items=context['cart_items'],
+ data=self.request.POST)
+ if formset.is_valid():
+ formset.save()
+ return self.put_success()
+ context.update({'formset': formset, })
+ return self.render_to_response(context)
Something went wrong with that request. Please try again.