hackclub / hackclub/hcb

[Ledger] Settle-merge orphans the CT's original ledger item, leaving a $0 row on the org ledger

Open
#14,968 0 comments 0 reactions 0 assignees View on GitHub
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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.