Add start balance to manual accounts #735

2024-05-11T03:54:43+08:00

jakubkottnauer commented

2024-05-11 03:54:43 +08:00

(Migrated from github.com)

This PR makes balance of manual accounts be automatically updated whenever a new valuation or a transaction is added to that account. In this first iteration, the solution "hijacks" the existing sync flow by first computing the correct current balance which is then treated as the "source of truth" in the rest of the sync (as if it was an account connected to a 3rd party provider).

Current balance of a manual account is calculated as follows:

take the latest valuation's value, or start_balance if no valuation exists
get the sum of all transactions that occurred between the valuation (or all transactions if no valuation exists) and today
-> sum of these two numbers gives you the current balance

Changes

A new accounts.start_balance column has been added. I have considered making the migration populate the column for all accounts (as we only have manual accounts at the time of running the migration) but then to be consistent I would also have to change all of the seed data to create manual accounts, which doesn't sound like something we'd like to do. Happy to discuss this further.

All newly created accounts will be manual by default as we don't have support for connected accounts yet.

Next steps

Start balance of a manual account should be made editable (are there designs for an account edit dialog yet?)
Valuations and transactions can have their date set in the future. This means that accounts need to be synced periodically to make sure the balance is up to date every day the user opens Maybe. Somewhat similar to the issue https://github.com/maybe-finance/maybe/issues/724 and can be fixed the same way.
Come up with a better way of telling manual and connected accounts apart - currently it's done by checking account.start_balance.nil?. In the future it would be nicer to check if a provider connection exists for the account, but we're not quite there yet.
We should have some UI elements distinguishing between manual/connected accounts - maybe we could temporarily show a fake-ish error message on connected accounts saying that the "provider for this account is inactive"? After this PR, some accounts will react to transactions/valuations being added (manual accounts) while others will not (connected accounts) and this might cause confusion without any way of telling them apart.

This PR makes `balance` of manual accounts be automatically updated whenever a new valuation or a transaction is added to that account. In this first iteration, the solution "hijacks" the existing sync flow by first computing the correct current balance which is then treated as the "source of truth" in the rest of the sync (as if it was an account connected to a 3rd party provider). Current balance of a manual account is calculated as follows: - take the latest valuation's value, or `start_balance` if no valuation exists - get the sum of all transactions that occurred between the valuation (or all transactions if no valuation exists) and today - -> sum of these two numbers gives you the current balance ## Changes A new `accounts.start_balance` column has been added. I have considered making the migration populate the column for all accounts (as we only have manual accounts at the time of running the migration) but then to be consistent I would also have to change all of the seed data to create manual accounts, which doesn't sound like something we'd like to do. Happy to discuss this further. All newly created accounts will be manual by default as we don't have support for connected accounts yet. ## Next steps - Start balance of a manual account should be made editable (are there designs for an account edit dialog yet?) - Valuations and transactions can have their `date` set in the future. This means that accounts need to be synced periodically to make sure the balance is up to date every day the user opens Maybe. Somewhat similar to the issue https://github.com/maybe-finance/maybe/issues/724 and can be fixed the same way. - Come up with a better way of telling manual and connected accounts apart - currently it's done by checking `account.start_balance.nil?`. In the future it would be nicer to check if a provider connection exists for the account, but we're not quite there yet. - We should have some UI elements distinguishing between manual/connected accounts - maybe we could temporarily show a fake-ish error message on connected accounts saying that the "provider for this account is inactive"? After this PR, some accounts will react to transactions/valuations being added (manual accounts) while others will not (connected accounts) and this might cause confusion without any way of telling them apart.

jakubkottnauer commented

2024-05-13 15:27:16 +08:00

(Migrated from github.com)

@zachgoll After this is reviewed I will need some help with updating the Google Sheet to include the new manual account fixtures.

👍 1

zachgoll commented

2024-05-14 01:23:58 +08:00

(Migrated from github.com)

@jakubkottnauer will take a look this afternoon!

zachgoll commented

2024-05-14 23:33:27 +08:00

(Migrated from github.com)

@jakubkottnauer thanks for the work on this! I've addressed some of the various questions below along with some overall thoughts:

