Comment mail throws error on no recipients (#241)

Improve Array methods and add Equatable + Comparable interface
123inkt · Apr 19, 2023 · ac256c5 · ac256c5
1 parent 11e96a0
commit ac256c5
Show file tree

Hide file tree

Showing 11 changed files with 178 additions and 25 deletions.
diff --git a/composer.json b/composer.json
@@ -98,7 +98,7 @@
         "phpstan/phpstan-phpunit": "^1.0",
         "phpstan/phpstan-strict-rules": "^1.1",
         "phpstan/phpstan-symfony": "^1.1",
-        "phpunit/phpunit": "^10.0",
+        "phpunit/phpunit": "10.0.*",
         "roave/security-advisories": "dev-latest",
         "slevomat/coding-standard": "^8.8",
         "squizlabs/php_codesniffer": "^3.6",

diff --git a/composer.lock b/composer.lock
diff --git a/src/Entity/User/User.php b/src/Entity/User/User.php
@@ -18,6 +18,8 @@
 use DR\Review\Entity\Review\CommentReply;
 use DR\Review\Repository\User\UserRepository;
 use DR\Review\Security\Role\Roles;
+use DR\Review\Utility\ComparableInterface;
+use DR\Review\Utility\EquatableInterface;
 use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
 use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -36,7 +38,7 @@
 #[ORM\Entity(repositoryClass: UserRepository::class)]
 #[UniqueEntity(fields: ['email'], message: 'user.email.already.exists')]
 #[ORM\UniqueConstraint('EMAIL', ['email'])]
-class User implements UserInterface, PasswordAuthenticatedUserInterface
+class User implements UserInterface, PasswordAuthenticatedUserInterface, EquatableInterface, ComparableInterface
 {
     #[ORM\Id]
     #[ORM\GeneratedValue]
@@ -269,4 +271,14 @@ public function setReplies(Collection $replies): self
 
         return $this;
     }
+
+    public function equalsTo(mixed $other): bool
+    {
+        return $other instanceof self && $this->id === $other->id;
+    }
+
+    public function compareTo(mixed $other): int
+    {
+        return $other instanceof self === false ? -1 : $this->id <=> $other->id;
+    }
 }
diff --git a/src/Service/Mail/CommentMailService.php b/src/Service/Mail/CommentMailService.php
@@ -42,9 +42,6 @@ public function sendNewCommentMail(CodeReview $review, Comment $comment, ?array
             $recipients = array_merge($recipients, $this->recipientService->getUserForComment($comment));
         }
         $recipients = Arrays::remove(Arrays::unique($recipients), Assert::notNull($comment->getUser()));
-        if (count($recipients) === 0) {
-            return;
-        }
 
         $subject = $this->translator->trans(
             'mail.new.comment.subject',
@@ -67,6 +64,11 @@ public function sendNewCommentMail(CodeReview $review, Comment $comment, ?array
             $email->addBcc(new Address((string)$recipient->getEmail(), (string)$recipient->getName()));
             $this->logger?->info(sprintf('Sending mail to %s for comment %d.', $recipient->getEmail(), $comment->getId()));
         }
+
+        if (count($email->getBcc()) === 0) {
+            return;
+        }
+
         $this->mailer->send($email);
     }
 
@@ -83,9 +85,6 @@ public function sendNewCommentReplyMail(CodeReview $review, Comment $comment, Co
             $recipients = array_merge($recipients, $this->recipientService->getUsersForReply($comment, $reply));
         }
         $recipients = Arrays::remove(Arrays::unique($recipients), Assert::notNull($reply->getUser()));
-        if (count($recipients) === 0) {
-            return;
-        }
 
         $subject = $this->translator->trans(
             'mail.updated.comment.subject',
@@ -108,6 +107,11 @@ public function sendNewCommentReplyMail(CodeReview $review, Comment $comment, Co
             $email->addBcc(new Address((string)$recipient->getEmail(), (string)$recipient->getName()));
             $this->logger?->info(sprintf('Sending mail to %s for comment %d.', $recipient->getEmail(), $reply->getId()));
         }
+
+        if (count($email->getBcc()) === 0) {
+            return;
+        }
+
         $this->mailer->send($email);
     }
 
@@ -120,9 +124,6 @@ public function sendCommentResolvedMail(CodeReview $review, Comment $comment, Us
         $recipients = array_merge($recipients, $this->recipientService->getUserForComment($comment));
         $recipients = array_merge($recipients, $this->recipientService->getUsersForReply($comment));
         $recipients = Arrays::remove(Arrays::unique($recipients), $resolvedBy);
-        if (count($recipients) === 0) {
-            return;
-        }
 
         $subject = $this->translator->trans(
             'mail.comment.resolved.subject',
@@ -152,6 +153,11 @@ public function sendCommentResolvedMail(CodeReview $review, Comment $comment, Us
                 )
             );
         }
