Moving Account.amount to Account.credit & Account.debit (#133)

Large refactor as part of Hordak 2.0. The amount field is now split into two fields: `credit` and `debit`
---
* Adding migrations and early work on code changes

* Code changes to support PR: Experiment in moving Account.amount to Account.credit & Account.debit

* Test changes to support PR: Experiment in moving Account.amount to Account.credit & Account.debit

* Now supporting legacy `amount` argument to Leg, with deprecation warning

* Tests now pass for mysql

* Setting mysql error code correctly

* Adding tests and errors for invalid Leg credit/debit values

* Now storing creation migrations for debugging

* Now storing creation migrations for debugging

* Now storing creation migrations for debugging

* Now storing creation migrations for debugging

* Now storing creation migrations for debugging

* Requiring djmoney>3 due to migration creation problem with djmoney 3.0 adding two new currencies. This then causes migrations to be generated for currency fields with new default values.

* Formatting fix

* Fixing typo in deprecation warning

* Expanding out the changelog

* Making model choices available using the old syntax

* Fixing SQL error message

* Fixing postgres reverse migration issue

* Migrations now correctly set number of decimal places for decimal fields

* Fixing error in changelog, thanks to @nitsujri for catching it

* Including migration sql files in build

* GET_BALANCE() now sets sign correctly (postgres)

* GET_BALANCE() now sets sign correctly mysql & postgres)

* Updating orm-based balance calculations to calculate correct sign

* Fixes to balance calculations given updates to sum_to_balance(). Removed balance(raw=...).

* Renaming balance() to get_balance() (as per deprecation in 1.17.0)

* Renaming all uses of balance() to get_balance() (as per deprecation in 1.17.0)

* Updating admin to use the balance property now it is available

* Moving account_balance_after() and account_balance_before() to use database function get_balance()

* Moving account_balance_after() and account_balance_before() to use annotations

* get_balance() db function now uses leg_id rather than transcation ID (in case multiple legs in the same transaction point to the same account)

* Simplifying tests as it was a bit flaky between postgres and mysql

* Docs improvements

* Docs improvements

* Adding warning in docs re views

* Work on changelog

* Fixing error in credit/debit data migration, #133

.github/workflows/main.yml

@@ -83,11 +83,20 @@ jobs:
 
       - name: Check All Migrations Exist
         run: |
-          pip install "py-moneyed<3" --upgrade
-          PYTHONPATH=`pwd` ./manage.py makemigrations --check hordak
-          pip install "py-moneyed>=3" --upgrade  # Different version of py-moneyed should not create different migrations
           PYTHONPATH=`pwd` ./manage.py makemigrations --check hordak
 
+      - name: Create missing migrations
+        if: ${{ failure() }}
+        run: |
+          PYTHONPATH=`pwd` ./manage.py makemigrations hordak
+
+      - name: Archive created migrations for debugging
+        if: ${{ failure() }}
+        uses: actions/upload-artifact@v1
+        with:
+          name: created-migrations
+          path: hordak/migrations
+
       - name: Testing (PostgreSQL)
         run: |
           PYTHONPATH=`pwd` python -Wall -W error::DeprecationWarning -m coverage run ./manage.py test hordak

CHANGES.txt

@@ -1,13 +1,29 @@
 Version 2.0.0 (unreleased)
 ==========================
 
-* **Breaking**: New account currencies now default to ``DEFAULT_CURRENCY`` rather than all available currencies (trading accounts should be the only accounts with multiple currencies).
+.. note::
+
+    **Upgrade to Hordak 1.17 before upgrading to 2.0**
+
+    Hordak 1.17 will issue deprecation warnings for most issues you may encounter in the
+    2.0 upgrade. Fix those warnings and your upgrade should be smooth.
+
+* **Breaking**: ``transfer_to()`` was deprecated in 1.16. This old implementation has now been removed
+  and the newer ``transfer_to_accounting()`` has been renamed to take its place.
+  Existing uses of ``transfer_to_accounting()`` should be updated to
+  point to ``transfer_to()``. This change standardises the behaviour of this method on industry norms.
+* **Breaking**: ``Leg.account_balance_after()`` and ``Leg.account_balance_before()`` have been removed and replaced
+  with annotations which populate properties of the same name. Enable this annotations using
+  ``Leg.objects.with_account_balance_after()`` and ``Leg.objects.with_account_balance_before()``
+* **Breaking**: New account currencies now default to ``DEFAULT_CURRENCY`` rather than all available currencies
+  (trading accounts should be the only accounts with multiple currencies).
 * **Breaking**: Removed ``django-smalluuid``. UUIDs in URLs will now be rendered in the regular UUID format.
-* **Breaking**: Removed use of ``django-model-utils``. Model choices have the same values, but the data structures have changed to use the Django-native ``models.TextChoices``:
+* **Breaking**: Balance methods have be renamed to make way for (more performant) annotated properties.
+  Deprecation notices are issued by Hordak 1.17:
 
-  * ``Account.TYPES`` is now ``AccountType``
-  * ``TransactionCsvImportColumn.TO_FIELDS`` is now ``ToField``
-  * ``TransactionCsvImport.STATES`` is now ``TransactionCsvImportState``
+  * ``Account.balance()`` -> ``Account.get_balance()``
+  * ``Account.simple_balance()`` -> ``Account.get_simple_balance()``
+  * ``Transaction.balance()`` -> ``Transaction.get_balance()``
 
 * **Feature:** New accounting-oriented database view on the Legs table (keep your accountants happier). Adds columns
 
@@ -23,10 +39,25 @@ Version 2.0.0 (unreleased)
   * Shows the transaction amount (JSON list, one value per currency)
   * Shows credited/debited account IDs & names
 
-* **Feature:** Added ``GET_BALANCE(account_id: BIGINT, as_of: DATE = NULL)`` database function. Will get the balance of an account.
+* **Feature:** Added ``GET_BALANCE(account_id: BIGINT, as_of: DATE = NULL, as_of_leg_id: INT = NULL)`` database function. Will get the balance of an account.
+* **Feature**: Many balance calculations are now available as query annotations, and are performed in-database. This represents a
+  significant performance improvement. See the ``AccountQueryset`` and ``LegQueryset``.
 * **Enhancement:** Account code max length increased from 3 to 6
 * ``Balance.__eq__()`` now returns False rather than raising an exception if the other object is not a ``Balance``
 * Removed used of ``django-sql-utils``
+* Removed use of ``django-model-utils``. Model choices have the same values, but the data
+  structures have changed to use the Django-native ``models.TextChoices``. The old syntax will continue to work until Hordak 3.0:
+
+  * ``Account.TYPES`` is now ``AccountType``
+  * ``TransactionCsvImportColumn.TO_FIELDS`` is now ``ToField``
+  * ``TransactionCsvImport.STATES`` is now ``TransactionCsvImportState``
+
+Version 1.17.0 (unreleased)
+===========================
+
+* Deprecated Account.balance(), now renamed to Account.get_balance().
+  The `balance` property can be pre-populated in Hordak 2.0 using `Account.objects.with_balances()`.
+* Deprecated Transaction.balance(), now renamed to Transaction.get_balance().
 
 Version 1.16.0, Fri, 25 June 2024
 =================================

docs/.gitignore

@@ -1,2 +1,3 @@
 /_build_html
 /_build
+/html

docs/api/index.rst

@@ -7,6 +7,7 @@ API Documentation
     :maxdepth: 2
 
     models
+    models_statements
     views
     forms
     utilities_money

docs/api/models.rst

@@ -1,7 +1,7 @@
 .. _api_models:
 
-Models
-======
+Models (Core)
+=============
 
 .. contents::
 
@@ -13,6 +13,10 @@ Account
 .. autoclass:: hordak.models.Account
     :members:
 
