From 483d67846c249f2a898b090d2e7aec2115e53c3b Mon Sep 17 00:00:00 2001 From: Jakub Kottnauer Date: Mon, 27 May 2024 18:10:28 +0200 Subject: [PATCH] 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 | 53 ++++++++++++------- app/models/account/syncable.rb | 9 ++-- app/models/exchange_rate.rb | 6 +-- app/models/exchange_rate/provided.rb | 2 + app/models/provider/synth.rb | 4 ++ app/views/accounts/show.html.erb | 6 +++ app/views/shared/_alert.html.erb | 11 ++++ config/i18n-tasks.yml | 1 + config/locales/views/account/en.yml | 3 ++ ...59_change_account_error_columns_default.rb | 8 +++ db/schema.rb | 6 +-- test/models/exchange_rate_test.rb | 38 +++++++------ 12 files changed, 102 insertions(+), 45 deletions(-) create mode 100644 app/views/shared/_alert.html.erb create mode 100644 db/migrate/20240524203959_change_account_error_columns_default.rb diff --git a/app/models/account/balance/calculator.rb b/app/models/account/balance/calculator.rb index 8d086d56..24bd451a 100644 --- a/app/models/account/balance/calculator.rb +++ b/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 @@ class Account::Balance::Calculator 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 @@ class Account::Balance::Calculator 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 diff --git a/app/models/account/syncable.rb b/app/models/account/syncable.rb index afccec00..1b47caeb 100644 --- a/app/models/account/syncable.rb +++ b/app/models/account/syncable.rb @@ -18,10 +18,10 @@ module Account::Syncable 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 @@ module Account::Syncable 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 diff --git a/app/models/exchange_rate.rb b/app/models/exchange_rate.rb index 5dc04ecc..b3260b55 100644 --- a/app/models/exchange_rate.rb +++ b/app/models/exchange_rate.rb @@ -12,11 +12,11 @@ class ExchangeRate < ApplicationRecord 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:) diff --git a/app/models/exchange_rate/provided.rb b/app/models/exchange_rate/provided.rb index afbb644e..8da089d7 100644 --- a/app/models/exchange_rate/provided.rb +++ b/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, diff --git a/app/models/provider/synth.rb b/app/models/provider/synth.rb index 6426d580..9966b92f 100644 --- a/app/models/provider/synth.rb +++ b/app/models/provider/synth.rb @@ -5,6 +5,10 @@ class Provider::Synth @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| diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 4e648d9a..edbfcd21 100644 --- a/app/views/accounts/show.html.erb +++ b/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 %>
diff --git a/app/views/shared/_alert.html.erb b/app/views/shared/_alert.html.erb new file mode 100644 index 00000000..21091fe4 --- /dev/null +++ b/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 %> +
"> + <%= lucide_icon("info", class: "w-5 h-5 shrink-0") %> +

<%= content %>

+
+ <%= 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 %> diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index be226048..9b2ac779 100644 --- a/config/i18n-tasks.yml +++ b/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 diff --git a/config/locales/views/account/en.yml b/config/locales/views/account/en.yml index 1a22a756..38248678 100644 --- a/config/locales/views/account/en.yml +++ b/config/locales/views/account/en.yml @@ -35,6 +35,9 @@ en: />

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.

" 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: diff --git a/db/migrate/20240524203959_change_account_error_columns_default.rb b/db/migrate/20240524203959_change_account_error_columns_default.rb new file mode 100644 index 00000000..c85cc844 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 91b81239..08a9712e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2024_05_22_151453) do +ActiveRecord::Schema[7.2].define(version: 2024_05_24_203959) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -90,8 +90,8 @@ ActiveRecord::Schema[7.2].define(version: 2024_05_22_151453) do t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY (ARRAY[('Account::Loan'::character varying)::text, ('Account::Credit'::character varying)::text, ('Account::OtherLiability'::character varying)::text])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true t.boolean "is_active", default: true, null: false t.enum "status", default: "ok", null: false, enum_type: "account_status" - t.jsonb "sync_warnings", default: "[]", null: false - t.jsonb "sync_errors", default: "[]", null: false + t.jsonb "sync_warnings", default: [], null: false + t.jsonb "sync_errors", default: [], null: false t.date "last_sync_date" t.index ["accountable_type"], name: "index_accounts_on_accountable_type" t.index ["family_id"], name: "index_accounts_on_family_id" diff --git a/test/models/exchange_rate_test.rb b/test/models/exchange_rate_test.rb index 30e8b03b..ac2ddf55 100644 --- a/test/models/exchange_rate_test.rb +++ b/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 + 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 end - - assert_match "Failed to fetch exchange rate from Provider::Synth", error.message 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