scottohara / loot

An implementation of some of the core MS Money features in Ruby on Rails
MIT License
4 stars 3 forks source link

Transaction total field for investment accounts is incorrect #192

Closed scottohara closed 2 years ago

scottohara commented 2 years ago

When viewing transactions for an investment account, the total column is displayed as:

<!-- Balance / Investment Amount -->
<td class="amount">
  <span ng-switch-default ng-class="{'text-danger': transaction.balance < 0}">{{transaction.balance | currency}}</span>
  <span ng-switch-when="investment" ng-if="::transaction.transaction_type == 'SecurityInvestment'">{{::transaction.quantity * transaction.price - transaction.commission | currency}}</span>
  <span ng-switch-when="investment" ng-if="::transaction.transaction_type == 'Dividend'">{{::transaction.amount | currency}}</span>
</td>

i.e.

The intention is to show the total transaction amount, but the calculation is incorrect for inflow transactions (e.g. Buy) because the commission should be added, while for outflow transactions (e.g. Sell) the commission should be subtracted, e.g.

image

In the above example, the correct totals for these Buy transactions should be:

Quantity Price Commission Total Formula
405 $123.32 $54.94 $49,999.54 qty * price + commission
535 $93.01 $54.74 $49,815.09 qty * price + commission

...and for completeness, a Sell transaction might be:

Quantity Price Commission Total Formula
405 $123.32 $54.94 $49,889.66 qty * price - commission

The JSON returned from the API already has an amount field that has the correctly calculated value:

{
  "openingBalance": 0,
  "transactions": [
    {
      ...
      "amount": "49999.54",   <---
      "quantity": "405.0",
      "commission": "54.94",
      "price": "123.32",
      "direction": "inflow",
      "memo": "405 @ $123.32 (plus $54.94 commission)",
      ...
    },
    {
      ...
      "amount": "49815.09",   <---
      "quantity": "535.0",
      "commission": "54.74",
      "price": "93.01000000000001",
      "direction": "inflow",
      "memo": "535 @ $93.01 (plus $54.74 commission)",
      ...
    }
  ],
  "atEnd": true
}

...so the questions to be answered are:

  1. Was there a reason not to use the pre-calculated amount, and recalculate (incorrectly) in the template?
  2. If we change this, will it have any impact on account reconciliation?
  3. If we change this, will it break any Cypress e2e tests?
  4. Does this have any impact on closing balances?

(note that the closing balance already doesn't match the sum of the totals, for either the correct or incorrect calculations, so there may also be some further investigation into how the closing balances for investment accounts is being calculated)

scottohara commented 2 years ago

Was there a reason not to use the pre-calculated amount, and recalculate (incorrectly) in the template?

Earliest revision of the template from 2015 has the calcs as per current, so it has always been like this.

Does this have any impact on closing balances?

Shouldn't, because it's only used for display. Closing balance is calculated on the backend, so changing the template won't have any impact.

scottohara commented 2 years ago

Also worth noting that if a SecurityInvestment transaction has no memo value, both /schedules/views/index.html and transactions/views/index.html will default the memo as follows:

<td class="memo" colspan="2">
  <span>{{::transaction.memo}}</span>
  <span ng-if="::(!transaction.memo) && transaction.transaction_type == 'SecurityInvestment'">{{::transaction.quantity | number}} @ ${{::transaction.price | number:3}} <span ng-if="::transaction.commission">(less {{::transaction.commission | currency}} commission)</span></span>
</td>

(hardcoded "less "...should be "plus " for an inflow (Buy).

We should fix this, and also add an additional Cypress test scenario for "Security Sell (no memo)" to match the existing "Security Buy (no memo)" test case.

scottohara commented 2 years ago

Does this have any impact on closing balances?

(note that the closing balance already doesn't match the sum of the totals, for either the correct or incorrect calculations, so there may also be some further investigation into how the closing balances for investment accounts is being calculated)

The closing balance for an investment account is not intended to be the sum of its transactions. Rather, the closing balance is calculated by determining the total quantities of each security held in that account, and multiplying each by the last known price for those securities (which itself is likely to be outdated, as the last known price is at at the time of the last buy/sell - not the current market).