Merge branch 'feature/PB-28918_Release-v442' into 'release'

ishanvyas22 · ishanvyas22 · commit 60cc51db925e · 2023-11-29T04:59:34.000Z
PB-28991: Fix email queue entries not marked as sent

See merge request passbolt/passbolt-ce-api!211
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,10 +2,13 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
-## [4.4.2] - 2023-11-28
+## [4.4.2] - 2023-11-29
 ### Improved
 - PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point
 
+### Fixed
+- PB-28991 As a user emails should be resent if the first attempt failed
+
 ## [4.4.2-test.1] - 2023-11-27
 ### Improved
 - PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -7,6 +7,10 @@ The first fix concerns the Time-based One-Time Password (TOTP) feature. In the p
 The second fix improves the performance of the application, specifically when users are retrieving their resources. This update is part of an ongoing effort to enhance the overall performance of the application, with further improvements planned for future releases.
 
 We extend our gratitude to the community member who reported this issue.
+
 ## [4.4.2] - 2023-11-28
 ### Improved
 - PB-27616 As a user I should see improved performances when retrieving resources on the GET resources.json entry point
+
+### Fixed
+- PB-28991 As a user emails should be resent if the first attempt failed
diff --git a/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php b/plugins/PassboltCe/EmailDigest/src/Utility/Factory/EmailPreviewFactory.php
@@ -90,6 +90,7 @@ public function buildSingleEmailDigest(Entity $emailQueueEntity): EmailDigest
         return (new EmailDigest())
             ->addEmailData($emailQueueEntity)
             ->setSubject($emailQueueEntity->get('subject'))
+            ->setEmailIds([$emailQueueEntity->id])
             ->setEmailRecipient($emailQueueEntity->get('email'));
     }
 
@@ -111,6 +112,8 @@ public function buildMultipleEmailDigest(Digest $digest): EmailDigest
                 ->setEmailRecipient($emailQueueEntity->get('email'));
         }
 
+        $emailDigest->setEmailIds($digest->getEmailQueueIds());
+
         return $emailDigest;
     }
 
diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/EmailQueueFactory.php
@@ -146,11 +146,12 @@ public function setFullBaseUrl(string $fullBaseUrl)
     }
 
     /**
-     * @param string $subject locale of the email
-     * @return EmailQueueFactory
+     * @param ?string $subject locale of the email
+     * @return $this
      */
-    public function setSubject(string $subject)
+    public function setSubject(?string $subject = null)
     {
+        $subject = $subject ?? $this->getFaker()->sentence(3);
         $this->setField('template_vars.body.subject', $subject);
 
         return $this->setField('subject', $subject);
@@ -164,4 +165,15 @@ public function setLocale(string $locale)
     {
         return $this->setField('template_vars.locale', $locale);
     }
+
+    /**
+     * Sets the id of the email. Useful in unit testing, when an ID should be defined, but no persistence needed
+     *
+     * @param int|null $id email Id
+     * @return $this
+     */
+    public function setId(?int $id = null)
+    {
+        return $this->setField('id', $id ?? $this->getFaker()->randomNumber(4));
+    }
 }
diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/GroupUserDeleteEmailQueueFactory.php
@@ -23,9 +23,11 @@ class GroupUserDeleteEmailQueueFactory extends EmailQueueFactory
     protected function setDefaultTemplate(): void
     {
         parent::setDefaultTemplate();
-        $this->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE);
-        $this->setGroup();
-        $this->setOperator();
+        $this
+            ->setSubject()
+            ->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE)
+            ->setGroup()
+            ->setOperator();
     }
 
     /**
diff --git a/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php b/plugins/PassboltCe/EmailDigest/tests/Factory/ResourceDeleteEmailQueueFactory.php
@@ -28,6 +28,7 @@ protected function setDefaultTemplate(): void
     {
         parent::setDefaultTemplate();
         $this
+            ->setSubject()
             ->setTemplate(ResourceDeleteEmailRedactor::TEMPLATE)
             ->setResource()
             ->setOperator()
diff --git a/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php b/plugins/PassboltCe/EmailDigest/tests/TestCase/Command/SenderCommandTest.php
@@ -24,10 +24,12 @@
 use App\Test\Factory\UserFactory;
 use App\Test\Lib\AppIntegrationTestCase;
 use App\Test\Lib\Utility\EmailTestTrait;
+use App\Utility\UuidFactory;
 use Cake\Chronos\Chronos;
 use Cake\Console\TestSuite\ConsoleIntegrationTestTrait;
 use Cake\I18n\I18n;
 use Cake\Mailer\Mailer;
+use Cake\Network\Exception\SocketException;
 use Passbolt\EmailDigest\Test\Factory\EmailQueueFactory;
 use Passbolt\EmailDigest\Test\Lib\EmailDigestMockTestTrait;
 use Passbolt\EmailDigest\Utility\Digest\DigestTemplateRegistry;
@@ -124,17 +126,86 @@ public function testSenderCommandLocale()
         $this->assertMailBodyStringCount(1, '</head>');
     }
 
-    public function testSenderCommand_NoRowsAreLockedWhenThresholdIsExceeded()
+    public function noRowsLockedProvider(): array
     {
-        $nResourcesAdded = 15;
-        $operator = UserFactory::make()->withAvatar()->persist();
+        return [
+            ['single_email' => 1],
+            ['multiple_emails_in_digest' => 2],
+            ['threshold_exceeded' => 11],
+        ];
+    }
+
+    /**
+     * @dataProvider noRowsLockedProvider
+     */
+    public function testSenderCommand_EmailQueueRowsAreUnlockedAfterSuccessfulSend(int $nResourcesAdded)
+    {
+        $operator = UserFactory::make()->withAvatar()->getEntity();
+        $recipient = 'john@test.test';
+        EmailQueueFactory::make($nResourcesAdded)
+            ->setRecipient($recipient)
+            ->setTemplate(ResourceCreateEmailRedactor::TEMPLATE)
+            ->setField('template_vars.body.user', $operator)
+            ->setField('template_vars.body.resource', [
+                // Dummy data to render email without any warnings
+                'id' => UuidFactory::uuid(),
+                'created' => 'now',
+                'name' => 'My pass',
+                'secrets' => [['data' => 'foo bar baz']],
+            ])
+            ->setField('template_vars.body.showUsername', false)
+            ->setField('template_vars.body.showUri', false)
+            ->setField('template_vars.body.showDescription', false)
+            ->setField('template_vars.body.showSecret', false)
+            ->persist();
+
+        $this->exec('passbolt email_digest send');
+
+        $this->assertExitSuccess();
+        $this->assertMailCount(1);
+        $this->assertMailSentToAt(0, [$recipient => $recipient]);
+        // Make sure email queue entries are not locked
+        $count = EmailQueueFactory::find()->where(['locked' => true])->count();
+        $this->assertEquals(0, $count);
+        // Make sure email queue entries are sent
+        $count = EmailQueueFactory::find()->where(['sent' => true, 'locked' => false])->count();
+        $this->assertEquals($nResourcesAdded, $count);
+    }
+
+    /**
+     * @dataProvider noRowsLockedProvider
+     */
+    public function testSenderCommand_EmailQueueRowsAreUnlockedAfterFailedSend(int $nResourcesAdded)
+    {
+        $operator = UserFactory::make()->withAvatar()->getEntity();
         $recipient = 'john@test.test';
         EmailQueueFactory::make($nResourcesAdded)
             ->setRecipient($recipient)
             ->setTemplate(ResourceCreateEmailRedactor::TEMPLATE)
             ->setField('template_vars.body.user', $operator)
+            ->setField('template_vars.body.resource', [
+                // Dummy data to render email without any warnings
+                'id' => UuidFactory::uuid(),
+                'created' => 'now',
+                'name' => 'My pass',
+                'secrets' => [['data' => 'foo bar baz']],
+            ])
+            ->setField('template_vars.body.showUsername', false)
+            ->setField('template_vars.body.showUri', false)
+            ->setField('template_vars.body.showDescription', false)
+            ->setField('template_vars.body.showSecret', false)
             ->persist();
 
+        $mockedEmailQueue = $this->getMockForModel(
+            'EmailQueue.EmailQueue',
+            ['success',],
+            ['table' => 'email_queue',]
+        );
+        $mockedEmailQueue
+            ->expects($this->once())
+            ->method('success')
+            ->willThrowException(new SocketException());
+
         $this->exec('passbolt email_digest send');
 
         $this->assertExitSuccess();
@@ -143,6 +214,9 @@ public function testSenderCommand_NoRowsAreLockedWhenThresholdIsExceeded()
         // Make sure email queue entries are not locked
         $count = EmailQueueFactory::find()->where(['locked' => true])->count();
         $this->assertEquals(0, $count);
+        // Make sure email queue entries are not sent
+        $count = EmailQueueFactory::find()->where(['sent' => false, 'locked' => false])->count();
+        $this->assertEquals($nResourcesAdded, $count);
     }
 
     public function testSenderCommandMultipleDigests()
