Merge pull request #8103 from nextcloud/techdebt/noid/more

Move more methods away from room model

11 files changed, 99 insertions, 85 deletions

diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php
index e62e44b62..984144cc2 100644
--- a/lib/Chat/ChatManager.php
+++ b/lib/Chat/ChatManager.php
@@ -36,6 +36,7 @@ use OCA\Talk\Room;
 use OCA\Talk\Service\AttachmentService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\PollService;
+use OCA\Talk\Service\RoomService;
 use OCA\Talk\Share\RoomShareProvider;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Collaboration\Reference\IReferenceManager;
@@ -89,6 +90,7 @@ class ChatManager {
 	private IManager $shareManager;
 	private RoomShareProvider $shareProvider;
 	private ParticipantService $participantService;
+	private RoomService $roomService;
 	private PollService $pollService;
 	private Notifier $notifier;
 	protected ITimeFactory $timeFactory;
@@ -104,6 +106,7 @@ class ChatManager {
 								IManager $shareManager,
 								RoomShareProvider $shareProvider,
 								ParticipantService $participantService,
+								RoomService $roomService,
 								PollService $pollService,
 								Notifier $notifier,
 								ICacheFactory $cacheFactory,
@@ -117,6 +120,7 @@ class ChatManager {
 		$this->shareManager = $shareManager;
 		$this->shareProvider = $shareProvider;
 		$this->participantService = $participantService;
+		$this->roomService = $roomService;
 		$this->pollService = $pollService;
 		$this->notifier = $notifier;
 		$this->cache = $cacheFactory->createDistributed('talk/lastmsgid');
@@ -186,7 +190,7 @@ class ChatManager {
 
 			if (!$shouldSkipLastMessageUpdate) {
 				// Update last_message
-				$chat->setLastMessage($comment);
+				$this->roomService->setLastMessage($chat, $comment);
 				$this->unreadCountCache->clear($chat->getId() . '-');
 			}
 
@@ -226,7 +230,7 @@ class ChatManager {
 			$this->commentsManager->save($comment);
 
 			// Update last_message
-			$chat->setLastMessage($comment);
+			$this->roomService->setLastMessage($chat, $comment);
 			$this->unreadCountCache->clear($chat->getId() . '-');
 
 			$this->dispatcher->dispatch(self::EVENT_AFTER_SYSTEM_MESSAGE_SEND, $event);
@@ -277,10 +281,10 @@ class ChatManager {
 
 			// Update last_message
 			if ($comment->getActorType() !== 'bots' || $comment->getActorId() === 'changelog') {
-				$chat->setLastMessage($comment);
+				$this->roomService->setLastMessage($chat, $comment);
 				$this->unreadCountCache->clear($chat->getId() . '-');
 			} else {
-				$chat->setLastActivity($comment->getCreationDateTime());
+				$this->roomService->setLastActivity($chat, $comment->getCreationDateTime());
 			}
 
 			$alreadyNotifiedUsers = [];
diff --git a/lib/Command/Room/TRoomCommand.php b/lib/Command/Room/TRoomCommand.php
index 58d45dac7..15991b832 100644
--- a/lib/Command/Room/TRoomCommand.php
+++ b/lib/Command/Room/TRoomCommand.php
@@ -90,7 +90,7 @@ trait TRoomCommand {
 			throw new InvalidArgumentException('Invalid room name.');
 		}
 
-		if (!$room->setName($name)) {
+		if (!$this->roomService->setName($room, $name)) {
 			throw new InvalidArgumentException('Unable to change room name.');
 		}
 	}
diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php
index a28db9e84..2fb836562 100644
--- a/lib/Controller/RoomController.php
+++ b/lib/Controller/RoomController.php
@@ -873,7 +873,7 @@ class RoomController extends AEnvironmentAwareController {
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);
 		}
 
-		$this->room->setName($roomName);
+		$this->roomService->setName($this->room, $roomName);
 		return new DataResponse();
 	}
 
diff --git a/lib/Manager.php b/lib/Manager.php
index cc800d2d5..a574aaf00 100644
--- a/lib/Manager.php
+++ b/lib/Manager.php
@@ -393,8 +393,10 @@ class Manager {
 		foreach ($leftRooms as $room) {
 			// We are changing the room type and name so a potential follow up
 			// user with the same user-id can not reopen the one-to-one conversation.
-			Server::get(RoomService::class)->setType($room, Room::TYPE_GROUP, true);
-			$room->setName($user->getDisplayName(), '');
+			/** @var RoomService $roomService */
+			$roomService = Server::get(RoomService::class);
+			$roomService->setType($room, Room::TYPE_GROUP, true);
+			$roomService->setName($room, $user->getDisplayName(), '');
 		}
 	}
 
@@ -975,7 +977,9 @@ class Manager {
 
 
 		if ($room->getType() !== Room::TYPE_ONE_TO_ONE && $room->getName() === '') {
-			$room->setName($this->getRoomNameByParticipants($room));
+			/** @var RoomService $roomService */
+			$roomService = Server::get(RoomService::class);
+			$roomService->setName($room, $this->getRoomNameByParticipants($room), '');
 		}
 
 		// Set the room name to the other participant for one-to-one rooms
diff --git a/lib/Room.php b/lib/Room.php
index 9512f4c4a..2d7ea3281 100644
--- a/lib/Room.php
+++ b/lib/Room.php
@@ -27,7 +27,6 @@ declare(strict_types=1);
 
 namespace OCA\Talk;
 
-use OCA\Talk\Events\ModifyRoomEvent;
 use OCA\Talk\Events\SignalingRoomPropertiesEvent;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Model\Attendee;
@@ -37,7 +36,6 @@ use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\RoomService;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Comments\IComment;
-use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IDBConnection;
 use OCP\Security\IHasher;
@@ -351,7 +349,9 @@ class Room {
 				// Fill the room name with the participants for 1-to-1 conversations
 				$users = $participantService->getParticipantUserIds($this);
 				sort($users);
-				$this->setName(json_encode($users), '');
+				/** @var RoomService $roomService */
+				$roomService = Server::get(RoomService::class);
+				$roomService->setName($this, json_encode($users), '');
 			} elseif (strpos($this->name, '["') !== 0) {
 				// TODO use DI
 				$participantService = Server::get(ParticipantService::class);
@@ -361,12 +361,18 @@ class Room {
 					$users[] = $this->name;
 				}
 				sort($users);
-				$this->setName(json_encode($users), '');
+				/** @var RoomService $roomService */
+				$roomService = Server::get(RoomService::class);
+				$roomService->setName($this, json_encode($users), '');
 			}
 		}
 		return $this->name;
 	}
 
+	public function setName(string $name): void {
+		$this->name = $name;
+	}
+
 	public function getSecondParticipant(string $userId): string {
 		if ($this->getType() !== self::TYPE_ONE_TO_ONE) {
 			throw new \InvalidArgumentException('Not a one-to-one room');
@@ -435,6 +441,10 @@ class Room {
 		return $this->lastActivity;
 	}
 
+	public function setLastActivity(\DateTime $now): void {
+		$this->lastActivity = $now;
+	}
+
 	public function getLastMessage(): ?IComment {
 		if ($this->lastMessageId && $this->lastMessage === null) {
 			$this->lastMessage = $this->manager->loadLastCommentInfo($this->lastMessageId);
@@ -446,6 +456,11 @@ class Room {
 		return $this->lastMessage;
 	}
 
+	public function setLastMessage(IComment $message): void {
+		$this->lastMessage = $message;
+		$this->lastMessageId = (int) $message->getId();
+	}
+
 	public function getObjectType(): string {
 		return $this->objectType;
 	}
@@ -572,48 +587,6 @@ class Room {
 		return $this->manager->createParticipantObject($this, $row);
 	}
 
-	/**
-	 * @param string $newName Currently it is only allowed to rename: self::TYPE_GROUP, self::TYPE_PUBLIC
-	 * @param string|null $oldName
-	 * @return bool True when the change was valid, false otherwise
-	 */
-	public function setName(string $newName, ?string $oldName = null): bool {
-		$oldName = $oldName !== null ? $oldName : $this->getName();
-		if ($newName === $oldName) {
-			return false;
-		}
-
-		$event = new ModifyRoomEvent($this, 'name', $newName, $oldName);
-		$this->dispatcher->dispatch(self::EVENT_BEFORE_NAME_SET, $event);
-
-		$update = $this->db->getQueryBuilder();
-		$update->update('talk_rooms')
-			->set('name', $update->createNamedParameter($newName))
-			->where($update->expr()->eq('id', $update->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT)));
-		$update->executeStatement();
-		$this->name = $newName;
-
-		$this->dispatcher->dispatch(self::EVENT_AFTER_NAME_SET, $event);
-
-		return true;
-	}
-
-	/**
-	 * @param \DateTime $now
-	 * @return bool
-	 */
-	public function setLastActivity(\DateTime $now): bool {
-		$update = $this->db->getQueryBuilder();
-		$update->update('talk_rooms')
-			->set('last_activity', $update->createNamedParameter($now, 'datetime'))
-			->where($update->expr()->eq('id', $update->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT)));
-		$update->executeStatement();
-
-		$this->lastActivity = $now;
-
-		return true;
-	}
-
 	public function setActiveSince(\DateTime $since, int $callFlag, bool $isGuest): void {
 		if (!$this->activeSince) {
 			$this->activeSince = $since;
@@ -623,17 +596,4 @@ class Room {
 			$this->activeGuests++;
 		}
 	}
-
-	public function setLastMessage(IComment $message): void {
-		$update = $this->db->getQueryBuilder();
-		$update->update('talk_rooms')
-			->set('last_message', $update->createNamedParameter((int) $message->getId()))
-			->set('last_activity', $update->createNamedParameter($message->getCreationDateTime(), 'datetime'))
-			->where($update->expr()->eq('id', $update->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT)));
-		$update->executeStatement();
-
-		$this->lastMessage = $message;
-		$this->lastMessageId = (int) $message->getId();
-		$this->lastActivity = $message->getCreationDateTime();
-	}
 }
diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php
index 9be4e8264..cffbdb323 100644
--- a/lib/Service/ParticipantService.php
+++ b/lib/Service/ParticipantService.php
@@ -435,7 +435,10 @@ class ParticipantService {
 	}
 
 	protected function updateRoomLastMessage(Room $room, IComment $message): void {
-		$room->setLastMessage($message);
+		/** @var RoomService $roomService */
+		$roomService = Server::get(RoomService::class);
+		$roomService->setLastMessage($room, $message);
+
 		$lastMessageCache = $this->cacheFactory->createDistributed('talk/lastmsgid');
 		$lastMessageCache->remove($room->getToken());
 		$unreadCountCache = $this->cacheFactory->createDistributed('talk/unreadcount');
diff --git a/lib/Service/RoomService.php b/lib/Service/RoomService.php
index f3bd36a7d..f0e9bec8e 100644
--- a/lib/Service/RoomService.php
+++ b/lib/Service/RoomService.php
@@ -24,7 +24,6 @@ declare(strict_types=1);
 namespace OCA\Talk\Service;
 
 use InvalidArgumentException;
-use OCA\Talk\Chat\ChatManager;
 use OCA\Talk\Events\ModifyLobbyEvent;
 use OCA\Talk\Events\ModifyRoomEvent;
 use OCA\Talk\Events\RoomEvent;
@@ -37,6 +36,7 @@ use OCA\Talk\Room;
 use OCA\Talk\Webinary;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
+use OCP\Comments\IComment;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\HintException;
@@ -49,7 +49,6 @@ use OCP\Share\IManager as IShareManager;
 
 class RoomService {
 	protected Manager $manager;
-	protected ChatManager $chatManager;
 	protected ParticipantService $participantService;
 	protected IDBConnection $db;
 	protected ITimeFactory $timeFactory;
@@ -59,7 +58,6 @@ class RoomService {
 	protected IJobList $jobList;
 
 	public function __construct(Manager $manager,
-								ChatManager $chatManager,
 								ParticipantService $participantService,
 								IDBConnection $db,
 								ITimeFactory $timeFactory,
@@ -68,7 +66,6 @@ class RoomService {
 								IEventDispatcher $dispatcher,
 								IJobList $jobList) {
 		$this->manager = $manager;
-		$this->chatManager = $chatManager;
 		$this->participantService = $participantService;
 		$this->db = $db;
 		$this->timeFactory = $timeFactory;
@@ -269,6 +266,32 @@ class RoomService {
 
 		return true;
 	}
+	/**
+	 * @param string $newName Currently it is only allowed to rename: self::TYPE_GROUP, self::TYPE_PUBLIC
+	 * @param string|null $oldName
+	 * @return bool True when the change was valid, false otherwise
+	 */
+	public function setName(Room $room, string $newName, ?string $oldName = null): bool {
+		$oldName = $oldName !== null ? $oldName : $room->getName();
+		if ($newName === $oldName) {
+			return false;
+		}
+
+		$event = new ModifyRoomEvent($room, 'name', $newName, $oldName);
+		$this->dispatcher->dispatch(Room::EVENT_BEFORE_NAME_SET, $event);
+
+		$update = $this->db->getQueryBuilder();
+		$update->update('talk_rooms')
+			->set('name', $update->createNamedParameter($newName))
+			->where($update->expr()->eq('id', $update->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT)));
+		$update->executeStatement();
+
+		$room->setName($newName);
+
+		$this->dispatcher->dispatch(Room::EVENT_AFTER_NAME_SET, $event);
+
+		return true;
+	}
 
 	/**
 	 * @param Room $room
@@ -614,6 +637,28 @@ class RoomService {
 		return true;
 	}
 
+	public function setLastMessage(Room $room, IComment $message): void {
+		$update = $this->db->getQueryBuilder();
+		$update->update('talk_rooms')
+			->set('last_message', $update->createNamedParameter((int) $message->getId()))
+			->set('last_activity', $update->createNamedParameter($message->getCreationDateTime(), 'datetime'))
+			->where($update->expr()->eq('id', $update->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT)));
+		$update->executeStatement();
+
+		$room->setLastMessage($message);
+		$room->setLastActivity($message->getCreationDateTime());
+	}
+
+	public function setLastActivity(Room $room, \DateTime $now): void {
+		$update = $this->db->getQueryBuilder();
+		$update->update('talk_rooms')
+			->set('last_activity', $update->createNamedParameter($now, 'datetime'))
+			->where($update->expr()->eq('id', $update->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT)));
+		$update->executeStatement();
+
+		$room->setLastActivity($now);
+	}
+
 	public function deleteRoom(Room $room): void {
 		$event = new RoomEvent($room);
 		$this->dispatcher->dispatch(Room::EVENT_BEFORE_ROOM_DELETE, $event);
diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php
index 37d998d4e..9849a4d7a 100644
--- a/tests/php/Chat/ChatManagerTest.php
+++ b/tests/php/Chat/ChatManagerTest.php
@@ -34,6 +34,7 @@ use OCA\Talk\Room;
 use OCA\Talk\Service\AttachmentService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\PollService;
+use OCA\Talk\Service\RoomService;
 use OCA\Talk\Share\RoomShareProvider;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Collaboration\Reference\IReferenceManager;
@@ -65,6 +66,8 @@ class ChatManagerTest extends TestCase {
 	protected $shareProvider;
 	/** @var ParticipantService|MockObject */
 	protected $participantService;
+	/** @var RoomService|MockObject */
+	protected $roomService;
 	/** @var PollService|MockObject */
 	protected $pollService;
 	/** @var Notifier|MockObject */
@@ -86,6 +89,7 @@ class ChatManagerTest extends TestCase {
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->shareProvider = $this->createMock(RoomShareProvider::class);
 		$this->participantService = $this->createMock(ParticipantService::class);
+		$this->roomService = $this->createMock(RoomService::class);
 		$this->pollService = $this->createMock(PollService::class);
 		$this->notifier = $this->createMock(Notifier::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
@@ -101,6 +105,7 @@ class ChatManagerTest extends TestCase {
 			$this->shareManager,
 			$this->shareProvider,
 			$this->participantService,
+			$this->roomService,
 			$this->pollService,
 			$this->notifier,
 			$cacheFactory,
@@ -127,6 +132,7 @@ class ChatManagerTest extends TestCase {
 					$this->shareManager,
 					$this->shareProvider,
 					$this->participantService,
+					$this->roomService,
 					$this->pollService,
 					$this->notifier,
 					$cacheFactory,
@@ -146,6 +152,7 @@ class ChatManagerTest extends TestCase {
 			$this->shareManager,
 			$this->shareProvider,
 			$this->participantService,
+			$this->roomService,
 			$this->pollService,
 			$this->notifier,
 			$cacheFactory,
diff --git a/tests/php/Chat/SystemMessage/ListenerTest.php b/tests/php/Chat/SystemMessage/ListenerTest.php
index dd59d3dbf..274b47c2f 100644
--- a/tests/php/Chat/SystemMessage/ListenerTest.php
+++ b/tests/php/Chat/SystemMessage/ListenerTest.php
@@ -82,11 +82,11 @@ class ListenerTest extends TestCase {
 		$this->timeFactory->method('getDateTime')->willReturn($this->dummyTime);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 
-
 		$this->handlers = [];
 
 		$this->eventDispatcher->method('addListener')
 			->will($this->returnCallback(function ($eventName, $handler) {
+				$this->handlers[$eventName] ??= [];
 				$this->handlers[$eventName][] = $handler;
 			}));
 
diff --git a/tests/php/Service/RoomServiceTest.php b/tests/php/Service/RoomServiceTest.php
index 4f8626263..5c858f745 100644
--- a/tests/php/Service/RoomServiceTest.php
+++ b/tests/php/Service/RoomServiceTest.php
@@ -25,7 +25,6 @@ namespace OCA\Talk\Tests\php\Service;
 
 use InvalidArgumentException;
 use OC\EventDispatcher\EventDispatcher;
-use OCA\Talk\Chat\ChatManager;
 use OCA\Talk\Events\VerifyRoomPasswordEvent;
 use OCA\Talk\Exceptions\RoomNotFoundException;
 use OCA\Talk\Manager;
@@ -68,7 +67,6 @@ class RoomServiceTest extends TestCase {
 		parent::setUp();
 
 		$this->manager = $this->createMock(Manager::class);
-		$this->chatManager = $this->createMock(ChatManager::class);
 		$this->participantService = $this->createMock(ParticipantService::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->shareManager = $this->createMock(IShareManager::class);
@@ -77,7 +75,6 @@ class RoomServiceTest extends TestCase {
 		$this->jobList = $this->createMock(IJobList::class);
 		$this->service = new RoomService(
 			$this->manager,
-			$this->chatManager,
 			$this->participantService,
 			\OC::$server->get(IDBConnection::class),
 			$this->timeFactory,
@@ -348,7 +345,6 @@ class RoomServiceTest extends TestCase {
 
 		$service = new RoomService(