Merge branch 'notification/deferring-sending-notification-to-terminate' into 'master'

Notification/deferring sending notification to terminate

See merge request Chill-Projet/chill-bundles!414

CHANGELOG.md

@@ -23,6 +23,8 @@ and this project adheres to
 * [parcours]: change wording of warning message and button when user is not associated to a household yet (https://gitlab.com/champs-libres/departement-de-la-vendee/chill/-/issues/590#note_918370943)
 * [Accompanying period work evaluations] list documents associated to a work by creation date, and then by id, from the most recent to older
 * [Course comment] add validationConstraint NotNull and NotBlank on comment content, to avoid sql error
+* [Notifications] delay the sending of notificaiton to kernel.terminate
+* [Notifications / Period user change] fix the sending of notification when user changes 
 
 ## Test releases
 

src/Bundle/ChillMainBundle/Notification/EventListener/PersistNotificationOnTerminateEventSubscriber.php

@@ -0,0 +1,58 @@
+<?php
+
+/**
+ * Chill is a software for social workers
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Chill\MainBundle\Notification\EventListener;
+
+use Chill\MainBundle\Notification\NotificationPersisterInterface;
+use Doctrine\ORM\EntityManagerInterface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\Event\TerminateEvent;
+use function count;
+
+class PersistNotificationOnTerminateEventSubscriber implements EventSubscriberInterface
+{
+    private EntityManagerInterface $em;
+
+    private NotificationPersisterInterface $persister;
+
+    public function __construct(EntityManagerInterface $em, NotificationPersisterInterface $persister)
+    {
+        $this->em = $em;
+        $this->persister = $persister;
+    }
+
+    public static function getSubscribedEvents()
+    {
+        return [
+            'kernel.terminate' => [
+                ['onKernelTerminate', 1024], // we must ensure that the priority is before sending email
+            ],
+        ];
+    }
+
+    public function onKernelTerminate(TerminateEvent $event): void
+    {
+        if ($event->isMasterRequest()) {
+            $this->persistNotifications();
+        }
+    }
+
+    private function persistNotifications(): void
+    {
+        if (0 < count($this->persister->getWaitingNotifications())) {
+            foreach ($this->persister->getWaitingNotifications() as $notification) {
+                $this->em->persist($notification);
+            }
+
+            $this->em->flush();
+        }
+    }
+}

src/Bundle/ChillMainBundle/Notification/NotificationPersister.php

@@ -0,0 +1,32 @@
+<?php
+
+/**
+ * Chill is a software for social workers
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Chill\MainBundle\Notification;
+
+use Chill\MainBundle\Entity\Notification;
+
+class NotificationPersister implements NotificationPersisterInterface
+{
+    private array $waitingNotifications = [];
+
+    /**
+     * @return array|Notification[]
+     */
+    public function getWaitingNotifications(): array
+    {
+        return $this->waitingNotifications;
+    }
+
+    public function persist(Notification $notification): void
+    {
+        $this->waitingNotifications[] = $notification;
+    }
+}

src/Bundle/ChillMainBundle/Notification/NotificationPersisterInterface.php

@@ -0,0 +1,31 @@
+<?php
+
+/**
+ * Chill is a software for social workers
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Chill\MainBundle\Notification;
+
+use Chill\MainBundle\Entity\Notification;
+
+/**
+ * Store the notification.
+ *
+ * Those notification will be stored into database by the kernel. This
+ * will also ensure that this happens outside of regular operations
+ * operated by the entity manager.
+ */
+interface NotificationPersisterInterface
+{
+    /**
+     * @return array|Notification[]
+     */
+    public function getWaitingNotifications(): array;
+
+    public function persist(Notification $notification): void;
+}

src/Bundle/ChillMainBundle/Tests/Notification/EventListener/PersistNotificationOnTerminateEventSubscriberTest.php

