diff --git a/app/controllers/accountable_sparklines_controller.rb b/app/controllers/accountable_sparklines_controller.rb index 38b80cca..959717ec 100644 --- a/app/controllers/accountable_sparklines_controller.rb +++ b/app/controllers/accountable_sparklines_controller.rb @@ -2,25 +2,24 @@ class AccountableSparklinesController < ApplicationController def show @accountable = Accountable.from_type(params[:accountable_type]&.classify) - # Pre-load the series to catch any errors before rendering - @series = Rails.cache.fetch(cache_key) do - account_ids = family.accounts.active.where(accountable_type: @accountable.name).pluck(:id) + etag_key = cache_key - builder = Balance::ChartSeriesBuilder.new( - account_ids: account_ids, - currency: family.currency, - period: Period.last_30_days, - favorable_direction: @accountable.favorable_direction, - interval: "1 day" - ) + # Use HTTP conditional GET so the client receives 304 Not Modified when possible. + if stale?(etag: etag_key, last_modified: family.latest_sync_completed_at) + @series = Rails.cache.fetch(etag_key, expires_in: 24.hours) do + builder = Balance::ChartSeriesBuilder.new( + account_ids: account_ids, + currency: family.currency, + period: Period.last_30_days, + favorable_direction: @accountable.favorable_direction, + interval: "1 day" + ) - builder.balance_series + builder.balance_series + end + + render layout: false end - - render layout: false - rescue => e - Rails.logger.error "Accountable sparkline error for #{@accountable&.name}: #{e.message}" - render partial: "accountable_sparklines/error", layout: false end private @@ -28,7 +27,15 @@ class AccountableSparklinesController < ApplicationController Current.family end + def accountable + Accountable.from_type(params[:accountable_type]&.classify) + end + + def account_ids + family.accounts.active.where(accountable_type: accountable.name).pluck(:id) + end + def cache_key - family.build_cache_key("#{@accountable.name}_sparkline") + family.build_cache_key("#{@accountable.name}_sparkline", invalidate_on_data_updates: true) end end diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 2477be98..854bd826 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -23,12 +23,14 @@ class AccountsController < ApplicationController end def sparkline - # Pre-load the sparkline series to catch any errors before rendering - @sparkline_series = @account.sparkline_series - render layout: false - rescue => e - Rails.logger.error "Sparkline error for account #{@account.id}: #{e.message}" - render partial: "accounts/sparkline_error", layout: false + etag_key = @account.family.build_cache_key("#{@account.id}_sparkline", invalidate_on_data_updates: true) + + # Short-circuit with 304 Not Modified when the client already has the latest version. + # We defer the expensive series computation until we know the content is stale. + if stale?(etag: etag_key, last_modified: @account.family.latest_sync_completed_at) + @sparkline_series = @account.sparkline_series + render layout: false + end end private diff --git a/app/models/account.rb b/app/models/account.rb index 4984fb89..b1e2c80c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -61,18 +61,6 @@ class Account < ApplicationRecord end end - def syncing? - self_syncing = syncs.visible.any? - - # Since Plaid Items sync as a "group", if the item is syncing, even if the account - # sync hasn't yet started (i.e. we're still fetching the Plaid data), show it as syncing in UI. - if linked? - plaid_account&.plaid_item&.syncing? || self_syncing - else - self_syncing - end - end - def institution_domain url_string = plaid_account&.plaid_item&.institution_url return nil unless url_string.present? diff --git a/app/models/account/chartable.rb b/app/models/account/chartable.rb index 304fcbbd..b4f2645e 100644 --- a/app/models/account/chartable.rb +++ b/app/models/account/chartable.rb @@ -24,20 +24,10 @@ module Account::Chartable end def sparkline_series - cache_key = family.build_cache_key("#{id}_sparkline") + cache_key = family.build_cache_key("#{id}_sparkline", invalidate_on_data_updates: true) - Rails.cache.fetch(cache_key) do + Rails.cache.fetch(cache_key, expires_in: 24.hours) do balance_series end - rescue => e - Rails.logger.error "Sparkline series error for account #{id}: #{e.message}" - # Return empty series as fallback - Series.new( - start_date: 30.days.ago.to_date, - end_date: Date.current, - interval: "1 day", - values: [], - favorable_direction: favorable_direction - ) end end diff --git a/app/models/account/sync_complete_event.rb b/app/models/account/sync_complete_event.rb index 32315375..d26b62f8 100644 --- a/app/models/account/sync_complete_event.rb +++ b/app/models/account/sync_complete_event.rb @@ -16,13 +16,13 @@ class Account::SyncCompleteEvent locals: { account: account } ) - # Replace the groups this account belongs to in the sidebar - account_group_ids.each do |id| + # Replace the groups this account belongs to in both desktop and mobile sidebars + sidebar_targets.each do |(tab, mobile_flag)| account.broadcast_replace_to( account.family, - target: id, + target: account_group.dom_id(tab: tab, mobile: mobile_flag), partial: "accounts/accountable_group", - locals: { account_group: account_group, open: true } + locals: { account_group: account_group, open: true, all_tab: tab == :all, mobile: mobile_flag } ) end @@ -37,18 +37,18 @@ class Account::SyncCompleteEvent end private - # The sidebar will show the account in both its classification tab and the "all" tab, - # so we need to broadcast to both. - def account_group_ids - unless account_group.present? - error = Error.new("Account #{account.id} is not part of an account group") - Rails.logger.warn(error.message) - Sentry.capture_exception(error, level: :warning) - return [] - end + # Returns an array of [tab, mobile?] tuples that should receive an update. + # We broadcast to both the classification-specific tab and the "all" tab, + # for desktop (mobile: false) and mobile (mobile: true) variants. + def sidebar_targets + return [] unless account_group.present? - id = account_group.id - [ id, "#{account_group.classification}_#{id}" ] + [ + [ account_group.classification.to_sym, false ], + [ :all, false ], + [ account_group.classification.to_sym, true ], + [ :all, true ] + ] end def account_group diff --git a/app/models/assistant/function/get_balance_sheet.rb b/app/models/assistant/function/get_balance_sheet.rb index ea2b423e..5bea4c6d 100644 --- a/app/models/assistant/function/get_balance_sheet.rb +++ b/app/models/assistant/function/get_balance_sheet.rb @@ -31,11 +31,11 @@ class Assistant::Function::GetBalanceSheet < Assistant::Function monthly_history: historical_data(period) }, assets: { - current: family.balance_sheet.total_assets_money.format, + current: family.balance_sheet.assets.total_money.format, monthly_history: historical_data(period, classification: "asset") }, liabilities: { - current: family.balance_sheet.total_liabilities_money.format, + current: family.balance_sheet.liabilities.total_money.format, monthly_history: historical_data(period, classification: "liability") }, insights: insights_data @@ -65,8 +65,8 @@ class Assistant::Function::GetBalanceSheet < Assistant::Function end def insights_data - assets = family.balance_sheet.total_assets - liabilities = family.balance_sheet.total_liabilities + assets = family.balance_sheet.assets.total + liabilities = family.balance_sheet.liabilities.total ratio = liabilities.zero? ? 0 : (liabilities / assets.to_f) { diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index b5dc335d..bc5aaf15 100644 --- a/app/models/balance_sheet.rb +++ b/app/models/balance_sheet.rb @@ -1,7 +1,7 @@ class BalanceSheet include Monetizable - monetize :total_assets, :total_liabilities, :net_worth + monetize :net_worth attr_reader :family @@ -9,99 +9,36 @@ class BalanceSheet @family = family end - def total_assets - totals_query.filter { |t| t.classification == "asset" }.sum(&:converted_balance) + def assets + @assets ||= ClassificationGroup.new( + classification: "asset", + currency: family.currency, + accounts: account_totals.asset_accounts + ) end - def total_liabilities - totals_query.filter { |t| t.classification == "liability" }.sum(&:converted_balance) - end - - def net_worth - total_assets - total_liabilities + def liabilities + @liabilities ||= ClassificationGroup.new( + classification: "liability", + currency: family.currency, + accounts: account_totals.liability_accounts + ) end def classification_groups - Rails.cache.fetch(family.build_cache_key("bs_classification_groups")) do - asset_groups = account_groups("asset") - liability_groups = account_groups("liability") - - [ - ClassificationGroup.new( - key: "asset", - display_name: "Assets", - icon: "plus", - total_money: total_assets_money, - account_groups: asset_groups, - syncing?: asset_groups.any?(&:syncing?) - ), - ClassificationGroup.new( - key: "liability", - display_name: "Debts", - icon: "minus", - total_money: total_liabilities_money, - account_groups: liability_groups, - syncing?: liability_groups.any?(&:syncing?) - ) - ] - end + [ assets, liabilities ] end - def account_groups(classification = nil) - Rails.cache.fetch(family.build_cache_key("bs_account_groups_#{classification || 'all'}")) do - classification_accounts = classification ? totals_query.filter { |t| t.classification == classification } : totals_query - classification_total = classification_accounts.sum(&:converted_balance) + def account_groups + [ assets.account_groups, liabilities.account_groups ].flatten + end - account_groups = classification_accounts.group_by(&:accountable_type) - .transform_keys { |k| Accountable.from_type(k) } - - groups = account_groups.map do |accountable, accounts| - group_total = accounts.sum(&:converted_balance) - - key = accountable.model_name.param_key - - group = AccountGroup.new( - id: classification ? "#{classification}_#{key}_group" : "#{key}_group", - key: key, - name: accountable.display_name, - classification: accountable.classification, - total: group_total, - total_money: Money.new(group_total, currency), - weight: classification_total.zero? ? 0 : group_total / classification_total.to_d * 100, - missing_rates?: accounts.any? { |a| a.missing_rates? }, - color: accountable.color, - syncing?: accounts.any?(&:is_syncing), - accounts: accounts.map do |account| - account - end.sort_by(&:converted_balance).reverse - ) - - group - end - - groups.sort_by do |group| - manual_order = Accountable::TYPES - type_name = group.key.camelize - manual_order.index(type_name) || Float::INFINITY - end - end + def net_worth + assets.total - liabilities.total end def net_worth_series(period: Period.last_30_days) - memo_key = [ period.start_date, period.end_date ].compact.join("_") - - @net_worth_series ||= {} - - account_ids = active_accounts.pluck(:id) - - builder = (@net_worth_series[memo_key] ||= Balance::ChartSeriesBuilder.new( - account_ids: account_ids, - currency: currency, - period: period, - favorable_direction: "up" - )) - - builder.balance_series + net_worth_series_builder.net_worth_series(period: period) end def currency @@ -109,32 +46,19 @@ class BalanceSheet end def syncing? - classification_groups.any? { |group| group.syncing? } + sync_status_monitor.syncing? end private - ClassificationGroup = Struct.new(:key, :display_name, :icon, :total_money, :account_groups, :syncing?, keyword_init: true) - AccountGroup = Struct.new(:id, :key, :name, :accountable_type, :classification, :total, :total_money, :weight, :accounts, :color, :missing_rates?, :syncing?, keyword_init: true) - - def active_accounts - family.accounts.active.with_attached_logo + def sync_status_monitor + @sync_status_monitor ||= SyncStatusMonitor.new(family) end - def totals_query - @totals_query ||= active_accounts - .joins(ActiveRecord::Base.sanitize_sql_array([ "LEFT JOIN exchange_rates ON exchange_rates.date = CURRENT_DATE AND accounts.currency = exchange_rates.from_currency AND exchange_rates.to_currency = ?", currency ])) - .joins(ActiveRecord::Base.sanitize_sql_array([ - "LEFT JOIN syncs ON syncs.syncable_id = accounts.id AND syncs.syncable_type = 'Account' AND syncs.status IN (?) AND syncs.created_at > ?", - %w[pending syncing], - Sync::VISIBLE_FOR.ago - ])) - .select( - "accounts.*", - "SUM(accounts.balance * COALESCE(exchange_rates.rate, 1)) as converted_balance", - "COUNT(syncs.id) > 0 as is_syncing", - ActiveRecord::Base.sanitize_sql_array([ "COUNT(CASE WHEN accounts.currency <> ? AND exchange_rates.rate IS NULL THEN 1 END) as missing_rates", currency ]) - ) - .group(:classification, :accountable_type, :id) - .to_a + def account_totals + @account_totals ||= AccountTotals.new(family, sync_status_monitor: sync_status_monitor) + end + + def net_worth_series_builder + @net_worth_series_builder ||= NetWorthSeriesBuilder.new(family) end end diff --git a/app/models/balance_sheet/account_group.rb b/app/models/balance_sheet/account_group.rb new file mode 100644 index 00000000..a4b23d1f --- /dev/null +++ b/app/models/balance_sheet/account_group.rb @@ -0,0 +1,61 @@ +class BalanceSheet::AccountGroup + include Monetizable + + monetize :total, as: :total_money + + attr_reader :name, :color, :accountable_type, :accounts + + def initialize(name:, color:, accountable_type:, accounts:, classification_group:) + @name = name + @color = color + @accountable_type = accountable_type + @accounts = accounts + @classification_group = classification_group + end + + # A stable DOM id for this group. + # Example outputs: + # dom_id(tab: :asset) # => "asset_depository" + # dom_id(tab: :all, mobile: true) # => "mobile_all_depository" + # + # Keeping all of the logic here means the view layer and broadcaster only + # need to ask the object for its DOM id instead of rebuilding string + # fragments in multiple places. + def dom_id(tab: nil, mobile: false) + parts = [] + parts << "mobile" if mobile + parts << (tab ? tab.to_s : classification.to_s) + parts << key + parts.compact.join("_") + end + + def key + accountable_type.to_s.underscore + end + + def total + accounts.sum(&:converted_balance) + end + + def weight + return 0 if classification_group.total.zero? + + total / classification_group.total.to_d * 100 + end + + def syncing? + accounts.any?(&:syncing?) + end + + # "asset" or "liability" + def classification + classification_group.classification + end + + def currency + classification_group.currency + end + + private + attr_reader :classification_group +end diff --git a/app/models/balance_sheet/account_totals.rb b/app/models/balance_sheet/account_totals.rb new file mode 100644 index 00000000..ee3f1718 --- /dev/null +++ b/app/models/balance_sheet/account_totals.rb @@ -0,0 +1,63 @@ +class BalanceSheet::AccountTotals + def initialize(family, sync_status_monitor:) + @family = family + @sync_status_monitor = sync_status_monitor + end + + def asset_accounts + @asset_accounts ||= account_rows.filter { |t| t.classification == "asset" } + end + + def liability_accounts + @liability_accounts ||= account_rows.filter { |t| t.classification == "liability" } + end + + private + attr_reader :family, :sync_status_monitor + + AccountRow = Data.define(:account, :converted_balance, :is_syncing) do + def syncing? = is_syncing + + # Allows Rails path helpers to generate URLs from the wrapper + def to_param = account.to_param + delegate_missing_to :account + end + + def active_accounts + @active_accounts ||= family.accounts.active.with_attached_logo + end + + def account_rows + @account_rows ||= query.map do |account_row| + AccountRow.new( + account: account_row, + converted_balance: account_row.converted_balance, + is_syncing: sync_status_monitor.account_syncing?(account_row) + ) + end + end + + def cache_key + family.build_cache_key( + "balance_sheet_account_rows", + invalidate_on_data_updates: true + ) + end + + def query + @query ||= Rails.cache.fetch(cache_key) do + active_accounts + .joins(ActiveRecord::Base.sanitize_sql_array([ + "LEFT JOIN exchange_rates ON exchange_rates.date = ? AND accounts.currency = exchange_rates.from_currency AND exchange_rates.to_currency = ?", + Date.current, + family.currency + ])) + .select( + "accounts.*", + "SUM(accounts.balance * COALESCE(exchange_rates.rate, 1)) as converted_balance" + ) + .group(:classification, :accountable_type, :id) + .to_a + end + end +end diff --git a/app/models/balance_sheet/classification_group.rb b/app/models/balance_sheet/classification_group.rb new file mode 100644 index 00000000..a6d82bb3 --- /dev/null +++ b/app/models/balance_sheet/classification_group.rb @@ -0,0 +1,61 @@ +class BalanceSheet::ClassificationGroup + include Monetizable + + monetize :total, as: :total_money + + attr_reader :classification, :currency + + def initialize(classification:, currency:, accounts:) + @classification = normalize_classification!(classification) + @name = name + @currency = currency + @accounts = accounts + end + + def name + classification.titleize.pluralize + end + + def icon + classification == "asset" ? "plus" : "minus" + end + + def total + accounts.sum(&:converted_balance) + end + + def syncing? + accounts.any?(&:syncing?) + end + + # For now, we group by accountable type. This can be extended in the future to support arbitrary user groupings. + def account_groups + groups = accounts.group_by(&:accountable_type) + .transform_keys { |at| Accountable.from_type(at) } + .map do |accountable, account_rows| + BalanceSheet::AccountGroup.new( + name: accountable.display_name, + color: accountable.color, + accountable_type: accountable, + accounts: account_rows, + classification_group: self + ) + end + + # Sort the groups using the manual order defined by Accountable::TYPES so that + # the UI displays account groups in a predictable, domain-specific sequence. + groups.sort_by do |group| + manual_order = Accountable::TYPES + type_name = group.key.camelize + manual_order.index(type_name) || Float::INFINITY + end + end + + private + attr_reader :accounts + + def normalize_classification!(classification) + raise ArgumentError, "Invalid classification: #{classification}" unless %w[asset liability].include?(classification) + classification + end +end diff --git a/app/models/balance_sheet/net_worth_series_builder.rb b/app/models/balance_sheet/net_worth_series_builder.rb new file mode 100644 index 00000000..c4c79971 --- /dev/null +++ b/app/models/balance_sheet/net_worth_series_builder.rb @@ -0,0 +1,38 @@ +class BalanceSheet::NetWorthSeriesBuilder + def initialize(family) + @family = family + end + + def net_worth_series(period: Period.last_30_days) + Rails.cache.fetch(cache_key(period)) do + builder = Balance::ChartSeriesBuilder.new( + account_ids: active_account_ids, + currency: family.currency, + period: period, + favorable_direction: "up" + ) + + builder.balance_series + end + end + + private + attr_reader :family + + def active_account_ids + @active_account_ids ||= family.accounts.active.with_attached_logo.pluck(:id) + end + + def cache_key(period) + key = [ + "balance_sheet_net_worth_series", + period.start_date, + period.end_date + ].compact.join("_") + + family.build_cache_key( + key, + invalidate_on_data_updates: true + ) + end +end diff --git a/app/models/balance_sheet/sync_status_monitor.rb b/app/models/balance_sheet/sync_status_monitor.rb new file mode 100644 index 00000000..5682bd63 --- /dev/null +++ b/app/models/balance_sheet/sync_status_monitor.rb @@ -0,0 +1,35 @@ +class BalanceSheet::SyncStatusMonitor + def initialize(family) + @family = family + end + + def syncing? + syncing_account_ids.any? + end + + def account_syncing?(account) + syncing_account_ids.include?(account.id) + end + + private + attr_reader :family + + def syncing_account_ids + Rails.cache.fetch(cache_key) do + Sync.visible + .where(syncable_type: "Account", syncable_id: family.accounts.active.pluck(:id)) + .pluck(:syncable_id) + .to_set + end + end + + # We re-fetch the set of syncing IDs any time a sync that belongs to the family is started or completed. + # This ensures we're always fetching the latest sync statuses without re-querying on every page load in idle times (no syncs happening). + def cache_key + [ + "balance_sheet_sync_status", + family.id, + family.latest_sync_activity_at + ].join("_") + end +end diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 72556bf7..739d5381 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -6,7 +6,7 @@ module Syncable end def syncing? - raise NotImplementedError, "Subclasses must implement the syncing? method" + syncs.visible.any? end # Schedules a sync for syncable. If there is an existing sync pending/syncing for this syncable, diff --git a/app/models/family.rb b/app/models/family.rb index a4b2c8b1..20ad02a4 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -35,25 +35,6 @@ class Family < ApplicationRecord validates :locale, inclusion: { in: I18n.available_locales.map(&:to_s) } validates :date_format, inclusion: { in: DATE_FORMATS.map(&:last) } - # If any accounts or plaid items are syncing, the family is also syncing, even if a formal "Family Sync" is not running. - def syncing? - # Check for any in-progress syncs that belong directly to the family, to one of the - # family's accounts, or to one of the family's Plaid items. By moving the `visible` - # scope to the beginning we narrow down the candidate rows **before** performing the - # joins and by explicitly constraining the `syncable_type` for the direct Family - # match we allow Postgres to use the composite index on `(syncable_type, syncable_id)`. - Sync.visible - .joins("LEFT JOIN accounts ON accounts.id = syncs.syncable_id AND syncs.syncable_type = 'Account'") - .joins("LEFT JOIN plaid_items ON plaid_items.id = syncs.syncable_id AND syncs.syncable_type = 'PlaidItem'") - .where( - "(syncs.syncable_type = 'Family' AND syncs.syncable_id = :family_id) OR " \ - "accounts.family_id = :family_id OR " \ - "plaid_items.family_id = :family_id", - family_id: id - ) - .exists? - end - def assigned_merchants merchant_ids = transactions.where.not(merchant_id: nil).pluck(:merchant_id).uniq Merchant.where(id: merchant_ids) @@ -110,13 +91,15 @@ class Family < ApplicationRecord entries.order(:date).first&.date || Date.current end - # Cache key that is invalidated when any of the family's entries are updated (which affect rollups and other calculations) - def build_cache_key(key) + def build_cache_key(key, invalidate_on_data_updates: false) + # Our data sync process updates this timestamp whenever any family account successfully completes a data update. + # By including it in the cache key, we can expire caches every time family account data changes. + data_invalidation_key = invalidate_on_data_updates ? latest_sync_completed_at : nil + [ - "family", id, key, - entries.maximum(:updated_at) + data_invalidation_key ].compact.join("_") end diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index 25aa3e18..19970ce4 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -46,14 +46,6 @@ class PlaidItem < ApplicationRecord DestroyJob.perform_later(self) end - def syncing? - Sync.joins("LEFT JOIN accounts a ON a.id = syncs.syncable_id AND syncs.syncable_type = 'Account'") - .joins("LEFT JOIN plaid_accounts pa ON pa.id = a.plaid_account_id") - .where("syncs.syncable_id = ? OR pa.plaid_item_id = ?", id, id) - .visible - .exists? - end - def import_latest_plaid_data PlaidItem::Importer.new(self, plaid_provider: plaid_provider).import end diff --git a/app/models/sync.rb b/app/models/sync.rb index 37c05dfa..7baf9e63 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -29,13 +29,13 @@ class Sync < ApplicationRecord state :failed state :stale - after_all_transitions :log_status_change + after_all_transitions :handle_transition - event :start, after_commit: :report_warnings do + event :start, after_commit: :handle_start_transition do transitions from: :pending, to: :syncing end - event :complete do + event :complete, after_commit: :handle_completion_transition do transitions from: :syncing, to: :completed end @@ -163,9 +163,30 @@ class Sync < ApplicationRecord end end + def handle_start_transition + report_warnings + end + + def handle_transition + log_status_change + family.touch(:latest_sync_activity_at) + end + + def handle_completion_transition + family.touch(:latest_sync_completed_at) + end + def window_valid if window_start_date && window_end_date && window_start_date > window_end_date errors.add(:window_end_date, "must be greater than window_start_date") end end + + def family + if syncable.is_a?(Family) + syncable + else + syncable.family + end + end end diff --git a/app/views/accountable_sparklines/_error.html.erb b/app/views/accountable_sparklines/_error.html.erb deleted file mode 100644 index f43f609f..00000000 --- a/app/views/accountable_sparklines/_error.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -<%= turbo_frame_tag "#{params[:accountable_type]}_sparkline" do %> -
-
- <%= icon("alert-triangle", size: "sm", class: "text-warning") %> -
-

