From c8b1e1d059b263800a99f7159ec32d08ad800b41 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sat, 10 May 2025 10:28:00 -0400 Subject: [PATCH 01/27] PlaidConnectable concern --- app/controllers/plaid_items_controller.rb | 4 +- app/models/family.rb | 62 +------------------ app/models/family/plaid_connectable.rb | 51 +++++++++++++++ app/models/family/syncable.rb | 46 ++++++++++++++ app/models/plaid_item.rb | 15 ----- .../plaid_items_controller_test.rb | 2 +- 6 files changed, 101 insertions(+), 79 deletions(-) create mode 100644 app/models/family/plaid_connectable.rb create mode 100644 app/models/family/syncable.rb diff --git a/app/controllers/plaid_items_controller.rb b/app/controllers/plaid_items_controller.rb index 37efd5e3..8812cf0f 100644 --- a/app/controllers/plaid_items_controller.rb +++ b/app/controllers/plaid_items_controller.rb @@ -2,8 +2,8 @@ class PlaidItemsController < ApplicationController before_action :set_plaid_item, only: %i[destroy sync] def create - Current.family.plaid_items.create_from_public_token( - plaid_item_params[:public_token], + Current.family.create_plaid_item!( + public_token: plaid_item_params[:public_token], item_name: item_name, region: plaid_item_params[:region] ) diff --git a/app/models/family.rb b/app/models/family.rb index f565d54d..0df7fd12 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -1,5 +1,5 @@ class Family < ApplicationRecord - include Syncable, AutoTransferMatchable, Subscribeable + include PlaidConnectable, Syncable, AutoTransferMatchable, Subscribeable DATE_FORMATS = [ [ "MM-DD-YYYY", "%m-%d-%Y" ], @@ -15,7 +15,6 @@ class Family < ApplicationRecord has_many :users, dependent: :destroy has_many :accounts, dependent: :destroy - has_many :plaid_items, dependent: :destroy has_many :invitations, dependent: :destroy has_many :imports, dependent: :destroy @@ -65,69 +64,10 @@ class Family < ApplicationRecord @income_statement ||= IncomeStatement.new(self) end - def sync_data(sync, start_date: nil) - # We don't rely on this value to guard the app, but keep it eventually consistent - sync_trial_status! - - Rails.logger.info("Syncing accounts for family #{id}") - accounts.manual.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Syncing plaid items for family #{id}") - plaid_items.each do |plaid_item| - plaid_item.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Applying rules for family #{id}") - rules.each do |rule| - rule.apply_later - end - end - - def remove_syncing_notice! - broadcast_remove target: "syncing-notice" - end - - def post_sync(sync) - auto_match_transfers! - broadcast_refresh - end - - # If family has any syncs pending/syncing within the last 10 minutes, we show a persistent "syncing" notice. - # Ignore syncs older than 10 minutes as they are considered "stale" - def syncing? - Sync.where( - "(syncable_type = 'Family' AND syncable_id = ?) OR - (syncable_type = 'Account' AND syncable_id IN (SELECT id FROM accounts WHERE family_id = ? AND plaid_account_id IS NULL)) OR - (syncable_type = 'PlaidItem' AND syncable_id IN (SELECT id FROM plaid_items WHERE family_id = ?))", - id, id, id - ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? - end - def eu? country != "US" && country != "CA" end - def get_link_token(webhooks_url:, redirect_url:, accountable_type: nil, region: :us, access_token: nil) - provider = if region.to_sym == :eu - Provider::Registry.get_provider(:plaid_eu) - else - Provider::Registry.get_provider(:plaid_us) - end - - # early return when no provider - return nil unless provider - - provider.get_link_token( - user_id: id, - webhooks_url: webhooks_url, - redirect_url: redirect_url, - accountable_type: accountable_type, - access_token: access_token - ).link_token - end - def requires_data_provider? # If family has any trades, they need a provider for historical prices return true if trades.any? diff --git a/app/models/family/plaid_connectable.rb b/app/models/family/plaid_connectable.rb new file mode 100644 index 00000000..f2a997c8 --- /dev/null +++ b/app/models/family/plaid_connectable.rb @@ -0,0 +1,51 @@ +module Family::PlaidConnectable + extend ActiveSupport::Concern + + included do + has_many :plaid_items, dependent: :destroy + end + + def create_plaid_item!(public_token:, item_name:, region:) + provider = plaid_provider_for_region(region) + + public_token_response = provider.exchange_public_token(public_token) + + plaid_item = plaid_items.create!( + name: item_name, + plaid_id: public_token_response.item_id, + access_token: public_token_response.access_token, + plaid_region: region + ) + + plaid_item.sync_later + + plaid_item + end + + def get_link_token(webhooks_url:, redirect_url:, accountable_type: nil, region: :us, access_token: nil) + return nil unless plaid_us || plaid_eu + + provider = plaid_provider_for_region(region) + + provider.get_link_token( + user_id: self.id, + webhooks_url: webhooks_url, + redirect_url: redirect_url, + accountable_type: accountable_type, + access_token: access_token + ).link_token + end + + private + def plaid_us + @plaid ||= Provider::Registry.get_provider(:plaid_us) + end + + def plaid_eu + @plaid_eu ||= Provider::Registry.get_provider(:plaid_eu) + end + + def plaid_provider_for_region(region) + region.to_sym == :eu ? plaid_eu : plaid_us + end +end diff --git a/app/models/family/syncable.rb b/app/models/family/syncable.rb new file mode 100644 index 00000000..9b1c276a --- /dev/null +++ b/app/models/family/syncable.rb @@ -0,0 +1,46 @@ +module Family::Syncable + extend ActiveSupport::Concern + + # Top-level, generic Syncable concern + include ::Syncable + + def sync_data(sync, start_date: nil) + # We don't rely on this value to guard the app, but keep it eventually consistent + sync_trial_status! + + Rails.logger.info("Syncing accounts for family #{id}") + accounts.manual.each do |account| + account.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Syncing plaid items for family #{id}") + plaid_items.each do |plaid_item| + plaid_item.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Applying rules for family #{id}") + rules.each do |rule| + rule.apply_later + end + end + + def remove_syncing_notice! + broadcast_remove target: "syncing-notice" + end + + def post_sync(sync) + auto_match_transfers! + broadcast_refresh + end + + # If family has any syncs pending/syncing within the last 10 minutes, we show a persistent "syncing" notice. + # Ignore syncs older than 10 minutes as they are considered "stale" + def syncing? + Sync.where( + "(syncable_type = 'Family' AND syncable_id = ?) OR + (syncable_type = 'Account' AND syncable_id IN (SELECT id FROM accounts WHERE family_id = ? AND plaid_account_id IS NULL)) OR + (syncable_type = 'PlaidItem' AND syncable_id IN (SELECT id FROM plaid_items WHERE family_id = ?))", + id, id, id + ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? + end +end diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index 2226f12f..9bf91d70 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -22,21 +22,6 @@ class PlaidItem < ApplicationRecord scope :ordered, -> { order(created_at: :desc) } scope :needs_update, -> { where(status: :requires_update) } - class << self - def create_from_public_token(token, item_name:, region:) - response = plaid_provider_for_region(region).exchange_public_token(token) - - new_plaid_item = create!( - name: item_name, - plaid_id: response.item_id, - access_token: response.access_token, - plaid_region: region - ) - - new_plaid_item.sync_later - end - end - def sync_data(sync, start_date: nil) begin Rails.logger.info("Fetching and loading Plaid data") diff --git a/test/controllers/plaid_items_controller_test.rb b/test/controllers/plaid_items_controller_test.rb index 7aede85c..4cf8e10a 100644 --- a/test/controllers/plaid_items_controller_test.rb +++ b/test/controllers/plaid_items_controller_test.rb @@ -8,7 +8,7 @@ class PlaidItemsControllerTest < ActionDispatch::IntegrationTest test "create" do @plaid_provider = mock - PlaidItem.expects(:plaid_provider_for_region).with("us").returns(@plaid_provider) + Provider::Registry.expects(:get_provider).with(:plaid_us).returns(@plaid_provider) public_token = "public-sandbox-1234" -- 2.53.0 From 64147758a3cc5bb0d053ef4f7902e4fc9b5ce043 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sat, 10 May 2025 16:16:47 -0400 Subject: [PATCH 02/27] Remove bad abstraction --- app/models/balance/base_calculator.rb | 35 ------------- app/models/balance/forward_calculator.rb | 35 ++++++++++++- app/models/balance/reverse_calculator.rb | 35 ++++++++++++- app/models/holding/base_calculator.rb | 62 ------------------------ app/models/holding/forward_calculator.rb | 58 +++++++++++++++++++++- app/models/holding/reverse_calculator.rb | 54 ++++++++++++++++++++- 6 files changed, 178 insertions(+), 101 deletions(-) delete mode 100644 app/models/balance/base_calculator.rb delete mode 100644 app/models/holding/base_calculator.rb diff --git a/app/models/balance/base_calculator.rb b/app/models/balance/base_calculator.rb deleted file mode 100644 index 2d01dfe7..00000000 --- a/app/models/balance/base_calculator.rb +++ /dev/null @@ -1,35 +0,0 @@ -class Balance::BaseCalculator - attr_reader :account - - def initialize(account) - @account = account - end - - def calculate - Rails.logger.tagged(self.class.name) do - calculate_balances - end - end - - private - def sync_cache - @sync_cache ||= Balance::SyncCache.new(account) - end - - def build_balance(date, cash_balance, holdings_value) - Balance.new( - account_id: account.id, - date: date, - balance: holdings_value + cash_balance, - cash_balance: cash_balance, - currency: account.currency - ) - end - - def calculate_next_balance(prior_balance, transactions, direction: :forward) - flows = transactions.sum(&:amount) - negated = direction == :forward ? account.asset? : account.liability? - flows *= -1 if negated - prior_balance + flows - end -end diff --git a/app/models/balance/forward_calculator.rb b/app/models/balance/forward_calculator.rb index d024d2c6..4e6f2d5c 100644 --- a/app/models/balance/forward_calculator.rb +++ b/app/models/balance/forward_calculator.rb @@ -1,4 +1,16 @@ -class Balance::ForwardCalculator < Balance::BaseCalculator +class Balance::ForwardCalculator + attr_reader :account + + def initialize(account) + @account = account + end + + def calculate + Rails.logger.tagged("Balance::ForwardCalculator") do + calculate_balances + end + end + private def calculate_balances current_cash_balance = 0 @@ -25,4 +37,25 @@ class Balance::ForwardCalculator < Balance::BaseCalculator @balances end + + def sync_cache + @sync_cache ||= Balance::SyncCache.new(account) + end + + def build_balance(date, cash_balance, holdings_value) + Balance.new( + account_id: account.id, + date: date, + balance: holdings_value + cash_balance, + cash_balance: cash_balance, + currency: account.currency + ) + end + + def calculate_next_balance(prior_balance, transactions, direction: :forward) + flows = transactions.sum(&:amount) + negated = direction == :forward ? account.asset? : account.liability? + flows *= -1 if negated + prior_balance + flows + end end diff --git a/app/models/balance/reverse_calculator.rb b/app/models/balance/reverse_calculator.rb index 6a2ba70b..52a05608 100644 --- a/app/models/balance/reverse_calculator.rb +++ b/app/models/balance/reverse_calculator.rb @@ -1,4 +1,16 @@ -class Balance::ReverseCalculator < Balance::BaseCalculator +class Balance::ReverseCalculator + attr_reader :account + + def initialize(account) + @account = account + end + + def calculate + Rails.logger.tagged("Balance::ReverseCalculator") do + calculate_balances + end + end + private def calculate_balances current_cash_balance = account.cash_balance @@ -35,4 +47,25 @@ class Balance::ReverseCalculator < Balance::BaseCalculator @balances end + + def sync_cache + @sync_cache ||= Balance::SyncCache.new(account) + end + + def build_balance(date, cash_balance, holdings_value) + Balance.new( + account_id: account.id, + date: date, + balance: holdings_value + cash_balance, + cash_balance: cash_balance, + currency: account.currency + ) + end + + def calculate_next_balance(prior_balance, transactions, direction: :forward) + flows = transactions.sum(&:amount) + negated = direction == :forward ? account.asset? : account.liability? + flows *= -1 if negated + prior_balance + flows + end end diff --git a/app/models/holding/base_calculator.rb b/app/models/holding/base_calculator.rb deleted file mode 100644 index d9b85d03..00000000 --- a/app/models/holding/base_calculator.rb +++ /dev/null @@ -1,62 +0,0 @@ -class Holding::BaseCalculator - attr_reader :account - - def initialize(account) - @account = account - end - - def calculate - Rails.logger.tagged(self.class.name) do - holdings = calculate_holdings - Holding.gapfill(holdings) - end - end - - private - def portfolio_cache - @portfolio_cache ||= Holding::PortfolioCache.new(account) - end - - def empty_portfolio - securities = portfolio_cache.get_securities - securities.each_with_object({}) { |security, hash| hash[security.id] = 0 } - end - - def generate_starting_portfolio - empty_portfolio - end - - def transform_portfolio(previous_portfolio, trade_entries, direction: :forward) - new_quantities = previous_portfolio.dup - - trade_entries.each do |trade_entry| - trade = trade_entry.entryable - security_id = trade.security_id - qty_change = trade.qty - qty_change = qty_change * -1 if direction == :reverse - new_quantities[security_id] = (new_quantities[security_id] || 0) + qty_change - end - - new_quantities - end - - def build_holdings(portfolio, date, price_source: nil) - portfolio.map do |security_id, qty| - price = portfolio_cache.get_price(security_id, date, source: price_source) - - if price.nil? - next - end - - Holding.new( - account_id: account.id, - security_id: security_id, - date: date, - qty: qty, - price: price.price, - currency: price.currency, - amount: qty * price.price - ) - end.compact - end -end diff --git a/app/models/holding/forward_calculator.rb b/app/models/holding/forward_calculator.rb index d2f2e8d7..22759416 100644 --- a/app/models/holding/forward_calculator.rb +++ b/app/models/holding/forward_calculator.rb @@ -1,9 +1,65 @@ -class Holding::ForwardCalculator < Holding::BaseCalculator +class Holding::ForwardCalculator + attr_reader :account + + def initialize(account) + @account = account + end + + def calculate + Rails.logger.tagged("Holding::ForwardCalculator") do + holdings = calculate_holdings + Holding.gapfill(holdings) + end + end + private def portfolio_cache @portfolio_cache ||= Holding::PortfolioCache.new(account) end + def empty_portfolio + securities = portfolio_cache.get_securities + securities.each_with_object({}) { |security, hash| hash[security.id] = 0 } + end + + def generate_starting_portfolio + empty_portfolio + end + + def transform_portfolio(previous_portfolio, trade_entries, direction: :forward) + new_quantities = previous_portfolio.dup + + trade_entries.each do |trade_entry| + trade = trade_entry.entryable + security_id = trade.security_id + qty_change = trade.qty + qty_change = qty_change * -1 if direction == :reverse + new_quantities[security_id] = (new_quantities[security_id] || 0) + qty_change + end + + new_quantities + end + + def build_holdings(portfolio, date, price_source: nil) + portfolio.map do |security_id, qty| + price = portfolio_cache.get_price(security_id, date, source: price_source) + + if price.nil? + next + end + + Holding.new( + account_id: account.id, + security_id: security_id, + date: date, + qty: qty, + price: price.price, + currency: price.currency, + amount: qty * price.price + ) + end.compact + end + def calculate_holdings current_portfolio = generate_starting_portfolio next_portfolio = {} diff --git a/app/models/holding/reverse_calculator.rb b/app/models/holding/reverse_calculator.rb index 62e2dc95..f52184d7 100644 --- a/app/models/holding/reverse_calculator.rb +++ b/app/models/holding/reverse_calculator.rb @@ -1,4 +1,17 @@ -class Holding::ReverseCalculator < Holding::BaseCalculator +class Holding::ReverseCalculator + attr_reader :account + + def initialize(account) + @account = account + end + + def calculate + Rails.logger.tagged("Holding::ReverseCalculator") do + holdings = calculate_holdings + Holding.gapfill(holdings) + end + end + private # Reverse calculators will use the existing holdings as a source of security ids and prices # since it is common for a provider to supply "current day" holdings but not all the historical @@ -25,6 +38,11 @@ class Holding::ReverseCalculator < Holding::BaseCalculator holdings end + def empty_portfolio + securities = portfolio_cache.get_securities + securities.each_with_object({}) { |security, hash| hash[security.id] = 0 } + end + # Since this is a reverse sync, we start with today's holdings def generate_starting_portfolio holding_quantities = empty_portfolio @@ -37,4 +55,38 @@ class Holding::ReverseCalculator < Holding::BaseCalculator holding_quantities end + + def transform_portfolio(previous_portfolio, trade_entries, direction: :forward) + new_quantities = previous_portfolio.dup + + trade_entries.each do |trade_entry| + trade = trade_entry.entryable + security_id = trade.security_id + qty_change = trade.qty + qty_change = qty_change * -1 if direction == :reverse + new_quantities[security_id] = (new_quantities[security_id] || 0) + qty_change + end + + new_quantities + end + + def build_holdings(portfolio, date, price_source: nil) + portfolio.map do |security_id, qty| + price = portfolio_cache.get_price(security_id, date, source: price_source) + + if price.nil? + next + end + + Holding.new( + account_id: account.id, + security_id: security_id, + date: date, + qty: qty, + price: price.price, + currency: price.currency, + amount: qty * price.price + ) + end.compact + end end -- 2.53.0 From 3207077f9e1accd25f7306d07adff5bedf1ed262 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sat, 10 May 2025 16:38:47 -0400 Subject: [PATCH 03/27] Put sync implementations in own concerns --- app/models/account.rb | 21 --------------------- app/models/account/syncable.rb | 26 ++++++++++++++++++++++++++ app/models/family/syncable.rb | 3 +-- app/models/plaid_item.rb | 23 ----------------------- app/models/plaid_item/syncable.rb | 28 ++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 46 deletions(-) create mode 100644 app/models/account/syncable.rb create mode 100644 app/models/plaid_item/syncable.rb diff --git a/app/models/account.rb b/app/models/account.rb index 352335e0..ee2fc963 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -81,21 +81,6 @@ class Account < ApplicationRecord DestroyJob.perform_later(self) end - def sync_data(sync, start_date: nil) - Rails.logger.info("Processing balances (#{linked? ? 'reverse' : 'forward'})") - sync_balances - end - - def post_sync(sync) - family.remove_syncing_notice! - - accountable.post_sync(sync) - - unless sync.child? - family.auto_match_transfers! - end - end - def current_holdings holdings.where(currency: currency, date: holdings.maximum(:date)).order(amount: :desc) end @@ -172,10 +157,4 @@ class Account < ApplicationRecord def long_subtype_label accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name end - - private - def sync_balances - strategy = linked? ? :reverse : :forward - Balance::Syncer.new(self, strategy: strategy).sync_balances - end end diff --git a/app/models/account/syncable.rb b/app/models/account/syncable.rb new file mode 100644 index 00000000..f4547ce9 --- /dev/null +++ b/app/models/account/syncable.rb @@ -0,0 +1,26 @@ +module Account::Syncable + extend ActiveSupport::Concern + + include Syncable + + def sync_data(sync, start_date: nil) + Rails.logger.info("Processing balances (#{linked? ? 'reverse' : 'forward'})") + sync_balances + end + + def post_sync(sync) + family.remove_syncing_notice! + + accountable.post_sync(sync) + + unless sync.child? + family.auto_match_transfers! + end + end + + private + def sync_balances + strategy = linked? ? :reverse : :forward + Balance::Syncer.new(self, strategy: strategy).sync_balances + end +end diff --git a/app/models/family/syncable.rb b/app/models/family/syncable.rb index 9b1c276a..8615a528 100644 --- a/app/models/family/syncable.rb +++ b/app/models/family/syncable.rb @@ -1,8 +1,7 @@ module Family::Syncable extend ActiveSupport::Concern - # Top-level, generic Syncable concern - include ::Syncable + include Syncable def sync_data(sync, start_date: nil) # We don't rely on this value to guard the app, but keep it eventually consistent diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index 9bf91d70..c387dcc1 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -22,24 +22,6 @@ class PlaidItem < ApplicationRecord scope :ordered, -> { order(created_at: :desc) } scope :needs_update, -> { where(status: :requires_update) } - def sync_data(sync, start_date: nil) - begin - Rails.logger.info("Fetching and loading Plaid data") - fetch_and_load_plaid_data(sync) - update!(status: :good) if requires_update? - - # Schedule account syncs - accounts.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Plaid data fetched and loaded") - rescue Plaid::ApiError => e - handle_plaid_error(e) - raise e - end - end - def get_update_link_token(webhooks_url:, redirect_url:) begin family.get_link_token( @@ -61,11 +43,6 @@ class PlaidItem < ApplicationRecord end end - def post_sync(sync) - auto_match_categories! - family.broadcast_refresh - end - def destroy_later update!(scheduled_for_deletion: true) DestroyJob.perform_later(self) diff --git a/app/models/plaid_item/syncable.rb b/app/models/plaid_item/syncable.rb new file mode 100644 index 00000000..760887cf --- /dev/null +++ b/app/models/plaid_item/syncable.rb @@ -0,0 +1,28 @@ +module PlaidItem::Syncable + extend ActiveSupport::Concern + + include Syncable + + def sync_data(sync, start_date: nil) + begin + Rails.logger.info("Fetching and loading Plaid data") + fetch_and_load_plaid_data(sync) + update!(status: :good) if requires_update? + + # Schedule account syncs + accounts.each do |account| + account.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Plaid data fetched and loaded") + rescue Plaid::ApiError => e + handle_plaid_error(e) + raise e + end + end + + def post_sync(sync) + auto_match_categories! + family.broadcast_refresh + end +end -- 2.53.0 From 6ddb4d088ee2851fd98d57c788663fa4d791b60b Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sat, 10 May 2025 18:08:31 -0400 Subject: [PATCH 04/27] Sync strategies --- app/models/account/syncable.rb | 26 --- app/models/account/syncer.rb | 28 ++++ app/models/concerns/syncable.rb | 8 - app/models/family.rb | 15 ++ app/models/family/subscribeable.rb | 9 +- app/models/family/syncable.rb | 45 ------ app/models/family/syncer.rb | 32 ++++ app/models/plaid_item.rb | 118 +------------- app/models/plaid_item/provided.rb | 30 ---- app/models/plaid_item/syncable.rb | 28 ---- app/models/plaid_item/syncer.rb | 151 ++++++++++++++++++ app/models/sync.rb | 10 +- .../accounts/_accountable_group.html.erb | 4 +- app/views/category/dropdowns/show.html.erb | 14 +- app/views/settings/_settings_nav.html.erb | 32 ++-- test/interfaces/syncable_interface_test.rb | 4 - test/models/family/syncer_test.rb | 24 +++ test/models/family_test.rb | 20 --- test/models/sync_test.rb | 12 +- 19 files changed, 297 insertions(+), 313 deletions(-) delete mode 100644 app/models/account/syncable.rb create mode 100644 app/models/account/syncer.rb delete mode 100644 app/models/family/syncable.rb create mode 100644 app/models/family/syncer.rb delete mode 100644 app/models/plaid_item/provided.rb delete mode 100644 app/models/plaid_item/syncable.rb create mode 100644 app/models/plaid_item/syncer.rb create mode 100644 test/models/family/syncer_test.rb diff --git a/app/models/account/syncable.rb b/app/models/account/syncable.rb deleted file mode 100644 index f4547ce9..00000000 --- a/app/models/account/syncable.rb +++ /dev/null @@ -1,26 +0,0 @@ -module Account::Syncable - extend ActiveSupport::Concern - - include Syncable - - def sync_data(sync, start_date: nil) - Rails.logger.info("Processing balances (#{linked? ? 'reverse' : 'forward'})") - sync_balances - end - - def post_sync(sync) - family.remove_syncing_notice! - - accountable.post_sync(sync) - - unless sync.child? - family.auto_match_transfers! - end - end - - private - def sync_balances - strategy = linked? ? :reverse : :forward - Balance::Syncer.new(self, strategy: strategy).sync_balances - end -end diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb new file mode 100644 index 00000000..c35975e9 --- /dev/null +++ b/app/models/account/syncer.rb @@ -0,0 +1,28 @@ +class Account::Syncer + attr_reader :account + + def initialize(account) + @account = account + end + + def perform_sync(start_date: nil) + Rails.logger.info("Processing balances (#{account.linked? ? 'reverse' : 'forward'})") + sync_balances + end + + def perform_post_sync(sync) + account.family.remove_syncing_notice! + + account.accountable.post_sync(sync) + + unless sync.child? + account.family.auto_match_transfers! + end + end + + private + def sync_balances + strategy = account.linked? ? :reverse : :forward + Balance::Syncer.new(account, strategy: strategy).sync_balances + end +end diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index d804b992..43afed5f 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -18,14 +18,6 @@ module Syncable syncs.create!(start_date: start_date).perform end - def sync_data(sync, start_date: nil) - raise NotImplementedError, "Subclasses must implement the `sync_data` method" - end - - def post_sync(sync) - # no-op, syncable can optionally provide implementation - end - def sync_error latest_sync&.error end diff --git a/app/models/family.rb b/app/models/family.rb index 0df7fd12..0644776b 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -35,6 +35,21 @@ class Family < ApplicationRecord validates :locale, inclusion: { in: I18n.available_locales.map(&:to_s) } validates :date_format, inclusion: { in: DATE_FORMATS.map(&:last) } + def remove_syncing_notice! + broadcast_remove target: "syncing-notice" + end + + # If family has any syncs pending/syncing within the last 10 minutes, we show a persistent "syncing" notice. + # Ignore syncs older than 10 minutes as they are considered "stale" + def syncing? + Sync.where( + "(syncable_type = 'Family' AND syncable_id = ?) OR + (syncable_type = 'Account' AND syncable_id IN (SELECT id FROM accounts WHERE family_id = ? AND plaid_account_id IS NULL)) OR + (syncable_type = 'PlaidItem' AND syncable_id IN (SELECT id FROM plaid_items WHERE family_id = ?))", + id, id, id + ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? + end + def assigned_merchants merchant_ids = transactions.where.not(merchant_id: nil).pluck(:merchant_id).uniq Merchant.where(id: merchant_ids) diff --git a/app/models/family/subscribeable.rb b/app/models/family/subscribeable.rb index 4c8624b7..94ee0971 100644 --- a/app/models/family/subscribeable.rb +++ b/app/models/family/subscribeable.rb @@ -72,10 +72,9 @@ module Family::Subscribeable (1 - days_left_in_trial.to_f / Subscription::TRIAL_DAYS) * 100 end - private - def sync_trial_status! - if subscription&.status == "trialing" && days_left_in_trial < 0 - subscription.update!(status: "paused") - end + def sync_trial_status! + if subscription&.status == "trialing" && days_left_in_trial < 0 + subscription.update!(status: "paused") end + end end diff --git a/app/models/family/syncable.rb b/app/models/family/syncable.rb deleted file mode 100644 index 8615a528..00000000 --- a/app/models/family/syncable.rb +++ /dev/null @@ -1,45 +0,0 @@ -module Family::Syncable - extend ActiveSupport::Concern - - include Syncable - - def sync_data(sync, start_date: nil) - # We don't rely on this value to guard the app, but keep it eventually consistent - sync_trial_status! - - Rails.logger.info("Syncing accounts for family #{id}") - accounts.manual.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Syncing plaid items for family #{id}") - plaid_items.each do |plaid_item| - plaid_item.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Applying rules for family #{id}") - rules.each do |rule| - rule.apply_later - end - end - - def remove_syncing_notice! - broadcast_remove target: "syncing-notice" - end - - def post_sync(sync) - auto_match_transfers! - broadcast_refresh - end - - # If family has any syncs pending/syncing within the last 10 minutes, we show a persistent "syncing" notice. - # Ignore syncs older than 10 minutes as they are considered "stale" - def syncing? - Sync.where( - "(syncable_type = 'Family' AND syncable_id = ?) OR - (syncable_type = 'Account' AND syncable_id IN (SELECT id FROM accounts WHERE family_id = ? AND plaid_account_id IS NULL)) OR - (syncable_type = 'PlaidItem' AND syncable_id IN (SELECT id FROM plaid_items WHERE family_id = ?))", - id, id, id - ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? - end -end diff --git a/app/models/family/syncer.rb b/app/models/family/syncer.rb new file mode 100644 index 00000000..fe35f42f --- /dev/null +++ b/app/models/family/syncer.rb @@ -0,0 +1,32 @@ +class Family::Syncer + attr_reader :family + + def initialize(family) + @family = family + end + + def perform_sync(sync, start_date: nil) + # We don't rely on this value to guard the app, but keep it eventually consistent + family.sync_trial_status! + + Rails.logger.info("Syncing accounts for family #{family.id}") + family.accounts.manual.each do |account| + account.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Syncing plaid items for family #{family.id}") + family.plaid_items.each do |plaid_item| + plaid_item.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Applying rules for family #{family.id}") + family.rules.each do |rule| + rule.apply_later + end + end + + def perform_post_sync(sync) + family.auto_match_transfers! + family.broadcast_refresh + end +end diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index c387dcc1..c2708f49 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -1,5 +1,5 @@ class PlaidItem < ApplicationRecord - include Provided, Syncable + include Syncable enum :plaid_region, { us: "us", eu: "eu" } enum :status, { good: "good", requires_update: "requires_update" }, default: :good @@ -43,6 +43,10 @@ class PlaidItem < ApplicationRecord end end + def build_category_alias_matcher(user_categories) + Provider::Plaid::CategoryAliasMatcher.new(user_categories) + end + def destroy_later update!(scheduled_for_deletion: true) DestroyJob.perform_later(self) @@ -79,123 +83,11 @@ class PlaidItem < ApplicationRecord end private - def fetch_and_load_plaid_data(sync) - data = {} - - # Log what we're about to fetch - Rails.logger.info "Starting Plaid data fetch (accounts, transactions, investments, liabilities)" - - item = plaid_provider.get_item(access_token).item - update!(available_products: item.available_products, billed_products: item.billed_products) - - # Institution details - if item.institution_id.present? - begin - Rails.logger.info "Fetching Plaid institution details for #{item.institution_id}" - institution = plaid_provider.get_institution(item.institution_id) - update!( - institution_id: item.institution_id, - institution_url: institution.institution.url, - institution_color: institution.institution.primary_color - ) - rescue Plaid::ApiError => e - Rails.logger.warn "Failed to fetch Plaid institution details: #{e.message}" - end - end - - # Accounts - fetched_accounts = plaid_provider.get_item_accounts(self).accounts - data[:accounts] = fetched_accounts || [] - sync.update!(data: data) - Rails.logger.info "Processing Plaid accounts (count: #{fetched_accounts.size})" - - internal_plaid_accounts = fetched_accounts.map do |account| - internal_plaid_account = plaid_accounts.find_or_create_from_plaid_data!(account, family) - internal_plaid_account.sync_account_data!(account) - internal_plaid_account - end - - # Transactions - fetched_transactions = safe_fetch_plaid_data(:get_item_transactions) - data[:transactions] = fetched_transactions || [] - sync.update!(data: data) - - if fetched_transactions - Rails.logger.info "Processing Plaid transactions (added: #{fetched_transactions.added.size}, modified: #{fetched_transactions.modified.size}, removed: #{fetched_transactions.removed.size})" - transaction do - internal_plaid_accounts.each do |internal_plaid_account| - added = fetched_transactions.added.select { |t| t.account_id == internal_plaid_account.plaid_id } - modified = fetched_transactions.modified.select { |t| t.account_id == internal_plaid_account.plaid_id } - removed = fetched_transactions.removed.select { |t| t.account_id == internal_plaid_account.plaid_id } - - internal_plaid_account.sync_transactions!(added:, modified:, removed:) - end - - update!(next_cursor: fetched_transactions.cursor) - end - end - - # Investments - fetched_investments = safe_fetch_plaid_data(:get_item_investments) - data[:investments] = fetched_investments || [] - sync.update!(data: data) - - if fetched_investments - Rails.logger.info "Processing Plaid investments (transactions: #{fetched_investments.transactions.size}, holdings: #{fetched_investments.holdings.size}, securities: #{fetched_investments.securities.size})" - transaction do - internal_plaid_accounts.each do |internal_plaid_account| - transactions = fetched_investments.transactions.select { |t| t.account_id == internal_plaid_account.plaid_id } - holdings = fetched_investments.holdings.select { |h| h.account_id == internal_plaid_account.plaid_id } - securities = fetched_investments.securities - - internal_plaid_account.sync_investments!(transactions:, holdings:, securities:) - end - end - end - - # Liabilities - fetched_liabilities = safe_fetch_plaid_data(:get_item_liabilities) - data[:liabilities] = fetched_liabilities || [] - sync.update!(data: data) - - if fetched_liabilities - Rails.logger.info "Processing Plaid liabilities (credit: #{fetched_liabilities.credit&.size || 0}, mortgage: #{fetched_liabilities.mortgage&.size || 0}, student: #{fetched_liabilities.student&.size || 0})" - transaction do - internal_plaid_accounts.each do |internal_plaid_account| - credit = fetched_liabilities.credit&.find { |l| l.account_id == internal_plaid_account.plaid_id } - mortgage = fetched_liabilities.mortgage&.find { |l| l.account_id == internal_plaid_account.plaid_id } - student = fetched_liabilities.student&.find { |l| l.account_id == internal_plaid_account.plaid_id } - - internal_plaid_account.sync_credit_data!(credit) if credit - internal_plaid_account.sync_mortgage_data!(mortgage) if mortgage - internal_plaid_account.sync_student_loan_data!(student) if student - end - end - end - end - - def safe_fetch_plaid_data(method) - begin - plaid_provider.send(method, self) - rescue Plaid::ApiError => e - Rails.logger.warn("Error fetching #{method} for item #{id}: #{e.message}") - nil - end - end - def remove_plaid_item plaid_provider.remove_item(access_token) rescue StandardError => e Rails.logger.warn("Failed to remove Plaid item #{id}: #{e.message}") end - def handle_plaid_error(error) - error_body = JSON.parse(error.response_body) - - if error_body["error_code"] == "ITEM_LOGIN_REQUIRED" - update!(status: :requires_update) - end - end - class PlaidConnectionLostError < StandardError; end end diff --git a/app/models/plaid_item/provided.rb b/app/models/plaid_item/provided.rb deleted file mode 100644 index f2e8ee8f..00000000 --- a/app/models/plaid_item/provided.rb +++ /dev/null @@ -1,30 +0,0 @@ -module PlaidItem::Provided - extend ActiveSupport::Concern - - class_methods do - def plaid_us_provider - Provider::Registry.get_provider(:plaid_us) - end - - def plaid_eu_provider - Provider::Registry.get_provider(:plaid_eu) - end - - def plaid_provider_for_region(region) - region.to_sym == :eu ? plaid_eu_provider : plaid_us_provider - end - end - - def build_category_alias_matcher(user_categories) - Provider::Plaid::CategoryAliasMatcher.new(user_categories) - end - - private - def eu? - raise "eu? is not implemented for #{self.class.name}" - end - - def plaid_provider - eu? ? self.class.plaid_eu_provider : self.class.plaid_us_provider - end -end diff --git a/app/models/plaid_item/syncable.rb b/app/models/plaid_item/syncable.rb deleted file mode 100644 index 760887cf..00000000 --- a/app/models/plaid_item/syncable.rb +++ /dev/null @@ -1,28 +0,0 @@ -module PlaidItem::Syncable - extend ActiveSupport::Concern - - include Syncable - - def sync_data(sync, start_date: nil) - begin - Rails.logger.info("Fetching and loading Plaid data") - fetch_and_load_plaid_data(sync) - update!(status: :good) if requires_update? - - # Schedule account syncs - accounts.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Plaid data fetched and loaded") - rescue Plaid::ApiError => e - handle_plaid_error(e) - raise e - end - end - - def post_sync(sync) - auto_match_categories! - family.broadcast_refresh - end -end diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb new file mode 100644 index 00000000..4474c992 --- /dev/null +++ b/app/models/plaid_item/syncer.rb @@ -0,0 +1,151 @@ +class PlaidItem::Syncer + attr_reader :plaid_item + + def initialize(plaid_item) + @plaid_item = plaid_item + end + + def perform_sync(sync, start_date: nil) + begin + Rails.logger.info("Fetching and loading Plaid data") + fetch_and_load_plaid_data + plaid_item.update!(status: :good) if plaid_item.requires_update? + + # Schedule account syncs + plaid_item.accounts.each do |account| + account.sync_later(start_date: start_date, parent_sync: sync) + end + + Rails.logger.info("Plaid data fetched and loaded") + rescue Plaid::ApiError => e + handle_plaid_error(e) + raise e + end + end + + def perform_post_sync(sync) + plaid_item.auto_match_categories! + plaid_item.family.broadcast_refresh + end + + private + def plaid + plaid_item.plaid_region == "eu" ? plaid_eu : plaid_us + end + + def plaid_eu + @plaid_eu ||= Provider::Registry.get_provider(:plaid_eu) + end + + def plaid_us + @plaid_us ||= Provider::Registry.get_provider(:plaid_us) + end + + def safe_fetch_plaid_data(method) + begin + plaid.send(method, self) + rescue Plaid::ApiError => e + Rails.logger.warn("Error fetching #{method} for item #{id}: #{e.message}") + nil + end + end + + def handle_plaid_error(error) + error_body = JSON.parse(error.response_body) + + if error_body["error_code"] == "ITEM_LOGIN_REQUIRED" + update!(status: :requires_update) + end + end + + def fetch_and_load_plaid_data + data = {} + + # Log what we're about to fetch + Rails.logger.info "Starting Plaid data fetch (accounts, transactions, investments, liabilities)" + + item = plaid.get_item(plaid_item.access_token).item + update!(available_products: item.available_products, billed_products: item.billed_products) + + # Institution details + if item.institution_id.present? + begin + Rails.logger.info "Fetching Plaid institution details for #{item.institution_id}" + institution = plaid.get_institution(item.institution_id) + update!( + institution_id: item.institution_id, + institution_url: institution.institution.url, + institution_color: institution.institution.primary_color + ) + rescue Plaid::ApiError => e + Rails.logger.warn "Failed to fetch Plaid institution details: #{e.message}" + end + end + + # Accounts + fetched_accounts = plaid.get_item_accounts(plaid_item).accounts + data[:accounts] = fetched_accounts || [] + Rails.logger.info "Processing Plaid accounts (count: #{fetched_accounts.size})" + + internal_plaid_accounts = fetched_accounts.map do |account| + internal_plaid_account = plaid_item.plaid_accounts.find_or_create_from_plaid_data!(account, plaid_item.family) + internal_plaid_account.sync_account_data!(account) + internal_plaid_account + end + + # Transactions + fetched_transactions = safe_fetch_plaid_data(:get_item_transactions) + data[:transactions] = fetched_transactions || [] + + if fetched_transactions + Rails.logger.info "Processing Plaid transactions (added: #{fetched_transactions.added.size}, modified: #{fetched_transactions.modified.size}, removed: #{fetched_transactions.removed.size})" + transaction do + internal_plaid_accounts.each do |internal_plaid_account| + added = fetched_transactions.added.select { |t| t.account_id == internal_plaid_account.plaid_id } + modified = fetched_transactions.modified.select { |t| t.account_id == internal_plaid_account.plaid_id } + removed = fetched_transactions.removed.select { |t| t.account_id == internal_plaid_account.plaid_id } + + internal_plaid_account.sync_transactions!(added:, modified:, removed:) + end + + update!(next_cursor: fetched_transactions.cursor) + end + end + + # Investments + fetched_investments = safe_fetch_plaid_data(:get_item_investments) + data[:investments] = fetched_investments || [] + + if fetched_investments + Rails.logger.info "Processing Plaid investments (transactions: #{fetched_investments.transactions.size}, holdings: #{fetched_investments.holdings.size}, securities: #{fetched_investments.securities.size})" + transaction do + internal_plaid_accounts.each do |internal_plaid_account| + transactions = fetched_investments.transactions.select { |t| t.account_id == internal_plaid_account.plaid_id } + holdings = fetched_investments.holdings.select { |h| h.account_id == internal_plaid_account.plaid_id } + securities = fetched_investments.securities + + internal_plaid_account.sync_investments!(transactions:, holdings:, securities:) + end + end + end + + # Liabilities + fetched_liabilities = safe_fetch_plaid_data(:get_item_liabilities) + data[:liabilities] = fetched_liabilities || [] + + if fetched_liabilities + Rails.logger.info "Processing Plaid liabilities (credit: #{fetched_liabilities.credit&.size || 0}, mortgage: #{fetched_liabilities.mortgage&.size || 0}, student: #{fetched_liabilities.student&.size || 0})" + transaction do + internal_plaid_accounts.each do |internal_plaid_account| + credit = fetched_liabilities.credit&.find { |l| l.account_id == internal_plaid_account.plaid_id } + mortgage = fetched_liabilities.mortgage&.find { |l| l.account_id == internal_plaid_account.plaid_id } + student = fetched_liabilities.student&.find { |l| l.account_id == internal_plaid_account.plaid_id } + + internal_plaid_account.sync_credit_data!(credit) if credit + internal_plaid_account.sync_mortgage_data!(mortgage) if mortgage + internal_plaid_account.sync_student_loan_data!(student) if student + end + end + end + end +end diff --git a/app/models/sync.rb b/app/models/sync.rb index 826899ce..b335eb14 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -19,12 +19,12 @@ class Sync < ApplicationRecord start! begin - syncable.sync_data(self, start_date: start_date) + syncer.perform_sync(self, start_date: start_date) unless has_pending_child_syncs? complete! Rails.logger.info("Sync completed, starting post-sync") - syncable.post_sync(self) + syncer.perform_post_sync(self) Rails.logger.info("Post-sync completed") end rescue StandardError => error @@ -51,12 +51,16 @@ class Sync < ApplicationRecord # If this sync is both a child and a parent, we need to notify the parent of completion notify_parent_of_completion! if has_parent? - syncable.post_sync(self) + syncer.perform_post_sync(self) end end end private + def syncer + "#{syncable_type}::Syncer".constantize.new(syncable) + end + def has_pending_child_syncs? children.where(status: [ :pending, :syncing ]).any? end diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index a1ab2e3a..ab10dec7 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -15,11 +15,11 @@
<% account_group.accounts.each do |account| %> - <%= link_to account_path(account), + <%= link_to account_path(account), class: class_names( "block flex items-center gap-2 px-3 py-2 rounded-lg", page_active?(account_path(account)) ? "bg-container" : "hover:bg-surface-hover" - ), + ), data: { sidebar_tabs_target: "account", action: "click->sidebar-tabs#select" }, title: account.name do %> <%= render "accounts/logo", account: account, size: "sm", color: account_group.color %> diff --git a/app/views/category/dropdowns/show.html.erb b/app/views/category/dropdowns/show.html.erb index 65b5c9be..3b25083b 100644 --- a/app/views/category/dropdowns/show.html.erb +++ b/app/views/category/dropdowns/show.html.erb @@ -2,13 +2,13 @@
- " - autocomplete="nope" - type="search" - class="bg-container placeholder:text-sm placeholder:text-secondary font-normal h-10 relative pl-10 w-full border-none rounded-lg focus:outline-hidden focus:ring-0" - data-list-filter-target="input" - data-action="list-filter#filter" /> + " + autocomplete="nope" + type="search" + class="bg-container placeholder:text-sm placeholder:text-secondary font-normal h-10 relative pl-10 w-full border-none rounded-lg focus:outline-hidden focus:ring-0" + data-list-filter-target="input" + data-action="list-filter#filter"> <%= icon("search", class: "absolute inset-0 ml-2 transform top-1/2 -translate-y-1/2") %>
diff --git a/app/views/settings/_settings_nav.html.erb b/app/views/settings/_settings_nav.html.erb index 6a80bb87..5aa2c258 100644 --- a/app/views/settings/_settings_nav.html.erb +++ b/app/views/settings/_settings_nav.html.erb @@ -1,31 +1,31 @@ <% nav_sections = [ { - header: t('.general_section_title'), + header: t(".general_section_title"), items: [ - { label: t('.profile_label'), path: settings_profile_path, icon: 'circle-user' }, - { label: t('.preferences_label'), path: settings_preferences_path, icon: 'bolt' }, - { label: t('.security_label'), path: settings_security_path, icon: 'shield-check' }, - { label: t('.self_hosting_label'), path: settings_hosting_path, icon: 'database', if: self_hosted? }, - { label: t('.billing_label'), path: settings_billing_path, icon: 'circle-dollar-sign', if: !self_hosted? }, - { label: t('.accounts_label'), path: accounts_path, icon: 'layers' }, - { label: t('.imports_label'), path: imports_path, icon: 'download' } + { label: t(".profile_label"), path: settings_profile_path, icon: "circle-user" }, + { label: t(".preferences_label"), path: settings_preferences_path, icon: "bolt" }, + { label: t(".security_label"), path: settings_security_path, icon: "shield-check" }, + { label: t(".self_hosting_label"), path: settings_hosting_path, icon: "database", if: self_hosted? }, + { label: t(".billing_label"), path: settings_billing_path, icon: "circle-dollar-sign", if: !self_hosted? }, + { label: t(".accounts_label"), path: accounts_path, icon: "layers" }, + { label: t(".imports_label"), path: imports_path, icon: "download" } ] }, { - header: t('.transactions_section_title'), + header: t(".transactions_section_title"), items: [ - { label: t('.tags_label'), path: tags_path, icon: 'tags' }, - { label: t('.categories_label'), path: categories_path, icon: 'shapes' }, - { label: t('.rules_label'), path: rules_path, icon: 'git-branch' }, - { label: t('.merchants_label'), path: family_merchants_path, icon: 'store' } + { label: t(".tags_label"), path: tags_path, icon: "tags" }, + { label: t(".categories_label"), path: categories_path, icon: "shapes" }, + { label: t(".rules_label"), path: rules_path, icon: "git-branch" }, + { label: t(".merchants_label"), path: family_merchants_path, icon: "store" } ] }, { - header: t('.other_section_title'), + header: t(".other_section_title"), items: [ - { label: t('.whats_new_label'), path: changelog_path, icon: 'box' }, - { label: t('.feedback_label'), path: feedback_path, icon: 'megaphone' } + { label: t(".whats_new_label"), path: changelog_path, icon: "box" }, + { label: t(".feedback_label"), path: feedback_path, icon: "megaphone" } ] } ] diff --git a/test/interfaces/syncable_interface_test.rb b/test/interfaces/syncable_interface_test.rb index 6613dbcc..41c34b4b 100644 --- a/test/interfaces/syncable_interface_test.rb +++ b/test/interfaces/syncable_interface_test.rb @@ -17,8 +17,4 @@ module SyncableInterfaceTest @syncable.sync(start_date: 2.days.ago.to_date) end end - - test "implements sync_data" do - assert_respond_to @syncable, :sync_data - end end diff --git a/test/models/family/syncer_test.rb b/test/models/family/syncer_test.rb new file mode 100644 index 00000000..6fe17e1d --- /dev/null +++ b/test/models/family/syncer_test.rb @@ -0,0 +1,24 @@ +require "test_helper" + +class Family::SyncerTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + end + + test "syncs plaid items and manual accounts" do + family_sync = syncs(:family) + + manual_accounts_count = @family.accounts.manual.count + items_count = @family.plaid_items.count + + Account.any_instance.expects(:sync_later) + .with(start_date: nil, parent_sync: family_sync) + .times(manual_accounts_count) + + PlaidItem.any_instance.expects(:sync_later) + .with(start_date: nil, parent_sync: family_sync) + .times(items_count) + + Family::Syncer.new(@family).perform_sync(family_sync, start_date: family_sync.start_date) + end +end diff --git a/test/models/family_test.rb b/test/models/family_test.rb index 7223e64b..0229aa6e 100644 --- a/test/models/family_test.rb +++ b/test/models/family_test.rb @@ -1,29 +1,9 @@ require "test_helper" -require "csv" class FamilyTest < ActiveSupport::TestCase - include EntriesTestHelper include SyncableInterfaceTest def setup - @family = families(:empty) @syncable = families(:dylan_family) end - - test "syncs plaid items and manual accounts" do - family_sync = syncs(:family) - - manual_accounts_count = @syncable.accounts.manual.count - items_count = @syncable.plaid_items.count - - Account.any_instance.expects(:sync_later) - .with(start_date: nil, parent_sync: family_sync) - .times(manual_accounts_count) - - PlaidItem.any_instance.expects(:sync_later) - .with(start_date: nil, parent_sync: family_sync) - .times(items_count) - - @syncable.sync_data(family_sync, start_date: family_sync.start_date) - end end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 6b246a1f..621cd47d 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -7,7 +7,7 @@ class SyncTest < ActiveSupport::TestCase end test "runs successful sync" do - @sync.syncable.expects(:sync_data).with(@sync, start_date: @sync.start_date).once + Account::Syncer.any_instance.expects(:perform_sync).with(@sync, start_date: @sync.start_date).once assert_equal "pending", @sync.status @@ -20,7 +20,7 @@ class SyncTest < ActiveSupport::TestCase end test "handles sync errors" do - @sync.syncable.expects(:sync_data).with(@sync, start_date: @sync.start_date).raises(StandardError.new("test sync error")) + Account::Syncer.any_instance.expects(:perform_sync).with(@sync, start_date: @sync.start_date).raises(StandardError.new("test sync error")) assert_equal "pending", @sync.status previously_ran_at = @sync.last_ran_at @@ -42,10 +42,10 @@ class SyncTest < ActiveSupport::TestCase child2 = Sync.create!(syncable: family.accounts.second, parent: parent) grandchild = Sync.create!(syncable: family.accounts.last, parent: child2) - parent.syncable.expects(:sync_data).returns([]).once - child1.syncable.expects(:sync_data).returns([]).once - child2.syncable.expects(:sync_data).returns([]).once - grandchild.syncable.expects(:sync_data).returns([]).once + Family::Syncer.any_instance.expects(:perform_sync).with(parent, start_date: parent.start_date).once + Account::Syncer.any_instance.expects(:perform_sync).with(child1, start_date: parent.start_date).once + Account::Syncer.any_instance.expects(:perform_sync).with(child2, start_date: parent.start_date).once + Account::Syncer.any_instance.expects(:perform_sync).with(grandchild, start_date: parent.start_date).once assert_equal "pending", parent.status assert_equal "pending", child1.status -- 2.53.0 From 5432b3903dd4d20060178a7f7720101adddee770 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sat, 10 May 2025 21:04:59 -0400 Subject: [PATCH 05/27] Move sync orchestration to Sync class --- app/models/account/syncer.rb | 14 ++-- app/models/family/syncer.rb | 18 ++--- app/models/plaid_item/syncer.rb | 13 ++-- app/models/sync.rb | 11 +++- test/models/family/syncer_test.rb | 11 +--- test/models/sync_test.rb | 106 +++++++++++++++++------------- 6 files changed, 91 insertions(+), 82 deletions(-) diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index c35975e9..a1576aea 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -5,19 +5,23 @@ class Account::Syncer @account = account end + def child_syncables + [] + end + def perform_sync(start_date: nil) Rails.logger.info("Processing balances (#{account.linked? ? 'reverse' : 'forward'})") sync_balances end - def perform_post_sync(sync) + def perform_post_sync account.family.remove_syncing_notice! - account.accountable.post_sync(sync) + # account.accountable.post_sync(sync) - unless sync.child? - account.family.auto_match_transfers! - end + # unless sync.child? + account.family.auto_match_transfers! + # end end private diff --git a/app/models/family/syncer.rb b/app/models/family/syncer.rb index fe35f42f..3134a65c 100644 --- a/app/models/family/syncer.rb +++ b/app/models/family/syncer.rb @@ -5,27 +5,21 @@ class Family::Syncer @family = family end - def perform_sync(sync, start_date: nil) + def child_syncables + family.plaid_items + family.accounts.manual + end + + def perform_sync(start_date: nil) # We don't rely on this value to guard the app, but keep it eventually consistent family.sync_trial_status! - Rails.logger.info("Syncing accounts for family #{family.id}") - family.accounts.manual.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - - Rails.logger.info("Syncing plaid items for family #{family.id}") - family.plaid_items.each do |plaid_item| - plaid_item.sync_later(start_date: start_date, parent_sync: sync) - end - Rails.logger.info("Applying rules for family #{family.id}") family.rules.each do |rule| rule.apply_later end end - def perform_post_sync(sync) + def perform_post_sync family.auto_match_transfers! family.broadcast_refresh end diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 4474c992..031b586c 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -5,17 +5,16 @@ class PlaidItem::Syncer @plaid_item = plaid_item end - def perform_sync(sync, start_date: nil) + def child_syncables + plaid_item.accounts + end + + def perform_sync(start_date: nil) begin Rails.logger.info("Fetching and loading Plaid data") fetch_and_load_plaid_data plaid_item.update!(status: :good) if plaid_item.requires_update? - # Schedule account syncs - plaid_item.accounts.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) - end - Rails.logger.info("Plaid data fetched and loaded") rescue Plaid::ApiError => e handle_plaid_error(e) @@ -23,7 +22,7 @@ class PlaidItem::Syncer end end - def perform_post_sync(sync) + def perform_post_sync plaid_item.auto_match_categories! plaid_item.family.broadcast_refresh end diff --git a/app/models/sync.rb b/app/models/sync.rb index b335eb14..d3d9f225 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -19,12 +19,17 @@ class Sync < ApplicationRecord start! begin - syncer.perform_sync(self, start_date: start_date) + syncer.perform_sync(start_date: start_date) + + # Schedule child syncables to sync later + syncer.child_syncables.each do |child_syncable| + child_syncable.sync_later(start_date: start_date, parent_sync: self) + end unless has_pending_child_syncs? complete! Rails.logger.info("Sync completed, starting post-sync") - syncer.perform_post_sync(self) + syncer.perform_post_sync Rails.logger.info("Post-sync completed") end rescue StandardError => error @@ -51,7 +56,7 @@ class Sync < ApplicationRecord # If this sync is both a child and a parent, we need to notify the parent of completion notify_parent_of_completion! if has_parent? - syncer.perform_post_sync(self) + syncer.perform_post_sync end end end diff --git a/test/models/family/syncer_test.rb b/test/models/family/syncer_test.rb index 6fe17e1d..bc35e678 100644 --- a/test/models/family/syncer_test.rb +++ b/test/models/family/syncer_test.rb @@ -11,14 +11,9 @@ class Family::SyncerTest < ActiveSupport::TestCase manual_accounts_count = @family.accounts.manual.count items_count = @family.plaid_items.count - Account.any_instance.expects(:sync_later) - .with(start_date: nil, parent_sync: family_sync) - .times(manual_accounts_count) + syncer = Family::Syncer.new(@family) + syncer.perform_sync(start_date: family_sync.start_date) - PlaidItem.any_instance.expects(:sync_later) - .with(start_date: nil, parent_sync: family_sync) - .times(items_count) - - Family::Syncer.new(@family).perform_sync(family_sync, start_date: family_sync.start_date) + assert_equal manual_accounts_count + items_count, syncer.child_syncables.count end end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 621cd47d..2919c2f2 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -1,74 +1,86 @@ require "test_helper" class SyncTest < ActiveSupport::TestCase - setup do - @sync = syncs(:account) - @sync.update(status: "pending") - end + include ActiveJob::TestHelper test "runs successful sync" do - Account::Syncer.any_instance.expects(:perform_sync).with(@sync, start_date: @sync.start_date).once + sync = Sync.create!(syncable: accounts(:depository), last_ran_at: 1.day.ago) - assert_equal "pending", @sync.status + Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once - previously_ran_at = @sync.last_ran_at + assert_equal "pending", sync.status - @sync.perform + previously_ran_at = sync.last_ran_at - assert @sync.last_ran_at > previously_ran_at - assert_equal "completed", @sync.status + sync.perform + + assert sync.last_ran_at > previously_ran_at + assert_equal "completed", sync.status end test "handles sync errors" do - Account::Syncer.any_instance.expects(:perform_sync).with(@sync, start_date: @sync.start_date).raises(StandardError.new("test sync error")) + sync = Sync.create!(syncable: accounts(:depository), last_ran_at: 1.day.ago) + Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).raises(StandardError.new("test sync error")) - assert_equal "pending", @sync.status - previously_ran_at = @sync.last_ran_at + assert_equal "pending", sync.status + previously_ran_at = sync.last_ran_at - @sync.perform + sync.perform - assert @sync.last_ran_at > previously_ran_at - assert_equal "failed", @sync.status - assert_equal "test sync error", @sync.error + assert sync.last_ran_at > previously_ran_at + assert_equal "failed", sync.status + assert_equal "test sync error", sync.error end - # Order is important here. Parent syncs must implement sync_data so that their own work - # is 100% complete *prior* to queueing up child syncs. - test "runs sync with child syncs" do + test "can run nested syncs that alert the parent when complete" do + # Clear out fixture syncs + Sync.destroy_all + + # These fixtures represent a Parent -> Child -> Grandchild sync hierarchy + # Family -> PlaidItem -> Account family = families(:dylan_family) + plaid_item = plaid_items(:one) + account = accounts(:connected) - parent = Sync.create!(syncable: family) - child1 = Sync.create!(syncable: family.accounts.first, parent: parent) - child2 = Sync.create!(syncable: family.accounts.second, parent: parent) - grandchild = Sync.create!(syncable: family.accounts.last, parent: child2) + sync = Sync.create!(syncable: family) - Family::Syncer.any_instance.expects(:perform_sync).with(parent, start_date: parent.start_date).once - Account::Syncer.any_instance.expects(:perform_sync).with(child1, start_date: parent.start_date).once - Account::Syncer.any_instance.expects(:perform_sync).with(child2, start_date: parent.start_date).once - Account::Syncer.any_instance.expects(:perform_sync).with(grandchild, start_date: parent.start_date).once + Family::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once + Family::Syncer.any_instance.expects(:perform_post_sync).once + Family::Syncer.any_instance.expects(:child_syncables).returns([ plaid_item ]) - assert_equal "pending", parent.status - assert_equal "pending", child1.status - assert_equal "pending", child2.status - assert_equal "pending", grandchild.status + PlaidItem::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once + PlaidItem::Syncer.any_instance.expects(:perform_post_sync).once + PlaidItem::Syncer.any_instance.expects(:child_syncables).returns([ account ]) - parent.perform - assert_equal "syncing", parent.reload.status + Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once + Account::Syncer.any_instance.expects(:perform_post_sync).once + Account::Syncer.any_instance.expects(:child_syncables).returns([]) - child1.perform - assert_equal "completed", child1.reload.status - assert_equal "syncing", parent.reload.status + sync.perform - child2.perform - assert_equal "syncing", child2.reload.status - assert_equal "completed", child1.reload.status - assert_equal "syncing", parent.reload.status + assert_equal 1, family.syncs.count + assert_equal "syncing", family.syncs.first.status + assert_equal 1, plaid_item.syncs.count + assert_equal "pending", plaid_item.syncs.first.status - # Will complete the parent and grandparent syncs - grandchild.perform - assert_equal "completed", grandchild.reload.status - assert_equal "completed", child1.reload.status - assert_equal "completed", child2.reload.status - assert_equal "completed", parent.reload.status + # We have to perform jobs 2x because the child sync will schedule the grandchild sync, + # which then needs to be run. + perform_enqueued_jobs + + assert_equal 1, family.syncs.count + assert_equal "syncing", family.syncs.first.status + assert_equal 1, plaid_item.syncs.count + assert_equal "syncing", plaid_item.syncs.first.status + assert_equal 1, account.syncs.count + assert_equal "pending", account.syncs.first.status + + perform_enqueued_jobs + + assert_equal 1, family.syncs.count + assert_equal "completed", family.syncs.first.status + assert_equal 1, plaid_item.syncs.count + assert_equal "completed", plaid_item.syncs.first.status + assert_equal 1, account.syncs.count + assert_equal "completed", account.syncs.first.status end end -- 2.53.0 From 5c8bca31ec9ef8b1a9cecc1ab8a17fa88c68ab8c Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Sun, 11 May 2025 15:02:46 -0400 Subject: [PATCH 06/27] Clean up sync class, add state machine --- Gemfile | 4 + Gemfile.lock | 7 ++ app/models/account/syncer.rb | 12 +- app/models/concerns/accountable.rb | 2 +- app/models/concerns/syncable.rb | 16 ++- app/models/family/syncer.rb | 16 ++- app/models/import.rb | 2 +- app/models/plaid_item/syncer.rb | 10 +- app/models/sync.rb | 134 +++++++++------------ test/interfaces/syncable_interface_test.rb | 10 +- test/models/family/syncer_test.rb | 13 +- test/models/sync_test.rb | 72 +++++------ test/test_helper.rb | 1 + 13 files changed, 147 insertions(+), 152 deletions(-) diff --git a/Gemfile b/Gemfile index 26b29fc9..2a32b38b 100644 --- a/Gemfile +++ b/Gemfile @@ -63,6 +63,10 @@ gem "rotp", "~> 6.3" gem "rqrcode", "~> 3.0" gem "activerecord-import" +# State machines +gem "aasm" +gem "after_commit_everywhere", "~> 1.0" + # AI gem "ruby-openai" diff --git a/Gemfile.lock b/Gemfile.lock index 224f8f9c..81c6ccb0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -8,6 +8,8 @@ GIT GEM remote: https://rubygems.org/ specs: + aasm (5.5.0) + concurrent-ruby (~> 1.0) actioncable (7.2.2.1) actionpack (= 7.2.2.1) activesupport (= 7.2.2.1) @@ -83,6 +85,9 @@ GEM tzinfo (~> 2.0, >= 2.0.5) addressable (2.8.7) public_suffix (>= 2.0.2, < 7.0) + after_commit_everywhere (1.6.0) + activerecord (>= 4.2) + activesupport ast (2.4.3) aws-eventstream (1.3.2) aws-partitions (1.1093.0) @@ -561,7 +566,9 @@ PLATFORMS x86_64-linux-musl DEPENDENCIES + aasm activerecord-import + after_commit_everywhere (~> 1.0) aws-sdk-s3 (~> 1.177.0) bcrypt (~> 3.1) benchmark-ips diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index a1576aea..6b17b531 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -5,23 +5,15 @@ class Account::Syncer @account = account end - def child_syncables - [] - end - - def perform_sync(start_date: nil) + def perform_sync(sync:, start_date: nil) Rails.logger.info("Processing balances (#{account.linked? ? 'reverse' : 'forward'})") sync_balances end def perform_post_sync account.family.remove_syncing_notice! - - # account.accountable.post_sync(sync) - - # unless sync.child? + account.accountable.post_sync account.family.auto_match_transfers! - # end end private diff --git a/app/models/concerns/accountable.rb b/app/models/concerns/accountable.rb index 9d0ecb34..af9b04ab 100644 --- a/app/models/concerns/accountable.rb +++ b/app/models/concerns/accountable.rb @@ -68,7 +68,7 @@ module Accountable end end - def post_sync(sync) + def post_sync broadcast_replace_to( account, target: "chart_account_#{account.id}", diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 43afed5f..2d21a601 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -6,7 +6,7 @@ module Syncable end def syncing? - syncs.where(status: [ :syncing, :pending ]).any? + syncs.incomplete.any? end def sync_later(start_date: nil, parent_sync: nil) @@ -14,8 +14,12 @@ module Syncable SyncJob.perform_later(new_sync) end - def sync(start_date: nil) - syncs.create!(start_date: start_date).perform + def perform_sync(sync:, start_date: nil) + syncer.perform_sync(sync: sync, start_date: start_date) + end + + def perform_post_sync + syncer.perform_post_sync end def sync_error @@ -28,6 +32,10 @@ module Syncable private def latest_sync - syncs.order(created_at: :desc).first + syncs.ordered.first + end + + def syncer + self.class::Syncer.new(self) end end diff --git a/app/models/family/syncer.rb b/app/models/family/syncer.rb index 3134a65c..58836341 100644 --- a/app/models/family/syncer.rb +++ b/app/models/family/syncer.rb @@ -5,11 +5,7 @@ class Family::Syncer @family = family end - def child_syncables - family.plaid_items + family.accounts.manual - end - - def perform_sync(start_date: nil) + def perform_sync(sync:, start_date: nil) # We don't rely on this value to guard the app, but keep it eventually consistent family.sync_trial_status! @@ -17,10 +13,20 @@ class Family::Syncer family.rules.each do |rule| rule.apply_later end + + # Schedule child syncs + child_syncables.each do |syncable| + syncable.sync_later(start_date: start_date, parent_sync: sync) + end end def perform_post_sync family.auto_match_transfers! family.broadcast_refresh end + + private + def child_syncables + family.plaid_items + family.accounts.manual + end end diff --git a/app/models/import.rb b/app/models/import.rb index 3ea68015..e96d1fc1 100644 --- a/app/models/import.rb +++ b/app/models/import.rb @@ -62,7 +62,7 @@ class Import < ApplicationRecord def publish import! - family.sync + family.sync_later update! status: :complete rescue => error diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 031b586c..88cddeab 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -5,16 +5,16 @@ class PlaidItem::Syncer @plaid_item = plaid_item end - def child_syncables - plaid_item.accounts - end - - def perform_sync(start_date: nil) + def perform_sync(sync:, start_date: nil) begin Rails.logger.info("Fetching and loading Plaid data") fetch_and_load_plaid_data plaid_item.update!(status: :good) if plaid_item.requires_update? + plaid_item.accounts.each do |account| + account.sync_later(start_date: start_date, parent_sync: sync) + end + Rails.logger.info("Plaid data fetched and loaded") rescue Plaid::ApiError => e handle_plaid_error(e) diff --git a/app/models/sync.rb b/app/models/sync.rb index d3d9f225..bce26f85 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -1,111 +1,87 @@ class Sync < ApplicationRecord - Error = Class.new(StandardError) + include AASM belongs_to :syncable, polymorphic: true belongs_to :parent, class_name: "Sync", optional: true has_many :children, class_name: "Sync", foreign_key: :parent_id, dependent: :destroy - enum :status, { pending: "pending", syncing: "syncing", completed: "completed", failed: "failed" } - scope :ordered, -> { order(created_at: :desc) } + scope :incomplete, -> { where(status: [ :pending, :syncing ]) } - def child? - parent_id.present? - end + # Sync state machine + aasm column: :status do + state :pending, initial: true + state :syncing + state :completed + state :failed - def perform - Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do - start! + event :start, after_commit: :handle_start do + transitions from: :pending, to: :syncing + end - begin - syncer.perform_sync(start_date: start_date) + event :complete, after_commit: :handle_finalization do + transitions from: :syncing, to: :completed + end - # Schedule child syncables to sync later - syncer.child_syncables.each do |child_syncable| - child_syncable.sync_later(start_date: start_date, parent_sync: self) - end - - unless has_pending_child_syncs? - complete! - Rails.logger.info("Sync completed, starting post-sync") - syncer.perform_post_sync - Rails.logger.info("Post-sync completed") - end - rescue StandardError => error - fail! error, report_error: true - ensure - notify_parent_of_completion! if has_parent? - end + event :fail, after_commit: :handle_finalization do + transitions from: :syncing, to: :failed end end - def handle_child_completion_event + def perform(start_date: nil) + start! + + begin + syncable.perform_sync(sync: self, start_date: start_date) + attempt_finalization + rescue => e + fail! + handle_error(e) + end + end + + # If the sync doesn't have any in-progress children, finalize it. + def attempt_finalization Sync.transaction do - # We need this to ensure 2 child syncs don't update the parent at the exact same time with different results - # and cause the sync to hang in "syncing" status indefinitely - self.lock! + lock! - unless has_pending_child_syncs? - if has_failed_child_syncs? - fail!(Error.new("One or more child syncs failed")) - else - complete! - end + return unless all_children_finalized? - # If this sync is both a child and a parent, we need to notify the parent of completion - notify_parent_of_completion! if has_parent? - - syncer.perform_post_sync + if has_failed_children? + fail! + else + complete! end end end private - def syncer - "#{syncable_type}::Syncer".constantize.new(syncable) + def has_failed_children? + children.failed.any? end - def has_pending_child_syncs? - children.where(status: [ :pending, :syncing ]).any? + def all_children_finalized? + children.incomplete.empty? end - def has_failed_child_syncs? - children.where(status: :failed).any? - end + # Once sync finalizes, notify its parent and run its post-sync logic. + def handle_finalization + syncable.perform_post_sync - def has_parent? - parent_id.present? - end - - def notify_parent_of_completion! - parent.handle_child_completion_event - end - - def start! - Rails.logger.info("Starting sync") - update! status: :syncing - end - - def complete! - Rails.logger.info("Sync completed") - update! status: :completed, last_ran_at: Time.current - end - - def fail!(error, report_error: false) - Rails.logger.error("Sync failed: #{error.message}") - - if report_error - Sentry.capture_exception(error) do |scope| - scope.set_context("sync", { id: id, syncable_type: syncable_type, syncable_id: syncable_id }) - scope.set_tags(sync_id: id) - end + if parent + parent.attempt_finalization end + end - update!( - status: :failed, - error: error.message, - last_ran_at: Time.current - ) + def handle_error(error) + update!(error: error.message) + Sentry.capture_exception(error) do |scope| + scope.set_tags(sync_id: id) + end + end + + def handle_start + update!(last_ran_at: Time.current) end end diff --git a/test/interfaces/syncable_interface_test.rb b/test/interfaces/syncable_interface_test.rb index 41c34b4b..bd741675 100644 --- a/test/interfaces/syncable_interface_test.rb +++ b/test/interfaces/syncable_interface_test.rb @@ -7,14 +7,14 @@ module SyncableInterfaceTest test "can sync later" do assert_difference "@syncable.syncs.count", 1 do assert_enqueued_with job: SyncJob do - @syncable.sync_later + @syncable.sync_later(start_date: 2.days.ago.to_date) end end end - test "can sync" do - assert_difference "@syncable.syncs.count", 1 do - @syncable.sync(start_date: 2.days.ago.to_date) - end + test "can perform sync" do + mock_sync = mock + @syncable.class.any_instance.expects(:perform_sync).with(sync: mock_sync, start_date: 2.days.ago.to_date).once + @syncable.perform_sync(sync: mock_sync, start_date: 2.days.ago.to_date) end end diff --git a/test/models/family/syncer_test.rb b/test/models/family/syncer_test.rb index bc35e678..4ea82b00 100644 --- a/test/models/family/syncer_test.rb +++ b/test/models/family/syncer_test.rb @@ -12,8 +12,17 @@ class Family::SyncerTest < ActiveSupport::TestCase items_count = @family.plaid_items.count syncer = Family::Syncer.new(@family) - syncer.perform_sync(start_date: family_sync.start_date) - assert_equal manual_accounts_count + items_count, syncer.child_syncables.count + Account.any_instance + .expects(:sync_later) + .with(start_date: family_sync.start_date, parent_sync: family_sync) + .times(manual_accounts_count) + + PlaidItem.any_instance + .expects(:sync_later) + .with(start_date: family_sync.start_date, parent_sync: family_sync) + .times(items_count) + + syncer.perform_sync(sync: family_sync, start_date: family_sync.start_date) end end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 2919c2f2..5c689684 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -4,9 +4,10 @@ class SyncTest < ActiveSupport::TestCase include ActiveJob::TestHelper test "runs successful sync" do - sync = Sync.create!(syncable: accounts(:depository), last_ran_at: 1.day.ago) + syncable = accounts(:depository) + sync = Sync.create!(syncable: syncable, last_ran_at: 1.day.ago) - Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once + syncable.expects(:perform_sync).with(sync: sync, start_date: sync.start_date).once assert_equal "pending", sync.status @@ -19,8 +20,10 @@ class SyncTest < ActiveSupport::TestCase end test "handles sync errors" do - sync = Sync.create!(syncable: accounts(:depository), last_ran_at: 1.day.ago) - Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).raises(StandardError.new("test sync error")) + syncable = accounts(:depository) + sync = Sync.create!(syncable: syncable, last_ran_at: 1.day.ago) + + syncable.expects(:perform_sync).with(sync: sync, start_date: sync.start_date).raises(StandardError.new("test sync error")) assert_equal "pending", sync.status previously_ran_at = sync.last_ran_at @@ -33,54 +36,43 @@ class SyncTest < ActiveSupport::TestCase end test "can run nested syncs that alert the parent when complete" do - # Clear out fixture syncs - Sync.destroy_all - - # These fixtures represent a Parent -> Child -> Grandchild sync hierarchy - # Family -> PlaidItem -> Account family = families(:dylan_family) plaid_item = plaid_items(:one) account = accounts(:connected) - sync = Sync.create!(syncable: family) + family_sync = Sync.create!(syncable: family) + plaid_item_sync = Sync.create!(syncable: plaid_item, parent: family_sync) + account_sync = Sync.create!(syncable: account, parent: plaid_item_sync) - Family::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once - Family::Syncer.any_instance.expects(:perform_post_sync).once - Family::Syncer.any_instance.expects(:child_syncables).returns([ plaid_item ]) + assert_equal "pending", family_sync.status + assert_equal "pending", plaid_item_sync.status + assert_equal "pending", account_sync.status - PlaidItem::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once - PlaidItem::Syncer.any_instance.expects(:perform_post_sync).once - PlaidItem::Syncer.any_instance.expects(:child_syncables).returns([ account ]) + family.expects(:perform_sync).with(sync: family_sync, start_date: family_sync.start_date).once - Account::Syncer.any_instance.expects(:perform_sync).with(start_date: sync.start_date).once - Account::Syncer.any_instance.expects(:perform_post_sync).once - Account::Syncer.any_instance.expects(:child_syncables).returns([]) + family_sync.perform - sync.perform + assert_equal "syncing", family_sync.reload.status - assert_equal 1, family.syncs.count - assert_equal "syncing", family.syncs.first.status - assert_equal 1, plaid_item.syncs.count - assert_equal "pending", plaid_item.syncs.first.status + plaid_item.expects(:perform_sync).with(sync: plaid_item_sync, start_date: plaid_item_sync.start_date).once - # We have to perform jobs 2x because the child sync will schedule the grandchild sync, - # which then needs to be run. - perform_enqueued_jobs + plaid_item_sync.perform - assert_equal 1, family.syncs.count - assert_equal "syncing", family.syncs.first.status - assert_equal 1, plaid_item.syncs.count - assert_equal "syncing", plaid_item.syncs.first.status - assert_equal 1, account.syncs.count - assert_equal "pending", account.syncs.first.status + assert_equal "syncing", family_sync.reload.status + assert_equal "syncing", plaid_item_sync.reload.status - perform_enqueued_jobs + account.expects(:perform_sync).with(sync: account_sync, start_date: account_sync.start_date).once - assert_equal 1, family.syncs.count - assert_equal "completed", family.syncs.first.status - assert_equal 1, plaid_item.syncs.count - assert_equal "completed", plaid_item.syncs.first.status - assert_equal 1, account.syncs.count - assert_equal "completed", account.syncs.first.status + # Since these are accessed through `parent`, they won't necessarily be the same + # instance we configured above + Account.any_instance.expects(:perform_post_sync).once + PlaidItem.any_instance.expects(:perform_post_sync).once + Family.any_instance.expects(:perform_post_sync).once + + account_sync.perform + + assert_equal "completed", family_sync.reload.status + assert_equal "completed", plaid_item_sync.reload.status + assert_equal "completed", account_sync.reload.status end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 23f98faa..7eac9dde 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -17,6 +17,7 @@ require "rails/test_help" require "minitest/mock" require "minitest/autorun" require "mocha/minitest" +require "aasm/minitest" VCR.configure do |config| config.cassette_library_dir = "test/vcr_cassettes" -- 2.53.0 From 0fbafceea97b24a5b45709f3deaf6e328593a557 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 10:40:43 -0400 Subject: [PATCH 07/27] Basic market data sync cron --- Gemfile | 1 + Gemfile.lock | 16 ++ app/jobs/sync_market_data_job.rb | 9 + app/models/holding/forward_calculator.rb | 27 ++- app/models/holding/portfolio_cache.rb | 3 - app/models/market_data_syncer.rb | 183 ++++++++++++++++++++ app/models/provider.rb | 2 - config/initializers/sidekiq.rb | 5 + config/routes.rb | 1 + config/schedule.yml | 5 + config/sidekiq.yml | 3 +- test/models/holding/portfolio_cache_test.rb | 32 +--- test/models/market_data_syncer_test.rb | 71 ++++++++ 13 files changed, 306 insertions(+), 52 deletions(-) create mode 100644 app/jobs/sync_market_data_job.rb create mode 100644 app/models/market_data_syncer.rb create mode 100644 config/schedule.yml create mode 100644 test/models/market_data_syncer_test.rb diff --git a/Gemfile b/Gemfile index 2a32b38b..3ad4f0a8 100644 --- a/Gemfile +++ b/Gemfile @@ -29,6 +29,7 @@ gem "hotwire_combobox" # Background Jobs gem "sidekiq" +gem "sidekiq-cron" # Monitoring gem "vernier" diff --git a/Gemfile.lock b/Gemfile.lock index 81c6ccb0..a8c86724 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -144,6 +144,9 @@ GEM bigdecimal rexml crass (1.0.6) + cronex (0.15.0) + tzinfo + unicode (>= 0.4.4.5) css_parser (1.21.1) addressable csv (3.3.4) @@ -165,6 +168,8 @@ GEM rubocop (>= 1) smart_properties erubi (1.13.1) + et-orbi (1.2.11) + tzinfo event_stream_parser (1.0.0) faker (3.5.1) i18n (>= 1.8.11, < 2) @@ -187,6 +192,9 @@ GEM ffi (1.17.2-x86_64-linux-gnu) ffi (1.17.2-x86_64-linux-musl) foreman (0.88.1) + fugit (1.11.1) + et-orbi (~> 1, >= 1.2.11) + raabro (~> 1.4) globalid (1.2.1) activesupport (>= 6.1) hashdiff (1.1.2) @@ -351,6 +359,7 @@ GEM public_suffix (6.0.1) puma (6.6.0) nio4r (~> 2.0) + raabro (1.4.0) racc (1.8.1) rack (3.1.13) rack-mini-profiler (3.3.1) @@ -491,6 +500,11 @@ GEM logger (>= 1.6.2) rack (>= 3.1.0) redis-client (>= 0.23.2) + sidekiq-cron (2.2.0) + cronex (>= 0.13.0) + fugit (~> 1.8, >= 1.11.1) + globalid (>= 1.0.1) + sidekiq (>= 6.5.0) simplecov (0.22.0) docile (~> 1.1) simplecov-html (~> 0.11) @@ -524,6 +538,7 @@ GEM railties (>= 7.1.0) tzinfo (2.0.6) concurrent-ruby (~> 1.0) + unicode (0.4.4.5) unicode-display_width (3.1.4) unicode-emoji (~> 4.0, >= 4.0.4) unicode-emoji (4.0.4) @@ -619,6 +634,7 @@ DEPENDENCIES sentry-ruby sentry-sidekiq sidekiq + sidekiq-cron simplecov skylight stimulus-rails diff --git a/app/jobs/sync_market_data_job.rb b/app/jobs/sync_market_data_job.rb new file mode 100644 index 00000000..4c911593 --- /dev/null +++ b/app/jobs/sync_market_data_job.rb @@ -0,0 +1,9 @@ +class SyncMarketDataJob < ApplicationJob + queue_as :scheduled + + def perform(*args) + syncer = MarketDataSyncer.new + syncer.sync_exchange_rates + syncer.sync_prices + end +end diff --git a/app/models/holding/forward_calculator.rb b/app/models/holding/forward_calculator.rb index 22759416..43f91f7a 100644 --- a/app/models/holding/forward_calculator.rb +++ b/app/models/holding/forward_calculator.rb @@ -7,7 +7,17 @@ class Holding::ForwardCalculator def calculate Rails.logger.tagged("Holding::ForwardCalculator") do - holdings = calculate_holdings + current_portfolio = generate_starting_portfolio + next_portfolio = {} + holdings = [] + + account.start_date.upto(Date.current).each do |date| + trades = portfolio_cache.get_trades(date: date) + next_portfolio = transform_portfolio(current_portfolio, trades, direction: :forward) + holdings += build_holdings(next_portfolio, date) + current_portfolio = next_portfolio + end + Holding.gapfill(holdings) end end @@ -59,19 +69,4 @@ class Holding::ForwardCalculator ) end.compact end - - def calculate_holdings - current_portfolio = generate_starting_portfolio - next_portfolio = {} - holdings = [] - - account.start_date.upto(Date.current).each do |date| - trades = portfolio_cache.get_trades(date: date) - next_portfolio = transform_portfolio(current_portfolio, trades, direction: :forward) - holdings += build_holdings(next_portfolio, date) - current_portfolio = next_portfolio - end - - holdings - end end diff --git a/app/models/holding/portfolio_cache.rb b/app/models/holding/portfolio_cache.rb index 2d67a1d8..9ffed15b 100644 --- a/app/models/holding/portfolio_cache.rb +++ b/app/models/holding/portfolio_cache.rb @@ -83,9 +83,6 @@ class Holding::PortfolioCache securities.each do |security| Rails.logger.info "Loading security: ID=#{security.id} Ticker=#{security.ticker}" - # Load prices from provider to DB - security.sync_provider_prices(start_date: account.start_date) - # High priority prices from DB (synced from provider) db_prices = security.prices.where(date: account.start_date..Date.current).map do |price| PriceWithPriority.new( diff --git a/app/models/market_data_syncer.rb b/app/models/market_data_syncer.rb new file mode 100644 index 00000000..cd9c382f --- /dev/null +++ b/app/models/market_data_syncer.rb @@ -0,0 +1,183 @@ +class MarketDataSyncer + DEFAULT_HISTORY_DAYS = 30 + RATE_PROVIDER_NAME = :synth + PRICE_PROVIDER_NAME = :synth + + MissingExchangeRateError = Class.new(StandardError) + InvalidExchangeRateDataError = Class.new(StandardError) + MissingSecurityPriceError = Class.new(StandardError) + InvalidSecurityPriceDataError = Class.new(StandardError) + + class << self + def for(family: nil, account: nil) + new(family: family, account: account) + end + end + + # Syncer can optionally be scoped. Otherwise, it syncs all user data + def initialize(family: nil, account: nil) + @family = family + @account = account + end + + def sync_exchange_rates(full_history: false) + unless rate_provider + Rails.logger.warn("No rate provider configured for MarketDataSyncer.sync_exchange_rates, skipping sync") + return + end + + # Finds distinct currency pairs + entry_pairs = entries_scope.joins(:account) + .where.not("entries.currency = accounts.currency") + .select("entries.currency as source, accounts.currency as target") + .distinct + + # All accounts in currency not equal to the family currency require exchange rates to show a normalized historical graph + account_pairs = accounts_scope.joins(:family) + .where.not("families.currency = accounts.currency") + .select("accounts.currency as source, families.currency as target") + .distinct + + pairs = (entry_pairs + account_pairs).uniq + + pairs.each do |pair| + sync_exchange_rate(from: pair.source, to: pair.target, full_history: full_history) + end + end + + def sync_prices(full_history: false) + unless price_provider + Rails.logger.warn("No price provider configured for MarketDataSyncer.sync_prices, skipping sync") + nil + end + + securities_scope.each do |security| + sync_security_price(security: security, full_history: full_history) + end + end + + private + attr_reader :family, :account + + def accounts_scope + return Account.where(id: account.id) if account + return family.accounts if family + Account.all + end + + def entries_scope + account&.entries || family&.entries || Entry.all + end + + def securities_scope + if account + account.trades.joins(:security).where.not(securities: { exchange_operating_mic: nil }) + elsif family + family.trades.joins(:security).where.not(securities: { exchange_operating_mic: nil }) + else + Security.where.not(exchange_operating_mic: nil) + end + end + + def sync_security_price(security:, full_history:) + start_date = full_history ? find_oldest_required_price(security: security) : default_start_date + + Rails.logger.info("Syncing security price for: #{security.ticker}, start_date: #{start_date}, end_date: #{end_date}") + + fetched_prices = price_provider.fetch_security_prices( + security.ticker, + start_date: start_date, + end_date: end_date + ) + + unless fetched_prices.success? + message = "#{PRICE_PROVIDER_NAME} could not fetch security price for: #{security.ticker} between: #{start_date} and: #{Date.current}. Provider error: #{fetched_prices.error.message}" + Rails.logger.warn(message) + Sentry.capture_exception(MissingSecurityPriceError.new(message)) + return + end + + prices_for_upsert = fetched_prices.data.map do |price| + if price.security.nil? || price.date.nil? || price.price.nil? || price.currency.nil? + message = "#{PRICE_PROVIDER_NAME} returned invalid price data for security: #{security.ticker} on: #{price.date}. Price data: #{price.inspect}" + Rails.logger.warn(message) + Sentry.capture_exception(InvalidSecurityPriceDataError.new(message)) + next + end + + { + security_id: price.security.id, + date: price.date, + price: price.price, + currency: price.currency + } + end.compact + + Security::Price.upsert_all( + prices_for_upsert, + unique_by: %i[security_id date currency] + ) + end + + def sync_exchange_rate(from:, to:, full_history:) + start_date = full_history ? find_oldest_required_rate(from_currency: from) : default_start_date + + Rails.logger.info("Syncing exchange rate from: #{from}, to: #{to}, start_date: #{start_date}, end_date: #{end_date}") + + fetched_rates = rate_provider.fetch_exchange_rates( + from: from, + to: to, + start_date: start_date, + end_date: end_date + ) + + unless fetched_rates.success? + message = "#{RATE_PROVIDER_NAME} could not fetch exchange rate pair from: #{from} to: #{to} between: #{start_date} and: #{Date.current}. Provider error: #{fetched_rates.error.message}" + Rails.logger.warn(message) + Sentry.capture_exception(MissingExchangeRateError.new(message)) + return + end + + rates_for_upsert = fetched_rates.data.map do |rate| + if rate.from.nil? || rate.to.nil? || rate.date.nil? || rate.rate.nil? + message = "#{RATE_PROVIDER_NAME} returned invalid rate data for pair from: #{from} to: #{to} on: #{rate.date}. Rate data: #{rate.inspect}" + Rails.logger.warn(message) + Sentry.capture_exception(InvalidExchangeRateDataError.new(message)) + next + end + + { + from_currency: rate.from, + to_currency: rate.to, + date: rate.date, + rate: rate.rate + } + end.compact + + ExchangeRate.upsert_all( + rates_for_upsert, + unique_by: %i[from_currency to_currency date] + ) + end + + def rate_provider + Provider::Registry.for_concept(:exchange_rates).get_provider(RATE_PROVIDER_NAME) + end + + def price_provider + Provider::Registry.for_concept(:securities).get_provider(PRICE_PROVIDER_NAME) + end + + def find_oldest_required_rate(from_currency:) + entries_scope.where(currency: from_currency).minimum(:date) || default_start_date + end + + def default_start_date + DEFAULT_HISTORY_DAYS.days.ago.to_date + end + + # Since we're querying market data from a US-based API, end date should always be today (EST) + def end_date + Date.current.in_time_zone("America/New_York").to_date + end +end diff --git a/app/models/provider.rb b/app/models/provider.rb index c90866e7..e9702349 100644 --- a/app/models/provider.rb +++ b/app/models/provider.rb @@ -36,8 +36,6 @@ class Provider default_error_transformer(error) end - Sentry.capture_exception(transformed_error) - Response.new( success?: false, data: nil, diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 1209a4fa..9040f864 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -7,3 +7,8 @@ Sidekiq::Web.use(Rack::Auth::Basic) do |username, password| ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(username), configured_username) && ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(password), configured_password) end + +Sidekiq::Cron.configure do |config| + # 10 min "catch-up" window in case worker process is re-deploying when cron tick occurs + config.reschedule_grace_period = 600 +end diff --git a/config/routes.rb b/config/routes.rb index 8384d116..e44265f6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,4 +1,5 @@ require "sidekiq/web" +require "sidekiq/cron/web" Rails.application.routes.draw do # MFA routes diff --git a/config/schedule.yml b/config/schedule.yml new file mode 100644 index 00000000..561e2327 --- /dev/null +++ b/config/schedule.yml @@ -0,0 +1,5 @@ +sync_market_data: + cron: "0 17 * * 1-5" # 5:00 PM EST (1 hour after market close) + class: "SyncMarketDataJob" + queue: "scheduled" + description: "Syncs market data daily at 5:00 PM EST (1 hour after market close)" diff --git a/config/sidekiq.yml b/config/sidekiq.yml index 20343e8c..4fce6f00 100644 --- a/config/sidekiq.yml +++ b/config/sidekiq.yml @@ -1,6 +1,7 @@ concurrency: <%= ENV.fetch("RAILS_MAX_THREADS") { 3 } %> queues: - - [high_priority, 6] + - [scheduled, 10] # For cron-like jobs (e.g. "daily market data sync") + - [high_priority, 4] - [medium_priority, 2] - [low_priority, 1] - [default, 1] diff --git a/test/models/holding/portfolio_cache_test.rb b/test/models/holding/portfolio_cache_test.rb index 2518e2b7..4677fc41 100644 --- a/test/models/holding/portfolio_cache_test.rb +++ b/test/models/holding/portfolio_cache_test.rb @@ -28,37 +28,18 @@ class Holding::PortfolioCacheTest < ActiveSupport::TestCase price: db_price ) - expect_provider_prices([], start_date: @account.start_date) - cache = Holding::PortfolioCache.new(@account) assert_equal db_price, cache.get_price(@security.id, Date.current).price end - test "if no price in DB, try fetching from provider" do - Security::Price.delete_all - - provider_price = Security::Price.new( - security: @security, - date: Date.current, - price: 220, - currency: "USD" - ) - - expect_provider_prices([ provider_price ], start_date: @account.start_date) - - cache = Holding::PortfolioCache.new(@account) - assert_equal provider_price.price, cache.get_price(@security.id, Date.current).price - end - - test "if no price from db or provider, try getting the price from trades" do + test "if no price from db, try getting the price from trades" do Security::Price.destroy_all - expect_provider_prices([], start_date: @account.start_date) cache = Holding::PortfolioCache.new(@account) assert_equal @trade.price, cache.get_price(@security.id, @trade.entry.date).price end - test "if no price from db, provider, or trades, search holdings" do + test "if no price from db or trades, search holdings" do Security::Price.delete_all Entry.delete_all @@ -72,16 +53,7 @@ class Holding::PortfolioCacheTest < ActiveSupport::TestCase currency: "USD" ) - expect_provider_prices([], start_date: @account.start_date) - cache = Holding::PortfolioCache.new(@account, use_holdings: true) assert_equal holding.price, cache.get_price(@security.id, holding.date).price end - - private - def expect_provider_prices(prices, start_date:, end_date: Date.current) - @provider.expects(:fetch_security_prices) - .with(@security, start_date: start_date, end_date: end_date) - .returns(provider_success_response(prices)) - end end diff --git a/test/models/market_data_syncer_test.rb b/test/models/market_data_syncer_test.rb new file mode 100644 index 00000000..8f00e753 --- /dev/null +++ b/test/models/market_data_syncer_test.rb @@ -0,0 +1,71 @@ +require "test_helper" +require "ostruct" + +class MarketDataSyncerTest < ActiveSupport::TestCase + include EntriesTestHelper, ProviderTestHelper + + test "syncs exchange rates with upsert" do + empty_db + + family1 = Family.create!(name: "Family 1", currency: "USD") + account1 = family1.accounts.create!(name: "Account 1", currency: "USD", balance: 100, accountable: Depository.new) + account2 = family1.accounts.create!(name: "Account 2", currency: "CAD", balance: 100, accountable: Depository.new) + + family2 = Family.create!(name: "Family 2", currency: "EUR") + account3 = family2.accounts.create!(name: "Account 3", currency: "EUR", balance: 100, accountable: Depository.new) + account4 = family2.accounts.create!(name: "Account 4", currency: "USD", balance: 100, accountable: Depository.new) + + mock_provider = mock + Provider::Registry.any_instance.expects(:get_provider).with(:synth).returns(mock_provider).at_least_once + + start_date = 1.month.ago.to_date + end_date = Date.current.in_time_zone("America/New_York").to_date + + # Put an existing rate in DB to test upsert + ExchangeRate.create!(from_currency: "CAD", to_currency: "USD", date: start_date, rate: 2.0) + + mock_provider.expects(:fetch_exchange_rates) + .with(from: "CAD", to: "USD", start_date: start_date, end_date: end_date) + .returns(provider_success_response([ OpenStruct.new(from: "CAD", to: "USD", date: start_date, rate: 1.0) ])) + + mock_provider.expects(:fetch_exchange_rates) + .with(from: "USD", to: "EUR", start_date: start_date, end_date: end_date) + .returns(provider_success_response([ OpenStruct.new(from: "USD", to: "EUR", date: start_date, rate: 1.0) ])) + + assert_difference "ExchangeRate.count", 1 do + MarketDataSyncer.new.sync_exchange_rates + end + + assert_equal 1.0, ExchangeRate.where(from_currency: "CAD", to_currency: "USD", date: start_date).first.rate + end + + test "syncs security prices with upsert" do + empty_db + + aapl = Security.create!(ticker: "AAPL", exchange_operating_mic: "XNAS") + + family = Family.create!(name: "Family 1", currency: "USD") + account = family.accounts.create!(name: "Account 1", currency: "USD", balance: 100, accountable: Investment.new) + + mock_provider = mock + Provider::Registry.any_instance.expects(:get_provider).with(:synth).returns(mock_provider).at_least_once + + start_date = 1.month.ago.to_date + end_date = Date.current.in_time_zone("America/New_York").to_date + + mock_provider.expects(:fetch_security_prices) + .with("AAPL", start_date: start_date, end_date: end_date) + .returns(provider_success_response([ OpenStruct.new(security: aapl, date: start_date, price: 100, currency: "USD") ])) + + assert_difference "Security::Price.count", 1 do + MarketDataSyncer.new.sync_prices + end + end + + private + def empty_db + Invitation.destroy_all + Family.destroy_all + Security.destroy_all + end +end -- 2.53.0 From 7988ab64cc7f9d6e73549bd99297df841d9674e7 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 10:49:33 -0400 Subject: [PATCH 08/27] Fix price sync --- app/models/market_data_syncer.rb | 2 +- test/models/market_data_syncer_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/market_data_syncer.rb b/app/models/market_data_syncer.rb index cd9c382f..8e4baf43 100644 --- a/app/models/market_data_syncer.rb +++ b/app/models/market_data_syncer.rb @@ -85,7 +85,7 @@ class MarketDataSyncer Rails.logger.info("Syncing security price for: #{security.ticker}, start_date: #{start_date}, end_date: #{end_date}") fetched_prices = price_provider.fetch_security_prices( - security.ticker, + security, start_date: start_date, end_date: end_date ) diff --git a/test/models/market_data_syncer_test.rb b/test/models/market_data_syncer_test.rb index 8f00e753..299fb82e 100644 --- a/test/models/market_data_syncer_test.rb +++ b/test/models/market_data_syncer_test.rb @@ -54,7 +54,7 @@ class MarketDataSyncerTest < ActiveSupport::TestCase end_date = Date.current.in_time_zone("America/New_York").to_date mock_provider.expects(:fetch_security_prices) - .with("AAPL", start_date: start_date, end_date: end_date) + .with(aapl, start_date: start_date, end_date: end_date) .returns(provider_success_response([ OpenStruct.new(security: aapl, date: start_date, price: 100, currency: "USD") ])) assert_difference "Security::Price.count", 1 do -- 2.53.0 From 6a1d625a4f98895bf81fdcea476edae5021df86d Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 14:40:17 -0400 Subject: [PATCH 09/27] Improve sync window column names, add timestamps --- app/models/account/syncer.rb | 2 +- app/models/concerns/syncable.rb | 10 ++-- app/models/entry.rb | 2 +- app/models/family/syncer.rb | 4 +- app/models/market_data_syncer.rb | 20 +++++--- app/models/plaid_item/syncer.rb | 22 ++++---- app/models/sync.rb | 16 +++--- .../20250512171654_update_sync_timestamps.rb | 51 +++++++++++++++++++ db/schema.rb | 11 ++-- test/fixtures/syncs.yml | 6 +-- test/interfaces/syncable_interface_test.rb | 6 +-- test/jobs/sync_job_test.rb | 2 +- test/models/account/entry_test.rb | 6 +-- test/models/family/syncer_test.rb | 8 +-- test/models/sync_test.rb | 21 ++++---- 15 files changed, 126 insertions(+), 61 deletions(-) create mode 100644 db/migrate/20250512171654_update_sync_timestamps.rb diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index 6b17b531..b89bb9ca 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -5,7 +5,7 @@ class Account::Syncer @account = account end - def perform_sync(sync:, start_date: nil) + def perform_sync(sync) Rails.logger.info("Processing balances (#{account.linked? ? 'reverse' : 'forward'})") sync_balances end diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 2d21a601..4214daa0 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -9,13 +9,13 @@ module Syncable syncs.incomplete.any? end - def sync_later(start_date: nil, parent_sync: nil) - new_sync = syncs.create!(start_date: start_date, parent: parent_sync) + def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil) + new_sync = syncs.create!(parent: parent_sync, window_start_date: window_start_date, window_end_date: window_end_date) SyncJob.perform_later(new_sync) end - def perform_sync(sync:, start_date: nil) - syncer.perform_sync(sync: sync, start_date: start_date) + def perform_sync(sync) + syncer.perform_sync(sync) end def perform_post_sync @@ -27,7 +27,7 @@ module Syncable end def last_synced_at - latest_sync&.last_ran_at + latest_sync&.completed_at end private diff --git a/app/models/entry.rb b/app/models/entry.rb index c07f27cf..36f61c29 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -45,7 +45,7 @@ class Entry < ApplicationRecord def sync_account_later sync_start_date = [ date_previously_was, date ].compact.min unless destroyed? - account.sync_later(start_date: sync_start_date) + account.sync_later(window_start_date: sync_start_date) end def entryable_name_short diff --git a/app/models/family/syncer.rb b/app/models/family/syncer.rb index 58836341..ac06bce9 100644 --- a/app/models/family/syncer.rb +++ b/app/models/family/syncer.rb @@ -5,7 +5,7 @@ class Family::Syncer @family = family end - def perform_sync(sync:, start_date: nil) + def perform_sync(sync) # We don't rely on this value to guard the app, but keep it eventually consistent family.sync_trial_status! @@ -16,7 +16,7 @@ class Family::Syncer # Schedule child syncs child_syncables.each do |syncable| - syncable.sync_later(start_date: start_date, parent_sync: sync) + syncable.sync_later(parent_sync: sync, window_start_date: sync.window_start_date, window_end_date: sync.window_end_date) end end diff --git a/app/models/market_data_syncer.rb b/app/models/market_data_syncer.rb index 8e4baf43..785c6b05 100644 --- a/app/models/market_data_syncer.rb +++ b/app/models/market_data_syncer.rb @@ -91,17 +91,25 @@ class MarketDataSyncer ) unless fetched_prices.success? - message = "#{PRICE_PROVIDER_NAME} could not fetch security price for: #{security.ticker} between: #{start_date} and: #{Date.current}. Provider error: #{fetched_prices.error.message}" - Rails.logger.warn(message) - Sentry.capture_exception(MissingSecurityPriceError.new(message)) + error = MissingSecurityPriceError.new( + "#{PRICE_PROVIDER_NAME} could not fetch security price for: #{security.ticker} between: #{start_date} and: #{Date.current}. Provider error: #{fetched_prices.error.message}" + ) + + Rails.logger.warn(error.message) + Sentry.capture_exception(error, level: :warning) + return end prices_for_upsert = fetched_prices.data.map do |price| if price.security.nil? || price.date.nil? || price.price.nil? || price.currency.nil? - message = "#{PRICE_PROVIDER_NAME} returned invalid price data for security: #{security.ticker} on: #{price.date}. Price data: #{price.inspect}" - Rails.logger.warn(message) - Sentry.capture_exception(InvalidSecurityPriceDataError.new(message)) + error = InvalidSecurityPriceDataError.new( + "#{PRICE_PROVIDER_NAME} returned invalid price data for security: #{security.ticker} on: #{price.date}. Price data: #{price.inspect}" + ) + + Rails.logger.warn(error.message) + Sentry.capture_exception(error, level: :warning) + next end diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 88cddeab..4669e9b8 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -5,14 +5,14 @@ class PlaidItem::Syncer @plaid_item = plaid_item end - def perform_sync(sync:, start_date: nil) + def perform_sync(sync) begin Rails.logger.info("Fetching and loading Plaid data") fetch_and_load_plaid_data plaid_item.update!(status: :good) if plaid_item.requires_update? plaid_item.accounts.each do |account| - account.sync_later(start_date: start_date, parent_sync: sync) + account.sync_later(parent_sync: sync, window_start_date: sync.window_start_date, window_end_date: sync.window_end_date) end Rails.logger.info("Plaid data fetched and loaded") @@ -42,9 +42,9 @@ class PlaidItem::Syncer def safe_fetch_plaid_data(method) begin - plaid.send(method, self) + plaid.send(method, plaid_item) rescue Plaid::ApiError => e - Rails.logger.warn("Error fetching #{method} for item #{id}: #{e.message}") + Rails.logger.warn("Error fetching #{method} for item #{plaid_item.id}: #{e.message}") nil end end @@ -53,7 +53,7 @@ class PlaidItem::Syncer error_body = JSON.parse(error.response_body) if error_body["error_code"] == "ITEM_LOGIN_REQUIRED" - update!(status: :requires_update) + plaid_item.update!(status: :requires_update) end end @@ -64,14 +64,14 @@ class PlaidItem::Syncer Rails.logger.info "Starting Plaid data fetch (accounts, transactions, investments, liabilities)" item = plaid.get_item(plaid_item.access_token).item - update!(available_products: item.available_products, billed_products: item.billed_products) + plaid_item.update!(available_products: item.available_products, billed_products: item.billed_products) # Institution details if item.institution_id.present? begin Rails.logger.info "Fetching Plaid institution details for #{item.institution_id}" institution = plaid.get_institution(item.institution_id) - update!( + plaid_item.update!( institution_id: item.institution_id, institution_url: institution.institution.url, institution_color: institution.institution.primary_color @@ -98,7 +98,7 @@ class PlaidItem::Syncer if fetched_transactions Rails.logger.info "Processing Plaid transactions (added: #{fetched_transactions.added.size}, modified: #{fetched_transactions.modified.size}, removed: #{fetched_transactions.removed.size})" - transaction do + PlaidItem.transaction do internal_plaid_accounts.each do |internal_plaid_account| added = fetched_transactions.added.select { |t| t.account_id == internal_plaid_account.plaid_id } modified = fetched_transactions.modified.select { |t| t.account_id == internal_plaid_account.plaid_id } @@ -107,7 +107,7 @@ class PlaidItem::Syncer internal_plaid_account.sync_transactions!(added:, modified:, removed:) end - update!(next_cursor: fetched_transactions.cursor) + plaid_item.update!(next_cursor: fetched_transactions.cursor) end end @@ -117,7 +117,7 @@ class PlaidItem::Syncer if fetched_investments Rails.logger.info "Processing Plaid investments (transactions: #{fetched_investments.transactions.size}, holdings: #{fetched_investments.holdings.size}, securities: #{fetched_investments.securities.size})" - transaction do + PlaidItem.transaction do internal_plaid_accounts.each do |internal_plaid_account| transactions = fetched_investments.transactions.select { |t| t.account_id == internal_plaid_account.plaid_id } holdings = fetched_investments.holdings.select { |h| h.account_id == internal_plaid_account.plaid_id } @@ -134,7 +134,7 @@ class PlaidItem::Syncer if fetched_liabilities Rails.logger.info "Processing Plaid liabilities (credit: #{fetched_liabilities.credit&.size || 0}, mortgage: #{fetched_liabilities.mortgage&.size || 0}, student: #{fetched_liabilities.student&.size || 0})" - transaction do + PlaidItem.transaction do internal_plaid_accounts.each do |internal_plaid_account| credit = fetched_liabilities.credit&.find { |l| l.account_id == internal_plaid_account.plaid_id } mortgage = fetched_liabilities.mortgage&.find { |l| l.account_id == internal_plaid_account.plaid_id } diff --git a/app/models/sync.rb b/app/models/sync.rb index bce26f85..f9648726 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -9,14 +9,16 @@ class Sync < ApplicationRecord scope :ordered, -> { order(created_at: :desc) } scope :incomplete, -> { where(status: [ :pending, :syncing ]) } + validate :window_valid + # Sync state machine - aasm column: :status do + aasm column: :status, timestamps: true do state :pending, initial: true state :syncing state :completed state :failed - event :start, after_commit: :handle_start do + event :start do transitions from: :pending, to: :syncing end @@ -29,11 +31,11 @@ class Sync < ApplicationRecord end end - def perform(start_date: nil) + def perform(window_start_date: nil, window_end_date: nil) start! begin - syncable.perform_sync(sync: self, start_date: start_date) + syncable.perform_sync(self) attempt_finalization rescue => e fail! @@ -81,7 +83,9 @@ class Sync < ApplicationRecord end end - def handle_start - update!(last_ran_at: Time.current) + 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 end diff --git a/db/migrate/20250512171654_update_sync_timestamps.rb b/db/migrate/20250512171654_update_sync_timestamps.rb new file mode 100644 index 00000000..716f7019 --- /dev/null +++ b/db/migrate/20250512171654_update_sync_timestamps.rb @@ -0,0 +1,51 @@ +class UpdateSyncTimestamps < ActiveRecord::Migration[7.2] + def change + # Timestamps, managed by aasm + add_column :syncs, :pending_at, :datetime + add_column :syncs, :syncing_at, :datetime + add_column :syncs, :completed_at, :datetime + add_column :syncs, :failed_at, :datetime + + add_column :syncs, :window_start_date, :date + add_column :syncs, :window_end_date, :date + + reversible do |dir| + dir.up do + execute <<-SQL + UPDATE syncs + SET + completed_at = CASE + WHEN status = 'completed' THEN last_ran_at + ELSE NULL + END, + failed_at = CASE + WHEN status = 'failed' THEN last_ran_at + ELSE NULL + END + SQL + + execute <<-SQL + UPDATE syncs + SET window_start_date = start_date + SQL + end + + dir.down do + execute <<-SQL + UPDATE syncs + SET + last_ran_at = completed_at + SQL + + execute <<-SQL + UPDATE syncs + SET start_date = window_start_date + SQL + end + end + + remove_column :syncs, :start_date, :date + remove_column :syncs, :last_ran_at, :datetime + remove_column :syncs, :error_backtrace, :text, array: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 6d76a125..4de66be4 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_05_09_182903) do +ActiveRecord::Schema[7.2].define(version: 2025_05_12_171654) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -586,15 +586,18 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_09_182903) do create_table "syncs", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "syncable_type", null: false t.uuid "syncable_id", null: false - t.datetime "last_ran_at" - t.date "start_date" t.string "status", default: "pending" t.string "error" t.jsonb "data" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.text "error_backtrace", array: true t.uuid "parent_id" + t.datetime "pending_at" + t.datetime "syncing_at" + t.datetime "completed_at" + t.datetime "failed_at" + t.date "window_start_date" + t.date "window_end_date" t.index ["parent_id"], name: "index_syncs_on_parent_id" t.index ["syncable_type", "syncable_id"], name: "index_syncs_on_syncable" end diff --git a/test/fixtures/syncs.yml b/test/fixtures/syncs.yml index 1b010568..5d954280 100644 --- a/test/fixtures/syncs.yml +++ b/test/fixtures/syncs.yml @@ -1,17 +1,17 @@ account: syncable_type: Account syncable: depository - last_ran_at: <%= Time.now %> status: completed + completed_at: <%= Time.now %> plaid_item: syncable_type: PlaidItem syncable: one - last_ran_at: <%= Time.now %> status: completed + completed_at: <%= Time.now %> family: syncable_type: Family syncable: dylan_family - last_ran_at: <%= Time.now %> status: completed + completed_at: <%= Time.now %> diff --git a/test/interfaces/syncable_interface_test.rb b/test/interfaces/syncable_interface_test.rb index bd741675..a9f1fa7d 100644 --- a/test/interfaces/syncable_interface_test.rb +++ b/test/interfaces/syncable_interface_test.rb @@ -7,14 +7,14 @@ module SyncableInterfaceTest test "can sync later" do assert_difference "@syncable.syncs.count", 1 do assert_enqueued_with job: SyncJob do - @syncable.sync_later(start_date: 2.days.ago.to_date) + @syncable.sync_later(window_start_date: 2.days.ago.to_date) end end end test "can perform sync" do mock_sync = mock - @syncable.class.any_instance.expects(:perform_sync).with(sync: mock_sync, start_date: 2.days.ago.to_date).once - @syncable.perform_sync(sync: mock_sync, start_date: 2.days.ago.to_date) + @syncable.class.any_instance.expects(:perform_sync).with(mock_sync).once + @syncable.perform_sync(mock_sync) end end diff --git a/test/jobs/sync_job_test.rb b/test/jobs/sync_job_test.rb index b8d34400..b392b3a3 100644 --- a/test/jobs/sync_job_test.rb +++ b/test/jobs/sync_job_test.rb @@ -4,7 +4,7 @@ class SyncJobTest < ActiveJob::TestCase test "sync is performed" do syncable = accounts(:depository) - sync = syncable.syncs.create!(start_date: 2.days.ago.to_date) + sync = syncable.syncs.create!(window_start_date: 2.days.ago.to_date) sync.expects(:perform).once diff --git a/test/models/account/entry_test.rb b/test/models/account/entry_test.rb index 5a07ad6e..edd55e68 100644 --- a/test/models/account/entry_test.rb +++ b/test/models/account/entry_test.rb @@ -30,7 +30,7 @@ class EntryTest < ActiveSupport::TestCase prior_date = @entry.date - 1 @entry.update! date: prior_date - @entry.account.expects(:sync_later).with(start_date: prior_date) + @entry.account.expects(:sync_later).with(window_start_date: prior_date) @entry.sync_account_later end @@ -38,14 +38,14 @@ class EntryTest < ActiveSupport::TestCase prior_date = @entry.date @entry.update! date: @entry.date + 1 - @entry.account.expects(:sync_later).with(start_date: prior_date) + @entry.account.expects(:sync_later).with(window_start_date: prior_date) @entry.sync_account_later end test "triggers sync with correct start date when transaction deleted" do @entry.destroy! - @entry.account.expects(:sync_later).with(start_date: nil) + @entry.account.expects(:sync_later).with(window_start_date: nil) @entry.sync_account_later end diff --git a/test/models/family/syncer_test.rb b/test/models/family/syncer_test.rb index 4ea82b00..7fe01a7e 100644 --- a/test/models/family/syncer_test.rb +++ b/test/models/family/syncer_test.rb @@ -15,14 +15,16 @@ class Family::SyncerTest < ActiveSupport::TestCase Account.any_instance .expects(:sync_later) - .with(start_date: family_sync.start_date, parent_sync: family_sync) + .with(parent_sync: family_sync, window_start_date: nil, window_end_date: nil) .times(manual_accounts_count) PlaidItem.any_instance .expects(:sync_later) - .with(start_date: family_sync.start_date, parent_sync: family_sync) + .with(parent_sync: family_sync, window_start_date: nil, window_end_date: nil) .times(items_count) - syncer.perform_sync(sync: family_sync, start_date: family_sync.start_date) + syncer.perform_sync(family_sync) + + assert_equal "completed", family_sync.reload.status end end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 5c689684..bd5de249 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -5,32 +5,29 @@ class SyncTest < ActiveSupport::TestCase test "runs successful sync" do syncable = accounts(:depository) - sync = Sync.create!(syncable: syncable, last_ran_at: 1.day.ago) + sync = Sync.create!(syncable: syncable) - syncable.expects(:perform_sync).with(sync: sync, start_date: sync.start_date).once + syncable.expects(:perform_sync).with(sync).once assert_equal "pending", sync.status - previously_ran_at = sync.last_ran_at - sync.perform - assert sync.last_ran_at > previously_ran_at + assert sync.completed_at < Time.now assert_equal "completed", sync.status end test "handles sync errors" do syncable = accounts(:depository) - sync = Sync.create!(syncable: syncable, last_ran_at: 1.day.ago) + sync = Sync.create!(syncable: syncable) - syncable.expects(:perform_sync).with(sync: sync, start_date: sync.start_date).raises(StandardError.new("test sync error")) + syncable.expects(:perform_sync).with(sync).raises(StandardError.new("test sync error")) assert_equal "pending", sync.status - previously_ran_at = sync.last_ran_at sync.perform - assert sync.last_ran_at > previously_ran_at + assert sync.failed_at < Time.now assert_equal "failed", sync.status assert_equal "test sync error", sync.error end @@ -48,20 +45,20 @@ class SyncTest < ActiveSupport::TestCase assert_equal "pending", plaid_item_sync.status assert_equal "pending", account_sync.status - family.expects(:perform_sync).with(sync: family_sync, start_date: family_sync.start_date).once + family.expects(:perform_sync).with(family_sync).once family_sync.perform assert_equal "syncing", family_sync.reload.status - plaid_item.expects(:perform_sync).with(sync: plaid_item_sync, start_date: plaid_item_sync.start_date).once + plaid_item.expects(:perform_sync).with(plaid_item_sync).once plaid_item_sync.perform assert_equal "syncing", family_sync.reload.status assert_equal "syncing", plaid_item_sync.reload.status - account.expects(:perform_sync).with(sync: account_sync, start_date: account_sync.start_date).once + account.expects(:perform_sync).with(account_sync).once # Since these are accessed through `parent`, they won't necessarily be the same # instance we configured above -- 2.53.0 From efea7bfc01cef9b88a85fc06683b1a8a032d83c1 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 15:01:39 -0400 Subject: [PATCH 10/27] 30 day syncs by default --- app/models/concerns/syncable.rb | 2 +- app/models/sync.rb | 9 ++++++++- app/views/accounts/index.html.erb | 20 +++++++++++--------- app/views/accounts/show/_header.html.erb | 17 +++-------------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 4214daa0..e33c1ac9 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -10,7 +10,7 @@ module Syncable end def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil) - new_sync = syncs.create!(parent: parent_sync, window_start_date: window_start_date, window_end_date: window_end_date) + new_sync = syncs.create_with_defaults!(parent: parent_sync) SyncJob.perform_later(new_sync) end diff --git a/app/models/sync.rb b/app/models/sync.rb index f9648726..325b5730 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -31,7 +31,14 @@ class Sync < ApplicationRecord end end - def perform(window_start_date: nil, window_end_date: nil) + class << self + # By default, we sync the "visible" window of data (user sees 30 day graphs by default) + def create_with_defaults!(parent: nil) + create!(parent: parent, window_start_date: 30.days.ago.to_date) + end + end + + def perform start! begin diff --git a/app/views/accounts/index.html.erb b/app/views/accounts/index.html.erb index 4893e44c..5c99a4f2 100644 --- a/app/views/accounts/index.html.erb +++ b/app/views/accounts/index.html.erb @@ -2,15 +2,17 @@

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

- <%= render ButtonComponent.new( - text: "Sync all", - href: sync_all_accounts_path, - method: :post, - variant: "outline", - disabled: Current.family.syncing?, - icon: "refresh-cw", - class: "" - ) %> + <% if Rails.env.development? %> + <%= render ButtonComponent.new( + text: "Sync all", + href: sync_all_accounts_path, + method: :post, + variant: "outline", + disabled: Current.family.syncing?, + icon: "refresh-cw", + class: "" + ) %> + <% end %> <%= render LinkComponent.new( text: "New account", diff --git a/app/views/accounts/show/_header.html.erb b/app/views/accounts/show/_header.html.erb index 4c60e832..b64408ce 100644 --- a/app/views/accounts/show/_header.html.erb +++ b/app/views/accounts/show/_header.html.erb @@ -20,26 +20,15 @@ <% end %>
- <% if account.plaid_account_id.present? %> - <% if Rails.env.development? %> - <%= icon( + <% if Rails.env.development? %> + <%= icon( "refresh-cw", as_button: true, size: "sm", - href: sync_plaid_item_path(account.plaid_account.plaid_item), + href: account.linked? ? sync_plaid_item_path(account.plaid_account.plaid_item) : sync_account_path(account), disabled: account.syncing?, frame: :_top ) %> - <% end %> - <% else %> - <%= icon( - "refresh-cw", - as_button: true, - size: "sm", - href: sync_account_path(account), - disabled: account.syncing?, - frame: :_top - ) %> <% end %> <%= render "accounts/show/menu", account: account %> -- 2.53.0 From 267692d29711ee0ee3700e9b3b968cbd0ff72160 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 15:22:24 -0400 Subject: [PATCH 11/27] Clean up market data methods --- app/jobs/sync_market_data_job.rb | 4 +--- app/models/market_data_syncer.rb | 5 +++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/jobs/sync_market_data_job.rb b/app/jobs/sync_market_data_job.rb index 4c911593..38b1fe7c 100644 --- a/app/jobs/sync_market_data_job.rb +++ b/app/jobs/sync_market_data_job.rb @@ -2,8 +2,6 @@ class SyncMarketDataJob < ApplicationJob queue_as :scheduled def perform(*args) - syncer = MarketDataSyncer.new - syncer.sync_exchange_rates - syncer.sync_prices + MarketDataSyncer.new.sync_all end end diff --git a/app/models/market_data_syncer.rb b/app/models/market_data_syncer.rb index 785c6b05..d634cd45 100644 --- a/app/models/market_data_syncer.rb +++ b/app/models/market_data_syncer.rb @@ -20,6 +20,11 @@ class MarketDataSyncer @account = account end + def sync_all(full_history: false) + sync_exchange_rates(full_history: full_history) + sync_prices(full_history: full_history) + end + def sync_exchange_rates(full_history: false) unless rate_provider Rails.logger.warn("No rate provider configured for MarketDataSyncer.sync_exchange_rates, skipping sync") -- 2.53.0 From 0079710d47b06c231872ffe27b839afb2905f7e8 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 20:17:59 -0400 Subject: [PATCH 12/27] Report high duplicate sync counts to Sentry --- app/models/sync.rb | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/sync.rb b/app/models/sync.rb index 325b5730..0ccd3533 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -1,6 +1,8 @@ class Sync < ApplicationRecord include AASM + Error = Class.new(StandardError) + belongs_to :syncable, polymorphic: true belongs_to :parent, class_name: "Sync", optional: true @@ -18,7 +20,7 @@ class Sync < ApplicationRecord state :completed state :failed - event :start do + event :start, after_commit: :report_warnings do transitions from: :pending, to: :syncing end @@ -90,6 +92,17 @@ class Sync < ApplicationRecord end end + def report_warnings + todays_sync_count = syncable.syncs.where(created_at: Date.current.all_day).count + + if todays_sync_count > 10 + Sentry.capture_exception( + Error.new("#{syncable_type} (#{syncable.id}) has exceeded 10 syncs today (count: #{todays_sync_count})"), + level: :warning + ) + end + 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") -- 2.53.0 From 9d8d6bd86b0fad50755fadfc98f546096e5ebc8c Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 13 May 2025 15:49:02 -0400 Subject: [PATCH 13/27] Add sync states throughout app --- app/assets/tailwind/maybe-design-system.css | 11 ++- .../maybe-design-system/background-utils.css | 4 + .../maybe-design-system/border-utils.css | 5 ++ app/controllers/accounts_controller.rb | 8 -- app/controllers/concerns/auto_sync.rb | 8 +- app/controllers/concerns/notifiable.rb | 2 - app/models/account.rb | 4 + app/models/balance_sheet.rb | 20 ++++- app/models/family.rb | 14 ++- app/models/plaid_item.rb | 8 ++ app/models/subscription.rb | 1 + app/models/sync.rb | 22 +++-- app/views/accounts/_account.html.erb | 10 ++- .../accounts/_account_sidebar_tabs.html.erb | 3 - .../accounts/_accountable_group.html.erb | 45 +++++++--- app/views/accounts/_chart_loader.html.erb | 8 +- app/views/accounts/chart.html.erb | 28 +++--- app/views/accounts/index.html.erb | 11 --- app/views/accounts/index/_account_groups.erb | 7 +- app/views/accounts/show/_chart.html.erb | 13 ++- app/views/holdings/_cash.html.erb | 17 +++- app/views/holdings/_holding.html.erb | 42 ++++++--- app/views/layouts/shared/_htmldoc.html.erb | 4 - app/views/pages/dashboard.html.erb | 9 +- .../pages/dashboard/_balance_sheet.html.erb | 86 +++++++++++++------ .../pages/dashboard/_net_worth_chart.html.erb | 43 +++++++--- .../shared/notifications/_loading.html.erb | 9 -- app/views/transactions/index.html.erb | 3 - config/initializers/mini_profiler.rb | 1 + config/routes.rb | 4 - db/schema.rb | 2 +- test/controllers/accounts_controller_test.rb | 5 -- .../subscriptions_controller_test.rb | 2 - test/models/subscription_test.rb | 2 +- 34 files changed, 295 insertions(+), 166 deletions(-) delete mode 100644 app/views/shared/notifications/_loading.html.erb diff --git a/app/assets/tailwind/maybe-design-system.css b/app/assets/tailwind/maybe-design-system.css index 8bf9c6c8..bfca010d 100644 --- a/app/assets/tailwind/maybe-design-system.css +++ b/app/assets/tailwind/maybe-design-system.css @@ -241,6 +241,15 @@ stroke-dashoffset: 0; } } + + @keyframes shiny-wave { + 0% { + background-position: -200% 0; + } + 100% { + background-position: 200% 0; + } + } } /* Specific override for strong tags in prose under dark mode */ @@ -429,5 +438,3 @@ } } - - diff --git a/app/assets/tailwind/maybe-design-system/background-utils.css b/app/assets/tailwind/maybe-design-system/background-utils.css index 1c7bc56a..fad493b0 100644 --- a/app/assets/tailwind/maybe-design-system/background-utils.css +++ b/app/assets/tailwind/maybe-design-system/background-utils.css @@ -84,4 +84,8 @@ @variant theme-dark { background-color: var(--color-alpha-black-900); } +} + +@utility bg-loader { + @apply bg-surface-inset animate-pulse; } \ No newline at end of file diff --git a/app/assets/tailwind/maybe-design-system/border-utils.css b/app/assets/tailwind/maybe-design-system/border-utils.css index 94c54a55..bc2d7a60 100644 --- a/app/assets/tailwind/maybe-design-system/border-utils.css +++ b/app/assets/tailwind/maybe-design-system/border-utils.css @@ -1,6 +1,7 @@ /* Custom shadow borders used for surfaces / containers */ @utility shadow-border-xs { box-shadow: var(--shadow-xs), 0px 0px 0px 1px var(--color-alpha-black-50); + transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-xs), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -9,6 +10,7 @@ @utility shadow-border-sm { box-shadow: var(--shadow-sm), 0px 0px 0px 1px var(--color-alpha-black-50); + transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-sm), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -17,6 +19,7 @@ @utility shadow-border-md { box-shadow: var(--shadow-md), 0px 0px 0px 1px var(--color-alpha-black-50); + transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-md), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -25,6 +28,7 @@ @utility shadow-border-lg { box-shadow: var(--shadow-lg), 0px 0px 0px 1px var(--color-alpha-black-50); + transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-lg), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -33,6 +37,7 @@ @utility shadow-border-xl { box-shadow: var(--shadow-xl), 0px 0px 0px 1px var(--color-alpha-black-50); + transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-xl), 0px 0px 0px 1px var(--color-alpha-white-50); diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index f003ab31..904be2b5 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -26,14 +26,6 @@ class AccountsController < ApplicationController render layout: false end - def sync_all - unless family.syncing? - family.sync_later - end - - redirect_back_or_to accounts_path - end - private def family Current.family diff --git a/app/controllers/concerns/auto_sync.rb b/app/controllers/concerns/auto_sync.rb index 4e375359..a353a7be 100644 --- a/app/controllers/concerns/auto_sync.rb +++ b/app/controllers/concerns/auto_sync.rb @@ -14,6 +14,12 @@ module AutoSync return false unless Current.family.present? return false unless Current.family.accounts.active.any? - (Current.family.last_synced_at&.to_date || 1.day.ago) < Date.current + should_sync = (Current.family.last_synced_at&.to_date || 1.day.ago) < Date.current + + if should_sync + Rails.logger.info "Auto-syncing family #{Current.family.id}, last sync was #{Current.family.last_synced_at}" + end + + should_sync end end diff --git a/app/controllers/concerns/notifiable.rb b/app/controllers/concerns/notifiable.rb index 0d8ea384..b1689f67 100644 --- a/app/controllers/concerns/notifiable.rb +++ b/app/controllers/concerns/notifiable.rb @@ -46,8 +46,6 @@ module Notifiable [ { partial: "shared/notifications/alert", locals: { message: data } } ] when "cta" [ resolve_cta(data) ] - when "loading" - [ { partial: "shared/notifications/loading", locals: { message: data } } ] when "notice" messages = Array(data) messages.map { |message| { partial: "shared/notifications/notice", locals: { message: message } } } diff --git a/app/models/account.rb b/app/models/account.rb index ee2fc963..5e99315c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -61,6 +61,10 @@ class Account < ApplicationRecord end end + def syncing? + true + end + def institution_domain url_string = plaid_account&.plaid_item&.institution_url return nil unless url_string.present? diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index c289f86f..54ed199c 100644 --- a/app/models/balance_sheet.rb +++ b/app/models/balance_sheet.rb @@ -22,20 +22,25 @@ class BalanceSheet end def classification_groups + 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: account_groups("asset") + 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: account_groups("liability") + account_groups: liability_groups, + syncing?: liability_groups.any?(&:syncing?) ) ] end @@ -57,6 +62,7 @@ class BalanceSheet 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.define_singleton_method(:weight) do classification_total.zero? ? 0 : account.converted_balance / classification_total.to_d * 100 @@ -76,9 +82,13 @@ class BalanceSheet family.currency end + def syncing? + classification_groups.any? { |group| group.syncing? } + end + private - ClassificationGroup = Struct.new(:key, :display_name, :icon, :total_money, :account_groups, keyword_init: true) - AccountGroup = Struct.new(:key, :name, :accountable_type, :classification, :total, :total_money, :weight, :accounts, :color, :missing_rates?, keyword_init: true) + ClassificationGroup = Struct.new(:key, :display_name, :icon, :total_money, :account_groups, :syncing?, keyword_init: true) + AccountGroup = Struct.new(: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 @@ -87,9 +97,11 @@ class BalanceSheet 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("LEFT JOIN syncs ON syncs.syncable_id = accounts.id AND syncs.syncable_type = 'Account' AND (syncs.status = 'pending' OR syncs.status = 'syncing')") .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) diff --git a/app/models/family.rb b/app/models/family.rb index 0644776b..3383b0d4 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -39,15 +39,13 @@ class Family < ApplicationRecord broadcast_remove target: "syncing-notice" end - # If family has any syncs pending/syncing within the last 10 minutes, we show a persistent "syncing" notice. - # Ignore syncs older than 10 minutes as they are considered "stale" + # If any accounts or plaid items are syncing, the family is also syncing, even if a formal "Family Sync" is not running. def syncing? - Sync.where( - "(syncable_type = 'Family' AND syncable_id = ?) OR - (syncable_type = 'Account' AND syncable_id IN (SELECT id FROM accounts WHERE family_id = ? AND plaid_account_id IS NULL)) OR - (syncable_type = 'PlaidItem' AND syncable_id IN (SELECT id FROM plaid_items WHERE family_id = ?))", - id, id, id - ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? + Sync.joins("LEFT JOIN plaid_items ON plaid_items.id = syncs.syncable_id AND syncs.syncable_type = 'PlaidItem'") + .joins("LEFT JOIN accounts ON accounts.id = syncs.syncable_id AND syncs.syncable_type = 'Account'") + .where("syncs.syncable_id = ? OR accounts.family_id = ? OR plaid_items.family_id = ?", id, id, id) + .incomplete + .exists? end def assigned_merchants diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index c2708f49..4aae91ca 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -52,6 +52,14 @@ 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) + .incomplete + .exists? + end + def auto_match_categories! if family.categories.none? family.categories.bootstrap! diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 5f96361e..90b5303d 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -17,6 +17,7 @@ class Subscription < ApplicationRecord validates :stripe_id, presence: true, if: :active? validates :trial_ends_at, presence: true, if: :trialing? + validates :family_id, uniqueness: true class << self def new_trial_ends_at diff --git a/app/models/sync.rb b/app/models/sync.rb index 0ccd3533..65e47749 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -20,6 +20,8 @@ class Sync < ApplicationRecord state :completed state :failed + after_all_transitions :log_status_change + event :start, after_commit: :report_warnings do transitions from: :pending, to: :syncing end @@ -41,14 +43,16 @@ class Sync < ApplicationRecord end def perform - start! + Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do + start! - begin - syncable.perform_sync(self) - attempt_finalization - rescue => e - fail! - handle_error(e) + begin + syncable.perform_sync(self) + attempt_finalization + rescue => e + fail! + handle_error(e) + end end end @@ -68,6 +72,10 @@ class Sync < ApplicationRecord end private + def log_status_change + Rails.logger.info("changing from #{aasm.from_state} to #{aasm.to_state} (event: #{aasm.current_event})") + end + def has_failed_children? children.failed.any? end diff --git a/app/views/accounts/_account.html.erb b/app/views/accounts/_account.html.erb index 07ffd3d5..475f953e 100644 --- a/app/views/accounts/_account.html.erb +++ b/app/views/accounts/_account.html.erb @@ -30,9 +30,13 @@ <% end %>
-

"> - <%= format_money account.balance_money %> -

+ <% if account.syncing? %> +
+ <% else %> +

"> + <%= format_money account.balance_money %> +

+ <% end %> <% unless account.scheduled_for_deletion? %> <%= styled_form_with model: account, data: { turbo_frame: "_top", controller: "auto-submit-form" } do |f| %> diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index 9ca2781c..d8841a44 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -40,7 +40,6 @@ full_width: true, class: "justify-start" ) %> -
<% family.balance_sheet.account_groups("asset").each do |group| %> <%= render "accounts/accountable_group", account_group: group %> @@ -60,7 +59,6 @@ full_width: true, class: "justify-start" ) %> -
<% family.balance_sheet.account_groups("liability").each do |group| %> <%= render "accounts/accountable_group", account_group: group %> @@ -80,7 +78,6 @@ frame: :modal, class: "justify-start" ) %> -
<% family.balance_sheet.account_groups.each do |group| %> <%= render "accounts/accountable_group", account_group: group %> diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index ab10dec7..10ce632c 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -3,12 +3,20 @@ <%= render DisclosureComponent.new(title: account_group.name, align: :left, open: account_group.accounts.any? { |account| page_active?(account_path(account)) }) do |disclosure| %> <% disclosure.with_summary_content do %>
- <%= 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" do %> -
-
+ <% if account_group.syncing? %> +
+
+
+
+
+ <% 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" do %> +
+
+
+ <% end %> <% end %>
<% end %> @@ -29,15 +37,26 @@ <%= tag.p account.short_subtype_label, class: "text-sm text-secondary truncate" %>
-
- <%= 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" do %> -
-
+ <% if account_group.syncing? %> +
+
+
+
+
+
- <% end %> -
+
+ <% 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" do %> +
+
+
+ <% end %> +
+ <% end %> <% end %> <% end %>
diff --git a/app/views/accounts/_chart_loader.html.erb b/app/views/accounts/_chart_loader.html.erb index f6a9c852..a5e72097 100644 --- a/app/views/accounts/_chart_loader.html.erb +++ b/app/views/accounts/_chart_loader.html.erb @@ -1,5 +1,7 @@ -
+
+
-
-

