From fe44df1884a492ea5fb96ce80fa948a175c54267 Mon Sep 17 00:00:00 2001 From: John Oleksowicz Date: Mon, 28 Aug 2023 11:07:30 -0500 Subject: [PATCH] Prevent invalid states due to filter segment MAILPOET-5509 --- .../Workers/SendingQueue/SendingQueue.php | 13 +++++++- mailpoet/lib/Logging/LoggerFactory.php | 1 + mailpoet/lib/Segments/SegmentsRepository.php | 33 +++++++++++++++++-- mailpoet/lib/Segments/SubscribersFinder.php | 23 ++++++++----- .../Segments/SubscribersFinderTest.php | 4 +-- 5 files changed, 60 insertions(+), 14 deletions(-) diff --git a/mailpoet/lib/Cron/Workers/SendingQueue/SendingQueue.php b/mailpoet/lib/Cron/Workers/SendingQueue/SendingQueue.php index 93221512ba..131bae3bc3 100644 --- a/mailpoet/lib/Cron/Workers/SendingQueue/SendingQueue.php +++ b/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(); diff --git a/mailpoet/lib/Logging/LoggerFactory.php b/mailpoet/lib/Logging/LoggerFactory.php index 45a6853368..4d288c8647 100644 --- a/mailpoet/lib/Logging/LoggerFactory.php +++ b/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; diff --git a/mailpoet/lib/Segments/SegmentsRepository.php b/mailpoet/lib/Segments/SegmentsRepository.php index f201503530..605d15a524 100644 --- a/mailpoet/lib/Segments/SegmentsRepository.php +++ b/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 diff --git a/mailpoet/lib/Segments/SubscribersFinder.php b/mailpoet/lib/Segments/SubscribersFinder.php index ad50c2dd2b..9b3b46a0b6 100644 --- a/mailpoet/lib/Segments/SubscribersFinder.php +++ b/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); - if ($filterSegment instanceof SegmentEntity) { - if ($filterSegment->getType() !== SegmentEntity::TYPE_DYNAMIC) { - 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); - } + $filterSegment = $this->segmentsRepository->verifyFilterSegmentExists($filterSegmentId); + $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) { diff --git a/mailpoet/tests/integration/Segments/SubscribersFinderTest.php b/mailpoet/tests/integration/Segments/SubscribersFinderTest.php index 74cf2c6cf7..1fdeaec00d 100644 --- a/mailpoet/tests/integration/Segments/SubscribersFinderTest.php +++ b/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()); }