diff --git a/lib/Activity/ActivityManager.php b/lib/Activity/ActivityManager.php index 10e1ceb74..baedf109d 100644 --- a/lib/Activity/ActivityManager.php +++ b/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) { diff --git a/tests/unit/Activity/ActivityManagerTest.php b/tests/unit/Activity/ActivityManagerTest.php index d292d9023..8727820b9 100644 --- a/tests/unit/Activity/ActivityManagerTest.php +++ b/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([