Error

-
-<% end %> diff --git a/app/views/accountable_sparklines/show.html.erb b/app/views/accountable_sparklines/show.html.erb index 236c3616..c090551a 100644 --- a/app/views/accountable_sparklines/show.html.erb +++ b/app/views/accountable_sparklines/show.html.erb @@ -1,13 +1,11 @@ -<% cache Current.family.build_cache_key("#{@accountable.name}_sparkline_html") do %> - <%= turbo_frame_tag "#{@accountable.model_name.param_key}_sparkline" do %> -
-
- <%= render "shared/sparkline", id: dom_id(@accountable, :sparkline_chart), series: @series %> -
+<%= turbo_frame_tag "#{@accountable.model_name.param_key}_sparkline" do %> +
+
+ <%= render "shared/sparkline", id: dom_id(@accountable, :sparkline_chart), series: @series %> +
- <%= tag.p @series.trend.percent_formatted, + <%= tag.p @series.trend.percent_formatted, style: "color: #{@series.trend.color}", class: "font-mono text-right text-xs font-medium text-primary" %> -
- <% end %> +
<% end %> diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index 056e7be1..182bdd6e 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -41,7 +41,7 @@ ) %>
- <% family.balance_sheet.account_groups("asset").each do |group| %> + <% family.balance_sheet.assets.account_groups.each do |group| %> <%= render "accounts/accountable_group", account_group: group, mobile: mobile %> <% end %>
@@ -61,7 +61,7 @@ ) %>
- <% family.balance_sheet.account_groups("liability").each do |group| %> + <% family.balance_sheet.liabilities.account_groups.each do |group| %> <%= render "accounts/accountable_group", account_group: group, mobile: mobile %> <% end %>
@@ -82,7 +82,7 @@
<% family.balance_sheet.account_groups.each do |group| %> - <%= render "accounts/accountable_group", account_group: group, mobile: mobile %> + <%= render "accounts/accountable_group", account_group: group, mobile: mobile, all_tab: true %> <% end %>
diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index b4495afc..26b69be3 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -1,24 +1,21 @@ -<%# locals: (account_group:, mobile: false, open: nil, **args) %> +<%# locals: (account_group:, mobile: false, all_tab: false, open: nil, **args) %> -
"> +
<% is_open = open.nil? ? account_group.accounts.any? { |account| page_active?(account_path(account)) } : open %> <%= render DisclosureComponent.new(title: account_group.name, align: :left, open: is_open) do |disclosure| %> <% disclosure.with_summary_content do %> + <% if account_group.syncing? %> +
+ <%= render partial: "shared/sync_indicator", locals: { size: "xs" } %> +
+ <% end %> +
- <% if account_group.syncing? %> -
-
-
-
-
+ <%= tag.p format_money(account_group.total_money), class: "text-sm font-medium text-primary" %> + <%= turbo_frame_tag "#{account_group.key}_sparkline", src: accountable_sparkline_path(account_group.key), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %> +
+
- <% else %> - <%= tag.p format_money(account_group.total_money), class: "text-sm font-medium text-primary" %> - <%= turbo_frame_tag "#{account_group.key}_sparkline", src: accountable_sparkline_path(account_group.key), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %> -
-
-
- <% end %> <% end %>
<% end %> @@ -34,29 +31,23 @@ <%= render "accounts/logo", account: account, size: "sm", color: account_group.color %>
- <%= tag.p account.name, class: "text-sm text-primary font-medium mb-0.5 truncate" %> +
+ <%= tag.p account.name, class: "text-sm text-primary font-medium truncate" %> + <% if account.syncing? %> + <%= render partial: "shared/sync_indicator", locals: { size: "xs" } %> + <% end %> +
<%= tag.p account.short_subtype_label, class: "text-sm text-secondary truncate" %>
- <% if account.syncing? %> -
-
-
-
-
-
+
+ <%= tag.p format_money(account.balance_money), class: "text-sm font-medium text-primary whitespace-nowrap" %> + <%= turbo_frame_tag dom_id(account, :sparkline), src: sparkline_account_path(account), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %> +
+
-
- <% else %> -
- <%= tag.p format_money(account.balance_money), class: "text-sm font-medium text-primary whitespace-nowrap" %> - <%= turbo_frame_tag dom_id(account, :sparkline), src: sparkline_account_path(account), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %> -
-
-
- <% end %> -
- <% end %> + <% end %> +
<% end %> <% end %>
diff --git a/app/views/accounts/_sparkline_error.html.erb b/app/views/accounts/_sparkline_error.html.erb deleted file mode 100644 index dbee8dbf..00000000 --- a/app/views/accounts/_sparkline_error.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -<%= turbo_frame_tag dom_id(@account, :sparkline) do %> -
-
- <%= icon("alert-triangle", size: "sm", class: "text-warning") %> -
-

