From a67f36bf6439c7aeed7f9b40d227294d61b7b0bd Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 13:56:20 -0400 Subject: [PATCH 1/7] Prevent account deletions when account is linked to a Plaid Item (#2218) * Prevent account deletions when account is linked to a Plaid Item * Only guard deletions in UI and controller, not at model level --- .../concerns/accountable_resource.rb | 8 +++++-- app/helpers/application_helper.rb | 4 ---- app/models/plaid_account.rb | 21 ++++++++++++------- app/views/accounts/_form.html.erb | 5 ++++- app/views/accounts/show/_menu.html.erb | 20 ++++++++++-------- app/views/layouts/shared/_htmldoc.html.erb | 4 +++- .../shared/notifications/_alert.html.erb | 2 +- 7 files changed, 39 insertions(+), 25 deletions(-) diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 16fdbebd..1b2f0aa6 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -50,8 +50,12 @@ module AccountableResource end def destroy - @account.destroy_later - redirect_to accounts_path, notice: t("accounts.destroy.success", type: accountable_type.name.underscore.humanize) + if @account.linked? + redirect_to account_path(@account), alert: "Cannot delete a linked account" + else + @account.destroy_later + redirect_to accounts_path, notice: t("accounts.destroy.success", type: accountable_type.name.underscore.humanize) + end end private diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 999fd673..a86e374e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -45,10 +45,6 @@ module ApplicationHelper content_for(:header_description) { page_description } end - def family_stream - turbo_stream_from Current.family if Current.family - end - def page_active?(path) current_page?(path) || (request.path.start_with?(path) && path != "/") end diff --git a/app/models/plaid_account.rb b/app/models/plaid_account.rb index 65acf9ae..4f60013e 100644 --- a/app/models/plaid_account.rb +++ b/app/models/plaid_account.rb @@ -15,13 +15,20 @@ class PlaidAccount < ApplicationRecord class << self def find_or_create_from_plaid_data!(plaid_data, family) - find_or_create_by!(plaid_id: plaid_data.account_id) do |a| - a.account = family.accounts.new( - name: plaid_data.name, - balance: plaid_data.balances.current || plaid_data.balances.available, - currency: plaid_data.balances.iso_currency_code, - accountable: TYPE_MAPPING[plaid_data.type].new - ) + PlaidAccount.transaction do + plaid_account = find_or_create_by!(plaid_id: plaid_data.account_id) + + internal_account = family.accounts.find_or_initialize_by(plaid_account_id: plaid_account.id) + + internal_account.name = plaid_data.name + internal_account.balance = plaid_data.balances.current || plaid_data.balances.available + internal_account.currency = plaid_data.balances.iso_currency_code + internal_account.accountable = TYPE_MAPPING[plaid_data.type].new + + internal_account.save! + plaid_account.save! + + plaid_account end end end diff --git a/app/views/accounts/_form.html.erb b/app/views/accounts/_form.html.erb index 8cdb39ff..8a67d20f 100644 --- a/app/views/accounts/_form.html.erb +++ b/app/views/accounts/_form.html.erb @@ -6,7 +6,10 @@ <%= form.hidden_field :return_to, value: params[:return_to] %> <%= form.text_field :name, placeholder: t(".name_placeholder"), required: "required", label: t(".name_label") %> - <%= form.money_field :balance, label: t(".balance"), required: true, default_currency: Current.family.currency %> + + <% unless account.linked? %> + <%= form.money_field :balance, label: t(".balance"), required: true, default_currency: Current.family.currency %> + <% end %> <%= yield form %> diff --git a/app/views/accounts/show/_menu.html.erb b/app/views/accounts/show/_menu.html.erb index 56d78f62..0691f34e 100644 --- a/app/views/accounts/show/_menu.html.erb +++ b/app/views/accounts/show/_menu.html.erb @@ -13,13 +13,15 @@ ) %> <% end %> - <% menu.with_item( - variant: "button", - text: "Delete account", - href: account_path(account), - method: :delete, - icon: "trash-2", - confirm: CustomConfirm.for_resource_deletion("account", high_severity: true), - data: { turbo_frame: :_top } - ) %> + <% unless account.linked? %> + <% menu.with_item( + variant: "button", + text: "Delete account", + href: account_path(account), + method: :delete, + icon: "trash-2", + confirm: CustomConfirm.for_resource_deletion("account", high_severity: true), + data: { turbo_frame: :_top } + ) %> + <% end %> <% end %> diff --git a/app/views/layouts/shared/_htmldoc.html.erb b/app/views/layouts/shared/_htmldoc.html.erb index 9c3908c3..fadcd396 100644 --- a/app/views/layouts/shared/_htmldoc.html.erb +++ b/app/views/layouts/shared/_htmldoc.html.erb @@ -30,7 +30,9 @@ - <%= family_stream %> + <% if Current.family %> + <%= turbo_stream_from Current.family %> + <% end %> <%= turbo_frame_tag "modal" %> <%= turbo_frame_tag "drawer" %> diff --git a/app/views/shared/notifications/_alert.html.erb b/app/views/shared/notifications/_alert.html.erb index 06ccde60..d53793df 100644 --- a/app/views/shared/notifications/_alert.html.erb +++ b/app/views/shared/notifications/_alert.html.erb @@ -1,6 +1,6 @@ <%# locals: (message:) %> -<%= tag.div class: "flex gap-3 rounded-lg bg-container-inset p-4 group w-full md:max-w-80 shadow-border-lg", +<%= tag.div class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-lg", data: { controller: "element-removal" } do %>
-- 2.53.0 From 71be2a04add66e1fc86ca2de922039d1f8e8bcdb Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 14:10:56 -0400 Subject: [PATCH 2/7] Fix rule title reference --- app/models/rule/action_executor/auto_categorize.rb | 2 +- app/models/rule/action_executor/auto_detect_merchants.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/rule/action_executor/auto_categorize.rb b/app/models/rule/action_executor/auto_categorize.rb index 72ff324e..e1d8a50f 100644 --- a/app/models/rule/action_executor/auto_categorize.rb +++ b/app/models/rule/action_executor/auto_categorize.rb @@ -11,7 +11,7 @@ class Rule::ActionExecutor::AutoCategorize < Rule::ActionExecutor enrichable_transactions = transaction_scope.enrichable(:category_id) if enrichable_transactions.empty? - Rails.logger.info("No transactions to auto-categorize for #{rule.title} #{rule.id}") + Rails.logger.info("No transactions to auto-categorize for #{rule.id}") return end diff --git a/app/models/rule/action_executor/auto_detect_merchants.rb b/app/models/rule/action_executor/auto_detect_merchants.rb index cc523303..87c04682 100644 --- a/app/models/rule/action_executor/auto_detect_merchants.rb +++ b/app/models/rule/action_executor/auto_detect_merchants.rb @@ -11,7 +11,7 @@ class Rule::ActionExecutor::AutoDetectMerchants < Rule::ActionExecutor enrichable_transactions = transaction_scope.enrichable(:merchant_id) if enrichable_transactions.empty? - Rails.logger.info("No transactions to auto-detect merchants for #{rule.title} #{rule.id}") + Rails.logger.info("No transactions to auto-detect merchants for #{rule.id}") return end -- 2.53.0 From a07e9d40a3f3af51537c526738d93193ebde58db Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 16:28:58 -0400 Subject: [PATCH 3/7] Transactional locks for sync completions (#2219) * Transactional locks for sync completions * Lower sync display logic tolerance in UI --- app/models/family.rb | 6 +++--- app/models/sync.rb | 14 ++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/models/family.rb b/app/models/family.rb index ed919b11..3e877dc1 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -96,15 +96,15 @@ class Family < ApplicationRecord broadcast_refresh end - # If family has any syncs pending/syncing within the last hour, we show a persistent "syncing" notice. - # Ignore syncs older than 1 hour as they are considered "stale" + # 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: 1.hour.ago..).exists? + ).where(status: [ "pending", "syncing" ], created_at: 10.minutes.ago..).exists? end def eu? diff --git a/app/models/sync.rb b/app/models/sync.rb index 201fed02..da7b1a5c 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -41,10 +41,12 @@ class Sync < ApplicationRecord end def handle_child_completion_event - unless has_pending_child_syncs? - if has_failed_child_syncs? - fail!(Error.new("One or more child syncs failed")) - else + 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! + + unless has_pending_child_syncs? complete! syncable.post_sync(self) end @@ -56,10 +58,6 @@ class Sync < ApplicationRecord children.where(status: [ :pending, :syncing ]).any? end - def has_failed_child_syncs? - children.where(status: :failed).any? - end - def has_parent? parent_id.present? end -- 2.53.0 From 8b857e9c8a044d761f20393b4689966111641046 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 16:51:11 -0400 Subject: [PATCH 4/7] Notify parent sync in ensure block --- app/models/sync.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/models/sync.rb b/app/models/sync.rb index da7b1a5c..afd768ae 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -24,18 +24,19 @@ class Sync < ApplicationRecord complete! unless has_pending_child_syncs? - Rails.logger.info("Sync completed, starting post-sync") - - syncable.post_sync(self) unless has_pending_child_syncs? - - if has_parent? - notify_parent_of_completion! + unless has_pending_child_syncs? + Rails.logger.info("Sync completed, starting post-sync") + syncable.post_sync(self) + Rails.logger.info("Post-sync completed") end - - Rails.logger.info("Post-sync completed") rescue StandardError => error fail! error raise error if Rails.env.development? + ensure + if has_parent? + Rails.logger.info("notifying parent sync id=#{parent_id} of completion") + notify_parent_of_completion! + end end end end -- 2.53.0 From 2707a40a2a94be79dc2d0c2902f10409d49297a3 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 18:12:08 -0400 Subject: [PATCH 5/7] Handle nested child syncs (#2220) --- app/models/plaid_item.rb | 2 +- app/models/sync.rb | 12 ++++++------ test/models/sync_test.rb | 30 ++++++++++++++++++++++-------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index baab5104..0d0a6b1b 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -47,7 +47,7 @@ class PlaidItem < ApplicationRecord # Schedule account syncs accounts.each do |account| - account.sync_later(start_date: start_date) + account.sync_later(start_date: start_date, parent_sync: sync) end Rails.logger.info("Plaid data fetched and loaded") diff --git a/app/models/sync.rb b/app/models/sync.rb index afd768ae..0be26e5a 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -22,9 +22,8 @@ class Sync < ApplicationRecord data = syncable.sync_data(self, start_date: start_date) update!(data: data) if data - complete! unless has_pending_child_syncs? - unless has_pending_child_syncs? + complete! Rails.logger.info("Sync completed, starting post-sync") syncable.post_sync(self) Rails.logger.info("Post-sync completed") @@ -33,10 +32,7 @@ class Sync < ApplicationRecord fail! error raise error if Rails.env.development? ensure - if has_parent? - Rails.logger.info("notifying parent sync id=#{parent_id} of completion") - notify_parent_of_completion! - end + notify_parent_of_completion! if has_parent? end end end @@ -49,6 +45,10 @@ class Sync < ApplicationRecord unless has_pending_child_syncs? complete! + + # 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) end end diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index ec4482b6..6b246a1f 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -32,29 +32,43 @@ class SyncTest < ActiveSupport::TestCase 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 family = families(:dylan_family) parent = Sync.create!(syncable: family) child1 = Sync.create!(syncable: family.accounts.first, parent: parent) - child2 = Sync.create!(syncable: family.accounts.last, parent: parent) + 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 - parent.perform # no-op - - assert_equal "syncing", parent.status + assert_equal "pending", parent.status assert_equal "pending", child1.status assert_equal "pending", child2.status + assert_equal "pending", grandchild.status + + parent.perform + assert_equal "syncing", parent.reload.status child1.perform - assert_equal "completed", child1.status - assert_equal "syncing", parent.status + assert_equal "completed", child1.reload.status + assert_equal "syncing", parent.reload.status child2.perform - assert_equal "completed", child2.status - assert_equal "completed", parent.status + assert_equal "syncing", child2.reload.status + assert_equal "completed", child1.reload.status + assert_equal "syncing", parent.reload.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 end end -- 2.53.0 From ea1b6f2bd8a6592cae4a564bd435c67b55d08fa8 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 7 May 2025 22:19:09 -0400 Subject: [PATCH 6/7] Fix chart timezone bug (#2224) --- app/models/period.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/models/period.rb b/app/models/period.rb index 2fbcd30b..a1a4dea2 100644 --- a/app/models/period.rb +++ b/app/models/period.rb @@ -11,55 +11,55 @@ class Period PERIODS = { "last_day" => { - date_range: [ 1.day.ago.to_date, Date.current ], + date_range: -> { [ 1.day.ago.to_date, Date.current ] }, label_short: "1D", label: "Last Day", comparison_label: "vs. yesterday" }, "current_week" => { - date_range: [ Date.current.beginning_of_week, Date.current ], + date_range: -> { [ Date.current.beginning_of_week, Date.current ] }, label_short: "WTD", label: "Current Week", comparison_label: "vs. start of week" }, "last_7_days" => { - date_range: [ 7.days.ago.to_date, Date.current ], + date_range: -> { [ 7.days.ago.to_date, Date.current ] }, label_short: "7D", label: "Last 7 Days", comparison_label: "vs. last week" }, "current_month" => { - date_range: [ Date.current.beginning_of_month, Date.current ], + date_range: -> { [ Date.current.beginning_of_month, Date.current ] }, label_short: "MTD", label: "Current Month", comparison_label: "vs. start of month" }, "last_30_days" => { - date_range: [ 30.days.ago.to_date, Date.current ], + date_range: -> { [ 30.days.ago.to_date, Date.current ] }, label_short: "30D", label: "Last 30 Days", comparison_label: "vs. last month" }, "last_90_days" => { - date_range: [ 90.days.ago.to_date, Date.current ], + date_range: -> { [ 90.days.ago.to_date, Date.current ] }, label_short: "90D", label: "Last 90 Days", comparison_label: "vs. last quarter" }, "current_year" => { - date_range: [ Date.current.beginning_of_year, Date.current ], + date_range: -> { [ Date.current.beginning_of_year, Date.current ] }, label_short: "YTD", label: "Current Year", comparison_label: "vs. start of year" }, "last_365_days" => { - date_range: [ 365.days.ago.to_date, Date.current ], + date_range: -> { [ 365.days.ago.to_date, Date.current ] }, label_short: "365D", label: "Last 365 Days", comparison_label: "vs. 1 year ago" }, "last_5_years" => { - date_range: [ 5.years.ago.to_date, Date.current ], + date_range: -> { [ 5.years.ago.to_date, Date.current ] }, label_short: "5Y", label: "Last 5 Years", comparison_label: "vs. 5 years ago" @@ -72,7 +72,7 @@ class Period raise InvalidKeyError, "Invalid period key: #{key}" end - start_date, end_date = PERIODS[key].fetch(:date_range) + start_date, end_date = PERIODS[key].fetch(:date_range).call new(key: key, start_date: start_date, end_date: end_date) end -- 2.53.0 From 827934a953bf974709ae7eb19a101c6aac5e0369 Mon Sep 17 00:00:00 2001 From: hatz Date: Thu, 8 May 2025 07:59:52 -0500 Subject: [PATCH 7/7] Fix dark mode welcome screen for self-hosting --- app/views/registrations/new.html.erb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/registrations/new.html.erb b/app/views/registrations/new.html.erb index 7eb32066..6d63d0cb 100644 --- a/app/views/registrations/new.html.erb +++ b/app/views/registrations/new.html.erb @@ -3,9 +3,9 @@ %> <% if self_hosted_first_login? %> -
-

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

-

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

+
+

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

+

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

<% elsif @invitation %>
-- 2.53.0