@@ -174,6 +248,9 @@ public function testSenderCommandMultipleDigests()
         $this->exec('passbolt email_digest send');
         $this->assertExitSuccess();
 
+        $sentCount = EmailQueueFactory::find()->where(['sent' => true])->count();
+        $this->assertSame($nEmailsSent * 2, $sentCount);
+
         $this->assertMailCount(2);
         $subject = $user->profile->full_name . ' has made changes on several resources';
         $this->assertMailSubjectContainsAt(0, $subject);
diff --git a/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php b/plugins/PassboltCe/EmailDigest/tests/TestCase/Service/SendEmailBatchServiceUnitTest.php
@@ -34,6 +34,9 @@
 use Passbolt\EmailDigest\Utility\Digest\DigestTemplateRegistry;
 use Passbolt\Locale\LocalePlugin;
 
+/**
+ * @covers \Passbolt\EmailDigest\Service\SendEmailBatchService
+ */
 class SendEmailBatchServiceUnitTest extends TestCase
 {
     use EmailTestTrait;
@@ -64,13 +67,13 @@ public function tearDown(): void
         parent::tearDown();
     }
 
-    public function testSendEmailBatchService_On_No_Email()
+    public function testSendEmailBatchServiceUnitTest_On_No_Email()
     {
         $this->service->sendNextEmailsBatch([]);
         $this->assertMailCount(0);
     }
 
-    public function testSendEmailBatchService_On_Email_With_Unknown_Template()
+    public function testSendEmailBatchServiceUnitTest_On_Email_With_Unknown_Template()
     {
         $email = EmailQueueFactory::make()->setTemplate('foo')->getEntity();
         $this->expectException(MissingTemplateException::class);
@@ -90,7 +93,7 @@ public function withAndWithoutDigestTemplate(): array
     /**
      * @dataProvider withAndWithoutDigestTemplate
      */
-    public function testSendEmailBatchService_On_One_Email_Translated(bool $withDigestTemplate)
+    public function testSendEmailBatchServiceUnitTest_On_One_Email_Translated(bool $withDigestTemplate)
     {
         if (!$withDigestTemplate) {
             DigestTemplateRegistry::clearInstance();
@@ -99,6 +102,7 @@ public function testSendEmailBatchService_On_One_Email_Translated(bool $withDige
         $resourceDeleted = ResourceFactory::make()->getEntity();
         $subjectTranslated = 'Le sujet';
         $email = ResourceDeleteEmailQueueFactory::make()
+            ->setId()
             ->setOperator($operator)
             ->setResource($resourceDeleted)
             ->setSubject($subjectTranslated)
@@ -117,7 +121,7 @@ public function testSendEmailBatchService_On_One_Email_Translated(bool $withDige
     /**
      * @dataProvider withAndWithoutDigestTemplate
      */
-    public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Below_Threshold(
+    public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Same_Recipient_Below_Threshold(
         bool $withDigestTemplate
     ) {
         if (!$withDigestTemplate) {
@@ -130,6 +134,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Belo
         $recipient = 'foo@passbolt.com';
         $nEmails = rand(2, 10);
         $emails = ResourceDeleteEmailQueueFactory::make($nEmails)
+            ->setId()
             ->setRecipient($recipient)
             ->setOperator($operator)
             ->setResource($resourceDeleted)
@@ -159,7 +164,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Same_Recipient_Belo
     /**
      * @dataProvider withAndWithoutDigestTemplate
      */
-    public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipients(
+    public function testSendEmailBatchServiceUnitTest_On_Same_Digest_Template_Various_Recipients(
         bool $withDigestTemplate
     ) {
         if (!$withDigestTemplate) {
@@ -173,12 +178,14 @@ public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipi
         $nEmails1 = rand(2, 10);
         $nEmails2 = rand(2, 10);
         $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1)
+            ->setId()
             ->setRecipient($recipient1)
             ->setOperator($operator)
             ->setResource($resourceDeleted1)
             ->setSubject($subject)
             ->getEntities();
         $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2)
+            ->setId()
             ->setRecipient($recipient2)
             ->setOperator($operator)
             ->setResource($resourceDeleted2)
@@ -222,7 +229,7 @@ public function testSendEmailBatchService_On_Same_Digest_Template_Various_Recipi
     /**
      * @dataProvider withAndWithoutDigestTemplate
      */
-    public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Below_Threshold(
+    public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Below_Threshold(
         bool $withDigestTemplate
     ) {
         if (!$withDigestTemplate) {
@@ -236,13 +243,15 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_
         $nEmails1 = rand(2, 10);
         $nEmails2 = rand(2, 10);
         $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1)
+            ->setId()
             ->setRecipient($recipient)
             ->setOperator($operator1)
             ->setResource($resourceDeleted1)
             ->setSubject($subjectTranslated)
             ->setLocale('fr-FR')
             ->getEntities();
         $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2)
+            ->setId()
             ->setRecipient($recipient)
             ->setOperator($operator2)
             ->setResource($resourceDeleted2)
@@ -287,7 +296,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_
     /**
      * @dataProvider withAndWithoutDigestTemplate
      */
-    public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Below_And_Above_Threshold(
+    public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Below_And_Above_Threshold(
         bool $withDigestTemplate
     ) {
         if (!$withDigestTemplate) {
@@ -297,17 +306,19 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_
         [$resourceDeleted1, $resourceDeleted2] = ResourceFactory::make(2)->getEntities();
         $subjectTranslated = 'Le sujet';
         $recipient = 'foo@passbolt.com';
-        $nEmails1 = 10 + rand(2, 10);
-        $nEmails2 = rand(2, 10);
+        $nEmails1 = 12;
+        $nEmails2 = 2;
         // These emails are above the threshold
         $emails1 = ResourceDeleteEmailQueueFactory::make($nEmails1)
+            ->setId()
             ->setRecipient($recipient)
             ->setOperator($operator1)
             ->setResource($resourceDeleted1)
             ->setSubject($subjectTranslated)
             ->setLocale('fr-FR')
             ->getEntities();
         $emails2 = ResourceDeleteEmailQueueFactory::make($nEmails2)
+            ->setId()
             ->setRecipient($recipient)
             ->setOperator($operator2)
             ->setResource($resourceDeleted2)
@@ -350,7 +361,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_
         }
     }
 
-    public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_Multiple_Digest_Templates_Below_And_Above_Threshold()
+    public function testSendEmailBatchServiceUnitTest_On_Multiple_Emails_Multiple_Operators_Multiple_Digest_Templates_Below_And_Above_Threshold()
     {
         DigestTemplateRegistry::clearInstance();
         // Emails of the Group User delete template should be sent first
@@ -416,7 +427,7 @@ public function testSendEmailBatchService_On_Multiple_Emails_Multiple_Operators_
         $this->assertMailSubjectContainsAt(3, $operator2->profile->full_name . ' has made changes on several resources');
     }
 
-    public function testSendEmailBatchService_On_Multiple_Full_Base_Url()
+    public function testSendEmailBatchServiceUnitTest_On_Multiple_Full_Base_Url()
     {
         $recipient = 'test@passbolt.com';
         $subject = 'Some subject';

Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,7 @@ public function buildSingleEmailDigest(Entity $emailQueueEntity): EmailDigest`
`90`	`90`	`return (new EmailDigest())`
`91`	`91`	`->addEmailData($emailQueueEntity)`
`92`	`92`	`->setSubject($emailQueueEntity->get('subject'))`
	`93`	`+ ->setEmailIds([$emailQueueEntity->id])`
`93`	`94`	`->setEmailRecipient($emailQueueEntity->get('email'));`
`94`	`95`	`}`
`95`	`96`
`@@ -111,6 +112,8 @@ public function buildMultipleEmailDigest(Digest $digest): EmailDigest`
`111`	`112`	`->setEmailRecipient($emailQueueEntity->get('email'));`
`112`	`113`	`}`
`113`	`114`
	`115`	`+ $emailDigest->setEmailIds($digest->getEmailQueueIds());`
	`116`	`+`
`114`	`117`	`return $emailDigest;`
`115`	`118`	`}`
`116`	`119`
Original file line number	Diff line number	Diff line change
`@@ -146,11 +146,12 @@ public function setFullBaseUrl(string $fullBaseUrl)`
`146`	`146`	`}`
`147`	`147`
`148`	`148`	`/**`
`149`		`- * @param string $subject locale of the email`
`150`		`- * @return EmailQueueFactory`
	`149`	`+ * @param ?string $subject locale of the email`
	`150`	`+ * @return $this`
`151`	`151`	`*/`
`152`		`- public function setSubject(string $subject)`
	`152`	`+ public function setSubject(?string $subject = null)`
`153`	`153`	`{`
	`154`	`+ $subject = $subject ?? $this->getFaker()->sentence(3);`
`154`	`155`	`$this->setField('template_vars.body.subject', $subject);`
`155`	`156`
`156`	`157`	`return $this->setField('subject', $subject);`
`@@ -164,4 +165,15 @@ public function setLocale(string $locale)`
`164`	`165`	`{`
`165`	`166`	`return $this->setField('template_vars.locale', $locale);`
`166`	`167`	`}`
	`168`	`+`
	`169`	`+ /**`
	`170`	`+ * Sets the id of the email. Useful in unit testing, when an ID should be defined, but no persistence needed`
	`171`	`+ *`
	`172`	`+ * @param int\|null $id email Id`
	`173`	`+ * @return $this`
	`174`	`+ */`
	`175`	`+ public function setId(?int $id = null)`
	`176`	`+ {`
	`177`	`+ return $this->setField('id', $id ?? $this->getFaker()->randomNumber(4));`
	`178`	`+ }`
`167`	`179`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,9 +23,11 @@ class GroupUserDeleteEmailQueueFactory extends EmailQueueFactory`
`23`	`23`	`protected function setDefaultTemplate(): void`
`24`	`24`	`{`
`25`	`25`	`parent::setDefaultTemplate();`
`26`		`- $this->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE);`
`27`		`- $this->setGroup();`
`28`		`- $this->setOperator();`
	`26`	`+ $this`
	`27`	`+ ->setSubject()`
	`28`	`+ ->setTemplate(GroupUserDeleteEmailRedactor::TEMPLATE)`
	`29`	`+ ->setGroup()`
	`30`	`+ ->setOperator();`
`29`	`31`	`}`
`30`	`32`
`31`	`33`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ protected function setDefaultTemplate(): void`
`28`	`28`	`{`
`29`	`29`	`parent::setDefaultTemplate();`
`30`	`30`	`$this`
	`31`	`+ ->setSubject()`
`31`	`32`	`->setTemplate(ResourceDeleteEmailRedactor::TEMPLATE)`
`32`	`33`	`->setResource()`
`33`	`34`	`->setOperator()`