Error

-
-<% end %> diff --git a/app/views/accounts/chart.html.erb b/app/views/accounts/chart.html.erb index b181ef20..11dcbaac 100644 --- a/app/views/accounts/chart.html.erb +++ b/app/views/accounts/chart.html.erb @@ -2,25 +2,21 @@ <% trend = series.trend %> <%= turbo_frame_tag dom_id(@account, :chart_details) do %> - <% if @account.syncing? %> - <%= render "accounts/chart_loader" %> - <% else %> -
- <%= render partial: "shared/trend_change", locals: { trend: trend, comparison_label: @period.comparison_label } %> -
+
+ <%= render partial: "shared/trend_change", locals: { trend: trend, comparison_label: @period.comparison_label } %> +
-
- <% if series.any? %> -
+ <% if series.any? %> +
- <% else %> -
-

<%= t(".data_not_available") %>

-
- <% end %> -
- <% end %> + <% else %> +
+

<%= t(".data_not_available") %>

+
+ <% end %> +
<% end %> diff --git a/app/views/accounts/show/_chart.html.erb b/app/views/accounts/show/_chart.html.erb index bf23efd2..00506be2 100644 --- a/app/views/accounts/show/_chart.html.erb +++ b/app/views/accounts/show/_chart.html.erb @@ -9,18 +9,14 @@
<%= tag.p account.investment? ? "Total value" : default_value_title, class: "text-sm font-medium text-secondary" %> - <% if !account.syncing? && account.investment? %> + <% if account.investment? %> <%= render "investments/value_tooltip", balance: account.balance_money, holdings: account.balance_money - account.cash_balance_money, cash: account.cash_balance_money %> <% end %>
- <% if account.syncing? %> -
- <% else %> - <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %> - <% if account.currency != Current.family.currency %> - <%= tag.p format_money(account.balance_money.exchange_to(Current.family.currency, fallback_rate: 1)), class: "text-sm font-medium text-secondary" %> - <% end %> + <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %> + <% if account.currency != Current.family.currency %> + <%= tag.p format_money(account.balance_money.exchange_to(Current.family.currency, fallback_rate: 1)), class: "text-sm font-medium text-secondary" %> <% end %>
diff --git a/app/views/accounts/show/_header.html.erb b/app/views/accounts/show/_header.html.erb index 35dcc2ed..283b4e05 100644 --- a/app/views/accounts/show/_header.html.erb +++ b/app/views/accounts/show/_header.html.erb @@ -10,10 +10,16 @@
<%= render "accounts/logo", account: account %> -
-