+
+.. autoclass:: hordak.models.AccountQuerySet
+    :members:
+
 Transaction
 -----------
 
@@ -25,16 +29,7 @@ Leg
 .. autoclass:: hordak.models.Leg
     :members:
 
-StatementImport
----------------
-
-.. autoclass:: hordak.models.StatementImport
-    :members:
-
-StatementLine
--------------
-
-.. autoclass:: hordak.models.StatementLine
+.. autoclass:: hordak.models.LegQuerySet
     :members:
 
 

docs/api/models_statements.rst

@@ -0,0 +1,16 @@
+.. _api_models_statements:
+
+Models (Statements)
+===================
+
+StatementImport
+---------------
+
+.. autoclass:: hordak.models.StatementImport
+    :members:
+
+StatementLine
+-------------
+
+.. autoclass:: hordak.models.StatementLine
+    :members:

docs/conf.py

@@ -19,6 +19,7 @@
 #
 import os
 import sys
+from datetime import datetime
 
 
 # sys.path.insert(0, os.path.abspath('.'))
@@ -56,7 +57,7 @@
 
 # General information about the project.
 project = "Django Hordak"
-copyright = "2016, Adam Charnock"
+copyright = f"2016-{datetime.today().year}, Adam Charnock"
 author = "Adam Charnock"
 
 # The version info for the project you're documenting, acts as replacement for
@@ -73,7 +74,7 @@
 #
 # This is also used if you do content translation via gettext catalogs.
 # Usually you set "language" from the command line for these cases.
-language = None
+language = "en"
 
 # List of patterns, relative to source directory, that match files and
 # directories to ignore when looking for source files.

hordak/admin.py

@@ -1,5 +1,5 @@
 from django.contrib import admin
-from django.db.models import Q, Sum
+from django.db.models import Sum
 from django.utils.html import escape
 from django.utils.safestring import mark_safe
 from mptt.admin import MPTTModelAdmin
@@ -16,10 +16,10 @@ class AccountAdmin(MPTTModelAdmin):
         "code_",
         "type_",
         "currencies",
-        "balance_sum",
-        "income",
+        "balance",
+        "credits",
+        "debits",
     )
-    readonly_fields = ("balance",)
     raw_id_fields = ("parent",)
     search_fields = (
         "code",
@@ -28,31 +28,34 @@ class AccountAdmin(MPTTModelAdmin):
     )
     list_filter = ("type",)
 
-    @admin.display(ordering="balance_sum")
+    @admin.display(ordering="balance")
     def balance(self, obj):
-        return obj.balance()
+        if obj.balance is None:
+            return "-"
+        return obj.balance
 
-    @admin.display(ordering="balance_sum")
-    def balance_sum(self, obj):
-        if obj.balance_sum:
-            return -obj.balance_sum
-        return "-"
+    balance.admin_order_field = "balance"
 
-    balance_sum.admin_order_field = "balance_sum"
+    @admin.display(ordering="credits")
+    def credits(self, obj):
+        if obj.credits is None:
+            return "-"
+        return obj.credits
 
-    @admin.display(ordering="income")
-    def income(self, obj):
-        if obj.income:
-            return -obj.income
-        return "-"
+    @admin.display(ordering="debits")
+    def debits(self, obj):
+        if obj.debits is None:
+            return "-"
+        return obj.debits
 
     def get_queryset(self, *args, **kwargs):
         return (
             super()
             .get_queryset(*args, **kwargs)
+            .with_balances_orm()
             .annotate(
-                balance_sum=Sum("legs__amount"),
-                income=Sum("legs__amount", filter=Q(legs__amount__gt=0)),
+                credits=Sum("legs__credit"),
+                debits=Sum("legs__debit"),
             )
         )
 
@@ -186,6 +189,8 @@ def debit_(self, obj: models.LegView):
 
 
 def _fmt_admin_decimal(v):
