diff --git a/SPECIFICATION.md b/SPECIFICATION.md index b8a05a3..158222c 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -103,7 +103,14 @@ calendar forever, with an empty allowance and nothing to show. `GET /api/employees/search` rather than core's autocomplete, because only the server can tell a guest from a colleague. That endpoint wraps the same collaborator search, so the admin's user-enumeration settings still apply - exactly as elsewhere; guests are removed from what it returns. + exactly as elsewhere; guests are removed from what it returns. It does add one + person that search always withholds — **the searching user themselves**, whom the + collaborator search drops because it was built to answer "who can I share with". + Here the question is "whose absence is this?", and there you are a valid answer: + without it an HR member cannot record their own sick leave, since the dialog will + not submit without an employee and the self-service route offers only the + self-requestable types (§5.6). Returning you to yourself discloses nothing, so + this one result is not gated on enumeration settings. - Existing records for someone who later becomes a guest are left untouched in the database — they simply stop being listed. diff --git a/lib/Controller/EmployeeController.php b/lib/Controller/EmployeeController.php index 5633b64..29b9278 100644 --- a/lib/Controller/EmployeeController.php +++ b/lib/Controller/EmployeeController.php @@ -15,6 +15,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\Collaboration\Collaborators\ISearch; use OCP\IRequest; +use OCP\IUserManager; use OCP\Share\IShare; class EmployeeController extends Controller { @@ -23,6 +24,8 @@ class EmployeeController extends Controller { public function __construct( string $appName, IRequest $request, + private ?string $userId, + private IUserManager $userManager, private ISearch $collaboratorSearch, private EmployeeDirectory $employees, ) { @@ -42,6 +45,15 @@ public function __construct( * enumeration settings continue to apply exactly as they do everywhere else * — with enumeration restricted, that search returns nothing and so does * this. Guests are then removed from whatever it did return. + * + * The one thing that search gets wrong here is *you*: it drops the searching + * user from its own results ({@see \OC\Collaboration\Collaborators\UserPlugin}), + * because it was built to answer "who can I share with" and nobody shares with + * themselves. This picker asks a different question — "whose absence is this?" — + * and there you are a valid answer. Without this, an HR member could not record + * their own sick leave at all: the dialog refuses to submit until an employee is + * chosen, and the only other route, "New request", offers just the + * self-requestable types, which sick leave deliberately is not (§5.6). */ #[NoAdminRequired] #[UserRateLimit(limit: 60, period: 60)] @@ -62,6 +74,12 @@ public function search(string $search = '', int $limit = 20): DataResponse { ); $employees = []; + // First, because somebody typing their own name has matched it exactly and + // should not be pushed below looser matches on it. + $self = $this->matchingSelf($search); + if ($self !== null) { + $employees[$self['uid']] = $self; + } foreach ($candidates as $candidate) { $uid = (string)($candidate['value']['shareWith'] ?? ''); // Exact and wide matches overlap, so the same person can appear twice. @@ -82,4 +100,35 @@ public function search(string $search = '', int $limit = 20): DataResponse { return array_values($employees); }); } + + /** + * The signed-in user, when they match what was typed and may hold leave. + * + * Deliberately not gated on the admin's user-enumeration settings, unlike every + * other result: those exist to stop people discovering *colleagues* they have no + * business seeing, and returning you to yourself discloses nothing you do not + * already know. + * + * @return ?array{uid:string,displayName:string} + */ + private function matchingSelf(string $search): ?array { + $uid = (string)$this->userId; + if ($uid === '') { + return null; + } + $user = $this->userManager->get($uid); + // A guest is not an employee and takes no leave, even when it is their own + // account doing the searching (§2.2). + if ($user === null || !$this->employees->isEmployee($uid)) { + return null; + } + $needle = mb_strtolower($search); + $displayName = $user->getDisplayName(); + foreach ([$uid, $displayName] as $haystack) { + if (str_contains(mb_strtolower($haystack), $needle)) { + return ['uid' => $uid, 'displayName' => $displayName]; + } + } + return null; + } } diff --git a/tests/Unit/Controller/EmployeeControllerTest.php b/tests/Unit/Controller/EmployeeControllerTest.php new file mode 100644 index 0000000..fff7746 --- /dev/null +++ b/tests/Unit/Controller/EmployeeControllerTest.php @@ -0,0 +1,106 @@ +collaboratorSearch = $this->createMock(ISearch::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->employees = $this->createMock(EmployeeDirectory::class); + $this->controller = new EmployeeController( + 'absence', + $this->createMock(IRequest::class), + 'hr-user', + $this->userManager, + $this->collaboratorSearch, + $this->employees, + ); + } + + /** Core's collaborator search never returns the searching user. */ + private function collaboratorSearchReturns(array $users): void { + $this->collaboratorSearch->method('search')->willReturn([['exact' => ['users' => []], 'users' => $users], false]); + } + + private function selfIs(string $displayName): void { + $user = $this->createMock(IUser::class); + $user->method('getDisplayName')->willReturn($displayName); + $this->userManager->method('get')->with('hr-user')->willReturn($user); + } + + /** + * The reported bug: HR could not pick themselves in "Record absence", so they + * could not record their own sick leave — the dialog will not submit without an + * employee, and sick leave is not offered by the self-service route (§5.6). + */ + public function testSearchIncludesTheSigningInUserWhenTheyMatch(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + $data = $this->controller->search('frank')->getData(); + + self::assertSame([['uid' => 'hr-user', 'displayName' => 'Frank Karlitschek']], $data); + } + + public function testSearchMatchesTheSigningInUserByUid(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + self::assertSame('hr-user', $this->controller->search('hr-us')->getData()[0]['uid']); + } + + public function testSearchLeavesTheSigningInUserOutWhenTheyDoNotMatch(): void { + $this->collaboratorSearchReturns([ + ['label' => 'Lea Meyer', 'value' => ['shareWith' => 'lea']], + ]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + $data = $this->controller->search('lea')->getData(); + + self::assertSame([['uid' => 'lea', 'displayName' => 'Lea Meyer']], $data); + } + + public function testSearchNeverOffersTheSigningInUserWhenTheyAreAGuest(): void { + $this->collaboratorSearchReturns([]); + $this->selfIs('Guest Person'); + // Guests hold no entitlement and take no leave, their own account included. + $this->employees->method('isEmployee')->willReturn(false); + + self::assertSame([], $this->controller->search('guest')->getData()); + } + + public function testSearchDoesNotListTheSigningInUserTwice(): void { + // Should core ever stop filtering self out, the dedup must still hold. + $this->collaboratorSearchReturns([ + ['label' => 'Frank Karlitschek', 'value' => ['shareWith' => 'hr-user']], + ]); + $this->selfIs('Frank Karlitschek'); + $this->employees->method('isEmployee')->willReturn(true); + + self::assertCount(1, $this->controller->search('frank')->getData()); + } +}