From 75c6fb31898f24568a74448a54f34c6619b7365b Mon Sep 17 00:00:00 2001 From: jakub-przepiora Date: Thu, 30 Jul 2026 09:46:10 +0200 Subject: [PATCH 1/2] fix(onboarding): stop step 3 from crashing on resubmit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storeStep3 inserted the process template with a hardcoded `version => 1` via a plain `create()`. Because the (product_type_id, version) unique index is already satisfied by the first insert, any replay of the step — double click on "Next", browser back + resubmit, Inertia retry on a slow connection — hit a UniqueConstraintViolationException and surfaced as a 500 to the user. Three changes: - Bail out early when the session already holds `onboarding.template_id`, making the step idempotent and redirecting to step 4 instead. - Derive the version with `max('version') + 1`, the same rule the admin UI (ProcessTemplateManagementController) and the API already use. Onboarding was the only place assuming v1. - Wrap the template and its steps in a transaction so a failure part way through no longer leaves an orphaned template behind. Covered by two regression tests that fail without the fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../Controllers/Web/OnboardingController.php | 40 +++++++++++----- .../tests/Feature/OnboardingWizardTest.php | 48 +++++++++++++++++++ 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/backend/app/Http/Controllers/Web/OnboardingController.php b/backend/app/Http/Controllers/Web/OnboardingController.php index e7b367c3..5e3a69c0 100644 --- a/backend/app/Http/Controllers/Web/OnboardingController.php +++ b/backend/app/Http/Controllers/Web/OnboardingController.php @@ -119,6 +119,13 @@ public function step3(Request $request) public function storeStep3(Request $request) { + // The step is not repeatable: a resubmit (double click, browser back, + // Inertia retry) would otherwise insert a second template and violate + // the (product_type_id, version) unique index. + if ($request->session()->has('onboarding.template_id')) { + return redirect()->route('onboarding.step4'); + } + $validated = $request->validate([ 'name' => 'required|string|max:255', 'steps' => 'required|array|min:1', @@ -128,21 +135,28 @@ public function storeStep3(Request $request) $productTypeId = $request->session()->get('onboarding.product_type_id'); - $template = ProcessTemplate::create([ - 'product_type_id' => $productTypeId, - 'name' => $validated['name'], - 'version' => 1, - 'is_active' => true, - ]); + $template = DB::transaction(function () use ($validated, $productTypeId) { + // Same versioning rule as the admin UI and the API: never assume v1. + $nextVersion = ProcessTemplate::where('product_type_id', $productTypeId)->max('version') + 1; - foreach ($validated['steps'] as $i => $stepData) { - TemplateStep::create([ - 'process_template_id' => $template->id, - 'step_number' => $i + 1, - 'name' => $stepData['name'], - 'estimated_duration_minutes' => $stepData['estimated_duration_minutes'] ?? null, + $template = ProcessTemplate::create([ + 'product_type_id' => $productTypeId, + 'name' => $validated['name'], + 'version' => $nextVersion, + 'is_active' => true, ]); - } + + foreach ($validated['steps'] as $i => $stepData) { + TemplateStep::create([ + 'process_template_id' => $template->id, + 'step_number' => $i + 1, + 'name' => $stepData['name'], + 'estimated_duration_minutes' => $stepData['estimated_duration_minutes'] ?? null, + ]); + } + + return $template; + }); $request->session()->put('onboarding.template_id', $template->id); diff --git a/backend/tests/Feature/OnboardingWizardTest.php b/backend/tests/Feature/OnboardingWizardTest.php index 8a62d039..a0cee0fb 100644 --- a/backend/tests/Feature/OnboardingWizardTest.php +++ b/backend/tests/Feature/OnboardingWizardTest.php @@ -110,6 +110,54 @@ public function test_step3_creates_template_with_steps(): void $this->assertEquals(3, ProcessTemplate::first()->steps()->count()); } + public function test_step3_resubmit_does_not_create_duplicate_template(): void + { + $pt = ProductType::factory()->create(); + + $payload = [ + 'name' => 'Assembly Process', + 'steps' => [['name' => 'Preparation', 'estimated_duration_minutes' => 10]], + ]; + + $this->actingAs($this->admin) + ->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), $payload) + ->assertRedirect(route('onboarding.step4')); + + // Same session replays the step (double click / browser back / Inertia retry). + $this->actingAs($this->admin) + ->withSession([ + 'onboarding.product_type_id' => $pt->id, + 'onboarding.template_id' => ProcessTemplate::first()->id, + ]) + ->post(route('onboarding.step3'), $payload) + ->assertRedirect(route('onboarding.step4')); + + $this->assertEquals(1, ProcessTemplate::count()); + $this->assertEquals(1, ProcessTemplate::first()->steps()->count()); + } + + public function test_step3_assigns_next_version_when_product_type_already_has_template(): void + { + $pt = ProductType::factory()->create(); + ProcessTemplate::factory()->create(['product_type_id' => $pt->id, 'version' => 1]); + + $response = $this->actingAs($this->admin) + ->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), [ + 'name' => 'Second Process', + 'steps' => [['name' => 'Preparation', 'estimated_duration_minutes' => 10]], + ]); + + $response->assertRedirect(route('onboarding.step4')); + $this->assertDatabaseHas('process_templates', [ + 'product_type_id' => $pt->id, + 'name' => 'Second Process', + 'version' => 2, + ]); + $this->assertEquals(2, ProcessTemplate::where('product_type_id', $pt->id)->count()); + } + public function test_step4_creates_work_order(): void { $line = Line::factory()->create(); From f8b33b991e1bfb209755bd868e7940e32d38afc5 Mon Sep 17 00:00:00 2001 From: jakub-przepiora Date: Fri, 31 Jul 2026 08:06:17 +0200 Subject: [PATCH 2/2] fix(onboarding): make step 3 template creation race-safe + widen tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review on #217: - Concurrency: the session replay guard stops sequential resubmits, but two truly concurrent same-session POSTs can both read an empty template table and pick the same (product_type_id, version) — a FOR UPDATE lock can't serialise the first insert (no rows to lock yet). Instead of a cross-writer distributed lock, catch UniqueConstraintViolationException and adopt the winning template idempotently, so the loser continues to step 4 instead of hitting a 500. - Tests: capture onboarding.template_id from the session (not ProcessTemplate:: first()) for the replay case; add version-1 (no prior template), validation 422, and guest/non-admin authorization coverage for step 3. --- .../Controllers/Web/OnboardingController.php | 54 ++++++++------ .../tests/Feature/OnboardingWizardTest.php | 70 +++++++++++++++++-- 2 files changed, 100 insertions(+), 24 deletions(-) diff --git a/backend/app/Http/Controllers/Web/OnboardingController.php b/backend/app/Http/Controllers/Web/OnboardingController.php index 5e3a69c0..b09eda77 100644 --- a/backend/app/Http/Controllers/Web/OnboardingController.php +++ b/backend/app/Http/Controllers/Web/OnboardingController.php @@ -9,6 +9,7 @@ use App\Models\TemplateStep; use App\Services\WorkOrder\WorkOrderService; use App\Support\ModuleRegistry; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Http\Request; use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rule; @@ -135,28 +136,41 @@ public function storeStep3(Request $request) $productTypeId = $request->session()->get('onboarding.product_type_id'); - $template = DB::transaction(function () use ($validated, $productTypeId) { - // Same versioning rule as the admin UI and the API: never assume v1. - $nextVersion = ProcessTemplate::where('product_type_id', $productTypeId)->max('version') + 1; - - $template = ProcessTemplate::create([ - 'product_type_id' => $productTypeId, - 'name' => $validated['name'], - 'version' => $nextVersion, - 'is_active' => true, - ]); - - foreach ($validated['steps'] as $i => $stepData) { - TemplateStep::create([ - 'process_template_id' => $template->id, - 'step_number' => $i + 1, - 'name' => $stepData['name'], - 'estimated_duration_minutes' => $stepData['estimated_duration_minutes'] ?? null, + try { + $template = DB::transaction(function () use ($validated, $productTypeId) { + // Same versioning rule as the admin UI and the API: never assume v1. + $nextVersion = ProcessTemplate::where('product_type_id', $productTypeId)->max('version') + 1; + + $template = ProcessTemplate::create([ + 'product_type_id' => $productTypeId, + 'name' => $validated['name'], + 'version' => $nextVersion, + 'is_active' => true, ]); - } - return $template; - }); + foreach ($validated['steps'] as $i => $stepData) { + TemplateStep::create([ + 'process_template_id' => $template->id, + 'step_number' => $i + 1, + 'name' => $stepData['name'], + 'estimated_duration_minutes' => $stepData['estimated_duration_minutes'] ?? null, + ]); + } + + return $template; + }); + } catch (UniqueConstraintViolationException $e) { + // The session guard above stops sequential replays, but two truly + // concurrent same-session POSTs (rapid double click) can both read an + // empty table and pick the same (product_type_id, version) — a + // FOR UPDATE lock can't serialise the first insert because there are + // no rows to lock yet. Rather than a cross-writer lock, treat the loser + // idempotently: a template now exists, so adopt the latest one and + // continue instead of surfacing a 500. + $template = ProcessTemplate::where('product_type_id', $productTypeId) + ->orderByDesc('version') + ->firstOrFail(); + } $request->session()->put('onboarding.template_id', $template->id); diff --git a/backend/tests/Feature/OnboardingWizardTest.php b/backend/tests/Feature/OnboardingWizardTest.php index a0cee0fb..14f49a48 100644 --- a/backend/tests/Feature/OnboardingWizardTest.php +++ b/backend/tests/Feature/OnboardingWizardTest.php @@ -119,16 +119,22 @@ public function test_step3_resubmit_does_not_create_duplicate_template(): void 'steps' => [['name' => 'Preparation', 'estimated_duration_minutes' => 10]], ]; - $this->actingAs($this->admin) + $first = $this->actingAs($this->admin) ->withSession(['onboarding.product_type_id' => $pt->id]) - ->post(route('onboarding.step3'), $payload) - ->assertRedirect(route('onboarding.step4')); + ->post(route('onboarding.step3'), $payload); + + $first->assertRedirect(route('onboarding.step4')) + ->assertSessionHas('onboarding.template_id'); + + // Reuse the exact template id the controller stashed in the session, so the + // replay mirrors the real browser state rather than a proxy lookup. + $templateId = $first->getSession()->get('onboarding.template_id'); // Same session replays the step (double click / browser back / Inertia retry). $this->actingAs($this->admin) ->withSession([ 'onboarding.product_type_id' => $pt->id, - 'onboarding.template_id' => ProcessTemplate::first()->id, + 'onboarding.template_id' => $templateId, ]) ->post(route('onboarding.step3'), $payload) ->assertRedirect(route('onboarding.step4')); @@ -137,6 +143,62 @@ public function test_step3_resubmit_does_not_create_duplicate_template(): void $this->assertEquals(1, ProcessTemplate::first()->steps()->count()); } + public function test_step3_first_template_for_product_type_gets_version_1(): void + { + $pt = ProductType::factory()->create(); + + $this->actingAs($this->admin) + ->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), [ + 'name' => 'First Process', + 'steps' => [['name' => 'Preparation', 'estimated_duration_minutes' => 10]], + ]) + ->assertRedirect(route('onboarding.step4')); + + $this->assertDatabaseHas('process_templates', [ + 'product_type_id' => $pt->id, + 'name' => 'First Process', + 'version' => 1, + ]); + } + + public function test_step3_validation_errors_on_empty_payload(): void + { + $pt = ProductType::factory()->create(); + + $this->actingAs($this->admin) + ->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), []) + ->assertSessionHasErrors(['name', 'steps']); + + $this->assertEquals(0, ProcessTemplate::count()); + } + + public function test_step3_is_forbidden_for_guests_and_non_admins(): void + { + $pt = ProductType::factory()->create(); + $payload = [ + 'name' => 'Assembly Process', + 'steps' => [['name' => 'Preparation']], + ]; + + // Guest → redirected to login, no template created. + $this->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), $payload) + ->assertRedirect(route('login')); + + // Authenticated but wrong role → 403 (onboarding is Admin-only). + $operator = User::factory()->create(); + $operator->assignRole('Operator'); + + $this->actingAs($operator) + ->withSession(['onboarding.product_type_id' => $pt->id]) + ->post(route('onboarding.step3'), $payload) + ->assertForbidden(); + + $this->assertEquals(0, ProcessTemplate::count()); + } + public function test_step3_assigns_next_version_when_product_type_already_has_template(): void { $pt = ProductType::factory()->create();