fixup! fix: perform bulk message actions

Signed-off-by: SebastianKrupinski <[email protected]>
nextcloud · Nov 23, 2024 · fb2f861 · fb2f861
1 parent 02e2290
commit fb2f861
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 27 deletions.
diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php
@@ -799,10 +799,9 @@ public function setFlags(int $id, array $flags): JSONResponse {
 	#[NoAdminRequired]
 	#[TrapError]
 	public function changeFlags(array $identifiers, array $flags): JSONResponse {
-
-		$this->messageOperationService->changeFlags($this->currentUserId, $identifiers, $flags);
-		// TODO: add proper responses
-		return new JSONResponse();
+		return new JSONResponse(
+			$this->messageOperationService->changeFlags($this->currentUserId, $identifiers, $flags)
+		);
 	}
 
 	/**

diff --git a/lib/Db/MailboxMapper.php b/lib/Db/MailboxMapper.php
@@ -113,7 +113,9 @@ public function findById(int $id): Mailbox {
 	}
 
 	/**
-	 * @return Mailbox[]
+	 * @param array<string> $ids
+	 * 
+	 * @return array<Mailbox>
 	 *
 	 * @throws Exception
 	 */

diff --git a/lib/Db/MessageMapper.php b/lib/Db/MessageMapper.php
@@ -190,7 +190,7 @@ public function findMailboxAndUid(array $identifiers): array {
 		}
 
 		$cmd = $this->db->getQueryBuilder();
-		$cmd->select('mailbox_id', 'uid')
+		$cmd->select('id', 'mailbox_id', 'uid')
 			->from($this->getTableName())
 			->where(
 				$cmd->expr()->in('id', $cmd->createNamedParameter($identifiers, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY)

diff --git a/lib/Service/MessageOperationService.php b/lib/Service/MessageOperationService.php
@@ -9,14 +9,14 @@
 
 namespace OCA\Mail\Service;
 
-use Horde_Imap_Client_Exception;
 use OCA\Mail\Account;
 use OCA\Mail\Db\MailAccountMapper;
 use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\MessageMapper;
 use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MessageMapper as ImapMessageMapper;
 use OCA\Mail\Service\MailManager;
+use Throwable;
 
 class MessageOperationService {
 
@@ -29,18 +29,34 @@ public function __construct(
         protected ImapMessageMapper $imapMessageMapper
     ) {}
 
-    // group messages by mailbox ['mailbox_id' => [message_id, message_id]]
-    protected function groupByMailbox(array $collection) {
+    /**
+     * convert message collection to grouped collections by mailbox id 
+     * 
+     * [[mailbox_id, uid, id]] to [mailbox_id => [[id, uid]]]
+     * 
+     * @param array<array{0:int,1:int,2:int}> $collection
+     * 
+     * @return array<int,array<array{0:int,1:int}>>
+     */
+    protected function groupByMailbox(array $collection): array {
         return array_reduce($collection, function ($carry, $pair) {
             if (!isset($carry[$pair['mailbox_id']])) {
                 $carry[$pair['mailbox_id']] = [];
             }
-            $carry[$pair['mailbox_id']][] = $pair['uid'];
+            $carry[$pair['mailbox_id']][] = ['id' => $pair['id'], 'uid' => $pair['uid']];
             return $carry;
         }, []);
     }
 
-    // group mailboxes by account ['account_id' => [mailbox object]]
+    /**
+     * convert mailbox collection to grouped collections by account id 
+     * 
+     * [mailbox] to [account_id => [mailbox]]
+     * 
+     * @param array<\OCA\Mail\Db\MailBox> $collection
+     * 
+     * @return array<int,array<\OCA\Mail\Db\MailBox>>
+     */
     protected function groupByAccount(array $collection) {
         return array_reduce($collection, function ($carry, $entry) {
             if (!isset($carry[$entry->getAccountId()])) {
@@ -52,35 +68,57 @@ protected function groupByAccount(array $collection) {
     }
 
     /**
+     * generates operation status responses for each message
+     * 
+     * @param array<int,bool> &$results
+     * @param bool $value
+     * @param array<\OCA\Mail\Db\MailBox> $mailboxes
+     * @param array<int,array<array{0:int,1:int}>> $messages
+     */
+    protected function generateResult(array &$results, bool $value, array $mailboxes, array $messages) {
+        foreach ($mailboxes as $mailbox) {
+            foreach ($messages[$mailbox->getId()] as $message) {
+                $results[$message['id']] = $value;
+            }
+        }
+    }
+
+    /**
+     * Set/Unset system flags or keywords
+     * 
      * @param string $userId system user id
-     * @param array<int,int> $identifiers message ids
+     * @param array<int> $identifiers message ids
 	 * @param array<string,bool> $flags message flags
+     * 
+     * @return array<int,bool> operation results
      */
-    public function changeFlags(string $userId, array $identifiers, array $flags): void {
+    public function changeFlags(string $userId, array $identifiers, array $flags): array {
 
-        // retrieve meta data [uid, mailbox_id] for all messages
+        // retrieve message meta data [uid, mailbox_id] for all messages and group by mailbox id
         $messages = $this->groupByMailbox($this->messageMapper->findMailboxAndUid($identifiers));
-        // retrieve all mailboxes
+        // retrieve all mailboxes and group by account
         $mailboxes = $this->groupByAccount($this->mailboxMapper->findByIds(array_keys($messages)));
         // retrieve all accounts
         $accounts = $this->accountMapper->findByIds(array_keys($mailboxes));
-
+        // process every account
+        $results = [];
         foreach ($accounts as $account) {
             $account = new Account($account);
             // determine if account belongs to the user and skip if not
             if ($account->getUserId() != $userId) {
+                // add messages to results as failed
+                $this->generateResult($results, false, $mailboxes[$account->getId()], $messages);
                 continue;
             }
-
             $client = $this->clientFactory->getClient($account);
-
-            try {
-                foreach ($mailboxes[$account->getId()] as $mailbox) {
+            // process every mailbox
+            foreach ($mailboxes[$account->getId()] as $mailbox) {
+                try {
+                    // check if specific flags are supported and group them by action
                     $addFlags = [];
                     $removeFlags = [];
                     foreach ($flags as $flag => $value) {
                         $value = filter_var($value, FILTER_VALIDATE_BOOLEAN);
-                        // Only send system flags to the IMAP server as other flags might not be supported
                         $imapFlags = $this->mailManager->filterFlags($client, $account, $flag, $mailbox->getName());
                         if (empty($imapFlags)) {
                             continue;
@@ -91,21 +129,25 @@ public function changeFlags(string $userId, array $identifiers, array $flags): v
                             $removeFlags = array_merge($removeFlags, $imapFlags);
                         }
                     }
+                    // apply flags to messages on server
                     $this->imapMessageMapper->setFlags(
                         $client, 
-                        $mailbox, 
-                        $messages[$mailbox->getId()], 
+                        $mailbox,
+                        array_column($messages[$mailbox->getId()], 'uid'), 
                         $addFlags,
                         $removeFlags
                     );
+                    // add messages to results as successful
+                    $this->generateResult($results, true, [$mailbox], $messages);
+                } catch (Throwable $e) {
+                    // add messages to results as failed
+                    $this->generateResult($results, false, [$mailbox], $messages);
                 }
-            } catch (Horde_Imap_Client_Exception $e) {
-                // TODO: Add proper error handling
-            } finally {
-                $client->logout();
             }
+            $client->logout();
         }
 
+        return $results;
     }
 
 }