Refactor message flag logic into a service method

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

11 files changed, 160 insertions, 59 deletions

diff --git a/lib/Contracts/IMailManager.php b/lib/Contracts/IMailManager.php
index 7d372d686..2b86e621c 100644
--- a/lib/Contracts/IMailManager.php
+++ b/lib/Contracts/IMailManager.php
@@ -115,4 +115,16 @@ interface IMailManager {
 	 */
 	public function markFolderAsRead(Account $account, string $folderId): void;
 
+	/**
+	 * @param Account $account
+	 * @param string $mailbox
+	 * @param int $uid
+	 * @param string $flag
+	 * @param bool $value
+	 *
+	 * @throws ClientException
+	 * @throws ServiceException
+	 */
+	public function flagMessage(Account $account, string $mailbox, int $uid, string $flag, bool $value): void;
+
 }
diff --git a/lib/Controller/AccountsController.php b/lib/Controller/AccountsController.php
index 73689deb6..2374e3a2e 100644
--- a/lib/Controller/AccountsController.php
+++ b/lib/Controller/AccountsController.php
@@ -235,6 +235,8 @@ class AccountsController extends Controller {
 	 * @param int $id
 	 *
 	 * @return JSONResponse
+	 *
+	 * @throws ClientException
 	 */
 	public function destroy($id): JSONResponse {
 		$this->accountService->delete($this->currentUserId, $id);
diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php
index d431539c5..2851e37c4 100755
--- a/lib/Controller/MessagesController.php
+++ b/lib/Controller/MessagesController.php
@@ -449,7 +449,7 @@ class MessagesController extends Controller {
 	 * @throws ServiceException
 	 */
 	public function setFlags(int $accountId, string $folderId, int $messageId, array $flags): JSONResponse {
-		$mailBox = $this->getFolder($accountId, $folderId);
+		$account = $this->accountService->find($this->currentUserId, $accountId);
 
 		foreach ($flags as $flag => $value) {
 			$value = filter_var($value, FILTER_VALIDATE_BOOLEAN);
@@ -457,7 +457,7 @@ class MessagesController extends Controller {
 				$flag = 'seen';
 				$value = !$value;
 			}
-			$mailBox->setMessageFlag($messageId, '\\' . $flag, $value);
+			$this->mailManager->flagMessage($account, base64_decode($folderId), $messageId, $flag, $value);
 		}
 		return new JSONResponse();
 	}
@@ -493,6 +493,7 @@ class MessagesController extends Controller {
 	 * @param string $folderId
 	 *
 	 * @return IMailBox
+	 * @deprecated
 	 *
 	 * @throws ClientException
 	 * @throws ServiceException
diff --git a/lib/Db/MailAccountMapper.php b/lib/Db/MailAccountMapper.php
index a483b5e78..90a7d2ac7 100644
--- a/lib/Db/MailAccountMapper.php
+++ b/lib/Db/MailAccountMapper.php
@@ -47,6 +47,8 @@ class MailAccountMapper extends QBMapper {
 	 * @param int $accountId
 	 *
 	 * @return MailAccount
+	 *
+	 * @throws DoesNotExistException
 	 */
 	public function find(string $userId, int $accountId): MailAccount {
 		$qb = $this->db->getQueryBuilder();
diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php
index ee1880324..4e32aa0c4 100644
--- a/lib/IMAP/MessageMapper.php
+++ b/lib/IMAP/MessageMapper.php
@@ -260,6 +260,22 @@ class MessageMapper {
 	}
 
 	/**
+	 * @throws Horde_Imap_Client_Exception
+	 */
+	public function removeFlag(Horde_Imap_Client_Socket $client,
+							Mailbox $mailbox,
+							int $uid,
+							string $flag): void {
+		$client->store(
+			$mailbox->getName(),
+			[
+				'ids' => new Horde_Imap_Client_Ids($uid),
+				'remove' => [$flag],
+			]
+		);
+	}
+
+	/**
 	 * @param Horde_Imap_Client_Socket $client
 	 * @param string $mailbox
 	 * @param int $id
diff --git a/lib/Mailbox.php b/lib/Mailbox.php
index 09143f70f..128fe3c06 100644
--- a/lib/Mailbox.php
+++ b/lib/Mailbox.php
@@ -33,7 +33,6 @@ namespace OCA\Mail;
 
 use Horde_Imap_Client;
 use Horde_Imap_Client_Exception;
-use Horde_Imap_Client_Ids;
 use Horde_Imap_Client_Mailbox;
 use Horde_Imap_Client_Socket;
 use OCA\Mail\Model\IMAPMessage;
@@ -323,30 +322,4 @@ class Mailbox implements IMailBox {
 		return reset($uids);
 	}
 
-	/**
-	 * @param int $uid
-	 * @param string $flag
-	 * @param boolean $add
-	 *
-	 * @return void
-	 */
-	public function setMessageFlag(int $uid, string $flag, $add) {
-		$options = [
-			'ids' => new Horde_Imap_Client_Ids($uid)
-		];
-		if ($add) {
-			$options['add'] = [$flag];
-		} else {
-			$options['remove'] = [$flag];
-		}
-		$this->conn->store($this->mailBox, $options);
-	}
-
-	/**
-	 * @return Horde_Imap_Client_Mailbox
-	 */
-	public function getHordeMailBox(): Horde_Imap_Client_Mailbox {
-		return $this->mailBox;
-	}
-
 }
diff --git a/lib/Service/AccountService.php b/lib/Service/AccountService.php
index 04f1be2ca..0795237ee 100644
--- a/lib/Service/AccountService.php
+++ b/lib/Service/AccountService.php
@@ -101,14 +101,24 @@ class AccountService {
 			throw new ClientException("Account $accountId does not exist or you don\'t have permission to access it");
 		}
 
-		return new Account($this->mapper->find($uid, $accountId));
+		try {
+			return new Account($this->mapper->find($uid, $accountId));
+		} catch (DoesNotExistException $e) {
+			throw new ClientException("Account $accountId does not exist or you don\'t have permission to access it");
+		}
 	}
 
 	/**
 	 * @param int $accountId
+	 *
+	 * @throws ClientException
 	 */
 	public function delete(string $currentUserId, int $accountId): void {
-		$mailAccount = $this->mapper->find($currentUserId, $accountId);
+		try {
+			$mailAccount = $this->mapper->find($currentUserId, $accountId);
+		} catch (DoesNotExistException $e) {
+			throw new ClientException("Account $accountId does not exist", 0, $e);
+		}
 		$this->aliasesService->deleteAll($accountId);
 		$this->mapper->delete($mailAccount);
 	}
diff --git a/lib/Service/IMailBox.php b/lib/Service/IMailBox.php
index 177e33d22..1d6d98646 100644
--- a/lib/Service/IMailBox.php
+++ b/lib/Service/IMailBox.php
@@ -54,13 +54,6 @@ interface IMailBox {
 	public function getAttachment(int $messageId, string $attachmentId): Attachment;
 
 	/**
-	 * @param int $messageId
-	 * @param string $flag
-	 * @param mixed $value
-	 */
-	public function setMessageFlag(int $messageId, string $flag, $value);
-
-	/**
 	 * @param int $flags
 	 * @return array
 	 */
diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php
index 63c3e23a1..7cdc69b16 100644
--- a/lib/Service/MailManager.php
+++ b/lib/Service/MailManager.php
@@ -23,6 +23,7 @@ declare(strict_types=1);
 
 namespace OCA\Mail\Service;
 
+use Horde_Imap_Client;
 use Horde_Imap_Client_Exception;
 use OCA\Mail\Account;
 use OCA\Mail\Contracts\IMailManager;
@@ -41,9 +42,23 @@ use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Model\IMAPMessage;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\EventDispatcher\IEventDispatcher;
+use function in_array;
+use function strtolower;
 
 class MailManager implements IMailManager {
 
+	/**
+	 * https://tools.ietf.org/html/rfc3501#section-2.3.2
+	 */
+	private const ALLOWED_FLAGS = [
+		Horde_Imap_Client::FLAG_SEEN,
+		Horde_Imap_Client::FLAG_ANSWERED,
+		Horde_Imap_Client::FLAG_FLAGGED,
+		Horde_Imap_Client::FLAG_DELETED,
+		Horde_Imap_Client::FLAG_DRAFT,
+		Horde_Imap_Client::FLAG_RECENT,
+	];
+
 	/** @var IMAPClientFactory */
 	private $imapClientFactory;
 
@@ -175,9 +190,9 @@ class MailManager implements IMailManager {
 	 * @param Account $destinationAccount
 	 * @param string $destFolderId
 	 *
+	 * @return void
 	 * @throws ServiceException
 	 *
-	 * @return void
 	 */
 	public function moveMessage(Account $sourceAccount,
 								string $sourceFolderId,
@@ -248,9 +263,9 @@ class MailManager implements IMailManager {
 	 * @param string $destFolderId
 	 * @param int $messageId
 	 *
+	 * @return void
 	 * @throws ServiceException
 	 *
-	 * @return void
 	 */
 	private function moveMessageOnSameAccount(Account $account,
 											  string $sourceFolderId,
@@ -267,4 +282,27 @@ class MailManager implements IMailManager {
 		$this->messageMapper->markAllRead($client, $folderId);
 	}
 
+	public function flagMessage(Account $account, string $mailbox, int $uid, string $flag, bool $value): void {
+		$client = $this->imapClientFactory->getClient($account);
+		try {
+			$mb = $this->mailboxMapper->find($account, $mailbox);
+		} catch (DoesNotExistException $e) {
+			throw new ClientException("Mailbox $mailbox does not exist", 0, $e);
+		}
+
+		// Only send system flags to the IMAP server as other flags might not be supported
+		if (in_array(strtolower("\\$flag"), self::ALLOWED_FLAGS, true)) {
+			try {
+				$normalized = '\\' . strtolower($flag);
+				if ($value) {
+					$this->messageMapper->addFlag($client, $mb, $uid, $normalized);
+				} else {
+					$this->messageMapper->removeFlag($client, $mb, $uid, $normalized);
+				}
+			} catch (Horde_Imap_Client_Exception $e) {
+				throw new ServiceException("Could not set message flag on IMAP: " . $e->getMessage(), $e->getCode(), $e);
+			}
+		}
+	}
+
 }
diff --git a/tests/Unit/Controller/MessagesControllerTest.php b/tests/Unit/Controller/MessagesControllerTest.php
index 1178a03c4..42acc2725 100644
--- a/tests/Unit/Controller/MessagesControllerTest.php
+++ b/tests/Unit/Controller/MessagesControllerTest.php
@@ -129,7 +129,7 @@ class MessagesControllerTest extends TestCase {
 			->method('getTime')
 			->willReturn(10000);
 		$this->oldFactory = \OC::$server->offsetGet(ITimeFactory::class);
-		\OC::$server->registerService(ITimeFactory::class, function() use ($timeFactory) {
+		\OC::$server->registerService(ITimeFactory::class, function () use ($timeFactory) {
 			return $timeFactory;
 		});
 
@@ -372,17 +372,17 @@ class MessagesControllerTest extends TestCase {
 			->method('find')
 			->with($this->equalTo($this->userId), $this->equalTo($accountId))
 			->will($this->returnValue($this->account));
-		$this->account->expects($this->once())
-			->method('getMailbox')
-			->with(base64_decode($folderId))
-			->will($this->returnValue($this->mailbox));
-		$this->mailbox->expects($this->once())
-			->method('setMessageFlag')
-			->with($messageId, '\\seen', true);
+		$this->mailManager->expects($this->once())
+			->method('flagMessage')
+			->with($this->account, 'my folder', $messageId, 'seen', true);
 
 		$expected = new JSONResponse();
-		$response = $this->controller->setFlags($accountId, $folderId, $messageId,
-			$flags);
+		$response = $this->controller->setFlags(
+			$accountId,
+			$folderId,
+			$messageId,
+			$flags
+		);
 
 		$this->assertEquals($expected, $response);
 	}
@@ -399,17 +399,17 @@ class MessagesControllerTest extends TestCase {
 			->method('find')
 			->with($this->equalTo($this->userId), $this->equalTo($accountId))
 			->will($this->returnValue($this->account));
-		$this->account->expects($this->once())
-			->method('getMailbox')
-			->with(base64_decode($folderId))
-			->will($this->returnValue($this->mailbox));
-		$this->mailbox->expects($this->once())
-			->method('setMessageFlag')
-			->with($messageId, '\\flagged', true);
+		$this->mailManager->expects($this->once())
+			->method('flagMessage')
+			->with($this->account, 'my folder', $messageId, 'flagged', true);
 
 		$expected = new JSONResponse();
-		$response = $this->controller->setFlags($accountId, $folderId, $messageId,
-			$flags);
+		$response = $this->controller->setFlags(
+			$accountId,
+			$folderId,
+			$messageId,
+			$flags
+		);
 
 		$this->assertEquals($expected, $response);
 	}
diff --git a/tests/Unit/Service/MailManagerTest.php b/tests/Unit/Service/MailManagerTest.php
index 1a4a728ed..0570a34cf 100644
--- a/tests/Unit/Service/MailManagerTest.php
+++ b/tests/Unit/Service/MailManagerTest.php
@@ -269,4 +269,58 @@ class MailManagerTest extends TestCase {
 		);
 	}
 
+	public function testSetCustomFlag(): void {
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+		$account = $this->createMock(Account::class);
+		$this->imapClientFactory->expects($this->once())
+			->method('getClient')
+			->willReturn($client);
+		$this->messageMapper->expects($this->never())
+			->method('addFlag');
+		$this->messageMapper->expects($this->never())
+			->method('removeFlag');
+
+		$this->manager->flagMessage($account, 'INBOX', 123, 'important', true);
+	}
+
+	public function testCaseInsensitiveFlag(): void {
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+		$account = $this->createMock(Account::class);
+		$this->imapClientFactory->expects($this->once())
+			->method('getClient')
+			->willReturn($client);
+		$mb = $this->createMock(Mailbox::class);
+		$this->mailboxMapper->expects($this->once())
+			->method('find')
+			->with($account, 'INBOX')
+			->willReturn($mb);
+		$this->messageMapper->expects($this->once())
+			->method('addFlag')
+			->with($client, $mb, 123, '\\seen');
+		$this->messageMapper->expects($this->never())
+			->method('removeFlag');
+
+		$this->manager->flagMessage($account, 'INBOX', 123, 'SeEn', true);
+	}
+
+	public function testRemoveFlag(): void {
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+		$account = $this->createMock(Account::class);
+		$this->imapClientFactory->expects($this->once())
+			->method('getClient')
+			->willReturn($client);
+		$mb = $this->createMock(Mailbox::class);
+		$this->mailboxMapper->expects($this->once())
+			->method('find')
+			->with($account, 'INBOX')
+			->willReturn($mb);
+		$this->messageMapper->expects($this->never())
+			->method('addFlag');
+		$this->messageMapper->expects($this->once())
+			->method('removeFlag')
+			->with($client, $mb, 123, '\\seen');
+
+		$this->manager->flagMessage($account, 'INBOX', 123, 'seen', false);
+	}
+
 }