diff --git a/lib/Controller/OutboxController.php b/lib/Controller/OutboxController.php index 382333dabc..104d85fe3e 100644 --- a/lib/Controller/OutboxController.php +++ b/lib/Controller/OutboxController.php @@ -244,6 +244,14 @@ public function send(int $id): JsonResponse { $message = $this->service->sendMessage($message, $account); + if ($message->getStatus() === LocalMessage::STATUS_NO_SENT_MAILBOX) { + return JsonResponse::error( + 'Cannot send message: No "Sent" folder configured for this account.', + Http::STATUS_FORBIDDEN, + [$message] + ); + } + if ($message->getStatus() !== LocalMessage::STATUS_PROCESSED) { return JsonResponse::error('Could not send message', Http::STATUS_INTERNAL_SERVER_ERROR, [$message]); } diff --git a/lib/Send/Chain.php b/lib/Send/Chain.php index a55aeebbee..15035a01fc 100644 --- a/lib/Send/Chain.php +++ b/lib/Send/Chain.php @@ -10,10 +10,12 @@ use OCA\Mail\Account; use OCA\Mail\Db\LocalMessage; use OCA\Mail\Db\LocalMessageMapper; +use OCA\Mail\Exception\SentMailboxNotSetException; use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\IMAPClientFactory; use OCA\Mail\Service\Attachment\AttachmentService; use OCP\DB\Exception; +use Psr\Log\LoggerInterface; class Chain { public function __construct( @@ -25,6 +27,7 @@ public function __construct( private AttachmentService $attachmentService, private LocalMessageMapper $localMessageMapper, private IMAPClientFactory $clientFactory, + private LoggerInterface $logger, ) { } @@ -52,6 +55,14 @@ public function process(Account $account, LocalMessage $localMessage): LocalMess $client = $this->clientFactory->getClient($account); try { $result = $handlers->process($account, $localMessage, $client); + } catch (SentMailboxNotSetException $e) { + $this->logger->info('Message send aborted: No sent mailbox configured', [ + 'accountId' => $account->getId(), + 'messageId' => $localMessage->getId(), + ]); + // Set status to indicate the specific error + $localMessage->setStatus(LocalMessage::STATUS_NO_SENT_MAILBOX); + return $this->localMessageMapper->update($localMessage); } finally { $client->logout(); } diff --git a/lib/Send/SentMailboxHandler.php b/lib/Send/SentMailboxHandler.php index c86b4fcb61..6a48800432 100644 --- a/lib/Send/SentMailboxHandler.php +++ b/lib/Send/SentMailboxHandler.php @@ -10,8 +10,15 @@ use Horde_Imap_Client_Socket; use OCA\Mail\Account; use OCA\Mail\Db\LocalMessage; +use OCA\Mail\Exception\SentMailboxNotSetException; +use Psr\Log\LoggerInterface; class SentMailboxHandler extends AHandler { + public function __construct( + private LoggerInterface $logger, + ) { + } + #[\Override] public function process( Account $account, @@ -19,8 +26,11 @@ public function process( Horde_Imap_Client_Socket $client, ): LocalMessage { if ($account->getMailAccount()->getSentMailboxId() === null) { - $localMessage->setStatus(LocalMessage::STATUS_NO_SENT_MAILBOX); - return $localMessage; + $this->logger->warning('No sent mailbox configured for account', [ + 'accountId' => $account->getId(), + 'userId' => $account->getUserId(), + ]); + throw new SentMailboxNotSetException(); } return $this->processNext($account, $localMessage, $client); } diff --git a/src/components/OutboxMessageListItem.vue b/src/components/OutboxMessageListItem.vue index e8121222fa..9447482811 100644 --- a/src/components/OutboxMessageListItem.vue +++ b/src/components/OutboxMessageListItem.vue @@ -33,7 +33,7 @@ {{ t('mail', 'Send now') }} @@ -70,6 +70,7 @@ import logger from '../logger.js' import OutboxAvatarMixin from '../mixins/OutboxAvatarMixin.js' import { STATUS_IMAP_SENT_MAILBOX_FAIL, + STATUS_NO_SENT_MAILBOX, STATUS_RAW, STATUS_SMTP_ERROR, UNDO_DELAY, @@ -115,7 +116,9 @@ export default { }, details() { - if (this.message.status === STATUS_IMAP_SENT_MAILBOX_FAIL) { + if (this.message.status === STATUS_NO_SENT_MAILBOX) { + return this.t('mail', 'No "Sent" folder configured. Please pick one in the account settings.') + } else if (this.message.status === STATUS_IMAP_SENT_MAILBOX_FAIL) { return this.t('mail', 'Could not copy to "Sent" folder') } else if (this.message.status === STATUS_SMTP_ERROR) { return this.t('mail', 'Mail server error') @@ -145,6 +148,10 @@ export default { return STATUS_IMAP_SENT_MAILBOX_FAIL }, + statusNoSentMailbox() { + return STATUS_NO_SENT_MAILBOX + }, + statusSmtpError() { return STATUS_SMTP_ERROR }, diff --git a/src/store/constants.js b/src/store/constants.js index ee76ba8f4e..ab916a971e 100644 --- a/src/store/constants.js +++ b/src/store/constants.js @@ -14,6 +14,7 @@ export const UNDO_DELAY = TOAST_UNDO_TIMEOUT export const EDITOR_MODE_HTML = 'richtext' export const EDITOR_MODE_TEXT = 'plaintext' export const STATUS_RAW = 0 +export const STATUS_NO_SENT_MAILBOX = 1 export const STATUS_IMAP_SENT_MAILBOX_FAIL = 11 export const STATUS_SMTP_ERROR = 13 diff --git a/tests/Unit/Send/ChainTest.php b/tests/Unit/Send/ChainTest.php index 681f5e823b..97c702d382 100644 --- a/tests/Unit/Send/ChainTest.php +++ b/tests/Unit/Send/ChainTest.php @@ -15,6 +15,7 @@ use OCA\Mail\Db\LocalMessageMapper; use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\MessageMapper; +use OCA\Mail\Exception\SentMailboxNotSetException; use OCA\Mail\IMAP\IMAPClientFactory; use OCA\Mail\Send\AntiAbuseHandler; use OCA\Mail\Send\Chain; @@ -24,6 +25,7 @@ use OCA\Mail\Send\SentMailboxHandler; use OCA\Mail\Service\Attachment\AttachmentService; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; class ChainTest extends TestCase { private Chain $chain; @@ -36,6 +38,7 @@ class ChainTest extends TestCase { private AttachmentService|MockObject $attachmentService; private MockObject|LocalMessageMapper $localMessageMapper; private MockObject&IMAPClientFactory $clientFactory; + private LoggerInterface|MockObject $logger; protected function setUp(): void { $this->sentMailboxHandler = $this->createMock(SentMailboxHandler::class); @@ -46,6 +49,7 @@ protected function setUp(): void { $this->attachmentService = $this->createMock(AttachmentService::class); $this->localMessageMapper = $this->createMock(LocalMessageMapper::class); $this->clientFactory = $this->createMock(IMAPClientFactory::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->chain = new Chain($this->sentMailboxHandler, $this->antiAbuseHandler, $this->sendHandler, @@ -54,6 +58,7 @@ protected function setUp(): void { $this->attachmentService, $this->localMessageMapper, $this->clientFactory, + $this->logger, ); } @@ -128,4 +133,46 @@ public function testProcessNotProcessed() { $this->chain->process($account, $localMessage); } + + public function testProcessNoSentMailbox() { + $mailAccount = new MailAccount(); + $mailAccount->setUserId('bob'); + $mailAccount->setId(456); + $account = new Account($mailAccount); + $localMessage = new LocalMessage(); + $localMessage->setId(100); + $localMessage->setStatus(LocalMessage::STATUS_RAW); + $client = $this->createMock(Horde_Imap_Client_Socket::class); + $client->expects(self::once()) + ->method('logout'); + + $this->sentMailboxHandler->expects(self::once()) + ->method('setNext'); + $this->clientFactory->expects(self::once()) + ->method('getClient') + ->willReturn($client); + $this->sentMailboxHandler->expects(self::once()) + ->method('process') + ->with($account, $localMessage) + ->willThrowException(new SentMailboxNotSetException()); + $this->logger->expects(self::once()) + ->method('info') + ->with('Message send aborted: No sent mailbox configured', [ + 'accountId' => 456, + 'messageId' => 100, + ]); + $this->attachmentService->expects(self::never()) + ->method('deleteLocalMessageAttachments'); + $this->localMessageMapper->expects(self::never()) + ->method('deleteWithRecipients'); + $this->localMessageMapper->expects(self::once()) + ->method('update') + ->with($this->callback(function ($message) { + return $message->getStatus() === LocalMessage::STATUS_NO_SENT_MAILBOX; + })) + ->willReturnArgument(0); + + $result = $this->chain->process($account, $localMessage); + $this->assertEquals(LocalMessage::STATUS_NO_SENT_MAILBOX, $result->getStatus()); + } } diff --git a/tests/Unit/Send/SentMailboxHandlerTest.php b/tests/Unit/Send/SentMailboxHandlerTest.php index 0f29375f5d..3ead555d6f 100644 --- a/tests/Unit/Send/SentMailboxHandlerTest.php +++ b/tests/Unit/Send/SentMailboxHandlerTest.php @@ -12,17 +12,21 @@ use OCA\Mail\Account; use OCA\Mail\Db\LocalMessage; use OCA\Mail\Db\MailAccount; +use OCA\Mail\Exception\SentMailboxNotSetException; use OCA\Mail\Send\AntiAbuseHandler; use OCA\Mail\Send\SentMailboxHandler; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; class SentMailboxHandlerTest extends TestCase { private AntiAbuseHandler|MockObject $antiAbuseHandler; + private LoggerInterface|MockObject $logger; private SentMailboxHandler $handler; protected function setUp(): void { $this->antiAbuseHandler = $this->createMock(AntiAbuseHandler::class); - $this->handler = new SentMailboxHandler(); + $this->logger = $this->createMock(LoggerInterface::class); + $this->handler = new SentMailboxHandler($this->logger); $this->handler->setNext($this->antiAbuseHandler); } @@ -46,17 +50,20 @@ public function testNoSentMailbox(): void { $mailAccount->setUserId('bob'); $mailAccount->setId(123); $account = new Account($mailAccount); - $localMessage = $this->getMockBuilder(LocalMessage::class); - $localMessage->addMethods(['setStatus']); - $mock = $localMessage->getMock(); + $localMessage = new LocalMessage(); $client = $this->createMock(Horde_Imap_Client_Socket::class); - $mock->expects(self::once()) - ->method('setStatus') - ->with(LocalMessage::STATUS_NO_SENT_MAILBOX); + $this->logger->expects(self::once()) + ->method('warning') + ->with('No sent mailbox configured for account', [ + 'accountId' => 123, + 'userId' => 'bob', + ]); + $this->antiAbuseHandler->expects(self::never()) ->method('process'); - $this->handler->process($account, $mock, $client); + $this->expectException(SentMailboxNotSetException::class); + $this->handler->process($account, $localMessage, $client); } }