Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: perform bulk message actions #10280

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions lib/Controller/MessagesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,14 @@
use OCA\Mail\Service\AccountService;
use OCA\Mail\Service\AiIntegrations\AiIntegrationsService;
use OCA\Mail\Service\ItineraryService;
use OCA\Mail\Service\MessageOperationService;
use OCA\Mail\Service\SmimeService;
use OCA\Mail\Service\SnoozeService;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\FrontpageRoute;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\OpenAPI;
use OCP\AppFramework\Http\ContentSecurityPolicy;
use OCP\AppFramework\Http\JSONResponse;
Expand Down Expand Up @@ -74,7 +77,8 @@
private SnoozeService $snoozeService;
private AiIntegrationsService $aiIntegrationService;

public function __construct(string $appName,
public function __construct(
string $appName,
IRequest $request,
AccountService $accountService,
IMailManager $mailManager,
Expand All @@ -94,7 +98,9 @@
IDkimService $dkimService,
IUserPreferences $preferences,
SnoozeService $snoozeService,
AiIntegrationsService $aiIntegrationService) {
AiIntegrationsService $aiIntegrationService,
private MessageOperationService $messageOperationService,
) {
parent::__construct($appName, $request);
$this->accountService = $accountService;
$this->mailManager = $mailManager;
Expand Down Expand Up @@ -782,6 +788,24 @@
return new JSONResponse();
}

/**
*
* @NoAdminRequired
*
* @param array<int,int> $identifiers
* @param array<int,string> $flags
*
* @return JSONResponse
*/
#[FrontpageRoute(verb: 'PUT', url: '/api/messages/flags')]

Check warning on line 800 in lib/Controller/MessagesController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/MessagesController.php#L800

Added line #L800 was not covered by tests
#[NoAdminRequired]
#[TrapError]
public function changeFlags(array $identifiers, array $flags): JSONResponse {
return new JSONResponse(
$this->messageOperationService->changeFlags($this->currentUserId, $identifiers, $flags)

Check failure on line 805 in lib/Controller/MessagesController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidArgument

lib/Controller/MessagesController.php:805:84: InvalidArgument: Argument 3 of OCA\Mail\Service\MessageOperationService::changeFlags expects array<string, bool>, but array<int, string> provided (see https://psalm.dev/004)
);

Check warning on line 806 in lib/Controller/MessagesController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/MessagesController.php#L804-L806

Added lines #L804 - L806 were not covered by tests
}

/**
* @NoAdminRequired
*
Expand Down Expand Up @@ -882,7 +906,7 @@
#[TrapError]
public function smartReply(int $messageId):JSONResponse {
try {
$message = $this->mailManager->getMessage($this->currentUserId, $messageId);

Check failure on line 909 in lib/Controller/MessagesController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullArgument

lib/Controller/MessagesController.php:909:46: PossiblyNullArgument: Argument 1 of OCA\Mail\Contracts\IMailManager::getMessage cannot be null, possibly null value provided (see https://psalm.dev/078)
$mailbox = $this->mailManager->getMailbox($this->currentUserId, $message->getMailboxId());
$account = $this->accountService->find($this->currentUserId, $mailbox->getAccountId());
} catch (DoesNotExistException $e) {
Expand Down
19 changes: 19 additions & 0 deletions lib/Db/MailAccountMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,25 @@
return $this->findEntity($query);
}

/**
* Finds all mail accounts by account ids
*
* @param array<int,int> $identifiers
*
* @return array<int,MailAccount>
*/
public function findByIds(array $identifiers): array {

Check warning on line 76 in lib/Db/MailAccountMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MailAccountMapper.php#L76

Added line #L76 was not covered by tests

$cmd = $this->db->getQueryBuilder();
$cmd->select('*')
->from($this->getTableName())
->where(
$cmd->expr()->in('id', $cmd->createNamedParameter($identifiers, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY)
);

Check warning on line 83 in lib/Db/MailAccountMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MailAccountMapper.php#L78-L83

Added lines #L78 - L83 were not covered by tests

return $this->findEntities($cmd);

Check warning on line 85 in lib/Db/MailAccountMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MailAccountMapper.php#L85

Added line #L85 was not covered by tests
}

/**
* Finds all Mail Accounts by user id existing for this user
*
Expand Down
4 changes: 3 additions & 1 deletion lib/Db/MailboxMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ public function findById(int $id): Mailbox {
}

/**
* @return Mailbox[]
* @param array<string> $ids
*
* @return array<Mailbox>
*
* @throws Exception
*/
Expand Down
22 changes: 22 additions & 0 deletions lib/Db/MessageMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,28 @@
}, array_chunk($ids, 1000));
}

