diff --git a/lib/Controller/AppointmentController.php b/lib/Controller/AppointmentController.php index fffb7386..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; @@ -33,6 +34,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 { @@ -51,6 +53,7 @@ class AppointmentController extends Controller { private GuestService $guestService; private BookingService $bookingService; private TalkRoomService $talkRoomService; + private DirectorySearchService $directorySearchService; public function __construct( string $appName, @@ -70,6 +73,7 @@ public function __construct( GuestService $guestService, BookingService $bookingService, TalkRoomService $talkRoomService, + DirectorySearchService $directorySearchService, ) { parent::__construct($appName, $request); $this->appointmentService = $appointmentService; @@ -87,6 +91,7 @@ public function __construct( $this->guestService = $guestService; $this->bookingService = $bookingService; $this->talkRoomService = $talkRoomService; + $this->directorySearchService = $directorySearchService; } /** @@ -1460,7 +1465,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 +1486,7 @@ public function searchUsersGroupsTeams(string $search = ''): DataResponse { } try { - $results = $this->appointmentService->searchUsersGroupsTeams($search); + $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/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..7fe74164 100644 --- a/lib/Service/AppointmentService.php +++ b/lib/Service/AppointmentService.php @@ -10,12 +10,9 @@ use OCA\Attendance\Db\AttendanceResponseMapper; use OCA\Attendance\Db\CategoryMapper; use OCA\Attendance\Db\DatetimeFormatTrait; -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. @@ -48,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; @@ -74,8 +69,6 @@ public function __construct( ResponseSummaryService $responseSummaryService, NotificationService $notificationService, AttachmentService $attachmentService, - ICollaboratorSearch $collaboratorSearch, - IAppManager $appManager, GuestService $guestService, AuditEventService $auditEventService, BookingService $bookingService, @@ -99,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; @@ -2120,118 +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. - */ - public function searchUsersGroupsTeams(string $search = ''): 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->formatCollaboratorResults($searchResult); - } - - /** - * Format collaborator search results into a consistent format. - */ - 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 = $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, - ]; - } - } - - // 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/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..3f519f68 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1409,43 +1409,16 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -1508,9 +1481,6 @@ - - - @@ -1749,6 +1719,32 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php b/tests/unit/Controller/AppointmentControllerSearchUsersGroupsTeamsTest.php index ce90efcf..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')->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 1cbd2040..e981c33f 100644 --- a/tests/unit/Service/AppointmentServiceTest.php +++ b/tests/unit/Service/AppointmentServiceTest.php @@ -32,9 +32,7 @@ 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; @@ -70,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; @@ -114,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); @@ -149,8 +139,6 @@ protected function setUp(): void { $this->responseSummaryService, $this->notificationService, $this->attachmentService, - $this->collaboratorSearch, - $this->appManager, $this->guestService, $this->auditEventService, $this->bookingService, 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')); + } +}