<%= title || account.name %>

- <% if subtitle.present? %> -

<%= subtitle %>

+
+
+

<%= title || account.name %>

+ <% if subtitle.present? %> +

<%= subtitle %>

+ <% end %> +
+ + <% if account.syncing? %> + <%= render partial: "shared/sync_indicator", locals: { size: "sm" } %> <% end %>
diff --git a/app/views/accounts/sparkline.html.erb b/app/views/accounts/sparkline.html.erb index 5eb8aa04..0a07c653 100644 --- a/app/views/accounts/sparkline.html.erb +++ b/app/views/accounts/sparkline.html.erb @@ -1,13 +1,11 @@ -<% cache Current.family.build_cache_key("account_#{@account.id}_sparkline_html") do %> - <%= turbo_frame_tag dom_id(@account, :sparkline) do %> -
-
- <%= render "shared/sparkline", id: dom_id(@account, :sparkline_chart), series: @sparkline_series %> -
+<%= turbo_frame_tag dom_id(@account, :sparkline) do %> +
+
+ <%= render "shared/sparkline", id: dom_id(@account, :sparkline_chart), series: @sparkline_series %> +
- <%= tag.p @sparkline_series.trend.percent_formatted, + <%= tag.p @sparkline_series.trend.percent_formatted, style: "color: #{@sparkline_series.trend.color}", class: "font-mono text-right text-xs font-medium text-primary" %> -
- <% end %> +
<% end %> diff --git a/app/views/holdings/_cash.html.erb b/app/views/holdings/_cash.html.erb index 702f40d0..98ce1d3a 100644 --- a/app/views/holdings/_cash.html.erb +++ b/app/views/holdings/_cash.html.erb @@ -20,12 +20,8 @@
<% cash_weight = account.balance.zero? ? 0 : account.cash_balance / account.balance * 100 %> - <% if account.syncing? %> -
- <% else %> - <%= render "shared/progress_circle", progress: cash_weight %> - <%= tag.p number_to_percentage(cash_weight, precision: 1) %> - <% end %> + <%= render "shared/progress_circle", progress: cash_weight %> + <%= tag.p number_to_percentage(cash_weight, precision: 1) %>
@@ -33,13 +29,7 @@
- <% if account.syncing? %> -
-
-
- <% else %> - <%= tag.p format_money account.cash_balance_money %> - <% end %> + <%= tag.p format_money account.cash_balance_money %>
diff --git a/app/views/holdings/_holding.html.erb b/app/views/holdings/_holding.html.erb index c8a2ac59..5fe0e4c9 100644 --- a/app/views/holdings/_holding.html.erb +++ b/app/views/holdings/_holding.html.erb @@ -17,9 +17,7 @@
- <% if holding.account.syncing? %> -
- <% elsif holding.weight %> + <% if holding.weight %> <%= render "shared/progress_circle", progress: holding.weight %> <%= tag.p number_to_percentage(holding.weight, precision: 1) %> <% else %> @@ -28,39 +26,21 @@
- <% if holding.account.syncing? %> -
-
-
- <% else %> - <%= tag.p format_money holding.avg_cost %> - <%= tag.p t(".per_share"), class: "font-normal text-secondary" %> - <% end %> + <%= tag.p format_money holding.avg_cost %> + <%= tag.p t(".per_share"), class: "font-normal text-secondary" %>
- <% if holding.account.syncing? %> -
-
-
-
+ <% if holding.amount_money %> + <%= tag.p format_money holding.amount_money %> <% else %> - <% if holding.amount_money %> - <%= tag.p format_money holding.amount_money %> - <% else %> - <%= tag.p "--", class: "text-secondary" %> - <% end %> - <%= tag.p t(".shares", qty: number_with_precision(holding.qty, precision: 1)), class: "font-normal text-secondary" %> + <%= tag.p "--", class: "text-secondary" %> <% end %> + <%= tag.p t(".shares", qty: number_with_precision(holding.qty, precision: 1)), class: "font-normal text-secondary" %>
- <% if holding.account.syncing? %> -
-
-
-
- <% elsif holding.trend %> + <% if holding.trend %> <%= tag.p format_money(holding.trend.value), style: "color: #{holding.trend.color};" %> <%= tag.p "(#{number_to_percentage(holding.trend.percent, precision: 1)})", style: "color: #{holding.trend.color};" %> <% else %> diff --git a/app/views/pages/dashboard/_balance_sheet.html.erb b/app/views/pages/dashboard/_balance_sheet.html.erb index 62a5077a..b0687042 100644 --- a/app/views/pages/dashboard/_balance_sheet.html.erb +++ b/app/views/pages/dashboard/_balance_sheet.html.erb @@ -3,26 +3,24 @@
<% balance_sheet.classification_groups.each do |classification_group| %>
-