/**
* @param array<int,int> $identifiers
*
* @return array
*/
public function findMailboxAndUid(array $identifiers): array {

Check warning on line 186 in lib/Db/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MessageMapper.php#L186

Added line #L186 was not covered by tests

if ($identifiers === []) {
return [];

Check warning on line 189 in lib/Db/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MessageMapper.php#L188-L189

Added lines #L188 - L189 were not covered by tests
}

$cmd = $this->db->getQueryBuilder();
$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)
);

Check warning on line 197 in lib/Db/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MessageMapper.php#L192-L197

Added lines #L192 - L197 were not covered by tests

return $cmd->executeQuery()->fetchAll();

Check warning on line 199 in lib/Db/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/MessageMapper.php#L199

Added line #L199 was not covered by tests

}

/**
* @param Account $account
*
Expand Down
38 changes: 38 additions & 0 deletions lib/IMAP/MessageMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,44 @@
);
}

/**
* @throws Horde_Imap_Client_Exception
*/
public function setFlags(

Check warning on line 441 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L441

Added line #L441 was not covered by tests
Horde_Imap_Client_Socket $client,
Mailbox $mailbox,
array $uids,
array $addFlags = [],
array $removeFlags = [],
array $replaceFlags = [],
): array {

if (count($uids) === 0) {
return [];

Check warning on line 451 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L450-L451

Added lines #L450 - L451 were not covered by tests
}

$cmd = [];

Check warning on line 454 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L454

Added line #L454 was not covered by tests

if (count($replaceFlags) > 0) {
$cmd['replace'] = $replaceFlags;

Check warning on line 457 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L456-L457

Added lines #L456 - L457 were not covered by tests
} else {
if (count($addFlags) > 0) {
$cmd['add'] = $addFlags;

Check warning on line 460 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L459-L460

Added lines #L459 - L460 were not covered by tests
}
if (count($removeFlags) > 0) {
$cmd['remove'] = $removeFlags;

Check warning on line 463 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L462-L463

Added lines #L462 - L463 were not covered by tests
}
}

if (count($cmd) > 0) {
$cmd['ids'] = new Horde_Imap_Client_Ids($uids);
$result = $client->store($mailbox->getName(), $cmd);
return $result->ids;

Check warning on line 470 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L467-L470

Added lines #L467 - L470 were not covered by tests
}

return [];

Check warning on line 473 in lib/IMAP/MessageMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/MessageMapper.php#L473

Added line #L473 was not covered by tests
}

/**
* @param Horde_Imap_Client_Socket $client
* @param Mailbox $mailbox
Expand Down
153 changes: 153 additions & 0 deletions lib/Service/MessageOperationService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Mail\Service;

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 Throwable;

class MessageOperationService {

public function __construct(

Check warning on line 22 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L22

Added line #L22 was not covered by tests
protected IMAPClientFactory $clientFactory,
protected MailAccountMapper $accountMapper,
protected MailboxMapper $mailboxMapper,
protected MessageMapper $messageMapper,
protected MailManager $mailManager,
protected ImapMessageMapper $imapMessageMapper,
) {
}

Check warning on line 30 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L30

Added line #L30 was not covered by tests

/**
* 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) {

Check failure on line 42 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidReturnStatement

lib/Service/MessageOperationService.php:42:10: InvalidReturnStatement: The inferred type 'array<array-key, mixed|non-empty-list<array{id: mixed, uid: mixed}>>|mixed' does not match the declared return type 'array<int, array<array-key, array{0: int, 1: int}>>' for OCA\Mail\Service\MessageOperationService::groupByMailbox (see https://psalm.dev/128)
if (!isset($carry[$pair['mailbox_id']])) {
$carry[$pair['mailbox_id']] = [];

Check warning on line 44 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L41-L44

Added lines #L41 - L44 were not covered by tests
}
$carry[$pair['mailbox_id']][] = ['id' => $pair['id'], 'uid' => $pair['uid']];
return $carry;
}, []);

Check warning on line 48 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L46-L48

Added lines #L46 - L48 were not covered by tests
}

/**
* convert mailbox collection to grouped collections by account id
*
* [mailbox] to [account_id => [mailbox]]
*
* @param array<\OCA\Mail\Db\MailBox> $collection

Check failure on line 56 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidClass

lib/Service/MessageOperationService.php:56:12: InvalidClass: Class, interface or enum OCA\Mail\Db\MailBox has wrong casing (see https://psalm.dev/007)
*
* @return array<int,array<\OCA\Mail\Db\MailBox>>

Check failure on line 58 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidClass

lib/Service/MessageOperationService.php:58:13: InvalidClass: Class, interface or enum OCA\Mail\Db\MailBox has wrong casing (see https://psalm.dev/007)
*/
protected function groupByAccount(array $collection) {
return array_reduce($collection, function ($carry, $entry) {
if (!isset($carry[$entry->getAccountId()])) {
$carry[$entry->getAccountId()] = [];

Check warning on line 63 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L60-L63

Added lines #L60 - L63 were not covered by tests
}
$carry[$entry->getAccountId()][] = $entry;
return $carry;
}, []);

Check warning on line 67 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L65-L67

Added lines #L65 - L67 were not covered by tests
}