@@ -0,0 +1,47 @@
+<?php
+
+/**
+ * Chill is a software for social workers
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Notification\EventListener;
+
+use Chill\MainBundle\Entity\Notification;
+use Chill\MainBundle\Notification\EventListener\PersistNotificationOnTerminateEventSubscriber;
+use Chill\MainBundle\Notification\NotificationPersister;
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Symfony\Component\HttpKernel\Event\TerminateEvent;
+
+/**
+ * @internal
+ * @coversNothing
+ */
+final class PersistNotificationOnTerminateEventSubscriberTest extends TestCase
+{
+    use ProphecyTrait;
+
+    public function testNotificationIsPersisted()
+    {
+        $persister = new NotificationPersister();
+        $em = $this->prophesize(EntityManagerInterface::class);
+        $em->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
+        $em->flush()->shouldBeCalledTimes(1);
+        $event = $this->prophesize(TerminateEvent::class);
+        $event->isMasterRequest()->willReturn(true);
+
+        $eventSubscriber = new PersistNotificationOnTerminateEventSubscriber($em->reveal(), $persister);
+
+        $notification = new Notification();
+        $persister->persist($notification);
+
+        $eventSubscriber->onKernelTerminate($event->reveal());
+    }
+}

src/Bundle/ChillMainBundle/config/services/notification.yaml

@@ -3,6 +3,11 @@ services:
         autowire: true
         autoconfigure: true
 
+    Chill\MainBundle\Notification\:
+        resource: ../../Notification/
+        autoconfigure: true
+        autowire: true
+
     Chill\MainBundle\Notification\Mailer:
         arguments:
             $logger: '@Psr\Log\LoggerInterface'
@@ -17,12 +22,6 @@ services:
         arguments:
             $handlers: !tagged_iterator chill_main.notification_handler
 
-    Chill\MainBundle\Notification\NotificationPresence: ~
-
-    Chill\MainBundle\Notification\Templating\NotificationTwigExtension: ~
-
-    Chill\MainBundle\Notification\Templating\NotificationTwigExtensionRuntime: ~
-
     Chill\MainBundle\Notification\Counter\NotificationByUserCounter:
         autoconfigure: true
         autowire: true

src/Bundle/ChillPersonBundle/AccompanyingPeriod/Events/PersonAddressMoveEventSubscriber.php

@@ -13,10 +13,10 @@ namespace Chill\PersonBundle\AccompanyingPeriod\Events;
 
 use Chill\MainBundle\Entity\Address;
 use Chill\MainBundle\Entity\Notification;
+use Chill\MainBundle\Notification\NotificationPersisterInterface;
 use Chill\PersonBundle\Entity\AccompanyingPeriod;
 use Chill\PersonBundle\Event\Person\PersonAddressMoveEvent;
 use DateTimeImmutable;
-use Doctrine\ORM\EntityManagerInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\Security\Core\Security;
 use Symfony\Component\Templating\EngineInterface;
@@ -26,7 +26,7 @@ class PersonAddressMoveEventSubscriber implements EventSubscriberInterface
 {
     private EngineInterface $engine;
 
-    private EntityManagerInterface $entityManager;
+    private NotificationPersisterInterface $notificationPersister;
 
     private Security $security;
 
@@ -34,12 +34,12 @@ class PersonAddressMoveEventSubscriber implements EventSubscriberInterface
 
     public function __construct(
         EngineInterface $engine,
-        EntityManagerInterface $entityManager,
+        NotificationPersisterInterface $notificationPersister,
         Security $security,
         TranslatorInterface $translator
     ) {
         $this->engine = $engine;
-        $this->entityManager = $entityManager;
+        $this->notificationPersister = $notificationPersister;
         $this->security = $security;
         $this->translator = $translator;
     }
@@ -87,7 +87,7 @@ class PersonAddressMoveEventSubscriber implements EventSubscriberInterface
                             'period' => $period,
                         ]));
 