Start balance of a manual account should be made editable (are there designs for an account edit dialog yet?)

We do have some designs, I'll be getting them out for dev soon!

Valuations and transactions can have their date set in the future.

Should future dates be valid? I know we're not enforcing this currently but it may be a good idea to set a max date of Date.current for valuations and transactions.

Come up with a better way of telling manual and connected accounts apart - currently it's done by checking account.start_balance.nil?. In the future it would be nicer to check if a provider connection exists for the account, but we're not quite there yet.

We should have some UI elements distinguishing between manual/connected accounts - maybe we could temporarily show a fake-ish error message on connected accounts saying that the "provider for this account is inactive"? After this PR, some accounts will react to transactions/valuations being added (manual accounts) while others will not (connected accounts) and this might cause confusion without any way of telling them apart.

There are some designs in the works to distinguish between the two, but I'm starting to think that we should simply overwrite the existing sync process and tests and assume that all accounts are manual for now. I'm completely fine deleting the existing sync logic and making this the new "base case" until we have connected accounts to work with. We can always go back in source control to find the "connected account logic". For now, I think it will be a lot simpler to deal with concrete flows rather than trying to design for the future.

Off the top of my head, I think the pieces that we'd need to address would be:

Strategy

In my head, a "start balance" is ultimately the same thing as a Valuation. It has a monetary value + date.

Rather than introducing a new DB field, I think we can just use the "oldest Valuation" as our start balance. When a user creates an account, we can ask them for this information and our system will automatically create the Valuation:

Current day balance
Account start value (we create a Valuation equal to the date + amount input)

If we did this, I think we'd be able to keep almost all of our existing balance calculator logic. The only thing that would then change would be this line:

5ed2c47c20/app/models/account/balance/calculator.rb (L34)

We would then need to make sure that on every sync, @account.balance is updated to reflect the calculated series.

@jakubkottnauer thanks for the work on this! I've addressed some of the various questions below along with some overall thoughts: > Start balance of a manual account should be made editable (are there designs for an account edit dialog yet?) We do have some designs, I'll be getting them out for dev soon! > Valuations and transactions can have their date set in the future. Should future dates be valid? I know we're not enforcing this currently but it may be a good idea to set a max date of `Date.current` for valuations and transactions. > Come up with a better way of telling manual and connected accounts apart - currently it's done by checking account.start_balance.nil?. In the future it would be nicer to check if a provider connection exists for the account, but we're not quite there yet. > We should have some UI elements distinguishing between manual/connected accounts - maybe we could temporarily show a fake-ish error message on connected accounts saying that the "provider for this account is inactive"? After this PR, some accounts will react to transactions/valuations being added (manual accounts) while others will not (connected accounts) and this might cause confusion without any way of telling them apart. There are some designs in the works to distinguish between the two, but I'm starting to think that we should simply overwrite the existing sync process and tests and assume that all accounts are manual for now. I'm completely fine deleting the existing sync logic and making this the new "base case" until we have connected accounts to work with. We can always go back in source control to find the "connected account logic". For now, I think it will be a lot simpler to deal with concrete flows rather than trying to design for the future. Off the top of my head, I think the pieces that we'd need to address would be: ### Strategy In my head, a "start balance" is ultimately the same thing as a `Valuation`. It has a monetary value + date. Rather than introducing a new DB field, I think we can just use the "oldest Valuation" as our start balance. When a user creates an account, we can ask them for this information and our system will automatically create the `Valuation`: 1. Current day balance 2. Account start value (we create a `Valuation` equal to the date + amount input) If we did this, I _think_ we'd be able to keep almost all of our existing balance calculator logic. The only thing that would then change would be this line: https://github.com/maybe-finance/maybe/blob/5ed2c47c200e11840527f94a9fa164656ba3da40/app/models/account/balance/calculator.rb#L34 We would then need to make sure that on every sync, `@account.balance` is updated to reflect the calculated series.

jakubkottnauer commented

2024-05-15 02:47:07 +08:00

(Migrated from github.com)

