Code cleanup

Signed-off-by: Julius Härtl <jus@bitgrid.net>

lib/Activity/ActivityManager.php

@@ -67,8 +67,6 @@ class ActivityManager {
 	const SUBJECT_BOARD_RESTORE = 'board_restore';
 	const SUBJECT_BOARD_SHARE = 'board_share';
 	const SUBJECT_BOARD_UNSHARE = 'board_unshare';
-	const SUBJECT_BOARD_ARCHIVE = 'board_archive';
-	const SUBJECT_BOARD_UNARCHIVE = 'board_unarchive';
 
 	const SUBJECT_STACK_CREATE = 'stack_create';
 	const SUBJECT_STACK_UPDATE = 'stack_update';
@@ -88,8 +86,6 @@ class ActivityManager {
 	const SUBJECT_CARD_UPDATE_STACKID = 'card_update_stackId';
 	const SUBJECT_CARD_USER_ASSIGN = 'card_user_assign';
 	const SUBJECT_CARD_USER_UNASSIGN = 'card_user_unassign';
-	const SUBJECT_CARD_MOVE_STACK = 'card_move_stack';
-
 
 	const SUBJECT_ATTACHMENT_CREATE = 'attachment_create';
 	const SUBJECT_ATTACHMENT_UPDATE = 'attachment_update';
@@ -148,18 +144,16 @@ class ActivityManager {
 			case self::SUBJECT_BOARD_UNSHARE:
 				$subject = $ownActivity ? $this->l10n->t('You have removed {acl} from the board {board}') : $this->l10n->t('{user} has removed {acl} from the board {board}');
 				break;
-			case self::SUBJECT_BOARD_ARCHIVE:
-				$subject = $ownActivity ? $this->l10n->t('You have archived the board {board}') : $this->l10n->t('{user} has archived the board {board}');
-				break;
-			case self::SUBJECT_BOARD_UNARCHIVE:
-				$subject = $ownActivity ? $this->l10n->t('You have unarchived the board {board}') : $this->l10n->t('{user} has unarchived the board {board}');
-				break;
 
 			case self::SUBJECT_BOARD_UPDATE_TITLE:
 				$subject = $ownActivity ? $this->l10n->t('You have renamed the board {before} to {board}') : $this->l10n->t('{user} has has renamed the board {before} to {board}');
 				break;
 			case self::SUBJECT_BOARD_UPDATE_ARCHIVED:
-				$subject = $ownActivity ? $this->l10n->t('You have renamed the board {before} to {board}') : $this->l10n->t('{user} has has renamed the board {before} to {board}');
+				if (isset($subjectParams['after']) && $subjectParams['after']) {
+					$subject = $ownActivity ? $this->l10n->t('You have archived the board {board}') : $this->l10n->t('{user} has archived the board {before}');
+				} else {
+					$subject = $ownActivity ? $this->l10n->t('You have unarchived the board {board}') : $this->l10n->t('{user} has unarchived the board {before}');
+				}
 				break;
 
 			case self::SUBJECT_STACK_CREATE:
@@ -219,7 +213,7 @@ class ActivityManager {
 				$subject = $ownActivity ? $this->l10n->t('You have unassigned {assigneduser} from {card} on {board}') : $this->l10n->t('{user} has unassigned {assigneduser} from {card} on {board}');
 				break;
 			case self::SUBJECT_CARD_UPDATE_STACKID:
-				$subject = $ownActivity ? $this->l10n->t('You have moved the card {card} from {before} to {stack}') : $this->l10n->t('{user} has moved the card {card} from {before} to {stack}');
+				$subject = $ownActivity ? $this->l10n->t('You have moved the card {card} from {stackBefore} to {stack}') : $this->l10n->t('{user} has moved the card {card} from {stackBefore} to {stack}');
 				break;
 			case self::SUBJECT_ATTACHMENT_CREATE:
 				$subject = $ownActivity ? $this->l10n->t('You have added the attachment {attachment} to {card}') : $this->l10n->t('{user} has added the attachment {attachment} to {card}');
@@ -276,7 +270,6 @@ class ActivityManager {
 					}
 				}
 			}
-
 		} else {
 			try {
 				$events = [$this->createEvent($objectType, $entity, $subject)];
@@ -317,6 +310,8 @@ class ActivityManager {
 			case self::SUBJECT_BOARD_CREATE:
 			case self::SUBJECT_BOARD_UPDATE_TITLE:
 			case self::SUBJECT_BOARD_UPDATE_ARCHIVED:
+			case self::SUBJECT_BOARD_DELETE:
+			case self::SUBJECT_BOARD_RESTORE:
 			// Not defined as there is no activity for
 			// case self::SUBJECT_BOARD_UPDATE_COLOR
 				break;
@@ -341,7 +336,6 @@ class ActivityManager {
 			case self::SUBJECT_LABEL_UNASSING:
 			case self::SUBJECT_CARD_USER_ASSIGN:
 			case self::SUBJECT_CARD_USER_UNASSIGN:
-			case self::SUBJECT_CARD_MOVE_STACK:
 				$subjectParams = $this->findDetailsForCard($entity->getId());
 				$object = $entity;
 				break;
@@ -362,8 +356,10 @@ class ActivityManager {
 		}
 
 		if ($subject === self::SUBJECT_CARD_UPDATE_DESCRIPTION){
-			// TODO: Use ChangeSet and getDiff method
+			$subjectParams['diff'] = true;
-			$message = $additionalParams['after'];
+		}
+		if ($subject === self::SUBJECT_CARD_UPDATE_STACKID) {
+			$subjectParams['stackBefore'] = $this->stackMapper->find($additionalParams['before']);
 		}
 
 		$event = $this->manager->generateEvent();

lib/Activity/ChangeSet.php

@@ -24,7 +24,7 @@
 namespace OCA\Deck\Activity;
 
 
-class ChangeSet {
+class ChangeSet implements \JsonSerializable {
 
 	private $before;
 	private $after;
@@ -70,4 +70,21 @@ class ChangeSet {
 	public function getAfter() {
 		return $this->after;
 	}
+
+	/**
+	 * Specify data which should be serialized to JSON
+	 *
+	 * @link http://php.net/manual/en/jsonserializable.jsonserialize.php
+	 * @return mixed data which can be serialized by <b>json_encode</b>,
+	 * which is a value of any type other than a resource.
+	 * @since 5.4.0
+	 */
+	public function jsonSerialize() {
+		return [
+			'before' => $this->getBefore(),
+			'after' => $this->getAfter(),
+			'diff' => $this->getDiff(),
+			'type' => get_class($this->before)
+		];
+	}
 }

lib/Activity/DeckProvider.php

@@ -49,7 +49,6 @@ class DeckProvider implements IProvider {
 		$this->userManager = $userManager;
 	}
 
-
 	/**
 	 * @param string $language The language which should be used for translating, e.g. "en"
 	 * @param IEvent $event The current event which should be parsed
@@ -65,32 +64,15 @@ class DeckProvider implements IProvider {
 			throw new \InvalidArgumentException();
 		}
 
-		$event->setIcon($this->urlGenerator->imagePath('deck', 'deck-dark.svg'));
+		$event = $this->getIcon($event);
-		if (strpos($event->getSubject(), '_update') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('files', 'change.svg'));
-		}
-		if (strpos($event->getSubject(), '_create') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('files', 'add-color.svg'));
-		}
-		if (strpos($event->getSubject(), '_delete') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('files', 'delete-color.svg'));
-		}
-		if (strpos($event->getSubject(), 'archive') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('deck', 'archive.svg'));
-		}
-		if (strpos($event->getSubject(), '_restore') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('core', 'actions/history.svg'));
-		}
-		if (strpos($event->getSubject(), 'attachment_') !== false) {
-			$event->setIcon($this->urlGenerator->imagePath('core', 'places/files.svg'));
-		}
-
 
 		$subjectIdentifier = $event->getSubject();
 		$subjectParams = $event->getSubjectParameters();
-
 		$ownActivity = ($event->getAuthor() === $this->userId);
 
+		/**
+		 * Map stored parameter objects to rich string types
+		 */
 		$board = null;
 		if ($event->getObjectType() === ActivityManager::DECK_OBJECT_BOARD) {
 			$board = [
@@ -127,23 +109,10 @@ class DeckProvider implements IProvider {
 			]
 		];
 
-		if (array_key_exists('stack', $subjectParams)) {
+		$params = $this->parseParamForBoard('board', $subjectParams, $params);
-			$params['stack'] = [
+		$params = $this->parseParamForStack('stack', $subjectParams, $params);
-				'type' => 'highlight',
+		$params = $this->parseParamForStack('stackBefore', $subjectParams, $params);
-				'id' => $subjectParams['stack']['id'],
+		$params = $this->parseParamForAttachment('attachment', $subjectParams, $params);
-				'name' => $subjectParams['stack']['title'],
-				'link' => $this->deckUrl('/board/' . $subjectParams['stack']['boardId'] . '/'),
-			];
-		}
-
-		if (array_key_exists('board', $subjectParams)) {
-			$params['board'] = [
-				'type' => 'highlight',
-				'id' => $subjectParams['board']['id'],
-				'name' => $subjectParams['board']['title'],
-				'link' => $this->deckUrl('/board/' . $subjectParams['board']['id'] . '/'),
-			];
-		}
 
 		if (array_key_exists('label', $subjectParams)) {
 			$params['label'] = [
@@ -153,15 +122,6 @@ class DeckProvider implements IProvider {
 			];
 		}
 
-		if (array_key_exists('attachment', $subjectParams)) {
-			$params['attachment'] = [
-				'type' => 'highlight',
-				'id' => $subjectParams['attachment']['id'],
-				'name' => $subjectParams['attachment']['data'],
-				'link' => $this->urlGenerator->linkToRoute('deck.attachment.display', ['cardId' => $subjectParams['card']['id'], 'attachmentId' => $subjectParams['attachment']['id']]),
-			];
-		}
-
 		if (array_key_exists('assigneduser', $subjectParams)) {
 			$user = $this->userManager->get($subjectParams['assigneduser']);
 			$params['assigneduser'] = [
@@ -171,6 +131,76 @@ class DeckProvider implements IProvider {
 			];
 		}
 
+		$params = $this->parseParamsForAcl($subjectParams, $params);
+		$params = $this->parseParamsForChanges($subjectParams, $params, $event);
+
+		try {
+			$subject = $this->activityManager->getActivityFormat($subjectIdentifier, $subjectParams, $ownActivity);
+			$event->setParsedSubject($subject);
+			$event->setRichSubject($subject, $params);
+		} catch (\Exception $e) {
+		}
+		return $event;
+	}
+
+	private function getIcon(IEvent $event) {
+		$event->setIcon($this->urlGenerator->imagePath('deck', 'deck-dark.svg'));
+		if (strpos($event->getSubject(), '_update') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('files', 'change.svg'));
+		}
+		if (strpos($event->getSubject(), '_create') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('files', 'add-color.svg'));
+		}
+		if (strpos($event->getSubject(), '_delete') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('files', 'delete-color.svg'));
+		}
+		if (strpos($event->getSubject(), 'archive') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('deck', 'archive.svg'));
+		}
+		if (strpos($event->getSubject(), '_restore') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('core', 'actions/history.svg'));
+		}
+		if (strpos($event->getSubject(), 'attachment_') !== false) {
+			$event->setIcon($this->urlGenerator->imagePath('core', 'places/files.svg'));
+		}
+		return $event;
+	}
+
+	private function parseParamForBoard($paramName, $subjectParams, $params) {
+		if (array_key_exists($paramName, $subjectParams)) {
+			$params[$paramName] = [
+				'type' => 'highlight',
+				'id' => $subjectParams[$paramName]['id'],
+				'name' => $subjectParams[$paramName]['title'],
+				'link' => $this->deckUrl('/board/' . $subjectParams[$paramName]['id'] . '/'),
+			];
+		}
+		return $params;
+	}
+	private function parseParamForStack($paramName, $subjectParams, $params) {
+		if (array_key_exists($paramName, $subjectParams)) {
+			$params[$paramName] = [
+				'type' => 'highlight',
+				'id' => $subjectParams[$paramName]['id'],
+				'name' => $subjectParams[$paramName]['title'],
+			];
+		}
+		return $params;
+	}
+
+	private function parseParamForAttachment($paramName, $subjectParams, $params) {
+		if (array_key_exists('attachment', $subjectParams)) {
+			$params[$paramName] = [
+				'type' => 'highlight',
+				'id' => $subjectParams[$paramName]['id'],
+				'name' => $subjectParams[$paramName]['data'],
+				'link' => $this->urlGenerator->linkToRoute('deck.attachment.display', ['cardId' => $subjectParams['card']['id'], 'attachmentId' => $subjectParams['attachment']['id']]),
+			];
+		}
+		return $params;
+	}
+
+	private function parseParamsForAcl($subjectParams, $params) {
 		if (array_key_exists('acl', $subjectParams)) {
 			if ($subjectParams['acl']['type'] === Acl::PERMISSION_TYPE_USER) {
 				$user = $this->userManager->get($subjectParams['acl']['participant']);
@@ -186,9 +216,25 @@ class DeckProvider implements IProvider {
 					'name' => $subjectParams['acl']['participant']
 				];
 			}
-
+		}
+		return $params;
 	}
 
+	/**
+	 * Add diff to message if the subject parameter 'diff' is set, otherwise
+	 * the changed values are added to before/after
+	 *
+	 * @param $subjectParams
+	 * @param $params
+	 * @return mixed
+	 */
+	private function parseParamsForChanges($subjectParams, $params, $event) {
+		if (array_key_exists('diff', $subjectParams) && $subjectParams['diff']) {
+			$diff = new Diff();
+			$event->setMessage($subjectParams['after']);
+			$event->setParsedMessage('<pre class="visualdiff">' . $diff->render($subjectParams['before'], $subjectParams['after']) . '</pre>');
+			return $params;
+		}
 		if (array_key_exists('before', $subjectParams)) {
 			$params['before'] = [
 				'type' => 'highlight',
@@ -196,31 +242,14 @@ class DeckProvider implements IProvider {
 				'name' => $subjectParams['before']
 			];
 		}
-		if (array_key_exists('after', $subjectParams)) {
+		if (array_key_exists('before', $subjectParams)) {
 			$params['after'] = [
 				'type' => 'highlight',
 				'id' => $subjectParams['after'],
 				'name' => $subjectParams['after']
 			];
 		}
-
+		return $params;
-		try {
-			$subject = $this->activityManager->getActivityFormat($subjectIdentifier, $subjectParams, $ownActivity);
-		} catch (\Exception $e) {
-			return $event;
-		}
-
-		$event->setParsedSubject($subject);
-		$event->setRichSubject(
-			$subject,
-			$params
-		);
-		if ($event->getMessage() !== '') {
-			$diff = new Diff();
-			$event->setParsedMessage('<pre class="visualdiff">' . $diff->render($subjectParams['before'], $subjectParams['after']) . '</pre>');
-		}
-
-		return $event;
 	}
 
 	public function deckUrl($endpoint) {

tests/unit/Activity/ActivityManagerTest.php

@@ -23,6 +23,7 @@
 
 namespace OCA\Deck\Activity;
 
+use OCA\Deck\Db\AclMapper;
 use OCA\Deck\Db\AssignedUsers;
 use OCA\Deck\Db\Attachment;
 use OCA\Deck\Db\AttachmentMapper;
@@ -57,6 +58,8 @@ class ActivityManagerTest extends TestCase {
 	private $stackMapper;
 	/** @var AttachmentMapper|MockObject */
 	private $attachmentMapper;
+	/** @var AclMapper|MockObject */
+	private $aclMapper;
 	/** @var IL10N|MockObject */
 	private $l10n;
 	/** @var string */
@@ -69,6 +72,7 @@ class ActivityManagerTest extends TestCase {
 		$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->l10n = $this->createMock(IL10N::class);
 		$this->activityManager = new ActivityManager(
 			$this->manager,
@@ -77,6 +81,7 @@ class ActivityManagerTest extends TestCase {
 			$this->cardMapper,
 			$this->stackMapper,
 			$this->attachmentMapper,
+			$this->aclMapper,
 			$this->l10n,
 			$this->userId
 		);

tests/unit/Activity/DeckProviderTest.php

@@ -91,7 +91,7 @@ class DeckProviderTest extends TestCase {
 
 	public function dataEventIcons() {
 		return [
-			[ActivityManager::SUBJECT_CARD_MOVE_STACK, 'deck', 'deck-dark.svg'],
+			[ActivityManager::SUBJECT_LABEL_ASSIGN, 'deck', 'deck-dark.svg'],
 			[ActivityManager::SUBJECT_CARD_CREATE, 'files', 'add-color.svg'],
 			[ActivityManager::SUBJECT_CARD_UPDATE, 'files', 'change.svg'],
 			[ActivityManager::SUBJECT_CARD_DELETE, 'files', 'delete-color.svg'],
@@ -237,7 +237,8 @@ class DeckProviderTest extends TestCase {
 		$event->setApp('deck');
 		$event->setSubject(ActivityManager::SUBJECT_CARD_UPDATE_DESCRIPTION, [
 			'before' => 'ABC',
-			'after' => 'BCD'
+			'after' => 'BCD',
+			'diff' => true,
 		]);
 		$event->setAffectedUser($this->userId);
 		$event->setAuthor($this->userId);
@@ -257,15 +258,12 @@ class DeckProviderTest extends TestCase {
 				'id' => 'admin',
 				'name' => 'Administrator',
 			],
-			'before' => ['type' => 'highlight', 'id' => 'ABC', 'name' => 'ABC'],
-			'after' => ['type' => 'highlight', 'id' => 'BCD', 'name' => 'BCD']
 		];
 		$this->assertEquals($data, $event->getRichSubjectParameters());
 		$this->assertEquals('test string {board}', $event->getParsedSubject());
 		$this->assertEquals('test string {board}', $event->getRichSubject());
 		$this->assertEquals('BCD', $event->getMessage());
 		$this->assertEquals('<pre class="visualdiff"><del>A</del>BC<ins>D</ins></pre>', $event->getParsedMessage());
-
 	}
 
 }