-                    $this->entityManager->persist($notification);
+                    $this->notificationPersister->persist($notification);
                 }
             }
         }

src/Bundle/ChillPersonBundle/AccompanyingPeriod/Events/UserRefEventSubscriber.php

@@ -13,8 +13,8 @@ namespace Chill\PersonBundle\AccompanyingPeriod\Events;
 
 use Chill\MainBundle\Entity\Notification;
 use Chill\MainBundle\Entity\User;
+use Chill\MainBundle\Notification\NotificationPersisterInterface;
 use Chill\PersonBundle\Entity\AccompanyingPeriod;
-use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\Persistence\Event\LifecycleEventArgs;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\Security\Core\Security;
@@ -24,20 +24,20 @@ use Symfony\Contracts\Translation\TranslatorInterface;
 
 class UserRefEventSubscriber implements EventSubscriberInterface
 {
-    private EntityManagerInterface $em;
-
     private EngineInterface $engine;
 
+    private NotificationPersisterInterface $notificationPersister;
+
     private Security $security;
 
     private TranslatorInterface $translator;
 
-    public function __construct(Security $security, TranslatorInterface $translator, EngineInterface $engine, EntityManagerInterface $em)
+    public function __construct(Security $security, TranslatorInterface $translator, EngineInterface $engine, NotificationPersisterInterface $notificationPersister)
     {
         $this->security = $security;
         $this->translator = $translator;
         $this->engine = $engine;
-        $this->em = $em;
+        $this->notificationPersister = $notificationPersister;
     }
 
     public static function getSubscribedEvents()
@@ -58,16 +58,13 @@ class UserRefEventSubscriber implements EventSubscriberInterface
 
     public function postUpdate(AccompanyingPeriod $period, LifecycleEventArgs $args): void
     {
-        if ($period->hasPreviousUser()
+        if ($period->isChangedUser()
             && $period->getUser() !== $this->security->getUser()
             && null !== $period->getUser()
             && $period->getStep() !== AccompanyingPeriod::STEP_DRAFT
         ) {
             $this->generateNotificationToUser($period);
         }
-
-        // we are just out of a flush operation. Launch a new one
-        $this->em->flush();
     }
 
     private function generateNotificationToUser(AccompanyingPeriod $period)
@@ -89,7 +86,7 @@ class UserRefEventSubscriber implements EventSubscriberInterface
             ))
             ->addAddressee($period->getUser());
 
-        $this->em->persist($notification);
+        $this->notificationPersister->persist($notification);
     }
 
     private function onPeriodConfirmed(AccompanyingPeriod $period)

src/Bundle/ChillPersonBundle/Controller/HouseholdApiController.php

@@ -102,7 +102,6 @@ class HouseholdApiController extends ApiController
             $event
                 ->setPreviousAddress($household->getPreviousAddressOf($address))
                 ->setNextAddress($address);
-            dump($event);
             $this->eventDispatcher->dispatch($event);
         }
 

src/Bundle/ChillPersonBundle/Entity/AccompanyingPeriod.php

@@ -361,6 +361,8 @@ class AccompanyingPeriod implements
      */
     private Collection $userHistories;
 
+    private bool $userIsChanged = false;
+
     /**
      * Temporary field, which is filled when the user is changed.
      *
@@ -978,6 +980,11 @@ class AccompanyingPeriod implements
         return null !== $this->userPrevious;
     }
 
+    public function isChangedUser(): bool
+    {
+        return $this->userIsChanged && $this->user !== $this->userPrevious;
+    }
+
     /**
      * Returns true if the closing date is after the opening date.
      */
@@ -1103,6 +1110,14 @@ class AccompanyingPeriod implements
         $this->setStep(AccompanyingPeriod::STEP_CONFIRMED);
     }
 
+    public function resetPreviousUser(): self
+    {
+        $this->userPrevious = null;
+        $this->userIsChanged = false;
+
+        return $this;
+    }
+
     /**
      * @Groups({"write"})
      */
