Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions app/models/entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>] 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.
#
Expand Down
9 changes: 8 additions & 1 deletion app/models/merchant/merger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 14 additions & 2 deletions app/models/provider_merchant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
35 changes: 35 additions & 0 deletions test/models/merchant/merger_test.rb
Original file line number Diff line number Diff line change
@@ -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
37 changes: 37 additions & 0 deletions test/models/provider_merchant_test.rb
Original file line number Diff line number Diff line change
@@ -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
Loading