- - <%= classification_group.display_name %> - +
+

+ + <%= classification_group.name %> + - <% if classification_group.account_groups.any? %> - · - - <% if classification_group.syncing? %> -
-
-
- <% else %> + <% if classification_group.account_groups.any? %> + · <%= classification_group.total_money.format(precision: 0) %> <% end %> +

+ + <% if classification_group.syncing? %> + <%= render partial: "shared/sync_indicator", locals: { size: "sm" } %> <% end %> -

+
<% if classification_group.account_groups.any? %> -
<% classification_group.account_groups.each do |account_group| %> @@ -30,19 +28,15 @@ <% end %>
- <% if classification_group.syncing? %> -

Calculating latest balance data...

- <% else %> -
- <% classification_group.account_groups.each do |account_group| %> -
-
-

<%= account_group.name %>

-

<%= number_to_percentage(account_group.weight, precision: 0) %>

-
- <% end %> -
- <% end %> +
+ <% classification_group.account_groups.each do |account_group| %> +
+
+

<%= account_group.name %>

+

<%= number_to_percentage(account_group.weight, precision: 0) %>

+
+ <% end %> +
@@ -71,27 +65,15 @@

<%= account_group.name %>

- <% if account_group.syncing? %> -
-
-
-
- -
-
-
+
+
+ <%= render "pages/dashboard/group_weight", weight: account_group.weight, color: account_group.color %>
- <% else %> -
-
- <%= render "pages/dashboard/group_weight", weight: account_group.weight, color: account_group.color %> -
-
-

