Share Horde_Client between sync and perflags queryenhancement/remove-duplicate-login

Signed-off-by: Anna Larch <anna@nextcloud.com>

3 files changed, 19 insertions, 100 deletions

diff --git a/tests/Integration/Migration/MigrateImportantFromImapAndDbTest.php b/tests/Integration/Migration/MigrateImportantFromImapAndDbTest.php
index 548d63cef..79a4937ed 100644
--- a/tests/Integration/Migration/MigrateImportantFromImapAndDbTest.php
+++ b/tests/Integration/Migration/MigrateImportantFromImapAndDbTest.php
@@ -38,10 +38,6 @@ use OCA\Mail\Migration\MigrateImportantFromImapAndDb;
 use Psr\Log\LoggerInterface;
 
 class MigrateImportantFromImapAndDbTest extends TestCase {
-
-	/** @var MockObject */
-	private $clientFactory;
-
 	/** @var MockObject */
 	private $client;
 
@@ -64,7 +60,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->mailboxMapper = $this->createMock(MailboxMapper::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->migration = new MigrateImportantFromImapAndDb(
-			$this->clientFactory,
 			$this->messageMapper,
 			$this->mailboxMapper,
 			$this->logger
@@ -77,10 +72,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -91,7 +82,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapNoUids() {
@@ -99,10 +90,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$uids = [];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -112,7 +99,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapExceptionGetFlagged() {
@@ -120,10 +107,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$e = new Horde_Imap_Client_Exception('');
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -134,7 +117,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->method('debug');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapExceptionOnFlag() {
@@ -144,10 +127,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$e = new Horde_Imap_Client_Exception('');
 		$uids = [1,2,3,4];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -161,7 +140,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->with('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function migrateImportantFromDb() {
@@ -170,10 +149,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox->setId(1);
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -184,7 +159,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantFromDbNoUids() {
@@ -193,10 +168,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox->setId(1);
 		$uids = [];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -206,7 +177,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantFromDbExceptionOnFlag() {
@@ -217,10 +188,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$e = new Horde_Imap_Client_Exception('');
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -234,6 +201,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->with('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 }
diff --git a/tests/Unit/Migration/MigrateImportantFromImapAndDbTest.php b/tests/Unit/Migration/MigrateImportantFromImapAndDbTest.php
index 58b799000..7627c0bcb 100644
--- a/tests/Unit/Migration/MigrateImportantFromImapAndDbTest.php
+++ b/tests/Unit/Migration/MigrateImportantFromImapAndDbTest.php
@@ -33,7 +33,6 @@ use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Db\Tag;
 use OCA\Mail\Exception\ServiceException;
-use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\Migration\MigrateImportantFromImapAndDb;
 use Psr\Log\LoggerInterface;
 
@@ -58,13 +57,11 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 	private $migration;
 
 	protected function setUp(): void {
-		$this->clientFactory = $this->createMock(IMAPClientFactory::class);
 		$this->client = $this->createMock(Horde_Imap_Client_Socket::class);
 		$this->messageMapper = $this->createMock(MessageMapper::class);
 		$this->mailboxMapper = $this->createMock(MailboxMapper::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->migration = new MigrateImportantFromImapAndDb(
-			$this->clientFactory,
 			$this->messageMapper,
 			$this->mailboxMapper,
 			$this->logger
@@ -77,10 +74,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -91,7 +84,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapNoUids() {
@@ -99,10 +92,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$uids = [];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -112,7 +101,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapExceptionGetFlagged() {
@@ -120,10 +109,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox = $this->createMock(Mailbox::class);
 		$e = new Horde_Imap_Client_Exception('');
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -134,7 +119,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->method('debug');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantOnImapExceptionOnFlag() {
@@ -144,10 +129,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$e = new Horde_Imap_Client_Exception('');
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->messageMapper->expects($this->once())
 			->method('getFlagged')
 			->with($this->client, $mailbox, '$important')
@@ -161,7 +142,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->with('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantOnImap($account, $mailbox);
+		$this->migration->migrateImportantOnImap($this->client, $account, $mailbox);
 	}
 
 	public function migrateImportantFromDb() {
@@ -170,10 +151,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox->setId(1);
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -184,7 +161,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantFromDbNoUids() {
@@ -193,10 +170,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$mailbox->setId(1);
 		$uids = [];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -206,7 +179,7 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$this->logger->expects($this->never())
 			->method('debug');
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 
 	public function testMigrateImportantFromDbExceptionOnFlag() {
@@ -217,10 +190,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 		$e = new Horde_Imap_Client_Exception('');
 		$uids = [1,2,3];
 
-		$this->clientFactory->expects($this->once())
-			->method('getClient')
-			->with($account)
-			->willReturn($this->client);
 		$this->mailboxMapper->expects($this->once())
 			->method('findFlaggedImportantUids')
 			->with($mailbox->getId())
@@ -234,6 +203,6 @@ class MigrateImportantFromImapAndDbTest extends TestCase {
 			->with('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
 		$this->expectException(ServiceException::class);
 
-		$this->migration->migrateImportantFromDb($account, $mailbox);
+		$this->migration->migrateImportantFromDb($this->client, $account, $mailbox);
 	}
 }
diff --git a/tests/Unit/Service/MailManagerTest.php b/tests/Unit/Service/MailManagerTest.php
index b9b7f60d2..21cd84c64 100644
--- a/tests/Unit/Service/MailManagerTest.php
+++ b/tests/Unit/Service/MailManagerTest.php
@@ -351,13 +351,9 @@ class MailManagerTest extends TestCase {
 			'mdnsent' => [\Horde_Imap_Client::FLAG_MDNSENT],
 		];
 
-		$this->imapClientFactory->expects($this->any())
-			->method('getClient')
-			->willReturn($client);
-
 		//standard flags
 		foreach ($flags as $k => $flag) {
-			$this->assertEquals($this->manager->filterFlags($account, $k, 'INBOX'), $flags[$k]);
+			$this->assertEquals($this->manager->filterFlags($client, $account, $k, 'INBOX'), $flags[$k]);
 		}
 	}
 
@@ -365,53 +361,40 @@ class MailManagerTest extends TestCase {
 		$account = $this->createMock(Account::class);
 		$client = $this->createMock(Horde_Imap_Client_Socket::class);
 
-		$this->imapClientFactory->expects($this->any())
-		->method('getClient')
-		->willReturn($client);
-
-		$this->assertEquals([], $this->manager->filterFlags($account, Tag::LABEL_IMPORTANT, 'INBOX'));
+		$this->assertEquals([], $this->manager->filterFlags($client, $account, Tag::LABEL_IMPORTANT, 'INBOX'));
 	}
 
 	public function testSetFilterFlagsImportant() {
 		$account = $this->createMock(Account::class);
 		$client = $this->createMock(Horde_Imap_Client_Socket::class);
 
-		$this->imapClientFactory->expects($this->once())
-			->method('getClient')
-			->willReturn($client);
 		$client->expects($this->once())
 			->method('status')
 			->willReturn(['permflags' => [ "11" => "\*" ]]);
 
-		$this->assertEquals([Tag::LABEL_IMPORTANT], $this->manager->filterFlags($account, Tag::LABEL_IMPORTANT, 'INBOX'));
+		$this->assertEquals([Tag::LABEL_IMPORTANT], $this->manager->filterFlags($client, $account, Tag::LABEL_IMPORTANT, 'INBOX'));
 	}
 
 	public function testIsPermflagsEnabledTrue(): void {
 		$account = $this->createMock(Account::class);
 		$client = $this->createMock(Horde_Imap_Client_Socket::class);
 
-		$this->imapClientFactory->expects($this->once())
-			->method('getClient')
-			->willReturn($client);
 		$client->expects($this->once())
 			->method('status')
 			->willReturn(['permflags' => [ "11" => "\*"] ]);
 
-		$this->assertTrue($this->manager->isPermflagsEnabled($account, 'INBOX'));
+		$this->assertTrue($this->manager->isPermflagsEnabled($client, $account, 'INBOX'));
 	}
 
 	public function testIsPermflagsEnabledFalse(): void {
 		$account = $this->createMock(Account::class);
 		$client = $this->createMock(Horde_Imap_Client_Socket::class);
 
-		$this->imapClientFactory->expects($this->once())
-			->method('getClient')
-			->willReturn($client);
 		$client->expects($this->once())
 			->method('status')
 			->willReturn([]);
 
-		$this->assertFalse($this->manager->isPermflagsEnabled($account, 'INBOX'));
+		$this->assertFalse($this->manager->isPermflagsEnabled($client, $account, 'INBOX'));
 	}
 
 	public function testRemoveFlag(): void {