Merchants fixes

[sokie]

• Fix display issues on the settings -> merchants page.

  • The controller was using Current.family.merchants which only returns FamilyMerchant records (user-created merchants)
  • ProviderMerchant records (auto-created from Lunchflow, Plaid, SimpleFin) were being created correctly but not displayed

• SimpleFin had its own MerchantDetector class that directly called ProviderMerchant.find_or_create_by!

  • This was inconsistent with Plaid and Lunchflow which use the centralized import_adapter.find_or_create_merchant method
  • Different unique keys: SimpleFin used source + name, others used source + provider_merchant_id

Summary by CodeRabbit

• Bug Fixes
  • Improved merchant list display to include all merchants assigned to transactions.
  • Enhanced SimpleFin merchant import with improved payee-based detection and error handling.

[coderabbitai]

Walkthrough

Two files updated: the family merchants controller now displays merchants assigned to transactions rather than only family-owned merchants, and the SimpleFin entry processor implements payee-based merchant creation with deterministic MD5-generated IDs and error handling for invalid records.

Changes

Cohort / File(s)	Change Summary
Controller Updates app/controllers/family_merchants_controller.rb	Index action now retrieves merchants via Current.family.assigned_merchants.alphabetically instead of Current.family.merchants.alphabetically to display all merchants associated with transactions (both FamilyMerchant and ProviderMerchant).
Merchant Processing Logic app/models/simplefin_entry/processor.rb	Replaces merchant detection with payee-based merchant creation flow. Method merchant now strips and validates payee, creates/finds merchants via import adapter with generated MD5-based provider merchant IDs, and includes error handling for invalid records. Adds private helper generate_merchant_id(merchant_name) for deterministic ID generation and requires digest/md5.

Sequence Diagram(s)

sequenceDiagram
    participant Proc as SimpleFin<br/>Processor
    participant Payee as Payee Data
    participant Gen as ID Generator
    participant Adapter as Import<br/>Adapter
    participant DB as Database
    participant Log as Logger

    Proc->>Payee: strip & validate payee
    alt payee empty
        Proc->>Proc: return nil
    else payee present
        Proc->>Gen: generate_merchant_id<br/>(payee)
        Gen->>Gen: MD5 hash payee
        Gen-->>Proc: "simplefin_<hash>"
        Proc->>Adapter: find_or_create_merchant<br/>(id, payee, source)
        alt success
            Adapter->>DB: create/fetch<br/>ProviderMerchant
            DB-->>Adapter: merchant record
            Adapter-->>Proc: merchant
        else RecordInvalid
            Proc->>Log: error log
            Proc->>Proc: return nil
        end
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Verify the behavioral difference between assigned_merchants and merchants in the context of transaction display
• Confirm MD5-based ID generation is deterministic and handles edge cases (nil, empty, special characters in payee names)
• Check error handling logic in SimpleFin processor—ensure nil returns are handled gracefully upstream

Poem

🐰 A merchant tale in code so neat,
Payees now guide our payment fleet,
With MD5 hashes, IDs take form,
Assigned merchants weather every storm,
The SimpleFin rabbit hops along! 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check	❓ Inconclusive	The PR title "Merchants fixes" relates to the changeset's focus on merchant-related changes, but it uses vague and non-descriptive terminology that fails to convey the specific nature of the fixes. While the PR addresses important merchant issues—including a controller display bug where only family-created merchants were shown instead of all assigned merchants, and a SimpleFin import inconsistency in how merchants are created—the title provides no indication of these specifics. A teammate scanning the PR history would not understand the scope or nature of the changes from the title alone, as "fixes" is a generic term that could apply to many different types of merchant-related changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5eadfaa and c3b013e.

📒 Files selected for processing (2)

• app/controllers/family_merchants_controller.rb (1 hunks)
• app/models/simplefin_entry/processor.rb (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

app/models/**/*.rb

📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)

Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods

Use ActiveRecord validations for forms and complex domain constraints.

app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required

Files:

• app/models/simplefin_entry/processor.rb

app/**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/

Files:

• app/models/simplefin_entry/processor.rb
• app/controllers/family_merchants_controller.rb

**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules

Files:

• app/models/simplefin_entry/processor.rb
• app/controllers/family_merchants_controller.rb

**/*.{rb,js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.

Files:

• app/models/simplefin_entry/processor.rb
• app/controllers/family_merchants_controller.rb

app/{helpers,models}/**/*.rb

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Format currencies, numbers, and dates on the server side (Ruby) before sending to the client

Files:

• app/models/simplefin_entry/processor.rb

app/{models,controllers,views}/**/*.{rb,erb}

📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)

Avoid N+1 queries

Files:

• app/models/simplefin_entry/processor.rb
• app/controllers/family_merchants_controller.rb

🧠 Learnings (3)

📚 Learning: 2025-09-13T11:36:45.479Z

Learnt from: alessiocappa
PR: we-promise/sure#141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.

Applied to files:

• app/models/simplefin_entry/processor.rb

📚 Learning: 2025-09-13T11:36:45.479Z

Learnt from: alessiocappa
PR: we-promise/sure#141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.

Applied to files:

• app/models/simplefin_entry/processor.rb

📚 Learning: 2025-09-16T13:15:56.406Z

Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account.rb : Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated

Applied to files:

• app/models/simplefin_entry/processor.rb

🧬 Code graph analysis (2)

app/models/simplefin_entry/processor.rb (3)

app/models/plaid_entry/processor.rb (1)

• import_adapter (25-27)

app/models/account/provider_import_adapter.rb (1)

• find_or_create_merchant (66-77)

app/models/simplefin_account/transactions/merchant_detector.rb (1)

• generate_merchant_id (27-30)

app/controllers/family_merchants_controller.rb (1)

app/models/family.rb (1)

• assigned_merchants (42-45)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: ci / test

🔇 Additional comments (4)

app/models/simplefin_entry/processor.rb (3)

1-1: LGTM: Required for MD5-based merchant ID generation.

The require statement is necessary for the generate_merchant_id helper method and follows the existing pattern from the codebase.

---

104-117: Excellent: Centralized merchant creation aligned with Plaid/Lunchflow.

This implementation correctly addresses the PR objectives:

• Uses the centralized import_adapter.find_or_create_merchant instead of direct ProviderMerchant.find_or_create_by!
• Uses provider_merchant_id + source as the uniqueness key (consistent with Plaid and Lunchflow)
• Includes appropriate error handling for validation failures
• The memoization pattern is safe since processor instances are created per-transaction

---

119-122: LGTM: Deterministic merchant ID generation.

The MD5-based ID generation is appropriate and matches the existing pattern from SimplefinAccount::Transactions::MerchantDetector. The use of .downcase ensures case-insensitive merchant matching, which is the desired behavior.

app/controllers/family_merchants_controller.rb (1)

7-8: No N+1 query issue exists; original concern was based on incorrect assumption.

All attributes accessed in the view (logo_url, name, color) are simple database columns on the merchants table, not associations. There is no N+1 risk regardless of eager loading, since the query only fetches scalar values from a single table.

The original approval of the code change remains valid—the fix correctly displays both FamilyMerchant and ProviderMerchant records in the index view.

Likely an incorrect or invalid review comment.

---

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.}

[jjmata]

Is it worth adding some tests around this so merchants don't disappear again in the future?