<%= format_money(account_group.total_money) %>

-
+
+

<%= format_money(account_group.total_money) %>

- <% end %> +
@@ -103,32 +85,20 @@ <%= link_to account.name, account_path(account) %>
- <% if account.syncing? %> -
-
-
-
- -
-
-
-
- <% else %> -
-
- <% +
+
+ <% # Calculate weight as percentage of classification total classification_total = classification_group.total_money.amount account_weight = classification_total.zero? ? 0 : account.converted_balance / classification_total * 100 %> - <%= render "pages/dashboard/group_weight", weight: account_weight, color: account_group.color %> -
- -
-

<%= format_money(account.balance_money) %>

-
+ <%= render "pages/dashboard/group_weight", weight: account_weight, color: account_group.color %>
- <% end %> + +
+

<%= format_money(account.balance_money) %>

+
+
<% if idx < account_group.accounts.size - 1 %> diff --git a/app/views/pages/dashboard/_net_worth_chart.html.erb b/app/views/pages/dashboard/_net_worth_chart.html.erb index d526247b..fabc0267 100644 --- a/app/views/pages/dashboard/_net_worth_chart.html.erb +++ b/app/views/pages/dashboard/_net_worth_chart.html.erb @@ -5,23 +5,22 @@
-

<%= t(".title") %>

