Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from Dec 3, 2011
View
@@ -19,3 +19,4 @@ database.db
*.swp
*coverage.xml
.idea
+.ropeproject
View
@@ -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
@@ -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):
+ """A form for the CartItem model. To be used in the CartDetails view."""
@mbrochh

mbrochh Nov 18, 2011

Contributor

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 :)

+
+ 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,
+ quantity)
@mbrochh

mbrochh Nov 18, 2011

Contributor

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

chrisglass Nov 21, 2011

Contributor

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

+ 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:
+ for cart_item in cart_items:
@mbrochh

mbrochh Nov 18, 2011

Contributor

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.

+ if form.instance.id == cart_item.id:
+ form.instance = cart_item
+ return form_set
@@ -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

mbrochh Nov 18, 2011

Contributor

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

chrisglass Nov 21, 2011

Contributor

Nice.

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

mbrochh Nov 18, 2011

Contributor

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

chrisglass Nov 21, 2011

Contributor

Yeah this makes sense.

Still not sure, maybe we should return None?

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

mbrochh Nov 18, 2011

Contributor

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

+ '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

mbrochh Nov 18, 2011

Contributor

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

@chrisglass

chrisglass Nov 21, 2011

Contributor

Ohhh good catch. Thanks.

+ 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,21 +415,21 @@ 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,
verbose_name=_('Product name'))
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'
Oops, something went wrong.