Fix sending messages to groupsfix/sending-messages-to-groups

Groups were expanded in the accounts controller. Since moving over to
the outbox logic this feature was missing and internal group identifiers
were passed to SMTP.

With this patch groups are expanded again just before a message is sent.
This means the group memberships are read as late as possible and
editing an outbox message looks like the original message because
members have not been expanded there yet.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

3 files changed, 101 insertions, 93 deletions

diff --git a/tests/Integration/Service/MailTransmissionIntegrationTest.php b/tests/Integration/Service/MailTransmissionIntegrationTest.php
index 6d14c79a6..e726ae388 100644
--- a/tests/Integration/Service/MailTransmissionIntegrationTest.php
+++ b/tests/Integration/Service/MailTransmissionIntegrationTest.php
@@ -40,9 +40,9 @@ use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Model\NewMessageData;
 use OCA\Mail\Model\RepliedMessageData;
-use OCA\Mail\Service\AccountService;
 use OCA\Mail\Service\AliasesService;
 use OCA\Mail\Service\Attachment\UploadedFile;
+use OCA\Mail\Service\GroupsIntegration;
 use OCA\Mail\Service\MailTransmission;
 use OCA\Mail\SMTP\SmtpClientFactory;
 use OCA\Mail\Support\PerformanceLogger;
@@ -109,7 +109,6 @@ class MailTransmissionIntegrationTest extends TestCase {
 
 		$this->transmission = new MailTransmission(
 			$userFolder,
-			OC::$server->query(AccountService::class),
 			$this->attachmentService,
 			OC::$server->query(IMailManager::class),
 			OC::$server->query(IMAPClientFactory::class),
@@ -119,7 +118,8 @@ class MailTransmissionIntegrationTest extends TestCase {
 			OC::$server->query(MessageMapper::class),
 			OC::$server->query(LoggerInterface::class),
 			OC::$server->query(PerformanceLogger::class),
-			OC::$server->get(AliasesService::class)
+			OC::$server->get(AliasesService::class),
+			OC::$server->get(GroupsIntegration::class)
 		);
 	}
 
diff --git a/tests/Unit/Service/GroupsIntegrationTest.php b/tests/Unit/Service/GroupsIntegrationTest.php
index 49e1fa02d..98454cb49 100644
--- a/tests/Unit/Service/GroupsIntegrationTest.php
+++ b/tests/Unit/Service/GroupsIntegrationTest.php
@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * @author Matthias Rella <mrella@pisys.eu>
  *
@@ -22,6 +24,7 @@
 namespace OCA\Mail\Tests\Unit\Service;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
+use OCA\Mail\Db\Recipient;
 use OCA\Mail\Service\Group\ContactsGroupService;
 use OCA\Mail\Service\GroupsIntegration;
 use OCA\Mail\Service\Group\NextcloudGroupService;
@@ -53,7 +56,7 @@ class GroupsIntegrationTest extends TestCase {
 		);
 	}
 
