Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculated income excludes off-budget to on-budget transactions #1007

Closed
ekeih opened this issue Apr 2, 2024 · 3 comments · Fixed by #1023
Closed

Calculated income excludes off-budget to on-budget transactions #1007

ekeih opened this issue Apr 2, 2024 · 3 comments · Fixed by #1023
Labels
bug Something isn't working as supposed to

Comments

@ekeih
Copy link
Contributor

ekeih commented Apr 2, 2024

Describe the bug

  • Querying /v4/months returns an attribute .income which is documented in the source as The total income for the month (sum of all incoming transactions without an Envelope).
  • When transferring money from an off-budget account to an on-budget account it is possible to select an envelope, but not mandatory.
    • When selecting an envelope the transaction is not considered income, which seems correct because it is not possible allocate it afterwards.
    • I think when no envelope is selected the transaction should be considered income because it is money that is now possible to be allocated (which it wasn't while it was in an off-budget account). This is also noticeable because it is only included in .available if it is not transferred to an envelope.
  • I think this happens because of
    Where("source_account.external = 1").
  • At first glance I thought it could be resolved by changing this line to Where("source_account.external = 1 OR source_account.on_budget = 0").. But after thinking about it a bit more I think the query needs to be more specific. Because there are also cases like transferring off-budget to off-budget to consider and maybe more.
  • Looking at the implementation of the INCOMING direction filter for transactions in
    if filter.Direction == DirectionIncoming {
    // Incoming is off-budget (external accounts are enforced to be off-budget) to on-budget accounts
    q = q.
    Joins("JOIN accounts AS accounts_source on accounts_source.id = transactions.source_account_id").
    Joins("JOIN accounts AS accounts_destination on accounts_destination.id = transactions.destination_account_id").
    Where("accounts_source.on_budget = false AND accounts_destination.on_budget = true")
    }
    I think Where("source_account.external = 1"). in budget.go should be Where("accounts_source.on_budget = true AND accounts_destination.on_budget = false"). instead.
  • (A bit off-topic, but I actually wonder if the INCOMING direction filter should exclude transactions that are allocated to an envelope right away?)

To Reproduce

  1. Create an off-budget account and an on-budget account.
  2. Check the .income attribute of /api/v4/months of the budget.
  3. Add a transaction transferring money from the off-budget to the on-budget account.
  4. Check .income again. It is the same as in step 2, but I believe it should have increased by the transferred amount.

Expected behavior
Off-budget to on-budget transactions without an envelope are included in the .income calculation of v4/months.

Additional context

  • Currently, this has little to no impact because the official frontend never accesses .income as far as I can see.
  • I came across it while working on the statistics implementation where I access .income in the frontend. I could work around this by quering transactions (filtered for direction=INCOMING and envelope=) and adding their amounts. But after thinking about this for an hour and looking at the backend source I think this is a bug in the backend.
@ekeih ekeih added the bug Something isn't working as supposed to label Apr 2, 2024
@morremeyer
Copy link
Member

Thanks for this detailed report.

Bug

You are indeed correct, this is a bug. Transactions from off-budget to on-budget accounts without an Envelope need to be considered for .income of the /v4/months endpoint.

Transaction filtering

The transaction filtering again has a different understanding, which I have come to consider wrong, too.

I thought about this for a while and arrived at the conclusion that we'll need two different filters, one direction filter and one type filter.

direction: purely for internal/external account considerations

Has the following filter values

  • incoming: Transaction from an external to an internal account
  • outgoing: Transaction from an internal account to an external account
  • internal: Transaction from an internal to an internal account

type: For the effect a transaction has on the budget

Has the following filter values

  • income: From an off-budget to an on-budget account
  • spend: From an on-budget to an off-budget account
  • transfer: From an on-budget to an on-budget account

I'm not yet happy with incoming and income being named this similarly. Any suggestions to improve this are very welcome.

@morremeyer
Copy link
Member

I think Where("source_account.external = 1"). in budget.go should be Where("accounts_source.on_budget = true AND accounts_destination.on_budget = false"). instead.

@ekeih I think you meant that it should be

Where("source_account.on_budget = false AND destination_account.on_budget = true").

instead, right? You had the true and false condititons the other way around.
I used the updated version in #1023, which resolves this issue.

I moved the changes to transaction filtering to #1024 to handle them separately.

@ekeih
Copy link
Contributor Author

ekeih commented Apr 27, 2024

Oh, yes you are right, I mixed that up 😬 Thanks for the fix! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as supposed to
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants