diff --git a/app/Http/Controllers/OnboardingController.php b/app/Http/Controllers/OnboardingController.php index 7bb3a62a..3689b520 100644 --- a/app/Http/Controllers/OnboardingController.php +++ b/app/Http/Controllers/OnboardingController.php @@ -5,6 +5,7 @@ namespace App\Http\Controllers; use App\Enums\BankingConnectionStatus; use App\Jobs\CategorizeOnboardingTransactionsJob; use App\Models\Bank; +use App\Models\BankingConnection; use App\Models\Category; use App\Models\Transaction; use Illuminate\Http\JsonResponse; @@ -75,15 +76,31 @@ class OnboardingController extends Controller ]); } + /** + * Report whether the onboarding sync step should keep waiting. + * + * A connection that already recorded an error will never set last_synced_at + * (rate limits keep the status Active while only storing the message), so it + * must not hold the user on the syncing step forever. It is reported as + * failed instead, so the step can say so rather than claim everything worked. + */ public function syncStatus(Request $request): JsonResponse { - $pending = $request->user() + $unsynced = $request->user() ->bankingConnections() - ->where('status', BankingConnectionStatus::Active) + ->whereIn('status', [BankingConnectionStatus::Active, BankingConnectionStatus::Error]) ->whereNull('last_synced_at') - ->exists(); + ->get(['status', 'error_message']); - return response()->json(['pending' => $pending]); + $pending = $unsynced->contains( + fn (BankingConnection $connection): bool => $connection->status === BankingConnectionStatus::Active + && $connection->error_message === null + ); + + return response()->json([ + 'pending' => $pending, + 'failed' => ! $pending && $unsynced->isNotEmpty(), + ]); } public function complete(Request $request): RedirectResponse diff --git a/app/Http/Controllers/OpenBanking/AccountMappingController.php b/app/Http/Controllers/OpenBanking/AccountMappingController.php index 558ee0b1..bf65449e 100644 --- a/app/Http/Controllers/OpenBanking/AccountMappingController.php +++ b/app/Http/Controllers/OpenBanking/AccountMappingController.php @@ -126,6 +126,8 @@ class AccountMappingController extends Controller $connection->update([ 'status' => BankingConnectionStatus::Active, 'pending_accounts_data' => null, + 'error_message' => null, + 'consecutive_sync_failures' => 0, ]); SyncBankingConnectionJob::dispatch($connection); diff --git a/database/factories/BankingConnectionFactory.php b/database/factories/BankingConnectionFactory.php index 71edb947..6b55de10 100644 --- a/database/factories/BankingConnectionFactory.php +++ b/database/factories/BankingConnectionFactory.php @@ -171,4 +171,18 @@ class BankingConnectionFactory extends Factory 'consecutive_sync_failures' => 1, ]); } + + /** + * A connection the provider rate limited: still Active, backing off, and + * carrying an error message instead of a sync. + */ + public function rateLimited(): static + { + return $this->state(fn (array $attributes) => [ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => null, + 'rate_limited_until' => now()->addHour(), + 'error_message' => 'Rate limit exceeded. Please wait a few minutes and try again.', + ]); + } } diff --git a/lang/es.json b/lang/es.json index 1b149aa4..7fe492b8 100644 --- a/lang/es.json +++ b/lang/es.json @@ -1983,6 +1983,7 @@ "Your :provider connection has expired.": "Tu conexión con :provider ha caducado.", "Your :provider connection needs attention": "Tu conexión con :provider necesita atención", "Your Accounts": "Tus Cuentas", + "Your bank is taking longer than expected. We’ll keep trying in the background and your transactions will show up automatically.": "Tu banco está tardando más de lo previsto. Seguiremos intentándolo en segundo plano y tus transacciones aparecerán automáticamente.", "Your Categories Include:": "Tus Categorías Incluyen:", "Your Data is Truly Private": "Tus Datos Son Verdaderamente Privados", "Your Data, Your Privacy": "Tus Datos, Tu Privacidad", @@ -2178,6 +2179,7 @@ "No thanks": "No, gracias", "AI suggestions need more data": "Las sugerencias de IA necesitan más datos", "Once you have at least :count transactions, you can generate rule suggestions from Settings → Automation rules.": "Cuando tengas al menos :count transacciones, podrás generar sugerencias de reglas desde Ajustes → Reglas de automatización.", + "We couldn’t finish importing right now": "No hemos podido terminar la importación", "We couldn’t generate suggestions": "No pudimos generar sugerencias", "Something went wrong. You can try again or skip for now.": "Algo salió mal. Puedes intentarlo de nuevo u omitirlo por ahora.", "Try again": "Intentar de nuevo", diff --git a/resources/js/components/onboarding/step-syncing.test.tsx b/resources/js/components/onboarding/step-syncing.test.tsx new file mode 100644 index 00000000..a09c4bcf --- /dev/null +++ b/resources/js/components/onboarding/step-syncing.test.tsx @@ -0,0 +1,81 @@ +import { act, render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { StepSyncing } from './step-syncing'; + +const { get } = vi.hoisted(() => ({ get: vi.fn() })); + +vi.mock('axios', () => ({ + default: { get, isAxiosError: () => false }, +})); + +const reload = vi.fn(); + +vi.mock('@inertiajs/react', () => ({ + router: { reload: (...args: unknown[]) => reload(...args) }, +})); + +describe('StepSyncing', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + get.mockReset(); + reload.mockReset(); + }); + + it('stops spinning and offers to continue when a connection failed', async () => { + get.mockResolvedValue({ data: { pending: false, failed: true } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect( + screen.getByText('We couldn’t finish importing right now'), + ).toBeInTheDocument(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('gives up on a sync that never resolves instead of polling forever', async () => { + get.mockResolvedValue({ data: { pending: true, failed: false } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect( + screen.getByText('This will only take a moment.'), + ).toBeInTheDocument(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(5 * 60_000 + 3_000); + }); + + expect( + screen.getByText('We couldn’t finish importing right now'), + ).toBeInTheDocument(); + expect( + screen.queryByText('This will only take a moment.'), + ).not.toBeInTheDocument(); + }); + + it('advances on its own once every connection has synced', async () => { + get.mockResolvedValue({ data: { pending: false, failed: false } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect(reload).toHaveBeenCalled(); + expect( + screen.queryByText('We couldn’t finish importing right now'), + ).not.toBeInTheDocument(); + }); +}); diff --git a/resources/js/components/onboarding/step-syncing.tsx b/resources/js/components/onboarding/step-syncing.tsx index 9ab788cd..9c6c9048 100644 --- a/resources/js/components/onboarding/step-syncing.tsx +++ b/resources/js/components/onboarding/step-syncing.tsx @@ -1,10 +1,19 @@ +import { StepButton } from '@/components/onboarding/step-button'; +import { StepHeader } from '@/components/onboarding/step-header'; import { syncStatus } from '@/routes/onboarding'; import { __ } from '@/utils/i18n'; import { router } from '@inertiajs/react'; import axios from 'axios'; -import { Loader2 } from 'lucide-react'; +import { CloudOff, Loader2 } from 'lucide-react'; import { useCallback, useEffect, useRef, useState } from 'react'; +// Client-side give-up: the sync keeps running on the queue, but this guarantees +// the spinner resolves even if the worker dies. Beyond the worst case a healthy +// first sync can take (3 attempts of a 120s job, 30s apart). +const MAX_POLL_MS = 5 * 60_000; + +const POLL_INTERVAL_MS = 3_000; + const MESSAGES = [ 'Importing your balances...', 'Fetching your transactions...', @@ -24,6 +33,7 @@ interface StepSyncingProps { export function StepSyncing({ onComplete }: StepSyncingProps) { const [messageIndex, setMessageIndex] = useState(0); const [isPending, setIsPending] = useState(null); + const [hasStalled, setHasStalled] = useState(false); const onCompleteRef = useRef(onComplete); onCompleteRef.current = onComplete; @@ -40,28 +50,49 @@ export function StepSyncing({ onComplete }: StepSyncingProps) { useEffect(() => { let cancelled = false; let pollTimer: ReturnType; + const deadline = Date.now() + MAX_POLL_MS; + + const stall = () => { + setIsPending(false); + setHasStalled(true); + }; + + // A failing status check says nothing about the sync itself, so both the + // still-pending and the request-failed paths keep polling until the + // deadline rather than dropping the user into the next step early. + const keepPolling = () => { + if (Date.now() > deadline) { + stall(); + + return; + } + + setIsPending(true); + pollTimer = setTimeout(() => check(), POLL_INTERVAL_MS); + }; const check = async () => { try { - const { data } = await axios.get<{ pending: boolean }>( - syncStatus().url, - ); + const { data } = await axios.get<{ + pending: boolean; + failed: boolean; + }>(syncStatus().url, { timeout: POLL_INTERVAL_MS }); if (cancelled) { return; } - if (!data.pending) { + if (data.failed) { + stall(); + } else if (data.pending) { + keepPolling(); + } else { setIsPending(false); advance(); - } else { - setIsPending(true); - pollTimer = setTimeout(() => check(), 3000); } } catch { if (!cancelled) { - // On error, advance anyway to not block the user - advance(); + keepPolling(); } } }; @@ -87,6 +118,22 @@ export function StepSyncing({ onComplete }: StepSyncingProps) { return () => clearInterval(interval); }, [isPending]); + if (hasStalled) { + return ( +
+ + +
+ ); + } + // Don't render anything until we know sync is pending if (!isPending) { return null; diff --git a/tests/Feature/Onboarding/OnboardingSyncStatusTest.php b/tests/Feature/Onboarding/OnboardingSyncStatusTest.php index c76051ed..ee75e623 100644 --- a/tests/Feature/Onboarding/OnboardingSyncStatusTest.php +++ b/tests/Feature/Onboarding/OnboardingSyncStatusTest.php @@ -54,6 +54,46 @@ it('returns pending false when unsynced connection has an error status', functio ->assertJson(['pending' => false]); }); +it('reports a rate limited connection as failed instead of pending', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->rateLimited()->create(); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => false, 'failed' => true]); +}); + +it('keeps waiting while a healthy connection syncs alongside a failed one', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->rateLimited()->create(); + BankingConnection::factory()->for($user)->create([ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => null, + ]); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => true, 'failed' => false]); +}); + +it('does not report a synced connection as failed', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->create([ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => now(), + ]); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => false, 'failed' => false]); +}); + it('returns pending false when unsynced connection is revoked', function () { $user = User::factory()->create(['onboarded_at' => null]);