fix(open-banking): stop storing the XXX no-currency placeholder on accounts (#602)
## 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.
This commit is contained in:
parent
e5350ff1a6
commit
bc57eae5c3
|
|
@ -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';
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,44 @@
|
|||
<?php
|
||||
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
return new class extends Migration
|
||||
{
|
||||
/**
|
||||
* Backfill accounts and their owners left on the ISO 4217 "no currency"
|
||||
* placeholder "XXX". That code has no exchange rate, so every conversion
|
||||
* for such an account fell back to an unconverted (wrong) amount.
|
||||
*
|
||||
* Owners first: a user whose first account was XXX inherited XXX too, so
|
||||
* resolve those to the app default before accounts fall back to the owner.
|
||||
*/
|
||||
public function up(): void
|
||||
{
|
||||
$default = strtoupper(config('cashier.currency', 'eur'));
|
||||
|
||||
DB::table('users')
|
||||
->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.
|
||||
}
|
||||
};
|
||||
|
|
@ -0,0 +1,33 @@
|
|||
<?php
|
||||
|
||||
use App\Models\User;
|
||||
use App\Services\AccountUserCurrencyService;
|
||||
|
||||
beforeEach(function () {
|
||||
$this->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');
|
||||
});
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Account;
|
||||
use App\Models\User;
|
||||
|
||||
test('backfill resolves XXX accounts to their owner currency and XXX owners to the app default', function () {
|
||||
config(['cashier.currency' => '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');
|
||||
});
|
||||
|
|
@ -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();
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue