[Ledger] Settle-merge orphans the CT's original ledger item, leaving a $0 row on the org ledger
- Dominant language
- Ruby
- Stars
- 870
- Forks
- 138
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 208
Description
### Summary
When a `CanonicalPendingSettledMapping` merges a CT onto the CPT's ledger item, the CT's **original** ledger item is left behind. It is never destroyed, it keeps its `Ledger::Mapping`, and it stays in `ledger.items` — so it renders on the org's ledger as a $0 "Transaction" row with no underlying transactions.
The code already documents that this row is supposed to be destroyed. From [`Ledger::Item#assign_linked_object!`](https://github.com/hackclub/hcb/blob/main/app/models/ledger/item.rb#L331-L341):
> In the event of a merger of ledger items (e.g. mapping a CT to an LI with an existing CPT), the ledger item with the CPT will persist, **and the ledger item with the CT will be destroyed.**
Nothing implements that destruction.
### Evidence
Creating a CPT and a CT (each getting its own ledger item), mapping both to the event's ledger, then settling:
```
[VIS] before settle: ledger 85 has items [104, 105]
[VIS] orphan #105 exists? true
[VIS] orphan still mapped to ledger? 85
[VIS] orphan ct_count=0 cpt_count=0 amount=0 memo="Transaction"
[VIS] after settle: ledger 85 has items [104, 105]
```
The ledger still lists both items afterwards. Item 105 has no CTs and no CPTs, `amount_cents = 0`, and the fallback memo `"Transaction"`.
### Why it happens
[`canonical_pending_settled_mapping.rb:40`](https://github.com/hackclub/hcb/blob/main/app/models/canonical_pending_settled_mapping.rb#L40) reassigns the CT:
```ruby
canonical_transaction.update(ledger_item: canonical_pending_transaction.ledger_item)
```
That fires the `previous_changes.key?("ledger_item_id")` hook at [canonical_transaction.rb:144-147](https://github.com/hackclub/hcb/blob/main/app/models/canonical_transaction.rb#L144-L147), which calls `refresh!` on the old item — but **not** `map!`, so the mapping is never reconsidered.
Calling `map!` would not help either. `Ledger::Mapper#run` bails at [mapper.rb:16](https://github.com/hackclub/hcb/blob/main/app/services/ledger/mapper.rb#L16) with `return if (ledger = calculate_ledger).nil?`, and `calculate_ledger` returns `nil` once the item has no transactions left. `Ledger::Mapping.map_primary!` is therefore never reached, and the existing mapping is left in place. The only code path that removes a primary mapping is the human-remap branch at [mapping.rb:68-70](https://github.com/hackclub/hcb/blob/main/app/models/ledger/mapping.rb#L68-L70).
### Impact
Every CPT that settles against a CT which had been given its own ledger item leaves a permanent empty row on the org's ledger. Users would see a $0 transaction labelled "Transaction". Worth checking how many such rows already exist in production:
```sql
SELECT COUNT(*) FROM ledger_items li
WHERE li.ct_count = 0 AND li.cpt_count = 0
AND EXISTS (SELECT 1 FROM ledger_mappings m WHERE m.ledger_item_id = li.id);
```
### Suggested fix
Destroy the emptied ledger item (and its mappings) as part of the merge in `CanonicalPendingSettledMapping`, matching the behaviour `assign_linked_object!` already documents. A backfill for existing orphans is likely needed too.
### How this was found
Surfaced while auditing redundant `Ledger::Item#map!`/`#refresh!` calls on transaction creation, and verified independently with the test above.
Contributor guide
Research direction
Start with app/models/canonical_pending_settled_mapping.rb and trace the ledger_item reassignment through app/models/canonical_transaction.rb, app/services/ledger/mapper.rb, and app/models/ledger/mapping.rb. Run the supplied reproduction and orphan query, then add coverage for settling a CT onto a CPT's ledger item; done means the emptied item and its mapping are removed, with existing orphans considered for backfill.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- ruby
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100