search

nadineproject / nadine

Open Coworking Software
http://nadineproject.org
GNU Affero General Public License v3.0
163 stars 78 forks source link

Improve outstanding bill report performance and include 'total applied tax' #398

Closed tnightingale closed 6 years ago

tnightingale commented 6 years ago

This branch/pull request also includes the work in the payments branch/pull request and has been published as a means of reviewing a partial solution to the outstanding bills problem.

This adds a tax_amount property to BillLineItem which gets cached whenever calculate_tax() gets called. It breaks the rules a little bit by causing a database write when calculate_tax() is called but the idea is that it is kept up-to-date.

I got stuck when I attempted to include the tax_amount value in the outstanding_bills() query and it's subsequent views (I hadn't realized that outstanding_bills() seeds 3 filtered views and I got lost trying to figure out exactly what each view was used for).

@jsayles I'd like to review this with you to determine how we can get this to a point where it is useful to you.

tnightingale commented 6 years ago

@jsayles I'm pretty confident I have the alternative Outstanding Bills query sufficiently working with cached UserBill values. It adds 4 cache columns _(cached_total_amount, cached_total_tax_amount, cached_total_owed and cached_total_paid)_ to the UserBilll model. They're kept up-to-date in two locations:

  1. After completion of a billing batch; the batch keeps a list of all of the UserBill objects it encountered. Just after closing the batch the process now iterates over the relevant bills and calculates & saves the cached_total_* columns.
  2. Whenever a Payment object is updated or deleted (via Django's models Signals); the bill associated to the payment has its cached_total_* columns recalculated.

The Outstanding Bills query itself is now just:

def outstanding(self):
    return self.filter(mark_paid=False, cached_total_owed__gt = 0)\
        .annotate(bill_total=F('cached_total_amount') + F('cached_total_tax_amount'))

I have done my best to update the user-facing totals to include applied taxes (changing bill.amount to bill.total) wherever I can. It's possible that I have still missed some, we will likely need to clean any stragglers up as we go.

There is a fair amount of noise in the commit summary & files changed pages of this pull as it is based on the other payment work I have done. You can view just the changes in this pull request here: https://github.com/nadineproject/nadine/compare/2969b4c3c37e681322d3931f57bbdfa4d53e027d...5d2c6f55c7ac1d01daab1e121c568efd41d982b5