Thank you for the review! I'll incorporate the changes you've suggested, making manual accounts the "base case" for now 👍

Should future dates be valid?

Some other apps I have used allow future transactions, though I don't see much value in allowing future valuations. And if we won't be allowing those, maybe it makes not to allow transactions either for consistency 😄 Up to you!

Rather than introducing a new DB field, I think we can just use the "oldest Valuation" as our start balance

I like the proposed solution, I'll make the change today/tomorrow

Thank you for the review! I'll incorporate the changes you've suggested, making manual accounts the "base case" for now 👍 > Should future dates be valid? Some other apps I have used allow future transactions, though I don't see much value in allowing future valuations. And if we won't be allowing those, maybe it makes not to allow transactions either for consistency 😄 Up to you! > Rather than introducing a new DB field, I think we can just use the "oldest Valuation" as our start balance I like the proposed solution, I'll make the change today/tomorrow

zachgoll commented

2024-05-15 22:59:05 +08:00

(Migrated from github.com)

@jakubkottnauer I think for now we can enforce a max date of Date.current for simplicity. I could see a case for future dates for both, but it's not worth the complexity right now:

Valuation - for example, a user provides a "projected" account balance
Transaction - for example, a user provides a "recurring" transaction that is expected

@jakubkottnauer I think for now we can enforce a max date of `Date.current` for simplicity. I could see a case for future dates for both, but it's not worth the complexity right now: - Valuation - for example, a user provides a "projected" account balance - Transaction - for example, a user provides a "recurring" transaction that is expected

👍 1

jakubkottnauer commented

2024-05-16 03:23:07 +08:00

(Migrated from github.com)

@zachgoll I've implemented the changes, seems to work well

zachgoll (Migrated from github.com) reviewed 2024-05-16 04:18:30 +08:00

zachgoll (Migrated from github.com) left a comment

Looks good! Can you update the tests to expect this new convention? Will get it merged after those pass.

app/models/exchange_rate.rb

						
				@@ -18,5 +18,12 @@ class ExchangeRate < ApplicationRecord

				    def get_rate_series(from, to, date_range)

zachgoll (Migrated from github.com) commented

2024-05-16 04:15:58 +08:00

      raise "Conversion from: #{from} to: #{to} on: #{date} not found" unless rate

```suggestion raise "Conversion from: #{from} to: #{to} on: #{date} not found" unless rate ```

jakubkottnauer commented

2024-05-17 00:51:39 +08:00

(Migrated from github.com)

@zachgoll I've pushed a few new tests. Can you help out with updating the CSVs?

zachgoll commented

2024-05-17 01:43:00 +08:00

(Migrated from github.com)

@jakubkottnauer here is the link to the Google sheet that I've been using to visualize the test cases:

https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit?usp=sharing

I'll go ahead and get this updated after this PR is merged. But for now, I think all we need to do is update the final row in each of these CSVs:

daf7ff8ef4/test/fixtures/family/expected_snapshots.csv (L32)

daf7ff8ef4/test/fixtures/account/expected_balances.csv (L32)

@jakubkottnauer here is the link to the Google sheet that I've been using to visualize the test cases: https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit?usp=sharing I'll go ahead and get this updated after this PR is merged. But for now, I think all we need to do is update the final row in each of these CSVs: https://github.com/maybe-finance/maybe/blob/daf7ff8ef4da8718a1038f4c679539f2305195f2/test/fixtures/family/expected_snapshots.csv?plain=1#L32 https://github.com/maybe-finance/maybe/blob/daf7ff8ef4da8718a1038f4c679539f2305195f2/test/fixtures/account/expected_balances.csv?plain=1#L32

jakubkottnauer commented

2024-05-17 03:48:45 +08:00

(Migrated from github.com)

@zachgoll All tests are green now 😄

🎉 1

zachgoll (Migrated from github.com) approved these changes 2024-05-17 03:56:15 +08:00

zachgoll (Migrated from github.com) left a comment

Awesome, looks great! Thanks for the work on this!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#735

				`@@ -18,5 +18,12 @@ class ExchangeRate < ApplicationRecord`
				`def get_rate_series(from, to, date_range)`