adamcharnock / django-hordak

Double entry accounting in Django
http://django-hordak.readthedocs.io
MIT License
239 stars 55 forks source link

Improve logic in Account.trasfer_to() #26

Closed adamcharnock closed 4 months ago

adamcharnock commented 6 years ago

the logic in transfer_to() is somewhat unclear. It is trying to paper over complexities in double entry accounting in a intuitive way and only partly succeeds in doing so.

There are a number of tests which check the functionality of transfer_to(). I'd be happy to accept changes to transfer_to() which update the tests to show intuitive functionality.

Otherwise I will look into this when I get around to it.

nitsujri commented 1 year ago

I ran into a transfer_to problem that I originally thought would be simply fixed by https://github.com/adamcharnock/django-hordak/pull/81, but diving deeper I realized there might be deeper logic problem, including my undrstanding.

In particular the defining comment from core.py:

# Transfers from liability -> asset accounts should reduce both.
# For example, moving money from Rent Payable (liability) to your Rent (expense) account
# should use the funds you've built up in the liability account to pay off the expense account.

My understanding is - moving money from a liability => asset should increase both.

The problem with the comment from core.py is assuming that Rent (expense) account should decrease when rent is paid. It's the opposite - expense should increase. For this exact scenario accountingtools.com link has this image:

T-Accounts

To make the image above easier to tease apart in terms of Hordak:

To match the image above I think the spec should be:

    def test_pay_rent_via_invoice(self):
        cash = self.account(type=Account.TYPES.asset)
        expense = self.account(type=Account.TYPES.expense)
        payable = self.account(type=Account.TYPES.liability)

        # Jul 1 - Landloard sends Invoice for Rent, $1000
        # We can't pay immediately, so we book it to a Payable (pay later), i.e. Loan
        payable.transfer_to(expense, Money(1000, "EUR"))
        self.assertEqual(expense.balance(), Balance(1000, "EUR"))
        self.assertEqual(payable.balance(), Balance(1000, "EUR"))

        # Jul 6 - We pay the landlord, $1000
        cash.transfer_to(payable, Money(1000, "EUR"))
        self.assertEqual(cash.balance(), Balance(-1000, "EUR"))
        self.assertEqual(payable.balance(), Balance(0, "EUR"))
        self.assertEqual(expense.balance(), Balance(1000, "EUR"))

Other specs should change as well, test_transfer_liability_to_expense and test_transfer_expense_to_liability.

Would really appreciate comments from @PetrDlouhy and @adamcharnock.

[Edit] One thing I think is telling us there is a fundamental problem are these two tests:

Both of those tests expect the balance to +$100, but that's creating free money?

nitsujri commented 1 year ago

I made a PR to match the above logic, but I'm scared of it so invite lots of feedback.

adamcharnock commented 4 months ago

Closing this issue since @nitsujri's PR was merged.