Django views for banking operations

Question

How can I Pythonize/Djangoize/DRY my Django code?

views.py

class AccountDetail(DetailView):
    model = BankAccount
    template_name = 'bank/account_detail.html'

    def get(self, *args, **kwargs):
        self.object = self.get_object()

        if self.object.is_owner(self.request.user.citizen):
            return render(self.request, self.template_name, self.get_context_data())
        raise Http404


class SendTransfer(SingleObjectMixin, FormView):
    model = BankAccount
    form_class = SendTransferForm
    template_name = 'bank/send_transfer.html'

    def dispatch(self, request, *args, **kwargs):
        self.object = self.get_object()
        return super(SendTransfer, self).dispatch(request, *args, **kwargs)

    def get_object(self, queryset=None):
        obj = super(SendTransfer, self).get_object(queryset)

        if not obj.is_owner(self.request.user.citizen):
            raise Http404

        return obj

    def form_valid(self, form):
        data = form.cleaned_data
        MoneyTransfer.objects.create(sender=self.object,
                                     receiver=data['receiver'],  # ModelChoiceField in the form
                                     total=data['total'],  # FloatField in the form, etc.
                                     when=timezone.localtime(timezone.now()),
                                     comment=data['comment'])
        return redirect('AccountDetail', self.object.pk)

    def form_invalid(self, form):
        return render(self.request, self.template_name, self.get_context_data())

    def get_form_kwargs(self):
        kwargs = super(SendTransfer, self).get_form_kwargs()
        kwargs['sender'] = BankAccount.objects.get(id=self.kwargs['pk'])
        kwargs['user'] = self.request.user
        return kwargs


class OutcomeTransfers(DetailView):
    model = BankAccount
    template_name = 'bank/report.html'

    def get_object(self, queryset=None):
        obj = super(OutcomeTransfers, self).get_object(queryset)

        if not obj.is_owner(self.request.user.citizen):
            raise Http404

        return obj

    def _get_queryset(self):
        return self.model.objects.get(id=self.kwargs['pk']).outcome_transfers.all()

    def get_context_data(self, **kwargs):
        data = super(OutcomeTransfers, self).get_context_data()
        queryset = self._get_queryset()
        if 'sort' in self.request.GET:
            queryset = queryset.order_by(self.request.GET['sort'])
        data['table'] = MoneyTransferTable(queryset)
        return data


class IncomeTransfers(DetailView):
    model = BankAccount
    template_name = 'bank/report.html'

    def get_object(self, queryset=None):
        obj = super(IncomeTransfers, self).get_object(queryset)

        if not obj.is_owner(self.request.user.citizen):
            raise Http404

        return obj

    def _get_queryset(self):
        return self.model.objects.get(id=self.kwargs['pk']).income_transfers.all()

    def get_context_data(self, **kwargs):
        data = super(IncomeTransfers, self).get_context_data()
        queryset = self._get_queryset()
        if 'sort' in self.request.GET:
            queryset = queryset.order_by(self.request.GET['sort'])
        data['table'] = MoneyTransferTable(queryset)
        return data

models.py

class RuleQuerySet(models.QuerySet):

    def on_date(self, _date):
        return self.get(start_date__lte=_date, end_date__gte=_date)


class TransferQuerySet(models.QuerySet):

    def on_date(self, _date):
        return self.filter(when__lt=_date + timedelta(days=1), when__gte=_date)


class Rule(models.Model):
    start_date = models.DateField()
    end_date = models.DateField()

    deposit_percent = models.DecimalField(max_digits=10, decimal_places=3)
    credit_percent = models.DecimalField(max_digits=10, decimal_places=3)

    objects = RuleQuerySet.as_manager()


