diff --git a/app/models/entry.rb b/app/models/entry.rb index 84c2719f2e..1599f4ad47 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -301,6 +301,30 @@ def protected_from_sync? excluded? || user_modified? || import_locked? end + # Bulk-marks the entries of the given transactions as user-modified so a + # later provider sync won't overwrite them (issue #1977). Used by merchant + # merge/convert/unlink flows, which reassign merchant_id directly on + # transactions and must protect that manual change from being reverted. + # + # Accepts a Transaction relation (preferred — the selection runs as a + # subquery so large merges/unlinks don't materialize ids or hit SQL + # parameter limits) or an explicit array of ids. + # + # @param transactions [ActiveRecord::Relation, Array] Transactions or their ids + # @return [void] + def self.mark_user_modified_for_transactions!(transactions) + entryable_ids = + if transactions.is_a?(ActiveRecord::Relation) + transactions.select(:id) + else + ids = Array(transactions).compact.uniq + return if ids.empty? + ids + end + + where(entryable_type: "Transaction", entryable_id: entryable_ids).update_all(user_modified: true) + end + # Marks entry as user-modified after manual edit. # Called when user edits any field to prevent provider sync from overwriting. # diff --git a/app/models/merchant/merger.rb b/app/models/merchant/merger.rb index 7baa78e678..4c57e4844b 100644 --- a/app/models/merchant/merger.rb +++ b/app/models/merchant/merger.rb @@ -39,8 +39,15 @@ def merge! Merchant.transaction do source_merchants.each do |source| + scope = family.transactions.where(merchant_id: source.id) + + # Protect the manual reassignment from being reverted on the next + # provider sync (issue #1977). Must run before the merchant_id update + # so the scope still matches the source merchant. + Entry.mark_user_modified_for_transactions!(scope) + # Reassign family's transactions to target - family.transactions.where(merchant_id: source.id).update_all(merchant_id: target_merchant.id) + scope.update_all(merchant_id: target_merchant.id) # Delete FamilyMerchant, keep ProviderMerchant (it may be used by other families) source.destroy! if source.is_a?(FamilyMerchant) diff --git a/app/models/provider_merchant.rb b/app/models/provider_merchant.rb index 5cfb2fdf9d..fbfba77353 100644 --- a/app/models/provider_merchant.rb +++ b/app/models/provider_merchant.rb @@ -15,8 +15,14 @@ def convert_to_family_merchant_for(family, attributes = {}) website_url: attributes[:website_url].presence || website_url ) + scope = family.transactions.where(merchant_id: id) + + # Protect the manual reassignment from being reverted on the next + # provider sync (issue #1977). Must run before the merchant_id update. + Entry.mark_user_modified_for_transactions!(scope) + # Update only this family's transactions to point to new merchant - family.transactions.where(merchant_id: id).update_all(merchant_id: family_merchant.id) + scope.update_all(merchant_id: family_merchant.id) family_merchant end @@ -37,7 +43,13 @@ def generate_logo_url_from_website! # Does NOT delete the ProviderMerchant since it may be used by other families. # Tracks the unlink in FamilyMerchantAssociation so it shows as "recently unlinked". def unlink_from_family(family) - family.transactions.where(merchant_id: id).update_all(merchant_id: nil) + scope = family.transactions.where(merchant_id: id) + + # Protect the manual unlink from being reverted on the next provider sync + # (issue #1977). Must run before the merchant_id is nulled. + Entry.mark_user_modified_for_transactions!(scope) + + scope.update_all(merchant_id: nil) # Track that this merchant was unlinked from this family association = FamilyMerchantAssociation.find_or_initialize_by(family: family, merchant: self) diff --git a/test/models/merchant/merger_test.rb b/test/models/merchant/merger_test.rb new file mode 100644 index 0000000000..cc50b4950b --- /dev/null +++ b/test/models/merchant/merger_test.rb @@ -0,0 +1,35 @@ +require "test_helper" + +class Merchant::MergerTest < ActiveSupport::TestCase + include EntriesTestHelper + + setup do + @family = families(:dylan_family) + @target = merchants(:netflix) + @source = merchants(:amazon) + end + + # Regression: issue #1977. Merging merchants reassigns merchant_id via + # update_all; without flagging the entries as user_modified, the next + # provider sync reverts the merge. + test "merge! flags reassigned transactions as user_modified" do + entry = create_transaction(merchant: @source) + assert_not entry.user_modified? + + Merchant::Merger.new(family: @family, target_merchant: @target, source_merchants: [ @source ]).merge! + + entry.reload + assert_equal @target.id, entry.entryable.merchant_id, "transaction is reassigned to the target merchant" + assert entry.user_modified?, "merged transaction's entry must be flagged so provider sync won't revert it" + end + + test "merge! does not touch entries of unrelated merchants" do + other = create_transaction(merchant: @target) + assert_not other.user_modified? + + Merchant::Merger.new(family: @family, target_merchant: @target, source_merchants: [ @source ]).merge! + + other.reload + assert_not other.user_modified?, "transactions already on the target are untouched" + end +end diff --git a/test/models/provider_merchant_test.rb b/test/models/provider_merchant_test.rb new file mode 100644 index 0000000000..0f15aeaa18 --- /dev/null +++ b/test/models/provider_merchant_test.rb @@ -0,0 +1,37 @@ +require "test_helper" + +class ProviderMerchantTest < ActiveSupport::TestCase + include EntriesTestHelper + + setup do + @family = families(:dylan_family) + @provider_merchant = ProviderMerchant.create!(name: "Acme Synced", source: "plaid") + end + + # Regression: issue #1977. Converting a synced merchant to a family merchant + # reassigns merchant_id via update_all; the entries must be flagged so the + # next provider sync doesn't revert the conversion. + test "convert_to_family_merchant_for flags reassigned transactions as user_modified" do + entry = create_transaction(merchant: @provider_merchant) + assert_not entry.user_modified? + + family_merchant = @provider_merchant.convert_to_family_merchant_for(@family) + + entry.reload + assert_equal family_merchant.id, entry.entryable.merchant_id + assert entry.user_modified?, "converted transaction's entry must be flagged so provider sync won't revert it" + end + + # Regression: issue #1977. Unlinking a synced merchant nulls merchant_id; + # without the flag the next sync re-links it. + test "unlink_from_family flags affected transactions as user_modified" do + entry = create_transaction(merchant: @provider_merchant) + assert_not entry.user_modified? + + @provider_merchant.unlink_from_family(@family) + + entry.reload + assert_nil entry.entryable.merchant_id + assert entry.user_modified?, "unlinked transaction's entry must be flagged so provider sync won't re-link it" + end +end