1

Suppose I have a view for saving an order to a database based on cart contents:

def cart_checkout(request):
    order = Order()
    order.first_name = 'x'
    order.last_name = 'y'
    order.address = 'z'
    order.save()

    cart = Cart(request)
    for product_id, product_quantity in cart:
        product = Product.objects.get(pk=product_id)
        order_item = OrderItem()
        order_item.order = order
        order_item.name = product.name
        order_item.price = product.price
        order_item.amount = product_quantity
        order_item.save()
    order.update_total_price() # updates the Order total price field with the sum of order items prices
    order.save()
    return HttpResponse('Checked-out!')

As you can see, I am calling order.save() twice in this view: first to create an Order instance the OrderItems can be attached to in the for loop, and then to update the total price of the order based on order items in it. If I removed the first .save(), I would get an error on the second one telling me the order needs to be saved first.

Calling the .save() method twice does not seem DRY enough to me. Is there a way to do it only once?


Note that I am not subclassing ModelForm, so I cannot use .save(commit=False). Also, I do not want to just hide the save() method in the update_total_price() method.

Models.py:

from django.db import models
from .mixins import DisplayNameMixin

class Product(DisplayNameMixin, models.Model):
    name = models.CharField(max_length=255)
    price = models.DecimalField(max_digits=6, decimal_places=2)
    amount = models.IntegerField()

class Order(models.Model):
    first_name = models.CharField(max_length=255)
    last_name = models.CharField(max_length=255)
    address = models.CharField(max_length=255)
    total_price = models.DecimalField(max_digits=10, decimal_places=2, default=0)

    def update_total_price(self):
        order_items = self.orderitem_set.all()
        self.total_price = sum([
            x.price * x.amount
            for x in order_items
        ])

class OrderItem(models.Model):
    order = models.ForeignKey('Order', on_delete=models.CASCADE)
    name = models.CharField(max_length=255)
    price = models.DecimalField(max_digits=6, decimal_places=2)
    amount = models.IntegerField()
Dabble
  • 182
  • 1
  • 3
  • 14
barciewicz
  • 3,511
  • 6
  • 32
  • 72

3 Answers3

2

I think, you can't help but save the order twice, as you need to have an order_id to create the OrderItems, and then update the order with the items' amount. I have a few suggestions to make though.

  • You can make total_price a calculated property, so that you would not have to save the order:

    class Order(models.Model):
        first_name = models.CharField(max_length=255)
        last_name = models.CharField(max_length=255)
        address = models.CharField(max_length=255)
        total_price = models.DecimalField(max_digits=10, decimal_places=2, default=0)
    
        @property
        def total_price(self):
            return sum([
                x.price * x.amount
                for x in self.orderitem_set.all()
            ])
    
engin_ipek
  • 887
  • 1
  • 8
  • 10
1

From DB theory perspective your DB structure is wrong. It needs to be normalized first.

Why it is wrong?

Order.total_price is redundant table column. That information can be found with aggregation. At DB level there are no protections preventing DB users (Django app in your case) from entering compromised data. So your DB can be telling two different total prices (Order.total_price != SUM(OrderItem.price * OrderItem.amount)) at the same time.

So to appease DB normalization gods you need to drop total_price field and use Django aggregations/annotations (https://docs.djangoproject.com/en/3.0/topics/db/aggregation/) when you need to access it.


Saying that, there could be a totally valid reason to put total_price inside Order table. That reason usually is performance. Sometimes SQL query complexity (It is very annoying to filter by an aggregated column).

But there is a price. And that price is de-normalization of your DB. In your case you are paying breaking DRY principle.

Just make sure that you are calling both save()'s in the same transaction.

petraszd
  • 4,249
  • 1
  • 20
  • 12
1

To expand on petraszd's answer (i.e. remove the total_price field) and engin_ipek's answer (i.e. add total_price as a calculated property), you could try making total_price a cached property, to avoid calculating the same value more than once - as long as the same Order instance is passed around.

You would also probably make the calculation a little less expensive if you used aggregation to calculate the total price, as petraszd mentioned, e.g. adding the products of price and amount.

Dabble
  • 182
  • 1
  • 3
  • 14