fix(bots): Fix notifications of bot messages and reactions

Signed-off-by: Joas Schilling <coding@schilljs.com>

lib/Listener/BotListener.php

@@ -93,7 +93,7 @@ class BotListener implements IEventListener {
 		try {
 			$this->botService->validateBotParameters($event->getName(), $event->getSecret(), $event->getUrl(), $event->getDescription());
 		} catch (\InvalidArgumentException $e) {
-			$this->logger->error('Invalid data in bot install event', ['exception' => $e]);
+			$this->logger->error('Invalid data in bot install event: ' . $e->getMessage(), ['exception' => $e]);
 			throw $e;
 		}
 
@@ -107,7 +107,7 @@ class BotListener implements IEventListener {
 			try {
 				$this->botServerMapper->findByUrl($event->getUrl());
 				$e = new \InvalidArgumentException('Bot with the same URL and a different secret is already registered');
-				$this->logger->error('Invalid data in bot install event', ['exception' => $e]);
+				$this->logger->error('Invalid data in bot install event: ' . $e->getMessage(), ['exception' => $e]);
 				throw $e;
 			} catch (DoesNotExistException) {
 			}

lib/Model/BotServerMapper.php

@@ -75,8 +75,13 @@ class BotServerMapper extends QBMapper {
 	 * @throws DoesNotExistException
 	 */
 	public function findByUrl(string $url): BotServer {
-		$urlHash = sha1($url);
+		return $this->findByUrlHash(sha1($url));
+	}
 
+	/**
+	 * @throws DoesNotExistException
+	 */
+	public function findByUrlHash(string $urlHash): BotServer {
 		$query = $this->db->getQueryBuilder();
 		$query->select('*')
 			->from($this->getTableName())

lib/Notification/Notifier.php

@@ -34,11 +34,13 @@ use OCA\Talk\Exceptions\RoomNotFoundException;
 use OCA\Talk\GuestManager;
 use OCA\Talk\Manager;
 use OCA\Talk\Model\Attendee;
+use OCA\Talk\Model\BotServerMapper;
 use OCA\Talk\Participant;
 use OCA\Talk\Room;
 use OCA\Talk\Service\AvatarService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Webinary;
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Comments\ICommentsManager;
 use OCP\Comments\NotFoundException;
@@ -87,6 +89,7 @@ class Notifier implements INotifier {
 		protected ITimeFactory $timeFactory,
 		protected Definitions $definitions,
 		protected AddressHandler $addressHandler,
+		protected BotServerMapper $botServerMapper,
 	) {
 		$this->commentManager = $commentManager;
 	}
@@ -454,7 +457,7 @@ class Notifier implements INotifier {
 
 		$richSubjectUser = null;
 		$isGuest = false;
-		if ($subjectParameters['userType'] === 'users') {
+		if ($subjectParameters['userType'] === Attendee::ACTOR_USERS) {
 			$userId = $subjectParameters['userId'];
 			$userDisplayName = $this->userManager->getDisplayName($userId);
 
@@ -465,6 +468,22 @@ class Notifier implements INotifier {
 					'name' => $userDisplayName,
 				];
 			}
+		} elseif ($subjectParameters['userType'] === Attendee::ACTOR_BOTS) {
+			$botId = $subjectParameters['userId'];
+			try {
+				$bot = $this->botServerMapper->findByUrlHash(substr($botId, strlen(Attendee::ACTOR_BOT_PREFIX)));
+				$richSubjectUser = [
+					'type' => 'highlight',
+					'id' => $botId,
+					'name' => $bot->getName() . ' (Bot)',
+				];
+			} catch (DoesNotExistException $e) {
+				$richSubjectUser = [
+					'type' => 'highlight',
+					'id' => $botId,
+					'name' => 'Bot',
+				];
+			}
 		} else {
 			$isGuest = true;
 		}

tests/integration/features/bootstrap/FeatureContext.php

@@ -2307,6 +2307,7 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 			// replies; this is needed to get special messages not explicitly
 			// sent like those for shared files.
 			self::$textToMessageId[$message['message']] = $message['id'];
+			self::$messageIdToText[$message['id']] = $message['message'];
 			if ($message['message'] === '{file}' && isset($message['messageParameters']['file']['name'])) {
 				self::$textToMessageId['shared::file::' . $message['messageParameters']['file']['name']] = $message['id'];
 				self::$messageIdToText[$message['id']] = 'shared::file::' . $message['messageParameters']['file']['name'];

tests/integration/features/chat/bots.feature

@@ -204,31 +204,36 @@ Feature: chat/bots
 
   # Unchanged from above
   Scenario: Bot with response only feature
-    Given invoking occ with "talk:bot:install Bot Secret1234567890123456789012345678901234567890 https://localhost/bot1 --feature=response"
+    Given invoking occ with "talk:bot:install Bot1 Secret1234567890123456789012345678901234567890 https://localhost/bot1 --feature=response"
     And the command was successful
     And read bot ids from OCC
     And user "participant1" creates room "room1" (v4)
       | roomType | 2 |
       | roomName | room1 |
-    And invoking occ with "talk:bot:setup BOT(Bot) ROOM(room1)"
+    And invoking occ with "talk:bot:setup BOT(Bot1) ROOM(room1)"
     And the command was successful
+    And user "participant1" sets notifications to all for room "room1" (v4)
     And user "participant1" sends message "Message 1" to room "room1" with 201
-    When Bot "Bot" sends a message for room "room1" with 201 (v1)
+    When Bot "Bot1" sends a message for room "room1" with 201 (v1)
       | secret | Secret1234567890123456789012345678901234567890 |
       | message   | Response 1 |
       | replyTo   | Message 1  |
-    When Bot "Bot" sends a reaction for room "room1" with 201 (v1)
+    When Bot "Bot1" sends a reaction for room "room1" with 201 (v1)
       | secret | Secret1234567890123456789012345678901234567890 |
       | messageId | Message 1 |
       | reaction  | 👍 |
     Then user "participant1" sees the following messages in room "room1" with 200
       | room  | actorType | actorId           | actorDisplayName         | message    | messageParameters | parentMessage |
-      | room1 | bots      | BOT(Bot)          | Bot (Bot)                | Response 1 | []                | Message 1     |
+      | room1 | bots      | BOT(Bot1)         | Bot1 (Bot)               | Response 1 | []                | Message 1     |
       | room1 | users     | participant1      | participant1-displayname | Message 1  | []                |               |
+    And user participant1 has the following notifications
+      | app    | object_type | object_id        | subject                                                          |
+      | spreed | chat        | room1/Message 1  | Bot1 (Bot) reacted with 👍 to your message in conversation room1 |
+      | spreed | chat        | room1/Response 1 | Bot1 (Bot) replied to your message in conversation room1         |
     Then user "participant1" retrieve reactions "👍" of message "Message 1" in room "room1" with 200
-      | actorType | actorId           | actorDisplayName | reaction |
-      | bots      | BOT(Bot)          | Bot (Bot)        | 👍       |
-    When Bot "Bot" removes a reaction for room "room1" with 200 (v1)
+      | actorType | actorId            | actorDisplayName  | reaction |
+      | bots      | BOT(Bot1)          | Bot1 (Bot)        | 👍       |
+    When Bot "Bot1" removes a reaction for room "room1" with 200 (v1)
       | secret | Secret1234567890123456789012345678901234567890 |
       | messageId | Message 1 |
       | reaction  | 👍 |

tests/php/Notification/NotifierTest.php

@@ -30,6 +30,7 @@ use OCA\Talk\Exceptions\RoomNotFoundException;
 use OCA\Talk\GuestManager;
 use OCA\Talk\Manager;
 use OCA\Talk\Model\Attendee;
+use OCA\Talk\Model\BotServerMapper;
 use OCA\Talk\Model\Message;
 use OCA\Talk\Notification\Notifier;
 use OCA\Talk\Participant;
@@ -91,6 +92,8 @@ class NotifierTest extends TestCase {
 	protected ?Notifier $notifier = null;
 	/** @var AddressHandler|MockObject */
 	protected $addressHandler;
+	/** @var BotServerMapper|MockObject */
+	protected $botServerMapper;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -113,6 +116,7 @@ class NotifierTest extends TestCase {
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->definitions = $this->createMock(Definitions::class);
 		$this->addressHandler = $this->createMock(AddressHandler::class);
+		$this->botServerMapper = $this->createMock(BotServerMapper::class);
 
 		$this->notifier = new Notifier(
 			$this->lFactory,
@@ -132,7 +136,8 @@ class NotifierTest extends TestCase {
 			$this->rootFolder,
 			$this->timeFactory,
 			$this->definitions,
-			$this->addressHandler
+			$this->addressHandler,
+			$this->botServerMapper,
 		);
 	}