class BankAccount(models.Model):

    organization = models.OneToOneField(Organization, related_name='bank_account', blank=True, null=True)
    citizen = models.OneToOneField(Citizen, related_name='bank_account', blank=True, null=True)

    DEBET = 1
    CREDIT = 2

    method = models.PositiveSmallIntegerField(choices=(
        (DEBET, 'Дебет'),
        (CREDIT, 'Кредит'),
    ))

    when_opened = models.DateField()

    @property
    def owner(self):
        if self.is_legal():
            return self.organization
        return self.citizen

    def __unicode__(self):
        prefix = "L" if self.is_legal() else "N"
        prefix += "D" if self.method == self.DEBET else "C"
        public_id = str(self.id).zfill(4)
        return "%s (#%s)" % (self.owner, prefix + public_id)

    def is_legal(self):
        if self.organization and self.citizen:
            raise ValueError("Account is legal and natural at the same time. Make it ")
        return self.organization is not None

    def is_owner(self, citizen):
        return (self.citizen and self.citizen.id == citizen.id) or \
               (self.organization and self.organization.owners.all().filter(id=citizen.id).count())

    @property
    def balance(self):
        today = timezone.localtime(timezone.now()).date()
        if self.method == self.CREDIT:
            percent = Rule.objects.on_date(self.when_opened).credit_percent * decimal.Decimal(str(0.01))
        else:
            percent = Rule.objects.on_date(self.when_opened).deposit_percent * decimal.Decimal(str(0.01))
        result = 0
        current_date = self.when_opened
        while current_date <= today:
            dd = self.sum_total_transfers_on_date(current_date)
            result += dd
            if current_date < today:
                result *= 1 + percent
            current_date += timedelta(days=1)
        return result

    def sum_total_transfers_on_date(self, _date):
        result = 0
        result -= sum(x.total for x in self.cashing_set.on_date(_date))
        result -= sum(x.total for x in self.outcome_transfers.on_date(_date))
        result += sum(x.total for x in self.deposit_set.all().on_date(_date))
        result += sum(x.total for x in self.income_transfers.on_date(_date))
        return result


class MoneyTransfer(models.Model):
    sender = models.ForeignKey(BankAccount, related_name='outcome_transfers', verbose_name="Отправитель")
    receiver = models.ForeignKey(BankAccount, related_name='income_transfers', verbose_name="Получатель")
    when = models.DateTimeField(verbose_name="Время")
    total = models.DecimalField(max_digits=10, decimal_places=3, verbose_name="Сумма")
    comment = models.CharField(max_length=255, verbose_name="Комментарий")

    objects = TransferQuerySet.as_manager()


class Deposit(models.Model):
    account = models.ForeignKey(BankAccount)
    total = models.DecimalField(max_digits=10, decimal_places=3)
    banker = models.ForeignKey(Citizen)
    when = models.DateTimeField()

    objects = TransferQuerySet.as_manager()


class Cashing(models.Model):
    account = models.ForeignKey(BankAccount)
    total = models.DecimalField(max_digits=10, decimal_places=3)
    banker = models.ForeignKey(Citizen)
    when = models.DateTimeField()

    objects = TransferQuerySet.as_manager()

Answer 1

1. Use exists instead of count when trying to find if there are objects that match that filter.

   self.organization.owners.filter(id=citizen.id).exists()
   

2. Use generic foreign keys instead of multiple one-to-one relationships. Change organization and citizen to one field owner that is generic. There are lots of great examples on these but here is just one.

3. Why do you have Cashing, Deposit and MoneyTransfer as seperate models instead of one model Transaction with a type choice field? Then you can just do your math based on the type

   class Transaction(models.Model):
       DEPOSIT = 1
       CASHING = 2
       TRANSFER = 3
   
       CHOICES = (
           (DEPOSIT, 'Deposit'),
           (CASHING, 'Cashing'),
           (TRANSFER, 'Transfer')
       )
   
       account = models.ForeignKey(BankAccount)
       total = models.DecimalField(max_digits=10, decimal_places=3)
       banker = models.ForeignKey(Citizen)
       when = models.DateTimeField()
       type = models.IntegerField(choices=CHOICES)
   
       objects = TransferQuerySet.as_manager()
   

