fixup! feat: check connection performance of mail service

Signed-off-by: SebastianKrupinski <[email protected]>
nextcloud · Feb 12, 2025 · c5a993c · c5a993c
1 parent ad1aea8
commit c5a993c
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 56 deletions.
diff --git a/lib/Db/ProvisioningMapper.php b/lib/Db/ProvisioningMapper.php
@@ -141,8 +141,10 @@ public function get(int $id): ?Provisioning {
 
 	/**
 	 * @since 4.2.0
+	 * 
+	 * @return array<int,string>
 	 */
-	public function uniqueImapHosts(): array {
+	public function findUniqueImapHosts(): array {
 		$query = $this->db->getQueryBuilder();
 		$query->selectDistinct('imap_host')
 			->from('mail_provisionings');

diff --git a/lib/SetupChecks/MailConnectionPerformance.php b/lib/SetupChecks/MailConnectionPerformance.php
@@ -28,11 +28,12 @@ public function __construct(
 		private MailAccountMapper $accountMapper,
 		private IMAPClientFactory $clientFactory,
 		private FolderMapper $folderMapper,
+		private MicroTime $microtime,
 	) {
 	}
 
 	public function getName(): string {
-		return $this->l10n->t('Mail Connection Performance');
+		return $this->l10n->t('mail connection performance');
 	}
 
 	public function getCategory(): string {
@@ -41,7 +42,7 @@ public function getCategory(): string {
 
 	public function run(): SetupResult {
 		// retrieve unique imap hosts for provisionings and abort if none exists
-		$hosts = $this->provisioningMapper->uniqueImapHosts();
+		$hosts = $this->provisioningMapper->findUniqueImapHosts();
 		if (empty($hosts)) {
 			return SetupResult::success();
 		}
@@ -57,14 +58,14 @@ public function run(): SetupResult {
 				$account = new Account($this->accountMapper->findById((int)$accountId));
 				$client = $this->clientFactory->getClient($account);
 				try {
-					$tStart = $this->microtime(true);
+					$tStart = $this->microtime->get(true);
 					// time login
 					$client->login();
-					$tLogin = $this->microtime(true);
+					$tLogin = $this->microtime->get(true);
 					// time operation
 					$list = $client->listMailboxes('*');
 					$status = $client->status(key($list));
-					$tOperation = $this->microtime(true);
+					$tOperation = $this->microtime->get(true);
 
 					$tests[$host][$accountId] = ['start' => $tStart, 'login' => $tLogin, 'operation' => $tOperation];
 				} catch (Throwable $e) {
@@ -104,8 +105,4 @@ public function run(): SetupResult {
 		return SetupResult::success();
 	}
 
-	public function microtime(bool $asFloat = false): mixed {
-		return microtime($asFloat);
-	}
-
 }
diff --git a/lib/SetupChecks/MicroTime.php b/lib/SetupChecks/MicroTime.php
@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Mail\SetupChecks;
+
+class MicroTime {
+
+	public function get(bool $asFloat = false): string|float {
+		return microtime($asFloat);
+	}
+
+}
diff --git a/tests/Integration/Db/ProvisioningMapperTest.php b/tests/Integration/Db/ProvisioningMapperTest.php
@@ -180,4 +180,26 @@ public function testGet() {
 			}
 		}
 	}
+
+	public function testFindUniqueImapHosts() {
+		$provisioning = new Provisioning();
+		$provisioning->setProvisioningDomain($this->data['provisioningDomain']);
+		$provisioning->setEmailTemplate($this->data['emailTemplate']);
+		$provisioning->setImapUser($this->data['imapUser']);
+		$provisioning->setImapHost($this->data['imapHost']);
+		$provisioning->setImapPort(42);
+		$provisioning->setImapSslMode($this->data['imapSslMode']);
+		$provisioning->setSmtpUser($this->data['smtpUser']);
+		$provisioning->setSmtpHost($this->data['smtpHost']);
+		$provisioning->setSmtpPort(24);
+		$provisioning->setSmtpSslMode($this->data['smtpSslMode']);
+		$provisioning->setSieveEnabled($this->data['sieveEnabled']);
+		$provisioning = $this->mapper->insert($provisioning);
+
+		$hosts = $this->mapper->findUniqueImapHosts();
+
+		$this->assertIsArray($hosts);
+		$this->assertNotEmpty($hosts);
+		$this->assertEquals($this->data['imapHost'], $hosts[0]);
+	}
 }
diff --git a/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php b/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php
@@ -18,47 +18,49 @@
 use OCA\Mail\IMAP\FolderMapper;
 use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\SetupChecks\MailConnectionPerformance;
+use OCA\Mail\SetupChecks\MicroTime;
 use OCP\IL10N;
 use OCP\SetupCheck\SetupResult;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\NullLogger;
+use Psr\Log\Test\TestLogger;
 
 class MailConnectionPerformanceTest extends TestCase {
 
 	private IL10N&MockObject $l10n;
-	private NullLogger $logger;
+	private TestLogger $logger;
 	private ProvisioningMapper&MockObject $provisioningMapper;
 	private MailAccountMapper&MockObject $accountMapper;
 	private IMAPClientFactory&MockObject $clientFactory;
 	private FolderMapper&MockObject $folderMapper;
-	private MailConnectionPerformance&MockObject $check;
+	private MicroTime&MockObject $microtime;
+	private MailConnectionPerformance $check;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->l10n = $this->createMock(L10N::class);
-		$this->logger = new NullLogger();
+		$this->logger = new TestLogger();
 		$this->provisioningMapper = $this->createMock(ProvisioningMapper::class);
 		$this->accountMapper = $this->createMock(MailAccountMapper::class);
 		$this->clientFactory = $this->createMock(IMAPClientFactory::class);
 		$this->folderMapper = $this->createMock(FolderMapper::class);
-
-		$this->check = $this->getMockBuilder(MailConnectionPerformance::class)
-			->setConstructorArgs([
-				$this->l10n,
-				$this->logger,
-				$this->provisioningMapper,
-				$this->accountMapper,
-				$this->clientFactory,
-				$this->folderMapper
-			])
-			->onlyMethods(['microtime'])
-			->getMock();
+		$this->microtime = $this->createMock(MicroTime::class);
+
+		$this->check = new MailConnectionPerformance(
+			$this->l10n,
+			$this->logger,
+			$this->provisioningMapper,
+			$this->accountMapper,
+			$this->clientFactory,
+			$this->folderMapper,
+			$this->microtime
+		);
 	}
 
 	public function testNoHosts(): void {
 		$this->provisioningMapper->expects($this->once())
-			->method('uniqueImapHosts')
+			->method('findUniqueImapHosts')
 			->willReturn([]);
 
 		$result = $this->check->run();
@@ -68,7 +70,7 @@ public function testNoHosts(): void {
 
 	public function testNoAccounts(): void {
 		$this->provisioningMapper->expects($this->once())
-			->method('uniqueImapHosts')
+			->method('findUniqueImapHosts')
 			->willReturn(['imap.example.com']);
 
 		$this->accountMapper->expects($this->once())
@@ -82,40 +84,36 @@ public function testNoAccounts(): void {
 	}
 
 	public function testConnectionSuccess(): void {
-
 		$account = new MailAccount(
 			[
-				'id' => 42,
-				'user_id' => 'user',
-				'imap_host' => 'imap.example.com',
-				'imap_user' => 'user',
-				'imap_password' => 'password',
-				'imap_port' => 143,
-				'imap_sslmode' => 'none',
+				'accountId' => 42,
+				'imapHost' => 'imap.example.com',
+				'imapUser' => 'user',
+				'imapPassword' => 'password',
+				'imapPort' => 143,
+				'imapSslMode' => 'none',
 			]
 		);
 
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+
 		$this->provisioningMapper->expects($this->once())
-			->method('uniqueImapHosts')
+			->method('findUniqueImapHosts')
 			->willReturn(['imap.example.com']);
 
 		$this->accountMapper->expects($this->once())
 			->method('getRandomAccountIdsByImapHost')
 			->with('imap.example.com')
 			->willReturn([42]);
-
 		$this->accountMapper->expects($this->once())
 			->method('findById')
 			->with(42)
 			->willReturn($account);
 
-		$client = $this->createMock(Horde_Imap_Client_Socket::class);
-
 		$client->expects($this->once())
 			->method('listMailboxes')
 			->with('*')
 			->willReturn(['INBOX' => []]);
-
 		$client->expects($this->once())
 			->method('status')
 			->with('INBOX')
@@ -126,7 +124,7 @@ public function testConnectionSuccess(): void {
 			->with(new Account($account))
 			->willReturn($client);
 
-		$this->check->method('microtime')
+		$this->microtime->method('get')
 			->willReturnOnConsecutiveCalls(0, .2, .2);
 
 		$result = $this->check->run();
@@ -136,40 +134,36 @@ public function testConnectionSuccess(): void {
 	}
 
 	public function testConnectionWarning(): void {
-
 		$account = new MailAccount(
 			[
-				'id' => 42,
-				'user_id' => 'user',
-				'imap_host' => 'imap.example.com',
-				'imap_user' => 'user',
-				'imap_password' => 'password',
-				'imap_port' => 143,
-				'imap_sslmode' => 'none',
+				'accountId' => 42,
+				'imapHost' => 'imap.example.com',
+				'imapUser' => 'user',
+				'imapPassword' => 'password',
+				'imapPort' => 143,
+				'imapSslMode' => 'none',
 			]
 		);
 
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+
 		$this->provisioningMapper->expects($this->once())
-			->method('uniqueImapHosts')
+			->method('findUniqueImapHosts')
 			->willReturn(['imap.example.com']);
 
 		$this->accountMapper->expects($this->once())
 			->method('getRandomAccountIdsByImapHost')
 			->with('imap.example.com')
 			->willReturn([42]);
-
 		$this->accountMapper->expects($this->once())
 			->method('findById')
 			->with(42)
 			->willReturn($account);
 
-		$client = $this->createMock(Horde_Imap_Client_Socket::class);
-
 		$client->expects($this->once())
 			->method('listMailboxes')
 			->with('*')
 			->willReturn(['INBOX' => []]);
-
 		$client->expects($this->once())
 			->method('status')
 			->with('INBOX')
@@ -179,13 +173,56 @@ public function testConnectionWarning(): void {
 			->method('getClient')
 			->with(new Account($account))
 			->willReturn($client);
-	
-		$this->check->method('microtime')
+
+		$this->microtime->method('get')
 			->willReturnOnConsecutiveCalls(0, 2, 3);
 
 		$result = $this->check->run();
 
 		$this->assertEquals(SetupResult::WARNING, $result->getSeverity());
 
 	}
+
+	public function testConnectionFailure(): void {
+		$account = new MailAccount(
+			[
+				'accountId' => 42,
+				'imapHost' => 'imap.example.com',
+				'imapUser' => 'user',
+				'imapPassword' => 'password',
+				'imapPort' => 143,
+				'imapSslMode' => 'none',
+			]
+		);
+
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+
+		$this->provisioningMapper->expects($this->once())
+			->method('findUniqueImapHosts')
+			->willReturn(['imap.example.com']);
+
+		$this->accountMapper->expects($this->once())
+			->method('getRandomAccountIdsByImapHost')
+			->with('imap.example.com')
+			->willReturn([42]);
+		$this->accountMapper->expects($this->once())
+			->method('findById')
+			->with(42)
+			->willReturn($account);
+
+		$this->clientFactory->expects($this->once())
+			->method('getClient')
+			->with(new Account($account))
+			->willReturn($client);
+
+		$client->expects($this->once())
+			->method('login')
+			->willThrowException(new \Exception('Login failed'));
+
+		$result = $this->check->run();
+
+		$this->assertEquals(SetupResult::SUCCESS, $result->getSeverity());
+		$this->assertTrue($this->logger->hasWarningThatContains('Error occurred while performing system check on mail account: 42'));
+	}
+
 }