fix(onboarding): stop step 3 from crashing on resubmit - #217
Open
jakub-przepiora wants to merge 1 commit into
Open
fix(onboarding): stop step 3 from crashing on resubmit#217jakub-przepiora wants to merge 1 commit into
jakub-przepiora wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
📝 WalkthroughWalkthroughStep 3 now skips duplicate template creation when a template ID exists in session, creates templates transactionally with incremented product-type versions, and adds feature tests for replay prevention and version assignment. ChangesOnboarding template flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Step3Form
participant OnboardingController
participant Session
participant Database
Step3Form->>OnboardingController: Submit Step 3
OnboardingController->>Session: Read onboarding.template_id
alt Template already exists
Session-->>OnboardingController: Existing template ID
OnboardingController-->>Step3Form: Redirect to onboarding.step4
else No existing template
OnboardingController->>Database: Read max version for product_type_id
OnboardingController->>Database: Create template and steps transactionally
Database-->>OnboardingController: Created template
OnboardingController->>Session: Store template ID
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/app/Http/Controllers/Web/OnboardingController.php`:
- Around line 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.
In `@backend/tests/Feature/OnboardingWizardTest.php`:
- Around line 113-159: Expand Step 3 contract coverage in OnboardingWizardTest:
capture onboarding.template_id from the first successful POST response/session
state and reuse it for the duplicate submission instead of reading
ProcessTemplate::first(). Add tests for validation-422, guest access, and
unauthorized-role access, and add the no-existing-template case asserting the
created template receives version 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04be96b8-362b-441d-b30c-1c59af62a3ae
📒 Files selected for processing (2)
backend/app/Http/Controllers/Web/OnboardingController.phpbackend/tests/Feature/OnboardingWizardTest.php
Comment on lines
+122
to
+128
| // 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'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ 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)
PYRepository: 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.
Comment on lines
+113
to
+159
| 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()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^frontend|backend/)' | rg 'OnboardingWizardTest|Onboarding|onboarding|ProcessTemplate|ProductType' | sed -n '1,200p'
echo
echo "== OnboardingWizardTest outline/lines =="
if [ -f backend/tests/Feature/OnboardingWizardTest.php ]; then
wc -l backend/tests/Feature/OnboardingWizardTest.php
sed -n '1,220p' backend/tests/Feature/OnboardingWizardTest.php | cat -n
else
fd -i 'OnboardingWizardTest\.php' .
fi
echo
echo "== search step3 route/controller and ProcessTemplate =="
rg -n "step3|step[34]|onboarding/template|ProcessTemplate|product_type_id|onboarding\\." backend frontend -S | sed -n '1,260p'Repository: Mes-Open/OpenMes
Length of output: 46376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OnboardingController step3 implementation =="
sed -n '110,165p' backend/app/Http/Controllers/Web/OnboardingController.php | cat -n
echo
echo "== Step 3 tests beyond existing =="
sed -n '220,328p' backend/tests/Feature/OnboardingWizardTest.php | cat -n
echo
echo "== process template factory =="
sed -n '1,80p' backend/database/factories/ProcessTemplateFactory.php | cat -n
echo
echo "== process template model =="
sed -n '1,180p' backend/app/Models/ProcessTemplate.php | cat -n
echo
echo "== onboarding routes =="
rg -n "Route::|onboarding\\.|step3|step4" backend/routes -S | sed -n '1,220p'
echo
echo "== behavioral probe from source text =="
python3 - <<'PY'
from pathlib import Path
src = Path('backend/app/Http/Controllers/Web/OnboardingController.php').read_text()
test = Path('backend/tests/Feature/OnboardingWizardTest.php').read_text()
checks = {
'step3_stores_template_id': "\$request->session()->put('onboarding.template_id');" in src,
'test_replay_fetches_from_db': "ProcessTemplate::first()->id" in test and "\$request->session()->has('onboarding.template_id')" not in test.split("test_step3_resubmit_does_not_create_duplicate_template")[1].split("test_step3_assigns_next_version")[0],
'test_asserts_step3_session_session_state': "\$response->withSession" in test or ".assertSessionHas('onboarding.template_id'" in test,
'test_missing_validation_422': "\$response->assertStatus(422)" not in test and "assertStatus(422)" not in test,
'test_missing_guest_authorization': ".actingAsGuest()" not in test and "guest())" not in test,
'test_missing_wrong_role': "wrong role" not in test.lower() and "wrong-role" not in test.lower(),
'test_missing_no_existing_template_version_1': "version' => 1" in test and "\$pt = ProductType::factory()->create();" in test and "ProcessTemplate::factory()->create([" not in test.split("no existing template", 1)[1] if "no existing template" in test.lower() else False,
}
for k, v in checks.items():
print(f"{k}={v}")
start_step3_create = test.index("test_step3_creates_template_with_steps")
step3_block = test[start_step3_create:test.index("test_step4_creates_work_order")]
print("step3_block_processes_no_existing_template_to_version_one=", "version' => 1" in step3_block and "ProcessTemplate::factory()->create([" not in step3_block and "assertEquals(1, ProcessTemplate::where" not in step3_block)
PYRepository: Mes-Open/OpenMes
Length of output: 48857
Complete Step 3 contract coverage.
Store onboarding.template_id during the first successful POST and replay the duplicate-submit case through that response state instead of deriving it from the database. Add Step 3 validation-422, guest, and wrong-role tests, and cover the no-existing-template path that should assign version 1.
🧰 Tools
🪛 PHPStan (2.2.6)
[error] 115-115: Call to an undefined static method App\Models\ProductType::factory().
(staticMethod.notFound)
[error] 131-131: Call to an undefined static method App\Models\ProcessTemplate::first().
(staticMethod.notFound)
[error] 136-136: Call to an undefined static method App\Models\ProcessTemplate::count().
(staticMethod.notFound)
[error] 137-137: Call to an undefined static method App\Models\ProcessTemplate::first().
(staticMethod.notFound)
[error] 142-142: Call to an undefined static method App\Models\ProductType::factory().
(staticMethod.notFound)
[error] 143-143: Call to an undefined static method App\Models\ProcessTemplate::factory().
(staticMethod.notFound)
[error] 158-158: Call to an undefined static method App\Models\ProcessTemplate::where().
(staticMethod.notFound)
🤖 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/tests/Feature/OnboardingWizardTest.php` around lines 113 - 159,
Expand Step 3 contract coverage in OnboardingWizardTest: capture
onboarding.template_id from the first successful POST response/session state and
reuse it for the duplicate submission instead of reading
ProcessTemplate::first(). Add tests for validation-422, guest access, and
unauthorized-role access, and add the no-existing-template case asserting the
created template receives version 1.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
POST /onboarding/step/3returns a 500 whenever the step is submitted twice:storeStep3()created the template with a hardcodedversion => 1through a plaincreate(). The first insert already satisfies the(product_type_id, version)unique index, so any replay of the step blows up:The session still holds
onboarding.product_type_idafter step 3 (onlytemplate_idis added), so nothing stopped the controller from inserting a second row for the same product type.Worth noting this was the only place in the codebase assuming v1 —
ProcessTemplateManagementController::storeandApi\V1\ProcessTemplateController::storeboth already derive the version withmax('version') + 1, and the demo seeders useupdateOrCreate. Onboarding was the odd one out.Fix
onboarding.template_id, so a replay is a no-op instead of a second insert.max('version') + 1, matching the admin UI and the API.TemplateSteprows left an orphaned template behind.Tests
Two regression tests added to
OnboardingWizardTest, both verified to fail onmainand pass with this change:test_step3_resubmit_does_not_create_duplicate_template— replays the step in the same session, asserts exactly one template and one steptest_step3_assigns_next_version_when_product_type_already_has_template— pre-existing v1 template, asserts the new one lands on v2 rather than throwingFull suite green locally: 1975 tests, 6284 assertions. Pint clean.
Impact
Not demo-specific — any installation running the onboarding wizard is affected. It surfaces most often on demo instances because every new tenant is routed through the wizard, so the double-submit window is hit far more often.
Summary by CodeRabbit