Fix duplicate category name validation (#364)
## Summary - validate category names against the existing per-user unique index - return validation errors for create/update duplicate races instead of 500s - cover create, update, soft-delete, cross-user, and DB race cases Fixes PHP-LARAVEL-1E Fixes PHP-LARAVEL-1F ## Tests - php artisan test --compact tests/Feature/Settings/CategoryTest.php - vendor/bin/pint --dirty --format agent - git diff --check
This commit is contained in:
parent
5784e25f0a
commit
e3c2d2fd82
|
|
@ -6,8 +6,10 @@ use App\Http\Controllers\Controller;
|
|||
use App\Http\Requests\Settings\StoreCategoryRequest;
|
||||
use App\Http\Requests\Settings\UpdateCategoryRequest;
|
||||
use App\Models\Category;
|
||||
use Illuminate\Database\UniqueConstraintViolationException;
|
||||
use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
|
||||
use Illuminate\Http\RedirectResponse;
|
||||
use Illuminate\Validation\ValidationException;
|
||||
use Inertia\Inertia;
|
||||
use Inertia\Response;
|
||||
|
||||
|
|
@ -35,7 +37,11 @@ class CategoryController extends Controller
|
|||
*/
|
||||
public function store(StoreCategoryRequest $request): RedirectResponse
|
||||
{
|
||||
auth()->user()->categories()->create($request->validated());
|
||||
try {
|
||||
auth()->user()->categories()->create($request->validated());
|
||||
} catch (UniqueConstraintViolationException $exception) {
|
||||
$this->throwDuplicateCategoryNameValidationException($exception);
|
||||
}
|
||||
|
||||
return to_route('categories.index');
|
||||
}
|
||||
|
|
@ -47,7 +53,11 @@ class CategoryController extends Controller
|
|||
{
|
||||
$this->authorize('update', $category);
|
||||
|
||||
$category->update($request->validated());
|
||||
try {
|
||||
$category->update($request->validated());
|
||||
} catch (UniqueConstraintViolationException $exception) {
|
||||
$this->throwDuplicateCategoryNameValidationException($exception);
|
||||
}
|
||||
|
||||
return to_route('categories.index');
|
||||
}
|
||||
|
|
@ -63,4 +73,15 @@ class CategoryController extends Controller
|
|||
|
||||
return to_route('categories.index');
|
||||
}
|
||||
|
||||
private function throwDuplicateCategoryNameValidationException(UniqueConstraintViolationException $exception): never
|
||||
{
|
||||
if (! str_contains($exception->getMessage(), 'categories_user_id_name_unique')) {
|
||||
throw $exception;
|
||||
}
|
||||
|
||||
throw ValidationException::withMessages([
|
||||
'name' => __('validation.unique', ['attribute' => 'name']),
|
||||
]);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -42,7 +42,13 @@ class StoreCategoryRequest extends FormRequest
|
|||
public function rules(): array
|
||||
{
|
||||
return [
|
||||
'name' => ['required', 'string', 'max:255'],
|
||||
'name' => [
|
||||
'required',
|
||||
'string',
|
||||
'max:255',
|
||||
Rule::unique('categories', 'name')
|
||||
->where('user_id', auth()->id()),
|
||||
],
|
||||
'icon' => ['required', 'string'],
|
||||
'color' => [
|
||||
'required',
|
||||
|
|
|
|||
|
|
@ -42,7 +42,14 @@ class UpdateCategoryRequest extends FormRequest
|
|||
public function rules(): array
|
||||
{
|
||||
return [
|
||||
'name' => ['required', 'string', 'max:255'],
|
||||
'name' => [
|
||||
'required',
|
||||
'string',
|
||||
'max:255',
|
||||
Rule::unique('categories', 'name')
|
||||
->where('user_id', auth()->id())
|
||||
->ignore($this->route('category')),
|
||||
],
|
||||
'icon' => ['required', 'string'],
|
||||
'color' => [
|
||||
'required',
|
||||
|
|
|
|||
|
|
@ -3,10 +3,13 @@
|
|||
use App\Actions\CreateDefaultCategories;
|
||||
use App\Enums\CategoryCashflowDirection;
|
||||
use App\Enums\CategoryType;
|
||||
use App\Http\Controllers\Settings\CategoryController;
|
||||
use App\Http\Requests\Settings\StoreCategoryRequest;
|
||||
use App\Models\Category;
|
||||
use App\Models\User;
|
||||
use Illuminate\Database\UniqueConstraintViolationException;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Illuminate\Validation\ValidationException;
|
||||
|
||||
beforeEach(function () {
|
||||
config(['landing.hide_auth_buttons' => false]);
|
||||
|
|
@ -80,6 +83,72 @@ test('category name is required', function () {
|
|||
$response->assertSessionHasErrors(['name']);
|
||||
});
|
||||
|
||||
test('category names must be unique for each user when creating', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
Category::factory()->create([
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Healthcare',
|
||||
]);
|
||||
|
||||
$response = $this->actingAs($user)->post(route('categories.store'), [
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]);
|
||||
|
||||
$response->assertSessionHasErrors(['name']);
|
||||
|
||||
expect($user->categories()->where('name', 'Healthcare')->count())->toBe(1);
|
||||
});
|
||||
|
||||
test('different users can create categories with the same name', function () {
|
||||
$user = User::factory()->create();
|
||||
$otherUser = User::factory()->create();
|
||||
|
||||
Category::factory()->create([
|
||||
'user_id' => $otherUser->id,
|
||||
'name' => 'Healthcare',
|
||||
]);
|
||||
|
||||
$response = $this->actingAs($user)->post(route('categories.store'), [
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]);
|
||||
|
||||
$response->assertRedirect(route('categories.index'));
|
||||
|
||||
$this->assertDatabaseHas('categories', [
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Healthcare',
|
||||
]);
|
||||
});
|
||||
|
||||
test('category names from deleted categories remain reserved', function () {
|
||||
$user = User::factory()->create();
|
||||
$category = Category::factory()->create([
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Healthcare',
|
||||
]);
|
||||
|
||||
$category->delete();
|
||||
|
||||
$response = $this->actingAs($user)->post(route('categories.store'), [
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]);
|
||||
|
||||
$response->assertSessionHasErrors(['name']);
|
||||
});
|
||||
|
||||
test('category icon is required', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
|
|
@ -171,6 +240,64 @@ test('authenticated users can update their own category', function () {
|
|||
]);
|
||||
});
|
||||
|
||||
test('users can keep their category name when updating', function () {
|
||||
$user = User::factory()->create();
|
||||
$category = Category::factory()->create([
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Healthcare',
|
||||
'type' => 'expense',
|
||||
]);
|
||||
|
||||
$response = $this->actingAs($user)->patch(
|
||||
route('categories.update', $category),
|
||||
[
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]
|
||||
);
|
||||
|
||||
$response->assertRedirect(route('categories.index'));
|
||||
|
||||
$this->assertDatabaseHas('categories', [
|
||||
'id' => $category->id,
|
||||
'name' => 'Healthcare',
|
||||
'color' => 'pink',
|
||||
]);
|
||||
});
|
||||
|
||||
test('category names must be unique for each user when updating', function () {
|
||||
$user = User::factory()->create();
|
||||
$category = Category::factory()->create([
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Old Name',
|
||||
]);
|
||||
Category::factory()->create([
|
||||
'user_id' => $user->id,
|
||||
'name' => 'Healthcare',
|
||||
]);
|
||||
|
||||
$response = $this->actingAs($user)->patch(
|
||||
route('categories.update', $category),
|
||||
[
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]
|
||||
);
|
||||
|
||||
$response->assertSessionHasErrors(['name']);
|
||||
|
||||
$this->assertDatabaseHas('categories', [
|
||||
'id' => $category->id,
|
||||
'name' => 'Old Name',
|
||||
]);
|
||||
});
|
||||
|
||||
test('transfer categories can store a cashflow direction', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
|
|
@ -324,6 +451,40 @@ test('default categories are created without repeated category lookups', functio
|
|||
->and($categorySelects)->toBe(1);
|
||||
});
|
||||
|
||||
test('duplicate category name database errors become validation errors', function () {
|
||||
$user = User::factory()->create();
|
||||
$controller = new CategoryController;
|
||||
|
||||
$user->categories()->create([
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
]);
|
||||
|
||||
$request = Mockery::mock(StoreCategoryRequest::class);
|
||||
$request->shouldReceive('validated')->once()->andReturn([
|
||||
'name' => 'Healthcare',
|
||||
'icon' => 'Heart',
|
||||
'color' => 'pink',
|
||||
'type' => 'expense',
|
||||
'cashflow_direction' => 'hidden',
|
||||
]);
|
||||
|
||||
$this->actingAs($user);
|
||||
|
||||
$thrown = null;
|
||||
|
||||
try {
|
||||
$controller->store($request);
|
||||
} catch (ValidationException $exception) {
|
||||
$thrown = $exception;
|
||||
}
|
||||
|
||||
expect($thrown)->toBeInstanceOf(ValidationException::class);
|
||||
expect($thrown->errors())->toHaveKey('name');
|
||||
});
|
||||
|
||||
test('category names are unique per user', function () {
|
||||
$user = User::factory()->create();
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue