feat(mercury): pending transactions, kind/counterpartyId metadata, and test coverage#2452
feat(mercury): pending transactions, kind/counterpartyId metadata, and test coverage#2452JSONbored wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesMercury Extra Metadata and Pending Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 203cc9fedf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/kraken_account/ledger_processor.rb`:
- Around line 33-35: The process_ledger_entry method is performing an exists?
database query for each ledger in the raw_ledgers.each loop, creating an N+1
query pattern. Before the loop that iterates through raw_ledgers, preload all
existing Kraken ledger external IDs into a Set or Hash for in-memory lookup.
Then replace the exists? query at line 79 with a lookup against this preloaded
collection instead of querying the database for each iteration.
In `@test/models/mercury_account/processor_test.rb`:
- Around line 49-56: The test method "returns nil without error when no linked
account" claims to verify a nil return value but only asserts that no exception
is raised using assert_nothing_raised. Capture the return value from the
MercuryAccount::Processor.new(mercury_account).process method call and add an
explicit assert_nil assertion to verify the return value is actually nil,
ensuring the test properly validates the return contract stated in the test
name.
🪄 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: 990bf48d-3549-4b54-ad18-2cb47b1d9695
📒 Files selected for processing (12)
app/models/kraken_account/ledger_processor.rbapp/models/kraken_account/processor.rbapp/models/kraken_item/importer.rbapp/models/mercury_entry/processor.rbapp/models/provider/kraken.rbapp/models/transaction.rbtest/models/kraken_account/ledger_processor_test.rbtest/models/kraken_item/importer_test.rbtest/models/mercury_account/processor_test.rbtest/models/mercury_entry/processor_test.rbtest/models/mercury_item/importer_test.rbtest/models/provider/kraken_test.rb
There was a problem hiding this comment.
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/mercury_item/importer.rb`:
- Around line 240-243: Replace the two Rails.logger.info calls in the
MercuryItem::Importer with DebugLogEntry.capture calls. The first call logs
transaction counts when storing new and updated transactions, and the second
logs when there are no new or updated transactions. Update both log statements
to use DebugLogEntry.capture with the same message content to ensure these
import diagnostics are properly captured in the support debug UI as per the
coding guidelines for provider sync/import diagnostics.
🪄 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: c55dff3a-22bf-49be-b586-67abf4a02810
📒 Files selected for processing (4)
app/models/account/provider_import_adapter.rbapp/models/kraken_account/ledger_processor.rbapp/models/mercury_item/importer.rbtest/models/mercury_account/processor_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/models/mercury_account/processor_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/kraken_account/ledger_processor.rb
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
The Mercury additions are well thought out — especially the pending-to-posted transition logic that handles Mercury reusing the same transaction ID when a pending entry posts. The existing_by_id map approach is clean. A few observations:
Overlap with PR #2451 — This PR's diff includes KrakenAccount::LedgerProcessor, KrakenItem::Importer ledger fetching, and Provider::Kraken#get_ledgers — the same files changed in PR #2451. Importantly, this version of LedgerProcessor#process preloads existing external IDs into a Set upfront:
existing_external_ids = account.entries
.where(source: "kraken")
.where("external_id LIKE 'kraken_ledger_%'")
.pluck(:external_id)
.to_set…whereas PR #2451's version does a per-entry account.entries.exists?(...) call — one DB query per ledger entry. This PR's version is better for performance. Whichever order these merge, the per-entry query version shouldn't land. The two PRs need to be coordinated.
DebugLogEntry over Rails.logger for Mercury importer — The old importer used Rails.logger.info for transaction storage; this PR upgrades to DebugLogEntry.capture, which is consistent with the project's debug logging guidelines. Good.
extra["mercury"]["kind"] vs. Sure transaction kinds — The Mercury kind field (e.g., "externalTransfer") is stored as raw metadata in extra, not mapped to Sure's Transaction#kind enum. That's the safe choice for now, but a future follow-up could map known Mercury kinds to funds_movement where appropriate (similar to what PR #2460 does for Up's transferAccount).
Generated by Claude Code
Address review feedback (jjmata): - N+1: LedgerProcessor#process_ledger_entry ran `account.entries.exists?(...)` per ledger entry (up to ~10k per sync). Load the existing Kraken external IDs once into a Set and test membership in memory (newly created IDs are added so the same run stays idempotent) — same pattern as we-promise#2452. - Tests: the idempotency test now asserts the first pass actually creates the entry (assert_difference) before asserting the second is a no-op; add a guard asserting the second (all-skipped) pass issues a single bulk external_id pluck, not one query per entry. No behavior change to imported entries.
…d test coverage
- Transaction::PENDING_PROVIDERS: add "mercury" so the existing pending
reconciliation pipeline (pending→posted amount matching in
ProviderImportAdapter) activates for Mercury entries
- MercuryEntry::Processor: pass extra: to import_transaction with
extra["mercury"]["pending"] = true/false (status == "pending")
extra["mercury"]["kind"] = ACH / Wire / Card / etc.
extra["mercury"]["counterparty_id"] = Mercury counterparty UUID
Pending transactions are now imported with the flag set rather than
being silently ignored; failed transactions continue to be skipped
- Tests (new files, 30 cases):
- test/models/mercury_entry/processor_test.rb — sign convention,
date fallback, name priority, notes concat, pending flag, kind,
counterpartyId, failed skip, idempotency, merchant creation,
no-linked-account guard
- test/models/mercury_item/importer_test.rb — account discovery,
no-duplicate unlinked records, balance update on linked accounts,
transaction dedup (append-only new ids), sync window (90-day first
sync, last_synced_at-7d subsequent), 401 marks requires_update
- test/models/mercury_account/processor_test.rb — balance update,
CreditCard sign negation, cash_balance parity, no-linked-account
no-op, transaction processing delegation
…1, nil assertion - provider_import_adapter: add mercury to all three find_pending_transaction* SQL predicates so Mercury pending entries are found and claimed when the posted version arrives (exact, fuzzy, and low-confidence paths) - mercury_item/importer: replace append-only dedup with an upsert-by-id that replaces the stored raw payload when status changes from pending to non-pending; prevents pending flag from persisting indefinitely when Mercury reuses the same transaction ID for the posted version - kraken_account/ledger_processor: preload all existing kraken ledger external_ids into a Set before the loop; replaces per-entry exists? query (N+1) with an in-memory Set#include? lookup - test/models/mercury_account/processor_test: capture return value from process and add assert_nil to enforce the nil contract stated in the test name
JSONbored
force-pushed
the
feat/mercury-sync-enhancements
branch
from
cb16c6b to
a828f3e
Compare
|
Thanks @jjmata — good catch on the overlap. Fixed: Overlap with #2451 (resolved): this branch was accidentally stacked on #2451 — it carried the three Kraken commits plus a stray edit to The good Set-based idempotency you flagged now lives solely in #2451, which I updated to the same approach (single bulk pluck scoped to the DebugLogEntry: 👍 thanks.
|
2 participants
Closes #2463.
Summary
Mercury Banking sync currently misses three data points available in every Mercury API payload:
status: "pending"entries were never imported, so Mercury pending charges don't appear until they postkind) is not stored — Mercury returnskind: "ACH" | "Wire" | "Card" | "InternalTransfer"on every transaction; the field was unusedcounterpartyId(a stable UUID) for deduplication and future analytics; the field was unusedTransaction::PENDING_PROVIDERSwas missing"mercury"— even if pending data had been stored, the existing pending-detection SQL andTransaction#pending?would never check itChanges
app/models/transaction.rbAdd
"mercury"toPENDING_PROVIDERSso the existing reconciliation pipeline (pending→posted amount-match inAccount::ProviderImportAdapter) activates for Mercury entries.app/models/mercury_entry/processor.rbPass
extra:toimport_transactionwith the following structure:Pending transactions are now imported with the flag set. Only
"failed"status continues to be skipped.New test files — 30 cases
test/models/mercury_entry/processor_test.rb— sign convention, date fallback (postedAt→createdAt), name priority (nickname → name → bankDescription), notes concatenation, pending flag, kind, counterpartyId, failed-skip, idempotency, merchant creation, no-linked-account guardtest/models/mercury_item/importer_test.rb— account discovery, no-duplicate unlinked records, balance update on linked accounts, transaction dedup (append-only new ids), sync window (90-day first sync,last_synced_at - 7dfor subsequent syncs), 401 marksrequires_updatetest/models/mercury_account/processor_test.rb— balance update, CreditCard sign negation,cash_balanceparity, no-linked-account no-op, transaction processing delegationTest run
Summary by CodeRabbit
New Features
Bug Fixes
Tests