Fix foreign account sync crash #794

2024-05-23T01:51:49+08:00

jakubkottnauer commented

2024-05-23 01:51:49 +08:00

(Migrated from github.com)

Syncing foreign currency accounts currently crashes as soon as a new exchange rate needs to be fetched.

find_rate_or_fetch already saves the fetched rate to the database (https://github.com/maybe-finance/maybe/blob/main/app/models/exchange_rate.rb#L15) so this PR removes the duplicate save from the Syncable module

Syncing foreign currency accounts currently crashes as soon as a new exchange rate needs to be fetched. `find_rate_or_fetch` already saves the fetched rate to the database (https://github.com/maybe-finance/maybe/blob/main/app/models/exchange_rate.rb#L15) so this PR removes the duplicate save from the Syncable module

zachgoll (Migrated from github.com) reviewed 2024-05-23 01:51:49 +08:00

zachgoll commented

2024-05-23 04:28:42 +08:00

(Migrated from github.com)

@jakubkottnauer I could be remembering this incorrectly, but wouldn't find_rate_or_fetch raise an error if the user has not configured an exchange rates provider?

I'm thinking we'll want to make sure that no errors are raised if the user hasn't yet configured the provider and go with the "warning" approach we were talking about here.

@jakubkottnauer I could be remembering this incorrectly, but wouldn't `find_rate_or_fetch` raise an error if the user has not configured an exchange rates provider? I'm thinking we'll want to make sure that no errors are raised if the user hasn't yet configured the provider and go with the "warning" approach we were talking about [here](https://github.com/maybe-finance/maybe/issues/724#issuecomment-2100489937).

jakubkottnauer commented

2024-05-24 02:58:09 +08:00

(Migrated from github.com)

@zachgoll I went ahead and implemented a UI alert that shows up if the SYNTH_API_KEY env var hasn't been provided. I have added a Provider::Synth.initialized? and use it to show the alert:

I have also added the use of Provider::Synth.initialized? into the sync call itself. This took me down a fun rabbit hole of failing tests, because in tests initialized? would always return false. That lead me to refactor the Synth provider to be properly configurable using the pattern described in this article, instead of depending on the env var directly. What do you think?

@zachgoll I went ahead and implemented a UI alert that shows up if the `SYNTH_API_KEY` env var hasn't been provided. I have added a `Provider::Synth.initialized?` and use it to show the alert: ![Screenshot 2024-05-23 at 20 49 59](https://github.com/maybe-finance/maybe/assets/113784/4fb9980f-9199-4763-8189-43dca0ac3a02) I have also added the use of `Provider::Synth.initialized?` into the `sync` call itself. This took me down a fun rabbit hole of failing tests, because in tests `initialized?` would always return false. That lead me to refactor the Synth provider to be properly configurable using the pattern described in [this article](https://thoughtbot.com/blog/mygem-configure-block), instead of depending on the env var directly. What do you think?

zachgoll (Migrated from github.com) reviewed 2024-05-24 03:57:09 +08:00

app/models/account/syncable.rb

zachgoll (Migrated from github.com) commented

2024-05-24 03:42:11 +08:00

I like the idea of canceling a sync based on a condition here, but I think a foreign_currency? account should still be able to sync regardless of whether the exchange rates are available.

The reason being—as long as all the transactions/valuations are in the same currency, we can still successfully build a historical balance graph in that currency. It's only during those "rollup" scenarios where we are trying to combine balances of different currencies where we'd need to abort and handle.

All that said, multi_currency? accounts could benefit from cancelling the sync early if we don't have exchange rates. Because in that case, we cannot even build a historical balance graph properly without first converting values.

I like the idea of canceling a sync based on a condition here, but I think a `foreign_currency?` account should still be able to sync regardless of whether the exchange rates are available. The reason being—as long as all the transactions/valuations are in the same currency, we can still successfully build a historical balance graph in that currency. It's only during those "rollup" scenarios where we are trying to combine balances of different currencies where we'd need to abort and handle. All that said, `multi_currency?` accounts could benefit from cancelling the sync early if we don't have exchange rates. Because in that case, we cannot even build a historical balance graph properly without first converting values.

config/initializers/synth.rb

zachgoll (Migrated from github.com) commented

2024-05-24 03:54:36 +08:00

The issue I think we'll run into here is for self-hosters. They'll need to be able to configure their Synth API key directly in the UI via the settings model, which we haven't configured quite yet, but will need to soon.

You had mentioned that a big reason for this refactor is due to initialized? always returning false during the tests. I'm thinking that may be more of a symptom of tightly-coupled dependencies rather than a reason to change the class itself. I'm not sure which tests were failing on you, but I think we should try to address it elsewhere so we can keep the config for Synth available at runtime. Let me know which tests are causing problems and I'd be happy to throw around some ideas.

Something that may help out is the with_env_override helper, which allows us to toggle environment variables on and off during tests for special cases where we need to test "enabled/disabled" behaviors:

457247da8e/test/test_helper.rb (L57-L59)

Here's an example usage:

457247da8e/test/controllers/registrations_controller_test.rb (L28)

The issue I think we'll run into here is for self-hosters. They'll need to be able to configure their Synth API key directly in the UI via the [settings model](https://github.com/maybe-finance/maybe/blob/main/app/models/setting.rb), which we haven't configured quite yet, but will need to soon. You had mentioned that a big reason for this refactor is due to `initialized?` always returning false during the tests. I'm thinking that may be more of a symptom of tightly-coupled dependencies rather than a reason to change the class itself. I'm not sure which tests were failing on you, but I think we should try to address it elsewhere so we can keep the config for `Synth` available at runtime. Let me know which tests are causing problems and I'd be happy to throw around some ideas. Something that may help out is the `with_env_override` helper, which allows us to toggle environment variables on and off during tests for special cases where we need to test "enabled/disabled" behaviors: https://github.com/maybe-finance/maybe/blob/457247da8e40254c469a1133fefbba4ffc5013b5/test/test_helper.rb#L57-L59 Here's an example usage: https://github.com/maybe-finance/maybe/blob/457247da8e40254c469a1133fefbba4ffc5013b5/test/controllers/registrations_controller_test.rb#L28

jakubkottnauer (Migrated from github.com) reviewed 2024-05-24 04:09:46 +08:00

app/models/account/syncable.rb

jakubkottnauer (Migrated from github.com) commented

2024-05-24 04:09:46 +08:00

Depending on what exchange rates are available during a sync, there is a chance that an error is raised in the calculator here https://github.com/maybe-finance/maybe/blob/main/app/models/account/balance/calculator.rb#L54
which is why I went with short-circuiting the entire sync process.

If we're ok with not raising errors here and just generating invalid balances (together with showing the UI messages) then I can change this

Depending on what exchange rates are available during a sync, there is a chance that an error is raised in the calculator here https://github.com/maybe-finance/maybe/blob/main/app/models/account/balance/calculator.rb#L54 which is why I went with short-circuiting the entire sync process. If we're ok with not raising errors here and just generating invalid balances (together with showing the UI messages) then I can change this

jakubkottnauer (Migrated from github.com) reviewed 2024-05-24 04:11:50 +08:00

config/initializers/synth.rb

jakubkottnauer (Migrated from github.com) commented

2024-05-24 04:11:50 +08:00

It was specifically the sync tests that were failing because of the new early return we discuss in the other thread. If we were to remove the early return and instead proceed with the sync generating possibly invalid balances, then that would also remove the immediate need for this refactor. Though I still think it's beneficial to have the provider configurable in the same way as if it was a 3rd party gem (this seems to be quite a common pattern from what I've gathered).

but I think we should try to address it elsewhere so we can keep the config for Synth available at runtime

Doesn't the refactor already solve it? Maybe I'm missing something but I think with the refactor you can access/modify the synth configuration from anywhere at any time

It was specifically the `sync` tests that were failing because of the new early return we discuss in the other thread. If we were to remove the early return and instead proceed with the sync generating possibly invalid balances, then that would also remove the immediate need for this refactor. Though I still think it's beneficial to have the provider configurable in the same way as if it was a 3rd party gem (this seems to be quite a common pattern from what I've gathered). > but I think we should try to address it elsewhere so we can keep the config for Synth available at runtime Doesn't the refactor already solve it? Maybe I'm missing something but I think with the refactor you can access/modify the synth configuration from anywhere at any time

zachgoll (Migrated from github.com) reviewed 2024-05-24 23:01:46 +08:00

app/models/account/syncable.rb

zachgoll (Migrated from github.com) commented

2024-05-24 23:01:46 +08:00

Yeah this is a tricky one. To me this feels like the calculator logic I wrote is more at fault here than anything.

Ultimately, there are 2 stages of currency conversion going on in this calculator:

Normalizing multi_currency? balances - if this fails, then we can't show anything for the account
Converting an entire series of foreign_currency? balances - if this fails, we can't rollup the account, but we can still show a history graph in its native currency

We could either:

Loop through balances and for balances where there is no exchange rate, we don't generate it at all (leaving balance gaps)
Pre-fetch exchange rates for the known balances and if any exchange fails, we abort the entire conversion

The "all or nothing" approach in #2 seems a bit simpler here and in order to make that happen I think this calculator is what needs the refactor to decouple the balance logic from the exchange rate logic. The two are intermingled right now. For example:

6e59fdb369/app/models/account/balance/calculator.rb (L76-L82)

Yeah this is a tricky one. To me this feels like the calculator logic I wrote is more at fault here than anything. Ultimately, there are 2 stages of currency conversion going on in this calculator: 1. Normalizing `multi_currency?` balances - if this fails, then we can't show anything for the account 2. Converting an entire series of `foreign_currency?` balances - if this fails, we can't rollup the account, but we can still show a history graph in its native currency We could either: 1. Loop through balances and for balances where there is no exchange rate, we don't generate it at all (leaving balance gaps) 2. Pre-fetch exchange rates for the known balances and if any exchange fails, we abort the entire conversion The "all or nothing" approach in #2 seems a bit simpler here and in order to make that happen I think this calculator is what needs the refactor to decouple the balance logic from the exchange rate logic. The two are intermingled right now. For example: https://github.com/maybe-finance/maybe/blob/6e59fdb369d18dcd1163e9088f272a716ace6acb/app/models/account/balance/calculator.rb#L76-L82

zachgoll (Migrated from github.com) reviewed 2024-05-24 23:30:24 +08:00

config/initializers/synth.rb

zachgoll (Migrated from github.com) commented

2024-05-24 23:30:24 +08:00

Yes this does allow for configuration at runtime, but it would be more of a "re-configuration" (initializes, then user changes the api key in their settings), which feels like an extra layer of abstraction (global state) that is not necessary.

Here is the original data providers PR where I had started down this "global state" path and shows how we ended up with the current design which is more tightly integrated with the domain models.

I think rather than focusing on configuration and the initialized? method that checks whether an API key is present (which still doesn't guarantee rates are present), we need to dig into our calculator class and refactor the logic to make these decisions based on the presence/absence of the exchange rates as I mentioned in the other comment.

In other words, if we're syncing foreign currency account XYZ, we need to figure out which exchange rates are needed, check if we have all of them available (regardless of providers as they could be in our database), and then proceed/abort the sync depending on that response.

Yes this does allow for configuration at runtime, but it would be more of a "re-configuration" (initializes, then user changes the api key in their settings), which feels like an extra layer of abstraction (global state) that is not necessary. Here is the [original data providers PR](https://github.com/maybe-finance/maybe/pull/574) where I had started down this "global state" path and shows how we ended up with the current design which is more tightly integrated with the domain models. I think rather than focusing on configuration and the `initialized?` method that checks whether an API key is present (which still doesn't guarantee rates are present), we need to dig into our calculator class and refactor the logic to make these decisions based on the presence/absence of the exchange rates as I mentioned in the other comment. In other words, if we're syncing foreign currency account XYZ, we need to figure out which exchange rates are needed, check if we have all of them available (regardless of providers as they could be in our database), and then proceed/abort the sync depending on that response.

zachgoll (Migrated from github.com) reviewed 2024-05-24 23:34:30 +08:00

config/initializers/synth.rb

zachgoll (Migrated from github.com) commented

2024-05-24 23:34:29 +08:00

And then for the UI warnings/errors, we can check on an account-by-account basis by populating these during the sync process (these are currently not in use, but I had added as placeholders):

6e59fdb369/db/schema.rb (L93-L94)

That way, we can simply say:

<% if @account.sync_errors.present? %>
  <%= render partial: "accounts/some-partial", locals: { errors: @account.sync_errors } %>
<% end %>

And then for the UI warnings/errors, we can check on an account-by-account basis by populating these during the sync process (these are currently not in use, but I had added as placeholders): https://github.com/maybe-finance/maybe/blob/6e59fdb369d18dcd1163e9088f272a716ace6acb/db/schema.rb#L93-L94 That way, we can simply say: ```erb <% if @account.sync_errors.present? %> <%= render partial: "accounts/some-partial", locals: { errors: @account.sync_errors } %> <% end %> ```

jakubkottnauer (Migrated from github.com) reviewed 2024-05-25 01:17:04 +08:00

config/initializers/synth.rb

jakubkottnauer (Migrated from github.com) commented

2024-05-25 01:17:04 +08:00

Thanks for the reply! All this makes sense, I'll rework the PR 👍

zachgoll (Migrated from github.com) reviewed 2024-05-25 02:16:22 +08:00

config/initializers/synth.rb

zachgoll (Migrated from github.com) commented

2024-05-25 02:16:21 +08:00

@jakubkottnauer feel free to rework the sync_warnings and sync_errors if you need to. We may only need one of these? And could potentially be worth a dedicated table so we can provide more structured warnings/errors?

I was thinking that long term, we'd have a folder in the repo with the following format:

/docs/troubleshooting/[error_type]/[error_name].md

And then we'd be able to write some guides for each error that could be rendered in the sidebar drawer or something.

Not necessarily needed yet, just wanted to share that idea if it helps for this

@jakubkottnauer feel free to rework the `sync_warnings` and `sync_errors` if you need to. We may only need one of these? And could potentially be worth a dedicated table so we can provide more structured warnings/errors? I was thinking that long term, we'd have a folder in the repo with the following format: `/docs/troubleshooting/[error_type]/[error_name].md` And then we'd be able to write some guides for each error that could be rendered in the sidebar drawer or something. Not necessarily needed yet, just wanted to share that idea if it helps for this

jakubkottnauer commented

2024-05-26 19:29:07 +08:00

(Migrated from github.com)

@zachgoll Ready for another round of review. I tried to keep the changes to a bare minimum so I have mostly left the sync_warnings and sync_errors the way they were, just fixed a few related bugs I found along the way.

@zachgoll Ready for another round of review. I tried to keep the changes to a bare minimum so I have mostly left the `sync_warnings` and `sync_errors` the way they were, just fixed a few related bugs I found along the way.

jakubkottnauer (Migrated from github.com) reviewed 2024-05-26 19:37:49 +08:00

app/models/exchange_rate.rb

						
				@@ -20,2 +18,4 @@

				    def get_rates(from, to, dates)

				      where(base_currency: from, converted_currency: to, date: dates).order(:date)

				    end

jakubkottnauer (Migrated from github.com) commented

2024-05-26 19:37:49 +08:00

Renamed the method as it can be used not just for a continuous series of dates, but also for an arbitrary array of dates

zachgoll (Migrated from github.com) approved these changes 2024-05-27 22:26:24 +08:00

zachgoll (Migrated from github.com) left a comment

@jakubkottnauer nice updates here, I think this is a good improvement on what we had.

A few notes that could be good to keep in mind moving forward:

In the future, we'll probably have to introduce some sort of UI that warns the user that they will be using API credits for a sync process (not needed yet)
As I'm looking through the errors vs. warnings, my guess is that we'll probably end up with a single errors array with varying severities. I'm thinking that it might be adding a bit of ambiguity to our sync process trying to decide whether something is an "error" or "warning". Not something we need to update here, but just a thought moving forward.

@jakubkottnauer nice updates here, I think this is a good improvement on what we had. A few notes that could be good to keep in mind moving forward: - In the future, we'll probably have to introduce some sort of UI that warns the user that they will be using API credits for a sync process (not needed yet) - As I'm looking through the errors vs. warnings, my _guess_ is that we'll probably end up with a single `errors` array with varying severities. I'm thinking that it might be adding a bit of ambiguity to our sync process trying to decide whether something is an "error" or "warning". Not something we need to update here, but just a thought moving forward.

zachgoll (Migrated from github.com) reviewed 2024-05-27 22:30:44 +08:00

app/views/shared/_alert.html.erb

						
				@@ -0,0 +1,11 @@

				<%# locals: (type: "error", content: "") -%>

zachgoll (Migrated from github.com) commented

2024-05-27 22:30:43 +08:00

Since Tailwind does static analysis of the classes, bg-#{color}-50 won't always work properly:

https://tailwindcss.com/docs/content-configuration#dynamic-class-names

Since Tailwind does static analysis of the classes, `bg-#{color}-50` won't always work properly: https://tailwindcss.com/docs/content-configuration#dynamic-class-names

zachgoll (Migrated from github.com) reviewed 2024-05-27 22:31:14 +08:00

app/views/shared/_alert.html.erb

						
				@@ -0,0 +1,11 @@

				<%# locals: (type: "error", content: "") -%>

zachgoll (Migrated from github.com) commented

2024-05-27 22:31:14 +08:00

Same as above for text-#{color}-500

Same as above for `text-#{color}-500`

jakubkottnauer (Migrated from github.com) reviewed 2024-05-27 23:32:13 +08:00

app/views/shared/_alert.html.erb

						
				@@ -0,0 +1,11 @@

				<%# locals: (type: "error", content: "") -%>

jakubkottnauer (Migrated from github.com) commented

2024-05-27 23:32:13 +08:00

Great point, I always forget about this 🙇

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#794

				`@@ -0,0 +1,11 @@`
				`<%# locals: (type: "error", content: "") -%>`