+
+        if (count($email->getBcc()) === 0) {
+            return;
+        }
+
         $this->mailer->send($email);
     }
 }
diff --git a/src/Utility/Arrays.php b/src/Utility/Arrays.php
@@ -118,14 +118,32 @@ public static function mapAssoc(array $items, callable $callback): array
      */
     public static function remove(array $items, mixed $item): array
     {
-        $index = array_search($item, $items, true);
+        $index = self::search($items, $item);
         if ($index !== false) {
             unset($items[$index]);
         }
 
         return $items;
     }
 
+    /**
+     * @template T of array-key
+     *
+     * @param array<T, mixed> $items
+     *
+     * @phpstan-return T|false
+     */
+    public static function search(array $items, mixed $needle): int|string|false
+    {
+        foreach ($items as $key => $value) {
+            if ($value === $needle || ($needle instanceof EquatableInterface && $needle->equalsTo($value))) {
+                return $key;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * @template T
      *
@@ -137,7 +155,7 @@ public static function unique(array $items): array
     {
         $result = [];
         foreach ($items as $key => $item) {
-            if (in_array($item, $result, true) === false) {
+            if (self::search($result, $item) === false) {
                 $result[$key] = $item;
             }
         }
@@ -159,7 +177,11 @@ public static function diff(array $itemsA, array $itemsB): array
         return array_udiff(
             $itemsA,
             $itemsB,
-            static function ($itemA, $itemB) {
+            static function ($itemA, $itemB): int {
+                if ($itemA instanceof ComparableInterface) {
+                    return $itemA->compareTo($itemB);
+                }
+
                 $keyA = is_object($itemA) ? spl_object_hash($itemA) : (string)$itemA;
                 $keyB = is_object($itemB) ? spl_object_hash($itemB) : (string)$itemB;
 

diff --git a/src/Utility/ComparableInterface.php b/src/Utility/ComparableInterface.php
@@ -0,0 +1,13 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DR\Review\Utility;
+
+interface ComparableInterface
+{
+    /**
+     * @phpstan-return -1|0|1
+     */
+    public function compareTo(mixed $other): int;
+}
diff --git a/src/Utility/EquatableInterface.php b/src/Utility/EquatableInterface.php
@@ -0,0 +1,9 @@
+<?php
+declare(strict_types=1);
+
+namespace DR\Review\Utility;
+
+interface EquatableInterface
+{
+    public function equalsTo(mixed $other): bool;
+}
diff --git a/tests/Unit/Entity/User/UserTest.php b/tests/Unit/Entity/User/UserTest.php
@@ -133,4 +133,41 @@ public function testReplies(): void
         $user->setReplies($collection);
         static::assertSame($collection, $user->getReplies());
     }
+
+    /**
+     * @covers ::equalsTo
+     */
+    public function testEqualsTo(): void
+    {
+        $userA = new User();
+        $userA->setId(123);
+        $userB = new User();
+        $userB->setId(456);
+        $userC = new User();
+        $userC->setId(123);
+
+        static::assertTrue($userA->equalsTo($userA));
+        static::assertFalse($userA->equalsTo($userB));
+        static::assertTrue($userA->equalsTo($userC));
+        static::assertFalse($userA->equalsTo("foobar"));
+    }
+
+    /**
+     * @covers ::compareTo
+     */
+    public function testCompareTo(): void
+    {
+        $userA = new User();
+        $userA->setId(123);
+        $userB = new User();
+        $userB->setId(456);
+        $userC = new User();
+        $userC->setId(123);
+
+        static::assertSame(0, $userA->compareTo($userA));
+        static::assertSame(-1, $userA->compareTo($userB));
+        static::assertSame(1, $userB->compareTo($userA));
+        static::assertSame(0, $userA->compareTo($userC));
+        static::assertSame(-1, $userA->compareTo("foobar"));
+    }
 }
diff --git a/tests/Unit/Service/Mail/CommentMailServiceTest.php b/tests/Unit/Service/Mail/CommentMailServiceTest.php
@@ -57,7 +57,8 @@ public function testSendNewCommentMailShouldNotMailWithoutRecipients(): void
 
         $this->recipientService->expects(self::once())->method('getUsersForReview')->with($review)->willReturn([$user]);
         $this->recipientService->expects(self::once())->method('getUserForComment')->with($comment)->willReturn([$user]);
-        $this->translator->expects(self::never())->method('trans');
+        $this->translator->expects(self::once())->method('trans');
+        $this->mailer->expects(self::never())->method('send');
 
         $this->service->sendNewCommentMail($review, $comment);
     }
@@ -69,12 +70,15 @@ public function testSendNewCommentMailShouldNotMailWithoutRecipients(): void
     public function testSendNewCommentMailShouldSendMail(): void
     {
         $userA = new User();
+        $userA->setId(5);
         $userA->setEmail('sherlock@example.com');
         $userA->getSetting()->setMailCommentAdded(true);
         $userB = new User();
+        $userB->setId(6);
         $userB->setEmail('watson@example.com');
         $userB->getSetting()->setMailCommentAdded(true);
         $userC = new User();
+        $userC->setId(7);
         $userC->setEmail('enola@example.com');
         $userC->getSetting()->setMailCommentAdded(false);
         $comment = new Comment();
@@ -108,7 +112,8 @@ static function (TemplatedEmail $email) {
      */
     public function testSendNewCommentReplyMailNoMailForEmptyRecipients(): void
     {
-        $user    = new User();
+        $user = new User();
+        $user->setId(5);
         $comment = new Comment();
         $comment->setUser($user);
         $reply = new CommentReply();
@@ -118,7 +123,8 @@ public function testSendNewCommentReplyMailNoMailForEmptyRecipients(): void
         $this->recipientService->expects(self::once())->method('getUsersForReview')->with($review)->willReturn([$user]);
         $this->recipientService->expects(self::once())->method('getUserForComment')->with($comment)->willReturn([$user]);
         $this->recipientService->expects(self::once())->method('getUsersForReply')->with($comment, $reply)->willReturn([$user]);
-        $this->translator->expects(self::never())->method('trans');
+        $this->translator->expects(self::once())->method('trans');
+        $this->mailer->expects(self::never())->method('send');
 
         $this->service->sendNewCommentReplyMail($review, $comment, $reply);
     }
@@ -130,12 +136,15 @@ public function testSendNewCommentReplyMailNoMailForEmptyRecipients(): void
     public function testSendNewCommentReplyMail(): void
     {
         $userA = new User();
+        $userA->setId(5);
         $userA->setEmail('sherlock@example.com');
         $userA->getSetting()->setMailCommentReplied(true);
         $userB = new User();
+        $userB->setId(6);
         $userB->setEmail('watson@example.com');
         $userB->getSetting()->setMailCommentReplied(true);
         $userC = new User();
+        $userC->setId(7);
         $userC->setEmail('enola@example.com');
         $userC->getSetting()->setMailCommentReplied(false);
         $comment = new Comment();
@@ -172,16 +181,17 @@ static function (TemplatedEmail $email) {
      */
     public function testSendCommentResolvedMailNoRecipientsNoMail(): void
     {
-        $user    = new User();
+        $user    = (new User())->setId(5);
         $comment = new Comment();
         $review  = new CodeReview();
 
         $this->recipientService->expects(self::once())->method('getUsersForReview')->with($review)->willReturn([$user]);
         $this->recipientService->expects(self::once())->method('getUserForComment')->with($comment)->willReturn([$user]);
         $this->recipientService->expects(self::once())->method('getUsersForReply')->with($comment)->willReturn([$user]);
-        $this->translator->expects(self::never())->method('trans');
+        $this->translator->expects(self::once())->method('trans');
+        $this->mailer->expects(self::never())->method('send');
 
-        $this->service->sendCommentResolvedMail($review, $comment, $user);
+        $this->service->sendCommentResolvedMail($review, $comment, (new User())->setId(5));
     }
 
     /**
@@ -191,12 +201,15 @@ public function testSendCommentResolvedMailNoRecipientsNoMail(): void
     public function testSendCommentResolvedMail(): void
     {
         $userA = new User();
+        $userA->setId(5);
         $userA->setEmail('sherlock@example.com');
         $userA->getSetting()->setMailCommentResolved(true);
         $userB = new User();
+        $userB->setId(6);
         $userB->setEmail('watson@example.com');
         $userB->getSetting()->setMailCommentResolved(true);
         $userC = new User();
+        $userC->setId(7);
         $userC->setEmail('enola@example.com');
         $userC->getSetting()->setMailCommentResolved(false);
         $comment = new Comment();

diff --git a/tests/Unit/Service/Mail/MailRecipientServiceTest.php b/tests/Unit/Service/Mail/MailRecipientServiceTest.php
@@ -85,8 +85,11 @@ public function testGetUserForComment(): void
     public function testGetUsersForReview(): void
     {
         $userA = new User();
+        $userA->setId(123);
         $userB = new User();
+        $userB->setId(456);
         $userC = new User();
+        $userC->setId(789);
 
         $reviewerA = new CodeReviewer();
         $reviewerA->setUser($userA);