3
\$\begingroup\$

I'm learning backend dev and python and django1.

My first app is a basic auction site. Users sign up, submit an auction, and other users bid on the auctions.

I would highly appreciate any experienced eyes running over the code and letting me know if I am doing anything wrong. I don't want to start the rather labrorious process of writing tests, or add extra functionality, until the code I'm testing has been checked to not be flawed.

I have two particular concerns with my code which I would like some input on.

First, I have "ATOMIC_REQUESTS" turned on in my settings file. In the form_valid method of the AuctionBid class, below, I generate a message depending on whether the update of the two models - called by the Bid.new_winner() static method - is successful or not. Is this the correct approach ? If it is, I am interested to know how I would rollback all the queries in a view if any error-ed. In my case there are none. But if update queries had been called in the AuctionBid class view prior to one of the queries called by Bid.new_winner() and I wanted to rollback everything how do I achieve this? Looking at the docs I think I am supposed to use nesting transaction blocks like in my case but pass a "savepoint=False" flag to each so that it rollbacks back to the prior parent block. So if I did this for each nested block in my view it would roll all the way back. Is this right?

Second, about the Bid.new_winner static method again. I check if the bidder is the auction creator and if so raise a PermissionDenied exception. Is this best approach?

Any tips or comments would be very welcome.

admin.py

from django.contrib import admin
from .models import Duration, Win, Bid, Auction, Payment

admin.site.register(Duration)
admin.site.register(Win)
admin.site.register(Bid)
admin.site.register(Auction)
admin.site.register(Payment)

apps.py

from django.apps import AppConfig

class AuctionConfig(AppConfig):
    name = 'auction'

forms.py

from django import forms
from django.core.exceptions import ValidationError
from django.utils import timezone
from django.utils.translation import ugettext_lazy as _
import datetime

from .models import Auction, Bid, Duration

class AuctionBidForm(forms.ModelForm):

    class Meta:
        model = Bid
        fields = ["value"]
        labels = {'value': _('Bid')}

    def __init__(self, *args, **kwargs):
        self.auction = kwargs.pop("auction")
        super().__init__(*args, **kwargs)

    def clean_value(self):
        value = self.cleaned_data["value"]
        if self.auction.reserve > value:
            raise ValidationError(_('Bid less than the reserve'))
        if (self.auction.winning_bid is not None
            and self.auction.winning_bid.value >= value):
            raise ValidationError(_('Bid not greater than current winning bid'))
        return value

class AuctionCreateForm(forms.ModelForm):

    class Meta:
        model = Auction
        fields = ['title', 'reserve']
        labels = {'reserve': _('Reserve')}

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.fields["duration_value"] = forms.ModelChoiceField(
            queryset=Duration.objects.all(),
            empty_label=None,
            to_field_name="value",
        )

models.py

from django.conf import settings
import uuid
from datetime import datetime, timedelta
import humanfriendly
from django.core.exceptions import PermissionDenied
from django.db import models
from django.db.models import Max, Sum
from django.urls import reverse, reverse_lazy
from django.utils import timezone
from django.http import Http404
from dateutil.relativedelta import relativedelta
from django.db.models import DateTimeField, F, ExpressionWrapper
import re

def time_x_months_ago(x):
    now = timezone.now()
    return now - relativedelta(months=x)

class Duration(models.Model):
    """ Choice durations for auction """
    value = models.DurationField()

    class Meta:
        ordering = ['value']

    def __str__(self):
        return humanfriendly.format_timespan(self.value)