/**
* generates operation status responses for each message
*
* @param array<int,bool> &$results
* @param bool $value
* @param array<\OCA\Mail\Db\MailBox> $mailboxes

Check failure on line 75 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidClass

lib/Service/MessageOperationService.php:75:12: InvalidClass: Class, interface or enum OCA\Mail\Db\MailBox has wrong casing (see https://psalm.dev/007)
* @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;

Check warning on line 81 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L78-L81

Added lines #L78 - L81 were not covered by tests

Check failure on line 81 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidArrayOffset

lib/Service/MessageOperationService.php:81:14: InvalidArrayOffset: Cannot access value on variable $message using offset value of 'id', expecting 0 or 1 (see https://psalm.dev/115)
}
}
}

/**
* Set/Unset system flags or keywords
*
* @param string $userId system user id
* @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): array {

Check warning on line 95 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L95

Added line #L95 was not covered by tests

// retrieve message meta data [uid, mailbox_id] for all messages and group by mailbox id
$messages = $this->groupByMailbox($this->messageMapper->findMailboxAndUid($identifiers));

Check warning on line 98 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L98

Added line #L98 was not covered by tests
// retrieve all mailboxes and group by account
$mailboxes = $this->groupByAccount($this->mailboxMapper->findByIds(array_keys($messages)));

Check warning on line 100 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L100

Added line #L100 was not covered by tests

Check failure on line 100 in lib/Service/MessageOperationService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidArgument

lib/Service/MessageOperationService.php:100:70: InvalidArgument: Argument 1 of OCA\Mail\Db\MailboxMapper::findByIds expects array<array-key, string>, but list<int> provided (see https://psalm.dev/004)
// retrieve all accounts
$accounts = $this->accountMapper->findByIds(array_keys($mailboxes));

Check warning on line 102 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L102

Added line #L102 was not covered by tests
// process every account
$results = [];
foreach ($accounts as $account) {
$account = new Account($account);

Check warning on line 106 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L104-L106

Added lines #L104 - L106 were not covered by tests
// determine if account belongs to the user and skip if not
if ($account->getUserId() != $userId) {

Check warning on line 108 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L108

Added line #L108 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to check, but please move this down into the db when looking up accounts. Make it a WHERE id IN (xxx) AND user_id = 'sebastian'. That avoids loaded any foreign accounts.

// add messages to results as failed
$this->generateResult($results, false, $mailboxes[$account->getId()], $messages);
continue;

Check warning on line 111 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L110-L111

Added lines #L110 - L111 were not covered by tests
}
$client = $this->clientFactory->getClient($account);

Check warning on line 113 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L113

Added line #L113 was not covered by tests
// process every mailbox
foreach ($mailboxes[$account->getId()] as $mailbox) {

Check warning on line 115 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L115

Added line #L115 was not covered by tests
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);
$imapFlags = $this->mailManager->filterFlags($client, $account, $flag, $mailbox->getName());
if (empty($imapFlags)) {
continue;

Check warning on line 124 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L118-L124

Added lines #L118 - L124 were not covered by tests
}
if ($value) {
$addFlags = array_merge($addFlags, $imapFlags);

Check warning on line 127 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L126-L127

Added lines #L126 - L127 were not covered by tests
} else {
$removeFlags = array_merge($removeFlags, $imapFlags);

Check warning on line 129 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L129

Added line #L129 was not covered by tests
}
}
// apply flags to messages on server
$this->imapMessageMapper->setFlags(
$client,
$mailbox,
array_column($messages[$mailbox->getId()], 'uid'),
$addFlags,
$removeFlags
);

Check warning on line 139 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L133-L139

Added lines #L133 - L139 were not covered by tests
// add messages to results as successful
$this->generateResult($results, true, [$mailbox], $messages);
} catch (Throwable $e) {

Check warning on line 142 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L141-L142

Added lines #L141 - L142 were not covered by tests
// add messages to results as failed
$this->generateResult($results, false, [$mailbox], $messages);

Check warning on line 144 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L144

Added line #L144 was not covered by tests
}
}
$client->logout();

Check warning on line 147 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L147

Added line #L147 was not covered by tests
}

return $results;

Check warning on line 150 in lib/Service/MessageOperationService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/MessageOperationService.php#L150

Added line #L150 was not covered by tests
}

}
Loading
Loading