+
+

<%= t(".title") %>

- <% if balance_sheet.syncing? %> -
-
-
-
- <% else %> -

- <%= series.trend.current.format %> -

- - <% if series.trend.nil? %> -

<%= t(".data_not_available") %>

- <% else %> - <%= render partial: "shared/trend_change", locals: { trend: series.trend, comparison_label: period.comparison_label } %> + <% if balance_sheet.syncing? %> + <%= render partial: "shared/sync_indicator", locals: { size: "sm" } %> <% end %> +
+ +

+ <%= series.trend.current.format %> +

+ + <% if series.trend.nil? %> +

<%= t(".data_not_available") %>

+ <% else %> + <%= render partial: "shared/trend_change", locals: { trend: series.trend, comparison_label: period.comparison_label } %> <% end %>
@@ -35,21 +34,16 @@ <% end %>
- <% if balance_sheet.syncing? %> -
-
-
- <% else %> - <% if series.any? %> -
+
- <% else %> -
-

<%= t(".data_not_available") %>

-
- <% end %> + <% else %> +
+

<%= t(".data_not_available") %>

+
<% end %> +
diff --git a/app/views/shared/_sync_indicator.html.erb b/app/views/shared/_sync_indicator.html.erb new file mode 100644 index 00000000..2ef56bf5 --- /dev/null +++ b/app/views/shared/_sync_indicator.html.erb @@ -0,0 +1,5 @@ +<%# locals: (size: "md") %> + +
+ <%= icon "loader-circle", color: "current", size: size %> +
diff --git a/db/migrate/20250610181219_add_sync_timestamps_to_family.rb b/db/migrate/20250610181219_add_sync_timestamps_to_family.rb new file mode 100644 index 00000000..8d5f7cab --- /dev/null +++ b/db/migrate/20250610181219_add_sync_timestamps_to_family.rb @@ -0,0 +1,6 @@ +class AddSyncTimestampsToFamily < ActiveRecord::Migration[7.2] + def change + add_column :families, :latest_sync_activity_at, :datetime, default: -> { "CURRENT_TIMESTAMP" } + add_column :families, :latest_sync_completed_at, :datetime, default: -> { "CURRENT_TIMESTAMP" } + end +end diff --git a/db/schema.rb b/db/schema.rb index c38c5375..9c10112c 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: 2025_06_05_031616) do +ActiveRecord::Schema[7.2].define(version: 2025_06_10_181219) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -228,6 +228,8 @@ ActiveRecord::Schema[7.2].define(version: 2025_06_05_031616) do t.boolean "data_enrichment_enabled", default: false t.boolean "early_access", default: false t.boolean "auto_sync_on_login", default: true, null: false + t.datetime "latest_sync_activity_at", default: -> { "CURRENT_TIMESTAMP" } + t.datetime "latest_sync_completed_at", default: -> { "CURRENT_TIMESTAMP" } end create_table "holdings", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/test/controllers/accountable_sparklines_controller_test.rb b/test/controllers/accountable_sparklines_controller_test.rb index b43c09e0..51ec49f3 100644 --- a/test/controllers/accountable_sparklines_controller_test.rb +++ b/test/controllers/accountable_sparklines_controller_test.rb @@ -9,13 +9,4 @@ class AccountableSparklinesControllerTest < ActionDispatch::IntegrationTest get accountable_sparkline_url("depository") assert_response :success end - - test "should handle sparkline errors gracefully" do - # Mock an error in the balance_series method - Balance::ChartSeriesBuilder.any_instance.stubs(:balance_series).raises(StandardError.new("Test error")) - - get accountable_sparkline_url("depository") - assert_response :success - assert_match /Error/, response.body - end end diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 6c51c525..ba0b937e 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -25,13 +25,4 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest get sparkline_account_url(@account) assert_response :success end - - test "should handle sparkline errors gracefully" do - # Mock an error in the balance_series method to bypass the rescue in sparkline_series - Balance::ChartSeriesBuilder.any_instance.stubs(:balance_series).raises(StandardError.new("Test error")) - - get sparkline_account_url(@account) - assert_response :success - assert_match /Error/, response.body -end end diff --git a/test/models/balance_sheet_test.rb b/test/models/balance_sheet_test.rb index fb131ea7..c9478979 100644 --- a/test/models/balance_sheet_test.rb +++ b/test/models/balance_sheet_test.rb @@ -6,23 +6,23 @@ class BalanceSheetTest < ActiveSupport::TestCase end test "calculates total assets" do - assert_equal 0, BalanceSheet.new(@family).total_assets + assert_equal 0, BalanceSheet.new(@family).assets.total create_account(balance: 1000, accountable: Depository.new) create_account(balance: 5000, accountable: OtherAsset.new) create_account(balance: 10000, accountable: CreditCard.new) # ignored - assert_equal 1000 + 5000, BalanceSheet.new(@family).total_assets + assert_equal 1000 + 5000, BalanceSheet.new(@family).assets.total end test "calculates total liabilities" do - assert_equal 0, BalanceSheet.new(@family).total_liabilities + assert_equal 0, BalanceSheet.new(@family).liabilities.total create_account(balance: 1000, accountable: CreditCard.new) create_account(balance: 5000, accountable: OtherLiability.new) create_account(balance: 10000, accountable: Depository.new) # ignored - assert_equal 1000 + 5000, BalanceSheet.new(@family).total_liabilities + assert_equal 1000 + 5000, BalanceSheet.new(@family).liabilities.total end test "calculates net worth" do @@ -42,8 +42,8 @@ class BalanceSheetTest < ActiveSupport::TestCase other_liability.update!(is_active: false) assert_equal 10000 - 1000, BalanceSheet.new(@family).net_worth - assert_equal 10000, BalanceSheet.new(@family).total_assets - assert_equal 1000, BalanceSheet.new(@family).total_liabilities + assert_equal 10000, BalanceSheet.new(@family).assets.total + assert_equal 1000, BalanceSheet.new(@family).liabilities.total end test "calculates asset group totals" do @@ -53,7 +53,7 @@ class BalanceSheetTest < ActiveSupport::TestCase create_account(balance: 5000, accountable: OtherAsset.new) create_account(balance: 10000, accountable: CreditCard.new) # ignored - asset_groups = BalanceSheet.new(@family).account_groups("asset") + asset_groups = BalanceSheet.new(@family).assets.account_groups assert_equal 3, asset_groups.size assert_equal 1000 + 2000, asset_groups.find { |ag| ag.name == "Cash" }.total @@ -68,7 +68,7 @@ class BalanceSheetTest < ActiveSupport::TestCase create_account(balance: 5000, accountable: OtherLiability.new) create_account(balance: 10000, accountable: Depository.new) # ignored - liability_groups = BalanceSheet.new(@family).account_groups("liability") + liability_groups = BalanceSheet.new(@family).liabilities.account_groups assert_equal 2, liability_groups.size assert_equal 1000 + 2000, liability_groups.find { |ag| ag.name == "Credit Cards" }.total