[Ledger::Item] Route all custom_memo writes through HcbCode#update_custom_memo! - #14536
Open
garyhtou wants to merge 4 commits into
Open
[Ledger::Item] Route all custom_memo writes through HcbCode#update_custom_memo!#14536garyhtou wants to merge 4 commits into
garyhtou wants to merge 4 commits into
Conversation
`custom_memo` lives on canonical transactions, canonical pending transactions and ledger items. `Ledger::Item#memo` — what the ledger renders and searches — is cached in `refresh!` from the ledger item's own copy, so a write that only lands on a canonical transaction desyncs it. Linked-object transactions then fall back to their system memo and the rename is silently discarded; `refresh!` cannot recover it. Raw bank transactions have no system memo, so `fallback_memo` surfaces the canonical transaction's copy and they only look correct. Route the two organizer rename controllers, the pending-to-settled memo copy and the nightly Stripe card rename through `HcbCode#update_custom_memo!`. Settling only copies a memo that exists — routing a nil through it would clear the memo on every record in the group. Also fix `HcbCode#update_custom_memo!` to resolve ledger items through its transactions: `hcb_codes.ledger_item_id` is only back-linked when the item is created, so it is null for any transaction that adopted an existing one, and the method fell back to writing the canonical transactions directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126DFcNZ5ZECDEij5J4zu2p
Review caught a case the previous commit made worse: when the HCB code has no ledger item back-link *and* one of its transactions has no `ledger_item_id`, resolving through the ledger items and returning early meant that transaction was never written. Previously the fallback wrote every canonical transaction, so this traded a stale ledger item for a stale canonical transaction. Write the canonical transactions, the pending transactions and every ledger item they resolve to, unconditionally. This also drops the duplicate writes that came from looping ledger items whose HCB code covered the whole group anyway, and lets `Ledger::Item#update_custom_memo!` go back to owning just its own transactions. Also guard the nightly Stripe card rename against a missing HCB code — it was the one call site dereferencing `local_hcb_code` without a nil check, and `safely` would have swallowed the error and retried it every night forever. Restore the settle spec's "canonical transaction has a custom_memo" context to its original parent; it had been left under a context where the pending transaction's memo is nil, making its assertion vacuous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126DFcNZ5ZECDEij5J4zu2p
Settle wrote to the wrong ledger item. CanonicalPendingSettledMapping hands the canonical transaction over to the *pending* transaction's ledger item after this transaction commits, and several settle paths pair a pending transaction with a still-ungrouped HCB-000 canonical transaction (PendingEventMappingEngine::Settle::Donation, AdminController#set_wire), so the two HCB codes routinely differ and the destination item never got the memo. Write it there too. The nightly Stripe card rename clobbered sibling memos. Renaming writes the whole HCB code group while `without_custom_memo` only filters on one transaction, so a group holding an organizer's memo got stamped with the default. Skip any group where a transaction already carries one — `HcbCode#custom_memo` only reads the first transaction, so check them all. Blanking the rename form wiped the group. Both forms prefilled from the record's own column, which is empty for transactions renamed before the group write existed; submitting that blank field cleared every record in the group. Prefill from the HCB code's memo instead. The `local_hcb_code` fallbacks left the ledger item stale — a transaction with no HCB code can still have one — so they now go through it before writing the column directly. Also short-circuit both `update_custom_memo!` methods when nothing would change. Every write cascades into `Ledger::Item#map!` and `#refresh!` per record, and the settle path re-renames on each settlement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126DFcNZ5ZECDEij5J4zu2p
Both tables number their `HCB-000-<id>` codes off their own sequence, so a canonical transaction and a pending transaction get the same code whenever the two sequences line up — which they do on a fresh database. CI hit that; my local database had drifted far enough apart to hide it. The collision also trips `CanonicalTransaction#assign_ledger_item`, which reports an unexpected error when its calculated ledger item disagrees with its HCB code's, so the spec failed during setup rather than on an assertion. Group the canonical transaction under an explicit HCB code via an `HCB-<short code>` memo, so the two records stay in separate groups whatever the sequences hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126DFcNZ5ZECDEij5J4zu2p
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.
Summary of the problem
custom_memois stored in three places:canonical_transactions,canonical_pending_transactions, andledger_items.Ledger::Item#memo— the memo the ledger renders and searches — is a cached column computed inLedger::Item#refresh!from the ledger item's own copy:HcbCode#update_custom_memo!is the only writer that keeps all three in sync, but five paths wrote the canonical transactions directly and leftledger_items.custom_memoNULL:CanonicalTransactionsController#set_custom_memoCanonicalPendingTransactionsController#updateCanonicalPendingTransactionService::SettleStripeCardService::Nightlyupdate_all, skips every callbackHcbCode#update_custom_memo!itselfhcb_codes.ledger_item_idis NULLThe first two are reachable from the ✏️ icons in
hcb_codes/_transaction_history,hcb_codes/_pending_transaction_historyandtransactions/show.A direct write does still fire
ledger_item.refresh!, butrefresh!reads the ledger item's own (still-NULL)custom_memo, so:system_memo, so the rename is silently discarded — andrefresh!cannot recover it;system_memoand fall through tofallback_memo→ct.smart_memo, which is why callingrefresh!looks like a fix for those.ledger_items.custom_memostays NULL even then.Since
pg_search_scope :search_memoruns againstledger_items.memo, renamed transactions were also unfindable by their new name in the ledger.The fifth path is why the others can't simply be re-pointed:
hcb_codes.ledger_item_idis only back-linked when the transaction engine creates the item, so it is NULL for any transaction that adopted an existing one — andupdate_custom_memo!then fell back to writing the canonical transactions only.Describe your changes
Every
custom_memowrite onCanonicalTransaction/CanonicalPendingTransactionnow goes throughHcbCode#update_custom_memo!.HcbCode#update_custom_memo!resolves ledger items through its transactions as well as its ownledger_item_id, then writes the canonical transactions, the pending transactions and every resolved ledger item — no early return, so a transaction with no ledger item is still renamed. It short-circuits when nothing would change, since each write cascades intoLedger::Item#map!and#refresh!per record.Ledger::Item#update_custom_memo!owns just its own transactions (keyed byledger_item_id); group renames areHcbCode's job.fronted/fee_waivedas a normal update, and both only touch the memo when the param is submitted, so an absent field no longer clears it.Settlecopies through the HCB code, and only when the pending transaction actually has a memo — routing anilthroughupdate_custom_memo!would clear the memo on every record in the group. It also writes the pending transaction's ledger item:CanonicalPendingSettledMappinghands the canonical transaction over to that item after the transaction commits, and several settle paths (Settle::Donation,AdminController#set_wire) pair a pending transaction with a still-ungroupedHCB-000canonical transaction, so the two HCB codes differ and the destination item would keep its old memo.StripeCardService::Nightlyreplacesupdate_all(which fired no callbacks at all) with a batched per-transaction rename, skipping any group where a transaction already carries an organizer's memo — the group write would otherwise stamp the default over it.Behaviour change worth a look
Renaming from the per-transaction ✏️ now renames the whole HCB code group instead of the single canonical transaction, matching what the HCB code page and the v4 API already did. Two consequences:
HCB-900-<year>_<week>groups every fee reimbursement for a week onto one HCB code and one ledger item, across organizations — so a group rename there is cross-org, and the write is wider than the single-event policy these two endpoints authorize against.HcbCodesController#updatealready had this reach; these endpoints didn't. Flagging rather than changing authorization in a bugfix — happy to split that out either way.Remediation
Already-desynced rows are not repaired here.
Maintenance::BackfillLedgerItemCustomMemosTaskkeys offledger_item.hcb_code&.custom_memo, which is NULL for exactly the un-back-linked population above, so it needs to resolve the memo through the item's own transactions before it is re-run.Test plan
bundle exec rspec spec/models spec/services spec/controllers spec/jobs spec/mailboxes— 1691 examples, the only failures being 4 that need thewkhtmltopdfbinary and 2 that need Redis, neither available locally and neither memo-related.rubocopanderb_lintclean.New specs cover each path (rename and clear) and pin the regressions worth guarding: settling must not clear a memo an organizer already set and must reach the item the transaction is handed to; the nightly must not stamp over a group that already has a memo; a rename must reach the ledger item when the HCB code is not back-linked; and a transaction with no ledger item must still be renamed alongside one that has it. Assertions are on
custom_memorather thanmemo, sincefallback_memoreads the canonical transaction and masks a desynced item.Generated by Claude Code