hackclub / hackclub/hcb

[Ledger] Clean up CanonicalTransaction ledger item inheritance

Open
#14,866 0 comments 0 reactions 0 assignees View on GitHub
TX Engine Rewrite 2026
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.