fix(open-banking): unblock account mapping when the bank reports accounts without a uid (#746)
## Problem
A paying user reported that "Save & Sync" on the account mapping screen
did nothing — no error, no navigation, nothing.
EnableBanking returned their Société Générale card (`CB Visa`) with
`uid: null`:
```json
{"uid": null, "name": "CB Visa", "cash_account_type": "CARD", "account_id": {"iban": null, "other": {"scheme_name": "CPAN", "identification": "************8210"}}}
```
The page renders one mapping row per pending account, so the form posted
`bank_account_uid: null`. `MapAccountsRequest` requires it, the request
422'd, and since the page never rendered validation errors the button
looked dead. The user retried the connection six times.
A second problem kept them from doing what they actually wanted: both
accounts reported `currency: "XXX"` — ISO 4217 for "no currency".
`getCompatibleAccounts` filtered their existing EUR accounts against
`"XXX"`, matched nothing, and the "Link to existing account" option was
never rendered. The backend already knew `XXX` isn't a currency
(`AccountUserCurrencyService::resolveImportedCurrency`); the frontend
didn't.
## Fix
- Accounts without a uid are filtered out of the mapping screen. They
can never be synced anyway — every sync service early-returns on an
empty `external_account_id` — and `CreatesAccountsFromPending` already
skipped them during onboarding. That rule now lives in one place,
`BankingConnection::mappablePendingAccounts()`.
- The skipped accounts are named on the page, so users don't go hunting
for an account they can see in their bank app.
- A connection where *no* account has a uid is closed out instead of
parking on a mapping page that could only ever 422.
- Validation errors surface as a toast, with app-authored messages in
`MapAccountsRequest::messages()` instead of raw field paths.
- `XXX` (and a blank/lowercase variant) is treated as "unknown
currency": the code isn't displayed, and it no longer filters out every
linkable account.
## Scope
One user affected in production (the reporter). Their connection has one
valid account alongside the card, so this fix unblocks them on deploy —
no data change needed.
## Testing
- `tests/Feature/OpenBanking` — 315 passed, including two new cases:
uid-less accounts are hidden and named, and an all-uid-less connection
is closed rather than left awaiting mapping.
- Browser QA against a local reproduction of the exact production
payload: only `Compte Bancaire` is offered, `CB Visa` is named as
skipped, "Link to existing account" appears despite the `XXX` currency,
submitting without picking an account toasts the error, and picking
`Dany SG` links it (`external_account_id` set, connection `active`).
## Demo
https://github.com/user-attachments/assets/6275834a-a518-47b9-9015-a698e5926d71
<!-- PLACEHOLDER: drag account-mapping-fix-qa.mp4 here -->
## Not in this PR
`AuthorizationController::refreshAccountIds()` consumes the raw pending
list on reconnect; its positional fallback can pair a legacy account
with a uid-less entry. Not reachable for any account in production
today, it belongs to a different flow, and it deserves its own test —
filed separately rather than smuggled in here.
This commit is contained in:
Víctor Falcón
GitHub
parent
a3eafecf60
commit
eb60f2eb30
|
|
@ -42,6 +42,18 @@ class AccountMappingController extends Controller
|
|||
->with('success', 'Bank account connected successfully.');
|
||||
}
|
||||
|
||||
$mappableAccounts = $connection->mappablePendingAccounts();
|
||||
|
||||
// Nothing the bank gave us can be mapped, so there is no decision left for the
|
||||
// user to make. Close the connection instead of leaving it awaiting a mapping
|
||||
// that can never happen.
|
||||
if ($mappableAccounts === []) {
|
||||
$this->createAccountsFromPending($user, $connection, $accountUserCurrencyService);
|
||||
|
||||
return redirect()->route('settings.connections.index')
|
||||
->with('error', __('Your bank did not provide an identifier for any of its accounts, so they cannot be synced.'));
|
||||
}
|
||||
|
||||
$existingAccounts = $user
|
||||
->accounts()
|
||||
->whereNull('banking_connection_id')
|
||||
|
|
@ -50,8 +62,9 @@ class AccountMappingController extends Controller
|
|||
|
||||
return Inertia::render('open-banking/map-accounts', [
|
||||
'connection' => $connection,
|
||||
'bankAccounts' => $connection->pending_accounts_data,
|
||||
'bankAccounts' => $mappableAccounts,
|
||||
'existingAccounts' => $existingAccounts,
|
||||
'unmappableAccountNames' => $connection->unmappablePendingAccountNames(),
|
||||
]);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -30,12 +30,8 @@ trait CreatesAccountsFromPending
|
|||
|
||||
$accountType = $connection->provider->defaultAccountType();
|
||||
|
||||
foreach ($connection->pending_accounts_data ?? [] as $accountData) {
|
||||
$uid = $accountData['uid'] ?? null;
|
||||
|
||||
if (! $uid) {
|
||||
continue;
|
||||
}
|
||||
foreach ($connection->mappablePendingAccounts() as $accountData) {
|
||||
$uid = $accountData['uid'];
|
||||
|
||||
$currency = $accountUserCurrencyService->resolveImportedCurrency($accountData['currency'] ?? null, $user);
|
||||
$name = $accountData['name']
|
||||
|
|
|
|||
|
|
@ -31,4 +31,17 @@ class MapAccountsRequest extends FormRequest
|
|||
],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array<string, string>
|
||||
*/
|
||||
public function messages(): array
|
||||
{
|
||||
return [
|
||||
'mappings.required' => 'There are no accounts to map.',
|
||||
'mappings.min' => 'There are no accounts to map.',
|
||||
'mappings.*.bank_account_uid.required' => 'This account cannot be mapped because your bank did not provide an identifier for it.',
|
||||
'mappings.*.existing_account_id.required_if' => 'Choose the account you want to link to.',
|
||||
];
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ use Illuminate\Database\Eloquent\SoftDeletes;
|
|||
* @property Carbon|null $bank_transactions_email_cutoff_at
|
||||
* @property Carbon|null $rate_limited_until
|
||||
* @property int $consecutive_sync_failures
|
||||
* @property array<int, mixed>|null $pending_accounts_data
|
||||
* @property array<int, array<string, mixed>>|null $pending_accounts_data
|
||||
*/
|
||||
class BankingConnection extends Model
|
||||
{
|
||||
|
|
@ -152,6 +152,38 @@ class BankingConnection extends Model
|
|||
return ! empty($this->pending_accounts_data);
|
||||
}
|
||||
|
||||
/**
|
||||
* Pending accounts the user can actually map. Providers sometimes report accounts
|
||||
* without a uid (EnableBanking does it for some French cards); those can never be
|
||||
* synced, so offering them for mapping only produces a validation error.
|
||||
*
|
||||
* @return array<int, array<string, mixed>>
|
||||
*/
|
||||
public function mappablePendingAccounts(): array
|
||||
{
|
||||
return array_values(array_filter(
|
||||
$this->pending_accounts_data ?? [],
|
||||
fn (array $account): bool => ! empty($account['uid']),
|
||||
));
|
||||
}
|
||||
|
||||
/**
|
||||
* Names of the pending accounts left out of the mapping screen, so the user is told
|
||||
* why an account they can see in their bank never shows up here.
|
||||
*
|
||||
* @return array<int, string>
|
||||
*/
|
||||
public function unmappablePendingAccountNames(): array
|
||||
{
|
||||
return array_values(array_map(
|
||||
fn (array $account): string => $account['name'] ?? $account['account_id']['iban'] ?? __('Bank Account'),
|
||||
array_filter(
|
||||
$this->pending_accounts_data ?? [],
|
||||
fn (array $account): bool => empty($account['uid']),
|
||||
),
|
||||
));
|
||||
}
|
||||
|
||||
public function isExpired(): bool
|
||||
{
|
||||
return $this->status === BankingConnectionStatus::Expired
|
||||
|
|
|
|||
|
|
@ -1150,6 +1150,8 @@
|
|||
"No balance records found.": "No se encontraron registros de balance.",
|
||||
"No bank connections yet. Connect a bank to automatically sync your transactions.": "Aún no hay conexiones bancarias. Conecta un banco para sincronizar automáticamente tus transacciones.",
|
||||
"No bank found.": "No se encontró ningún banco.",
|
||||
"Not shown: :accounts. Your bank did not provide an identifier, so syncing is not possible.": "No se muestran: :accounts. Tu banco no proporcionó un identificador, así que no es posible sincronizarlas.",
|
||||
"Your bank did not provide an identifier for any of its accounts, so they cannot be synced.": "Tu banco no proporcionó un identificador para ninguna de sus cuentas, así que no se pueden sincronizar.",
|
||||
"No banks found.": "No se encontraron bancos.",
|
||||
"No cashflow data for this period": "Sin datos de flujo de dinero para este período",
|
||||
"No categories found.": "No se encontraron categorías.",
|
||||
|
|
|
|||
|
|
@ -1111,6 +1111,8 @@
|
|||
"No balance records found.": "Aucun enregistrement de solde trouvé.",
|
||||
"No bank connections yet. Connect a bank to automatically sync your transactions.": "Il n'y a pas encore de connexions bancaires. Connectez une banque pour synchroniser automatiquement vos transactions.",
|
||||
"No bank found.": "Aucun banc n'a été trouvé.",
|
||||
"Not shown: :accounts. Your bank did not provide an identifier, so syncing is not possible.": "Non affichés : :accounts. Votre banque n'a pas fourni d'identifiant, la synchronisation est donc impossible.",
|
||||
"Your bank did not provide an identifier for any of its accounts, so they cannot be synced.": "Votre banque n'a fourni d'identifiant pour aucun de ses comptes, ils ne peuvent donc pas être synchronisés.",
|
||||
"No banks found.": "Aucun banc n'a été trouvé.",
|
||||
"No cashflow data for this period": "Aucune donnée sur les flux monétaires pour cette période",
|
||||
"No categories found.": "Aucune catégorie trouvée.",
|
||||
|
|
|
|||
|
|
@ -21,6 +21,17 @@ import type { BankingConnection, PendingBankAccount } from '@/types/banking';
|
|||
import { __ } from '@/utils/i18n';
|
||||
import { Head, Link, router } from '@inertiajs/react';
|
||||
import { useState } from 'react';
|
||||
import { toast } from 'sonner';
|
||||
|
||||
/**
|
||||
* Banks don't always report a usable currency: EnableBanking sends XXX, the ISO 4217
|
||||
* code for "no currency". Treat that as unknown rather than as a real code.
|
||||
*/
|
||||
function usableCurrency(reported: string | null | undefined): string | null {
|
||||
const currency = (reported ?? '').trim().toUpperCase();
|
||||
|
||||
return currency === '' || currency === 'XXX' ? null : currency;
|
||||
}
|
||||
|
||||
interface Mapping {
|
||||
bank_account_uid: string;
|
||||
|
|
@ -32,12 +43,14 @@ interface Props {
|
|||
connection: BankingConnection;
|
||||
bankAccounts: PendingBankAccount[];
|
||||
existingAccounts: Account[];
|
||||
unmappableAccountNames: string[];
|
||||
}
|
||||
|
||||
export default function MapAccountsPage({
|
||||
connection,
|
||||
bankAccounts,
|
||||
existingAccounts,
|
||||
unmappableAccountNames,
|
||||
}: Props) {
|
||||
const [mappings, setMappings] = useState<Mapping[]>(
|
||||
bankAccounts.map((ba) => ({
|
||||
|
|
@ -56,7 +69,12 @@ export default function MapAccountsPage({
|
|||
);
|
||||
}
|
||||
|
||||
function getCompatibleAccounts(currency: string) {
|
||||
/** With no currency reported there is nothing to match on, so anything is linkable. */
|
||||
function getLinkableAccounts(currency: string | null) {
|
||||
if (!currency) {
|
||||
return existingAccounts;
|
||||
}
|
||||
|
||||
return existingAccounts.filter((a) => a.currency_code === currency);
|
||||
}
|
||||
|
||||
|
|
@ -67,6 +85,11 @@ export default function MapAccountsPage({
|
|||
`/open-banking/connections/${connection.id}/map-accounts`,
|
||||
{ mappings },
|
||||
{
|
||||
onError: (errors) => {
|
||||
toast.error(
|
||||
Object.values(errors)[0] ?? __('Something went wrong.'),
|
||||
);
|
||||
},
|
||||
onFinish: () => setProcessing(false),
|
||||
},
|
||||
);
|
||||
|
|
@ -94,13 +117,17 @@ export default function MapAccountsPage({
|
|||
const mapping = mappings.find(
|
||||
(m) => m.bank_account_uid === bankAccount.uid,
|
||||
);
|
||||
const compatibleAccounts = getCompatibleAccounts(
|
||||
bankAccount.currency,
|
||||
);
|
||||
const currency = usableCurrency(bankAccount.currency);
|
||||
const compatibleAccounts =
|
||||
getLinkableAccounts(currency);
|
||||
const iban = bankAccount.account_id?.iban;
|
||||
const displayName =
|
||||
bankAccount.name ||
|
||||
bankAccount.account_id?.iban ||
|
||||
__('Bank Account');
|
||||
bankAccount.name || iban || __('Bank Account');
|
||||
const details = [
|
||||
currency,
|
||||
// Don't repeat the IBAN when it is already the title.
|
||||
displayName === iban ? null : iban,
|
||||
].filter(Boolean);
|
||||
|
||||
return (
|
||||
<Card key={bankAccount.uid}>
|
||||
|
|
@ -108,12 +135,11 @@ export default function MapAccountsPage({
|
|||
<CardTitle className="text-base">
|
||||
{displayName}
|
||||
</CardTitle>
|
||||
<CardDescription>
|
||||
{bankAccount.currency}
|
||||
{bankAccount.account_id?.iban &&
|
||||
bankAccount.name &&
|
||||
` \u00b7 ${bankAccount.account_id.iban}`}
|
||||
</CardDescription>
|
||||
{details.length > 0 && (
|
||||
<CardDescription>
|
||||
{details.join(' \u00b7 ')}
|
||||
</CardDescription>
|
||||
)}
|
||||
</CardHeader>
|
||||
<CardContent>
|
||||
<RadioGroup
|
||||
|
|
@ -244,6 +270,15 @@ export default function MapAccountsPage({
|
|||
);
|
||||
})}
|
||||
|
||||
{unmappableAccountNames.length > 0 && (
|
||||
<p className="text-sm text-muted-foreground">
|
||||
{__(
|
||||
'Not shown: :accounts. Your bank did not provide an identifier, so syncing is not possible.',
|
||||
{ accounts: unmappableAccountNames.join(', ') },
|
||||
)}
|
||||
</p>
|
||||
)}
|
||||
|
||||
<div className="flex items-center justify-end gap-3">
|
||||
<Link href="/settings/connections">
|
||||
<Button type="button" variant="outline">
|
||||
|
|
|
|||
|
|
@ -34,6 +34,66 @@ test('show returns mapping page with correct props', function () {
|
|||
);
|
||||
});
|
||||
|
||||
test('show hides pending accounts the provider reported without a uid', function () {
|
||||
$user = User::factory()->onboarded()->create();
|
||||
$connection = BankingConnection::factory()->awaitingMapping()->create([
|
||||
'user_id' => $user->id,
|
||||
'pending_accounts_data' => [
|
||||
[
|
||||
'uid' => 'ext-1',
|
||||
'currency' => 'EUR',
|
||||
'name' => 'Compte Bancaire',
|
||||
'account_id' => ['iban' => 'FR1234567890'],
|
||||
],
|
||||
[
|
||||
'uid' => null,
|
||||
'currency' => 'XXX',
|
||||
'name' => 'CB Visa',
|
||||
'account_id' => ['iban' => null],
|
||||
],
|
||||
],
|
||||
]);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('open-banking.map-accounts', $connection))
|
||||
->assertOk()
|
||||
->assertInertia(fn ($page) => $page
|
||||
->component('open-banking/map-accounts')
|
||||
->has('bankAccounts', 1)
|
||||
->where('bankAccounts.0.uid', 'ext-1')
|
||||
->where('unmappableAccountNames', ['CB Visa'])
|
||||
);
|
||||
});
|
||||
|
||||
test('show closes the connection when no pending account can be mapped', function () {
|
||||
Queue::fake();
|
||||
|
||||
$user = User::factory()->onboarded()->create();
|
||||
$connection = BankingConnection::factory()->awaitingMapping()->create([
|
||||
'user_id' => $user->id,
|
||||
'pending_accounts_data' => [
|
||||
[
|
||||
'uid' => null,
|
||||
'currency' => 'XXX',
|
||||
'name' => 'CB Visa',
|
||||
'account_id' => ['iban' => null],
|
||||
],
|
||||
],
|
||||
]);
|
||||
|
||||
$this->actingAs($user)
|
||||
->get(route('open-banking.map-accounts', $connection))
|
||||
->assertRedirect(route('settings.connections.index'));
|
||||
|
||||
$connection->refresh();
|
||||
expect($connection->status)->toBe(BankingConnectionStatus::Active);
|
||||
expect($connection->pending_accounts_data)->toBeNull();
|
||||
|
||||
$this->assertDatabaseMissing('accounts', [
|
||||
'banking_connection_id' => $connection->id,
|
||||
]);
|
||||
});
|
||||
|
||||
test('show redirects if no pending accounts', function () {
|
||||
$user = User::factory()->onboarded()->create();
|
||||
$connection = BankingConnection::factory()->create([
|
||||
|
|
|
|||
Loading…
Reference in New Issue