class WinQuerySet(models.QuerySet):
    """ Personalised queryset to improve
    usability """

    # To be a perodic task -- when i learn Celery
    def find(self):
        undeclaredWinners = (
            Auction.objects
                .select_related("winning_bid")
                .select_related("winning_bid__win")
                .annotate(
                    valid_until=ExpressionWrapper(
                            F('valid_from') + F('duration__value'), output_field=DateTimeField()
                        )
                )
                .filter(valid_until__lt=timezone.now())
                .exclude(winning_bid=None)
                .filter(winning_bid__win=None)
        )
        if undeclaredWinners:
            for auction in undeclaredWinners:
                winning_bid = auction.winning_bid
                win = self.create(
                        duration=Duration.objects.get(value=timedelta(seconds=4*60*60)),
                        bid=winning_bid
                    )
                winning_bid.win = win
                winning_bid.save()

    def seller(self, **kwargs):
        seller = kwargs["seller"]
        return (
            self.filter(bid__auction__creator=seller)
        )

    def sales(self):
        back_three_months = time_x_months_ago(3)
        return (
            self.select_related("bid")
                .select_related("bid__auction")
                .filter(payment__paid_timestamp__gt=back_three_months)
                .filter(payment__paid=True)
        )

    def total(self, **kwargs):
        return (
            self.aggregate(Sum('bid__value'))
        )

    def buyer(self, **kwargs):
        buyer = kwargs["buyer"]
        return (
            self.filter(bid__bidder=buyer)
        )

    def offers(self):
        return (
            self.annotate(
                valid_until=ExpressionWrapper(
                    F('valid_from') + F('duration__value'), output_field=DateTimeField()
                )
            )
            .filter(valid_until__gt=timezone.now())
            .filter(payment=None)
        )

    # We need lots of foreign properties for the
    # __str__ method so this is important to avoid
    # a deluge of db hits
    def get_queryset(self):
        return super().get_queryset().select_related()

class Win(models.Model):
    """ Model representing
    a win on an auction """
    uuid = models.UUIDField(
        db_index=True, 
        default=uuid.uuid4, 
        editable=False
    ) 
    valid_from = models.DateTimeField(auto_now_add=True)
    duration = models.ForeignKey(
        Duration, 
        on_delete=models.SET_NULL, 
        null=True
    )
    bid = models.ForeignKey(
        'Bid',
        related_name='win_bid',
        on_delete=models.SET_NULL,
        null=True,
    )
    payment = models.ForeignKey(
        'Payment',
        related_name='win_payment',
        on_delete=models.SET_NULL,
        null=True,
        blank=True,
    )

    objects = WinQuerySet.as_manager()

    def __str__(self):
        """ String representing a win """

        # In the Model Manager we override get_queryset
        # to avoid hitting the db several times in this method

        now = timezone.now()
        if self.payment:
            if self.payment.paid: 
                # RP = Payment received
                return (
                    f"""RP, val: {self.bid.value}, """
                    f"""auction: {self.bid.auction.id}"""
                )
            else:
                # PNR = Payment not received
                return (
                    f"""PNR, val: {self.bid.value}, """
                    f"""auction: {self.bid.auction.id} """
                )                
        else:
            # NRP = Never received payment
            if self.valid_from + self.duration.value < now:
                return (
                    f"""NRP, val: {self.bid.value}, """
                    f"""auction: {self.bid.auction.id}"""
                )
            # AP = Awaiting payment
            if self.valid_from + self.duration.value > now:
                return (
                    f"""AP, {self.bid.value}, """
                    f"""auction: {self.bid.auction.id}"""
                )

class BidQuerySet(models.QuerySet):
    """ Personalised queryset to improve
    usability """

    def get_queryset(self):
        return super().get_queryset().select_related()

    def bidder(self, **kwargs):
        bidder = kwargs["bidder"]
        return (
            self.filter(bidder=bidder)
        )

    def bids(self):
        return (
            self.annotate(
                    valid_until=ExpressionWrapper(
                        F('auction__valid_from') + F('auction__duration__value'), output_field=DateTimeField()
                    )
                )
                .filter(valid_until__gt=timezone.now())
                .values('auction', 'auction__uuid', 'auction__title', 'auction__winning_bid__value')
                .annotate(last_bid=Max('value'))
        )

    def total(self):
        return (
            self.aggregate(Sum('last_bid'))
        )

class Bid(models.Model):
    """ Model representing
    a bid in an auction """
    uuid = models.UUIDField(
        db_index=True, 
        default=uuid.uuid4, 
        editable=False
    ) 
    auction = models.ForeignKey(
        'Auction',
        related_name='bid_auction',
        on_delete=models.SET_NULL,
        null=True
    )
    bid_time = models.DateTimeField(auto_now_add=True)
    bidder = models.ForeignKey(
        settings.AUTH_USER_MODEL,
        related_name='bid_user',
        on_delete=models.SET_NULL,
        null=True
    )
    value = models.PositiveIntegerField()
    win = models.ForeignKey(
        Win,
        related_name='bid_win',
        on_delete=models.SET_NULL,
        null=True,
        blank=True,
    )

    objects = BidQuerySet.as_manager()

    def __str__(self):
        return f"£{self.value} in Auction No: {self.auction.id}"

    def get_absolute_url(self):
        """ Returns the url to access
        a particular auction """
        return reverse_lazy(
            'auction:list'
        )

    @staticmethod
    def new_winner(auction, bidder, bid_value):
        """ Method to create a new winning bid
        on the auction 

        :param auction: the auction the bid in
        :param bidder: the user who made the bid
        :param bid_value: the bid value
        """
        if not auction.isRunning:
            return
        if bid_value < auction.reserve:
            return
        if (auction.winning_bid and
            bid_value < auction.winning_bid.value):
            return
        if bidder.id is auction.creator.id:
            raise PermissionDenied

        new_winning_bid = Bid.objects.create(
            auction=auction,
            bidder=bidder,
            value=bid_value
        )
        auction.winning_bid = new_winning_bid
        auction.save()


class AuctionQuerySet(models.QuerySet):
    """ Personalised queryset to improve
    usability """

    def get_object_or_404(self, **kwargs):
        uuid = kwargs["uuid"]
        now = timezone.now()
        try:
            a = (self.annotate(
                    valid_until=ExpressionWrapper(
                        F('valid_from') + F('duration__value'), output_field=DateTimeField()
                    )
                ).filter(valid_until__gt=now)
                .get(uuid=uuid)
            )
        except Auction.DoesNotExist:
            raise Http404("No live auction found")
        return a


class Auction(models.Model):
    """ Model for an auction """
    uuid = models.UUIDField(
        db_index=True,
        default=uuid.uuid4,
        editable=False,
    )
    title = models.CharField(max_length=10)
    valid_from = models.DateTimeField(auto_now_add=True)
    duration = models.ForeignKey(
        Duration, 
        on_delete=models.SET_NULL, 
        null=True
    )
    reserve = models.PositiveIntegerField(default=0)
    creator = models.ForeignKey(
        settings.AUTH_USER_MODEL,
        on_delete=models.SET_NULL,
        null=True,
    )
    winning_bid = models.ForeignKey(
        'Bid',
        related_name='highest_bid',
        on_delete=models.SET_NULL,
        null=True,
        blank=True,
    )

    objects = AuctionQuerySet.as_manager()    

    def __str__(self):
        return f"Auction: {self.id}"

    def get_absolute_url(self):
        """ Returns the url to access
        a particular auction """
        return reverse_lazy(
            'auction:detail',
            kwargs={'uuid': self.uuid}
        )

    def isRunning(self):
        expiration = self.valid_from + self.duration.value
        return timezone.now() < expiration

class Payment(models.Model):
    """ Model for a payment 
    i.e. represents a payment via
    Stripe """
    win = models.ForeignKey(
        Win,
        related_name='payment_win',
        on_delete=models.SET_NULL,
        null=True,
    )
    valid_from = models.DateTimeField(auto_now_add=True)
    duration = models.ForeignKey(
        Duration, 
        on_delete=models.SET_NULL, 
        null=True
    )
    paid_timestamp = models.DateTimeField(
        null=True,
        blank=True
    )
    paid = models.BooleanField(default=False)

urls.py

from django.urls import path
from . import views

app_name = 'auction'

urlpatterns = [
    path('', views.AuctionList.as_view(), name='list'),
    path('create/', views.AuctionCreate.as_view(), name='create'),
    path('<uuid:uuid>/', views.AuctionBid.as_view(), name='detail'),
    path('dashboard/', views.dashboard, name="dashboard"),
    path('sales/', views.Sales.as_view(), name='sales-list'),
    path('purchases/', views.Purchases.as_view(), name='purchases-list'),
    path('offers/', views.Offers.as_view(), name='offers-list'),
    path('bids/', views.Bids.as_view(), name='bids-list'),
    path('offers/<uuid:uuid>/', views.Buy.as_view(), name="buy"),
]

utils.py

from django.utils import timezone
from datetime import datetime, timedelta
import re

def days_hours_minutes_seconds(td):
    s = (td.seconds % 3600) // 60, (td.seconds % 3600) % 60
    st = days_hours_minutes(td)
    st += " s "
    st = re.sub("^((00 [dhms] )|(0 [dhms] ))+","", st)
    return st

