Fix foreign account sync crash (#794)

* Fix foreign account sync crash

* Refactor synth provider and show UI error if not configured

* Generate error message on missing exchange rates while converting balances

* Ignore sync messaged in i18n-tasks unused

* Generate missing exchange rate error during entry normalization

* Update alert classes

app/models/account/balance/calculator.rb

@@ -1,11 +1,10 @@
 class Account::Balance::Calculator
     attr_reader :daily_balances, :errors, :warnings
 
-    @daily_balances = []
-    @errors = []
-    @warnings = []
-
     def initialize(account, options = {})
+      @daily_balances = []
+      @errors = []
+      @warnings = []
       @account = account
       @calc_start_date = [ options[:calc_start_date], @account.effective_start_date ].compact.max
     end
@@ -43,34 +42,50 @@ def calculate
 
     private
       def convert_balances_to_family_currency
-        rates = ExchangeRate.get_rate_series(
+        rates = ExchangeRate.get_rates(
           @account.currency,
           @account.family.currency,
           @calc_start_date..Date.current
         ).to_a
 
-        @daily_balances.map do |balance|
-          rate = rates.find { |rate| rate.date == balance[:date] }
-          raise "Rate for #{@account.currency} to #{@account.family.currency} on #{balance[:date]} not found" if rate.nil?
-          converted_balance = balance[:balance] * rate.rate
+        # Abort conversion if some required rates are missing
+        if rates.length != @daily_balances.length
+          @errors << :sync_message_missing_rates
+          return []
+        end
+
+        @daily_balances.map.with_index do |balance, index|
+          converted_balance = balance[:balance] * rates[index].rate
           { date: balance[:date], balance: converted_balance, currency: @account.family.currency, updated_at: Time.current }
         end
       end
 
       # For calculation, all transactions and valuations need to be normalized to the same currency (the account's primary currency)
       def normalize_entries_to_account_currency(entries, value_key)
-        entries.map do |entry|
-          currency = entry.currency
-          date = entry.date
-          value = entry.send(value_key)
+        grouped_entries = entries.group_by(&:currency)
+        normalized_entries = []
 
+        grouped_entries.each do |currency, entries|
           if currency != @account.currency
-            value = ExchangeRate.convert(value:, from: currency, to: @account.currency, date:)
-            currency = @account.currency
+            dates = entries.map(&:date).uniq
+            rates = ExchangeRate.get_rates(currency, @account.currency, dates).to_a
+            if rates.length != dates.length
+              @errors << :sync_message_missing_rates
+            else
+              entries.each do |entry|
+                ## There can be several entries on the same date so we cannot rely on indeces
+                rate = rates.find { |rate| rate.date == entry.date }
+                value = entry.send(value_key)
+                value *= rate.rate
+                normalized_entries << entry.attributes.merge(value_key.to_s => value, "currency" => currency)
+              end
+            end
+          else
+            normalized_entries.concat(entries)
           end
-
-          entry.attributes.merge(value_key.to_s => value, "currency" => currency)
         end
+
+        normalized_entries
       end
 
       def normalized_valuations
@@ -92,8 +107,8 @@ def implied_start_balance
           return @account.balance_on(@calc_start_date)
         end
 
-        oldest_valuation_date = normalized_valuations.first&.dig("date")
-        oldest_transaction_date = normalized_transactions.first&.dig("date")
+        oldest_valuation_date = normalized_valuations.first&.date
+        oldest_transaction_date = normalized_transactions.first&.date
         oldest_entry_date = [ oldest_valuation_date, oldest_transaction_date ].compact.min
 
         if oldest_entry_date.present? && oldest_entry_date == oldest_valuation_date

app/models/account/syncable.rb

@@ -18,10 +18,10 @@ def sync(start_date = nil)
     self.balances.where("date < ?", effective_start_date).delete_all
     new_balance = calculator.daily_balances.select { |b| b[:currency] == self.currency }.last[:balance]
 
-    update!(status: "ok", last_sync_date: Date.today, balance: new_balance)
+    update!(status: "ok", last_sync_date: Date.today, balance: new_balance, sync_errors: calculator.errors, sync_warnings: calculator.warnings)
   rescue => e
-    update!(status: "error")
-    Rails.logger.error("Failed to sync account #{id}: #{e.message}")
+    update!(status: "error", sync_errors: [ :sync_message_unknown_error ])
+    logger.error("Failed to sync account #{id}: #{e.message}")
   end
 
   def can_sync?
@@ -77,8 +77,7 @@ def sync_exchange_rates
       next if existing_rates_set.include?([ rc_from, rc_to, rc_date.to_s ])
 
       logger.info "Fetching exchange rate from provider for account #{self.name}: #{self.id} (#{rc_from} to #{rc_to} on #{rc_date})"
-      rate = ExchangeRate.find_rate_or_fetch from: rc_from, to: rc_to, date: rc_date
-      ExchangeRate.create! base_currency: rc_from, converted_currency: rc_to, date: rc_date, rate: rate if rate
+      ExchangeRate.find_rate_or_fetch from: rc_from, to: rc_to, date: rc_date
     end
 
     nil

app/models/exchange_rate.rb

@@ -12,11 +12,11 @@ def find_rate(from:, to:, date:)
     end
 
     def find_rate_or_fetch(from:, to:, date:)
-      find_rate(from:, to:, date:) || fetch_rate_from_provider(from:, to:, date:).tap(&:save!)
+      find_rate(from:, to:, date:) || fetch_rate_from_provider(from:, to:, date:)&.tap(&:save!)
     end
 
-    def get_rate_series(from, to, date_range)
-      where(base_currency: from, converted_currency: to, date: date_range).order(:date)
+    def get_rates(from, to, dates)
+      where(base_currency: from, converted_currency: to, date: dates).order(:date)
     end
 
     def convert(value:, from:, to:, date:)

app/models/exchange_rate/provided.rb

@@ -5,6 +5,8 @@ module ExchangeRate::Provided
   class_methods do
     private
       def fetch_rate_from_provider(from:, to:, date:)
+        return nil unless exchange_rates_provider.configured?
+
         response = exchange_rates_provider.fetch_exchange_rate \
           from: Money::Currency.new(from).iso_code,
           to: Money::Currency.new(to).iso_code,

app/models/provider/synth.rb

@@ -5,6 +5,10 @@ def initialize(api_key = ENV["SYNTH_API_KEY"])
     @api_key = api_key || ENV["SYNTH_API_KEY"]
   end
 
+  def configured?
+    @api_key.present?
+  end
+
   def fetch_exchange_rate(from:, to:, date:)
     retrying Provider::Base.known_transient_errors do |on_last_attempt|
       response = Faraday.get("#{base_url}/rates/historical") do |req|

app/views/accounts/show.html.erb

@@ -41,6 +41,12 @@
   <%= turbo_frame_tag "sync_message" do %>
     <%= render partial: "accounts/sync_message", locals: { is_syncing: @account.syncing? } %>
   <% end %>
+  <% @account.sync_errors.each do |message| %>
+    <%= render partial: "shared/alert", locals: { type: "error", content: t("." + message) } %>
+  <% end %>
+  <% @account.sync_warnings.each do |message| %>
+    <%= render partial: "shared/alert", locals: { type: "warning", content: t("." + message) } %>
+  <% end %>
   <div class="bg-white shadow-xs rounded-xl border border-alpha-black-25 rounded-lg">
     <div class="p-4 flex justify-between">
       <div class="space-y-2">

app/views/shared/_alert.html.erb

@@ -0,0 +1,11 @@
+<%# locals: (type: "error", content: "") -%>
+<%= content_tag :div,
+  class: "flex justify-between rounded-xl p-3 #{type == "error" ? "bg-red-50" : "bg-yellow-50"}",
+  data: {controller: "element-removal" },
+  role: type == "error" ? "alert" : "status" do %>
+  <div class="flex gap-3 items-center <%= type == "error" ? "text-red-500" : "text-yellow-500" %>">
+    <%= lucide_icon("info", class: "w-5 h-5 shrink-0") %>
+    <p class="text-sm"><%= content %></p>
+  </div>
+  <%= content_tag :a, lucide_icon("x", class: "w-5 h-5 shrink-0 #{type == "error" ? "text-red-500" : "text-yellow-500"}"), data: { action: "click->element-removal#remove" }, class:"flex gap-1 font-medium items-center text-gray-900 px-3 py-1.5 rounded-lg cursor-pointer" %>
+<% end %>

config/i18n-tasks.yml

@@ -28,3 +28,4 @@ ignore_unused:
   - 'activerecord.models.account*' # i18n-tasks does not detect use in dynamic model names (e.g. object.model_name.human)
   - 'helpers.submit.*' # i18n-tasks does not detect used at forms
   - 'helpers.label.*' # i18n-tasks does not detect used at forms
+  - 'accounts.show.sync_message_*' # messages generated in the sync ActiveJob

config/locales/views/account/en.yml

@@ -35,6 +35,9 @@ en:
         /> <p>After deletion, there is no way you'll be able to restore the account
         information because you'll need to add it as a new account.</p>"
       confirm_title: Delete account?
+      sync_message_missing_rates: Since exchange rates haven't been synced, balance
+        graphs may not reflect accurate values.
+      sync_message_unknown_error: An error has occurred during the sync.
     summary:
       new: New account
     sync:

db/migrate/20240524203959_change_account_error_columns_default.rb

@@ -0,0 +1,8 @@
+class ChangeAccountErrorColumnsDefault < ActiveRecord::Migration[7.2]
+  def up
+    change_column_default :accounts, :sync_warnings, from: "[]", to: []
+    change_column_default :accounts, :sync_errors, from: "[]", to: []
+    Account.update_all(sync_warnings: [])
+    Account.update_all(sync_errors: [])
+  end
+end

test/models/exchange_rate_test.rb

@@ -7,36 +7,44 @@ class ExchangeRateTest < ActiveSupport::TestCase
   end
 
   test "fetch rate from provider when it's not found in db" do
-    ExchangeRate
-      .expects(:fetch_rate_from_provider)
-      .returns(ExchangeRate.new(base_currency: "USD", converted_currency: "MXN", rate: 1.0, date: Date.current))
+    with_env_overrides SYNTH_API_KEY: "true" do
+      ExchangeRate
+        .expects(:fetch_rate_from_provider)
+        .returns(ExchangeRate.new(base_currency: "USD", converted_currency: "MXN", rate: 1.0, date: Date.current))
 
-    ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+      ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+    end
   end
 
   test "provided rates are saved to the db" do
-    VCR.use_cassette "synth_exchange_rate" do
-      assert_difference "ExchangeRate.count", 1 do
-        ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+    with_env_overrides SYNTH_API_KEY: "true" do
+      VCR.use_cassette "synth_exchange_rate" do
+        assert_difference "ExchangeRate.count", 1 do
+          ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+        end
       end
     end
   end
 
   test "retrying, then raising on provider error" do
-    Faraday.expects(:get).returns(OpenStruct.new(success?: false)).times(3)
+    with_env_overrides SYNTH_API_KEY: "true" do
+      Faraday.expects(:get).returns(OpenStruct.new(success?: false)).times(3)
 
-    error = assert_raises Provider::Base::ProviderError do
-      ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
-    end
+      error = assert_raises Provider::Base::ProviderError do
+        ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+      end
 
-    assert_match "Failed to fetch exchange rate from Provider::Synth", error.message
+      assert_match "Failed to fetch exchange rate from Provider::Synth", error.message
+    end
   end
 
   test "retrying, then raising on network error" do
-    Faraday.expects(:get).raises(Faraday::TimeoutError).times(3)
+    with_env_overrides SYNTH_API_KEY: "true" do
+      Faraday.expects(:get).raises(Faraday::TimeoutError).times(3)
 
-    assert_raises Faraday::TimeoutError do
-      ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+      assert_raises Faraday::TimeoutError do
+        ExchangeRate.find_rate_or_fetch from: "USD", to: "MXN", date: Date.current
+      end
     end
   end
 end