Commit Graph

7 Commits

Author SHA1 Message Date
Víctor Falcón 5bdf734c42 fix(banking): clamp and retry the sync window when the bank rejects the period
On WrongTransactionsPeriodException, restart the account sync from the first
page with a progressively narrower window (90/30/7 days before date_to) so the
user still gets the history the bank is willing to serve, instead of the whole
connection sync crashing. strategy='longest' is dropped on the narrowed retry so
the explicit date_from is honoured, and re-fetched pages are idempotent (dedup +
date-keyed daily balances). If even the narrowest window is refused, the
exception is rethrown so the syncer can skip just that account.
2026-07-07 02:53:07 +02:00
Víctor Falcón 84bad76316
perf(banking): kill per-transaction dedup N+1 in bank sync (PHP-LARAVEL-3Y) (#621)
## What

Fixes the N+1 flagged by Sentry **PHP-LARAVEL-3Y** in
`App\Jobs\SyncBankingConnectionJob`.

`TransactionSyncService::importTransaction()` ran one `exists()` probe
**per incoming bank transaction** to detect duplicates:

```sql
select exists(
  select * from transactions
  where account_id = ?
    and (dedup_fingerprint = ? or external_transaction_id = ?)
) as exists
```

A sync importing N transactions issued N dedup `SELECT`s. As transaction
volume per sync grows, so does the query count.

## How

Preload the account's existing dedup keys **once** at the start of
`sync()` and match against in-memory sets:

- `loadExistingDedupKeys()` streams (`cursor()`) the two dedup columns
for the account — including soft-deleted rows (`withTrashed()`) — into
two keyed sets. One query instead of N.
- `importTransaction()` checks membership in memory (`isset()`), an
exact mirror of the old `fingerprint OR external_id` predicate.
- Keys from successful inserts are folded back into the sets, so
duplicates **within the same sync** (e.g. the same transaction repeated
across pages) are still caught in memory.
- The `(account_id, dedup_fingerprint)` unique index +
`UniqueConstraintViolationException` catch still backstop concurrent
syncs (`SyncBankingConnectionJob` is already `ShouldBeUnique` per
connection).

## Commits

1. `perf(banking): preload dedup keys to kill per-transaction N+1` — the
core fix + a query-count regression test.
2. `perf(banking): stream dedup key preload with a cursor` — avoids
buffering every historical row into a Collection on top of the sets
(memory guard for very large accounts).
3. `fix(banking): match external ids case-insensitively in dedup` — the
old query compared `external_transaction_id` under the production
`utf8mb4_unicode_ci` collation; the in-memory `isset()` is byte-exact.
With no unique index on `external_transaction_id`, a legacy id stored as
`ABC` vs an incoming `abc` would have silently double-imported.
Normalized with `mb_strtolower()` on both sides + a test.
4. `test(banking): cover intra-run cross-page dedup` — new coverage for
the same transaction appearing on two pages of one sync.

## Testing

- 303/303 in `tests/Feature/OpenBanking/` green; full backend suite
green except one **pre-existing, unrelated** failure on `main`
(`DashboardTest` returns 409 locally — an Inertia asset-version artifact
that resolves once `bun run build` runs in CI).
- New tests: dedup runs as a single preload SELECT regardless of batch
size; case-insensitive external-id dedup; cross-page intra-run dedup.
- `pint` clean; `larastan` clean on the changed file.

## Reviewed by two independent agents (architecture +
product/correctness)

Both rated the change behavior-preserving and shippable. Follow-ups they
surfaced, intentionally **not** bundled here to keep this fix focused
and low-risk:

- **`WiseTransactionSyncService` has the identical latent N+1**
(`:149-158`). It is not currently firing in Sentry (lower volume). Worth
a separate PR — possibly extracting a shared dedup-set helper once both
call sites need it.
- **Residual collation divergence**: accent/width folding from
`utf8mb4_unicode_ci` is not replicated in PHP. Irrelevant for real bank
reference ids (ASCII), accepted.
- The test comment referencing a "cleanup command" that repoints orphan
fingerprinted rows describes a command that does not exist in the repo —
pre-existing, worth correcting separately.

