From a5f2cd7baf57c0e16f01b4b4dfd5f3e34bd194ea 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 [skip ci] --- lib/Search/ConversationSearch.php | 69 +++++++++++++++++-- .../features/bootstrap/FeatureContext.php | 61 ++++++++++++---- .../features/conversation-5/search.feature | 34 +++++++++ 3 files changed, 145 insertions(+), 19 deletions(-) create mode 100644 tests/integration/features/conversation-5/search.feature diff --git a/lib/Search/ConversationSearch.php b/lib/Search/ConversationSearch.php index f55b14d877c..1f89ee2141c 100644 --- a/lib/Search/ConversationSearch.php +++ b/lib/Search/ConversationSearch.php @@ -83,11 +83,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) { @@ -99,10 +105,11 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $parameters['token'] === $room->getToken() && strpos($query->getRoute(), Application::APP_ID . '.') === 0) { // 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()), @@ -110,7 +117,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; } @@ -120,7 +127,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()]), '', @@ -129,12 +136,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 68bc9cdaf16..5de7660c4e1 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -2673,27 +2673,62 @@ protected function compareSearchResponse(TableNode $formData = null) { $messages = $this->getDataFromResponse($this->response)['entries']; 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::assertCount($count, $results, 'Result 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::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 |