-	public function testGetMatchingGroups() {
+	public function testGetMatchingGroups(): void {
 		$term = 'te'; // searching for: John Doe
 		$searchResult1 = [
 			[
@@ -61,52 +64,45 @@ class GroupsIntegrationTest extends TestCase {
 				'name' => "first test group"
 			]
 		];
-
 		$this->groupService1->expects($this->once())
 			->method('search')
 			->with($term)
-			->will($this->returnValue($searchResult1));
+			->willReturn($searchResult1);
 
-		$expected = [
-			[
-				'id' => 'namespace1:testgroup',
-				'label' => 'first test group (Namespace1)',
-				'email' => 'namespace1:testgroup',
-				'photo' => null,
-			]
-		];
 		$actual = $this->groupsIntegration->getMatchingGroups($term);
 
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testExpandNone() {
-		$recipients = "john@doe.com,alice@smith.net";
-		$members = [
+		$this->assertEquals(
 			[
-				'id' => 'bob',
-				'name' => "Bobby",
-				'email' => "bob@smith.net"
+				[
+					'id' => 'namespace1:testgroup',
+					'label' => 'first test group (Namespace1)',
+					'email' => 'namespace1:testgroup',
+					'photo' => null,
+				]
 			],
-			[
-				'id' => 'mary',
-				'name' => 'Mary',
-				'email' => 'mary@smith.net'
-			]
+			$actual
+		);
+	}
+
+	public function testExpandNone(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'alice@smith.net', 'email' => 'alice@smith.net']),
 		];
 		$this->groupService1->expects($this->never())
-			->method('getUsers')
-			->willReturn($members);
+			->method('getUsers');
 
-		$expected = $recipients;
+		$expanded = $this->groupsIntegration->expand($recipients);
 
-		$actual = $this->groupsIntegration->expand($recipients);
-
-		$this->assertEquals($expected, $actual);
+		$this->assertEquals($recipients, $expanded);
 	}
 
-	public function testExpand() {
-		$recipients = "john@doe.com,namespace1:testgroup,alice@smith.net";
+	public function testExpand(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'testgroup (namespace1)', 'email' => 'namespace1:testgroup']),
+			Recipient::fromParams(['label' => 'alice@smith.net', 'email' => 'alice@smith.net']),
+		];
 		$members = [
 			[
 				'id' => 'bob',
@@ -123,40 +119,25 @@ class GroupsIntegrationTest extends TestCase {
 			->method('getUsers')
 			->willReturn($members);
 
-		$expected = "john@doe.com,bob@smith.net,mary@smith.net,alice@smith.net";
-
-		$actual = $this->groupsIntegration->expand($recipients);
+		$expanded = $this->groupsIntegration->expand($recipients);
 
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testExpand2() {
-		$recipients = "john@doe.com,namespace1:testgroup,alice@smith.net";
-		$members = [
+		$this->assertEquals(
 			[
-				'id' => 'bob',
-				'name' => "Bobby",
-				'email' => "bob@smith.net"
+				Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+				Recipient::fromParams(['label' => 'Bobby', 'email' => 'bob@smith.net']),
+				Recipient::fromParams(['label' => 'Mary', 'email' => 'mary@smith.net']),
+				Recipient::fromParams(['label' => 'alice@smith.net', 'email' => 'alice@smith.net']),
 			],
-			[
-				'id' => 'mary',
-				'name' => 'Mary',
-				'email' => 'mary@smith.net'
-			]
-		];
-		$this->groupService1->expects($this->once())
-			->method('getUsers')
-			->willReturn($members);
-
-		$expected = "john@doe.com,bob@smith.net,mary@smith.net,alice@smith.net";
-
-		$actual = $this->groupsIntegration->expand($recipients);
-
-		$this->assertEquals($expected, $actual);
+			$expanded
+		);
 	}
 
-	public function testExpandUmlauts() {
-		$recipients = "john@doe.com,namespace1:ümlaut";
+	public function testExpandUmlauts(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'ümlaut (namespace1)', 'email' => 'namespace1:ümlaut']),
+			Recipient::fromParams(['label' => 'alice@smith.net', 'email' => 'alice@smith.net']),
+		];
 		$members = [
 			[
 				'id' => 'bob',
@@ -173,15 +154,24 @@ class GroupsIntegrationTest extends TestCase {
 			->method('getUsers')
 			->willReturn($members);
 
-		$expected = "john@doe.com,bob@smith.net,mary@smith.net";
+		$expanded = $this->groupsIntegration->expand($recipients);
 
-		$actual = $this->groupsIntegration->expand($recipients);
-
-		$this->assertEquals($expected, $actual);
+		$this->assertEquals(
+			[
+				Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+				Recipient::fromParams(['label' => 'Bobby', 'email' => 'bob@smith.net']),
+				Recipient::fromParams(['label' => 'Mary', 'email' => 'mary@smith.net']),
+				Recipient::fromParams(['label' => 'alice@smith.net', 'email' => 'alice@smith.net']),
+			],
+			$expanded
+		);
 	}
 
-	public function testExpandSpace() {
-		$recipients = "john@doe.com,namespace1:test group";
+	public function testExpandSpace(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'test group (namespace1)', 'email' => 'namespace1:test group']),
+		];
 		$members = [
 			[
 				'id' => 'bob',
@@ -199,36 +189,54 @@ class GroupsIntegrationTest extends TestCase {
 			->with('test group')
 			->willReturn($members);
 
-		$expected = "john@doe.com,bob@smith.net,mary@smith.net";
-
-		$actual = $this->groupsIntegration->expand($recipients);
+		$expanded = $this->groupsIntegration->expand($recipients);
 
-		$this->assertEquals($expected, $actual);
+		$this->assertEquals(
+			[
+				Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+				Recipient::fromParams(['label' => 'Bobby', 'email' => 'bob@smith.net']),
+				Recipient::fromParams(['label' => 'Mary', 'email' => 'mary@smith.net']),
+			],
+			$expanded
+		);
 	}
 
-	public function testExpandEmpty() {
-		$this->expectException(ServiceException::class);
-		$recipients = "john@doe.com,namespace1:testgroup,alice@smith.net";
-		$members = [
+	public function testExpandEmpty(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'testgroup (namespace1)', 'email' => 'namespace1:testgroup']),
 		];
+		$members = [];
 		$this->groupService1->expects($this->once())
 			->method('getUsers')
 			->willReturn($members);
+		$this->expectException(ServiceException::class);
+
 		$this->groupsIntegration->expand($recipients);
 	}
 
-	public function testExpandWrong() {
-		$recipients = "john@doe.com,nons:testgroup,alice@smith.net";
-		$expected = "john@doe.com,nons:testgroup,alice@smith.net";
+	public function testExpandWrong(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'testgroup (nons)', 'email' => 'nons:testgroup']),
+		];
 
 		$actual = $this->groupsIntegration->expand($recipients);
 
-		$this->assertEquals($expected, $actual);
+		$this->assertEquals(
+			[
+				Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+				Recipient::fromParams(['label' => 'testgroup (nons)', 'email' => 'nons:testgroup']),
+			],
+			$actual
+		);
 	}
 
-	public function testExpandWrong2() {
+	public function testExpandWrong2(): void {
+		$recipients = [
+			Recipient::fromParams(['label' => 'John Doe', 'email' => 'john@doe.com']),
+			Recipient::fromParams(['label' => 'nogroup (namespace1)', 'email' => 'namespace1:nogroup']),
+		];
 		$this->expectException(ServiceException::class);
-		$recipients = "john@doe.com,namespace1:nogroup,alice@smith.net";
 
 		$this->groupsIntegration->expand($recipients);
 	}
diff --git a/tests/Unit/Service/MailTransmissionTest.php b/tests/Unit/Service/MailTransmissionTest.php
index 70a1609fd..324dcc415 100644
--- a/tests/Unit/Service/MailTransmissionTest.php
+++ b/tests/Unit/Service/MailTransmissionTest.php
@@ -41,8 +41,8 @@ use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Model\Message;
 use OCA\Mail\Model\NewMessageData;
-use OCA\Mail\Service\AccountService;
 use OCA\Mail\Service\AliasesService;
+use OCA\Mail\Service\GroupsIntegration;
 use OCA\Mail\Service\MailTransmission;
 use OCA\Mail\SMTP\SmtpClientFactory;
 use OCA\Mail\Support\PerformanceLogger;
@@ -56,9 +56,6 @@ class MailTransmissionTest extends TestCase {
 	/** @var Folder|MockObject */
 	private $userFolder;
 
-	/** @var AccountService|MockObject */
-	private $accountService;
-
 	/** @var IAttachmentService|MockObject */
 	private $attachmentService;
 
@@ -89,11 +86,13 @@ class MailTransmissionTest extends TestCase {
 	/** @var AliasesService|MockObject */
 	private $aliasService;
 
+	/** @var GroupsIntegration|MockObject */
+	private $groupsIntegration;
+
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->userFolder = $this->createMock(Folder::class);
-		$this->accountService = $this->createMock(AccountService::class);
 		$this->attachmentService = $this->createMock(IAttachmentService::class);
 		$this->mailManager = $this->createMock(IMailManager::class);
 		$this->imapClientFactory = $this->createMock(IMAPClientFactory::class);
@@ -104,10 +103,10 @@ class MailTransmissionTest extends TestCase {
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->performanceLogger = $this->createMock(PerformanceLogger::class);
 		$this->aliasService = $this->createMock(AliasesService::class);
+		$this->groupsIntegration = $this->createMock(GroupsIntegration::class);
 
 		$this->transmission = new MailTransmission(
 			$this->userFolder,
-			$this->accountService,
 			$this->attachmentService,
 			$this->mailManager,
 			$this->imapClientFactory,
@@ -117,7 +116,8 @@ class MailTransmissionTest extends TestCase {
 			$this->messageMapper,
 			$this->logger,
 			$this->performanceLogger,
-			$this->aliasService
+			$this->aliasService,
+			$this->groupsIntegration,
 		);
 	}
 
@@ -416,7 +416,7 @@ class MailTransmissionTest extends TestCase {
 		$this->assertEquals(13, $newId);
 	}
 
-	public function testSendLocalMessage() {
+	public function testSendLocalMessage(): void {
 		$mailAccount = new MailAccount();
 		$mailAccount->setId(10);
 		$mailAccount->setUserId('testuser');