Clean up the code responsible for deleting a message

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

12 files changed, 358 insertions, 65 deletions

diff --git a/lib/Account.php b/lib/Account.php
index 1a2c009e7..78421c0d2 100644
--- a/lib/Account.php
+++ b/lib/Account.php
@@ -420,55 +420,6 @@ class Account implements JsonSerializable {
 	}
 
 	/**
-	 * @param string $sourceFolderId
-	 * @param int $messageId
-	 */
-	public function deleteMessage($sourceFolderId, $messageId) {
-		$mb = $this->getMailbox($sourceFolderId);
-		$hordeSourceMailBox = $mb->getHordeMailBox();
-		// by default we will create a 'Trash' folder if no trash is found
-		$trashId = "Trash";
-		$createTrash = true;
-
-		$trashFolders = $this->getSpecialFolder('trash', true);
-
-		if (count($trashFolders) !== 0) {
-			$trashId = $trashFolders[0]->getFolderId();
-			$createTrash = false;
-		} else {
-			// no trash -> guess
-			$trashes = array_filter($this->getMailboxes(), function($box) {
-				/**
-				 * @var Mailbox $box
-				 */
-				return (stripos($box->getDisplayName(), 'trash') !== false);
-			});
-			if (!empty($trashes)) {
-				$trashId = array_values($trashes);
-				$trashId = $trashId[0]->getFolderId();
-				$createTrash = false;
-			}
-		}
-
-		$hordeMessageIds = new Horde_Imap_Client_Ids($messageId);
-		$hordeTrashMailBox = new Horde_Imap_Client_Mailbox($trashId);
-
-		if ($sourceFolderId === $trashId) {
-			$this->getImapConnection()->expunge($hordeSourceMailBox,
-				array('ids' => $hordeMessageIds, 'delete' => true));
-
-			OC::$server->getLogger()->info("Message expunged: {message} from mailbox {mailbox}",
-				array('message' => $messageId, 'mailbox' => $sourceFolderId));
-		} else {
-			$this->getImapConnection()->copy($hordeSourceMailBox, $hordeTrashMailBox,
-				array('create' => $createTrash, 'move' => true, 'ids' => $hordeMessageIds));
-
-			OC::$server->getLogger()->info("Message moved to trash: {message} from mailbox {mailbox}",
-				array('message' => $messageId, 'mailbox' => $sourceFolderId, 'app' => 'mail'));
-		}
-	}
-
-	/**
 	 * @param int $messageId
 	 */
 	private function deleteDraft($messageId) {
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 6f12dca77..873eef879 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -29,7 +29,9 @@ use OCA\Mail\Contracts\IAvatarService;
 use OCA\Mail\Contracts\IMailManager;
 use OCA\Mail\Contracts\IMailTransmission;
 use OCA\Mail\Contracts\IUserPreferences;
+use OCA\Mail\Events\BeforeMessageDeletedEvent;
 use OCA\Mail\Http\Middleware\ErrorMiddleware;
+use OCA\Mail\Listener\MessageDeleteTrashCreatorListener;
 use OCA\Mail\Service\Attachment\AttachmentService;
 use OCA\Mail\Service\AvatarService;
 use OCA\Mail\Service\Group\IGroupService;
@@ -38,6 +40,7 @@ use OCA\Mail\Service\MailManager;
 use OCA\Mail\Service\MailTransmission;
 use OCA\Mail\Service\UserPreferenceSevice;
 use OCP\AppFramework\App;
+use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Util;
 
 class Application extends App {
@@ -46,6 +49,7 @@ class Application extends App {
 		parent::__construct('mail', $urlParams);
 
 		$this->initializeAppContainer();
+		$this->registerEvents();
 	}
 
 	private function initializeAppContainer() {
@@ -78,4 +82,11 @@ class Application extends App {
 		$container->registerAlias(IGroupService::class, NextcloudGroupService::class);
 	}
 
+	private function registerEvents(): void {
+		/** @var IEventDispatcher $dispatcher */
+		$dispatcher = $this->getContainer()->query(IEventDispatcher::class);
+
+		$dispatcher->addServiceListener(BeforeMessageDeletedEvent::class, MessageDeleteTrashCreatorListener::class);
+	}
+
 }
diff --git a/lib/Contracts/IMailManager.php b/lib/Contracts/IMailManager.php
index 6382c3c55..fee019836 100644
--- a/lib/Contracts/IMailManager.php
+++ b/lib/Contracts/IMailManager.php
@@ -71,11 +71,22 @@ interface IMailManager {
 	 * @param int $messageId
 	 * @param Account $destinationAccount
 	 * @param string $destFolderId
+	 *
+	 * @throws ServiceException
 	 */
 	public function moveMessage(Account $sourceAccount, string $sourceFolderId, int $messageId,
 								Account $destinationAccount, string $destFolderId);
 
 	/**
+	 * @param Account $account
+	 * @param string $mailboxId
+	 * @param int $messageId
+	 *
+	 * @throws ServiceException
+	 */
+	public function deleteMessage(Account $account, string $mailboxId, int $messageId): void;
+
+	/**
 	 * Mark all messages of a folder as read
 	 *
 	 * @param Account $account
diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php
index 94dce68da..04fcac35d 100755
--- a/lib/Controller/MessagesController.php
+++ b/lib/Controller/MessagesController.php
@@ -340,7 +340,7 @@ class MessagesController extends Controller {
 		$this->logger->debug("deleting message <$id> of folder <$folderId>, account <$accountId>");
 		try {
 			$account = $this->getAccount($accountId);
-			$account->deleteMessage(base64_decode($folderId), $id);
+			$this->mailManager->deleteMessage($account, base64_decode($folderId), $id);
 			return new JSONResponse();
 		} catch (DoesNotExistException $e) {
 			$this->logger->error("could not delete message <$id> of folder <$folderId>, "
diff --git a/lib/Db/Mailbox.php b/lib/Db/Mailbox.php
index da911f74c..e3509c6dc 100644
--- a/lib/Db/Mailbox.php
+++ b/lib/Db/Mailbox.php
@@ -43,7 +43,7 @@ use OCP\AppFramework\Db\Entity;
  * @method void setUnseen(int $unseen)
  * @method bool getSelectable()
  * @method void setSelectable(bool $selectable)
- * @method bool getSpecialUse()
+ * @method string getSpecialUse()
  * @method void setSpecialUse(string $specialUse)
  */
 class Mailbox extends Entity {
diff --git a/lib/Db/MailboxMapper.php b/lib/Db/MailboxMapper.php
index 5dd357c97..4cf8e1baf 100644
--- a/lib/Db/MailboxMapper.php
+++ b/lib/Db/MailboxMapper.php
@@ -24,6 +24,9 @@
 namespace OCA\Mail\Db;
 
 use OCA\Mail\Account;
+use OCA\Mail\Exception\ServiceException;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\AppFramework\Db\QBMapper;
 use OCP\IDBConnection;
 
@@ -48,4 +51,52 @@ class MailboxMapper extends QBMapper {
 		return $this->findEntities($select);
 	}
 
+	/**
+	 * @throws DoesNotExistException
+	 * @throws ServiceException
+	 */
+	public function find(Account $account, string $name): Mailbox {
+		$qb = $this->db->getQueryBuilder();
+
+		$select = $qb->select('*')
+			->from($this->getTableName())
+			->where(
+				$qb->expr()->eq('account_id', $qb->createNamedParameter($account->getId())),
+				$qb->expr()->eq('name', $qb->createNamedParameter($name))
+			);
+
+		try {
+			return $this->findEntity($select);
+		} catch (MultipleObjectsReturnedException $e) {
+			// Not possible due to DB constraints
+			throw new ServiceException("The impossible has happened", 42, $e);
+		}
+	}
+
+	/**
+	 * @throws DoesNotExistException
+	 */
+	public function findSpecial(Account $account, string $specialUse): Mailbox {
+		$mailboxes = $this->findAll($account);
+
+		// First, let's try to detect by special use attribute
+		foreach ($mailboxes as $mailbox) {
+			$specialUses = json_decode($mailbox->getSpecialUse(), true) ?? [];
+			if (in_array($specialUse, $specialUses, true)) {
+				return $mailbox;
+			}
+		}
+
+		// No luck so far, let's do another round and just guess
+		foreach ($mailboxes as $mailbox) {
+			// TODO: also check localized name
+			if (strtolower($mailbox->getName()) === strtolower($specialUse)) {
+				return $mailbox;
+			}
+		}
+
+		// Give up
+		throw new DoesNotExistException("Special mailbox $specialUse does not exist");
+	}
+
 }
diff --git a/lib/Events/BeforeMessageDeletedEvent.php b/lib/Events/BeforeMessageDeletedEvent.php
new file mode 100644
index 000000000..928ee8abb
--- /dev/null
+++ b/lib/Events/BeforeMessageDeletedEvent.php
@@ -0,0 +1,60 @@
+<?php declare(strict_types=1);
+
+/**
+ * @copyright 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @author 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+namespace OCA\Mail\Events;
+
+use OCA\Mail\Account;
+use OCP\EventDispatcher\Event;
+
+class BeforeMessageDeletedEvent extends Event {
+
+	/** @var Account */
+	private $account;
+
+	/** @var string */
+	private $folderId;
+
+	/** @var int */
+	private $messageId;
+
+	public function __construct(Account $account, string $folderId, int $messageId) {
+		parent::__construct();
+		$this->account = $account;
+		$this->folderId = $folderId;
+		$this->messageId = $messageId;
+	}
+
+	public function getAccount(): Account {
+		return $this->account;
+	}
+
+	public function getFolderId(): string {
+		return $this->folderId;
+	}
+
+	public function getMessageId(): int {
+		return $this->messageId;
+	}
+
+
+}
diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php
index b47774d51..bbd19e796 100644
--- a/lib/IMAP/MailboxSync.php
+++ b/lib/IMAP/MailboxSync.php
@@ -66,8 +66,8 @@ class MailboxSync {
 		$this->logger = $logger;
 	}
 
-	public function sync(Account $account): void {
-		if ($account->getMailAccount()->getLastMailboxSync() >= ($this->timeFactory->getTime() - 7200)) {
+	public function sync(Account $account, $force = false): void {
+		if (!$force && $account->getMailAccount()->getLastMailboxSync() >= ($this->timeFactory->getTime() - 7200)) {
 			$this->logger->debug("account is up to date, skipping mailbox sync");
 			return;
 		}
diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php
index ef4b672a0..81c176cb4 100644
--- a/lib/IMAP/MessageMapper.php
+++ b/lib/IMAP/MessageMapper.php
@@ -30,15 +30,26 @@ use Horde_Imap_Client_Exception;
 use Horde_Imap_Client_Exception_NoSupportExtension;
 use Horde_Imap_Client_Fetch_Query;
 use Horde_Imap_Client_Ids;
+use OCA\Mail\Db\Mailbox;
+use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Folder;
 use OCA\Mail\Model\IMAPMessage;
+use OCP\ILogger;
 
 class MessageMapper {
 
+	/** @var ILogger */
+	private $logger;
+
+	public function __construct(ILogger $logger) {
+		$this->logger = $logger;
+	}
+
 	/**
 	 * @param Horde_Imap_Client_Base $client
 	 * @param Folder $mailbox
 	 * @param array $ids
+	 *
 	 * @return IMAPMessage[]
 	 * @throws Horde_Imap_Client_Exception
 	 * @throws Horde_Imap_Client_Exception_NoSupportExtension
@@ -76,16 +87,29 @@ class MessageMapper {
 	 * @param string $sourceFolderId
 	 * @param int $messageId
 	 * @param string $destFolderId
-	 * @throws Horde_Imap_Client_Exception
-	 * @throws Horde_Imap_Client_Exception_NoSupportExtension
 	 */
-	public function move(Horde_Imap_Client_Base $client, string $sourceFolderId,
-						 int $messageId, string $destFolderId) {
-		$client->copy($sourceFolderId, $destFolderId,
-			[
-				'ids' => new Horde_Imap_Client_Ids($messageId),
-				'move' => true,
-			]);
+	public function move(Horde_Imap_Client_Base $client,
+						 string $sourceFolderId,
+						 int $messageId,
+						 string $destFolderId): void {
+		try {
+			$client->copy($sourceFolderId, $destFolderId,
+				[
+					'ids' => new Horde_Imap_Client_Ids($messageId),
+					'move' => true,
+				]);
+		} catch (Horde_Imap_Client_Exception $e) {
+			$this->logger->logException(
+				$e,
+				['level' => ILogger::DEBUG]
+			);
+
+			throw new ServiceException(
+				"Could not move message $$messageId from $sourceFolderId to $destFolderId",
+				0,
+				$e
+			);
+		}
 	}
 
 	public function markAllRead(Horde_Imap_Client_Base $client,
@@ -97,4 +121,35 @@ class MessageMapper {
 		]);
 	}
 
+	/**
+	 * @throws ServiceException
+	 */
+	public function expunge(Horde_Imap_Client_Base $client,
+							string $mailbox,
+							int $id): void {
+		try {
+			$client->expunge(
+				$mailbox,
+				[
+					'ids' => new Horde_Imap_Client_Ids([$id]),
+					'delete' => true,
+				]);
+		} catch (Horde_Imap_Client_Exception $e) {
+			$this->logger->logException(
+				$e,
+				['level' => ILogger::DEBUG]
+			);
+
+			throw new ServiceException("Could not expunge message $id", 0, $e);
+		}
+
+		$this->logger->info(
+			"Message expunged: {message} from mailbox {mailbox}",
+			[
+				'message' => $id,
+				'mailbox' => $mailbox,
+			]
+		);
+	}
+
 }
diff --git a/lib/Listener/MessageDeleteTrashCreatorListener.php b/lib/Listener/MessageDeleteTrashCreatorListener.php
new file mode 100644
index 000000000..c13240458
--- /dev/null
+++ b/lib/Listener/MessageDeleteTrashCreatorListener.php
@@ -0,0 +1,101 @@
+<?php declare(strict_types=1);
+
+/**
+ * @copyright 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @author 2019 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+namespace OCA\Mail\Listener;
+
+use Horde_Imap_Client_Exception;
+use OCA\Mail\Account;
+use OCA\Mail\Db\MailboxMapper;
+use OCA\Mail\Events\BeforeMessageDeletedEvent;
+use OCA\Mail\IMAP\IMAPClientFactory;
+use OCA\Mail\IMAP\MailboxSync;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\EventDispatcher\Event;
+use OCP\EventDispatcher\IEventListener;
+use OCP\ILogger;
+
+class MessageDeleteTrashCreatorListener implements IEventListener {
+
+	/** @var MailboxMapper */
+	private $mailboxMapper;
+
+	/** @var IMAPClientFactory */
+	private $imapClientFactory;
+
+	/** @var MailboxSync */
+	private $mailboxSync;
+
+	/** @var ILogger */
+	private $logger;
+
+	public function __construct(MailboxMapper $mailboxMapper,
+								IMAPClientFactory $imapClientFactory,
+								MailboxSync $mailboxSync,
+								ILogger $logger) {
+		$this->mailboxMapper = $mailboxMapper;
+		$this->imapClientFactory = $imapClientFactory;
+		$this->mailboxSync = $mailboxSync;
+		$this->logger = $logger;
+	}
+
+	public function handle(Event $event): void {
+		if (!($event instanceof BeforeMessageDeletedEvent)) {
+			return;
+		}
+
+		try {
+			$this->mailboxMapper->findSpecial(
+				$event->getAccount(),
+				"trash"
+			);
+		} catch (DoesNotExistException $e) {
+			$this->logger->debug("Creating trash mailbox");
+			$this->createTrash($event->getAccount());
+			$this->logger->debug("Trash mailbox created");
+		}
+	}
+
+	private function createTrash(Account $account): void {
+		$client = $this->imapClientFactory->getClient($account);
+
+		try {
+			// TODO: localize mailbox name
+			$client->createMailbox(
+				"Trash",
+				[
+					"special_use" => [
+						\Horde_Imap_Client::SPECIALUSE_TRASH,
+					],
+				]
+			);
+		} catch (Horde_Imap_Client_Exception $e) {
+			$this->logger->logException($e, [
+				"message" => "Could not creat trash mailbox",
+			]);
+		}
+
+		// TODO: find a more elegant solution for updating the mailbox cache
+		$this->mailboxSync->sync($account, true);
+	}
+
+}
diff --git a/lib/Mailbox.php b/lib/Mailbox.php
index 934376e34..506425d55 100644
--- a/lib/Mailbox.php
+++ b/lib/Mailbox.php
@@ -152,7 +152,7 @@ class Mailbox implements IMailBox {
 
 	/**
 	 * Get message page
-	 * 
+	 *
 	 * Build the list of UIDs for the current page on the client side
 	 *
 	 * This is done by fetching a list of *all* UIDs and their data, sorting them
@@ -479,7 +479,7 @@ class Mailbox implements IMailBox {
 	/**
 	 * @return Horde_Imap_Client_Mailbox
 	 */
-	public function getHordeMailBox() {
+	public function getHordeMailBox(): Horde_Imap_Client_Mailbox {
 		return $this->mailBox;
 	}
 
diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php
index 9c5a865cf..62d43e485 100644
--- a/lib/Service/MailManager.php
+++ b/lib/Service/MailManager.php
@@ -29,6 +29,7 @@ use OCA\Mail\Account;
 use OCA\Mail\Contracts\IMailManager;
 use OCA\Mail\Db\Mailbox;
 use OCA\Mail\Db\MailboxMapper;
+use OCA\Mail\Events\BeforeMessageDeletedEvent;
 use OCA\Mail\Exception\ClientException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Folder;
@@ -40,6 +41,8 @@ use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\IMAP\Sync\Request;
 use OCA\Mail\IMAP\Sync\Response;
 use OCA\Mail\IMAP\Sync\Synchronizer;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\EventDispatcher\IEventDispatcher;
 
 class MailManager implements IMailManager {
 
@@ -61,18 +64,23 @@ class MailManager implements IMailManager {
 	/** @var MessageMapper */
 	private $messageMapper;
 
+	/** @var IEventDispatcher */
+	private $eventDispatcher;
+
 	public function __construct(IMAPClientFactory $imapClientFactory,
 								MailboxMapper $mailboxMapper,
 								MailboxSync $mailboxSync,
 								FolderMapper $folderMapper,
 								Synchronizer $synchronizer,
-								MessageMapper $messageMapper) {
+								MessageMapper $messageMapper,
+								IEventDispatcher $eventDispatcher) {
 		$this->imapClientFactory = $imapClientFactory;
 		$this->mailboxMapper = $mailboxMapper;
 		$this->mailboxSync = $mailboxSync;
 		$this->folderMapper = $folderMapper;
 		$this->synchronizer = $synchronizer;
 		$this->messageMapper = $messageMapper;
+		$this->eventDispatcher = $eventDispatcher;
 	}
 
 	/**
@@ -145,6 +153,8 @@ class MailManager implements IMailManager {
 	 * @param int $messageId
 	 * @param Account $destinationAccount
 	 * @param string $destFolderId
+	 *
+	 * @throws ServiceException
 	 */
 	public function moveMessage(Account $sourceAccount,
 								string $sourceFolderId,
@@ -164,10 +174,53 @@ class MailManager implements IMailManager {
 	}
 
 	/**
+	 * @todo evaluate if we should sync mailboxes first
+	 *
+	 * @throws ServiceException
+	 */
+	public function deleteMessage(Account $account,
+								  string $mailboxId,
+								  int $messageId): void {
+		$this->eventDispatcher->dispatch(
+			BeforeMessageDeletedEvent::class,
+			new BeforeMessageDeletedEvent($account, $mailboxId, $messageId)
+		);
+
+		try {
+			$sourceFolder = $this->mailboxMapper->find($account, $mailboxId);
+		} catch (DoesNotExistException $e) {
+			throw new ServiceException("Source mailbox $mailboxId does not exist", 0, $e);
+		}
+		try {
+			$trashFolder = $this->mailboxMapper->findSpecial($account, 'trash');
+		} catch (DoesNotExistException $e) {
+			throw new ServiceException("No trash folder", 0, $e);
+		}
+
+		if ($mailboxId === $trashFolder->getName()) {
+			// Delete inside trash -> expunge
+			$this->messageMapper->expunge(
+				$this->imapClientFactory->getClient($account),
+				$sourceFolder->getName(),
+				$messageId
+			);
+		} else {
+			$this->messageMapper->move(
+				$this->imapClientFactory->getClient($account),
+				$sourceFolder->getName(),
+				$messageId,
+				$trashFolder->getName()
+			);
+		}
+	}
+
+	/**
 	 * @param Account $account
 	 * @param string $sourceFolderId
 	 * @param string $destFolderId
 	 * @param int $messageId
+	 *
+	 * @throws ServiceException
 	 */
 	private function moveMessageOnSameAccount(Account $account,
 											  string $sourceFolderId,