+    if hasattr(v, "amount"):
+        v = v.amount
     if v is None:
         v = "-"
     else:

hordak/exceptions.py

@@ -85,3 +85,27 @@ class NoMoreAccountCodesAvailableInSequence(HordakError):
     """
 
     pass
+
+
+class InvalidOrMissingAccountTypeError(Exception):
+    """When an unexpected account type is encountered"""
+
+    pass
+
+
+class NeitherCreditNorDebitPresentError(Exception):
+    """When neither credit nor debit present for a Leg"""
+
+    pass
+
+
+class BothCreditAndDebitPresentError(Exception):
+    """When both credit and debit present for a Leg"""
+
+    pass
+
+
+class CreditOrDebitIsNegativeError(Exception):
+    """When either the credit and debit field of a Leg is negative"""
+
+    pass

hordak/forms/transactions.py

@@ -133,6 +133,16 @@ def clean_amount(self):
 
         return amount
 
+    def save(self, commit=True):
+        inst: Leg = super().save(commit=False)
+        if self.cleaned_data["amount"].amount > 0:
+            inst.credit = self.cleaned_data["amount"]
+        else:
+            inst.debit = abs(self.cleaned_data["amount"])
+
+        if commit:
+            inst.save()
+
 
 class BaseLegFormSet(BaseInlineFormSet):
     def __init__(self, **kwargs):
@@ -240,17 +250,17 @@ def save(self):
             description=self.cleaned_data.get("description")
         )
         Leg.objects.create(
-            transaction=transaction, account=source_account, amount=source_amount
+            transaction=transaction, account=source_account, credit=source_amount
         )
         Leg.objects.create(
-            transaction=transaction, account=trading_account, amount=-source_amount
+            transaction=transaction, account=trading_account, debit=source_amount
         )
         Leg.objects.create(
-            transaction=transaction, account=trading_account, amount=destination_amount
+            transaction=transaction, account=trading_account, credit=destination_amount
         )
         Leg.objects.create(
             transaction=transaction,
             account=destination_account,
-            amount=-destination_amount,
+            debit=destination_amount,
         )
         return transaction

hordak/management/commands/create_benchmark_transactions.py

@@ -51,18 +51,20 @@ def handle(self, *args, **options):
         print("Done")
         print("")
 
-        print(f"Bank:         {str(bank.balance()):<15}  {str(bank.legs.count()):<15}")
         print(
-            f"Assets:       {str(assets.balance()):<15}  {str(assets.legs.count()):<15}"
+            f"Bank:         {str(bank.get_balance()):<15}  {str(bank.legs.count()):<15}"
         )
         print(
-            f"Expenses:     {str(expenses.balance()):<15}  {str(expenses.legs.count()):<15}"
+            f"Assets:       {str(assets.get_balance()):<15}  {str(assets.legs.count()):<15}"
         )
         print(
-            f"Liabilities:  {str(liabilities.balance()):<15}  {str(liabilities.legs.count()):<15}"
+            f"Expenses:     {str(expenses.get_balance()):<15}  {str(expenses.legs.count()):<15}"
         )
         print(
-            f"Capital:      {str(capital.balance()):<15}  {str(capital.legs.count()):<15}"
+            f"Liabilities:  {str(liabilities.get_balance()):<15}  {str(liabilities.legs.count()):<15}"
+        )
+        print(
+            f"Capital:      {str(capital.get_balance()):<15}  {str(capital.legs.count()):<15}"
         )
 
 
@@ -101,6 +103,6 @@ def _transfer_no_commit(
 
     transaction = Transaction()
     legs = []
-    legs.append(Leg(transaction=transaction, account=debit, amount=-amount))
-    legs.append(Leg(transaction=transaction, account=credit, amount=amount))
+    legs.append(Leg(transaction=transaction, account=debit, debit=amount))
+    legs.append(Leg(transaction=transaction, account=credit, credit=amount))
     return transaction, legs