@@ -1329,6 +1344,7 @@ class AccompanyingPeriod implements
     {
         if ($this->user !== $user) {
             $this->userPrevious = $this->user;
+            $this->userIsChanged = true;
 
             foreach ($this->userHistories as $history) {
                 if (null === $history->getEndDate()) {

src/Bundle/ChillPersonBundle/Entity/AccompanyingPeriod/Comment.php

@@ -41,8 +41,8 @@ class Comment implements TrackCreationInterface, TrackUpdateInterface
     /**
      * @ORM\Column(type="text")
      * @Groups({"read", "write"})
-     * @Assert\NotBlank()
+     * @Assert\NotBlank
-     * @Assert\NotNull()
+     * @Assert\NotNull
      */
     private $content;
 

src/Bundle/ChillPersonBundle/Tests/AccompanyingPeriod/Events/PersonMoveEventSubscriberTest.php

@@ -14,6 +14,8 @@ namespace AccompanyingPeriod\Events;
 use Chill\MainBundle\Entity\Address;
 use Chill\MainBundle\Entity\Notification;
 use Chill\MainBundle\Entity\User;
+use Chill\MainBundle\Notification\NotificationPersister;
+use Chill\MainBundle\Notification\NotificationPersisterInterface;
 use Chill\PersonBundle\AccompanyingPeriod\Events\PersonAddressMoveEventSubscriber;
 use Chill\PersonBundle\Entity\AccompanyingPeriod;
 use Chill\PersonBundle\Entity\Household\Household;
@@ -22,7 +24,6 @@ use Chill\PersonBundle\Entity\Person;
 use Chill\PersonBundle\Event\Person\PersonAddressMoveEvent;
 use DateTime;
 use DateTimeImmutable;
-use Doctrine\ORM\EntityManagerInterface;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use ReflectionClass;
@@ -73,9 +74,9 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
             ->setPreviousMembership($previousMembership)
             ->setNextMembership($nextMembership);
 
-        $em = $this->prophesize(EntityManagerInterface::class);
+        $notificationPersister = $this->prophesize(NotificationPersisterInterface::class);
-        $em->persist(Argument::type(Notification::class))->shouldNotBeCalled();
+        $notificationPersister->persist(Argument::type(Notification::class))->shouldNotBeCalled();
-        $eventSubscriber = $this->buildSubscriber(null, $em->reveal(), null, null);
+        $eventSubscriber = $this->buildSubscriber(null, $notificationPersister->reveal(), null, null);
 
         $eventSubscriber->resetPeriodLocation($event);
     }
@@ -115,9 +116,9 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
             ->setPreviousMembership($previousMembership)
             ->setNextMembership($nextMembership);
 
-        $em = $this->prophesize(EntityManagerInterface::class);
+        $notificationPersister = $this->prophesize(NotificationPersisterInterface::class);
-        $em->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
+        $notificationPersister->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
-        $eventSubscriber = $this->buildSubscriber(null, $em->reveal(), null, null);
+        $eventSubscriber = $this->buildSubscriber(null, $notificationPersister->reveal(), null, null);
 
         $eventSubscriber->resetPeriodLocation($event);
 
@@ -160,9 +161,9 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
             ->setPreviousMembership($previousMembership)
             ->setNextMembership($nextMembership);
 
-        $em = $this->prophesize(EntityManagerInterface::class);
+        $notificationPersister = $this->prophesize(NotificationPersisterInterface::class);
-        $em->persist(Argument::type(Notification::class))->shouldBeCalled(1);
+        $notificationPersister->persist(Argument::type(Notification::class))->shouldBeCalled(1);
-        $eventSubscriber = $this->buildSubscriber(null, $em->reveal(), null, null);
+        $eventSubscriber = $this->buildSubscriber(null, $notificationPersister->reveal(), null, null);
 
         $eventSubscriber->resetPeriodLocation($event);
 
@@ -195,9 +196,9 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
         $event
             ->setPreviousMembership($previousMembership);
 
-        $em = $this->prophesize(EntityManagerInterface::class);
+        $notificationPersister = $this->prophesize(NotificationPersisterInterface::class);
-        $em->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
+        $notificationPersister->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
-        $eventSubscriber = $this->buildSubscriber(null, $em->reveal(), null, null);
+        $eventSubscriber = $this->buildSubscriber(null, $notificationPersister->reveal(), null, null);
 
         $eventSubscriber->resetPeriodLocation($event);
 
@@ -235,9 +236,9 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
             ->setPreviousAddress($household->getPreviousAddressOf($newAddress))
             ->setNextAddress($newAddress);
 
-        $em = $this->prophesize(EntityManagerInterface::class);
+        $notificationPersister = $this->prophesize(NotificationPersisterInterface::class);
-        $em->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
+        $notificationPersister->persist(Argument::type(Notification::class))->shouldBeCalledTimes(1);
-        $eventSubscriber = $this->buildSubscriber(null, $em->reveal(), null, null);
+        $eventSubscriber = $this->buildSubscriber(null, $notificationPersister->reveal(), null, null);
 
         $eventSubscriber->resetPeriodLocation($event);
 
@@ -247,7 +248,7 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
 
     private function buildSubscriber(
         ?EngineInterface $engine = null,
-        ?EntityManagerInterface $entityManager = null,
+        ?NotificationPersisterInterface $notificationPersister = null,
         ?Security $security = null,
         ?TranslatorInterface $translator = null
     ): PersonAddressMoveEventSubscriber {
@@ -267,14 +268,13 @@ final class PersonMoveEventSubscriberTest extends KernelTestCase
             $engine = $double->reveal();
         }
 
-        if (null === $entityManager) {
+        if (null === $notificationPersister) {
-            $double = $this->prophesize(EntityManagerInterface::class);
+            $notificationPersister = new NotificationPersister();
-            $entityManager = $double->reveal();
         }
 
         return new PersonAddressMoveEventSubscriber(
             $engine,
-            $entityManager,
+            $notificationPersister,
             $security,
             $translator
         );

src/Bundle/ChillPersonBundle/Tests/Entity/AccompanyingPeriodTest.php

@@ -13,6 +13,7 @@ namespace Chill\PersonBundle\Tests\Entity;
 
 use ArrayIterator;
 use Chill\MainBundle\Entity\Address;
+use Chill\MainBundle\Entity\User;
 use Chill\PersonBundle\Entity\AccompanyingPeriod;
 use Chill\PersonBundle\Entity\AccompanyingPeriod\Comment;
 use Chill\PersonBundle\Entity\AccompanyingPeriodParticipation;
@@ -62,6 +63,29 @@ final class AccompanyingPeriodTest extends \PHPUnit\Framework\TestCase
         $this->assertFalse($period->isClosingAfterOpening());
     }
 
+    public function testHasChangedUser()
+    {
+        $period = new AccompanyingPeriod();
+
+        $this->assertFalse($period->isChangedUser());
+        $this->assertFalse($period->hasPreviousUser());
+
+        $period->setUser($user1 = new User());
+
+        $this->assertTrue($period->isChangedUser());
+        $this->assertFalse($period->hasPreviousUser());
+
+        $period->resetPreviousUser();
+        $this->assertFalse($period->isChangedUser());
+        $this->assertFalse($period->hasPreviousUser());
+
+        $period->setUser($user2 = new User());
+
+        $this->assertTrue($period->isChangedUser());
+        $this->assertTrue($period->hasPreviousUser());
+        $this->assertSame($user1, $period->getPreviousUser());
+    }
+
     public function testHistoryLocation()
     {
         $period = new AccompanyingPeriod();