Change of architecture :)

[lie-nielsen]

Update from @adamcharnock: Implementation now in #133

---

I know it would be a huge re-factor but using a credit and debit field on each leg rather than a signed amount would ease your problems with signs. The accounting module of the Tryton project uses this principle. Instead of transaction/leg they use the terminology move/line.
https://github.com/tryton/account/blob/1b182f260d52ea7767547ff688ee94e24f29092a/move.py#L562

Thanks for this module! I am using it in a payroll app.

[adamcharnock]

Hi @lie-nielsen, sorry for the delay in replying!

You could well be right. From my point of view, I was coming to this from a strong engineering and software background, and it seemed very odd to me that the meaning of debit and credit would change depending on the account type. Using signs made the engineer in me much happier (see Double Entry Accounting for Developers in the docs).

That being said, I came to this with minimal accountancy experience. I'm very willing to accept that there are good reasons why Hordak's implementation is less than ideal, but I'd just need those reasons spelling out to me.

I'd be happy to see a change of architecture in Hordak 2.0, and I'd also be happy for that process to involve adding an additional maintainer to the project (as per #43).

[adamcharnock]

Also, for the record, I'd like to see the 'account code' field ditched in Hordak 2 (#40, #41), it causes a lot of hassle and I don't think it gets much use :-)

[rajeshr188]

adam im not sure if this project is still maintained or not but if you are looking for concrete implementation of double entry acounting for hordak 2.0 you should have look at this stackoferflow answer https://stackoverflow.com/questions/59432964/relational-data-model-for-double-entry-accounting/59465148#59465148

[nitsujri]

adam im not sure if this project is still maintained or not but if you are looking for concrete implementation of double entry acounting for hordak 2.0 you should have look at this stackoferflow answer https://stackoverflow.com/questions/59432964/relational-data-model-for-double-entry-accounting/59465148#59465148

I'm getting pretty deep into hordak and ledgers in general.

@rajeshr188 thanks for linking that stack post, I learned a lot from it. If you follow the comments, it seems like there was a pretty heated battle - lots of ego, but I think there were enough corrections to the answer that it makes a lot of sense in the end. In particular the Data Model diagram is a great starting place.

Benefits of Dr/Cr vs +/-

• T accounts are SUPER easy to build
• No specialized knowledge needed to use DB data to build external dashboards (i.e. metabase, powerbi).

Drawbacks

• Account Balance is as simple as SUM(...) in +/-. Would need a more complex-function/sql-query. (I imagine why @adamcharnock built it like this initially)

I'm sure there's more to add, but I think the biggest reason to do a rewrite + change is the fact that we can hand any accountant/auditor that knows SQL and they can put together the audit. Same with anyone building a Metabase dashboard, otherwise you need to learn how Hordak inserted the data and your queries become specialized.

FWIW - Hordak's data model is quite good and assumes very little of the use case. It actually covers the StackOverflow post image quite well (arguably better).

I blurred out the sections that don't apply, but if you were to map the equivlent objects it would be:

• Ledger == Account
• LedgerTransaction == Leg
• AccountTransaction == Transaction

Hordak can already do >2 Legged transactions.

In the StackOverflow's post about Account which I'll call SO_Account, I argue that Hordak shouldn't care and remain a general library that doesn't carry specific use cases.

[rajeshr188]

@nitsujri can i have look at your implementation of @performancedba s stackoverflow answer please?

[nitsujri]

So I'm pretty new to Django and Hordak, accounting in general, so I'm definitely open to feedback on our data models.

For us, we use Hordak directly and almost as is. So to do the "External" part of @performancedba SO_Post and make it useful to our business case we:

1. Have "org objects" have accounts, Partners Merchants -> various accounts
2. Have an TransactionSource join object/table.

class Partner(TimeStampedModel):
    account_assets = models.ForeignKey(Account...)

   def setupAccounts(self):
      self.account_assets = Account.objects.create(type=Account.TYPES.asset, name..., currencies...)

      ...other accounts

Above here is pretty boring, I think everyone is doing this.

class TransactionSource(TimeStampedModel):
    transaction = models.ForeignKey(Transaction...)

    payment = models.ForeignKey(Payment..., null=True, blank=True)
    some_other_money =  models.ForeignKey(Payment..., null=True, blank=True)

    SOURCE_NAMES = {
        "payment": "Payment",
        "some_other_money": "Some Other Money",
    }

    @property
    def source(self):
        for source_key in self.SOURCE_NAMES.keys():
            source = getattr(self, source_key)
            if source:
                return source

    @property
    def source_display_name(self):
        for source_key in self.SOURCE_NAMES.keys():
            source = getattr(self, source_key)
            if source:
                return self.SOURCE_NAMES[source_key]

