[Ledger] Clean up CanonicalTransaction ledger item inheritance
- Dominant language
- Ruby
- Stars
- 870
- Forks
- 138
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 208
Description
`CanonicalPendingSettledMapping`'s `after_create_commit` blindly overwrites the settled transaction's ledger item with the pending transaction's:
https://github.com/hackclub/hcb/blob/536765ef06a9610c2441de76396c5f98d11a1f83/app/models/canonical_pending_settled_mapping.rb#L40
By the time a settle mapping exists, `CanonicalTransaction` should already have inherited that ledger item on its own ([`before_create`](https://github.com/hackclub/hcb/blob/536765ef06a9610c2441de76396c5f98d11a1f83/app/models/canonical_transaction.rb#L127-L136) + [`#assign_ledger_item`](https://github.com/hackclub/hcb/blob/536765ef06a9610c2441de76396c5f98d11a1f83/app/models/canonical_transaction.rb#L487-L500)), so the line looks like it should be a no-op. Tracing a realistic Stripe card settlement locally (create a `RawPendingStripeTransaction` → import the CPT → create a `RawStripeTransaction` carrying the same `authorization` → create the CT → settle) turned up a few related things.
### 1. Confirm the overwrite is dead, then remove it
In a realistic `HCB-600` settlement the CT and CPT share one `HcbCode`, and the CT already holds the CPT's ledger item when the mapping is created — the overwrite changes nothing. Before deleting it outright, instrument it for a release to be sure that holds for every settlement path (Column, ACH, checks, disbursements, invoices, donations, bank fees), then delete the whole block:
```ruby
if canonical_transaction.ledger_item_id != canonical_pending_transaction.ledger_item_id
Rails.error.unexpected "CT ##{canonical_transaction.id} settled from CPT ##{canonical_pending_transaction.id} had a different ledger item (#{canonical_transaction.ledger_item_id.inspect} vs. #{canonical_pending_transaction.ledger_item_id.inspect}). The CT's ledger item was overwritten."
canonical_transaction.update(ledger_item_id: canonical_pending_transaction.ledger_item_id)
end
```
Comparing `ledger_item_id` rather than `ledger_item` keeps the no-op case free of association loads.
Note that landing this requires aligning the specs: `Rails.error.unexpected` raises in test, and every spec that settles builds an unrelated CT and CPT from factories, so each has its own ledger item and the report fires on 9 examples. Fix is to align the pair where the settlement is constructed — an `after(:build)` in `spec/factories/canonical_pending_settled_mapping_factory.rb` that points the CT at the CPT's ledger item, plus the same in `spec/services/canonical_pending_transaction_service/settle_spec.rb`, which builds its pair directly instead of through the factory.
### 2. `CanonicalTransaction`'s `before_create` chain short-circuits for card charges
```ruby
self.ledger_item_id ||= if short_code.present? && (li = Ledger::Item.find_by(short_code:))
li.id
elsif linked_object.present?
linked_object.try(:canonical_pending_transaction).try(:ledger_item_id)
elsif raw_stripe_transaction&.stripe_authorization_id
rpst = RawPendingStripeTransaction.find_by(stripe_transaction_id: raw_stripe_transaction.stripe_authorization_id)
rpst&.canonical_pending_transaction&.ledger_item_id
end
```
For a card charge, `linked_object` is a `CardCharge`, and `CardCharge` has no `canonical_pending_transaction` method — so `try` returns nil, that branch "wins" with nil, and the `RawPendingStripeTransaction` lookup below it never runs. Card-charge CTs are therefore created with `ledger_item_id` nil every time, and only recover a commit later in `#assign_ledger_item` (via `linked_object_v2 → card_charge.ledger_item`).
These should be `||` fallbacks rather than exclusive branches, so a present-but-unhelpful `linked_object` doesn't skip the branch that would have worked. That also closes the window where a CT exists with no ledger item, and makes removing the overwrite in (1) provably safe rather than probably safe.
### 3. `#assign_ledger_item`'s divergence report fires on a nil
```ruby
if calculated_ledger_item != local_hcb_code.ledger_item
Rails.error.unexpected("CanonicalTransaction #{id} has calculated a different ledger item from its local_hcb_code. (#{calculated_ledger_item&.id} vs. #{local_hcb_code.ledger_item&.id})")
end
```
A `HcbCode` that isn't linked to a ledger item yet reports as a divergence. Reproduced locally, note the empty right-hand side:
```
CanonicalTransaction 522 has calculated a different ledger item from its local_hcb_code. (5394 vs. )
```
nil means "not linked yet", not "diverged", so this should be guarded with `local_hcb_code.ledger_item.present? &&`. This is the report that #14739 skipped `BadSettledMapping`'s "settled into several canonical transactions" example over; that example can probably come back once this is fixed (it passes 11/11 seeds in isolation today, so whatever's left is suite-order dependent).
### 4. `#calculated_ledger_item` doesn't memoize nil
```ruby
def calculated_ledger_item
@calculated_ledger_item ||= Ledger::Item.find_by(short_code:) || linked_object_v2&.ledger_item
end
```
When the result is nil — the common case for a freshly created CT — `||=` re-runs the whole thing on every call, and `#assign_ledger_item` calls it two or three times. Each evaluation is a `short_code` lookup plus `linked_object_v2`, which is itself up to four `find_by`s for Column transactions.
### Not an issue (checked)
After `CanonicalPendingTransactionService::Unsettle`, the CT still points at the CPT's ledger item. That's correct — they share the `HcbCode` the ledger item is keyed to, so there's nothing to reset.
Contributor guide
Research direction
Start with app/models/canonical_pending_settled_mapping.rb and app/models/canonical_transaction.rb, then read spec/factories/canonical_pending_settled_mapping_factory.rb and spec/services/canonical_pending_transaction_service/settle_spec.rb. Run the relevant settlement specs and trace the listed settlement paths. Done means the overwrite is safely removed, ledger-item fallback and divergence handling are corrected, nil lookup results are memoized, and the affected specs pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rails, ruby
- Domain
- backend, payments
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 56/100