4. Implement a IsOwnerMixin that has a get_queryset method that does a filter on the objects where owner is equal to the users owner.

   def get_queryset(self):
        default_qs = super(IsOwnerMixin, self).get_queryset()
        qs = default_qs.filter(owner=self.request.user.citizen)
        return qs
   

   This of course is an overly simplified example. You will want to add logic to figure out if a user is apart of an organization and if so filter based on the organization id instead of the citizen id. Here is an example taken straight out of our code base that only returns objects that belong to a users `company'. This isn't all of the code but it is enough to get you started.

   class CompanyRelationMixin(object):
       company_path = "company"
       force_company_only = False
   
       def is_customer(self):
           return not self.request.user.is_superuser and self.request.user.user_profile.company
   
       def company_fk_allows_null(self, qs):
           ## Figures out which field on model relates to `company`, these details aren't important for your case
   
       def get_queryset(self):
           default_qs = super(CompanyRelationMixin, self).get_queryset()
           request = self.request
           options = ''
   
           if not self.force_company_only:
               allows_none = self.company_fk_allows_null(default_qs)
               if allows_none:
                   options = Q(**{self.company_path: None})
   
           if self.is_customer():
               company = request.user.user_profile.company.pk
               if options:
                    options |= Q(**{self.company_path: company})
               else:
                    options = Q(**{self.company_path: company})
   
           elif request.user.is_superuser:
                try:
                    request.QUERY_PARAMS['company']
                except KeyError:
                   return default_qs
                options = Q(**{self.company_path: request.QUERY_PARAMS['company']})
                options |= Q(**{self.company_path: None})
           if options:
               default_qs = default_qs.filter(options)
           return default_qs
   

   These are extremely useful as the amount of views you have start to grow. This remove a LOT of redundancy in your views.

I will add more later if I can think of anything else that will help DRY this up.

Answer 2

Your models

We've already covered most of these in your other question, but there are some changes that you've made that I might as well review.

Create a filter for getting bank accounts owned by a citizen

You already have querysets for filtering the Rule and Transfer objects, but there is logic in your BankAccount that can be grouped. Specifically, the logic for determining if someone is the owner of a bank account can be done in a query.

class BankQuerySet(models.QuerySet):

    def owned_by(self, citizen):
        from django.db.models import Q

        organizations_owned_by_citizen = Organization.objects.filter(
            owners__in=[citizen]
        ).distinct()

        return self.filter(
            (Q(owner_id=citizen.id) & Q(owner_ct=Citizen)) |
            (Q(owner_id__in=organizations_owned_by_citizen) & Q(owner_ct=Organization))
        )

Use a generic foreign key instead of two nullable foreign keys

Right now it's possible to create a BankAccount without a citizen or an organization. With a generic foreign key, the BankAccount will point to the owner (the organization or citizen) using one common set of fields, and this removes the need for your owner property. You will also be able to add other models as owners of the bank account in the future without making large changes to your code. This also reduces the logic needed in is_legal, which sounds like it might be better named is_owned_by_organization, but I may be missing something.

owner = GenericForeignKey(
    'object_id',
    'object_ct',
)
owner_id = models.PositiveIntegerField()
owner_ct = models.ForeignKey("contenttypes.ContentType")

def is_legal(self):
    return self.owner_ct is Organization

def is_owner(self, citizen):
    if self.owner is Citizen:
        return self.owner_id == citizen.id

    return self.owner.owners.filter(id=citizen.id).exists()

Use "0.01" instead of str(0.01) in your filters

This is a small one, but the point is that you shouldn't be computing values that you already know ahead of time. str(0.01) should be the same as 0.01, and it avoids the risk of your Decimal object not actually having the intended value of 0.01.

if self.method == self.CREDIT:
    percent = Rule.objects.on_date(self.when_opened).credit_percent * decimal.Decimal("0.01")
else:
    percent = Rule.objects.on_date(self.when_opened).deposit_percent * decimal.Decimal("0.01")

Use QuerySet.count() when you want the count and QuerySet.exists() when you want a boolean

Your logic for determining if someone is an owner of an organization returns the .count() of a queryset, even though the method is supposed to be returning a boolean. The .exists() method will make a lighter request to the database, which will only check to see if a single row exists instead of getting a count of all rows returned by the queryset.

Your views

These are the main point of your question, and there are also a few things that can be improved by them.

Limit your queryset instead of overriding get_object

Right now all of your views share the same get_object method which calls get_queryset and checks if the BankAccount returned is valid. You can remove the need for this by just not returning invalid objects from get_queryset, which will raise the 404 error that you are currently manually raising.

def get_queryset(self):
    return BankAccount.objects.owned_by(self.request.user.citizen)

Use a mixin for get_queryset/get_object

You are currently repeating your get_object method across all of your views, even though the logic is the same. Python supports mixins which allow you to group common code and use it across multiple classes, such as your get_object/get_queryset method. This way, if you ever need to make a change in the future you will only need to make it in a single place.

class BankObjectMixin(object):

    def get_queryset(self):
        return BankAccount.objects.owned_by(self.request.user.citizen)


class AccountDetail(BankObjectMixin, DetailView):

class SendTransfer(BankObjectMixin, SingleObjectMixin, FormView):

class OutcomeTransfers(BankObjectMixin, DetailView):

class IncomeTransfers(BankObjectMixin, DetailView):
    # ...

Avoid overriding dispatch unless you absolutely need to

Usually this is a sign that you are doing something wrong, in this case setting self.object for all methods. The change suggested for get_queryset will get around the issue you were likely facing, and you can instead just keep calling self.get_object() whenever you need the object that was requested.

BankAccount.objects.get(id=self.kwargs['pk'])

would become

self.get_object()