From 137219c121fdd31bb171d307ec364524c4d6f71a Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 19 May 2025 16:39:31 -0400 Subject: [PATCH] Fix attribute locking namespace conflict, duplicate syncs --- app/controllers/transactions_controller.rb | 4 ++-- app/models/concerns/enrichable.rb | 6 ++--- app/models/concerns/syncable.rb | 26 ++++++++++++++------- app/models/entry.rb | 2 +- app/models/family/auto_categorizer.rb | 2 +- app/models/family/auto_merchant_detector.rb | 2 +- app/models/sync.rb | 23 ++++++++++++++++++ test/controllers/concerns/auto_sync_test.rb | 4 ++-- test/interfaces/syncable_interface_test.rb | 18 ++++++++++---- test/models/rule/action_test.rb | 8 +++---- test/models/sync_test.rb | 20 ++++++++++++++++ 11 files changed, 87 insertions(+), 28 deletions(-) diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index 4d47c06b..e5382e73 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -61,7 +61,7 @@ class TransactionsController < ApplicationController if @entry.save @entry.sync_account_later @entry.lock_saved_attributes! - @entry.transaction.lock!(:tag_ids) if @entry.transaction.tags.any? + @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any? flash[:notice] = "Transaction created" @@ -88,7 +88,7 @@ class TransactionsController < ApplicationController @entry.sync_account_later @entry.lock_saved_attributes! - @entry.transaction.lock!(:tag_ids) if @entry.transaction.tags.any? + @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any? respond_to do |format| format.html { redirect_back_or_to account_path(@entry.account), notice: "Transaction updated" } diff --git a/app/models/concerns/enrichable.rb b/app/models/concerns/enrichable.rb index e5804786..febc358f 100644 --- a/app/models/concerns/enrichable.rb +++ b/app/models/concerns/enrichable.rb @@ -42,17 +42,17 @@ module Enrichable !locked?(attr) end - def lock!(attr) + def lock_attr!(attr) update!(locked_attributes: locked_attributes.merge(attr.to_s => Time.current)) end - def unlock!(attr) + def unlock_attr!(attr) update!(locked_attributes: locked_attributes.except(attr.to_s)) end def lock_saved_attributes! saved_changes.keys.reject { |attr| ignored_enrichable_attributes.include?(attr) }.each do |attr| - lock!(attr) + lock_attr!(attr) end end diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 6b0ba684..72556bf7 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -9,20 +9,28 @@ module Syncable raise NotImplementedError, "Subclasses must implement the syncing? method" end + # Schedules a sync for syncable. If there is an existing sync pending/syncing for this syncable, + # we do not create a new sync, and attempt to expand the sync window if needed. def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil) Sync.transaction do - # Since we're scheduling a new sync, mark old syncs for this syncable as stale - self.syncs.incomplete.find_each(&:mark_stale!) + with_lock do + sync = self.syncs.incomplete.first - new_sync = self.syncs.create!( - parent: parent_sync, - window_start_date: window_start_date, - window_end_date: window_end_date - ) + if sync + Rails.logger.info("There is an existing sync, expanding window if needed (#{sync.id})") + sync.expand_window_if_needed(window_start_date, window_end_date) + else + sync = self.syncs.create!( + parent: parent_sync, + window_start_date: window_start_date, + window_end_date: window_end_date + ) - SyncJob.perform_later(new_sync) + SyncJob.perform_later(sync) + end - new_sync + sync + end end end diff --git a/app/models/entry.rb b/app/models/entry.rb index 36f61c29..5b14987a 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -85,7 +85,7 @@ class Entry < ApplicationRecord entry.update! bulk_attributes entry.lock_saved_attributes! - entry.entryable.lock!(:tag_ids) if entry.transaction? && entry.transaction.tags.any? + entry.entryable.lock_attr!(:tag_ids) if entry.transaction? && entry.transaction.tags.any? end end diff --git a/app/models/family/auto_categorizer.rb b/app/models/family/auto_categorizer.rb index c35aa3b9..038be1d8 100644 --- a/app/models/family/auto_categorizer.rb +++ b/app/models/family/auto_categorizer.rb @@ -27,7 +27,7 @@ class Family::AutoCategorizer end scope.each do |transaction| - transaction.lock!(:category_id) + transaction.lock_attr!(:category_id) auto_categorization = result.data.find { |c| c.transaction_id == transaction.id } diff --git a/app/models/family/auto_merchant_detector.rb b/app/models/family/auto_merchant_detector.rb index 4b791e7a..39ddcd18 100644 --- a/app/models/family/auto_merchant_detector.rb +++ b/app/models/family/auto_merchant_detector.rb @@ -27,7 +27,7 @@ class Family::AutoMerchantDetector end scope.each do |transaction| - transaction.lock!(:merchant_id) + transaction.lock_attr!(:merchant_id) auto_detection = result.data.find { |c| c.transaction_id == transaction.id } diff --git a/app/models/sync.rb b/app/models/sync.rb index aa40b313..08b2f842 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -95,6 +95,29 @@ class Sync < ApplicationRecord parent&.finalize_if_all_children_finalized end + # If a sync is pending, we can adjust the window if new syncs are created with a wider window. + def expand_window_if_needed(new_window_start_date, new_window_end_date) + return unless pending? + return if self.window_start_date.nil? && self.window_end_date.nil? # already as wide as possible + + earliest_start_date = if self.window_start_date && new_window_start_date + [self.window_start_date, new_window_start_date].min + else + nil + end + + latest_end_date = if self.window_end_date && new_window_end_date + [self.window_end_date, new_window_end_date].max + else + nil + end + + update( + window_start_date: earliest_start_date, + window_end_date: latest_end_date + ) + end + private def log_status_change Rails.logger.info("changing from #{aasm.from_state} to #{aasm.to_state} (event: #{aasm.current_event})") diff --git a/test/controllers/concerns/auto_sync_test.rb b/test/controllers/concerns/auto_sync_test.rb index 462850f8..0ae19ab1 100644 --- a/test/controllers/concerns/auto_sync_test.rb +++ b/test/controllers/concerns/auto_sync_test.rb @@ -20,7 +20,7 @@ class AutoSyncTest < ActionDispatch::IntegrationTest travel_to Time.current.beginning_of_day last_sync_datetime = 1.hour.ago - Sync.create!(syncable: @family, created_at: last_sync_datetime) + Sync.create!(syncable: @family, created_at: last_sync_datetime, status: "completed") assert_difference "Sync.count", 1 do get root_path @@ -32,7 +32,7 @@ class AutoSyncTest < ActionDispatch::IntegrationTest last_created_sync_at = 23.hours.ago - Sync.create!(syncable: @family, created_at: last_created_sync_at) + Sync.create!(syncable: @family, created_at: last_created_sync_at, status: "completed") assert_no_difference "Sync.count" do get root_path diff --git a/test/interfaces/syncable_interface_test.rb b/test/interfaces/syncable_interface_test.rb index 95f6789d..df142052 100644 --- a/test/interfaces/syncable_interface_test.rb +++ b/test/interfaces/syncable_interface_test.rb @@ -18,11 +18,19 @@ module SyncableInterfaceTest @syncable.perform_sync(mock_sync) end - test "any prior syncs for the same syncable entity are marked stale when new sync is requested" do - stale_sync = @syncable.sync_later - new_sync = @syncable.sync_later + test "second sync request widens existing pending window" do + later_start = 2.days.ago.to_date + first_sync = @syncable.sync_later(window_start_date: later_start, window_end_date: later_start) - assert_equal "stale", stale_sync.reload.status - assert_equal "pending", new_sync.reload.status + earlier_start = 5.days.ago.to_date + wider_end = Date.current + + assert_no_difference "@syncable.syncs.count" do + @syncable.sync_later(window_start_date: earlier_start, window_end_date: wider_end) + end + + first_sync.reload + assert_equal earlier_start, first_sync.window_start_date + assert_equal wider_end, first_sync.window_end_date end end diff --git a/test/models/rule/action_test.rb b/test/models/rule/action_test.rb index 624849fc..f71bb2cb 100644 --- a/test/models/rule/action_test.rb +++ b/test/models/rule/action_test.rb @@ -21,7 +21,7 @@ class Rule::ActionTest < ActiveSupport::TestCase test "set_transaction_category" do # Does not modify transactions that are locked (user edited them) - @txn1.lock!(:category_id) + @txn1.lock_attr!(:category_id) action = Rule::Action.new( rule: @transaction_rule, @@ -42,7 +42,7 @@ class Rule::ActionTest < ActiveSupport::TestCase tag = @family.tags.create!(name: "Rule test tag") # Does not modify transactions that are locked (user edited them) - @txn1.lock!(:tag_ids) + @txn1.lock_attr!(:tag_ids) action = Rule::Action.new( rule: @transaction_rule, @@ -63,7 +63,7 @@ class Rule::ActionTest < ActiveSupport::TestCase merchant = @family.merchants.create!(name: "Rule test merchant") # Does not modify transactions that are locked (user edited them) - @txn1.lock!(:merchant_id) + @txn1.lock_attr!(:merchant_id) action = Rule::Action.new( rule: @transaction_rule, @@ -84,7 +84,7 @@ class Rule::ActionTest < ActiveSupport::TestCase new_name = "Renamed Transaction" # Does not modify transactions that are locked (user edited them) - @txn1.lock!(:name) + @txn1.lock_attr!(:name) action = Rule::Action.new( rule: @transaction_rule, diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 09266bb5..05765ea0 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -188,4 +188,24 @@ class SyncTest < ActiveSupport::TestCase assert_equal "stale", stale_pending.reload.status assert_equal "stale", stale_syncing.reload.status end + + test "expand_window_if_needed widens start and end dates on a pending sync" do + initial_start = 1.day.ago.to_date + initial_end = 1.day.ago.to_date + + sync = Sync.create!( + syncable: accounts(:depository), + window_start_date: initial_start, + window_end_date: initial_end + ) + + new_start = 5.days.ago.to_date + new_end = Date.current + + sync.expand_window_if_needed(new_start, new_end) + sync.reload + + assert_equal new_start, sync.window_start_date + assert_equal new_end, sync.window_end_date + end end