From c13dfd66d6752ddc2433159aacecca89434c8194 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Wed, 7 Oct 2026 23:52:44 +0200 Subject: [PATCH 1/2] fix(search): let the searching user pick themselves as attendee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audience and organizer pickers query Nextcloud's sharee search, which always drops the searching user — nobody shares with themselves. Since 1.54.0 only the audience may answer an appointment, so a sole organizer who invites individuals could no longer add themselves and was locked out of responding: the web shows the "not an attendee" hint, older mobile builds surface the server error. The service now puts the searching user back when the query fits their user ID, display name or email: an exact hit leads the list, a partial hit closes it. The entry shape moves to ResponseDefinitions as AttendanceDirectoryEntry, which turns the inlined search schema into a named component in the generated specs. Two psalm baseline entries go because the typing fixed them. --- lib/Controller/AppointmentController.php | 5 +- lib/ResponseDefinitions.php | 1 + lib/Service/AppointmentService.php | 66 ++++++++++++++--- openapi-full.json | 53 ++++++------- openapi.json | 53 ++++++------- psalm-baseline.xml | 4 - ...ntControllerSearchUsersGroupsTeamsTest.php | 2 +- tests/unit/Service/AppointmentServiceTest.php | 74 +++++++++++++++++++ 8 files changed, 189 insertions(+), 69 deletions(-) diff --git a/lib/Controller/AppointmentController.php b/lib/Controller/AppointmentController.php index fffb7386..a1fe6c31 100644 --- a/lib/Controller/AppointmentController.php +++ b/lib/Controller/AppointmentController.php @@ -33,6 +33,7 @@ /** * @psalm-import-type AttendanceAppointmentPage from \OCA\Attendance\ResponseDefinitions + * @psalm-import-type AttendanceDirectoryEntry from \OCA\Attendance\ResponseDefinitions * @psalm-import-type AttendanceExportSeries from \OCA\Attendance\ResponseDefinitions */ class AppointmentController extends Controller { @@ -1460,7 +1461,7 @@ public function exportSeries(): DataResponse { * Search for users, groups, and teams * * @param string $search Search query - * @return DataResponse, array{}>|DataResponse|DataResponse|DataResponse + * @return DataResponse, array{}>|DataResponse|DataResponse|DataResponse */ #[NoAdminRequired] #[NoCSRFRequired] @@ -1481,7 +1482,7 @@ public function searchUsersGroupsTeams(string $search = ''): DataResponse { } try { - $results = $this->appointmentService->searchUsersGroupsTeams($search); + $results = $this->appointmentService->searchUsersGroupsTeams($search, $user->getUID()); return new DataResponse($results); } catch (\Exception $e) { return new DataResponse(['error' => $e->getMessage()], 400); diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index fa13ecf9..7d997ea5 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -170,6 +170,7 @@ * isAllDay?: bool, * } * @psalm-type AttendanceAudienceRef = array{id: string, label: string, type: string, isGuest?: bool} + * @psalm-type AttendanceDirectoryEntry = array{id: string, label: string, type: string, icon: string, isGuest: bool} * @psalm-type AttendanceCategoryTemplate = array{ * name: string, * description: string, diff --git a/lib/Service/AppointmentService.php b/lib/Service/AppointmentService.php index de82d968..82bea1e7 100644 --- a/lib/Service/AppointmentService.php +++ b/lib/Service/AppointmentService.php @@ -10,6 +10,7 @@ use OCA\Attendance\Db\AttendanceResponseMapper; use OCA\Attendance\Db\CategoryMapper; use OCA\Attendance\Db\DatetimeFormatTrait; +use OCA\Attendance\ResponseDefinitions; use OCP\App\IAppManager; use OCP\AppFramework\Db\DoesNotExistException; use OCP\Collaboration\Collaborators\ISearch as ICollaboratorSearch; @@ -22,6 +23,7 @@ * Delegates to specialized services for summary, visibility, and check-in operations. * * @psalm-type ListRow = array{appointment: Appointment, isAttendee: bool, isOrganizer: bool, isPast: bool} + * @psalm-import-type AttendanceDirectoryEntry from ResponseDefinitions */ class AppointmentService { use DatetimeFormatTrait; @@ -2123,8 +2125,10 @@ public function getUpcomingAppointmentsForWidget(string $userId, int $limit = 5) /** * Search for users, groups, and teams (circles). * Uses the ICollaboratorSearch interface to include all registered share types. + * + * @return list */ - public function searchUsersGroupsTeams(string $search = ''): array { + public function searchUsersGroupsTeams(string $search, string $currentUserId): array { $shareTypes = [ IShare::TYPE_USER, IShare::TYPE_GROUP, @@ -2146,11 +2150,57 @@ public function searchUsersGroupsTeams(string $search = ''): array { 0 ); - return $this->formatCollaboratorResults($searchResult); + return $this->withCurrentUser($this->formatCollaboratorResults($searchResult), $search, $currentUserId); + } + + /** + * Nobody shares with themselves, so the sharee search drops the searching + * user — but an organizer may well attend their own appointment. + * + * @param list $results + * @return list + */ + private function withCurrentUser(array $results, string $search, string $currentUserId): array { + $user = $this->userManager->get($currentUserId); + if ($user === null) { + return $results; + } + $needle = mb_strtolower(trim($search)); + $hits = array_filter( + array_map(mb_strtolower(...), [$user->getUID(), $user->getDisplayName(), $user->getEMailAddress() ?? '']), + static fn (string $name): bool => $name !== '' && str_contains($name, $needle), + ); + if ($hits === []) { + return $results; + } + foreach ($results as $result) { + if ($result['type'] === 'user' && $result['id'] === $user->getUID()) { + return $results; + } + } + + $self = $this->directoryEntry('user', $user->getUID(), $user->getDisplayName()); + // An exact hit leads, like the sharee search's own exact matches do. + return in_array($needle, $hits, true) ? [$self, ...$results] : [...$results, $self]; + } + + /** + * @return AttendanceDirectoryEntry + */ + private function directoryEntry(string $type, string $id, string $label): array { + return [ + 'id' => $id, + 'label' => $label, + 'type' => $type, + 'icon' => $this->getIconForType($type), + 'isGuest' => $type === 'user' && $id !== '' && $this->guestService->isGuestUser($id), + ]; } /** * Format collaborator search results into a consistent format. + * + * @return list */ private function formatCollaboratorResults($searchResult): array { $results = []; @@ -2181,16 +2231,8 @@ private function formatCollaboratorResults($searchResult): array { continue; } - $id = $item['value']['shareWith'] ?? $item['shareWith'] ?? ''; - $results[] = [ - 'id' => $id, - 'label' => $item['label'] ?? '', - 'type' => $type, - 'icon' => $this->getIconForType($type), - 'isGuest' => $type === 'user' && $id !== '' - ? $this->guestService->isGuestUser((string)$id) - : false, - ]; + $id = (string)($item['value']['shareWith'] ?? $item['shareWith'] ?? ''); + $results[] = $this->directoryEntry($type, $id, (string)($item['label'] ?? '')); } } diff --git a/openapi-full.json b/openapi-full.json index 8ffff55a..33374ed4 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -1129,6 +1129,33 @@ } } }, + "DirectoryEntry": { + "type": "object", + "required": [ + "id", + "label", + "type", + "icon", + "isGuest" + ], + "properties": { + "id": { + "type": "string" + }, + "label": { + "type": "string" + }, + "type": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "isGuest": { + "type": "boolean" + } + } + }, "ExportResult": { "type": "object", "required": [ @@ -7320,31 +7347,7 @@ "schema": { "type": "array", "items": { - "type": "object", - "required": [ - "id", - "label", - "type", - "icon", - "isGuest" - ], - "properties": { - "id": { - "type": "string" - }, - "label": { - "type": "string" - }, - "type": { - "type": "string" - }, - "icon": { - "type": "string" - }, - "isGuest": { - "type": "boolean" - } - } + "$ref": "#/components/schemas/DirectoryEntry" } } } diff --git a/openapi.json b/openapi.json index 0d191c1b..61ed95d2 100644 --- a/openapi.json +++ b/openapi.json @@ -879,6 +879,33 @@ } } }, + "DirectoryEntry": { + "type": "object", + "required": [ + "id", + "label", + "type", + "icon", + "isGuest" + ], + "properties": { + "id": { + "type": "string" + }, + "label": { + "type": "string" + }, + "type": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "isGuest": { + "type": "boolean" + } + } + }, "ExportResult": { "type": "object", "required": [ @@ -6955,31 +6982,7 @@ "schema": { "type": "array", "items": { - "type": "object", - "required": [ - "id", - "label", - "type", - "icon", - "isGuest" - ], - "properties": { - "id": { - "type": "string" - }, - "label": { - "type": "string" - }, - "type": { - "type": "string" - }, - "icon": { - "type": "string" - }, - "isGuest": { - "type": "boolean" - } - } + "$ref": "#/components/schemas/DirectoryEntry" } } } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 27ec954c..d9fc01db 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1432,7 +1432,6 @@ - @@ -1443,9 +1442,6 @@ - - - diff --git a/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php b/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php index ce90efcf..5b61ddd0 100644 --- a/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php +++ b/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php @@ -78,7 +78,7 @@ public function testAdminWithNoAppointmentRoleCanStillSearch(): void { $this->permissionService->method('isAdmin')->with('admin')->willReturn(true); $this->permissionService->method('canCreateAppointments')->willReturn(false); $this->appointmentService->method('isOrganizerAnywhere')->willReturn(false); - $this->appointmentService->method('searchUsersGroupsTeams')->with('acme')->willReturn([]); + $this->appointmentService->method('searchUsersGroupsTeams')->with('acme', 'admin')->willReturn([]); $response = $this->controller->searchUsersGroupsTeams('acme'); diff --git a/tests/unit/Service/AppointmentServiceTest.php b/tests/unit/Service/AppointmentServiceTest.php index 1cbd2040..e411cc4e 100644 --- a/tests/unit/Service/AppointmentServiceTest.php +++ b/tests/unit/Service/AppointmentServiceTest.php @@ -39,6 +39,7 @@ use OCP\IDateTimeZone; use OCP\IGroupManager; use OCP\IUserManager; +use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -2556,4 +2557,77 @@ public function testNavigationStopsReadingPastAppointmentsOnceThePageIsFull(): v $this->assertSame([201, 202], array_column($navigation['past'], 'id')); $this->assertFalse($navigation['pastHasMore']); } + + private function searchesAs(string $uid, string $displayName, ?string $email = null): void { + $user = $this->createMock(\OCP\IUser::class); + $user->method('getUID')->willReturn($uid); + $user->method('getDisplayName')->willReturn($displayName); + $user->method('getEMailAddress')->willReturn($email); + $this->userManager->method('get')->with($uid)->willReturn($user); + } + + /** @param list $uids */ + private function shareeSearchFinds(array $uids): void { + $users = array_map(static fn (string $uid): array => [ + 'label' => $uid, + 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => $uid], + ], $uids); + $this->collaboratorSearch->method('search')->willReturn([['users' => $users], false]); + } + + public function testSearchAddsTheSearchingUserWhenTheQueryFitsThem(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['larissa']); + + $results = $this->service->searchUsersGroupsTeams('lar', 'lars'); + + $this->assertSame(['larissa', 'lars'], array_column($results, 'id')); + $this->assertSame('Lars Müller', $results[1]['label']); + $this->assertSame('user', $results[1]['type']); + $this->assertFalse($results[1]['isGuest']); + } + + public function testSearchPutsAnExactHitOnTheSearchingUserFirst(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['larissa']); + + $results = $this->service->searchUsersGroupsTeams('Lars Müller', 'lars'); + + $this->assertSame(['lars', 'larissa'], array_column($results, 'id')); + } + + public function testSearchMatchesTheSearchingUserByEmail(): void { + $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); + $this->shareeSearchFinds([]); + + $results = $this->service->searchUsersGroupsTeams('example.org', 'lars'); + + $this->assertSame(['lars'], array_column($results, 'id')); + } + + public function testSearchLeavesTheSearchingUserOutWhenTheQueryDoesNotFit(): void { + $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); + $this->shareeSearchFinds(['christel']); + + $results = $this->service->searchUsersGroupsTeams('chris', 'lars'); + + $this->assertSame(['christel'], array_column($results, 'id')); + } + + public function testSearchDoesNotDuplicateASearchingUserTheShareeSearchAlreadyLists(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['lars']); + + $results = $this->service->searchUsersGroupsTeams('lars', 'lars'); + + $this->assertSame(['lars'], array_column($results, 'id')); + } + + public function testSearchIgnoresASearcherTheUserManagerDoesNotKnow(): void { + $this->shareeSearchFinds(['christel']); + + $results = $this->service->searchUsersGroupsTeams('chris', 'ghost'); + + $this->assertSame(['christel'], array_column($results, 'id')); + } } From 4b3d99fb4318a27e2cc777ead72c444f3c15628e Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Thu, 8 Oct 2026 00:09:38 +0200 Subject: [PATCH 2/2] refactor(search): move the directory search into its own service The users/groups/teams search depended on nothing appointment-related, yet lived in the 2600-line AppointmentService with its tests wired through that class's two dozen mocks. DirectorySearchService now owns it, with AppointmentController as its only caller. The move sheds what the old home had carried along: a dead ISearchResult branch on a method whose interface documents an array, an unused $hasMore, and comments restating the code. The psalm baseline entries that still apply moved with the code; the ones the typing fixed are gone. --- lib/Controller/AppointmentController.php | 6 +- lib/Service/AppointmentService.php | 163 ----------------- lib/Service/DirectorySearchService.php | 164 ++++++++++++++++++ psalm-baseline.xml | 52 +++--- ...ntControllerSearchUsersGroupsTeamsTest.php | 9 +- .../AppointmentControllerUserConfigTest.php | 2 + tests/unit/Service/AppointmentServiceTest.php | 86 --------- .../Service/DirectorySearchServiceTest.php | 107 ++++++++++++ 8 files changed, 311 insertions(+), 278 deletions(-) create mode 100644 lib/Service/DirectorySearchService.php create mode 100644 tests/unit/Service/DirectorySearchServiceTest.php diff --git a/lib/Controller/AppointmentController.php b/lib/Controller/AppointmentController.php index a1fe6c31..adc3e227 100644 --- a/lib/Controller/AppointmentController.php +++ b/lib/Controller/AppointmentController.php @@ -12,6 +12,7 @@ use OCA\Attendance\Service\CheckinService; use OCA\Attendance\Service\ConfigService; use OCA\Attendance\Service\DeadlineUpdate; +use OCA\Attendance\Service\DirectorySearchService; use OCA\Attendance\Service\ExportOptions; use OCA\Attendance\Service\ExportService; use OCA\Attendance\Service\GuestService; @@ -52,6 +53,7 @@ class AppointmentController extends Controller { private GuestService $guestService; private BookingService $bookingService; private TalkRoomService $talkRoomService; + private DirectorySearchService $directorySearchService; public function __construct( string $appName, @@ -71,6 +73,7 @@ public function __construct( GuestService $guestService, BookingService $bookingService, TalkRoomService $talkRoomService, + DirectorySearchService $directorySearchService, ) { parent::__construct($appName, $request); $this->appointmentService = $appointmentService; @@ -88,6 +91,7 @@ public function __construct( $this->guestService = $guestService; $this->bookingService = $bookingService; $this->talkRoomService = $talkRoomService; + $this->directorySearchService = $directorySearchService; } /** @@ -1482,7 +1486,7 @@ public function searchUsersGroupsTeams(string $search = ''): DataResponse { } try { - $results = $this->appointmentService->searchUsersGroupsTeams($search, $user->getUID()); + $results = $this->directorySearchService->search($search, $user->getUID()); return new DataResponse($results); } catch (\Exception $e) { return new DataResponse(['error' => $e->getMessage()], 400); diff --git a/lib/Service/AppointmentService.php b/lib/Service/AppointmentService.php index 82bea1e7..7fe74164 100644 --- a/lib/Service/AppointmentService.php +++ b/lib/Service/AppointmentService.php @@ -10,20 +10,15 @@ use OCA\Attendance\Db\AttendanceResponseMapper; use OCA\Attendance\Db\CategoryMapper; use OCA\Attendance\Db\DatetimeFormatTrait; -use OCA\Attendance\ResponseDefinitions; -use OCP\App\IAppManager; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\Collaboration\Collaborators\ISearch as ICollaboratorSearch; use OCP\IGroupManager; use OCP\IUserManager; -use OCP\Share\IShare; /** * Core service for managing appointments and responses. * Delegates to specialized services for summary, visibility, and check-in operations. * * @psalm-type ListRow = array{appointment: Appointment, isAttendee: bool, isOrganizer: bool, isPast: bool} - * @psalm-import-type AttendanceDirectoryEntry from ResponseDefinitions */ class AppointmentService { use DatetimeFormatTrait; @@ -50,8 +45,6 @@ class AppointmentService { private ResponseSummaryService $responseSummaryService; private NotificationService $notificationService; private AttachmentService $attachmentService; - private ICollaboratorSearch $collaboratorSearch; - private IAppManager $appManager; private GuestService $guestService; private AuditEventService $auditEventService; private BookingService $bookingService; @@ -76,8 +69,6 @@ public function __construct( ResponseSummaryService $responseSummaryService, NotificationService $notificationService, AttachmentService $attachmentService, - ICollaboratorSearch $collaboratorSearch, - IAppManager $appManager, GuestService $guestService, AuditEventService $auditEventService, BookingService $bookingService, @@ -101,8 +92,6 @@ public function __construct( $this->responseSummaryService = $responseSummaryService; $this->notificationService = $notificationService; $this->attachmentService = $attachmentService; - $this->collaboratorSearch = $collaboratorSearch; - $this->appManager = $appManager; $this->guestService = $guestService; $this->auditEventService = $auditEventService; $this->bookingService = $bookingService; @@ -2122,158 +2111,6 @@ public function getUpcomingAppointmentsForWidget(string $userId, int $limit = 5) return $result; } - /** - * Search for users, groups, and teams (circles). - * Uses the ICollaboratorSearch interface to include all registered share types. - * - * @return list - */ - public function searchUsersGroupsTeams(string $search, string $currentUserId): array { - $shareTypes = [ - IShare::TYPE_USER, - IShare::TYPE_GROUP, - ]; - - // Add circles/teams if the app is enabled - if ($this->appManager->isEnabledForUser('circles')) { - $shareTypes[] = IShare::TYPE_CIRCLE; - } - - [$searchResult, $hasMore] = $this->collaboratorSearch->search( - $search, - $shareTypes, - false, // lookup - // Bound the page so an empty/very-short query on a large directory - // can't fan out to every user/group/team. 200 leaves plenty of - // headroom for a typical org while keeping the response bounded. - 200, - 0 - ); - - return $this->withCurrentUser($this->formatCollaboratorResults($searchResult), $search, $currentUserId); - } - - /** - * Nobody shares with themselves, so the sharee search drops the searching - * user — but an organizer may well attend their own appointment. - * - * @param list $results - * @return list - */ - private function withCurrentUser(array $results, string $search, string $currentUserId): array { - $user = $this->userManager->get($currentUserId); - if ($user === null) { - return $results; - } - $needle = mb_strtolower(trim($search)); - $hits = array_filter( - array_map(mb_strtolower(...), [$user->getUID(), $user->getDisplayName(), $user->getEMailAddress() ?? '']), - static fn (string $name): bool => $name !== '' && str_contains($name, $needle), - ); - if ($hits === []) { - return $results; - } - foreach ($results as $result) { - if ($result['type'] === 'user' && $result['id'] === $user->getUID()) { - return $results; - } - } - - $self = $this->directoryEntry('user', $user->getUID(), $user->getDisplayName()); - // An exact hit leads, like the sharee search's own exact matches do. - return in_array($needle, $hits, true) ? [$self, ...$results] : [...$results, $self]; - } - - /** - * @return AttendanceDirectoryEntry - */ - private function directoryEntry(string $type, string $id, string $label): array { - return [ - 'id' => $id, - 'label' => $label, - 'type' => $type, - 'icon' => $this->getIconForType($type), - 'isGuest' => $type === 'user' && $id !== '' && $this->guestService->isGuestUser($id), - ]; - } - - /** - * Format collaborator search results into a consistent format. - * - * @return list - */ - private function formatCollaboratorResults($searchResult): array { - $results = []; - // Handle both ISearchResult object and array returns - $resultData = is_array($searchResult) ? $searchResult : $searchResult->asArray(); - - // Process exact matches and regular matches - foreach (['exact', 'users', 'groups', 'circles'] as $category) { - $items = []; - - if ($category === 'exact') { - // Exact matches are nested by type - $exactMatches = $resultData['exact'] ?? []; - foreach (['users', 'groups', 'circles'] as $subCategory) { - if (isset($exactMatches[$subCategory])) { - $items = array_merge($items, $exactMatches[$subCategory]); - } - } - } else { - $items = $resultData[$category] ?? []; - } - - foreach ($items as $item) { - $shareType = $item['value']['shareType'] ?? null; - $type = $this->mapShareTypeToString($shareType); - - if ($type === null) { - continue; - } - - $id = (string)($item['value']['shareWith'] ?? $item['shareWith'] ?? ''); - $results[] = $this->directoryEntry($type, $id, (string)($item['label'] ?? '')); - } - } - - // Remove duplicates based on id + type - $seen = []; - $uniqueResults = []; - foreach ($results as $result) { - $key = $result['type'] . ':' . $result['id']; - if (!isset($seen[$key])) { - $seen[$key] = true; - $uniqueResults[] = $result; - } - } - - return $uniqueResults; - } - - /** - * Map share type integer to string type. - */ - private function mapShareTypeToString(?int $shareType): ?string { - return match ($shareType) { - IShare::TYPE_USER => 'user', - IShare::TYPE_GROUP => 'group', - IShare::TYPE_CIRCLE => 'team', - default => null, - }; - } - - /** - * Get icon class for a given type. - */ - private function getIconForType(string $type): string { - return match ($type) { - 'user' => 'icon-user', - 'group' => 'icon-group', - 'team' => 'icon-team', - default => 'icon-user', - }; - } - /** * Get user IDs of users who have not responded to an appointment. * diff --git a/lib/Service/DirectorySearchService.php b/lib/Service/DirectorySearchService.php new file mode 100644 index 00000000..a5ef7063 --- /dev/null +++ b/lib/Service/DirectorySearchService.php @@ -0,0 +1,164 @@ + + */ + public function search(string $query, string $currentUserId): array { + $shareTypes = [ + IShare::TYPE_USER, + IShare::TYPE_GROUP, + ]; + + if ($this->appManager->isEnabledForUser('circles')) { + $shareTypes[] = IShare::TYPE_CIRCLE; + } + + [$searchResult] = $this->collaboratorSearch->search( + $query, + $shareTypes, + false, // lookup + // Bound the page so an empty/very-short query on a large directory + // can't fan out to every user/group/team. 200 leaves plenty of + // headroom for a typical org while keeping the response bounded. + 200, + 0 + ); + + // ISearch::search() is untyped; its first element is the result page as an array. + $page = is_array($searchResult) ? $searchResult : []; + return $this->withCurrentUser($this->formatCollaboratorResults($page), $query, $currentUserId); + } + + /** + * Nobody shares with themselves, so the sharee search drops the searching + * user — but an organizer may well attend their own appointment. + * + * @param list $results + * @return list + */ + private function withCurrentUser(array $results, string $query, string $currentUserId): array { + $user = $this->userManager->get($currentUserId); + if ($user === null) { + return $results; + } + $needle = mb_strtolower(trim($query)); + $hits = array_filter( + array_map(mb_strtolower(...), [$user->getUID(), $user->getDisplayName(), $user->getEMailAddress() ?? '']), + static fn (string $name): bool => $name !== '' && str_contains($name, $needle), + ); + if ($hits === []) { + return $results; + } + foreach ($results as $result) { + if ($result['type'] === 'user' && $result['id'] === $user->getUID()) { + return $results; + } + } + + $self = $this->directoryEntry('user', $user->getUID(), $user->getDisplayName()); + // An exact hit leads, like the sharee search's own exact matches do. + return in_array($needle, $hits, true) ? [$self, ...$results] : [...$results, $self]; + } + + /** + * @return AttendanceDirectoryEntry + */ + private function directoryEntry(string $type, string $id, string $label): array { + return [ + 'id' => $id, + 'label' => $label, + 'type' => $type, + 'icon' => $this->getIconForType($type), + 'isGuest' => $type === 'user' && $id !== '' && $this->guestService->isGuestUser($id), + ]; + } + + /** + * @return list + */ + private function formatCollaboratorResults(array $searchResult): array { + $results = []; + foreach (['exact', 'users', 'groups', 'circles'] as $category) { + $items = []; + + if ($category === 'exact') { + // Exact matches are nested by type + $exactMatches = $searchResult['exact'] ?? []; + foreach (['users', 'groups', 'circles'] as $subCategory) { + if (isset($exactMatches[$subCategory])) { + $items = array_merge($items, $exactMatches[$subCategory]); + } + } + } else { + $items = $searchResult[$category] ?? []; + } + + foreach ($items as $item) { + $shareType = $item['value']['shareType'] ?? null; + $type = $this->mapShareTypeToString($shareType); + + if ($type === null) { + continue; + } + + $id = (string)($item['value']['shareWith'] ?? $item['shareWith'] ?? ''); + $results[] = $this->directoryEntry($type, $id, (string)($item['label'] ?? '')); + } + } + + $seen = []; + $uniqueResults = []; + foreach ($results as $result) { + $key = $result['type'] . ':' . $result['id']; + if (!isset($seen[$key])) { + $seen[$key] = true; + $uniqueResults[] = $result; + } + } + + return $uniqueResults; + } + + private function mapShareTypeToString(?int $shareType): ?string { + return match ($shareType) { + IShare::TYPE_USER => 'user', + IShare::TYPE_GROUP => 'group', + IShare::TYPE_CIRCLE => 'team', + default => null, + }; + } + + private function getIconForType(string $type): string { + return match ($type) { + 'user' => 'icon-user', + 'group' => 'icon-group', + 'team' => 'icon-team', + default => 'icon-user', + }; + } +} diff --git a/psalm-baseline.xml b/psalm-baseline.xml index d9fc01db..3f519f68 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1409,39 +1409,16 @@ - - - - - - - - - - - - - - - - - - - - - - - @@ -1504,9 +1481,6 @@ - - - @@ -1745,6 +1719,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php b/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php index 5b61ddd0..42cf01bb 100644 --- a/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php +++ b/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php @@ -11,6 +11,7 @@ use OCA\Attendance\Service\CalendarService; use OCA\Attendance\Service\CheckinService; use OCA\Attendance\Service\ConfigService; +use OCA\Attendance\Service\DirectorySearchService; use OCA\Attendance\Service\ExportService; use OCA\Attendance\Service\GuestService; use OCA\Attendance\Service\NotificationService; @@ -38,6 +39,8 @@ class AppointmentControllerSearchUsersGroupsTeamsTest extends TestCase { private $permissionService; /** @var IUserSession|MockObject */ private $userSession; + /** @var DirectorySearchService|MockObject */ + private $directorySearchService; private AppointmentController $controller; @@ -45,6 +48,7 @@ protected function setUp(): void { $this->appointmentService = $this->createMock(AppointmentService::class); $this->permissionService = $this->createMock(PermissionService::class); $this->userSession = $this->createMock(IUserSession::class); + $this->directorySearchService = $this->createMock(DirectorySearchService::class); $this->controller = new AppointmentController( 'attendance', @@ -64,6 +68,7 @@ protected function setUp(): void { $this->createMock(GuestService::class), $this->createMock(BookingService::class), $this->createMock(TalkRoomService::class), + $this->directorySearchService, ); } @@ -78,7 +83,7 @@ public function testAdminWithNoAppointmentRoleCanStillSearch(): void { $this->permissionService->method('isAdmin')->with('admin')->willReturn(true); $this->permissionService->method('canCreateAppointments')->willReturn(false); $this->appointmentService->method('isOrganizerAnywhere')->willReturn(false); - $this->appointmentService->method('searchUsersGroupsTeams')->with('acme', 'admin')->willReturn([]); + $this->directorySearchService->method('search')->with('acme', 'admin')->willReturn([]); $response = $this->controller->searchUsersGroupsTeams('acme'); @@ -90,7 +95,7 @@ public function testUserWithNoRoleAtAllIsRejected(): void { $this->permissionService->method('isAdmin')->willReturn(false); $this->permissionService->method('canCreateAppointments')->willReturn(false); $this->appointmentService->method('isOrganizerAnywhere')->willReturn(false); - $this->appointmentService->expects($this->never())->method('searchUsersGroupsTeams'); + $this->directorySearchService->expects($this->never())->method('search'); $response = $this->controller->searchUsersGroupsTeams('acme'); diff --git a/tests/unit/Controller/AppointmentControllerUserConfigTest.php b/tests/unit/Controller/AppointmentControllerUserConfigTest.php index 1ecd36ce..c36c94a2 100644 --- a/tests/unit/Controller/AppointmentControllerUserConfigTest.php +++ b/tests/unit/Controller/AppointmentControllerUserConfigTest.php @@ -11,6 +11,7 @@ use OCA\Attendance\Service\CalendarService; use OCA\Attendance\Service\CheckinService; use OCA\Attendance\Service\ConfigService; +use OCA\Attendance\Service\DirectorySearchService; use OCA\Attendance\Service\ExportService; use OCA\Attendance\Service\GuestService; use OCA\Attendance\Service\NotificationService; @@ -65,6 +66,7 @@ protected function setUp(): void { $this->createMock(GuestService::class), $this->createMock(BookingService::class), $this->createMock(TalkRoomService::class), + $this->createMock(DirectorySearchService::class), ); $this->configService->method('getDisplayOrder')->willReturn('name_first'); diff --git a/tests/unit/Service/AppointmentServiceTest.php b/tests/unit/Service/AppointmentServiceTest.php index e411cc4e..e981c33f 100644 --- a/tests/unit/Service/AppointmentServiceTest.php +++ b/tests/unit/Service/AppointmentServiceTest.php @@ -32,14 +32,11 @@ use OCA\Attendance\Service\TimezoneService; use OCA\Attendance\Service\VacationService; use OCA\Attendance\Service\VisibilityService; -use OCP\App\IAppManager; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\Collaboration\Collaborators\ISearch as ICollaboratorSearch; use OCP\Config\IUserConfig; use OCP\IDateTimeZone; use OCP\IGroupManager; use OCP\IUserManager; -use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -71,12 +68,6 @@ class AppointmentServiceTest extends TestCase { /** @var AttachmentService|MockObject */ private $attachmentService; - /** @var ICollaboratorSearch|MockObject */ - private $collaboratorSearch; - - /** @var IAppManager|MockObject */ - private $appManager; - /** @var GuestService|MockObject */ private $guestService; @@ -115,8 +106,6 @@ protected function setUp(): void { $this->responseSummaryService = $this->createMock(ResponseSummaryService::class); $this->notificationService = $this->createMock(NotificationService::class); $this->attachmentService = $this->createMock(AttachmentService::class); - $this->collaboratorSearch = $this->createMock(ICollaboratorSearch::class); - $this->appManager = $this->createMock(IAppManager::class); $this->guestService = $this->createMock(GuestService::class); $this->auditEventService = $this->createMock(AuditEventService::class); $this->bookingService = $this->createMock(BookingService::class); @@ -150,8 +139,6 @@ protected function setUp(): void { $this->responseSummaryService, $this->notificationService, $this->attachmentService, - $this->collaboratorSearch, - $this->appManager, $this->guestService, $this->auditEventService, $this->bookingService, @@ -2557,77 +2544,4 @@ public function testNavigationStopsReadingPastAppointmentsOnceThePageIsFull(): v $this->assertSame([201, 202], array_column($navigation['past'], 'id')); $this->assertFalse($navigation['pastHasMore']); } - - private function searchesAs(string $uid, string $displayName, ?string $email = null): void { - $user = $this->createMock(\OCP\IUser::class); - $user->method('getUID')->willReturn($uid); - $user->method('getDisplayName')->willReturn($displayName); - $user->method('getEMailAddress')->willReturn($email); - $this->userManager->method('get')->with($uid)->willReturn($user); - } - - /** @param list $uids */ - private function shareeSearchFinds(array $uids): void { - $users = array_map(static fn (string $uid): array => [ - 'label' => $uid, - 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => $uid], - ], $uids); - $this->collaboratorSearch->method('search')->willReturn([['users' => $users], false]); - } - - public function testSearchAddsTheSearchingUserWhenTheQueryFitsThem(): void { - $this->searchesAs('lars', 'Lars Müller'); - $this->shareeSearchFinds(['larissa']); - - $results = $this->service->searchUsersGroupsTeams('lar', 'lars'); - - $this->assertSame(['larissa', 'lars'], array_column($results, 'id')); - $this->assertSame('Lars Müller', $results[1]['label']); - $this->assertSame('user', $results[1]['type']); - $this->assertFalse($results[1]['isGuest']); - } - - public function testSearchPutsAnExactHitOnTheSearchingUserFirst(): void { - $this->searchesAs('lars', 'Lars Müller'); - $this->shareeSearchFinds(['larissa']); - - $results = $this->service->searchUsersGroupsTeams('Lars Müller', 'lars'); - - $this->assertSame(['lars', 'larissa'], array_column($results, 'id')); - } - - public function testSearchMatchesTheSearchingUserByEmail(): void { - $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); - $this->shareeSearchFinds([]); - - $results = $this->service->searchUsersGroupsTeams('example.org', 'lars'); - - $this->assertSame(['lars'], array_column($results, 'id')); - } - - public function testSearchLeavesTheSearchingUserOutWhenTheQueryDoesNotFit(): void { - $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); - $this->shareeSearchFinds(['christel']); - - $results = $this->service->searchUsersGroupsTeams('chris', 'lars'); - - $this->assertSame(['christel'], array_column($results, 'id')); - } - - public function testSearchDoesNotDuplicateASearchingUserTheShareeSearchAlreadyLists(): void { - $this->searchesAs('lars', 'Lars Müller'); - $this->shareeSearchFinds(['lars']); - - $results = $this->service->searchUsersGroupsTeams('lars', 'lars'); - - $this->assertSame(['lars'], array_column($results, 'id')); - } - - public function testSearchIgnoresASearcherTheUserManagerDoesNotKnow(): void { - $this->shareeSearchFinds(['christel']); - - $results = $this->service->searchUsersGroupsTeams('chris', 'ghost'); - - $this->assertSame(['christel'], array_column($results, 'id')); - } } diff --git a/tests/unit/Service/DirectorySearchServiceTest.php b/tests/unit/Service/DirectorySearchServiceTest.php new file mode 100644 index 00000000..333a590d --- /dev/null +++ b/tests/unit/Service/DirectorySearchServiceTest.php @@ -0,0 +1,107 @@ +collaboratorSearch = $this->createMock(ICollaboratorSearch::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->service = new DirectorySearchService( + $this->collaboratorSearch, + $this->createMock(IAppManager::class), + $this->userManager, + $this->createMock(GuestService::class), + ); + } + + private function searchesAs(string $uid, string $displayName, ?string $email = null): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + $user->method('getDisplayName')->willReturn($displayName); + $user->method('getEMailAddress')->willReturn($email); + $this->userManager->method('get')->with($uid)->willReturn($user); + } + + /** @param list $uids */ + private function shareeSearchFinds(array $uids): void { + $users = array_map(static fn (string $uid): array => [ + 'label' => $uid, + 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => $uid], + ], $uids); + $this->collaboratorSearch->method('search')->willReturn([['users' => $users], false]); + } + + public function testSearchAddsTheSearchingUserWhenTheQueryFitsThem(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['larissa']); + + $results = $this->service->search('lar', 'lars'); + + $this->assertSame(['larissa', 'lars'], array_column($results, 'id')); + $this->assertSame('Lars Müller', $results[1]['label']); + $this->assertSame('user', $results[1]['type']); + $this->assertFalse($results[1]['isGuest']); + } + + public function testSearchPutsAnExactHitOnTheSearchingUserFirst(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['larissa']); + + $results = $this->service->search('Lars Müller', 'lars'); + + $this->assertSame(['lars', 'larissa'], array_column($results, 'id')); + } + + public function testSearchMatchesTheSearchingUserByEmail(): void { + $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); + $this->shareeSearchFinds([]); + + $results = $this->service->search('example.org', 'lars'); + + $this->assertSame(['lars'], array_column($results, 'id')); + } + + public function testSearchLeavesTheSearchingUserOutWhenTheQueryDoesNotFit(): void { + $this->searchesAs('lars', 'Lars Müller', 'lars@example.org'); + $this->shareeSearchFinds(['christel']); + + $results = $this->service->search('chris', 'lars'); + + $this->assertSame(['christel'], array_column($results, 'id')); + } + + public function testSearchDoesNotDuplicateASearchingUserTheShareeSearchAlreadyLists(): void { + $this->searchesAs('lars', 'Lars Müller'); + $this->shareeSearchFinds(['lars']); + + $results = $this->service->search('lars', 'lars'); + + $this->assertSame(['lars'], array_column($results, 'id')); + } + + public function testSearchIgnoresASearcherTheUserManagerDoesNotKnow(): void { + $this->shareeSearchFinds(['christel']); + + $results = $this->service->search('chris', 'ghost'); + + $this->assertSame(['christel'], array_column($results, 'id')); + } +}