Merge pull request #3650 from nextcloud/bugfix/3649

Handle description shortening more gracefully

lib/Activity/ActivityManager.php

@@ -31,7 +31,6 @@ use OCA\Deck\Db\Acl;
 use OCA\Deck\Db\AclMapper;
 use OCA\Deck\Db\Assignment;
 use OCA\Deck\Db\Attachment;
-use OCA\Deck\Db\AttachmentMapper;
 use OCA\Deck\Db\Board;
 use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\Card;
@@ -50,12 +49,15 @@ use OCP\L10N\IFactory;
 
 class ActivityManager {
 	public const DECK_NOAUTHOR_COMMENT_SYSTEM_ENFORCED = 'DECK_NOAUTHOR_COMMENT_SYSTEM_ENFORCED';
+
+	public const SUBJECT_PARAMS_MAX_LENGTH = 4000;
+	public const SHORTENED_DESCRIPTION_MAX_LENGTH = 2000;
+
 	private $manager;
 	private $userId;
 	private $permissionService;
 	private $boardMapper;
 	private $cardMapper;
-	private $attachmentMapper;
 	private $aclMapper;
 	private $stackMapper;
 	private $l10nFactory;
@@ -110,7 +112,6 @@ class ActivityManager {
 		BoardMapper $boardMapper,
 		CardMapper $cardMapper,
 		StackMapper $stackMapper,
-		AttachmentMapper $attachmentMapper,
 		AclMapper $aclMapper,
 		IFactory $l10nFactory,
 		$userId
@@ -120,7 +121,6 @@ class ActivityManager {
 		$this->boardMapper = $boardMapper;
 		$this->cardMapper = $cardMapper;
 		$this->stackMapper = $stackMapper;
-		$this->attachmentMapper = $attachmentMapper;
 		$this->aclMapper = $aclMapper;
 		$this->l10nFactory = $l10nFactory;
 		$this->userId = $userId;
@@ -249,19 +249,6 @@ class ActivityManager {
 		try {
 			$event = $this->createEvent($objectType, $entity, $subject, $additionalParams, $author);
 			if ($event !== null) {
-				$json = json_encode($event->getSubjectParameters());
-				if (mb_strlen($json) > 4000) {
-					$params = json_decode(json_encode($event->getSubjectParameters()), true);
-
-					$newContent = $params['after'];
-					unset($params['before'], $params['after'], $params['card']['description']);
-
-					$params['after'] = mb_substr($newContent, 0, 2000);
-					if (mb_strlen($newContent) > 2000) {
-						$params['after'] .= '...';
-					}
-					$event->setSubject($event->getSubject(), $params);
-				}
 				$this->sendToUsers($event);
 			}
 		} catch (\Exception $e) {
@@ -410,12 +397,31 @@ class ActivityManager {
 
 		$subjectParams['author'] = $author === null ? $this->userId : $author;
 
+		$subjectParams = array_merge($subjectParams, $additionalParams);
+		$json = json_encode($subjectParams);
+		if (mb_strlen($json) > self::SUBJECT_PARAMS_MAX_LENGTH) {
+			$params = json_decode(json_encode($subjectParams), true);
+
+			if ($subject === self::SUBJECT_CARD_UPDATE_DESCRIPTION && isset($params['after'])) {
+				$newContent = $params['after'];
+				unset($params['before'], $params['after'], $params['card']['description']);
+
+				$params['after'] = mb_substr($newContent, 0, self::SHORTENED_DESCRIPTION_MAX_LENGTH);
+				if (mb_strlen($newContent) > self::SHORTENED_DESCRIPTION_MAX_LENGTH) {
+					$params['after'] .= '...';
+				}
+				$subjectParams = $params;
+			} else {
+				throw new \Exception('Subject parameters too long');
+			}
+		}
+
 		$event = $this->manager->generateEvent();
 		$event->setApp('deck')
 			->setType($eventType)
 			->setAuthor($subjectParams['author'])
 			->setObject($objectType, (int)$object->getId(), $object->getTitle())
-			->setSubject($subject, array_merge($subjectParams, $additionalParams))
+			->setSubject($subject, $subjectParams)
 			->setTimestamp(time());
 
 		if ($message !== null) {

tests/unit/Activity/ActivityManagerTest.php

@@ -26,7 +26,6 @@ namespace OCA\Deck\Activity;
 use OCA\Deck\Db\AclMapper;
 use OCA\Deck\Db\Assignment;
 use OCA\Deck\Db\Attachment;
-use OCA\Deck\Db\AttachmentMapper;
 use OCA\Deck\Db\Board;
 use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\Card;
@@ -41,7 +40,7 @@ use OCP\IL10N;
 use OCP\IUser;
 use OCP\L10N\IFactory;
 use PHPUnit\Framework\TestCase;
-use PHPUnit_Framework_MockObject_MockObject as MockObject;
+use PHPUnit\Framework\MockObject\MockObject;
 
 class ActivityManagerTest extends TestCase {
 
@@ -57,8 +56,6 @@ class ActivityManagerTest extends TestCase {
 	private $cardMapper;
 	/** @var StackMapper|MockObject */
 	private $stackMapper;
-	/** @var AttachmentMapper|MockObject */
-	private $attachmentMapper;
 	/** @var AclMapper|MockObject */
 	private $aclMapper;
 	/** @var IFactory|MockObject */
@@ -74,7 +71,6 @@ class ActivityManagerTest extends TestCase {
 		$this->boardMapper = $this->createMock(BoardMapper::class);
 		$this->cardMapper = $this->createMock(CardMapper::class);
 		$this->stackMapper = $this->createMock(StackMapper::class);
-		$this->attachmentMapper = $this->createMock(AttachmentMapper::class);
 		$this->aclMapper = $this->createMock(AclMapper::class);
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->l10n = $this->createMock(IL10N::class);
@@ -84,7 +80,6 @@ class ActivityManagerTest extends TestCase {
 			$this->boardMapper,
 			$this->cardMapper,
 			$this->stackMapper,
-			$this->attachmentMapper,
 			$this->aclMapper,
 			$this->l10nFactory,
 			$this->userId
@@ -93,7 +88,7 @@ class ActivityManagerTest extends TestCase {
 
 	public function testGetActivityFormatOwn() {
 		$managerClass = new \ReflectionClass(ActivityManager::class);
-		$this->l10n->expects($this->any())
+		$this->l10n->expects(self::any())
 			->method('t')
 			->will($this->returnCallback(function ($s) {
 				return $s;
@@ -122,22 +117,29 @@ class ActivityManagerTest extends TestCase {
 		}
 	}
 
+	private function expectEventCreation($subject, $subjectParams) {
+		$event = $this->createMock(IEvent::class);
+		$this->manager->expects(self::once())
+			->method('generateEvent')
+			->willReturn($event);
+		$event->expects(self::once())->method('setApp')->willReturn($event);
+		$event->expects(self::once())->method('setType')->willReturn($event);
+		$event->expects(self::once())->method('setAuthor')->willReturn($event);
+		$event->expects(self::once())->method('setObject')->willReturn($event);
+		$event->expects(self::once())->method('setSubject')->with($subject, $subjectParams)->willReturn($event);
+		$event->expects(self::once())->method('setTimestamp')->willReturn($event);
+		return $event;
+	}
+
 	public function testCreateEvent() {
 		$board = new Board();
 		$board->setTitle('');
-		$this->boardMapper->expects($this->once())
+		$this->boardMapper->expects(self::once())
 			->method('find')
 			->willReturn($board);
-		$event = $this->createMock(IEvent::class);
-		$this->manager->expects($this->once())
-			->method('generateEvent')
-			->willReturn($event);
-		$event->expects($this->once())->method('setApp')->willReturn($event);
-		$event->expects($this->once())->method('setType')->willReturn($event);
-		$event->expects($this->once())->method('setAuthor')->willReturn($event);
-		$event->expects($this->once())->method('setObject')->willReturn($event);
-		$event->expects($this->once())->method('setSubject')->willReturn($event);
-		$event->expects($this->once())->method('setTimestamp')->willReturn($event);
+		$event = $this->expectEventCreation(ActivityManager::SUBJECT_BOARD_CREATE, [
+			'author' => 'admin'
+		]);
 		$actual = $this->invokePrivate($this->activityManager, 'createEvent', [
 			ActivityManager::DECK_OBJECT_BOARD,
 			$board,
@@ -146,6 +148,133 @@ class ActivityManagerTest extends TestCase {
 		$this->assertEquals($event, $actual);
 	}
 
+	public function testCreateEventDescription() {
+		$board = new Board();
+		$board->setTitle('');
+		$this->boardMapper->expects(self::once())
+			->method('find')
+			->willReturn($board);
+
+		$card = Card::fromRow([
+			'id' => 123,
+			'title' => 'My card',
+			'description' => str_repeat('A', 1000),
+		]);
+		$this->cardMapper->expects(self::any())
+			->method('find')
+			->willReturn($card);
+
+		$stack = Stack::fromRow([]);
+		$this->stackMapper->expects(self::any())
+			->method('find')
+			->willReturn($stack);
+
+		$expectedCard = $card->jsonSerialize();
+		unset($expectedCard['description']);
+		$event = $this->expectEventCreation(ActivityManager::SUBJECT_CARD_UPDATE_DESCRIPTION, [
+			'card' => $expectedCard,
+			'stack' => $stack->jsonSerialize(),
+			'board' => $board->jsonSerialize(),
+			'diff' => true,
+			'author' => 'admin',
+			'after' => str_repeat('C', 2000),
+		]);
+
+		$actual = $this->invokePrivate($this->activityManager, 'createEvent', [
+			ActivityManager::DECK_OBJECT_CARD,
+			$card,
+			ActivityManager::SUBJECT_CARD_UPDATE_DESCRIPTION,
+			[
+				'before' => str_repeat('B', 2000),
+				'after' => str_repeat('C', 2000)
+			],
+		]);
+		$this->assertEquals($event, $actual);
+	}
+
+	public function testCreateEventLongDescription() {
+		$board = new Board();
+		$board->setTitle('');
+		$this->boardMapper->expects(self::once())
+			->method('find')
+			->willReturn($board);
+
+		$card = new Card();
+		$card->setDescription(str_repeat('A', 5000));
+		$card->setTitle('My card');
+		$card->setId(123);
+		$this->cardMapper->expects(self::any())
+			->method('find')
+			->willReturn($card);
+
+		$stack = new Stack();
+		$this->stackMapper->expects(self::any())
+			->method('find')
+			->willReturn($stack);
+
+		$expectedCard = $card->jsonSerialize();
+		unset($expectedCard['description']);
+		$event = $this->expectEventCreation(ActivityManager::SUBJECT_CARD_UPDATE_DESCRIPTION, [
+			'card' => $expectedCard,
+			'stack' => $stack->jsonSerialize(),
+			'board' => $board->jsonSerialize(),
+			'diff' => true,
+			'author' => 'admin',
+			'after' => str_repeat('C', 2000) . '...',
+		]);
+
+		$actual = $this->invokePrivate($this->activityManager, 'createEvent', [
+			ActivityManager::DECK_OBJECT_CARD,
+			$card,
+			ActivityManager::SUBJECT_CARD_UPDATE_DESCRIPTION,
+			[
+				'before' => str_repeat('B', 5000),
+				'after' => str_repeat('C', 5000)
+			],
+		]);
+		$this->assertEquals($event, $actual);
+	}
+
+	public function testCreateEventLabel() {
+		$board = Board::fromRow([
+			'title' => 'My board'
+		]);
+		$this->boardMapper->expects(self::once())
+			->method('find')
+			->willReturn($board);
+
+		$card = Card::fromParams([]);
+		$card->setDescription(str_repeat('A', 5000));
+		$card->setTitle('My card');
+		$card->setId(123);
+		$this->cardMapper->expects(self::any())
+			->method('find')
+			->willReturn($card);
+
+		$stack = Stack::fromParams([]);
+		$this->stackMapper->expects(self::any())
+			->method('find')
+			->willReturn($stack);
+
+		$event = $this->expectEventCreation(ActivityManager::SUBJECT_CARD_UPDATE_TITLE, [
+			'card' => [
+				'id' => 123,
+				'title' => 'My card',
+				'archived' => false,
+			],
+			'stack' => $stack,
+			'board' => $board,
+			'author' => 'admin',
+		]);
+
+		$actual = $this->invokePrivate($this->activityManager, 'createEvent', [
+			ActivityManager::DECK_OBJECT_CARD,
+			$card,
+			ActivityManager::SUBJECT_CARD_UPDATE_TITLE
+		]);
+		$this->assertEquals($event, $actual);
+	}
+
 	public function dataSendToUsers() {
 		return [
 			[ActivityManager::DECK_OBJECT_BOARD],
@@ -155,7 +284,7 @@ class ActivityManagerTest extends TestCase {
 
 	private function mockUser($uid) {
 		$user = $this->createMock(IUser::class);
-		$user->expects($this->any())
+		$user->expects(self::any())
 			->method('getUID')
 			->willReturn($uid);
 		return $user;
@@ -169,15 +298,21 @@ class ActivityManagerTest extends TestCase {
 			$this->mockUser('user2'),
 		];
 		$event = $this->createMock(IEvent::class);
-		$event->expects($this->once())
+
+		$event->expects(self::once())
 			->method('getObjectType')
 			->willReturn($objectType);
-		$event->expects($this->once())
+		$event->expects(self::once())
 			->method('getObjectId')
 			->willReturn(1);
-		$event->expects($this->exactly(2))
+		$event->expects(self::exactly(2))
 			->method('setAffectedUser')
-			->withConsecutive(['user1'], ['user2']);
+			->withConsecutive(
+				['user1'],
+				['user2'],
+			)
+			->willReturnSelf();
+
 		$mapper = null;
 		switch ($objectType) {
 			case ActivityManager::DECK_OBJECT_BOARD:
@@ -187,13 +322,14 @@ class ActivityManagerTest extends TestCase {
 				$mapper = $this->cardMapper;
 				break;
 		}
-		$mapper->expects($this->once())
+		$mapper->expects(self::once())
 			->method('findBoardId')
 			->willReturn(123);
-		$this->permissionService->expects($this->once())
+		$this->permissionService->expects(self::once())
 			->method('findUsers')
 			->willReturn($users);
-		$this->manager->expects($this->exactly(2))
+
+		$this->manager->expects(self::exactly(2))
 			->method('publish')
 			->with($event);
 		$this->invokePrivate($this->activityManager, 'sendToUsers', [$event]);
@@ -237,14 +373,14 @@ class ActivityManagerTest extends TestCase {
 		$card->setId(3);
 		$expected = null;
 		if ($objectType === ActivityManager::DECK_OBJECT_BOARD) {
-			$this->boardMapper->expects($this->once())
+			$this->boardMapper->expects(self::once())
 				->method('find')
 				->with(1)
 				->willReturn($board);
 			$expected = $board;
 		}
 		if ($objectType === ActivityManager::DECK_OBJECT_CARD) {
-			$this->cardMapper->expects($this->once())
+			$this->cardMapper->expects(self::once())
 				->method('find')
 				->with(3)
 				->willReturn($card);
@@ -260,11 +396,11 @@ class ActivityManagerTest extends TestCase {
 		$stack->setBoardId(999);
 		$board = new Board();
 		$board->setId(999);
-		$this->stackMapper->expects($this->once())
+		$this->stackMapper->expects(self::once())
 			->method('find')
 			->with(123)
 			->willReturn($stack);
-		$this->boardMapper->expects($this->once())->method('find')
+		$this->boardMapper->expects(self::once())->method('find')
 			->with(999)
 			->willReturn($board);
 		$this->assertEquals([
@@ -283,15 +419,15 @@ class ActivityManagerTest extends TestCase {
 		$stack->setBoardId(999);
 		$board = new Board();
 		$board->setId(999);
-		$this->cardMapper->expects($this->once())
+		$this->cardMapper->expects(self::once())
 			->method('find')
 			->with(555)
 			->willReturn($card);
-		$this->stackMapper->expects($this->once())
+		$this->stackMapper->expects(self::once())
 			->method('find')
 			->with(123)
 			->willReturn($stack);
-		$this->boardMapper->expects($this->once())->method('find')
+		$this->boardMapper->expects(self::once())->method('find')
 			->with(999)
 			->willReturn($board);
 		$this->assertEquals([
@@ -317,15 +453,15 @@ class ActivityManagerTest extends TestCase {
 		$stack->setBoardId(999);
 		$board = new Board();
 		$board->setId(999);
-		$this->cardMapper->expects($this->once())
+		$this->cardMapper->expects(self::once())
 			->method('find')
 			->with(555)
 			->willReturn($card);
-		$this->stackMapper->expects($this->once())
+		$this->stackMapper->expects(self::once())
 			->method('find')
 			->with(123)
 			->willReturn($stack);
-		$this->boardMapper->expects($this->once())->method('find')
+		$this->boardMapper->expects(self::once())->method('find')
 			->with(999)
 			->willReturn($board);
 		$this->assertEquals([