Skip to content

fix(onboarding): stop step 3 from crashing on resubmit #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jakub-przepiora merged 2 commits into from
Jul 31, 2026
+152 −14
Merged

fix(onboarding): stop step 3 from crashing on resubmit #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75c6fb3
fix(onboarding): stop step 3 from crashing on resubmit
jakub-przepiora Jul 30, 2026
f8b33b9
fix(onboarding): make step 3 template creation race-safe + widen tests
jakub-przepiora Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions backend/app/Http/Controllers/Web/OnboardingController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -119,6 +120,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');
}

Comment on lines +123 to +129

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect Step 3 route blocking and every ProcessTemplate creation/version path.
fd -t f -e php . backend |
  xargs rg -n -C3 'onboarding\.step3|storeStep3|->block\(|ProcessTemplate::create|lockForUpdate\(|max\(.version.'

Repository: Mes-Open/OpenMes

Length of output: 23398

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "## Relevant controller snippets"
sed -n '70,170p' backend/app/Http/Controllers/Web/OnboardingController.php | cat -n
echo
sed -n '35,85p' backend/app/Http/Controllers/Web/Admin/ProcessTemplateManagementController.php | cat -n
echo
sed -n '40,55p' backend/app/Http/Controllers/Api/V1/ProcessTemplateController.php | cat -n

echo
echo "## Model migrations and ProcessTemplate schema"
fd -t f '.*\.php$' backend/database/migrations | sed -n '1,40p'
rg -n -C3 "create_process_templates|product_types|process_templates|unique|product_type_id.*version|version.*product_type_id" backend/database/migrations frontend backend -g '*.php' | sed -n '1,220p'

echo
echo "## Route/session blocking patterns"
rg -n -C2 "Route::block|csrf|middleware|web\(\)|StoreRequests|Form Request|Onboarding.*Step3|storeStep3" backend/routes backend/app backend/tests -g '*.php'

Repository: Mes-Open/OpenMes

Length of output: 33996

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "## Behavioral probe of the current guard ordering"
python3 - <<'PY'
from dataclasses import dataclass
from datetime import datetime
from enum import Enum

class State(Enum):
    SESSION_ABSENT = 0
    SESSION_PRESENT = 1
    TEMPLATE_CREATED = 2

`@dataclass`
class RequestState:
    session_present_before_guard: bool
    max_version_before_alias_compute: int = 1
    commit_time: float = 0.0

def process3(r: RequestState):
    # guard read
    if r.session_present_before_guard:
        return "redirect_step4"
    # transactional alias compute uses database value before this request writes
    next_version = r.max_version_before_alias_compute + 1
    # request commits unique insert after this line
    r.session_present_before_guard = True
    r.commit_time = datetime.now().timestamp()
    return next_version

# Concurrent same-session submissions without route/session lock.
r1, r2 = RequestState(False), RequestState(False)
v1, v2 = process3(r1), process3(r2)
print("r1_result", v1, "r2_result", v2, "same_version", v1 == v2)

# Sequential after one replay guard works.
r1, r2 = RequestState(False), RequestState(True)
v1, r_state_after_r1 = process3(r1), True
r2.session_present_before_guard = r_state_after_r1
v2 = process3(r2)
print("r1_result", v1, "r2_result", v2, "same_version", v1 == v2)
PY

Repository: Mes-Open/OpenMes

Length of output: 302

Make the Step 3 replay guard and template creation atomic.

The guard reads the session before the transaction writes it, so two concurrent same-session POSTs can both select the same MAX(version) + 1, and the duplicate insert violates the unique (product_type_id, version) index. Block the Step 3 POST route/session before the guard, and coordinate the Onboarding/API/template-allocator writers against a shared product-type lock so no future writer bypasses this protection.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/Http/Controllers/Web/OnboardingController.php` around lines 122 -
128, The Step 3 replay guard and template creation must be atomic: acquire the
shared per-product-type lock before checking onboarding.template_id, then
perform version allocation, template insertion, and session update while holding
it. Apply the same lock to Onboarding, API, and template-allocator writers, and
ensure the Step 3 POST route/session is blocked before the guard so concurrent
requests cannot bypass this protection.

$validated = $request->validate([
'name' => 'required|string|max:255',
'steps' => 'required|array|min:1',
Expand All @@ -128,20 +136,40 @@ 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,
]);

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,
]);

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);
Expand Down
110 changes: 110 additions & 0 deletions backend/tests/Feature/OnboardingWizardTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,116 @@ 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]],
];

$first = $this->actingAs($this->admin)
->withSession(['onboarding.product_type_id' => $pt->id])
->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' => $templateId,
])
->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_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();
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());
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

public function test_step4_creates_work_order(): void
{
$line = Line::factory()->create();
Expand Down
Loading