So the most interesting question is "Why did Money move?" and if you view @performancedba's model diagram it's very suited for a Bank. It's not great at things like Ecommerce where the "AccountTransaction" should point to say an Invoice/Order.

To answer "Why did Money move?" we are using a hard link via TransactionSource which is a form of Polymorphism but with hard links via multiple columns (makes the RMDBS gods happy). Now, building richer descriptions and auditing can be easier.

Every time we move money we have to do a bit more code:

invoice_payment = InvoicePayment...

hordak_trxn = partner.assets_cash.transfer_to(
    partner.assets_expense,
    invoice_payment.amount,
    description=_("Payment for Widget"),
)
audit = TransactionSource(transaction=hordak_trxn, payment=invoice_payment)
audit.full_clean()
audit.save()

So now I can query Transaction.objects.all().first().transaction_source.source and get back what exactly caused this transaction to happen.

Again, I'm quite new to this, so would love feedback.

---

Monkey Patches

Nothing special here, just making things easier for us to view/debug.

from hordak.models import Account, Leg, Transaction
from hordak.utilities.currency import Balance


def trxn_new_str_rep(self):
    return f"Transaction#{self.id}: {self.description[:15]} {self.timestamp.strftime('%d-%m-%Y')}"


Transaction.__str__ = trxn_new_str_rep


def in_currency(self, currency):
    return self.normalise(currency).monies()[0]


Balance.in_currency = in_currency


def get_transactions(self):
    account_ids = self.get_descendants(include_self=True).values_list("id", flat=True)

    return Transaction.objects.filter(legs__account_id__in=account_ids).order_by(
        "-timestamp"
    )


Account.get_transactions = get_transactions


def leg_new_str_rep(self):
    return f"Leg#{self.id}: {self.amount}; {self.account.name}; {self.transaction.description[:15]}"


Leg.__str__ = leg_new_str_rep

[adamcharnock]

Thank you for all the well thought-through and constructive comments on this. As it stands right now – this project is currently maintained by @PetrDlouhy, which I appreciate greatly. I sometimes come by and leave high-level non-mandatory comments, like now.

---

Cr/Dr

I can see that being able to view Hordak's internels through the lens of Dr/Cr would be helpful in some cases. In particular when interacting specifically with the database. This is the traditional view and sometimes it would be great to be able to access that via SQL (either because it makes more sense to trained-humans, or because writing queries based on Dr/Cr can make more sense sometimes)

Changing architecture is a bad idea

My reasons are entirely due to the reality of the project:

1. Many people have already built complex systems atop Hordak and this would certainly be a breaking change. I would expect these existing systems to require signifiant code changes. This would split our user base into Hordak v1 and the new Hordak v2 (think the hellish Python 2/3 transition). Except in our case I think (IMHO) some/many people wouldn't make the transition.
2. As a result we would need to split resources between supporting Hordak v1/v2. We just don't have the resources for this. And even if we did, I don't think this would be the right way to spend them.

The best of both worlds

But! I really can see that there is a demand for this. So my suggestion is that we create one or more views in the database. These views allow users to access the underlying +/- tables in a Dr/Cr format.

This way data is available in the format that is being asked for, but with minimal actual development overhead and no breaking changes.

We already keep a lot of logic in the database (which I feel good about), so I don't think anything will be lost by adding whatever views are needed.

Thoughts welcome!

[rajeshr188]

@nitsujri I'm oretty sure why money moved is or can be derived from xacttypecode_ext inperformancedba schema

[rajeshr188]

@nitsujri i ll be happy to show my implementation if you would like to review and criticize , I'm planning to publish it as django-dea

[nitsujri]

@nitsujri i ll be happy to show my implementation if you would like to review and criticize , I'm planning to publish it as django-dea

Would love to take a look. Tag in your repo once you've released so we can move the discussion over there. This thread isn't really the best place for that.

[nitsujri]

@adamcharnock thanks for the feedback and definitely agree the full rewrite is not necessary.

Similar to the view, but one step closer, I propose adding 2 extra columns:

• accounting_amount - moneyed; positive only
• accounting_type - DR/CR

Method Changes

• Leg#save - pre-fills accounting_amount, accounting_type columns if empty.
• Account#accounting_transfer_to - fills new accounting columns

Method Additions

