From b335faf035c984f1428c34133671d962e87a603f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 19 Dec 2024 07:47:16 +0100 Subject: [PATCH] fix(search): Paginate unified search results Signed-off-by: Joas Schilling --- lib/Search/ConversationSearch.php | 69 ++++++++++++++++-- .../features/bootstrap/FeatureContext.php | 72 ++++++++++++++----- .../features/conversation-5/search.feature | 34 +++++++++ 3 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 tests/integration/features/conversation-5/search.feature diff --git a/lib/Search/ConversationSearch.php b/lib/Search/ConversationSearch.php index a745c295b25..d92923509a6 100644 --- a/lib/Search/ConversationSearch.php +++ b/lib/Search/ConversationSearch.php @@ -66,11 +66,17 @@ public function getOrder(string $route, array $routeParameters): ?int { } /** + * Search for user's conversations + * + * Cursor is the conversation token + * Results are sorted by display name and then conversation token + * * @inheritDoc */ public function search(IUser $user, ISearchQuery $query): SearchResult { $rooms = $this->manager->getRoomsForUser($user->getUID()); + $cursorKey = null; $result = []; foreach ($rooms as $room) { if ($room->getType() === Room::TYPE_CHANGELOG) { @@ -82,10 +88,11 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $parameters['token'] === $room->getToken() && str_starts_with($query->getRoute(), Application::APP_ID . '.')) { // Don't search the current conversation. - //User most likely looks for other things with the same name + // User most likely looks for other things with the same name continue; } + $displayName = $room->getDisplayName($user->getUID()); if ($room->getType() === Room::TYPE_ONE_TO_ONE || $room->getType() === Room::TYPE_ONE_TO_ONE_FORMER) { $otherUserId = str_replace( json_encode($user->getUID()), @@ -93,7 +100,7 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $room->getName() ); if (stripos($otherUserId, $query->getTerm()) === false - && stripos($room->getDisplayName($user->getUID()), $query->getTerm()) === false) { + && stripos($displayName, $query->getTerm()) === false) { // Neither name nor displayname (one-to-one) match, skip continue; } @@ -103,7 +110,7 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $entry = new SearchResultEntry( $this->avatarService->getAvatarUrl($room), - $room->getDisplayName($user->getUID()), + $displayName, '', $this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]), '', @@ -112,12 +119,62 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $entry->addAttribute('conversation', $room->getToken()); - $result[] = $entry; + $result[strtolower($displayName . '#' . $room->getToken())] = $entry; + + if ($query->getCursor() === $room->getToken()) { + $cursorKey = strtolower($displayName . '#' . $room->getToken()); + } + } + + if (count($result) <= $query->getLimit()) { + return SearchResult::complete( + $this->l->t('Conversations'), + array_values($result), + ); } + ksort($result); + + $newCursorWithName = '#'; + if ($cursorKey) { + $foundCursor = false; + $filteredResults = []; + $lastPossibleCursor = '#'; + foreach ($result as $key => $entry) { + if ($cursorKey === $key) { + $foundCursor = true; + continue; + } + if (!$foundCursor) { + continue; + } + + if (count($filteredResults) === $query->getLimit()) { + // We already have enough results, but there are more, + // so we add the cursor for the next request. + $newCursorWithName = $lastPossibleCursor; + break; + } + + $filteredResults[] = $entry; + $lastPossibleCursor = $key; + } + } else { + $filteredResults = array_slice($result, 0, $query->getLimit()); + // Next page starts at the last result + $newCursorWithName = array_key_last($filteredResults); + } + + // Cursor is the token only (to survive renamed), + // but the array key is `display name#token`, so we split by the # + // and get the last part which is the token. + // If it's empty, there is no cursor for a next page + $parts = explode('#', $newCursorWithName); + $newCursor = end($parts); - return SearchResult::complete( + return SearchResult::paginated( $this->l->t('Conversations'), - $result + array_values($filteredResults), + $newCursor ); } } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index a0519a2dd6e..956955665fa 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -3237,31 +3237,71 @@ protected function compareDataResponse(?TableNode $formData = null) { /** * @param TableNode|null $formData */ - protected function compareSearchResponse(?TableNode $formData = null) { - $messages = $this->getDataFromResponse($this->response)['entries']; + protected function compareSearchResponse(?TableNode $formData = null, ?string $expectedCursor = null) { + $data = $this->getDataFromResponse($this->response); + $results = $data['entries']; + + if ($expectedCursor !== null) { + Assert::assertSame($expectedCursor, $data['cursor']); + } if ($formData === null) { - Assert::assertEmpty($messages); + Assert::assertEmpty($results); return; } - $expected = array_map(static function (array $message) { - $message['attributes.conversation'] = self::$identifierToToken[$message['attributes.conversation']]; - $message['attributes.messageId'] = self::$textToMessageId[$message['attributes.messageId']]; - return $message; + $expected = array_map(static function (array $result) { + if (isset($result['attributes.conversation'])) { + $result['attributes.conversation'] = self::$identifierToToken[$result['attributes.conversation']]; + } + if (isset($result['attributes.messageId'])) { + $result['attributes.messageId'] = self::$textToMessageId[$result['attributes.messageId']]; + } + return $result; }, $formData->getHash()); $count = count($expected); - Assert::assertCount($count, $messages, 'Message count does not match'); - - Assert::assertEquals($expected, array_map(static function ($message) { - return [ - 'title' => $message['title'], - 'subline' => $message['subline'], - 'attributes.conversation' => $message['attributes']['conversation'], - 'attributes.messageId' => $message['attributes']['messageId'], + Assert::assertCount($count, $results, 'Result count does not match'); + + Assert::assertEquals($expected, array_map(static function ($actual) { + $compare = [ + 'title' => $actual['title'], + 'subline' => $actual['subline'], ]; - }, $messages)); + if (isset($actual['attributes']['conversation'])) { + $compare['attributes.conversation'] = $actual['attributes']['conversation']; + } + if (isset($actual['attributes']['messageId'])) { + $compare['attributes.messageId'] = $actual['attributes']['messageId']; + } + return $compare; + }, $results)); + } + + /** + * @Then /^user "([^"]*)" searches for conversations with "([^"]*)"(?: offset "([^"]*)")? limit (\d+)(?: expected cursor "([^"]*)")?$/ + * + * @param string $user + * @param string $search + * @param int $limit + */ + public function userSearchesRooms(string $user, string $search, string $offset, int $limit, string $expectedCursor, ?TableNode $formData = null): void { + $searchUrl = '/search/providers/talk-conversations/search?limit=' . $limit; + if ($offset && array_key_exists($offset, self::$identifierToToken)) { + $searchUrl .= '&cursor=' . self::$identifierToToken[$offset]; + } + + $searchUrl .= '&term=' . $search; + + $this->setCurrentUser($user); + $this->sendRequest('GET', $searchUrl); + $this->assertStatusCode($this->response, 200); + + if ($expectedCursor !== null) { + $expectedCursor = self::$identifierToToken[$expectedCursor] ?? ''; + } + + $this->compareSearchResponse($formData, $expectedCursor); } /** diff --git a/tests/integration/features/conversation-5/search.feature b/tests/integration/features/conversation-5/search.feature new file mode 100644 index 00000000000..a3143136507 --- /dev/null +++ b/tests/integration/features/conversation-5/search.feature @@ -0,0 +1,34 @@ +Feature: conversation-5/search + + Background: + Given user "participant1" exists + + Scenario: Search for conversations with cursor and limit + Given user "participant1" creates room "Room 1" (v4) + | roomType | 2 | + | roomName | Room 1 | + Given user "participant1" creates room "Room 2" (v4) + | roomType | 2 | + | roomName | room 2 | + Given user "participant1" creates room "Room 3" (v4) + | roomType | 2 | + | roomName | ROOM 3 | + Given user "participant1" creates room "Room 4" (v4) + | roomType | 2 | + | roomName | Room 4 | + Given user "participant1" creates room "Room 5" (v4) + | roomType | 2 | + | roomName | Room 5 | + Given user "participant1" creates room "Room 6" (v4) + | roomType | 2 | + | roomName | Room 6 | + And user "participant1" searches for conversations with "o" limit 1 expected cursor "Room 1" + | title | subline | attributes.conversation | + | Room 1 | | Room 1 | + And user "participant1" searches for conversations with "o" offset "Room 4" limit 1 expected cursor "Room 5" + | title | subline | attributes.conversation | + | Room 5 | | Room 5 | + And user "participant1" searches for conversations with "o" offset "Room 4" limit 5 expected cursor "" + | title | subline | attributes.conversation | + | Room 5 | | Room 5 | + | Room 6 | | Room 6 |