fa/breadcrumb-the-shire
1
0
Fork 1
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor: fix 7 CRUD system inconsistencies for robustness and maintainability

Browse Source 
- Add user-assignment dependency check to DepartmentService.deleteByUuid (409 when users assigned)
- Standardize POST detection to isMethod('POST') in 10 create/edit actions
- Align RoleService code uniqueness to warning pattern (matching departments)
- Normalize repository create() return types from mixed to int|false (3 repos + 3 interfaces)
- Add JSON response branches to 4 non-user delete actions
- Document delete authorization ordering pattern
- Decompose AdminSettingsService.updateFromAdmin into domain-scoped private methods

Workflow: .agents/runs/CRUD-CONSISTENCY-001/
Reviewed by: Code Reviewer (pass), Security Reviewer (pass), Acceptance Tester (pass)
Quality gates: QG-001 PHPUnit pass, QG-002 PHPStan pass, QG-003 Architecture pass

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-03-22 18:25:33 +01:00
parent 984d0430d3
commit c429ff43cd
29 changed files with 495 additions and 207 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
tests/Service/Access/RoleServiceTest.php
View File

@@ -66,6 +66,41 @@ class RoleServiceTest extends TestCase
$this->assertTrue($result['ok']);
$this->assertSame('uuid-42', $result['uuid']);
$this->assertSame(42, $result['id']);
$this->assertEmpty($result['warnings']);
}
public function testCreateFromAdminReturnsDuplicateCodeAsWarning(): void
{
$this->roleRepo
->expects($this->any())
->method('existsByCode')
->with('DUPE', 0)
->willReturn(true);
$this->roleRepo
->expects($this->any())
->method('create')
->willReturn(50);
$this->roleRepo
->expects($this->any())
->method('find')
->with(50)
->willReturn(['id' => 50, 'uuid' => 'uuid-50', 'description' => 'Dupe Role']);
$this->auditService
->expects($this->once())
->method('record');
$result = $this->service->createFromAdmin([
'description' => 'Dupe Role',
'code' => 'DUPE',
'active' => 1,
], 1);
$this->assertTrue($result['ok']);
$this->assertNotEmpty($result['warnings']);
$this->assertEmpty($result['errors'] ?? []);
}
public function testCreateFromAdminRejectsEmptyDescription(): void
@@ -184,7 +219,7 @@ class RoleServiceTest extends TestCase
$this->assertTrue($result['ok']);
}
public function testUpdateFromAdminRejectsDuplicateCode(): void
public function testUpdateFromAdminReturnsDuplicateCodeAsWarning(): void
{
$this->roleRepo
->expects($this->once())
@@ -193,17 +228,30 @@ class RoleServiceTest extends TestCase
->willReturn(true);
$this->roleRepo
->expects($this->never())
->method('update');
->expects($this->once())
->method('find')
->with(10)
->willReturn(['id' => 10, 'uuid' => 'uuid-10', 'active' => 1]);
$this->roleRepo
->expects($this->once())
->method('update')
->with(10, $this->anything())
->willReturn(true);
$this->auditService
->expects($this->once())
->method('record');
$result = $this->service->updateFromAdmin(10, [
'description' => 'Manager',
'code' => 'ADMIN',
'active' => 1,
]);
], 5);
$this->assertFalse($result['ok']);
$this->assertNotEmpty($result['errors']);
$this->assertTrue($result['ok']);
$this->assertNotEmpty($result['warnings']);
$this->assertEmpty($result['errors'] ?? []);
}
public function testUpdateFromAdminReturnsErrorWhenRepositoryUpdateFails(): void

74
tests/Service/Org/DepartmentServiceTest.php
View File

@@ -207,6 +207,10 @@ class DepartmentServiceTest extends TestCase
->with('abc-123')
->willReturn($department);
$userDeptRepo = $this->createMock(UserDepartmentRepositoryInterface::class);
$userDeptRepo->expects($this->any())->method('countUsersByDepartmentIds')->with([5])->willReturn([]);
$this->userServicesFactory->expects($this->any())->method('createUserDepartmentRepository')->willReturn($userDeptRepo);
$this->departmentRepository
->expects($this->once())
->method('delete')
@@ -263,6 +267,10 @@ class DepartmentServiceTest extends TestCase
->with('abc-999')
->willReturn(['id' => 9, 'uuid' => 'abc-999', 'description' => 'Ops']);
$userDeptRepo = $this->createMock(UserDepartmentRepositoryInterface::class);
$userDeptRepo->expects($this->any())->method('countUsersByDepartmentIds')->with([9])->willReturn([]);
$this->userServicesFactory->expects($this->any())->method('createUserDepartmentRepository')->willReturn($userDeptRepo);
$this->departmentRepository
->expects($this->once())
->method('delete')
@@ -280,6 +288,72 @@ class DepartmentServiceTest extends TestCase
$this->assertSame('delete_failed', $result['error']);
}
public function testDeleteByUuidReturns409WhenUsersAssigned(): void
{
$this->departmentRepository
->expects($this->once())
->method('findByUuid')
->with('abc-dept')
->willReturn(['id' => 5, 'uuid' => 'abc-dept', 'description' => 'Sales']);
$userDeptRepo = $this->createMock(UserDepartmentRepositoryInterface::class);
$userDeptRepo->expects($this->once())
->method('countUsersByDepartmentIds')
->with([5])
->willReturn([5 => 3]);
$this->userServicesFactory->expects($this->once())
->method('createUserDepartmentRepository')
->willReturn($userDeptRepo);
$this->departmentRepository
->expects($this->never())
->method('delete');
$this->systemAuditService
->expects($this->never())
->method('record');
$result = $this->service->deleteByUuid('abc-dept');
$this->assertFalse($result['ok']);
$this->assertSame(409, $result['status']);
$this->assertSame('department_has_users', $result['error']);
$this->assertSame(3, $result['user_count']);
}
public function testDeleteByUuidDeletesWhenNoUsersAssigned(): void
{
$this->departmentRepository
->expects($this->once())
->method('findByUuid')
->with('abc-empty')
->willReturn(['id' => 7, 'uuid' => 'abc-empty', 'description' => 'Empty Dept']);
$userDeptRepo = $this->createMock(UserDepartmentRepositoryInterface::class);
$userDeptRepo->expects($this->once())
->method('countUsersByDepartmentIds')
->with([7])
->willReturn([]);
$this->userServicesFactory->expects($this->once())
->method('createUserDepartmentRepository')
->willReturn($userDeptRepo);
$this->departmentRepository
->expects($this->once())
->method('delete')
->with(7)
->willReturn(true);
$this->systemAuditService
->expects($this->once())
->method('record');
$result = $this->service->deleteByUuid('abc-empty');
$this->assertTrue($result['ok']);
$this->assertSame('abc-empty', $result['department']['uuid']);
}
public function testSyncTenantsUsesFirstId(): void
{
$this->departmentRepository