sdementen / piecash

Pythonic interface to GnuCash SQL documents
Other
289 stars 74 forks source link

Distinguishing debits vs credits for a split #105

Closed ebridges closed 5 years ago

ebridges commented 5 years ago

It would be helpful to have a convenience method on a Split that can be used to determine if a given split is either a Debit or a Credit.

Given the account type that the split is affecting and whether the split is increasing or decreasing its balance, this method would encapsulate the logic[1] of figuring out whether it's a Debit or a Credit.

Example usage: if split.is_credit: ...

[1]: See table here: https://en.wikipedia.org/wiki/Debits_and_credits#Aspects_of_transactions

alensiljak commented 5 years ago

Is there a problem with how it works currently - with numbers < 0 being withdrawals and numbers > 0 deposits? This is the way things worked for centuries, if I'm not mistaken. Another thing, since you are referring to multiple account types, which numbers are you referring to? I.e. share transactions may contain both the money (value) and shares (quantity). Usually, if I'm depositing shares, I'm withdrawing cash, but normally not to/from the same account. In brief, the way the properties currently work seem quite simple and logical to me. Perhaps, if you would describe the real problem you are experiencing, we might share some insights or experiences that may or may not help in that specific case.

ebridges commented 5 years ago

There's no issue, this is a feature request to encapsulate which types of accounts are, say, a liability or some other type of account and whether a split is an increase or a decrease on that account.

I could easily implement the logic laid out in the table linked in the original issue, but I believe the mapping of GnuCash account types to the standard set of types in that table would be better encapsulated in Piecash. This is standard practice in the accounting world, and any client of Piecash would have to needlessly replicate that.

One example is in a reporting package that I maintain. This is a budget report that I built around the idea of envelope-style budgeting where the sum of debits to the budget accounts is the budgeted amount and the sum of credits is the actuals spent against the budget.

As it currently stands, I have to make an implicit assumption that the accounts I query for budgeting are Liability accounts and I need to check that the value is less than zero to get the budgeted amount. It would be simpler client logic to query (for example) debits without having to check the sign of the value and the account type (as well as maintaining a mapping of GC account types to GAAP account types).

I'd also suggest that the assumption that "numbers < 0 [are] withdrawals and numbers > 0 deposits" is not accurate, since splits are not removing or adding money -- they're balancing entries across multiple accounts. For this reason, I'd assert that the concept of "debits" and "credits" is more robust. There are a bunch of examples in the article above on envelope accounting that illustrate this. I'd be glad to discuss those here, just not sure how much detail you're looking for.

By "account types" I'm referring to these: https://piecash.readthedocs.io/en/master/_modules/piecash/core/account.html#AccountType

ebridges commented 5 years ago

There's also some discussion on gitter about this request: https://gitter.im/sdementen/piecash?at=5ca12d530aad63501907d25e

sdementen commented 5 years ago

Would the rule be something like


if split.account.type in positive_types:
    is_credit = split.value > 0
else:
    is_credit = split.value < 0

@edward

could you test this logic (maybe the is_credit rule should be the opposite) ? And if it is ok, propose a PR with the change? Should we also add a property at account level that would tell if the account is on the left or right of the balance sheet? Based on the sets positive_types/negative_types ? At split level, should we have a property named type that would return CREDIT or DEBIT (which would be values of an enum like the AccountType)?

On Fri, Apr 5, 2019, 15:25 Edward Q. Bridges notifications@github.com wrote:

There's also some discussion on gitter about this request: https://gitter.im/sdementen/piecash?at=5ca12d530aad63501907d25e

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sdementen/piecash/issues/105#issuecomment-480274163, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPpdo1qzLK4EX74aJ36yQd_W7UcJuDqks5vd068gaJpZM4ccSmF .

sdementen commented 5 years ago

Sorry for the bad layout ! (editing from a mobile)

sdementen commented 5 years ago

Any link with issue #89 ?

ebridges commented 5 years ago

I'm not sure I understand what underlies issue #89 -- doesn't GnuCash constrain one from creating child accounts based on the type of the parent? That seems like it should prevent an issue as is described there...or maybe I'm misunderstanding?

On a side note, I will endeavor to make a PR for the change I'm requesting...just have to wrap up some other stuff on my side.

ebridges commented 5 years ago

Here's what I'd propose as a mapping from the different GnuCash Account types to the different categories of account. I'd appreciate any feedback.

GnuCash Account Type Kind of Account Debit Credit
RECEIVABLE Asset Increase Decrease
CASH Asset Increase Decrease
ASSET Asset Increase Decrease
BANK Asset Increase Decrease
EXPENSE Expense Increase Decrease
MUTUAL Equity Decrease Increase
STOCK Equity Decrease Increase
TRADING Equity Decrease Increase
EQUITY Equity Decrease Increase
INCOME Income Decrease Increase
CREDIT Liability Decrease Increase
LIABILITY Liability Decrease Increase
PAYABLE Liability Decrease Increase
sdementen commented 5 years ago

After some investigation, I wonder if the rule is not simply "if the split has a value < 0, it is a credit (otherwise it is a debit)". Could you @ebridges confirm this ? If so, the properties is_credit/is_debit would simply be

    @property
    def is_credit(self):
        return self.value < 0

    @property
    def is_debit(self):
        return self.value > 0

Regarding the table above, the mapping already exist in the sets positive_types and negative_types in the piecash.core.account.py file. What would you like to have more ?

It is probably more the get_balance that could benefit from a reversion of sign for the negative_types as it is done by gnucash through the option "reverse balance accounts" image

sdementen commented 5 years ago

An example of the use of the added field is_credit can be seen in the documentation of the branch https://piecash.readthedocs.io/en/105_credit_debit_property/tutorial/index_existing.html#transactions-and-splits

sdementen commented 5 years ago

I have added a "natural_sign" argument (default to True) to the account.get_balance function. You also have the account.sign field that returns 1 or -1 in function of the sign of the balance of the account (-1 for accounts of negative types)

ebridges commented 5 years ago

Hi @sdementen -- simply checking the sign is not sufficient as the cardinality is dependent on the type of account the split is affecting. This is due to the relationship established by the accounting equation (assets = liabilities + equity), and the need for the equation to balance across splits for a given transaction.

Something like this would likely be more appropriate:

    def is_debit(self):
        if self.account.type in positive_types:
            return True if self.value > 0 else False
        if self.account.type in negative_types:
            return True if self.value < 0 else False
        return False

    def is_credit(self):
        if self.account.type in positive_types:
            return True if self.value < 0 else False
        if self.account.type in negative_types:
            return True if self.value > 0 else False
        return False

    def get_debit_credit(self):
        if self.is_debit():
            return LedgerEntryType.debit
        if self.is_credit():
            return LedgerEntryType.credit
        return None
sdementen commented 5 years ago

This is what I tried first but it wasn't given correct results with respect to what I was seeing in gnucash when asking to show the credit/debit view instead of the default view (vs what piecash was giving with my code) Would the code you propose work on your sample gnucash book? If so, could you post a minimal version of a gnucash book (one with a couple of accounts of the different types and a couple of transactions)?

ebridges commented 5 years ago

I've not gotten as far as trying it on an actual gnucash account -- was still in the theoretical phase LoL.

I'll give that a go and see what I find.

sdementen commented 5 years ago

@ebridges, you can have a look at the doc on https://piecash.readthedocs.io/en/105_credit_debit_property/tutorial/index_existing.html#accounts to see the result of get_balance (with the sign reversal) and on https://piecash.readthedocs.io/en/105_credit_debit_property/tutorial/index_existing.html#transactions-and-splits to see the result of the credit/debit flag

ebridges commented 5 years ago

if you want to go ahead with that, it looks good to me! thanks for jumping on it