[Ledger] Replace Event#merchants scan with a Ledger-scoped lookup#14356
Open
garyhtou wants to merge 3 commits into
Open
[Ledger] Replace Event#merchants scan with a Ledger-scoped lookup#14356garyhtou wants to merge 3 commits into
garyhtou wants to merge 3 commits into
Conversation
The ledger page's merchant filter (both the @merchant_name label in set_ledger_filters and the merchants_filter dropdown action) called Event#merchants, which loads every CanonicalTransaction and CanonicalPendingTransaction the event has ever had, N+1s each one's raw_stripe_transaction, and calls YellowPages::Merchant.lookup per transaction (not deduped) - all just to list distinct merchants. Rendering the Ledger should only need Ledger, Ledger::Item, Ledger::Mapping, and linked_object; this reached around all of that into the old transaction engine directly. Replaced with ledger_merchants(ledgers): starts from Ledger::Item scoped to the given ledgers (linked_object_type: "CardCharge"), then extracts just the `merchant_data` JSON key via Postgres's `->` operator instead of loading full RawStripeTransaction/ RawPendingStripeTransaction records - merchant_data is a small part of a multi-KB JSONB blob, and loading the whole blob per charge (~2.6KB average) to read one key was most of the remaining cost after fixing the N+1. Falls back from the latest settled transaction to the pending one per charge, matching CardCharge#merchant_data's own logic. The legacy (non-Ledger) Transactions page still uses Event#merchants directly and is unaffected. Benchmarked medians across several orgs (before -> after): - Hack Club HQ: 33,598ms -> 88ms (382x) - HCB Operations: 11,393ms -> 32ms (353x) - 4 other orgs (org 5516, 2924, 1692, 1362): 5,930-16,259ms -> 24-46ms (228-353x) The new method returns slightly fewer distinct merchants than the old one on long-running orgs (e.g. 2,187 vs 2,959 on Hack Club HQ) because it's scoped to CardCharge items that actually exist on the Ledger - some very old canonical_transactions predate the Ledger migration and were never backfilled with a corresponding Ledger::Item. Since the merchant filter isn't wired to actually filter query results yet (see the "TODO: add filtering for merchant" in ledger_query), this is a display-only list, and matching what's actually in the Ledger is more correct for a Ledger page than the old event-wide scan. spec/controllers/events_controller_spec.rb covers both call sites, including that another org's ledger doesn't leak into the merchant list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
garyhtou
marked this pull request as ready for review
July 19, 2026 01:26
…ontroller internals The merchant filter tests read state off the controller with instance_variable_get(:@merchant_name / :@merchants) after firing an unrelated action - testing internal state a real caller never sees, rather than the response the controller actually produces. A Rails-testing-focused review flagged this. Added render_views and switched to asserting on response.body: the merchant name appears in the active-filter badge (#ledger), and distinct merchant names appear in the dropdown, most-transacted-first (#merchants_filter, tested via rendered order since the count itself isn't displayed). Kept one direct assertion on ledger_merchants' return value for the exact per-merchant counts, since that's real grouping/counting logic worth pinning precisely, called directly rather than inferred from unrelated state. Verified the cross-ledger test is not vacuous: temporarily removed the ledger_mappings scoping from ledger_merchants and confirmed it fails, then restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR was written by Claude Code. Gary directed the investigation and reviewed the changes, but the diagnosis, code, and benchmarks below were produced by Claude.
Summary of the problem
The Ledger page's merchant filter — both the
@merchant_namelabel (set_ledger_filters.rb) and themerchants_filterdropdown action — calledEvent#merchants, which:CanonicalTransactionandCanonicalPendingTransactionthe event has ever had, with no date bound or limit,raw_stripe_transaction/raw_pending_stripe_transaction,YellowPages::Merchant.lookuponce per transaction rather than deduping first.Per Gary's rule for this system — rendering the Ledger should only touch
Ledger,Ledger::Item,Ledger::Mapping, andlinked_object— this also reached directly into the old transaction-engine tables, which the Ledger page shouldn't need to know about at all.Describe your changes
Replaced with a new
ledger_merchants(ledgers)helper that:Ledger::Itemscoped to the given ledgers, filtered tolinked_object_type: "CardCharge"— so it only ever reflects what's actually on this Ledger, vialinked_object(aCardCharge), never the raw transaction engine directly.merchant_datakey from the underlying JSONB blob using Postgres's->operator, instead of loading fullRawStripeTransaction/RawPendingStripeTransactionrecords.merchant_datais a small part of a multi-KB JSONB payload (~2.6KB average per row); loading the whole blob per charge just to read one key turned out to be most of the remaining cost after fixing the N+1.CardCharge#merchant_data's ownraw_stripe_transactions.last || raw_pending_stripe_transactionlogic.The legacy (non-Ledger) Transactions page still calls
Event#merchantsdirectly for its own filter menu and is untouched — that page isn't bound by the Ledger-only-access rule.Benchmarks
Medians, before → after, across a few orgs (public IDs shown for the rest):
org_lbu5qRorg_XDunKAorg_MNOuwZorg_a7XunbA disclosed behavior difference
The new method returns slightly fewer distinct merchants than the old one on long-running orgs — e.g. 2,187 vs 2,959 on Hack Club HQ. This is because it's scoped to
CardCharge-linkedLedger::Items that actually exist; some very oldcanonical_transactionspredate the Ledger migration and were never backfilled with a correspondingLedger::Item. Since the merchant filter isn't wired up to actually filter query results yet (# TODO: add filtering for merchantinledger_query), this list is display-only today, and I think matching what's actually navigable on the Ledger page is more correct than the old event-wide scan, not less — but flagging it explicitly since the numbers differ.Tests
spec/controllers/events_controller_spec.rbcovers both call sites through their actual rendered response (render_views+ asserting onresponse.body, not internal controller state): the merchant name resolving into the active-filter badge, the generic fallback when a merchant isn't found, distinct merchants rendering most-transacted-first, and a direct assertion onledger_merchants' return value for the exact per-merchant counts. The one I'd flag as most important given the scoping change: that another org's ledger doesn't leak into the merchant list.