fix(merchants): preserve manual merchant edits across provider sync

[dripsmvcp]

Summary

Fixes #1977.

Merging merchants, converting a synced (provider) merchant to a family merchant, and unlinking a merchant all reassign transactions.merchant_id in bulk — but never flag the affected entries as user_modified. The next provider sync sees those entries as unmodified and reverts the change, forcing the user to re-merge or fix each transaction again.

Root cause

Three bulk-update_all sites change merchant_id without protecting the entries:

• Merchant::Merger#merge! — family.transactions.where(merchant_id: source.id).update_all(merchant_id: target.id)
• ProviderMerchant#convert_to_family_merchant_for — same pattern
• ProviderMerchant#unlink_from_family — nulls merchant_id

The provider sync skip-guard (Account::ProviderImportAdapter#determine_skip_reason) only leaves an entry alone when it's excluded?, user_modified?, or import_locked?. Since none of these flows set user_modified, the merchant gets overwritten on the next sync.

Fix

Add Entry.mark_user_modified_for_transactions!(transaction_ids) and call it in all three flows before the merchant_id update (so the scope still matches the original merchant). Flagged entries are then skipped by the sync, so the manual change sticks.

• app/models/entry.rb — new bulk helper
• app/models/merchant/merger.rb — flag before reassign
• app/models/provider_merchant.rb — flag before convert and before unlink

No schema change; reuses the existing protection mechanism.

Test plan

• New model tests (test/models/merchant/merger_test.rb, test/models/provider_merchant_test.rb): merge / convert / unlink flag the affected entries as user_modified, and merge leaves unrelated transactions untouched.
• RED-verified — the three user_modified assertions fail on main (no flag set) and pass on this branch.
• 23 affected model + controller tests pass (merger, provider_merchant, provider_merchant/enhancer, family_merchants_controller, api/v1/merchants_controller).
• bin/rubocop -a clean; bin/brakeman --no-pager no warnings.

Before / after

After merging two merchants, the affected transaction is now marked Protected from sync, so a re-sync won't revert it.

Before (main) — merged transaction, no protection:
Before

After (this PR) — the merge marks the entry protected:
After

Summary by CodeRabbit

• Bug Fixes
  • Manual merchant reassignments and unlinks are now preserved during merges and provider operations; affected transactions are flagged so subsequent syncs won't revert them.
• Tests
  • Added tests confirming merges, conversions, and unlinks update transactions and flag associated records to maintain integrity across syncs.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f67836ca-f145-4475-bbc1-550f5854da66

📥 Commits

Reviewing files that changed from the base of the PR and between 11b7bd1 and d449ea3.

📒 Files selected for processing (3)

• app/models/entry.rb
• app/models/merchant/merger.rb
• app/models/provider_merchant.rb

---

📝 Walkthrough

Walkthrough

Adds Entry.mark_user_modified_for_transactions! to bulk-flag entries for transaction IDs, and calls it from Merchant::Merger and ProviderMerchant before reassigning or nulling transaction merchant_id; tests added to assert merchant_id changes and that entries are marked user_modified.

Changes

Merchant Modification Persistence

Layer / File(s)	Summary
Entry utility for marking transactions app/models/entry.rb	New class method mark_user_modified_for_transactions! normalizes relation or id-array input, returns early for empty sets, and bulk-updates transaction-backed entries to set user_modified: true.
Merchant merger with transaction marking app/models/merchant/merger.rb, test/models/merchant/merger_test.rb	Merchant::Merger#merge! scopes source merchant transactions, calls Entry.mark_user_modified_for_transactions! on that scope, and uses scope.update_all to reassign merchant_id. Tests verify reassigned transactions are updated and their entries are flagged; unrelated transactions remain unchanged.
Provider merchant conversion and unlinking app/models/provider_merchant.rb, test/models/provider_merchant_test.rb	ProviderMerchant#convert_to_family_merchant_for and unlink_from_family build a scoped relation for affected family transactions, call Entry.mark_user_modified_for_transactions! on that scope, and perform scope.update_all to set or null merchant_id. Tests assert entries are marked user_modified? after each operation.

Sequence Diagram

sequenceDiagram
  participant Merge
  participant Scope
  participant EntryUtil
  participant DB

  Merge->>Scope: build scope for affected transactions
  Scope-->>Merge: return scoped relation / ids
  Merge->>EntryUtil: call with scope or ids
  EntryUtil->>EntryUtil: normalize ids, dedupe, return if empty
  EntryUtil->>DB: bulk update entries where entryable_type='Transaction' and entryable_id IN (ids) to set user_modified: true
  DB-->>EntryUtil: updated entries
  Merge->>DB: scope.update_all(merchant_id: target / nil)
  DB-->>Merge: merchant_id reassigned / nulled

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• jjmata

Poem

🐰 I hopped through rows of merchant ties,
Flagged the entries so changes will prize.
No more syncs that sweep them away,
A tiny hop keeps edits in play.
Reassign, unlink — persistence today!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main purpose of the PR: fixing merchant edits to persist across provider syncs by marking entries as user_modified.
Linked Issues check	✅ Passed	The PR implements all coding requirements from issue #1977: adds Entry.mark_user_modified_for_transactions! helper and applies it in the three bulk operations (merge, convert_to_family_merchant_for, unlink_from_family) to preserve manual merchant edits across provider syncs.
Out of Scope Changes check	✅ Passed	All changes are directly related to the issue #1977 objective of protecting manual merchant modifications from provider sync overwrites. No unrelated or extraneous changes are present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[dripsmvcp]

@dosu

[dosubot]

This PR looks solid! The fix is clean and well-thought-out. A few observations:

Approach — The 3-step pattern (capture scope → flag entries → update merchant_id) is applied consistently across all three flows, and the ordering is correct: flagging must happen before the merchant_id update so the where(merchant_id: source.id) scope still matches [1].

Reuses existing infrastructure — Rather than introducing new schema or a new protection mechanism, the fix piggybacks on the existing user_modified flag that determine_skip_reason already respects [2]. That keeps the change minimal and predictable.

Entry.mark_user_modified_for_transactions! — The defensive Array().compact + early return is a nice touch for safety in bulk operations [3].

One thing to consider going forward: are there other bulk operations that reassign merchant_id (or other sync-sensitive fields) without setting user_modified? It might be worth auditing for similar patterns or extracting a shared concern so future flows get the protection automatically.

Overall this is a focused, well-tested fix that addresses a real data-integrity issue. Nice work! 🎉

To reply, just mention @dosu.

---

Docs are dead. Just use Dosu.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/models/entry.rb`:
- Around line 311-315: The current mark_user_modified_for_transactions! method
materializes transaction IDs into an Array causing memory/SQL-parameter issues;
change it to accept either an ActiveRecord::Relation or an enumerable and let
the DB perform selection: detect if the argument is a Relation (e.g.,
respond_to?(:arel) or is_a?(ActiveRecord::Relation)) and build the update as
where(entryable_type: "Transaction").where(entryable_id:
relation_or_subquery).update_all(user_modified: true), falling back to the
existing behavior for small enumerables only if necessary; update callers to
pass a relation/subquery (e.g.,
Entry.mark_user_modified_for_transactions!(scope) instead of scope.pluck(:id)).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 49bc17cb-8505-459a-8ddc-76ce399a3f1d

📥 Commits

Reviewing files that changed from the base of the PR and between 8f5454a and 11b7bd1.

📒 Files selected for processing (5)

• app/models/entry.rb
• app/models/merchant/merger.rb
• app/models/provider_merchant.rb
• test/models/merchant/merger_test.rb
• test/models/provider_merchant_test.rb

[jjmata]

The fix is well-designed and directly addresses the root cause. A few notes:

Order dependency is critical: The comments noting that mark_user_modified_for_transactions! must run before the merchant_id update are important. If the order were reversed, the WHERE clause merchant_id: source.id would match nothing (the ids have already been reassigned). Worth keeping those comments in place.

Relation vs. Array design: Accepting an ActiveRecord::Relation and running it as a subquery is the right call for large merge sets — avoids materializing a potentially large id list and sidesteps SQL parameter limits. The Array path is still useful for callers that need explicit ids. The return if ids.empty? guard prevents a no-op update_all on an empty set.

update_all skips updated_at: This is intentional for a bulk flag-setting operation and correct here — the sync-skip behavior keyed on user_modified doesn't depend on updated_at. Just worth noting for anyone wondering why timestamps don't change.

Test coverage: The "does not touch entries of unrelated merchants" test is valuable — it confirms the scope is tight and won't accidentally protect transactions that were already pointing at the target merchant before the merge. Red-verified before submit is noted in the PR description, which builds confidence.

---

Generated by Claude Code

[dripsmvcp]

Thanks for the thorough review! Quick clarification so this thread isn't left implying a pending change: the relation-accepting refactor you praised in note #2 is already in — commit d449ea3 made mark_user_modified_for_transactions! accept an ActiveRecord::Relation (subquery path) or an explicit id array, and the three callers now pass the scope relation directly. That commit also resolved CodeRabbit's "avoid array-materializing IDs" comment.

The order-dependency comments (note #1) are still in place in all three call sites, and the update_all/updated_at behavior (note #3) is intentional as you noted. So there's nothing outstanding here — my earlier "will fix it" was a reflex; the design is already as-described.