Prevent invalid states due to filter segment

MAILPOET-5509

mailpoet/lib/Cron/Workers/SendingQueue/SendingQueue.php

@@ -11,6 +11,7 @@ use MailPoet\Cron\Workers\StatsNotifications\Scheduler as StatsNotificationsSche
 use MailPoet\Entities\NewsletterEntity;
 use MailPoet\Entities\ScheduledTaskEntity;
 use MailPoet\Entities\SubscriberEntity;
+use MailPoet\InvalidStateException;
 use MailPoet\Logging\LoggerFactory;
 use MailPoet\Mailer\MailerLog;
 use MailPoet\Mailer\MetaInfo;
@@ -237,7 +238,17 @@ class SendingQueue {
       );
       if (!empty($newsletterSegmentsIds[0])) {
         // Check that subscribers are in segments
-        $foundSubscribersIds = $this->subscribersFinder->findSubscribersInSegments($subscribersToProcessIds, $newsletterSegmentsIds, $filterSegmentId);
+        try {
+          $foundSubscribersIds = $this->subscribersFinder->findSubscribersInSegments($subscribersToProcessIds, $newsletterSegmentsIds, $filterSegmentId);
+        } catch (InvalidStateException $exception) {
+          $this->loggerFactory->getLogger(LoggerFactory::TOPIC_NEWSLETTERS)->info(
+            'paused task in sending queue due to problem finding subscribers: ' . $exception->getMessage(),
+            ['task_id' => $queue->taskId]
+          );
+          $queue->status = ScheduledTaskEntity::STATUS_PAUSED;
+          $queue->save();
+          return;
+        }
         $foundSubscribers = empty($foundSubscribersIds) ? [] : SubscriberModel::whereIn('id', $foundSubscribersIds)
           ->whereNull('deleted_at')
           ->findMany();

mailpoet/lib/Logging/LoggerFactory.php

@@ -36,6 +36,7 @@ class LoggerFactory {
   const TOPIC_TRACKING = 'tracking';
   const TOPIC_COUPONS = 'coupons';
   const TOPIC_PROVISIONING = 'provisioning';
+  const TOPIC_SEGMENTS = 'segments';
 
   /** @var LoggerFactory */
   private static $instance;

mailpoet/lib/Segments/SegmentsRepository.php

@@ -11,6 +11,8 @@ use MailPoet\Entities\NewsletterSegmentEntity;
 use MailPoet\Entities\SegmentEntity;
 use MailPoet\Entities\SubscriberSegmentEntity;
 use MailPoet\Form\FormsRepository;
+use MailPoet\InvalidStateException;
+use MailPoet\Logging\LoggerFactory;
 use MailPoet\Newsletter\Segment\NewsletterSegmentRepository;
 use MailPoet\NotFoundException;
 use MailPoet\WP\Functions as WPFunctions;
@@ -33,16 +35,21 @@ class SegmentsRepository extends Repository {
   /** @var WPFunctions */
   private $wp;
 
+  /** @var LoggerFactory */
+  private $loggerFactory;
+
   public function __construct(
     EntityManager $entityManager,
     NewsletterSegmentRepository $newsletterSegmentRepository,
     FormsRepository $formsRepository,
-    WPFunctions $wp
+    WPFunctions $wp,
+    LoggerFactory $loggerFactory
   ) {
     parent::__construct($entityManager);
     $this->newsletterSegmentRepository = $newsletterSegmentRepository;
     $this->formsRepository = $formsRepository;
     $this->wp = $wp;
+    $this->loggerFactory = $loggerFactory;
   }
 
   protected function getEntityClassName() {
@@ -54,7 +61,7 @@ class SegmentsRepository extends Repository {
    * @return SegmentEntity[]
    */
   public function findByTypeNotIn(array $types): array {
-    return $this->doctrineRepository->createQueryBuilder('s')
+      return $this->doctrineRepository->createQueryBuilder('s')
       ->select('s')
       ->where('s.type NOT IN (:types)')
       ->setParameter('types', $types)
@@ -136,6 +143,28 @@ class SegmentsRepository extends Repository {
     }
   }
 
+  /**
+   * @param int $id
+   *
+   * @return SegmentEntity
+   * @throws InvalidStateException
+   */
+  public function verifyFilterSegmentExists(int $id): SegmentEntity {
+    try {
+      $filterSegment = $this->findOneById($id);
+      if (!$filterSegment instanceof SegmentEntity) {
+        throw InvalidStateException::create()->withMessage("Filter segment with ID of $id could not be found.");
+      }
+      if ($filterSegment->getType() !== SegmentEntity::TYPE_DYNAMIC) {
+        throw InvalidStateException::create()->withMessage("Filter segment ID must be a dynamic segment. Type of filter with ID {$filterSegment->getId()} is {$filterSegment->getType()}.");
+      }
+    } catch (InvalidStateException $exception) {
+      $this->loggerFactory->getLogger(LoggerFactory::TOPIC_SEGMENTS)->error('Error verifying filter segment: ' . $exception->getMessage());
+      throw $exception;
+    }
+    return $filterSegment;
+  }
+
   /**
    * @param DynamicSegmentFilterData[] $filtersData
    * @throws ConflictException

mailpoet/lib/Segments/SubscribersFinder.php

@@ -8,7 +8,6 @@ use MailPoet\Entities\SegmentEntity;
 use MailPoet\Entities\SubscriberEntity;
 use MailPoet\Entities\SubscriberSegmentEntity;
 use MailPoet\InvalidStateException;
-use MailPoet\UnexpectedValueException;
 use MailPoetVendor\Doctrine\DBAL\Connection;
 use MailPoetVendor\Doctrine\DBAL\ParameterType;
 use MailPoetVendor\Doctrine\ORM\EntityManager;
@@ -34,6 +33,10 @@ class SubscribersFinder {
     $this->entityManager = $entityManager;
   }
 
+  /**
+   * @return array
+   * @throws InvalidStateException
+   */
   public function findSubscribersInSegments($subscribersToProcessIds, $newsletterSegmentsIds, ?int $filterSegmentId = null) {
     $result = [];
     foreach ($newsletterSegmentsIds as $segmentId) {
@@ -45,14 +48,9 @@ class SubscribersFinder {
     }
 
     if (is_int($filterSegmentId)) {
-      $filterSegment = $this->segmentsRepository->findOneById($filterSegmentId);
+      $filterSegment = $this->segmentsRepository->verifyFilterSegmentExists($filterSegmentId);
-      if ($filterSegment instanceof SegmentEntity) {
+      $idsInFilterSegment = $this->findSubscribersInSegment($filterSegment, $subscribersToProcessIds);
-        if ($filterSegment->getType() !== SegmentEntity::TYPE_DYNAMIC) {
+      $result = array_intersect($result, $idsInFilterSegment);
-          throw new UnexpectedValueException("Filter segment ID must be a dynamic segment. Type of filter with ID {$filterSegment->getId()} is {$filterSegment->getType()}.");
-        }
-        $idsInFilterSegment = $this->findSubscribersInSegment($filterSegment, $subscribersToProcessIds);
-        $result = array_intersect($result, $idsInFilterSegment);
-      }
     }
 
     return $this->unique($result);
@@ -74,6 +72,13 @@ class SubscribersFinder {
    */
   public function addSubscribersToTaskFromSegments(ScheduledTaskEntity $task, array $segmentIds, ?int $filterSegmentId = null) {
     // Prepare subscribers on the DB side for performance reasons
+    if (is_int($filterSegmentId)) {
+      try {
+        $this->segmentsRepository->verifyFilterSegmentExists($filterSegmentId);
+      } catch (InvalidStateException $exception) {
+        return 0;
+      }
+    }
     $staticSegmentIds = [];
     $dynamicSegmentIds = [];
     foreach ($segmentIds as $segment) {

mailpoet/tests/integration/Segments/SubscribersFinderTest.php

@@ -6,13 +6,13 @@ use Codeception\Util\Stub;
 use MailPoet\Entities\ScheduledTaskEntity;
 use MailPoet\Entities\SegmentEntity;
 use MailPoet\Entities\SubscriberEntity;
+use MailPoet\InvalidStateException;
 use MailPoet\Newsletter\Sending\ScheduledTaskSubscribersRepository;
 use MailPoet\Tasks\Sending as SendingTask;
 use MailPoet\Test\DataFactories\DynamicSegment;
 use MailPoet\Test\DataFactories\ScheduledTask as ScheduledTaskFactory;
 use MailPoet\Test\DataFactories\Segment as SegmentFactory;
 use MailPoet\Test\DataFactories\Subscriber as SubscriberFactory;
-use MailPoet\UnexpectedValueException;
 use MailPoetVendor\Carbon\Carbon;
 use PHPUnit\Framework\MockObject\MockObject;
 
@@ -214,7 +214,7 @@ class SubscribersFinderTest extends \MailPoetTest {
   }
 
   public function testFilterSegmentMustBeDynamicSegment() {
-    $this->expectException(UnexpectedValueException::class);
+    $this->expectException(InvalidStateException::class);
     $this->subscribersFinder->findSubscribersInSegments([$this->subscriber1->getId()], [$this->segment1->getId()], $this->segment2->getId());
   }