Fixes PHP-LARAVEL-3Y
2026-07-02 13:34:29 +00:00
Víctor Falcón 10da06ed84
feat(transactions): add counterparty fields (#440)
## Summary
- add creditor/debtor fields to transactions with raw_data backfill
- store counterparties on bank sync and CSV/XLS imports
- add creditor/debtor filters plus hidden table columns

## Tests
- php artisan test --compact tests/Feature/LocalizationTest.php
tests/Feature/OpenBanking/TransactionSyncServiceTest.php
tests/Feature/TransactionFilterTest.php
- npm test -- resources/js/lib/file-parser.test.ts --run
- vendor/bin/pint --dirty --format agent

Note: `npm run types` still has pre-existing unrelated errors; no
creditor/debtor/import-related errors remained in filtered output.

## Screenshot
<img width="1241" height="676" alt="8odYWtFcvUM"
src="https://github.com/user-attachments/assets/55653485-d588-4beb-9e6a-5c7c81ba7cf8"
/>
2026-05-27 16:20:55 +02: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
Víctor Falcón 9242b3fe5f
feat: Add per-bank description formatter for bank-synced transactions (#120)
## Summary

- Adds a `BankFormatter` interface with a `BbvaFormatter` implementation
that transforms ALL CAPS `//`-separated BBVA descriptions into readable
Title Case (preserving acronyms like SEPA, S.A., BIZUM and Spanish
stopwords)
- Stores the raw description in a new `original_description` column when
formatting changes it, keeping sync matching intact
- Existing non-BBVA banks and null-bank transactions pass through
unchanged

## Test plan

- [x] Unit tests for `BbvaFormatter` (11 tests: title case, acronyms,
stopwords, reference numbers, mixed-case passthrough, whitespace)
- [x] Unit tests for `TransactionDescriptionFormatter` dispatcher (4
tests: BBVA match, no-match, null bank)
- [x] Feature tests for BBVA sync integration (formatted description +
original stored)
- [x] Feature tests for non-BBVA sync (description unchanged, original
null)
- [x] All 25 existing + new tests pass
- [x] Pint formatting clean
2026-02-13 09:54:58 +01:00
Víctor Falcón d1ba18932e
fix: Prevent re-syncing deleted bank transactions (#114)
## Summary
- Added `withTrashed()` to the duplicate check in
`TransactionSyncService::importTransaction()` so soft-deleted
transactions are not re-created on subsequent bank syncs.
- Added test covering the soft-deleted transaction deduplication
scenario.

## Test plan
- [x] `php artisan test --filter=TransactionSyncServiceTest` — all 8
tests pass
2026-02-12 20:53:52 +01:00
Víctor Falcón db7b6e4da7
feat: Integrate EnableBanking as open banking provider (#106)
## Summary

- Adds **EnableBanking** as the first open banking provider, allowing
users to connect real bank accounts and automatically sync transactions
and balances
- Uses a **`BankingProviderInterface`** contract so future providers
(Plaid, GoCardless, etc.) can be added by implementing the same
interface
- Feature-flagged behind the **`open-banking`** Pennant flag (default:
off)
- Connected accounts are **unencrypted** and transactions have `source =
'enablebanking'`

### What's included

**Backend:**
- `BankingProviderInterface` contract + `EnableBankingProvider`
implementation (JWT RS256 auth)
- `BankingConnection` model with full lifecycle (pending →
awaiting_mapping → active → expired/revoked/error)
- `TransactionSyncService` — pagination, deduplication by
`external_transaction_id`, amount/date mapping
- `BalanceSyncService` — preferred balance type selection (CLBD → ITAV
fallback)
- Authorization flow: start auth → bank redirect → callback → session
creation → account mapping → sync
- `SyncBankingConnectionJob` (unique per connection, 3 retries) +
scheduled every 6 hours
- `banking:sync` artisan command
- 5 migrations: `banking_connections` table, account fields, transaction
`external_transaction_id`, `pending_accounts_data`, `linked_at`

**Frontend:**
- Manual vs Connected account choice in the create account dialog
- Multi-step bank connection dialog (country → bank selection →
confirmation → redirect)
- Account mapping page — map discovered bank accounts to existing
accounts, create new ones, or skip
- Settings/Connections page with status badges, sync/disconnect actions
- "Connected" badge on linked accounts in settings

**Tests:**
- 49 tests covering feature flags, controllers, account mapping,
transaction sync, balance sync, deduplication, and pagination

### Feature Flags

This PR introduces **two Pennant feature flags**:

1. **`open-banking`** — Gates the entire open banking feature
(institutions endpoint, authorization flow, connections page). When
disabled, all open banking routes return 404.

2. **`account-mapping`** — Controls whether users see an intermediate
account mapping step after connecting a bank. When **enabled**, users
are redirected to a mapping page where they can choose to create new
accounts, link to existing ones, or skip each discovered bank account.
When **disabled**, all discovered accounts are automatically created
(original behavior). Linked accounts only sync transactions from their
last transaction date and only update the current balance from the
provider (no historical balance calculation or daily balance tracking).

Enable per-user:
```bash
php artisan feature:enable open-banking user@example.com
php artisan feature:enable account-mapping user@example.com
```

Enable for all users:
```bash
php artisan feature:enable open-banking all
php artisan feature:enable account-mapping all
```

## Test plan

- [x] Enable feature flag: `php artisan feature:enable open-banking`
- [x] Verify "Connected" option appears in create account dialog
- [x] Start authorization flow and verify redirect to bank
- [x] With `account-mapping` **disabled**: verify callback creates
accounts directly and dispatches sync
- [x] With `account-mapping` **enabled**: verify callback redirects to
mapping page
- [x] Test mapping page: create new, link to existing, and skip actions
- [x] Verify linked accounts sync only from last transaction date and
only update current balance
- [x] Verify connections page shows "Setup Required" badge for
awaiting_mapping status
- [x] Run `php artisan banking:sync` and verify transactions sync
- [x] Verify connections page shows status, sync, and disconnect actions
- [x] Run full test suite: `php artisan test --compact`
2026-02-12 09:09:28 +01:00