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