From bc57eae5c34cdf37beb7a82b5569fb50d12e3ab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Sat, 27 Jun 2026 18:01:21 +0200 Subject: [PATCH] fix(open-banking): stop storing the XXX no-currency placeholder on accounts (#602) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Prod warning logs were flooded with `Exchange rate not found, returning unconverted amount` (≈500 lines from a single dashboard load). The `ExchangeRateService` was behaving correctly — the source currency was the ISO 4217 placeholder **`XXX`** ("no currency"), which has no exchange rate, so conversions silently fell back to an **unconverted (wrong) amount**. Root cause: when importing accounts from a banking connection (Enable Banking can report `currency: "XXX"`), the code did `$accountData['currency'] ?? 'EUR'`. The `??` only catches `null`/missing, not the literal `"XXX"`, so the placeholder was persisted as `currency_code`. Prod data confirmed `XXX` is the **only** account currency not covered by the rates table (342 currencies, incl. BTC/VES/exotics, are all present): 18 accounts (16 from bank links, 2 manual) across 14 users, plus **5 users** whose base currency had itself become `XXX` (a first-account `XXX` propagates to the user via `syncFromFirstAccount`). ## What 1. **Fix the source** — `AccountUserCurrencyService::resolveImportedCurrency()` resolves a bank-reported currency to: bank value → user base currency → app default (`cashier.currency`), treating `XXX`/empty/missing as "no currency". Wired into both creation paths (`CreatesAccountsFromPending`, `AccountMappingController`); also covers Interactive Brokers imports. 2. **Backfill** — migration fixes existing rows in order: `XXX` users → app default first, then `XXX` accounts → their owner's currency. ## Tests - `AccountUserCurrencyServiceTest` — resolver chain (valid / uppercasing / XXX·empty·null → user / both missing → default). - `AccountMappingTest` — mapping flow falls back to the user currency when the bank reports `XXX`. - `BackfillXxxAccountCurrenciesTest` — migration resolves both XXX owners and XXX accounts end-to-end. 23 tests green; Pint and Larastan clean. --- .../OpenBanking/AccountMappingController.php | 2 +- .../Concerns/CreatesAccountsFromPending.php | 2 +- app/Services/AccountUserCurrencyService.php | 20 +++++++++ ...000000_backfill_xxx_account_currencies.php | 44 +++++++++++++++++++ .../AccountUserCurrencyServiceTest.php | 33 ++++++++++++++ .../BackfillXxxAccountCurrenciesTest.php | 22 ++++++++++ .../OpenBanking/AccountMappingTest.php | 39 ++++++++++++++++ 7 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 database/migrations/2026_06_27_000000_backfill_xxx_account_currencies.php create mode 100644 tests/Feature/AccountUserCurrencyServiceTest.php create mode 100644 tests/Feature/BackfillXxxAccountCurrenciesTest.php diff --git a/app/Http/Controllers/OpenBanking/AccountMappingController.php b/app/Http/Controllers/OpenBanking/AccountMappingController.php index 61aa1cb5..558ee0b1 100644 --- a/app/Http/Controllers/OpenBanking/AccountMappingController.php +++ b/app/Http/Controllers/OpenBanking/AccountMappingController.php @@ -88,7 +88,7 @@ class AccountMappingController extends Controller } if ($action === 'create') { - $currency = $accountData['currency'] ?? 'EUR'; + $currency = $accountUserCurrencyService->resolveImportedCurrency($accountData['currency'] ?? null, $user); $name = $accountData['name'] ?? $accountData['account_id']['iban'] ?? $connection->aspsp_name.' Account'; diff --git a/app/Http/Controllers/OpenBanking/Concerns/CreatesAccountsFromPending.php b/app/Http/Controllers/OpenBanking/Concerns/CreatesAccountsFromPending.php index 95ef5fe5..49ae2f45 100644 --- a/app/Http/Controllers/OpenBanking/Concerns/CreatesAccountsFromPending.php +++ b/app/Http/Controllers/OpenBanking/Concerns/CreatesAccountsFromPending.php @@ -37,7 +37,7 @@ trait CreatesAccountsFromPending continue; } - $currency = $accountData['currency'] ?? 'EUR'; + $currency = $accountUserCurrencyService->resolveImportedCurrency($accountData['currency'] ?? null, $user); $name = $accountData['name'] ?? $accountData['account_id']['iban'] ?? $connection->aspsp_name.' Account'; diff --git a/app/Services/AccountUserCurrencyService.php b/app/Services/AccountUserCurrencyService.php index d2ba435c..72f31d7e 100644 --- a/app/Services/AccountUserCurrencyService.php +++ b/app/Services/AccountUserCurrencyService.php @@ -7,6 +7,26 @@ use App\Models\User; class AccountUserCurrencyService { + /** + * Resolve the currency code to store for a bank-imported account. + * + * Providers may report "XXX" (ISO 4217 "no currency") or omit the field; + * in those cases fall back to the user's base currency, then to the app + * default, so amounts stay convertible. + */ + public function resolveImportedCurrency(?string $reported, User $user): string + { + foreach ([$reported, $user->currency_code] as $candidate) { + $candidate = strtoupper(trim((string) $candidate)); + + if ($candidate !== '' && $candidate !== 'XXX') { + return $candidate; + } + } + + return strtoupper(config('cashier.currency', 'eur')); + } + public function syncFromFirstAccount(Account $account): void { $user = $account->user; diff --git a/database/migrations/2026_06_27_000000_backfill_xxx_account_currencies.php b/database/migrations/2026_06_27_000000_backfill_xxx_account_currencies.php new file mode 100644 index 00000000..82cbab17 --- /dev/null +++ b/database/migrations/2026_06_27_000000_backfill_xxx_account_currencies.php @@ -0,0 +1,44 @@ +where('currency_code', 'XXX') + ->update(['currency_code' => $default]); + + DB::table('accounts') + ->where('currency_code', 'XXX') + ->orderBy('id') + ->chunkById(200, function ($accounts): void { + $userCurrencies = DB::table('users') + ->whereIn('id', collect($accounts)->pluck('user_id')) + ->pluck('currency_code', 'id'); + + foreach ($accounts as $account) { + DB::table('accounts') + ->where('id', $account->id) + ->update(['currency_code' => strtoupper((string) ($userCurrencies[$account->user_id] ?? 'EUR'))]); + } + }); + } + + public function down(): void + { + // ponytail: irreversible — the original "XXX" carried no real currency. + } +}; diff --git a/tests/Feature/AccountUserCurrencyServiceTest.php b/tests/Feature/AccountUserCurrencyServiceTest.php new file mode 100644 index 00000000..763a41aa --- /dev/null +++ b/tests/Feature/AccountUserCurrencyServiceTest.php @@ -0,0 +1,33 @@ +service = app(AccountUserCurrencyService::class); + config(['cashier.currency' => 'eur']); +}); + +test('keeps a valid reported currency', function () { + $user = User::factory()->make(['currency_code' => 'USD']); + + expect($this->service->resolveImportedCurrency('GBP', $user))->toBe('GBP'); +}); + +test('uppercases the reported currency', function () { + $user = User::factory()->make(['currency_code' => 'USD']); + + expect($this->service->resolveImportedCurrency('gbp', $user))->toBe('GBP'); +}); + +test('falls back to the user currency for XXX, empty or missing codes', function (?string $reported) { + $user = User::factory()->make(['currency_code' => 'USD']); + + expect($this->service->resolveImportedCurrency($reported, $user))->toBe('USD'); +})->with(['XXX', 'xxx', '', null]); + +test('falls back to the app default when both the bank and the user lack a currency', function () { + $user = User::factory()->make(['currency_code' => 'XXX']); + + expect($this->service->resolveImportedCurrency('XXX', $user))->toBe('EUR'); +}); diff --git a/tests/Feature/BackfillXxxAccountCurrenciesTest.php b/tests/Feature/BackfillXxxAccountCurrenciesTest.php new file mode 100644 index 00000000..8cbc529c --- /dev/null +++ b/tests/Feature/BackfillXxxAccountCurrenciesTest.php @@ -0,0 +1,22 @@ + 'eur']); + + $normalUser = User::factory()->create(['currency_code' => 'MXN']); + $xxxUser = User::factory()->create(['currency_code' => 'XXX']); + + $fromNormalOwner = Account::factory()->create(['user_id' => $normalUser->id, 'currency_code' => 'XXX']); + $fromXxxOwner = Account::factory()->create(['user_id' => $xxxUser->id, 'currency_code' => 'XXX']); + $untouched = Account::factory()->create(['user_id' => $normalUser->id, 'currency_code' => 'MXN']); + + (require database_path('migrations/2026_06_27_000000_backfill_xxx_account_currencies.php'))->up(); + + expect($xxxUser->refresh()->currency_code)->toBe('EUR'); + expect($fromNormalOwner->refresh()->currency_code)->toBe('MXN'); + expect($fromXxxOwner->refresh()->currency_code)->toBe('EUR'); + expect($untouched->refresh()->currency_code)->toBe('MXN'); +}); diff --git a/tests/Feature/OpenBanking/AccountMappingTest.php b/tests/Feature/OpenBanking/AccountMappingTest.php index 48558b7c..9764f73d 100644 --- a/tests/Feature/OpenBanking/AccountMappingTest.php +++ b/tests/Feature/OpenBanking/AccountMappingTest.php @@ -182,6 +182,45 @@ test('store creates investment accounts for crypto provider connections', functi 'coinbase' => ['coinbase', 'Coinbase', 'coinbase-portfolio'], ]); +test('store falls back to the user currency when the bank reports XXX', function () { + Queue::fake(); + + $user = User::factory()->onboarded()->create(['currency_code' => 'USD']); + Account::factory()->create(['user_id' => $user->id, 'currency_code' => 'USD']); + + $connection = BankingConnection::factory()->awaitingMapping()->create([ + 'user_id' => $user->id, + 'aspsp_name' => 'Test Bank', + 'pending_accounts_data' => [ + [ + 'uid' => 'ext-1', + 'currency' => 'XXX', + 'name' => 'No-currency Account', + 'account_id' => [], + ], + ], + ]); + + $this->actingAs($user) + ->post(route('open-banking.map-accounts.store', $connection), [ + 'mappings' => [ + [ + 'bank_account_uid' => 'ext-1', + 'action' => 'create', + 'existing_account_id' => null, + ], + ], + ]) + ->assertRedirect(route('settings.connections.index')); + + $this->assertDatabaseHas('accounts', [ + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-1', + 'currency_code' => 'USD', + ]); + $this->assertDatabaseMissing('accounts', ['currency_code' => 'XXX']); +}); + test('store updates user currency from first account created from mapping', function () { Queue::fake();