Loading...

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

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

+
+ <% end %> +
+ <% end %> <% end %> diff --git a/app/views/accounts/index.html.erb b/app/views/accounts/index.html.erb index b4c78332..35e647ea 100644 --- a/app/views/accounts/index.html.erb +++ b/app/views/accounts/index.html.erb @@ -2,17 +2,6 @@

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

- <% if Rails.env.development? %> - <%= render ButtonComponent.new( - text: "Sync all", - href: sync_all_accounts_path, - method: :post, - variant: "outline", - disabled: Current.family.syncing?, - icon: "refresh-cw", - ) %> - <% end %> - <%= render LinkComponent.new( text: "New account", href: new_account_path(return_to: accounts_path), diff --git a/app/views/accounts/index/_account_groups.erb b/app/views/accounts/index/_account_groups.erb index 6347339c..51ba4d8b 100644 --- a/app/views/accounts/index/_account_groups.erb +++ b/app/views/accounts/index/_account_groups.erb @@ -6,9 +6,12 @@

<%= Accountable.from_type(group).display_name %>

·

<%= accounts.count %>

-

<%= totals_by_currency(collection: accounts, money_method: :balance_money) %>

+ + <% unless accounts.any?(&:syncing?) %> +

<%= totals_by_currency(collection: accounts, money_method: :balance_money) %>

+ <% end %>
-
+
<% accounts.each do |account| %> <%= render account %> <% end %> diff --git a/app/views/accounts/show/_chart.html.erb b/app/views/accounts/show/_chart.html.erb index 08e67e84..cc6c4f79 100644 --- a/app/views/accounts/show/_chart.html.erb +++ b/app/views/accounts/show/_chart.html.erb @@ -3,15 +3,22 @@ <% period = @period || Period.last_30_days %> <% default_value_title = account.asset? ? t(".balance") : t(".owed") %> -
+
<%= tag.p title || default_value_title, class: "text-sm font-medium text-secondary" %> - <%= tooltip %> + + <% unless account.syncing? %> + <%= tooltip %> + <% end %>
- <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %> + <% if account.syncing? %> +
+ <% else %> + <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %> + <% end %>
<%= form_with url: request.path, method: :get, data: { controller: "auto-submit-form" } do |form| %> diff --git a/app/views/holdings/_cash.html.erb b/app/views/holdings/_cash.html.erb index a279d9ab..702f40d0 100644 --- a/app/views/holdings/_cash.html.erb +++ b/app/views/holdings/_cash.html.erb @@ -19,8 +19,13 @@
<% cash_weight = account.balance.zero? ? 0 : account.cash_balance / account.balance * 100 %> - <%= render "shared/progress_circle", progress: cash_weight %> - <%= tag.p number_to_percentage(cash_weight, precision: 1) %> + + <% if account.syncing? %> +
+ <% else %> + <%= render "shared/progress_circle", progress: cash_weight %> + <%= tag.p number_to_percentage(cash_weight, precision: 1) %> + <% end %>
@@ -28,7 +33,13 @@
- <%= tag.p format_money account.cash_balance_money %> + <% if account.syncing? %> +
+
+
+ <% else %> + <%= tag.p format_money account.cash_balance_money %> + <% end %>
diff --git a/app/views/holdings/_holding.html.erb b/app/views/holdings/_holding.html.erb index 5fe0e4c9..c8a2ac59 100644 --- a/app/views/holdings/_holding.html.erb +++ b/app/views/holdings/_holding.html.erb @@ -17,7 +17,9 @@
- <% if holding.weight %> + <% if holding.account.syncing? %> +
+ <% elsif holding.weight %> <%= render "shared/progress_circle", progress: holding.weight %> <%= tag.p number_to_percentage(holding.weight, precision: 1) %> <% else %> @@ -26,21 +28,39 @@
- <%= tag.p format_money holding.avg_cost %> - <%= tag.p t(".per_share"), class: "font-normal text-secondary" %> -
- -
- <% if holding.amount_money %> - <%= tag.p format_money holding.amount_money %> + <% if holding.account.syncing? %> +
+
+
<% else %> - <%= tag.p "--", class: "text-secondary" %> + <%= tag.p format_money holding.avg_cost %> + <%= tag.p t(".per_share"), class: "font-normal text-secondary" %> <% end %> - <%= tag.p t(".shares", qty: number_with_precision(holding.qty, precision: 1)), class: "font-normal text-secondary" %>
- <% if holding.trend %> + <% if holding.account.syncing? %> +
+
+
+
+ <% 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" %> + <% end %> +
+ +
+ <% if holding.account.syncing? %> +
+
+
+
+ <% elsif 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/layouts/shared/_htmldoc.html.erb b/app/views/layouts/shared/_htmldoc.html.erb index fadcd396..30887f7b 100644 --- a/app/views/layouts/shared/_htmldoc.html.erb +++ b/app/views/layouts/shared/_htmldoc.html.erb @@ -23,10 +23,6 @@ <%= render_flash_notifications %>
- - <% if Current.family&.syncing? %> - <%= render "shared/notifications/loading", id: "syncing-notice", message: "Syncing accounts data..." %> - <% end %>
diff --git a/app/views/pages/dashboard.html.erb b/app/views/pages/dashboard.html.erb index 569caaa2..79d18d4b 100644 --- a/app/views/pages/dashboard.html.erb +++ b/app/views/pages/dashboard.html.erb @@ -25,11 +25,14 @@
<% if Current.family.accounts.any? %> -
- <%= render partial: "pages/dashboard/net_worth_chart", locals: { series: @balance_sheet.net_worth_series(period: @period), period: @period } %> +
+ <%= render partial: "pages/dashboard/net_worth_chart", locals: { + balance_sheet: @balance_sheet, + period: @period + } %>
<% else %> -
+
<%= render "pages/dashboard/no_accounts_graph_placeholder" %>
<% end %> diff --git a/app/views/pages/dashboard/_balance_sheet.html.erb b/app/views/pages/dashboard/_balance_sheet.html.erb index 302ae103..196d96ae 100644 --- a/app/views/pages/dashboard/_balance_sheet.html.erb +++ b/app/views/pages/dashboard/_balance_sheet.html.erb @@ -1,6 +1,6 @@ <%# locals: (balance_sheet:) %> -
+
<% balance_sheet.classification_groups.each do |classification_group| %>

@@ -11,26 +11,38 @@ <% if classification_group.account_groups.any? %> · - <%= classification_group.total_money.format(precision: 0) %> + <% if classification_group.syncing? %> +
+
+
+ <% else %> + <%= classification_group.total_money.format(precision: 0) %> + <% end %> <% end %>

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

<%= account_group.name %>

-

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

-
- <% 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%>
@@ -56,15 +68,27 @@

<%= account_group.name %>

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

<%= format_money(account_group.total_money) %>

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

<%= format_money(account_group.total_money) %>

+
+
+ <% end %>
@@ -76,15 +100,27 @@ <%= link_to account.name, account_path(account) %>
-
-
- <%= render "pages/dashboard/group_weight", weight: account.weight, color: account_group.color %> -
+ <% if account.syncing? %> +
+
+
+
-
-

<%= format_money(account.balance_money) %>

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

<%= format_money(account.balance_money) %>

+
+
+ <% end %>
<% 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 a8870cac..4d1e443a 100644 --- a/app/views/pages/dashboard/_net_worth_chart.html.erb +++ b/app/views/pages/dashboard/_net_worth_chart.html.erb @@ -1,16 +1,27 @@ -<%# locals: (series:, period:) %> +<%# locals: (balance_sheet:, period:) %> + +<% series = balance_sheet.net_worth_series(period: period) %>

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

-

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

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

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

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

+ <%= series.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 %> <% end %>
@@ -24,14 +35,20 @@ <% end %>
-<% if series.any? %> -
+
+
+
+<% else %> + <% if series.any? %> +
-<% else %> -
-

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

-
+ <% else %> +
+

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

+
+ <% end %> <% end %> diff --git a/app/views/shared/notifications/_loading.html.erb b/app/views/shared/notifications/_loading.html.erb deleted file mode 100644 index 18eabd89..00000000 --- a/app/views/shared/notifications/_loading.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -<%# locals: (message:, id: nil) %> - -<%= tag.div id: id, class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-xs" do %> -
- <%= icon "loader", class: "animate-pulse" %> -
- - <%= tag.p message, class: "text-primary text-sm font-medium" %> -<% end %> diff --git a/app/views/transactions/index.html.erb b/app/views/transactions/index.html.erb index 9781672f..52bb9c3d 100644 --- a/app/views/transactions/index.html.erb +++ b/app/views/transactions/index.html.erb @@ -4,9 +4,6 @@
<%= render MenuComponent.new do |menu| %> - <% if Rails.env.development? %> - <% menu.with_item(variant: "button", text: "Dev only: Sync all", href: sync_all_accounts_path, method: :post, icon: "refresh-cw") %> - <% end %> <% menu.with_item(variant: "link", text: "New rule", href: new_rule_path(resource_type: "transaction"), icon: "plus", data: { turbo_frame: :modal }) %> <% menu.with_item(variant: "link", text: "Edit rules", href: rules_path, icon: "git-branch", data: { turbo_frame: :_top }) %> <% menu.with_item(variant: "link", text: "Edit categories", href: categories_path, icon: "shapes", data: { turbo_frame: :_top }) %> diff --git a/config/initializers/mini_profiler.rb b/config/initializers/mini_profiler.rb index d304a50e..69391c80 100644 --- a/config/initializers/mini_profiler.rb +++ b/config/initializers/mini_profiler.rb @@ -1,3 +1,4 @@ Rails.application.configure do Rack::MiniProfiler.config.skip_paths = [ "/design-system" ] + Rack::MiniProfiler.config.max_traces_to_show = 30 end diff --git a/config/routes.rb b/config/routes.rb index e44265f6..132fa06a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -105,10 +105,6 @@ Rails.application.routes.draw do end resources :accounts, only: %i[index new], shallow: true do - collection do - post :sync_all - end - member do post :sync get :chart diff --git a/db/schema.rb b/db/schema.rb index 1f14a5c9..c2fa7e20 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_05_12_171654) do +ActiveRecord::Schema[7.2].define(version: 2025_05_13_122703) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index d85a5ffa..a3d827e8 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -15,9 +15,4 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest post sync_account_path(@account) assert_redirected_to account_path(@account) end - - test "can sync all accounts" do - post sync_all_accounts_path - assert_redirected_to accounts_path - end end diff --git a/test/controllers/subscriptions_controller_test.rb b/test/controllers/subscriptions_controller_test.rb index 1e791632..0b406dca 100644 --- a/test/controllers/subscriptions_controller_test.rb +++ b/test/controllers/subscriptions_controller_test.rb @@ -32,8 +32,6 @@ class SubscriptionsControllerTest < ActionDispatch::IntegrationTest end test "users who have already trialed cannot create a new subscription" do - @family.start_trial_subscription! - assert_no_difference "Subscription.count" do post subscription_path end diff --git a/test/models/subscription_test.rb b/test/models/subscription_test.rb index 3986335c..389aaaf0 100644 --- a/test/models/subscription_test.rb +++ b/test/models/subscription_test.rb @@ -2,7 +2,7 @@ require "test_helper" class SubscriptionTest < ActiveSupport::TestCase setup do - @family = families(:empty) + @family = Family.create!(name: "Test Family") end test "can create subscription without stripe details if trial" do -- 2.53.0 From 86cbb14d3b253fd7390e16c9aabef35968425108 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 13 May 2025 21:32:49 -0400 Subject: [PATCH 14/27] account tab session --- app/controllers/application_controller.rb | 5 +- app/controllers/concerns/account_groupable.rb | 44 +++++++ .../controllers/sidebar_tabs_controller.js | 16 --- app/models/account.rb | 4 - app/models/account/syncer.rb | 3 +- app/models/concerns/accountable.rb | 9 -- app/models/family.rb | 4 - app/models/sync.rb | 47 ++++--- .../accounts/_account_sidebar_tabs.html.erb | 123 +++++++++--------- .../accounts/_accountable_group.html.erb | 3 +- app/views/accounts/_chart_loader.html.erb | 2 +- app/views/layouts/application.html.erb | 2 +- config/initializers/mini_profiler.rb | 4 +- test/models/sync_test.rb | 86 +++++++++++- 14 files changed, 229 insertions(+), 123 deletions(-) create mode 100644 app/controllers/concerns/account_groupable.rb delete mode 100644 app/javascript/controllers/sidebar_tabs_controller.js diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a54dc088..8a1642f9 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,5 +1,8 @@ class ApplicationController < ActionController::Base - include Onboardable, Localize, AutoSync, Authentication, Invitable, SelfHostable, StoreLocation, Impersonatable, Breadcrumbable, FeatureGuardable, Notifiable + include Onboardable, Localize, AutoSync, Authentication, Invitable, + SelfHostable, StoreLocation, Impersonatable, Breadcrumbable, + FeatureGuardable, Notifiable, AccountGroupable + include Pagy::Backend before_action :detect_os diff --git a/app/controllers/concerns/account_groupable.rb b/app/controllers/concerns/account_groupable.rb new file mode 100644 index 00000000..cf7e9e1b --- /dev/null +++ b/app/controllers/concerns/account_groupable.rb @@ -0,0 +1,44 @@ +module AccountGroupable + extend ActiveSupport::Concern + + included do + before_action :set_account_group_tab + end + + def set_account_group_tab + last_selected_tab = session[:account_group_tab] || "asset" + + selected_tab = if account_group_tab_param + account_group_tab_param + elsif on_asset_page? + "asset" + elsif on_liability_page? + "liability" + else + last_selected_tab + end + + session[:account_group_tab] = selected_tab + @account_group_tab = selected_tab + end + + private + def account_group_tab_param + valid_tabs = %w[asset liability all] + params[:account_group_tab].in?(valid_tabs) ? params[:account_group_tab] : nil + end + + def on_asset_page? + accountable_controller_names_for("asset").include?(controller_name) + end + + def on_liability_page? + accountable_controller_names_for("liability").include?(controller_name) + end + + def accountable_controller_names_for(classification) + Accountable::TYPES.map(&:constantize) + .select { |a| a.classification == classification } + .map { |a| a.model_name.plural } + end +end diff --git a/app/javascript/controllers/sidebar_tabs_controller.js b/app/javascript/controllers/sidebar_tabs_controller.js deleted file mode 100644 index f88a70f7..00000000 --- a/app/javascript/controllers/sidebar_tabs_controller.js +++ /dev/null @@ -1,16 +0,0 @@ -import { Controller } from "@hotwired/stimulus"; - -// Connects to data-controller="sidebar-tabs" -export default class extends Controller { - static targets = ["account"]; - - select(event) { - this.accountTargets.forEach((account) => { - if (account.contains(event.target)) { - account.classList.add("bg-container"); - } else { - account.classList.remove("bg-container"); - } - }); - } -} diff --git a/app/models/account.rb b/app/models/account.rb index 5e99315c..ee2fc963 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -61,10 +61,6 @@ class Account < ApplicationRecord end end - def syncing? - true - end - def institution_domain url_string = plaid_account&.plaid_item&.institution_url return nil unless url_string.present? diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index b89bb9ca..4328e1d2 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -11,9 +11,8 @@ class Account::Syncer end def perform_post_sync - account.family.remove_syncing_notice! - account.accountable.post_sync account.family.auto_match_transfers! + account.family.broadcast_refresh end private diff --git a/app/models/concerns/accountable.rb b/app/models/concerns/accountable.rb index af9b04ab..12ee9888 100644 --- a/app/models/concerns/accountable.rb +++ b/app/models/concerns/accountable.rb @@ -68,15 +68,6 @@ module Accountable end end - def post_sync - broadcast_replace_to( - account, - target: "chart_account_#{account.id}", - partial: "accounts/show/chart", - locals: { account: account } - ) - end - def display_name self.class.display_name end diff --git a/app/models/family.rb b/app/models/family.rb index 3383b0d4..0cfb8b89 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -35,10 +35,6 @@ class Family < ApplicationRecord validates :locale, inclusion: { in: I18n.available_locales.map(&:to_s) } validates :date_format, inclusion: { in: DATE_FORMATS.map(&:last) } - def remove_syncing_notice! - broadcast_remove target: "syncing-notice" - end - # If any accounts or plaid items are syncing, the family is also syncing, even if a formal "Family Sync" is not running. def syncing? Sync.joins("LEFT JOIN plaid_items ON plaid_items.id = syncs.syncable_id AND syncs.syncable_type = 'PlaidItem'") diff --git a/app/models/sync.rb b/app/models/sync.rb index 65e47749..d004ec31 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -26,11 +26,11 @@ class Sync < ApplicationRecord transitions from: :pending, to: :syncing end - event :complete, after_commit: :handle_finalization do + event :complete do transitions from: :syncing, to: :completed end - event :fail, after_commit: :handle_finalization do + event :fail do transitions from: :syncing, to: :failed end end @@ -46,29 +46,40 @@ class Sync < ApplicationRecord Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do start! + sleep 10 + begin syncable.perform_sync(self) - attempt_finalization rescue => e - fail! - handle_error(e) + fail_and_report_error(e) + ensure + finalize_if_all_children_finalized end end end - # If the sync doesn't have any in-progress children, finalize it. - def attempt_finalization + # Finalizes the current sync AND parent (if it exists) + def finalize_if_all_children_finalized Sync.transaction do lock! + # If this is the "parent" and there are still children running, don't finalize. return unless all_children_finalized? - if has_failed_children? - fail! - else - complete! + # If we make it here, the sync is finalized. Run post-sync, regardless of failure/success. + perform_post_sync + + if syncing? + if has_failed_children? + fail! + else + complete! + end end end + + # If this sync has a parent, try to finalize it so the child status propagates up the chain. + parent&.finalize_if_all_children_finalized end private @@ -84,17 +95,15 @@ class Sync < ApplicationRecord children.incomplete.empty? end - # Once sync finalizes, notify its parent and run its post-sync logic. - def handle_finalization + def perform_post_sync syncable.perform_post_sync - - if parent - parent.attempt_finalization - end + rescue => e + fail_and_report_error(e) end - def handle_error(error) - update!(error: error.message) + def fail_and_report_error(error) + fail! + update(error: error.message) Sentry.capture_exception(error) do |scope| scope.set_tags(sync_id: id) end diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index d8841a44..06f70c2b 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -1,6 +1,6 @@ -<%# locals: (family:, active_account_group_tab:) %> +<%# locals: (family:, active_account_group_tab: "assets") %> -
+
<% if family.missing_data_provider? %>
@@ -21,70 +21,71 @@
<% end %> -
- <%= render TabsComponent.new(active_tab: active_account_group_tab, url_param_key: "account_group_tab", testid: "account-sidebar-tabs") do |tabs| %> - <% tabs.with_nav do |nav| %> - <% nav.with_btn(id: "assets", label: "Assets") %> - <% nav.with_btn(id: "debts", label: "Debts") %> - <% nav.with_btn(id: "all", label: "All") %> - <% end %> + <%= render TabsComponent.new(active_tab: active_account_group_tab, testid: "account-sidebar-tabs") do |tabs| %> + <% tabs.with_nav do |nav| %> + <% nav.with_btn(id: "asset", label: "Assets") %> + <% nav.with_btn(id: "liability", label: "Debts") %> + <% nav.with_btn(id: "all", label: "All") %> + <% end %> - <% tabs.with_panel(tab_id: "assets") do %> -
- <%= render LinkComponent.new( - text: "New asset", - variant: "ghost", - href: new_account_path(step: "method_select", classification: "asset"), - icon: "plus", - frame: :modal, - full_width: true, - class: "justify-start" - ) %> -
- <% family.balance_sheet.account_groups("asset").each do |group| %> - <%= render "accounts/accountable_group", account_group: group %> - <% end %> -
-
- <% end %> - - <% tabs.with_panel(tab_id: "debts") do %> -
- <%= render LinkComponent.new( - text: "New debt", + <% tabs.with_panel(tab_id: "asset") do %> +
+ <%= render LinkComponent.new( + text: "New asset", variant: "ghost", - href: new_account_path(step: "method_select", classification: "liability"), + href: new_account_path(step: "method_select", classification: "asset"), icon: "plus", frame: :modal, - full_width: true, - class: "justify-start" - ) %> -
- <% family.balance_sheet.account_groups("liability").each do |group| %> - <%= render "accounts/accountable_group", account_group: group %> - <% end %> -
-
- <% end %> - - <% tabs.with_panel(tab_id: "all") do %> -
- <%= render LinkComponent.new( - text: "New account", - variant: "ghost", full_width: true, - href: new_account_path(step: "method_select"), - icon: "plus", - frame: :modal, - class: "justify-start" - ) %> -
- <% family.balance_sheet.account_groups.each do |group| %> - <%= render "accounts/accountable_group", account_group: group %> - <% end %> -
+ class: "justify-start" + ) %> + +
+ <% family.balance_sheet.account_groups("asset").each do |group| %> + <%= render "accounts/accountable_group", account_group: group %> + <% end %>
- <% end %> +
<% end %> -
+ + <% tabs.with_panel(tab_id: "liability") do %> +
+ <%= render LinkComponent.new( + text: "New debt", + variant: "ghost", + href: new_account_path(step: "method_select", classification: "liability"), + icon: "plus", + frame: :modal, + full_width: true, + class: "justify-start" + ) %> + +
+ <% family.balance_sheet.account_groups("liability").each do |group| %> + <%= render "accounts/accountable_group", account_group: group %> + <% end %> +
+
+ <% end %> + + <% tabs.with_panel(tab_id: "all") do %> +
+ <%= render LinkComponent.new( + text: "New account", + variant: "ghost", + full_width: true, + href: new_account_path(step: "method_select"), + icon: "plus", + frame: :modal, + class: "justify-start" + ) %> + +
+ <% family.balance_sheet.account_groups.each do |group| %> + <%= render "accounts/accountable_group", account_group: group %> + <% end %> +
+
+ <% end %> + <% end %>
diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index 10ce632c..33fc42e8 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -28,7 +28,6 @@ "block flex items-center gap-2 px-3 py-2 rounded-lg", page_active?(account_path(account)) ? "bg-container" : "hover:bg-surface-hover" ), - data: { sidebar_tabs_target: "account", action: "click->sidebar-tabs#select" }, title: account.name do %> <%= render "accounts/logo", account: account, size: "sm", color: account_group.color %> @@ -37,7 +36,7 @@ <%= tag.p account.short_subtype_label, class: "text-sm text-secondary truncate" %>
- <% if account_group.syncing? %> + <% if account.syncing? %>
diff --git a/app/views/accounts/_chart_loader.html.erb b/app/views/accounts/_chart_loader.html.erb index a5e72097..b080329f 100644 --- a/app/views/accounts/_chart_loader.html.erb +++ b/app/views/accounts/_chart_loader.html.erb @@ -2,6 +2,6 @@
-
+
diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 53e7f666..361aabfd 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -80,7 +80,7 @@ <%= yield :sidebar %> <% else %>
-
+
<%= render "accounts/account_sidebar_tabs", family: Current.family, active_account_group_tab: params[:account_group_tab] || "assets" %>
diff --git a/config/initializers/mini_profiler.rb b/config/initializers/mini_profiler.rb index 69391c80..6a79f19e 100644 --- a/config/initializers/mini_profiler.rb +++ b/config/initializers/mini_profiler.rb @@ -1,4 +1,4 @@ Rails.application.configure do - Rack::MiniProfiler.config.skip_paths = [ "/design-system" ] - Rack::MiniProfiler.config.max_traces_to_show = 30 + Rack::MiniProfiler.config.skip_paths = [ "/design-system", "/assets", "/cable", "/manifest", "/favicon.ico", "/hotwire-livereload", "/logo-pwa.png" ] + Rack::MiniProfiler.config.max_traces_to_show = 50 end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index bd5de249..f9ca4144 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -68,8 +68,92 @@ class SyncTest < ActiveSupport::TestCase account_sync.perform - assert_equal "completed", family_sync.reload.status assert_equal "completed", plaid_item_sync.reload.status assert_equal "completed", account_sync.reload.status + assert_equal "completed", family_sync.reload.status + end + + test "failures propagate up the chain" do + family = families(:dylan_family) + plaid_item = plaid_items(:one) + account = accounts(:connected) + + family_sync = Sync.create!(syncable: family) + plaid_item_sync = Sync.create!(syncable: plaid_item, parent: family_sync) + account_sync = Sync.create!(syncable: account, parent: plaid_item_sync) + + assert_equal "pending", family_sync.status + assert_equal "pending", plaid_item_sync.status + assert_equal "pending", account_sync.status + + family.expects(:perform_sync).with(family_sync).once + + family_sync.perform + + assert_equal "syncing", family_sync.reload.status + + plaid_item.expects(:perform_sync).with(plaid_item_sync).once + + plaid_item_sync.perform + + assert_equal "syncing", family_sync.reload.status + assert_equal "syncing", plaid_item_sync.reload.status + + # This error should "bubble up" to the PlaidItem and Family sync results + account.expects(:perform_sync).with(account_sync).raises(StandardError.new("test account sync error")) + + # Since these are accessed through `parent`, they won't necessarily be the same + # instance we configured above + Account.any_instance.expects(:perform_post_sync).once + PlaidItem.any_instance.expects(:perform_post_sync).once + Family.any_instance.expects(:perform_post_sync).once + + account_sync.perform + + assert_equal "failed", plaid_item_sync.reload.status + assert_equal "failed", account_sync.reload.status + assert_equal "failed", family_sync.reload.status + end + + test "parent failure should not change status if child succeeds" do + family = families(:dylan_family) + plaid_item = plaid_items(:one) + account = accounts(:connected) + + family_sync = Sync.create!(syncable: family) + plaid_item_sync = Sync.create!(syncable: plaid_item, parent: family_sync) + account_sync = Sync.create!(syncable: account, parent: plaid_item_sync) + + assert_equal "pending", family_sync.status + assert_equal "pending", plaid_item_sync.status + assert_equal "pending", account_sync.status + + family.expects(:perform_sync).with(family_sync).raises(StandardError.new("test family sync error")) + + family_sync.perform + + assert_equal "failed", family_sync.reload.status + + plaid_item.expects(:perform_sync).with(plaid_item_sync).raises(StandardError.new("test plaid item sync error")) + + plaid_item_sync.perform + + assert_equal "failed", family_sync.reload.status + assert_equal "failed", plaid_item_sync.reload.status + + # Leaf level sync succeeds, but shouldn't change the status of the already-failed parent syncs + account.expects(:perform_sync).with(account_sync).once + + # Since these are accessed through `parent`, they won't necessarily be the same + # instance we configured above + Account.any_instance.expects(:perform_post_sync).once + PlaidItem.any_instance.expects(:perform_post_sync).once + Family.any_instance.expects(:perform_post_sync).once + + account_sync.perform + + assert_equal "failed", plaid_item_sync.reload.status + assert_equal "failed", family_sync.reload.status + assert_equal "completed", account_sync.reload.status end end -- 2.53.0 From c83aa10f01aa0956bde338477989eacfc07f2269 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 11:42:17 -0400 Subject: [PATCH 15/27] Persistent account tab selections --- app/components/tabs_component.html.erb | 6 +++ app/components/tabs_component.rb | 5 +- app/components/tabs_controller.js | 11 +++-- app/controllers/concerns/account_groupable.rb | 46 ++++++------------- app/controllers/cookie_sessions_controller.rb | 22 +++++++++ app/controllers/sessions_controller.rb | 4 ++ app/controllers/users_controller.rb | 2 +- app/models/balance_sheet.rb | 13 ++++-- .../accounts/_account_sidebar_tabs.html.erb | 2 +- app/views/accounts/chart.html.erb | 2 +- app/views/accounts/show/_chart.html.erb | 2 +- app/views/layouts/application.html.erb | 4 +- app/views/pages/dashboard.html.erb | 6 +-- .../pages/dashboard/_balance_sheet.html.erb | 2 +- app/views/rules/_rule.html.erb | 4 +- app/views/rules/confirm.html.erb | 4 +- app/views/rules/edit.html.erb | 2 +- app/views/rules/index.html.erb | 2 +- config/routes.rb | 2 + db/schema.rb | 3 +- 20 files changed, 86 insertions(+), 58 deletions(-) create mode 100644 app/controllers/cookie_sessions_controller.rb diff --git a/app/components/tabs_component.html.erb b/app/components/tabs_component.html.erb index 4ec901fa..9a1d938a 100644 --- a/app/components/tabs_component.html.erb +++ b/app/components/tabs_component.html.erb @@ -1,6 +1,7 @@ <%= tag.div data: { controller: "tabs", testid: testid, + tabs_session_key_value: session_key, tabs_url_param_key_value: url_param_key, tabs_nav_btn_active_class: active_btn_classes, tabs_nav_btn_inactive_class: inactive_btn_classes @@ -10,6 +11,11 @@ <% else %> <%= nav %> + <%= form_with url: cookie_session_path, scope: :cookie_session, method: :put, data: { tabs_target: "persistSelectionForm" } do |f| %> + <%= f.hidden_field :tab_key, value: "account_group_tab" %> + <%= f.hidden_field :tab_value, value: "all", data: { tabs_target: "selectionInput" } %> + <% end %> + <% panels.each do |panel| %> <%= panel %> <% end %> diff --git a/app/components/tabs_component.rb b/app/components/tabs_component.rb index 4017b308..747a9420 100644 --- a/app/components/tabs_component.rb +++ b/app/components/tabs_component.rb @@ -27,11 +27,12 @@ class TabsComponent < ViewComponent::Base } } - attr_reader :active_tab, :url_param_key, :variant, :testid + attr_reader :active_tab, :url_param_key, :session_key, :variant, :testid - def initialize(active_tab:, url_param_key: nil, variant: :default, active_btn_classes: "", inactive_btn_classes: "", testid: nil) + def initialize(active_tab:, url_param_key: nil, session_key: nil, variant: :default, active_btn_classes: "", inactive_btn_classes: "", testid: nil) @active_tab = active_tab @url_param_key = url_param_key + @session_key = session_key @variant = variant.to_sym @active_btn_classes = active_btn_classes @inactive_btn_classes = inactive_btn_classes diff --git a/app/components/tabs_controller.js b/app/components/tabs_controller.js index 43a4b192..5b83b0ad 100644 --- a/app/components/tabs_controller.js +++ b/app/components/tabs_controller.js @@ -3,8 +3,8 @@ import { Controller } from "@hotwired/stimulus"; // Connects to data-controller="tabs--components" export default class extends Controller { static classes = ["navBtnActive", "navBtnInactive"]; - static targets = ["panel", "navBtn"]; - static values = { urlParamKey: String }; + static targets = ["panel", "navBtn", "persistSelectionForm", "selectionInput"]; + static values = { sessionKey: String, urlParamKey: String }; show(e) { const btn = e.target.closest("button"); @@ -28,11 +28,16 @@ export default class extends Controller { } }); - // Update URL with the selected tab if (this.urlParamKeyValue) { const url = new URL(window.location.href); url.searchParams.set(this.urlParamKeyValue, selectedTabId); window.history.replaceState({}, "", url); } + + // Update URL with the selected tab + if (this.sessionKeyValue) { + this.selectionInputTarget.value = selectedTabId; + this.persistSelectionFormTarget.requestSubmit(); + } } } diff --git a/app/controllers/concerns/account_groupable.rb b/app/controllers/concerns/account_groupable.rb index cf7e9e1b..6d4a41c3 100644 --- a/app/controllers/concerns/account_groupable.rb +++ b/app/controllers/concerns/account_groupable.rb @@ -5,40 +5,20 @@ module AccountGroupable before_action :set_account_group_tab end - def set_account_group_tab - last_selected_tab = session[:account_group_tab] || "asset" - - selected_tab = if account_group_tab_param - account_group_tab_param - elsif on_asset_page? - "asset" - elsif on_liability_page? - "liability" - else - last_selected_tab - end - - session[:account_group_tab] = selected_tab - @account_group_tab = selected_tab - end - private + def set_account_group_tab + last_selected_tab = session["custom_account_group_tab"] || "asset" + + @account_group_tab = account_group_tab_param || last_selected_tab + end + + def valid_account_group_tabs + %w[asset liability all] + end + def account_group_tab_param - valid_tabs = %w[asset liability all] - params[:account_group_tab].in?(valid_tabs) ? params[:account_group_tab] : nil - end - - def on_asset_page? - accountable_controller_names_for("asset").include?(controller_name) - end - - def on_liability_page? - accountable_controller_names_for("liability").include?(controller_name) - end - - def accountable_controller_names_for(classification) - Accountable::TYPES.map(&:constantize) - .select { |a| a.classification == classification } - .map { |a| a.model_name.plural } + param_value = params[:account_group_tab] + return nil unless param_value.in?(valid_account_group_tabs) + param_value end end diff --git a/app/controllers/cookie_sessions_controller.rb b/app/controllers/cookie_sessions_controller.rb new file mode 100644 index 00000000..7e76636f --- /dev/null +++ b/app/controllers/cookie_sessions_controller.rb @@ -0,0 +1,22 @@ +class CookieSessionsController < ApplicationController + def update + save_kv_to_session( + cookie_session_params[:tab_key], + cookie_session_params[:tab_value] + ) + + redirect_back_or_to root_path + end + + private + def cookie_session_params + params.require(:cookie_session).permit(:tab_key, :tab_value) + end + + def save_kv_to_session(key, value) + raise "Key must be a string" unless key.is_a?(String) + raise "Value must be a string" unless value.is_a?(String) + + session["custom_#{key}"] = value + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3b7357f8..a5fe41d9 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -31,4 +31,8 @@ class SessionsController < ApplicationController def set_session @session = Current.user.sessions.find(params[:id]) end + + def session_params + params.require(:session).permit(:tab_key, :tab_value) + end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2b3c0866..4cad0863 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -87,7 +87,7 @@ class UsersController < ApplicationController def user_params params.require(:user).permit( - :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, + :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, :preferred_account_group_tab, :show_sidebar, :default_period, :show_ai_sidebar, :ai_enabled, :theme, :set_onboarding_preferences_at, :set_onboarding_goals_at, family_attributes: [ :name, :currency, :country, :locale, :date_format, :timezone, :id ], goals: [] diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index 54ed199c..090775d4 100644 --- a/app/models/balance_sheet.rb +++ b/app/models/balance_sheet.rb @@ -48,9 +48,10 @@ class BalanceSheet def account_groups(classification = nil) classification_accounts = classification ? totals_query.filter { |t| t.classification == classification } : totals_query classification_total = classification_accounts.sum(&:converted_balance) - account_groups = classification_accounts.group_by(&:accountable_type).transform_keys { |k| Accountable.from_type(k) } + account_groups = classification_accounts.group_by(&:accountable_type) + .transform_keys { |k| Accountable.from_type(k) } - account_groups.map do |accountable, accounts| + groups = account_groups.map do |accountable, accounts| group_total = accounts.sum(&:converted_balance) AccountGroup.new( @@ -71,7 +72,13 @@ class BalanceSheet account end.sort_by(&:weight).reverse ) - end.sort_by(&:weight).reverse + 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_series(period: Period.last_30_days) diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index 06f70c2b..9fb3acca 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -21,7 +21,7 @@ <% end %> - <%= render TabsComponent.new(active_tab: active_account_group_tab, testid: "account-sidebar-tabs") do |tabs| %> + <%= render TabsComponent.new(active_tab: active_account_group_tab, session_key: "account_group_tab", testid: "account-sidebar-tabs") do |tabs| %> <% tabs.with_nav do |nav| %> <% nav.with_btn(id: "asset", label: "Assets") %> <% nav.with_btn(id: "liability", label: "Debts") %> diff --git a/app/views/accounts/chart.html.erb b/app/views/accounts/chart.html.erb index b5ad27a1..b181ef20 100644 --- a/app/views/accounts/chart.html.erb +++ b/app/views/accounts/chart.html.erb @@ -2,7 +2,7 @@ <% trend = series.trend %> <%= turbo_frame_tag dom_id(@account, :chart_details) do %> - <% if @account.syncing?%> + <% if @account.syncing? %> <%= render "accounts/chart_loader" %> <% else %>
diff --git a/app/views/accounts/show/_chart.html.erb b/app/views/accounts/show/_chart.html.erb index cc6c4f79..6a1dc545 100644 --- a/app/views/accounts/show/_chart.html.erb +++ b/app/views/accounts/show/_chart.html.erb @@ -14,7 +14,7 @@ <% end %>
- <% if account.syncing? %> + <% if account.syncing? %>
<% else %> <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 361aabfd..4b6ff347 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -26,7 +26,7 @@ <%= render( "accounts/account_sidebar_tabs", family: Current.family, - active_account_group_tab: params[:account_group_tab] || "assets" + active_account_group_tab: @account_group_tab ) %>
@@ -81,7 +81,7 @@ <% else %>
- <%= render "accounts/account_sidebar_tabs", family: Current.family, active_account_group_tab: params[:account_group_tab] || "assets" %> + <%= render "accounts/account_sidebar_tabs", family: Current.family, active_account_group_tab: @account_group_tab %>
<% if Current.family.trialing? && !self_hosted? %> diff --git a/app/views/pages/dashboard.html.erb b/app/views/pages/dashboard.html.erb index 79d18d4b..1f2f347e 100644 --- a/app/views/pages/dashboard.html.erb +++ b/app/views/pages/dashboard.html.erb @@ -26,9 +26,9 @@
<% if Current.family.accounts.any? %>
- <%= render partial: "pages/dashboard/net_worth_chart", locals: { - balance_sheet: @balance_sheet, - period: @period + <%= render partial: "pages/dashboard/net_worth_chart", locals: { + balance_sheet: @balance_sheet, + period: @period } %>
<% else %> diff --git a/app/views/pages/dashboard/_balance_sheet.html.erb b/app/views/pages/dashboard/_balance_sheet.html.erb index 196d96ae..c3606614 100644 --- a/app/views/pages/dashboard/_balance_sheet.html.erb +++ b/app/views/pages/dashboard/_balance_sheet.html.erb @@ -42,7 +42,7 @@
<% end %>
- <% end%> + <% end %>
diff --git a/app/views/rules/_rule.html.erb b/app/views/rules/_rule.html.erb index 9307eb80..1dbf9641 100644 --- a/app/views/rules/_rule.html.erb +++ b/app/views/rules/_rule.html.erb @@ -1,5 +1,5 @@ <%# locals: (rule:) %> -
+
">
<% if rule.name.present? %>

<%= rule.name %>

@@ -49,7 +49,7 @@ <% if rule.effective_date.nil? %> All past and future <%= rule.resource_type.pluralize %> <% else %> - <%= rule.resource_type.pluralize %> on or after <%= rule.effective_date.strftime('%b %-d, %Y') %> + <%= rule.resource_type.pluralize %> on or after <%= rule.effective_date.strftime("%b %-d, %Y") %> <% end %>

diff --git a/app/views/rules/confirm.html.erb b/app/views/rules/confirm.html.erb index 28be9496..03311791 100644 --- a/app/views/rules/confirm.html.erb +++ b/app/views/rules/confirm.html.erb @@ -1,5 +1,5 @@ <%= render DialogComponent.new(reload_on_close: true) do |dialog| %> - <% + <% title = if @rule.name.present? "Confirm changes to \"#{@rule.name}\"" else @@ -7,7 +7,7 @@ end %> <% dialog.with_header(title: title) %> - + <% dialog.with_body do %>

You are about to apply this rule to diff --git a/app/views/rules/edit.html.erb b/app/views/rules/edit.html.erb index f73edc90..91dea816 100644 --- a/app/views/rules/edit.html.erb +++ b/app/views/rules/edit.html.erb @@ -1,7 +1,7 @@ <%= link_to "Back to rules", rules_path %> <%= render DialogComponent.new do |dialog| %> - <% + <% title = if @rule.name.present? "Edit #{@rule.resource_type} rule \"#{@rule.name}\"" else diff --git a/app/views/rules/index.html.erb b/app/views/rules/index.html.erb index d549e0f8..c6ca513f 100644 --- a/app/views/rules/index.html.erb +++ b/app/views/rules/index.html.erb @@ -60,7 +60,7 @@

<% @rules.each_with_index do |rule, idx| %> - <%= render "rule", rule: rule%> + <%= render "rule", rule: rule %> <% unless idx == @rules.size - 1 %>
<% end %> diff --git a/config/routes.rb b/config/routes.rb index 132fa06a..1d68af04 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -26,6 +26,8 @@ Rails.application.routes.draw do get "changelog", to: "pages#changelog" get "feedback", to: "pages#feedback" + resource :cookie_session, only: %i[update] + resource :registration, only: %i[new create] resources :sessions, only: %i[new create destroy] resource :password_reset, only: %i[new create edit update] diff --git a/db/schema.rb b/db/schema.rb index 10ac65c8..04b8548b 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_05_13_122703) do +ActiveRecord::Schema[7.2].define(version: 2025_05_14_143017) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -696,6 +696,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_13_122703) do t.text "goals", default: [], array: true t.datetime "set_onboarding_preferences_at" t.datetime "set_onboarding_goals_at" + t.string "preferred_account_group_tab", default: "asset" t.index ["email"], name: "index_users_on_email", unique: true t.index ["family_id"], name: "index_users_on_family_id" t.index ["last_viewed_chat_id"], name: "index_users_on_last_viewed_chat_id" -- 2.53.0 From 0ff128b942c4df9cb6f21350108f2e8f4a517668 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 11:43:28 -0400 Subject: [PATCH 16/27] Remove manual sleep --- app/models/sync.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/sync.rb b/app/models/sync.rb index d004ec31..14a46ed2 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -46,8 +46,6 @@ class Sync < ApplicationRecord Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do start! - sleep 10 - begin syncable.perform_sync(self) rescue => e -- 2.53.0 From 153bffcaf275e13201068c90b4400b5b6decf496 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 12:52:21 -0400 Subject: [PATCH 17/27] Add migration to clear stale syncs on self hosted apps --- app/assets/tailwind/maybe-design-system.css | 11 +---------- app/controllers/users_controller.rb | 2 +- .../20250512171654_update_sync_timestamps.rb | 14 ++++++++++++++ db/schema.rb | 3 +-- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/app/assets/tailwind/maybe-design-system.css b/app/assets/tailwind/maybe-design-system.css index bfca010d..f9dc6039 100644 --- a/app/assets/tailwind/maybe-design-system.css +++ b/app/assets/tailwind/maybe-design-system.css @@ -240,16 +240,7 @@ 100% { stroke-dashoffset: 0; } - } - - @keyframes shiny-wave { - 0% { - background-position: -200% 0; - } - 100% { - background-position: 200% 0; - } - } + } } /* Specific override for strong tags in prose under dark mode */ diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4cad0863..2b3c0866 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -87,7 +87,7 @@ class UsersController < ApplicationController def user_params params.require(:user).permit( - :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, :preferred_account_group_tab, + :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, :show_sidebar, :default_period, :show_ai_sidebar, :ai_enabled, :theme, :set_onboarding_preferences_at, :set_onboarding_goals_at, family_attributes: [ :name, :currency, :country, :locale, :date_format, :timezone, :id ], goals: [] diff --git a/db/migrate/20250512171654_update_sync_timestamps.rb b/db/migrate/20250512171654_update_sync_timestamps.rb index 716f7019..81c1658f 100644 --- a/db/migrate/20250512171654_update_sync_timestamps.rb +++ b/db/migrate/20250512171654_update_sync_timestamps.rb @@ -28,6 +28,20 @@ class UpdateSyncTimestamps < ActiveRecord::Migration[7.2] UPDATE syncs SET window_start_date = start_date SQL + + # Due to some recent bugs, some self hosters have syncs that are stuck. + # This manually fails those syncs so they stop seeing syncing UI notices. + if Rails.application.config.app_mode.self_hosted? + puts "Self hosted: Fail syncs older than 2 hours" + execute <<-SQL + UPDATE syncs + SET status = 'failed' + WHERE ( + status = 'syncing' AND + created_at < NOW() - INTERVAL '2 hours' + ) + SQL + end end dir.down do diff --git a/db/schema.rb b/db/schema.rb index 04b8548b..10ac65c8 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_05_14_143017) do +ActiveRecord::Schema[7.2].define(version: 2025_05_13_122703) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -696,7 +696,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_14_143017) do t.text "goals", default: [], array: true t.datetime "set_onboarding_preferences_at" t.datetime "set_onboarding_goals_at" - t.string "preferred_account_group_tab", default: "asset" t.index ["email"], name: "index_users_on_email", unique: true t.index ["family_id"], name: "index_users_on_family_id" t.index ["last_viewed_chat_id"], name: "index_users_on_last_viewed_chat_id" -- 2.53.0 From b041edb648ff32e5b6d336c80259458ce9f908c2 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 14:16:46 -0400 Subject: [PATCH 18/27] Tweak sync states --- app/controllers/sessions_controller.rb | 4 ---- app/jobs/sync_market_data_job.rb | 2 +- app/models/account/syncer.rb | 3 ++- app/models/family.rb | 8 ++++++++ app/models/plaid_item/syncer.rb | 2 +- app/views/accounts/_account_sidebar_tabs.html.erb | 2 +- 6 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a5fe41d9..3b7357f8 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -31,8 +31,4 @@ class SessionsController < ApplicationController def set_session @session = Current.user.sessions.find(params[:id]) end - - def session_params - params.require(:session).permit(:tab_key, :tab_value) - end end diff --git a/app/jobs/sync_market_data_job.rb b/app/jobs/sync_market_data_job.rb index 38b1fe7c..074cda9f 100644 --- a/app/jobs/sync_market_data_job.rb +++ b/app/jobs/sync_market_data_job.rb @@ -1,7 +1,7 @@ class SyncMarketDataJob < ApplicationJob queue_as :scheduled - def perform(*args) + def perform MarketDataSyncer.new.sync_all end end diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index 4328e1d2..fcb586b5 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -12,7 +12,8 @@ class Account::Syncer def perform_post_sync account.family.auto_match_transfers! - account.family.broadcast_refresh + account.broadcast_refresh + account.family.broadcast_sidebar_refresh end private diff --git a/app/models/family.rb b/app/models/family.rb index 0cfb8b89..4db7fdf0 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -44,6 +44,14 @@ class Family < ApplicationRecord .exists? end + def broadcast_sidebar_refresh + broadcast_replace( + target: "account-sidebar-tabs", + partial: "accounts/sidebar_tabs", + locals: { family: self, active_account_group_tab: "all" } + ) + end + def assigned_merchants merchant_ids = transactions.where.not(merchant_id: nil).pluck(:merchant_id).uniq Merchant.where(id: merchant_ids) diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 4669e9b8..f6333403 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -24,7 +24,7 @@ class PlaidItem::Syncer def perform_post_sync plaid_item.auto_match_categories! - plaid_item.family.broadcast_refresh + plaid_item.family.broadcast_sidebar_refresh end private diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index 9fb3acca..74c7d705 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -1,4 +1,4 @@ -<%# locals: (family:, active_account_group_tab: "assets") %> +<%# locals: (family:, active_account_group_tab: "asset") %>
<% if family.missing_data_provider? %> -- 2.53.0 From 246343a4bfdfb09f633e293e435d32d5818558c7 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 21:04:20 -0400 Subject: [PATCH 19/27] Sync completion event broadcasts --- .../maybe-design-system/border-utils.css | 5 - app/components/tabs_component.html.erb | 5 - app/components/tabs_controller.js | 18 +++- app/controllers/application_controller.rb | 4 +- ...pable.rb => restore_layout_preferences.rb} | 10 +- .../current_sessions_controller.rb | 14 +++ app/models/account/syncer.rb | 2 +- app/models/balance_sheet.rb | 7 +- app/models/family.rb | 8 -- app/models/plaid_item/syncer.rb | 2 +- app/models/session.rb | 10 ++ app/models/sync.rb | 16 +-- app/models/sync_complete_event.rb | 37 +++++++ .../accounts/_account_sidebar_tabs.html.erb | 10 +- .../accounts/_accountable_group.html.erb | 101 +++++++++--------- app/views/accounts/show/_chart.html.erb | 8 +- app/views/accounts/show/_template.html.erb | 4 +- app/views/investments/show.html.erb | 32 ++---- app/views/layouts/application.html.erb | 5 +- .../pages/dashboard/_balance_sheet.html.erb | 4 +- .../pages/dashboard/_net_worth_chart.html.erb | 79 +++++++------- config/routes.rb | 2 +- .../20250514214242_add_metadata_to_session.rb | 5 + db/schema.rb | 3 +- .../current_sessions_controller_test.rb | 15 +++ 25 files changed, 237 insertions(+), 169 deletions(-) rename app/controllers/concerns/{account_groupable.rb => restore_layout_preferences.rb} (58%) create mode 100644 app/controllers/current_sessions_controller.rb create mode 100644 app/models/sync_complete_event.rb create mode 100644 db/migrate/20250514214242_add_metadata_to_session.rb create mode 100644 test/controllers/current_sessions_controller_test.rb diff --git a/app/assets/tailwind/maybe-design-system/border-utils.css b/app/assets/tailwind/maybe-design-system/border-utils.css index bc2d7a60..94c54a55 100644 --- a/app/assets/tailwind/maybe-design-system/border-utils.css +++ b/app/assets/tailwind/maybe-design-system/border-utils.css @@ -1,7 +1,6 @@ /* Custom shadow borders used for surfaces / containers */ @utility shadow-border-xs { box-shadow: var(--shadow-xs), 0px 0px 0px 1px var(--color-alpha-black-50); - transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-xs), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -10,7 +9,6 @@ @utility shadow-border-sm { box-shadow: var(--shadow-sm), 0px 0px 0px 1px var(--color-alpha-black-50); - transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-sm), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -19,7 +17,6 @@ @utility shadow-border-md { box-shadow: var(--shadow-md), 0px 0px 0px 1px var(--color-alpha-black-50); - transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-md), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -28,7 +25,6 @@ @utility shadow-border-lg { box-shadow: var(--shadow-lg), 0px 0px 0px 1px var(--color-alpha-black-50); - transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-lg), 0px 0px 0px 1px var(--color-alpha-white-50); @@ -37,7 +33,6 @@ @utility shadow-border-xl { box-shadow: var(--shadow-xl), 0px 0px 0px 1px var(--color-alpha-black-50); - transform: translateZ(0); @variant theme-dark { box-shadow: var(--shadow-xl), 0px 0px 0px 1px var(--color-alpha-white-50); diff --git a/app/components/tabs_component.html.erb b/app/components/tabs_component.html.erb index 9a1d938a..bfceddad 100644 --- a/app/components/tabs_component.html.erb +++ b/app/components/tabs_component.html.erb @@ -11,11 +11,6 @@ <% else %> <%= nav %> - <%= form_with url: cookie_session_path, scope: :cookie_session, method: :put, data: { tabs_target: "persistSelectionForm" } do |f| %> - <%= f.hidden_field :tab_key, value: "account_group_tab" %> - <%= f.hidden_field :tab_value, value: "all", data: { tabs_target: "selectionInput" } %> - <% end %> - <% panels.each do |panel| %> <%= panel %> <% end %> diff --git a/app/components/tabs_controller.js b/app/components/tabs_controller.js index 5b83b0ad..4fc5cf22 100644 --- a/app/components/tabs_controller.js +++ b/app/components/tabs_controller.js @@ -36,8 +36,22 @@ export default class extends Controller { // Update URL with the selected tab if (this.sessionKeyValue) { - this.selectionInputTarget.value = selectedTabId; - this.persistSelectionFormTarget.requestSubmit(); + this.#updateSessionPreference(selectedTabId); } } + + #updateSessionPreference(selectedTabId) { + fetch("/current_session", { + method: "PUT", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + "X-CSRF-Token": document.querySelector('[name="csrf-token"]').content, + Accept: "application/json", + }, + body: new URLSearchParams({ + "current_session[tab_key]": this.sessionKeyValue, + "current_session[tab_value]": selectedTabId, + }).toString(), + }); + } } diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8a1642f9..260579d1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,7 +1,7 @@ class ApplicationController < ActionController::Base - include Onboardable, Localize, AutoSync, Authentication, Invitable, + include RestoreLayoutPreferences, Onboardable, Localize, AutoSync, Authentication, Invitable, SelfHostable, StoreLocation, Impersonatable, Breadcrumbable, - FeatureGuardable, Notifiable, AccountGroupable + FeatureGuardable, Notifiable include Pagy::Backend diff --git a/app/controllers/concerns/account_groupable.rb b/app/controllers/concerns/restore_layout_preferences.rb similarity index 58% rename from app/controllers/concerns/account_groupable.rb rename to app/controllers/concerns/restore_layout_preferences.rb index 6d4a41c3..284df4cc 100644 --- a/app/controllers/concerns/account_groupable.rb +++ b/app/controllers/concerns/restore_layout_preferences.rb @@ -1,13 +1,13 @@ -module AccountGroupable +module RestoreLayoutPreferences extend ActiveSupport::Concern included do - before_action :set_account_group_tab + before_action :restore_active_tabs end private - def set_account_group_tab - last_selected_tab = session["custom_account_group_tab"] || "asset" + def restore_active_tabs + last_selected_tab = Current.session&.get_preferred_tab("account_sidebar_tab") || "asset" @account_group_tab = account_group_tab_param || last_selected_tab end @@ -17,7 +17,7 @@ module AccountGroupable end def account_group_tab_param - param_value = params[:account_group_tab] + param_value = params[:account_sidebar_tab] return nil unless param_value.in?(valid_account_group_tabs) param_value end diff --git a/app/controllers/current_sessions_controller.rb b/app/controllers/current_sessions_controller.rb new file mode 100644 index 00000000..303b51f0 --- /dev/null +++ b/app/controllers/current_sessions_controller.rb @@ -0,0 +1,14 @@ +class CurrentSessionsController < ApplicationController + def update + if session_params[:tab_key].present? && session_params[:tab_value].present? + Current.session.set_preferred_tab(session_params[:tab_key], session_params[:tab_value]) + end + + head :ok + end + + private + def session_params + params.require(:current_session).permit(:tab_key, :tab_value) + end +end diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index fcb586b5..e21e25a1 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -13,7 +13,7 @@ class Account::Syncer def perform_post_sync account.family.auto_match_transfers! account.broadcast_refresh - account.family.broadcast_sidebar_refresh + SyncCompleteEvent.new(account.family, accounts: [ account ]).broadcast end private diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index 090775d4..cc50e3da 100644 --- a/app/models/balance_sheet.rb +++ b/app/models/balance_sheet.rb @@ -54,8 +54,11 @@ class BalanceSheet groups = account_groups.map do |accountable, accounts| group_total = accounts.sum(&:converted_balance) + key = accountable.model_name.param_key + AccountGroup.new( - key: accountable.model_name.param_key, + id: classification ? "#{classification}_#{key}_group" : "#{key}_group", + key: key, name: accountable.display_name, classification: accountable.classification, total: group_total, @@ -95,7 +98,7 @@ class BalanceSheet private ClassificationGroup = Struct.new(:key, :display_name, :icon, :total_money, :account_groups, :syncing?, keyword_init: true) - AccountGroup = Struct.new(:key, :name, :accountable_type, :classification, :total, :total_money, :weight, :accounts, :color, :missing_rates?, :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 diff --git a/app/models/family.rb b/app/models/family.rb index 4db7fdf0..0cfb8b89 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -44,14 +44,6 @@ class Family < ApplicationRecord .exists? end - def broadcast_sidebar_refresh - broadcast_replace( - target: "account-sidebar-tabs", - partial: "accounts/sidebar_tabs", - locals: { family: self, active_account_group_tab: "all" } - ) - end - def assigned_merchants merchant_ids = transactions.where.not(merchant_id: nil).pluck(:merchant_id).uniq Merchant.where(id: merchant_ids) diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index f6333403..51c3f32f 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -24,7 +24,7 @@ class PlaidItem::Syncer def perform_post_sync plaid_item.auto_match_categories! - plaid_item.family.broadcast_sidebar_refresh + SyncCompleteEvent.new(plaid_item.family, accounts: plaid_item.accounts).broadcast end private diff --git a/app/models/session.rb b/app/models/session.rb index ce26938a..66faa0f5 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -9,4 +9,14 @@ class Session < ApplicationRecord self.user_agent = Current.user_agent self.ip_address = Current.ip_address end + + def get_preferred_tab(tab_key) + data.dig("tab_preferences", tab_key) + end + + def set_preferred_tab(tab_key, tab_value) + data["tab_preferences"] ||= {} + data["tab_preferences"][tab_key] = tab_value + save! + end end diff --git a/app/models/sync.rb b/app/models/sync.rb index 14a46ed2..134b18c3 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -49,7 +49,9 @@ class Sync < ApplicationRecord begin syncable.perform_sync(self) rescue => e - fail_and_report_error(e) + fail! + update(error: e.message) + report_error(e) ensure finalize_if_all_children_finalized end @@ -64,9 +66,6 @@ class Sync < ApplicationRecord # If this is the "parent" and there are still children running, don't finalize. return unless all_children_finalized? - # If we make it here, the sync is finalized. Run post-sync, regardless of failure/success. - perform_post_sync - if syncing? if has_failed_children? fail! @@ -74,6 +73,9 @@ class Sync < ApplicationRecord complete! end end + + # If we make it here, the sync is finalized. Run post-sync, regardless of failure/success. + perform_post_sync end # If this sync has a parent, try to finalize it so the child status propagates up the chain. @@ -96,12 +98,10 @@ class Sync < ApplicationRecord def perform_post_sync syncable.perform_post_sync rescue => e - fail_and_report_error(e) + report_error(e) end - def fail_and_report_error(error) - fail! - update(error: error.message) + def report_error(error) Sentry.capture_exception(error) do |scope| scope.set_tags(sync_id: id) end diff --git a/app/models/sync_complete_event.rb b/app/models/sync_complete_event.rb new file mode 100644 index 00000000..44a17d37 --- /dev/null +++ b/app/models/sync_complete_event.rb @@ -0,0 +1,37 @@ +class SyncCompleteEvent + attr_reader :family, :accounts + + def initialize(family, accounts: []) + @family = family + @accounts = accounts + end + + def broadcast + account_groups = family.balance_sheet.account_groups.select do |group| + group.accounts.any? { |a| accounts.include?(a) } + end + + account_groups.each do |account_group| + [ nil, "asset", "liability" ].each do |classification| + id = classification ? "#{classification}_#{account_group.id}" : account_group.id + family.broadcast_replace( + target: id, + partial: "accounts/accountable_group", + locals: { account_group: account_group, open: true } + ) + end + end + + family.broadcast_replace( + target: "balance-sheet", + partial: "pages/dashboard/balance_sheet", + locals: { balance_sheet: family.balance_sheet } + ) + + family.broadcast_replace( + target: "net-worth-chart", + partial: "pages/dashboard/net_worth_chart", + locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days } + ) + end +end diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index 74c7d705..056e7be1 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -1,4 +1,4 @@ -<%# locals: (family:, active_account_group_tab: "asset") %> +<%# locals: (family:, active_tab:, mobile: false) %>
<% if family.missing_data_provider? %> @@ -21,7 +21,7 @@ <% end %> - <%= render TabsComponent.new(active_tab: active_account_group_tab, session_key: "account_group_tab", testid: "account-sidebar-tabs") do |tabs| %> + <%= render TabsComponent.new(active_tab: active_tab, session_key: "account_sidebar_tab", testid: "account-sidebar-tabs") do |tabs| %> <% tabs.with_nav do |nav| %> <% nav.with_btn(id: "asset", label: "Assets") %> <% nav.with_btn(id: "liability", label: "Debts") %> @@ -42,7 +42,7 @@
<% family.balance_sheet.account_groups("asset").each do |group| %> - <%= render "accounts/accountable_group", account_group: group %> + <%= render "accounts/accountable_group", account_group: group, mobile: mobile %> <% end %>
@@ -62,7 +62,7 @@
<% family.balance_sheet.account_groups("liability").each do |group| %> - <%= render "accounts/accountable_group", account_group: 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 %> + <%= render "accounts/accountable_group", account_group: group, mobile: mobile %> <% end %>
diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index 33fc42e8..26633918 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -1,67 +1,69 @@ -<%# locals: (account_group:) %> +<%# locals: (account_group:, mobile: false, open: nil, **args) %> -<%= render DisclosureComponent.new(title: account_group.name, align: :left, open: account_group.accounts.any? { |account| page_active?(account_path(account)) }) do |disclosure| %> - <% disclosure.with_summary_content do %> -
- <% if account_group.syncing? %> -
-
-
-
-
-
- <% 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" do %> -
-
+
"> + <% 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? %> +
+
+
+
+
+ <% 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" do %> +
+
+
+ <% end %> <% end %> - <% end %> -
- <% end %> +
+ <% end %> -
- <% account_group.accounts.each do |account| %> - <%= link_to account_path(account), +
+ <% account_group.accounts.each do |account| %> + <%= link_to account_path(account), class: class_names( "block flex items-center gap-2 px-3 py-2 rounded-lg", page_active?(account_path(account)) ? "bg-container" : "hover:bg-surface-hover" ), title: account.name do %> - <%= render "accounts/logo", account: account, size: "sm", color: account_group.color %> + <%= 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.short_subtype_label, class: "text-sm text-secondary truncate" %> -
+
+ <%= tag.p account.name, class: "text-sm text-primary font-medium mb-0.5 truncate" %> + <%= tag.p account.short_subtype_label, class: "text-sm text-secondary truncate" %> +
- <% if account.syncing? %> -
-
-
-
-
+ <% if account.syncing? %> +
+
+
+
+
+
-
- <% else %> -
- <%= tag.p format_money(account.balance_money), class: "text-sm font-medium text-primary whitespace-nowrap" %> + <% 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" do %> -
-
-
- <% end %> -
+ <%= turbo_frame_tag dom_id(account, :sparkline), src: sparkline_account_path(account), loading: "lazy" do %> +
+
+
+ <% end %> +
+ <% end %> <% end %> <% end %> - <% end %> -
+
-
- <%= render LinkComponent.new( +
+ <%= render LinkComponent.new( href: new_polymorphic_path(account_group.key, step: "method_select"), text: "New #{account_group.name.downcase.singularize}", icon: "plus", @@ -70,5 +72,6 @@ frame: :modal, class: "justify-start" ) %> -
-<% end %> +
+ <% end %> +
diff --git a/app/views/accounts/show/_chart.html.erb b/app/views/accounts/show/_chart.html.erb index 6a1dc545..0f63c9f1 100644 --- a/app/views/accounts/show/_chart.html.erb +++ b/app/views/accounts/show/_chart.html.erb @@ -1,4 +1,4 @@ -<%# locals: (account:, title: nil, tooltip: nil, chart_view: nil, **args) %> +<%# locals: (account:, tooltip: nil, chart_view: nil, **args) %> <% period = @period || Period.last_30_days %> <% default_value_title = account.asset? ? t(".balance") : t(".owed") %> @@ -7,10 +7,10 @@
- <%= tag.p title || default_value_title, class: "text-sm font-medium text-secondary" %> + <%= tag.p account.investment? ? "Total value" : default_value_title, class: "text-sm font-medium text-secondary" %> - <% unless account.syncing? %> - <%= tooltip %> + <% if !account.syncing? && account.investment? %> + <%= render "investments/value_tooltip", balance: account.balance_money, holdings: account.balance_money - account.cash_balance_money, cash: account.cash_balance_money %> <% end %>
diff --git a/app/views/accounts/show/_template.html.erb b/app/views/accounts/show/_template.html.erb index cfac9402..51327254 100644 --- a/app/views/accounts/show/_template.html.erb +++ b/app/views/accounts/show/_template.html.erb @@ -1,4 +1,4 @@ -<%# locals: (account:, header: nil, chart: nil, tabs: nil) %> +<%# locals: (account:, header: nil, chart: nil, chart_view: nil, tabs: nil) %> <%= turbo_stream_from account %> @@ -13,7 +13,7 @@ <% if chart.present? %> <%= chart %> <% else %> - <%= render "accounts/show/chart", account: account %> + <%= render "accounts/show/chart", account: account, chart_view: chart_view %> <% end %>
diff --git a/app/views/investments/show.html.erb b/app/views/investments/show.html.erb index 7bd7da3b..84be8629 100644 --- a/app/views/investments/show.html.erb +++ b/app/views/investments/show.html.erb @@ -1,25 +1,7 @@ -<%= turbo_stream_from @account %> - -<%= turbo_frame_tag dom_id(@account) do %> - <%= tag.div class: "space-y-4" do %> - <%= render "accounts/show/header", account: @account %> - - <%= render "accounts/show/chart", - account: @account, - title: t(".chart_title"), - chart_view: @chart_view, - tooltip: render( - "investments/value_tooltip", - balance: @account.balance_money, - holdings: @account.balance_money - @account.cash_balance_money, - cash: @account.cash_balance_money - ) %> - -
- <%= render "accounts/show/tabs", account: @account, tabs: [ - { key: "activity", contents: render("accounts/show/activity", account: @account) }, - { key: "holdings", contents: render("investments/holdings_tab", account: @account) }, - ] %> -
- <% end %> -<% end %> +<%= render "accounts/show/template", + account: @account, + chart_view: @chart_view, + tabs: render("accounts/show/tabs", account: @account, tabs: [ + { key: "activity", contents: render("accounts/show/activity", account: @account) }, + { key: "holdings", contents: render("investments/holdings_tab", account: @account) }, + ]) %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 4b6ff347..e8fc202e 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -26,7 +26,8 @@ <%= render( "accounts/account_sidebar_tabs", family: Current.family, - active_account_group_tab: @account_group_tab + active_tab: @account_group_tab, + mobile: true ) %>
@@ -81,7 +82,7 @@ <% else %>
- <%= render "accounts/account_sidebar_tabs", family: Current.family, active_account_group_tab: @account_group_tab %> + <%= render "accounts/account_sidebar_tabs", family: Current.family, active_tab: @account_group_tab %>
<% if Current.family.trialing? && !self_hosted? %> diff --git a/app/views/pages/dashboard/_balance_sheet.html.erb b/app/views/pages/dashboard/_balance_sheet.html.erb index c3606614..6b4ea525 100644 --- a/app/views/pages/dashboard/_balance_sheet.html.erb +++ b/app/views/pages/dashboard/_balance_sheet.html.erb @@ -1,6 +1,6 @@ -<%# locals: (balance_sheet:) %> +<%# locals: (balance_sheet:, **args) %> -
+
<% balance_sheet.classification_groups.each do |classification_group| %>

diff --git a/app/views/pages/dashboard/_net_worth_chart.html.erb b/app/views/pages/dashboard/_net_worth_chart.html.erb index 4d1e443a..a6b65852 100644 --- a/app/views/pages/dashboard/_net_worth_chart.html.erb +++ b/app/views/pages/dashboard/_net_worth_chart.html.erb @@ -1,54 +1,55 @@ -<%# locals: (balance_sheet:, period:) %> +<%# locals: (balance_sheet:, period:, **args) %> -<% series = balance_sheet.net_worth_series(period: period) %> - -
-
+
+ <% series = balance_sheet.net_worth_series(period: period) %> +
-

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

+
+

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

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

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

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

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

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

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

- <%= form_with url: root_path, method: :get, data: { controller: "auto-submit-form" } do |form| %> - <%= form.select :period, + <% if series.trend.nil? %> +

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

+ <% else %> + <%= render partial: "shared/trend_change", locals: { trend: series.trend, comparison_label: period.comparison_label } %> + <% end %> + <% end %> +
+
+ + <%= form_with url: root_path, method: :get, data: { controller: "auto-submit-form" } do |form| %> + <%= form.select :period, Period.as_options, { selected: period.key }, data: { "auto-submit-form-target": "auto" }, class: "bg-container border border-secondary font-medium rounded-lg px-3 py-2 text-sm pr-7 cursor-pointer text-primary focus:outline-hidden focus:ring-0" %> - <% end %> -
- -<% if balance_sheet.syncing? %> -
-
+ <% end %>
-<% else %> - <% if series.any? %> -
+
+
+
+ <% else %> + <% if series.any? %> +
- <% else %> -
-

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

-
+ <% else %> +
+

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

+
+ <% end %> <% end %> -<% end %> +
diff --git a/config/routes.rb b/config/routes.rb index 1d68af04..ec9e2cce 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -26,7 +26,7 @@ Rails.application.routes.draw do get "changelog", to: "pages#changelog" get "feedback", to: "pages#feedback" - resource :cookie_session, only: %i[update] + resource :current_session, only: %i[update] resource :registration, only: %i[new create] resources :sessions, only: %i[new create destroy] diff --git a/db/migrate/20250514214242_add_metadata_to_session.rb b/db/migrate/20250514214242_add_metadata_to_session.rb new file mode 100644 index 00000000..849cdccf --- /dev/null +++ b/db/migrate/20250514214242_add_metadata_to_session.rb @@ -0,0 +1,5 @@ +class AddMetadataToSession < ActiveRecord::Migration[7.2] + def change + add_column :sessions, :data, :jsonb, default: {} + end +end diff --git a/db/schema.rb b/db/schema.rb index 10ac65c8..7f25cb20 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_05_13_122703) do +ActiveRecord::Schema[7.2].define(version: 2025_05_14_214242) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -537,6 +537,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_13_122703) do t.uuid "active_impersonator_session_id" t.datetime "subscribed_at" t.jsonb "prev_transaction_page_params", default: {} + t.jsonb "data", default: {} t.index ["active_impersonator_session_id"], name: "index_sessions_on_active_impersonator_session_id" t.index ["user_id"], name: "index_sessions_on_user_id" end diff --git a/test/controllers/current_sessions_controller_test.rb b/test/controllers/current_sessions_controller_test.rb new file mode 100644 index 00000000..9d498fb3 --- /dev/null +++ b/test/controllers/current_sessions_controller_test.rb @@ -0,0 +1,15 @@ +require "test_helper" + +class CurrentSessionsControllerTest < ActionDispatch::IntegrationTest + setup do + @user = users(:family_admin) + sign_in @user + end + + test "can update the preferred tab for any namespace" do + put current_session_url, params: { current_session: { tab_key: "accounts_sidebar_tab", tab_value: "asset" } } + assert_response :success + session = Session.order(updated_at: :desc).first + assert_equal "asset", session.get_preferred_tab("accounts_sidebar_tab") + end +end -- 2.53.0 From 7d32423d8e7969a68d5b2f534092abf3fd4d65d2 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 14 May 2025 21:19:53 -0400 Subject: [PATCH 20/27] Fix timezones in tests --- .../models/balance/forward_calculator_test.rb | 20 ++++++------- .../models/balance/reverse_calculator_test.rb | 18 +++++------ .../models/holding/forward_calculator_test.rb | 26 ++++++++-------- .../models/holding/reverse_calculator_test.rb | 30 +++++++++---------- 4 files changed, 47 insertions(+), 47 deletions(-) diff --git a/test/models/balance/forward_calculator_test.rb b/test/models/balance/forward_calculator_test.rb index 24aecdb5..05215c25 100644 --- a/test/models/balance/forward_calculator_test.rb +++ b/test/models/balance/forward_calculator_test.rb @@ -15,19 +15,19 @@ class Balance::ForwardCalculatorTest < ActiveSupport::TestCase test "balance generation respects user timezone and last generated date is current user date" do # Simulate user in EST timezone - Time.zone = "America/New_York" + Time.use_zone("America/New_York") do + # Set current time to 1am UTC on Jan 5, 2025 + # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for) + travel_to Time.utc(2025, 01, 05, 1, 0, 0) - # Set current time to 1am UTC on Jan 5, 2025 - # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for) - travel_to Time.utc(2025, 01, 05, 1, 0, 0) + # Create a valuation for Jan 3, 2025 + create_valuation(account: @account, date: "2025-01-03", amount: 17000) - # Create a valuation for Jan 3, 2025 - create_valuation(account: @account, date: "2025-01-03", amount: 17000) + expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 17000 ], [ "2025-01-04", 17000 ] ] + calculated = Balance::ForwardCalculator.new(@account).calculate - expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 17000 ], [ "2025-01-04", 17000 ] ] - calculated = Balance::ForwardCalculator.new(@account).calculate - - assert_equal expected, calculated.map { |b| [ b.date.to_s, b.balance ] } + assert_equal expected, calculated.map { |b| [ b.date.to_s, b.balance ] } + end end # When syncing forwards, we don't care about the account balance. We generate everything based on entries, starting from 0. diff --git a/test/models/balance/reverse_calculator_test.rb b/test/models/balance/reverse_calculator_test.rb index 38ede057..6d73aea8 100644 --- a/test/models/balance/reverse_calculator_test.rb +++ b/test/models/balance/reverse_calculator_test.rb @@ -25,18 +25,18 @@ class Balance::ReverseCalculatorTest < ActiveSupport::TestCase test "balance generation respects user timezone and last generated date is current user date" do # Simulate user in EST timezone - Time.zone = "America/New_York" + Time.use_zone("America/New_York") do + # Set current time to 1am UTC on Jan 5, 2025 + # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for) + travel_to Time.utc(2025, 01, 05, 1, 0, 0) - # Set current time to 1am UTC on Jan 5, 2025 - # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for) - travel_to Time.utc(2025, 01, 05, 1, 0, 0) + create_valuation(account: @account, date: "2025-01-03", amount: 17000) - create_valuation(account: @account, date: "2025-01-03", amount: 17000) + expected = [ [ "2025-01-02", 17000 ], [ "2025-01-03", 17000 ], [ "2025-01-04", @account.balance ] ] + calculated = Balance::ReverseCalculator.new(@account).calculate - expected = [ [ "2025-01-02", 17000 ], [ "2025-01-03", 17000 ], [ "2025-01-04", @account.balance ] ] - calculated = Balance::ReverseCalculator.new(@account).calculate - - assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.balance ] } + assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.balance ] } + end end test "valuations sync" do diff --git a/test/models/holding/forward_calculator_test.rb b/test/models/holding/forward_calculator_test.rb index 7a8aaa53..89e9b28c 100644 --- a/test/models/holding/forward_calculator_test.rb +++ b/test/models/holding/forward_calculator_test.rb @@ -20,22 +20,22 @@ class Holding::ForwardCalculatorTest < ActiveSupport::TestCase test "holding generation respects user timezone and last generated date is current user date" do # Simulate user in EST timezone - Time.zone = "America/New_York" + Time.use_zone("America/New_York") do + # Set current time to 1am UTC on Jan 5, 2025 + # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for) + travel_to Time.utc(2025, 01, 05, 1, 0, 0) - # Set current time to 1am UTC on Jan 5, 2025 - # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for) - travel_to Time.utc(2025, 01, 05, 1, 0, 0) + voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF") + Security::Price.create!(security: voo, date: "2025-01-02", price: 500) + Security::Price.create!(security: voo, date: "2025-01-03", price: 500) + Security::Price.create!(security: voo, date: "2025-01-04", price: 500) + create_trade(voo, qty: 10, date: "2025-01-03", price: 500, account: @account) - voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF") - Security::Price.create!(security: voo, date: "2025-01-02", price: 500) - Security::Price.create!(security: voo, date: "2025-01-03", price: 500) - Security::Price.create!(security: voo, date: "2025-01-04", price: 500) - create_trade(voo, qty: 10, date: "2025-01-03", price: 500, account: @account) + expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 5000 ], [ "2025-01-04", 5000 ] ] + calculated = Holding::ForwardCalculator.new(@account).calculate - expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 5000 ], [ "2025-01-04", 5000 ] ] - calculated = Holding::ForwardCalculator.new(@account).calculate - - assert_equal expected, calculated.map { |b| [ b.date.to_s, b.amount ] } + assert_equal expected, calculated.map { |b| [ b.date.to_s, b.amount ] } + end end test "forward portfolio calculation" do diff --git a/test/models/holding/reverse_calculator_test.rb b/test/models/holding/reverse_calculator_test.rb index ec9dc204..785d7b94 100644 --- a/test/models/holding/reverse_calculator_test.rb +++ b/test/models/holding/reverse_calculator_test.rb @@ -20,26 +20,26 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase test "holding generation respects user timezone and last generated date is current user date" do # Simulate user in EST timezone - Time.zone = "America/New_York" + Time.use_zone("America/New_York") do + # Set current time to 1am UTC on Jan 5, 2025 + # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for) + travel_to Time.utc(2025, 01, 05, 1, 0, 0) - # Set current time to 1am UTC on Jan 5, 2025 - # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for) - travel_to Time.utc(2025, 01, 05, 1, 0, 0) + voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF") + Security::Price.create!(security: voo, date: "2025-01-02", price: 500) + Security::Price.create!(security: voo, date: "2025-01-03", price: 500) + Security::Price.create!(security: voo, date: "2025-01-04", price: 500) - voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF") - Security::Price.create!(security: voo, date: "2025-01-02", price: 500) - Security::Price.create!(security: voo, date: "2025-01-03", price: 500) - Security::Price.create!(security: voo, date: "2025-01-04", price: 500) + # Today's holdings (provided) + @account.holdings.create!(security: voo, date: "2025-01-04", qty: 10, price: 500, amount: 5000, currency: "USD") - # Today's holdings (provided) - @account.holdings.create!(security: voo, date: "2025-01-04", qty: 10, price: 500, amount: 5000, currency: "USD") + create_trade(voo, qty: 10, date: "2025-01-03", price: 500, account: @account) - create_trade(voo, qty: 10, date: "2025-01-03", price: 500, account: @account) + expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 5000 ], [ "2025-01-04", 5000 ] ] + calculated = Holding::ReverseCalculator.new(@account).calculate - expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 5000 ], [ "2025-01-04", 5000 ] ] - calculated = Holding::ReverseCalculator.new(@account).calculate - - assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.amount ] } + assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.amount ] } + end end # Should be able to handle this case, although we should not be reverse-syncing an account without provided current day holdings -- 2.53.0 From c1544d73c0dc09e1415c845ac90386f040a5edf5 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 07:55:47 -0400 Subject: [PATCH 21/27] Cleanup --- app/components/tabs_controller.js | 2 +- db/migrate/20250512171654_update_sync_timestamps.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/tabs_controller.js b/app/components/tabs_controller.js index 4fc5cf22..259765aa 100644 --- a/app/components/tabs_controller.js +++ b/app/components/tabs_controller.js @@ -3,7 +3,7 @@ import { Controller } from "@hotwired/stimulus"; // Connects to data-controller="tabs--components" export default class extends Controller { static classes = ["navBtnActive", "navBtnInactive"]; - static targets = ["panel", "navBtn", "persistSelectionForm", "selectionInput"]; + static targets = ["panel", "navBtn"]; static values = { sessionKey: String, urlParamKey: String }; show(e) { diff --git a/db/migrate/20250512171654_update_sync_timestamps.rb b/db/migrate/20250512171654_update_sync_timestamps.rb index 81c1658f..ac0830b6 100644 --- a/db/migrate/20250512171654_update_sync_timestamps.rb +++ b/db/migrate/20250512171654_update_sync_timestamps.rb @@ -48,7 +48,7 @@ class UpdateSyncTimestamps < ActiveRecord::Migration[7.2] execute <<-SQL UPDATE syncs SET - last_ran_at = completed_at + last_ran_at = COALESCE(completed_at, failed_at) SQL execute <<-SQL -- 2.53.0 From 65ab4e008942e17cf82b40f6abd47f5d07580e09 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 07:57:07 -0400 Subject: [PATCH 22/27] More cleanup --- app/models/security/provided.rb | 38 ------------------------------ test/models/security/price_test.rb | 19 --------------- 2 files changed, 57 deletions(-) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index b342c9e5..3d344f29 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -28,44 +28,6 @@ module Security::Provided end end - def sync_provider_prices(start_date:, end_date: Date.current) - unless has_prices? - Rails.logger.warn("Security id=#{id} ticker=#{ticker} is not known by provider, skipping price sync") - return 0 - end - - unless provider.present? - Rails.logger.warn("No security provider configured, cannot sync prices for id=#{id} ticker=#{ticker}") - return 0 - end - - response = provider.fetch_security_prices(self, start_date: start_date, end_date: end_date) - - unless response.success? - Rails.logger.error("Provider error for sync_provider_prices with id=#{id} ticker=#{ticker}: #{response.error}") - return 0 - end - - fetched_prices = response.data.map do |price| - { - security_id: price.security.id, - date: price.date, - price: price.price, - currency: price.currency - } - end - - valid_prices = fetched_prices.reject do |price| - is_invalid = price[:date].nil? || price[:price].nil? || price[:currency].nil? - if is_invalid - Rails.logger.warn("Invalid price data for security_id=#{id}: Missing required fields in price record: #{price.inspect}") - end - is_invalid - end - - Security::Price.upsert_all(valid_prices, unique_by: %i[security_id date currency]) - end - def find_or_fetch_price(date: Date.current, cache: true) price = prices.find_by(date: date) diff --git a/test/models/security/price_test.rb b/test/models/security/price_test.rb index bd150359..14c022fe 100644 --- a/test/models/security/price_test.rb +++ b/test/models/security/price_test.rb @@ -49,25 +49,6 @@ class Security::PriceTest < ActiveSupport::TestCase assert_not @security.find_or_fetch_price(date: Date.current) end - test "upserts historical prices from provider" do - Security::Price.delete_all - - # Will be overwritten by upsert - Security::Price.create!(security: @security, date: 1.day.ago.to_date, price: 190, currency: "USD") - - expect_provider_prices(security: @security, start_date: 2.days.ago.to_date, end_date: Date.current, prices: [ - Security::Price.new(security: @security, date: Date.current, price: 215, currency: "USD"), - Security::Price.new(security: @security, date: 1.day.ago.to_date, price: 214, currency: "USD"), - Security::Price.new(security: @security, date: 2.days.ago.to_date, price: 213, currency: "USD") - ]) - - @security.sync_provider_prices(start_date: 2.days.ago.to_date) - - assert_equal 215, @security.prices.find_by(date: Date.current).price - assert_equal 214, @security.prices.find_by(date: 1.day.ago.to_date).price - assert_equal 213, @security.prices.find_by(date: 2.days.ago.to_date).price - end - private def expect_provider_price(security:, price:, date:) @provider.expects(:fetch_security_price) -- 2.53.0 From dd5292555a5ad21bdbdd54dafbe0548797756981 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 08:18:12 -0400 Subject: [PATCH 23/27] Plaid item UI broadcasts for sync --- app/models/account.rb | 12 ++++++++++++ app/models/account/syncer.rb | 6 ++++++ app/models/concerns/syncable.rb | 2 +- app/models/plaid_item/syncer.rb | 6 ++++++ 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/models/account.rb b/app/models/account.rb index ee2fc963..8c74b83e 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -61,6 +61,18 @@ class Account < ApplicationRecord end end + def syncing? + self_syncing = syncs.incomplete.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/syncer.rb b/app/models/account/syncer.rb index e21e25a1..5554370b 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -14,6 +14,12 @@ class Account::Syncer account.family.auto_match_transfers! account.broadcast_refresh SyncCompleteEvent.new(account.family, accounts: [ account ]).broadcast + account.broadcast_replace_to( + account.family, + target: "account_#{account.id}", + partial: "accounts/account", + locals: { account: account } + ) end private diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 33668458..3d4ec844 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -6,7 +6,7 @@ module Syncable end def syncing? - syncs.incomplete.any? + raise NotImplementedError, "Subclasses must implement the syncing? method" end def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil) diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 51c3f32f..8baeed42 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -25,6 +25,12 @@ class PlaidItem::Syncer def perform_post_sync plaid_item.auto_match_categories! SyncCompleteEvent.new(plaid_item.family, accounts: plaid_item.accounts).broadcast + plaid_item.broadcast_replace_to( + plaid_item.family, + target: "plaid_item_#{plaid_item.id}", + partial: "plaid_items/plaid_item", + locals: { plaid_item: plaid_item } + ) end private -- 2.53.0 From 37d32e47a4c8efcfcb93f3ef33f6c1d0c72a5c71 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 08:40:59 -0400 Subject: [PATCH 24/27] Fix account ID namespace conflict --- app/models/concerns/syncable.rb | 2 +- app/models/sync.rb | 7 ------- app/views/accounts/show/_template.html.erb | 2 +- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 3d4ec844..5223d587 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -10,7 +10,7 @@ module Syncable end def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil) - new_sync = syncs.create_with_defaults!(parent: parent_sync) + new_sync = syncs.create!(parent: parent_sync, window_start_date: window_start_date, window_end_date: window_end_date) SyncJob.perform_later(new_sync) end diff --git a/app/models/sync.rb b/app/models/sync.rb index 134b18c3..cbe560d1 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -35,13 +35,6 @@ class Sync < ApplicationRecord end end - class << self - # By default, we sync the "visible" window of data (user sees 30 day graphs by default) - def create_with_defaults!(parent: nil) - create!(parent: parent, window_start_date: 30.days.ago.to_date) - end - end - def perform Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do start! diff --git a/app/views/accounts/show/_template.html.erb b/app/views/accounts/show/_template.html.erb index 51327254..efc7ea5a 100644 --- a/app/views/accounts/show/_template.html.erb +++ b/app/views/accounts/show/_template.html.erb @@ -2,7 +2,7 @@ <%= turbo_stream_from account %> -<%= turbo_frame_tag dom_id(account) do %> +<%= turbo_frame_tag dom_id(account, :container) do %> <%= tag.div class: "space-y-4 pb-32" do %> <% if header.present? %> <%= header %> -- 2.53.0 From a4eae6a9c726837ba0aa7614b90cd7e1857f5699 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 09:43:55 -0400 Subject: [PATCH 25/27] Sync broadcasters --- app/models/account/sync_complete_event.rb | 54 ++++++++++++++++++++ app/models/account/syncer.rb | 8 --- app/models/concerns/syncable.rb | 8 +++ app/models/family/sync_complete_event.rb | 21 ++++++++ app/models/family/syncer.rb | 1 - app/models/plaid_item/sync_complete_event.rb | 22 ++++++++ app/models/plaid_item/syncer.rb | 7 --- app/models/sync.rb | 5 ++ app/models/sync_complete_event.rb | 37 -------------- test/models/sync_test.rb | 11 ++++ 10 files changed, 121 insertions(+), 53 deletions(-) create mode 100644 app/models/account/sync_complete_event.rb create mode 100644 app/models/family/sync_complete_event.rb create mode 100644 app/models/plaid_item/sync_complete_event.rb delete mode 100644 app/models/sync_complete_event.rb diff --git a/app/models/account/sync_complete_event.rb b/app/models/account/sync_complete_event.rb new file mode 100644 index 00000000..3ef95507 --- /dev/null +++ b/app/models/account/sync_complete_event.rb @@ -0,0 +1,54 @@ +class Account::SyncCompleteEvent + attr_reader :account + + def initialize(account) + @account = account + end + + def broadcast + # Refresh entire account page (only applies if currently viewing this account) + account.broadcast_refresh + + # Replace account row in accounts list + account.broadcast_replace_to( + account.family, + target: "account_#{account.id}", + partial: "accounts/account", + locals: { account: account } + ) + + # Replace the groups this account belongs to in the sidebar + account_group_ids.each do |id| + account.broadcast_replace_to( + account.family, + target: id, + partial: "accounts/accountable_group", + locals: { account_group: account_group, open: true } + ) + end + + # If this is a manual, unlinked account (i.e. not part of a Plaid Item), + # trigger the family sync complete broadcast so net worth graph is updated + unless account.linked? + account.family.broadcast_sync_complete + end + 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 + id = account_group.id + [ id, "#{account_group.classification}_#{id}" ] + end + + def account_group + family_balance_sheet.account_groups.find do |group| + group.accounts.any? { |a| a.id == account.id } + end + end + + def family_balance_sheet + account.family.balance_sheet + end +end diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index 5554370b..7e5fabcd 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -12,14 +12,6 @@ class Account::Syncer def perform_post_sync account.family.auto_match_transfers! - account.broadcast_refresh - SyncCompleteEvent.new(account.family, accounts: [ account ]).broadcast - account.broadcast_replace_to( - account.family, - target: "account_#{account.id}", - partial: "accounts/account", - locals: { account: account } - ) end private diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 5223d587..8dfa8e41 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -22,6 +22,10 @@ module Syncable syncer.perform_post_sync end + def broadcast_sync_complete + sync_broadcaster.broadcast + end + def sync_error latest_sync&.error end @@ -42,4 +46,8 @@ module Syncable def syncer self.class::Syncer.new(self) end + + def sync_broadcaster + self.class::SyncCompleteEvent.new(self) + end end diff --git a/app/models/family/sync_complete_event.rb b/app/models/family/sync_complete_event.rb new file mode 100644 index 00000000..628841d0 --- /dev/null +++ b/app/models/family/sync_complete_event.rb @@ -0,0 +1,21 @@ +class Family::SyncCompleteEvent + attr_reader :family + + def initialize(family) + @family = family + end + + def broadcast + family.broadcast_replace( + target: "balance-sheet", + partial: "pages/dashboard/balance_sheet", + locals: { balance_sheet: family.balance_sheet } + ) + + family.broadcast_replace( + target: "net-worth-chart", + partial: "pages/dashboard/net_worth_chart", + locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days } + ) + end +end diff --git a/app/models/family/syncer.rb b/app/models/family/syncer.rb index ac06bce9..30ce2ad5 100644 --- a/app/models/family/syncer.rb +++ b/app/models/family/syncer.rb @@ -22,7 +22,6 @@ class Family::Syncer def perform_post_sync family.auto_match_transfers! - family.broadcast_refresh end private diff --git a/app/models/plaid_item/sync_complete_event.rb b/app/models/plaid_item/sync_complete_event.rb new file mode 100644 index 00000000..ca008a0a --- /dev/null +++ b/app/models/plaid_item/sync_complete_event.rb @@ -0,0 +1,22 @@ +class PlaidItem::SyncCompleteEvent + attr_reader :plaid_item + + def initialize(plaid_item) + @plaid_item = plaid_item + end + + def broadcast + plaid_item.accounts.each do |account| + account.broadcast_sync_complete + end + + plaid_item.broadcast_replace_to( + plaid_item.family, + target: "plaid_item_#{plaid_item.id}", + partial: "plaid_items/plaid_item", + locals: { plaid_item: plaid_item } + ) + + plaid_item.family.broadcast_sync_complete + end +end diff --git a/app/models/plaid_item/syncer.rb b/app/models/plaid_item/syncer.rb index 8baeed42..e32b9ed4 100644 --- a/app/models/plaid_item/syncer.rb +++ b/app/models/plaid_item/syncer.rb @@ -24,13 +24,6 @@ class PlaidItem::Syncer def perform_post_sync plaid_item.auto_match_categories! - SyncCompleteEvent.new(plaid_item.family, accounts: plaid_item.accounts).broadcast - plaid_item.broadcast_replace_to( - plaid_item.family, - target: "plaid_item_#{plaid_item.id}", - partial: "plaid_items/plaid_item", - locals: { plaid_item: plaid_item } - ) end private diff --git a/app/models/sync.rb b/app/models/sync.rb index cbe560d1..117e2d46 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -39,6 +39,8 @@ class Sync < ApplicationRecord Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do start! + sleep 10 + begin syncable.perform_sync(self) rescue => e @@ -89,8 +91,11 @@ class Sync < ApplicationRecord end def perform_post_sync + Rails.logger.info("Performing post-sync for #{syncable_type} (#{syncable.id})") syncable.perform_post_sync + syncable.broadcast_sync_complete rescue => e + Rails.logger.error("Error performing post-sync for #{syncable_type} (#{syncable.id}): #{e.message}") report_error(e) end diff --git a/app/models/sync_complete_event.rb b/app/models/sync_complete_event.rb deleted file mode 100644 index 44a17d37..00000000 --- a/app/models/sync_complete_event.rb +++ /dev/null @@ -1,37 +0,0 @@ -class SyncCompleteEvent - attr_reader :family, :accounts - - def initialize(family, accounts: []) - @family = family - @accounts = accounts - end - - def broadcast - account_groups = family.balance_sheet.account_groups.select do |group| - group.accounts.any? { |a| accounts.include?(a) } - end - - account_groups.each do |account_group| - [ nil, "asset", "liability" ].each do |classification| - id = classification ? "#{classification}_#{account_group.id}" : account_group.id - family.broadcast_replace( - target: id, - partial: "accounts/accountable_group", - locals: { account_group: account_group, open: true } - ) - end - end - - family.broadcast_replace( - target: "balance-sheet", - partial: "pages/dashboard/balance_sheet", - locals: { balance_sheet: family.balance_sheet } - ) - - family.broadcast_replace( - target: "net-worth-chart", - partial: "pages/dashboard/net_worth_chart", - locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days } - ) - end -end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index f9ca4144..cbab9ed3 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -63,8 +63,11 @@ class SyncTest < ActiveSupport::TestCase # Since these are accessed through `parent`, they won't necessarily be the same # instance we configured above Account.any_instance.expects(:perform_post_sync).once + Account.any_instance.expects(:broadcast_sync_complete).once PlaidItem.any_instance.expects(:perform_post_sync).once + PlaidItem.any_instance.expects(:broadcast_sync_complete).once Family.any_instance.expects(:perform_post_sync).once + Family.any_instance.expects(:broadcast_sync_complete).once account_sync.perform @@ -108,6 +111,10 @@ class SyncTest < ActiveSupport::TestCase PlaidItem.any_instance.expects(:perform_post_sync).once Family.any_instance.expects(:perform_post_sync).once + Account.any_instance.expects(:broadcast_sync_complete).once + PlaidItem.any_instance.expects(:broadcast_sync_complete).once + Family.any_instance.expects(:broadcast_sync_complete).once + account_sync.perform assert_equal "failed", plaid_item_sync.reload.status @@ -150,6 +157,10 @@ class SyncTest < ActiveSupport::TestCase PlaidItem.any_instance.expects(:perform_post_sync).once Family.any_instance.expects(:perform_post_sync).once + Account.any_instance.expects(:broadcast_sync_complete).once + PlaidItem.any_instance.expects(:broadcast_sync_complete).once + Family.any_instance.expects(:broadcast_sync_complete).once + account_sync.perform assert_equal "failed", plaid_item_sync.reload.status -- 2.53.0 From 9027647fa0a388e281fe14d948a081009f7acb12 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 09:58:30 -0400 Subject: [PATCH 26/27] Smoother account sync refreshes --- app/models/account/sync_complete_event.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/account/sync_complete_event.rb b/app/models/account/sync_complete_event.rb index 3ef95507..2a64ab80 100644 --- a/app/models/account/sync_complete_event.rb +++ b/app/models/account/sync_complete_event.rb @@ -6,9 +6,6 @@ class Account::SyncCompleteEvent end def broadcast - # Refresh entire account page (only applies if currently viewing this account) - account.broadcast_refresh - # Replace account row in accounts list account.broadcast_replace_to( account.family, @@ -32,6 +29,9 @@ class Account::SyncCompleteEvent unless account.linked? account.family.broadcast_sync_complete end + + # Refresh entire account page (only applies if currently viewing this account) + account.broadcast_refresh end private -- 2.53.0 From 237e05ae6f25d55181eca631a7e85bf61e43eeaf Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 15 May 2025 09:58:45 -0400 Subject: [PATCH 27/27] Remove test sync delay --- app/models/sync.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/sync.rb b/app/models/sync.rb index 117e2d46..f783bbc2 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -39,8 +39,6 @@ class Sync < ApplicationRecord Rails.logger.tagged("Sync", id, syncable_type, syncable_id) do start! - sleep 10 - begin syncable.perform_sync(self) rescue => e -- 2.53.0