Commit Graph

2 Commits

Author SHA1 Message Date
Víctor Falcón cd918523e8
fix(banking): stop duplicating EnableBanking transactions with positional entry_reference (#669)
## Problem

A user reported the same bank transactions appearing twice.
Investigation in prod showed this is a systematic dedup bug in the
EnableBanking sync affecting a few hundred rows across several
users/accounts.

## Root cause

The affected banks don't return a real `transaction_id`; their only id
is a **positional** `entry_reference` of the form
`{booking_date}.{index}` (e.g. `YYYY-MM-DD.0`). That field is **null the
day a transaction first appears** and only **populated on a later
sync**.

`TransactionFingerprint::for()` preferred `entry_reference` when present
and fell back to a content hash when absent. So the same transaction
produced two different fingerprints:

| | First sync (same day) | Later sync |
|---|---|---|
| `entry_reference` | `null` | `YYYY-MM-DD.0` |
| `dedup_fingerprint` | content-based | id-based |
| `external_transaction_id` | `null` | `YYYY-MM-DD.0` |

Neither the fingerprint nor the `external_transaction_id` dedup path
matched across the two syncs → duplicate row. The one-shot historical
first sync has no duplicates; the duplicates start with the daily
incremental syncs.

## Fix

Treat a positional (`^\d{4}-\d{2}-\d{2}\.\d+$`) `entry_reference` as "no
stable id" and fall through to the content hash, which is identical on
both syncs. Genuine `transaction_id` and non-positional
`entry_reference` keep keying exactly as before.

## Trade-off (accepted, follow-up tracked)

The positional index is also the only field that distinguishes two
genuinely-distinct same-day transactions with byte-identical content.
Falling to the content hash collapses them to one fingerprint, so only
the first is kept — a rare silent under-count. We accept it here over
the systematic duplication it fixes; **existing rows are unaffected**
(their distinct positional value is still stored in
`external_transaction_id` and caught by the fallback dedup path on
re-sync). Fixing both cases needs occurrence-aware dedup in the consumer
(a schema change) — left as a follow-up and documented in the code.

## Tests

- Regression: a positional `entry_reference` matches the same
transaction seen earlier without one.
- Boundary: non-positional references still key on `entry_reference`.
- `php artisan test
tests/Unit/Services/Banking/TransactionFingerprintTest.php` → 6 passed.
`pint` clean.

## Data cleanup (separate, after deploy)

The already-duplicated rows still need a one-off cleanup (soft-delete
the later copy per group, keeping the content-fingerprint original). It
must run **after** this fix ships, otherwise the next daily sync
re-creates them. Not included in this PR.
2026-07-12 16:25:13 +00:00
Víctor Falcón d9204bb3d6
fix(banking): dedup EnableBanking transactions by deterministic fingerprint (#390)
## Problem

Production user reported "inaccurate expenses, some appear multiple
times". DB inspection of their active BNP Paribas Fortis connection
confirmed duplicates growing every sync:

- `-3840` on `2026-05-11` x5
- `-2900` on `2026-05-08` x5
- `-2470` on `2026-05-12` x4
- 56 of 776 rows on the account had `external_transaction_id IS NULL`
- 16 (date, amount) duplicate groups, all with NULL upstream id

## Root cause

`TransactionSyncService::importTransaction()` short-circuited dedup when
both `transaction_id` and `entry_reference` were missing:

```php
$externalId = $data['transaction_id'] ?? $data['entry_reference'] ?? null;

if ($externalId) {
    // dedup check
}
// else: fall through, always insert
```

BNP returns no stable id for certain card transactions (`status:
"OTHR"`, `bank_transaction_code.code: "CCRD"`, foreign currency). Every
cron tick (every 6h) re-inserts a fresh copy.

Confirmed with prod data: of 16 NULL-id duplicate groups on this user,
**zero** ever got upgraded to a real id later. BNP simply doesn't issue
one.

## Fix

Deterministic per-transaction fingerprint, persisted in a new column,
protected by a unique index.

- **Migration**: adds `transactions.dedup_fingerprint` (nullable string,
80) and unique index on `(account_id, dedup_fingerprint)`. The unique
index is the real source of truth — it also closes the race between
overlapping sync runs that the prior `exists()` + `create()` pattern
couldn't.
- **`TransactionFingerprint::for($data)`**: two-mode fingerprint. If
`transaction_id` or `entry_reference` exists, the fingerprint is based
only on that canonical upstream id. If no upstream id exists, the
fallback fingerprint uses the prod-verified stable fields:
`booking_date`, amount, currency, credit/debit indicator,
creditor/debtor names + accounts, bank tx codes, reference number, and
remittance info. It intentionally excludes volatile fields (`status`,
`value_date`, raw `transaction_date`). Prefix `fp_` avoids mixing with
bank-issued ids.
- **`TransactionSyncService`**:
  - Always computes the fingerprint and writes it on every insert.
- Dedup lookup checks the fingerprint **and** (as a fallback) the legacy
`external_transaction_id` to gracefully handle rows imported before the
backfill runs.
- Wraps the insert in a `try/catch UniqueConstraintViolationException`
so concurrent syncs that pass `exists()` together don't crash.

## Rollout plan

Ship migration + service change → new duplicates stop. Existing
duplicate cleanup is intentionally out of scope for this PR.

## Tradeoff

Two genuinely distinct same-day, same-amount card transactions from the
same merchant on the same card collapse into one (no time-of-day in
raw_data for this BNP class). Today we over-count by 3–5x; after the fix
we may rarely under-count by 1. Acceptable net win, can monitor via logs
if needed.

## Tests

- `tests/Unit/Services/Banking/TransactionFingerprintTest.php` — 4 new
tests covering canonical id behavior and volatile-field exclusion.
- `tests/Feature/OpenBanking/TransactionSyncServiceTest.php` — 3 new
tests:
  - Dedupes payloads without an upstream id across consecutive syncs.
- Doesn't crash when a payload arrives later with an upstream id
(bounded behavior).
  - Dedupes against soft-deleted fingerprinted rows.
- Targeted suite green locally: **17 passed**.

## Files

-
`database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php`
*(new)*
- `app/Services/Banking/TransactionFingerprint.php` *(new)*
- `app/Services/Banking/TransactionSyncService.php`
- `app/Models/Transaction.php` (fillable)
- Tests above

## Out of scope

A smaller secondary pattern exists where BNP emitted distinct
`transaction_id`s for the same booking (Feb–Apr only, ~10 groups on the
reporter). Not addressed here because: (a) fingerprint includes
`transaction_id` so different ids = different fingerprints, (b) no
recent occurrences, (c) needs API-level analysis to determine when this
is genuine vs noise.
2026-05-13 11:30:11 +01:00