def days_hours_minutes(td):
    d, h = td.days, td.seconds //3600
    m = (td.seconds % 3600) // 60
    st = ""
    st += str(d) + " d "
    st += str(h) + " h "
    st += str(m) + " m "
    st = re.sub("^((00 [dhm] )|(0 [dhm] ))+","", st)
    return st

def calculate_time_remaining(expiration):
    remaining = expiration - timezone.now() 
    remaining -= timedelta(microseconds=remaining.microseconds) # cut off microseconds
    remaining = days_hours_minutes(remaining)
    return remaining

utils.py

from django.core.exceptions import PermissionDenied
from django.views import generic, View
from django.views.generic.list import ListView
from django.views.generic.edit import CreateView, UpdateView
from django.views.generic.detail import DetailView
from django.http import Http404, HttpResponse, HttpResponseRedirect, HttpResponseNotFound
from django.shortcuts import render, get_object_or_404
from datetime import datetime, timedelta
from django.utils.dateparse import parse_duration
from django.utils import timezone
from django.db import Error, IntegrityError, transaction
from django.db.models import DateTimeField, F, ExpressionWrapper
from django.contrib import messages
from django.contrib.auth.mixins import LoginRequiredMixin
from django.contrib.auth.decorators import login_required
from django.forms.utils import ErrorList
from django.urls import reverse
from .utils import calculate_time_remaining

from .models import Auction, Bid, Duration, Win, Payment
from .forms import AuctionBidForm, AuctionCreateForm

class Buy(LoginRequiredMixin, UpdateView):
    """ View for making a payment for
    a win on an auction """
    model = Win
    template_name = "auction/buy.html"
    context_object_name = "offer"
    fields = [] # keep django happy
    success_url = "/auction/offers"

    def get_object(self):
        buyer = self.request.user
        uuid = self.kwargs.get("uuid")
        try:
            offer = Win.objects.buyer(buyer=buyer).offers().get(uuid=uuid)
            offer.remaining = calculate_time_remaining(offer.valid_until)
        except self.model.DoesNotExist:
            raise Http404("No such offer exists") # Will need our own 404.html template i think
        return offer

    def form_valid(self, form):
        win = self.object
        try:
            with transaction.atomic():
            duration = Duration.objects.get(value=timedelta(seconds=4*60*60))
            payment = Payment.objects.create(
                    win=win, 
                    duration=duration, 
                    paid_timestamp=timezone.now(), 
                    paid=True
                )
            win.payment = payment
            win.save()
        except IntegrityError:
            # return a failure message to the user
            return Http404("Something went wrong")
        # db updates were successful
        # send back a success message to the user
        return super().form_valid(form)


class Sales(LoginRequiredMixin, ListView):
    """ List of users' sales
    over 3 month period """
    model = Win
    paginate_by = 10
    context_object_name = "sales_list"
    template_name = "auction/sales_list.html"  

    def get_queryset(self):
        user = self.request.user
        queryset = (
            Win.objects.seller(seller=user)
                .sales()
        )
        q = self.request.GET.get("q")
        if q:
            return queryset.filter(bid__auction__title__icontains=q)
        return queryset


class Purchases(LoginRequiredMixin, ListView):
    """ List of users' purchases
    over 3 month period """
    model = Win
    paginate_by = 10
    context_object_name = "purchases_list"
    template_name = "auction/purchases_list.html"

    def get_queryset(self):
        user = self.request.user
        queryset = (
            Win.objects.buyer(buyer=user)
                .sales()
        )
        q = self.request.GET.get("q")
        if q:
            return queryset.filter(bid__auction__title__icontains=q)
        return queryset

class Offers(LoginRequiredMixin, ListView):
    """ List of offers """
    model = Win
    paginate_by = 10
    template_name = "auction/offers_list.html"
    context_object_name = "offers_list"

    def get_context_data(self, **kwargs):
        """ Add the time remaining to the
        model object """
        cxt = super().get_context_data()
        offerList = cxt['object_list']
        for offer in offerList:
            expiration = offer.valid_until # added to the object on the queryset
            offer.remaining = calculate_time_remaining(expiration)
        return cxt

    def get_queryset(self):
        user = self.request.user
        queryset = (
            Win.objects.buyer(buyer=user)
                .offers()
        )
        q = self.request.GET.get("q")
        if q:
            return queryset.filter(bid__auction__title__icontains=q)
        return queryset

