Commit Graph

9 Commits

Author SHA1 Message Date
Víctor Falcón 46568700b2
fix(banking): skip inaccessible EnableBanking accounts instead of failing the connection (#559)
## Problem

Sentry issue
[PHP-LARAVEL-3J](https://whisper-money.sentry.io/issues/PHP-LARAVEL-3J)
started escalating again — but with a **different** error than the 401
expired-session fixed in #557. Sentry groups both under the same
`getTransactions` culprit:

```
RequestException: HTTP request returned status code 400:
{"code":400,"message":"Account not found","detail":{"message":"Account not found","error_name":"AccountNotAccessibleException"}}
```

When EnableBanking returns `400 AccountNotAccessibleException` for
**one** account (closed, or no longer covered by the consent), the raw
`RequestException` was re-thrown. This:

1. **Aborted the whole connection's account loop** in
`EnableBankingSyncer` — accounts after the dead one never synced.
2. Marked the **entire connection** `Error` (`friendlyErrorMessage`
default), even though its other accounts were fine.
3. **Reported to Sentry** on every scheduled retry (escalating noise).

Confirmed in production (`agent:db --prod`): connection `019ea143-…` is
`active` with `valid_until` in the future (2026-09-05) and **6
accounts**; one (`08bbcdf9-…`) returns the 400, and the connection is
now stuck in `error` with the generic "try again later" message. 2 users
affected.

## Fix

A single account the bank no longer exposes must not break the rest of
the connection.

- New domain exception `InaccessibleBankAccountException` (implements
`ShouldntReport`, like `TransientBankingProviderException` /
`ExpiredBankingSessionException`).
- `EnableBankingProvider::getTransactions()` / `getBalances()` wrap a
`400 AccountNotAccessibleException` in it (keyed on `detail.error_name`)
instead of re-throwing the raw `RequestException`.
- `EnableBankingSyncer` catches it **per account**, logs a warning, and
`continue`s — the remaining accounts sync and the connection stays
`Active`.

Connections currently stuck in `error` from this self-heal on the next
scheduled sync (they're `Error` + under the retry cap + `valid_until`
future, so the scheduler re-dispatches them; the dead account is now
skipped).

Composes with #558: a user can permanently *stop syncing* such an
account from the manage-accounts screen. This PR only stops one dead
account from breaking automated syncs in the meantime.

## Tests

- `EnableBankingProviderTest`: `getTransactions` / `getBalances` wrap a
`400 AccountNotAccessibleException` as a non-reportable
`InaccessibleBankAccountException`.
- `SyncRetryAndLoggingTest`: with one inaccessible and one good account,
the good one still syncs, the connection stays `Active`, and nothing is
thrown.

All three failed before the fix. Full `tests/Feature/OpenBanking` suite
passes (276), pint clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
2026-06-19 08:59:05 +02:00
Víctor Falcón c36df98d32
fix(banking): handle EnableBanking expired sessions as reconnect, not error (#557)
## Problem

Sentry issue
[PHP-LARAVEL-3J](https://whisper-money.sentry.io/issues/PHP-LARAVEL-3J)
— `RequestException: HTTP 401 {"error":"EXPIRED_SESSION"}` in
`EnableBankingProvider::getTransactions` — was escalating (26 events, 2
users).

When an EnableBanking **session** expires *before* its 90-day consent
window (`valid_until`), the sync hit a `401 EXPIRED_SESSION` that was
not classified as transient or ASPSP, so it was re-thrown as a raw
`RequestException`. Consequences:

1. **Sentry noise** — reported as an error on every scheduled retry
until the failure cap.
2. **Silent breakage for the user** — EnableBanking's syncer returns
`notifiesOnAuthFailure() === false`, so the 401 went through the generic
permanent-error path: connection set to `Error` (not `Expired`) with
**no reconnect email**.

Confirmed in production: **10 of 17** EnableBanking connections in
`error` status are actually expired sessions (`valid_until` in the
future + "Authentication failed" message), all with no notification
sent.

## Fix

Treat a `401 EXPIRED_SESSION` as the expected lifecycle event it is:

- New domain exception `ExpiredBankingSessionException` (implements
`ShouldntReport`, like `TransientBankingProviderException`).
- `EnableBankingProvider::getTransactions()` / `getBalances()` wrap the
`401 EXPIRED_SESSION` in it instead of re-throwing the raw
`RequestException`.
- `SyncBankingConnectionJob` catches it and routes through the existing
expiry handling (extracted to `markExpired()`): marks the connection
`Expired`, sends `BankingConnectionExpiredEmail`, logs a skipped
attempt, returns **without throwing**.
- The provider's HTTP error logger downgrades the expected expiry from
`error` → `warning`.

`Expired` connections are not re-dispatched by
`SyncAllBankingConnectionsJob`, so retries stop and the user is prompted
to reconnect.

## Tests

- `EnableBankingProviderTest`: `getTransactions` / `getBalances` wrap a
`401 EXPIRED_SESSION` as a non-reportable
`ExpiredBankingSessionException`.
- `SyncRetryAndLoggingTest`: an expired session marks the connection
`Expired`, queues the reconnect email, and does not throw.

All three failed before the fix. Full `tests/Feature/OpenBanking` suite
passes (264 tests).

## Follow-up (not in this PR)

The 10 connections already stuck in `Error` from this bug are past the
retry cap and won't self-heal. A one-off command to reclassify them to
`Expired` and send the reconnect email would recover those users — worth
a separate, reviewed change.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
2026-06-18 13:48:13 +00:00
Víctor Falcón 220b1e11f1
refactor(banking): scalable per-provider sync via a syncer factory (#545)
## Why

Syncing was a growing `if/elseif` chain in
`SyncBankingConnectionJob::handle()` plus one private method per
provider. Every new provider meant editing the job, and the file mixed
provider-specific logic with cross-cutting orchestration. This does not
scale to dozens/hundreds of providers.

## What

Introduces a **factory + strategy** pattern:

- **`App\Contracts\BankingConnectionSyncer`** — interface: `sync()`,
`expires()`, `notifiesOnAuthFailure()`.
- **`AbstractBankingConnectionSyncer`** — defaults for the common case
(API-key provider: never expires, notifies on auth failure). A new
provider usually only implements `sync()`.
- **`BankingConnectionSyncerFactory`** — maps `connection.provider` to
its syncer (resolved from the container, so dependencies are injected).
- **Six syncers** — `IndexaCapitalSyncer`, `BinanceSyncer`,
`WiseSyncer`, `BitpandaSyncer`, `CoinbaseSyncer`, `EnableBankingSyncer`.
Each fully owns its provider behavior, including EnableBanking's daily
email and first-sync cutoff.

`SyncBankingConnectionJob` drops from ~520 to ~190 lines and now only
orchestrates: deleted-user/expiry/rate-limit checks, error handling,
retries, logging, and status updates. It asks the syncer `expires()` /
`notifiesOnAuthFailure()` instead of hardcoding provider lists.

### Adding a provider now

1. Create `XSyncer extends AbstractBankingConnectionSyncer` implementing
`sync()`.
2. Add one line to the factory map.

No changes to the job.

## Tests

- Existing job tests (`SyncBankingConnectionJobTest`,
`SyncRetryAndLoggingTest`) migrated through a shared `runSync()` helper
in `tests/Pest.php`; all original assertions preserved.
- New `BankingConnectionSyncerFactoryTest`: provider→syncer resolution,
unknown-provider exception, and the capability flags.
- Full suite green: **1604 passed**, 0 failures. PHPStan clean, Pint
applied.

## Docs

Adds `docs/adding-a-banking-provider.md` — a step-by-step developer
guide (usable by a human or an AI agent) covering the sync provider and
the broader end-to-end integration touchpoints.
2026-06-16 16:25:29 +02:00
Víctor Falcón 0f527d0f26
Notify users about expired bank connections (#404)
## Summary
- mark expired EnableBanking connections during scheduled sync and queue
reconnect email
- add reconnect email/button route for expired connections
- surface expired connections in shared Inertia props and show
persistent reconnect toast

## Tests
- vendor/bin/pint --dirty --format agent
- php artisan test --compact
tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php
--filter='expired|scheduled sync includes active enablebanking'
- php artisan test --compact
tests/Feature/OpenBanking/AuthorizationControllerTest.php
--filter='reauthorize|reconnect link'
- php artisan test --compact tests/Feature/InertiaSharedDataTest.php
--filter='expired banking'
- npm run types (fails on existing unrelated TypeScript errors; no
app.tsx/expiredBanking errors after Wayfinder generation)
2026-05-20 09:20:13 +02:00
Víctor Falcón 917a9a655f
Handle transient EnableBanking sync failures (#358)
## Problem

Sentry showed two related production issues in the same sync path:

- `PHP-LARAVEL-13`: EnableBanking returned HTTP `400` from `GET
/accounts/{accountId}/transactions` with body `ASPSP_ERROR` / `Error
interacting with ASPSP`. This is an upstream bank/provider failure, but
the app threw an unhandled `RequestException`.
- `PHP-LARAVEL-14`: the same transactions endpoint timed out after 20s,
throwing an unhandled `ConnectionException`.

Both originated from `EnableBankingProvider::getTransactions()` and
bubbled through `SyncBankingConnectionJob`, creating Sentry issues for
expected transient provider/bank outages.

## New behavior

- Wrap EnableBanking `400 ASPSP_ERROR` responses in
`TransientBankingProviderException`.
- Wrap EnableBanking transaction connection failures / timeouts in the
same transient exception.
- Mark that exception as `ShouldntReport`, so these expected upstream
failures stop creating Sentry issues.
- Keep queue retry behavior intact. The job still retries and only marks
the connection as `Error` after normal retry exhaustion.
- Log transient sync failures as warnings instead of errors.
- Show users a retry-later message when retries are exhausted: the bank
provider is temporarily unavailable.
- Leave other `400` responses reportable. Validation / app-side request
bugs still throw `RequestException`.
- Leave auth failures and rate-limit handling unchanged.

Fixes PHP-LARAVEL-13
Fixes PHP-LARAVEL-14

## Testing

- `vendor/bin/pint --dirty --format agent`
- `php artisan test --compact
tests/Feature/OpenBanking/EnableBankingProviderTest.php
tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php
tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php`
2026-05-06 09:24:05 +02:00
Víctor Falcón f800847591
feat(banking): back off scheduler when EnableBanking returns 429 (#352)
## Problem

Production logs show repeated EnableBanking 429s on the same
connections, every cron cycle:

```
[2026-05-04 18:00:55] EnableBanking API error status:429
  body: [HUB046] Allowed number of accesses exceeded for consent
[2026-05-04 21:47:12] EnableBanking API error status:429
  body: Daily PSU not present consultation limit has been exceeded
[2026-05-05 00:01:41] same connection, same error
[2026-05-05 06:01:41] same connection, same error
```

Root cause: `SyncBankingConnectionJob` returned early on 429s without
persisting any backoff state. The scheduler kept re-dispatching the same
connection on every run, hammering the provider and burning the daily
quota.

## Fix

Persist a per-connection backoff window so the scheduler stops
re-dispatching until the provider quota resets.

- New `rate_limited_until` column on `banking_connections`.
- On 429: derive the window from
  1. `Retry-After` header if present,
2. "Daily ..." message → next UTC midnight (matches PSU daily limit
semantics),
  3. default 1 hour (consent / generic).
- Job short-circuits with a `Skipped` sync log if the window is still
active.
- Successful sync clears the window.
- `SyncAllBankingConnectionsJob` + `SyncBankingConnections` command
filter out connections still inside their backoff.

## Tests

- Existing rate-limit test updated (now also asserts the backoff is
set).
- New: daily message → next UTC midnight.
- New: `Retry-After` header honoured (1800s).
- New: rate-limited connection skipped without calling provider.
- New: successful sync clears `rate_limited_until`.
- New: scheduler excludes connections whose backoff has not expired.

`php artisan test --compact
--filter="SyncBankingConnectionJobTest|SyncRetryAndLoggingTest|SyncBankingConnectionsCommandTest"`
→ 70 passed.
2026-05-05 09:39:32 +02:00
Víctor Falcón 244344e953
feat(open-banking): remove feature flag gating (#297)
## Summary
- remove the Pennant-based `open-banking` flag and middleware gating so
open banking is always available for authenticated users
- simplify landing, onboarding, settings, and subscription flows to use
always-on open-banking behavior and remove stale frontend/shared flag
plumbing
- update open-banking tests and purge stored `open-banking` Pennant rows

## Testing
- `php artisan test --compact
tests/Feature/OpenBanking/InstitutionControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/BinanceControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/IndexaCapitalControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/BitpandaControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/AuthorizationControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/ConnectionControllerTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/AccountMappingTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/OpenBankingFeatureFlagTest.php`
- `php artisan test --compact
tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php`
- `php artisan test --compact tests/Feature/SubscriptionTest.php`
- `php artisan test --compact
tests/Feature/WelcomeBanksOrderingTest.php`
- `vendor/bin/pint --dirty --format agent`

## Notes
- `php artisan pennant:purge open-banking` was run to remove stale
stored values
- `php artisan test --compact tests/Browser/OnboardingFlowTest.php`
still has one unrelated real-estate onboarding browser failure (`it
creates a real estate account during onboarding when feature is
enabled`)
2026-04-17 10:20:05 +02:00
Víctor Falcón c42a48a952
chore: Remove account-mapping feature flag (#252)
## Summary

- Removes the `account-mapping` Pennant feature flag entirely, making
the account mapping flow (pending accounts data + map-accounts page) the
default and only code path
- Removes the old direct-creation branches from all banking controllers
(Authorization, Bitpanda, Binance, IndexaCapital)
- Extracts a shared `CreatesAccountsFromPending` trait for the
onboarding auto-create logic
- Updates all controller tests to reflect the always-on mapping behavior

## Changes

### Backend
- **AppServiceProvider** — removed `account-mapping` flag definition
- **AuthorizationController** — removed flag check +
`createAccountsFromSession()` dead code; always stores
`pending_accounts_data` and redirects to mapping
- **BitpandaController / BinanceController / IndexaCapitalController** —
removed flag checks, old inline account creation, and unused imports
- **HandleInertiaRequests / ActivateDevelopmentFeatures /
ResolvesFeatures** — removed `account-mapping` from flag arrays
- **New `CreatesAccountsFromPending` trait** — shared auto-create logic
for onboarding path (to be consumed next)

### Frontend
- Removed `'account-mapping'` from the TypeScript `Features` interface

### Tests
- Merged flag-specific test variants into single always-on tests
- Removed redundant tests that tested the old disabled-flag code path
- Updated assertions to check `pending_accounts_data` instead of direct
account creation
2026-04-01 12:09:22 +02:00
Víctor Falcón f3b5929ecc
fix(banking): retry failed sync connections and log every sync attempt (#251)
## Summary

- **Fixes the broken retry mechanism** — after the first failed attempt
set status to `Error`, subsequent retry attempts bailed out because
`isActive()` returns `false` for `Error`. Now both `Active` and `Error`
statuses are syncable.
- **Adds auto-retry across scheduled runs** —
`SyncAllBankingConnectionsJob` and `banking:sync` command now include
`Error` connections where `consecutive_sync_failures < 3` (configurable
via `MAX_SCHEDULED_RETRIES`). After 3 full dispatch cycles, manual
intervention is required.
- **Logs every sync attempt to DB** — new `banking_sync_logs` table
records status (Success/Failed/Skipped), attempt number, error details,
duration, and metadata for each sync.

## Changes

### Core logic (`SyncBankingConnectionJob`)
- `isSyncableStatus()` allows both `Active` and `Error` through the gate
- Temporary errors: status only set to `Error` on the final attempt
(attempt 3); earlier attempts re-throw without changing status
- Permanent auth errors (401/403): `$this->fail()` called immediately,
`consecutive_sync_failures` set beyond the cap
- Rate limit (429): handled silently (existing behavior preserved)
- Every attempt is logged to `banking_sync_logs`

### Query updates
- `SyncAllBankingConnectionsJob`: includes `Error` connections under
retry cap
- `SyncBankingConnections` command: same query update for
`--user`/`--connection` filtered runs

### Controller updates
- `ConnectionController::sync()` and `updateCredentials()` reset
`consecutive_sync_failures` to 0

### New files
- `BankingSyncLogStatus` enum (Success, Failed, Skipped)
- `BankingSyncLog` model
- Two migrations: `add_consecutive_sync_failures` column,
`create_banking_sync_logs` table

### Tests
- Updated 3 existing auth error tests in `SyncBankingConnectionJobTest`
(24 pass)
- Added 17 new tests in `SyncRetryAndLoggingTest` covering retry
behavior, sync logging, scheduled retry inclusion/exclusion, and manual
retry reset
- All 10 `SyncBankingConnectionsCommandTest` tests still pass
2026-03-31 11:34:35 +01:00