Skip to content

Commit

Permalink
Merge pull request #10832 from nextcloud/bugfix/noid/empty-reactions-…
Browse files Browse the repository at this point in the history 
…list

fix(reactions): Don't break mobile clients when the reactions list is…
  • Loading branch information
@nickvergessen
nickvergessen committed Nov 3, 2023
2 parents 1e5c84a + 96cf85e commit 306b13f
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 20 deletions.
3 changes: 1 addition & 2 deletions lib/Controller/ChatController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ protected function getActorInfo(string $actorDisplayName = ''): array {
}
return [Attendee::ACTOR_GUESTS, $this->participant->getAttendee()->getActorId()];
}

if ($this->userId === MatterbridgeManager::BRIDGE_BOT_USERID && $actorDisplayName) {
return [Attendee::ACTOR_BRIDGED, str_replace(['/', '"'], '', $actorDisplayName)];
}
Expand Down Expand Up @@ -936,7 +936,6 @@ public function getObjectsSharedInRoomOverview(int $limit = 7): DataResponse {
Attachment::TYPE_VOICE,
];

$messages = [];
$messageIdsByType = [];
// Get all attachments
foreach ($objectTypes as $objectType) {
Expand Down
27 changes: 21 additions & 6 deletions lib/Controller/ReactionController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function __construct(
*
* @param int $messageId ID of the message
* @param string $reaction Emoji to add
* @return DataResponse<Http::STATUS_OK|Http::STATUS_CREATED, array<string, TalkReaction[]>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_CREATED, array<string, TalkReaction[]>|\stdClass, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, array<empty>, array{}>
*
* 200: Reaction already existed
* 201: Reaction added successfully
Expand Down Expand Up @@ -89,15 +89,15 @@ public function react(int $messageId, string $reaction): DataResponse {
return new DataResponse([], Http::STATUS_BAD_REQUEST);
}
$reactions = $this->reactionManager->retrieveReactionMessages($this->getRoom(), $this->getParticipant(), $messageId);
return new DataResponse($reactions, $status);
return new DataResponse($this->formatReactions($reactions), $status);
}

/**
* Delete a reaction from a message
*
* @param int $messageId ID of the message
* @param string $reaction Emoji to remove
* @return DataResponse<Http::STATUS_OK, array<string, TalkReaction[]>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, array<string, TalkReaction[]>|\stdClass, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, array<empty>, array{}>
*
* 200: Reaction deleted successfully
* 400: Deleting reaction is not possible
Expand All @@ -124,15 +124,15 @@ public function delete(int $messageId, string $reaction): DataResponse {
return new DataResponse([], Http::STATUS_BAD_REQUEST);
}

return new DataResponse($reactions, Http::STATUS_OK);
return new DataResponse($this->formatReactions($reactions), Http::STATUS_OK);
}

/**
* Get a list of reactions for a message
*
* @param int $messageId ID of the message
* @param string|null $reaction Emoji to filter
* @return DataResponse<Http::STATUS_OK, array<string, TalkReaction[]>, array{}>|DataResponse<Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, array<string, TalkReaction[]>|\stdClass, array{}>|DataResponse<Http::STATUS_NOT_FOUND, array<empty>, array{}>
*
* 200: Reactions returned
* 404: Message or reaction not found
Expand All @@ -150,6 +150,21 @@ public function getReactions(int $messageId, ?string $reaction): DataResponse {

$reactions = $this->reactionManager->retrieveReactionMessages($this->getRoom(), $this->getParticipant(), $messageId, $reaction);

return new DataResponse($reactions, Http::STATUS_OK);
return new DataResponse($this->formatReactions($reactions), Http::STATUS_OK);
}


/**
* @param array<string, TalkReaction[]> $reactions
* @return array<string, TalkReaction[]>|\stdClass
*/
public function formatReactions(array $reactions): array|\stdClass {
if ($this->getResponseFormat() === 'json' && empty($reactions)) {
// Cheating here to make sure the reactions array is always a
// JSON object on the API, even when there is no reaction at all.
return new \stdClass();
}

return $reactions;
}
}
51 changes: 39 additions & 12 deletions tests/integration/features/bootstrap/FeatureContext.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2391,12 +2391,17 @@ public function userSeesTheFollowingSharedOverviewMediaInRoom($user, $identifier
$this->sendRequest('GET', '/apps/spreed/api/' . $apiVersion . '/chat/' . self::$identifierToToken[$identifier] . '/share/overview');
$this->assertStatusCode($this->response, $statusCode);

$overview = $this->getDataFromResponse($this->response);
$expected = $formData->getRowsHash();
$summarized = array_map(function ($type) {
return (string) count($type);
}, $overview);
Assert::assertEquals($expected, $summarized);
$contents = $this->response->getBody()->getContents();
$this->assertEmptyArrayIsNotAListButADictionary($formData, $contents);
$overview = $this->getDataFromResponseBody($contents);

if ($formData instanceof TableNode) {
$expected = $formData->getRowsHash();
$summarized = array_map(function ($type) {
return (string) count($type);
}, $overview);
Assert::assertEquals($expected, $summarized);
}
}

/**
Expand Down Expand Up @@ -2985,7 +2990,16 @@ public function userCheckCapability($user, $capability, $value) {
* @return array
*/
protected function getDataFromResponse(ResponseInterface $response) {
$jsonBody = json_decode($response->getBody()->getContents(), true);
return $this->getDataFromResponseBody($response->getBody()->getContents());
}

/**
* Parses the JSON answer to get the array of users returned.
* @param string $response
* @return array
*/
protected function getDataFromResponseBody(string $response) {
$jsonBody = json_decode($response, true);
return $jsonBody['ocs']['data'];
}

Expand Down Expand Up @@ -3465,7 +3479,9 @@ public function userReactWithOnMessageToRoomWith(string $user, string $action, s
'reaction' => $reaction
]);
$this->assertStatusCode($this->response, $statusCode);
$this->assertReactionList($formData);
if ($statusCode === 200 || $statusCode === 201) {
$this->assertReactionList($formData);
}
}

/**
Expand All @@ -3483,10 +3499,15 @@ public function userRetrieveReactionsOfMessageInRoomWith(string $user, string $r
}

private function assertReactionList(?TableNode $formData): void {
$contents = $this->response->getBody()->getContents();
$this->assertEmptyArrayIsNotAListButADictionary($formData, $contents);
$reactions = $this->getDataFromResponseBody($contents);

$expected = [];
if (!$formData instanceof TableNode) {
return;
}

foreach ($formData->getHash() as $row) {
$reaction = $row['reaction'];
unset($row['reaction']);
Expand All @@ -3501,8 +3522,7 @@ private function assertReactionList(?TableNode $formData): void {
$expected[$reaction][] = $row;
}

$result = $this->getDataFromResponse($this->response);
$result = array_map(static function ($reaction, $list) use ($expected): array {
$actual = array_map(static function ($reaction, $list) use ($expected): array {
$list = array_map(function ($reaction) {
unset($reaction['timestamp']);
$reaction['actorId'] = ($reaction['actorType'] === 'guests') ? self::$sessionIdToUser[$reaction['actorId']] : (string) $reaction['actorId'];
Expand All @@ -3515,8 +3535,8 @@ private function assertReactionList(?TableNode $formData): void {
usort($list, [self::class, 'sortAttendees']);
Assert::assertEquals($expected[$reaction], $list, 'Reaction list by type does not match');
return $list;
}, array_keys($result), array_values($result));
Assert::assertCount(count($expected), $result, 'Reaction count does not match');
}, array_keys($reactions), array_values($reactions));
Assert::assertCount(count($expected), $actual, 'Reaction count does not match');
}

/**
Expand Down Expand Up @@ -3988,6 +4008,13 @@ public function getRoomListWithSpecificUserAgent(string $userAgent, int $statusC
$this->assertStatusCode($this->response, $statusCode);
}

protected function assertEmptyArrayIsNotAListButADictionary(?TableNode $formData, string $content) {
if (!$formData instanceof TableNode || empty($formData->getHash())) {
$data = json_decode($content);
Assert::assertIsNotArray($data->ocs->data, 'Response ocs.data should be an "object" to represent a JSON dictionary, not a list-array');
}
}

/**
* @Then the response error matches with :error
*/
Expand Down
1 change: 1 addition & 0 deletions tests/integration/features/chat-2/rich-object-share.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ Feature: chat-2/rich-object-share
Given user "participant1" creates room "public room" (v4)
| roomType | 3 |
| roomName | room |
Then user "participant1" sees the following shared summarized overview in room "public room" with 200
When user "participant1" shares rich-object "call" "R4nd0mT0k3n" '{"name":"Another room","call-type":"group"}' to room "public room" with 201 (v1)
Then user "participant1" sees the following shared other in room "public room" with 200
| room | actorType | actorId | actorDisplayName | message | messageParameters |
Expand Down

0 comments on commit 306b13f

Please sign in to comment.