class Bids(LoginRequiredMixin, ListView):
    """ List of bids """
    model = Bid
    paginate_by = 10
    template_name = "auction/bids_list.html"

    def get_queryset(self):
        user = self.request.user
        return Bid.objects.bidder(bidder=user).bids()

class AuctionList(ListView):
    """ List of Live Auctions """
    model = Auction
    paginate_by = 10

    def get_context_data(self, **kwargs):
        """ Add the time remaining to the
        model object """
        cxt = super().get_context_data()
        auctionList = cxt['object_list']
        for auction in auctionList:
            expiration = auction.valid_until # added to the object on the queryset
            auction.remaining = calculate_time_remaining(expiration)
        return cxt

    def get_queryset(self):
        """ Live Auctions only """
        now = timezone.now()
        queryset = (
            Auction.objects
                .select_related('winning_bid')
                .select_related('duration')
                .annotate(
                    valid_until=ExpressionWrapper(
                            F('valid_from') + F('duration__value'), output_field=DateTimeField()
                        )
                )
                .filter(valid_until__gt=now)
        )
        # Get the q GET parameter -- pg 140 of Two Scoops
        q = self.request.GET.get("q")
        if q:
            return queryset.filter(title__icontains=q)
        return queryset

class AuctionCreate(LoginRequiredMixin, CreateView):
    """ View function for creating an auction """
    form_class = AuctionCreateForm
    template_name = "auction/auction_form.html"    
    success_url = "/auction/"

    def post(self, request, *args, **kwargs):
        request.POST = request.POST.copy() # beause request.POST is immutable first off
        duration_value = request.POST["duration_value"]
        duration_value = parse_duration(duration_value)
        request.POST["duration_value"] = duration_value
        return super().post(request, *args, **kwargs)                                                 

    def form_valid(self, form):
        form.instance.start = timezone.now()
        duration_value = form.cleaned_data.get('duration_value')
        form.instance.duration = duration_value
        form.instance.creator = self.request.user
        return super().form_valid(form)


class AuctionBid(LoginRequiredMixin, CreateView):
    model = Bid
    form_class = AuctionBidForm
    template_name = "auction/auction_detail.html"

    def get_context_data(self, **kwargs):
        user = self.request.user
        c = super().get_context_data(**kwargs)
        c["auction"] = self.auction
        if user.id is self.auction.creator.id:
            c["form"] = None
        return c

    def get_form_kwargs(self):       
        fkwargs = super().get_form_kwargs()
        uuid = self.kwargs.get("uuid")
        auction = Auction.objects.get_object_or_404(uuid=uuid)
        auction.remaining = calculate_time_remaining(
            auction.valid_from + auction.duration.value
        )
        fkwargs["auction"] = auction
        self.auction = auction
        return fkwargs

    def form_invalid(self, form):
        return super().form_invalid(form)

    def form_valid(self, form):
        bid_value = form.cleaned_data["value"]

        try:
            with transaction.atomic():
                Bid.new_winner(
                    self.auction, 
                    self.request.user, 
                    bid_value
                )
        except IntegrityError:
            messages.error(self.request, "An unexpected error occured so your bid did not submit")

        messages.success(self.request, "Bid submitted successfully!")

        return super().form_valid(form)

@login_required
def dashboard(request):
    # this should be a periodic task but until
    # i learn about Celery this will have to do
    user = request.user
    Win.objects.find()
    context = {}
    sales = Win.objects.seller(seller=user).sales().total()
    context["sales"] = sales
    purchases = Win.objects.buyer(buyer=user).sales().total()
    context["purchases"] = purchases
    offers = Win.objects.buyer(buyer=user).offers().total()
    context["offers"] = offers
    bids = Bid.objects.bidder(bidder=user).bids().total()
    context["bids"] = bids
    return render(
        request,
        'auction/dashboard.html',
        context
    )


1 What I've read so far: I've been using the excellent resource https://www.fullstackpython.com/ as a roadmap. This led me to complete the official tutorial first, the book "Django for beginners" second, the MDN tutorial third, and a lot of the book "Two Scoops of Django" finally.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I edited your question title so that it is more descriptive of what your are actually trying to accomplish. You can read more about this at How do I ask a good question?. \$\endgroup\$ – AlexV May 12 at 17:01
  • \$\begingroup\$ Thank you very much ! \$\endgroup\$ – user1849962 May 12 at 20:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.