• Leg#is_accounting_debit
• Leg#is_accounting_credit
• Transaction.multi_leg_transfer - i.e. multi_leg_transfer(from=[(account1, amount), (account2, amount2)], to=[(account3, amount3), (account4, amount4), (account5, amount5)] # returns Transaction
  • Helper function to set DR/CR.
  • Expand DB check_leg to check new colunn amount+amount2 == amount3+amount4+amount5

To ease migration path, releases are incremental over years:

• 1 minor version - columns allowed empty, only get filled with data via accounting_transfer_to
• +1 minor version
  • add data migration to backfill Legs with empty accounting_amount/type.
  • migration to set null=False for 2 new columns
• 2.0 version
  • Change balance() to use account_ columns
  • Update html templates to Accounting Debit/Credit
  • raise error on transfer_to, is_credit, is_debit
  • Maintain backward compat by converting Leg#save to backfill Leg.amount if empty

My Thoughts

While I understand it's not as easy/clean of a solution compared to a view, I'm motivated to help migrate this for 2 reasons:

• Closes the gap Hordak method vs accounting method.
• I needed to debug a problem with our accountant and while a view would help, I spent quite a bit of time translating our accountant's fix into the current data-model having to do multiple back-and-forths to ensure the correction and future code was applied correctly.

Thoughts/Corrections?

[adamcharnock]

Hi @nitsujri – My apologies for going quiet on this for so long. I'm now using hordak in a client project which is giving me some time to spend on it.

This sounds like a good plan to me. I'll take a look over your draft PR.

[adamcharnock]

After refreshing my memory on how Hordak works, I'm a now a little less convinced that this is required. I'm going to see how this conversation pans out.

[adamcharnock]

Ok, I've moved the discussion of the database view to another issue as that feels concrete and ready to implement.

Discussion of the other points can continue here.

EDIT: I realise @nitsujri was pitching for a non-view related solution, but personally I am leaning away from that still. I'm going to keep planning for 2.0, and thinking about the other points raised in this issue. After all that I'll see where my thoughts stand.

[adamcharnock]

Method changes

• Leg#save - pre-fills accounting_amount, accounting_type columns if empty.
• Account#accounting_transfer_to - fills new accounting columns

If we were to implement this then I feel fairly strongly that these should be implemented in database triggers

Leg#is_accounting_debit
Leg#is_accounting_credit

After looking into the codebase I think is_debit and is_credit are giving results correctly and as expected already, so I don't think these methods are required.

Overall comment: I find I open this issue with some trepidation these days as there is quite a lot of content here and I feel I cannot never quite get my head around all of it (at least not in the time I have available). I'm very happy for these architectural changes to be discussed, but at some point it would really help me if some smaller and more actionable issues could be spun-out.

If this issue remains quiet for a while then I may take it upon myself to close it. But feel free to comment and open it again or – even better – create a fresh issue with a fresh pitch. Other maintainers are of course also welcome to pick this up should they wish.

[nitsujri]

If we were to implement this then I feel fairly strongly that these should be implemented in database triggers

Definitely works, though I don't see a clear benefit in pushing it to triggers since it's not a check nor process that directly relies on other columns.

After looking into the codebase I think is_debit and is_credit are giving results correctly and as expected already, so I don't think these methods are required.

You're right. I tested it with specs it comes up correctly in the 4 key scenarios (L->L, L->R, R->L, R->R). A while ago I thought it was wrong, but was mistaken.

We added these to rely on the new accounting_type DR/CR column, which isn't necessary with a view.

---

We've been maintaining our own fork for almost a year in production without issues that does the above in DB columns and accounting_ methods and once we got the columns in, debugging with our accountant(s) became significantly simpler.

While a view cam be helpful, it does a few things:

• Requires a mental translation between the view and the underlying data during debugging.
  • For the developer, this means pos&neg continue to have different meanings at different levels. At the data storage level it's CREDIT/DEBIT and at the user display level, it gets translated to add/subtract.
• New features with deep dependancies on current data structures.

For us the benefits of matching the accounting process really show up because we do loans, fees, contra accounts and future payables. The complexity of debugging the money movement because significantly easier when the data matched our accountant's expectations.

[adamcharnock]

Hi @nitsujri, thank you for the reply and for the added context! Just so that I have it straight in my head it sounds like we want to do one of these (just phrasing things simply for now):

Option 1

Convert the signed amount field into an absolute amount field plus a type field (values: DR or CR)

Option 2

Convert the signed amount field into two absolute fields: amount_debit and amount_credit (PR: #133)

---

Either option seems fairly achievable, although admittedly I haven't had my morning tea yet.

[nitsujri]

Ah incredible, yes Option 2 is 100% viable. It's a pretty common architecture. Happy to see this PR and would be glad to migrate our current data.

Any PR feedback I'll leave directly on it.

[adamcharnock]

I'll close this as the PR was merged some time ago.

I'm happy how well this change was received!