Fix foreign account sync crash

[jakubkottnauer]

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]

@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]

@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 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.

[jakubkottnauer]

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

[zachgoll]

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:

maybe/app/models/account/balance/calculator.rb

Lines 76 to 82 in 6e59fdb

	def normalized_valuations
	@normalized_valuations ||= normalize_entries_to_account_currency(@account.valuations.where("date >= ?", @calc_start_date).order(:date).select(:date, :value, :currency), :value)
	end
	def normalized_transactions
	@normalized_transactions ||= normalize_entries_to_account_currency(@account.transactions.where("date >= ?", @calc_start_date).order(:date).select(:date, :amount, :currency), :amount)
	end

[zachgoll]

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:

maybe/test/test_helper.rb

Lines 57 to 59 in 457247d

	def with_env_overrides(overrides = {}, &block)
	ClimateControl.modify(**overrides, &block)
	end

Here's an example usage:

maybe/test/controllers/registrations_controller_test.rb

Line 28 in 457247d

with_env_overrides REQUIRE_INVITE_CODE: "true" do

[jakubkottnauer]

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]

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.

[zachgoll]

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):

maybe/db/schema.rb

Lines 93 to 94 in 6e59fdb

	t.jsonb "sync_warnings", default: "[]", null: false
	t.jsonb "sync_errors", default: "[]", null: false

That way, we can simply say:

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

[jakubkottnauer]

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

[zachgoll]

@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]

@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]

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]

@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]

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

[jakubkottnauer]

Great point, I always forget about this 🙇

[zachgoll]

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