Cavemanon/piratepoet
7
Fork 0
Code Pull Requests Actions Releases 16 Activity

Refactor SubscriberFinder::addSubscribersToTaskFromSegments to return void

Browse Source 
We removed the return value to prevent us from using it to check if there were recipients.
It is not 100% reliable for that purpose because it returns 0 for a repeated call.
[MAILPOET-6346]
This commit is contained in:
Rostislav Wolny
2024-11-27 11:22:07 +01:00
committed by Aschepikov
parent 74de985620
commit e6abec74d4
3 changed files with 38 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
mailpoet/lib/API/JSON/v1/SendingQueue.php
View File

@@ -166,7 +166,8 @@ class SendingQueue extends APIEndpoint {
$segments = $newsletter->getSegmentIds(); $segments = $newsletter->getSegmentIds();
$this->scheduledTasksRepository->refresh($scheduledTask); $this->scheduledTasksRepository->refresh($scheduledTask);
$subscribersCount = $this->subscribersFinder->addSubscribersToTaskFromSegments($scheduledTask, $segments, $newsletter->getFilterSegmentId()); $this->subscribersFinder->addSubscribersToTaskFromSegments($scheduledTask, $segments, $newsletter->getFilterSegmentId());
$subscribersCount = $scheduledTask->getSubscribers()->count();
if (!$subscribersCount) { if (!$subscribersCount) {
return $this->errorResponse([ return $this->errorResponse([

7
mailpoet/lib/Segments/SubscribersFinder.php
View File

@@ -68,15 +68,15 @@ class SubscribersFinder {
* @param ScheduledTaskEntity $task * @param ScheduledTaskEntity $task
* @param array<int> $segmentIds * @param array<int> $segmentIds
* *
* @return float|int * @return void
*/ */
public function addSubscribersToTaskFromSegments(ScheduledTaskEntity $task, array $segmentIds, ?int $filterSegmentId = null) { public function addSubscribersToTaskFromSegments(ScheduledTaskEntity $task, array $segmentIds, ?int $filterSegmentId = null): void {
// Prepare subscribers on the DB side for performance reasons // Prepare subscribers on the DB side for performance reasons
if (is_int($filterSegmentId)) { if (is_int($filterSegmentId)) {
try { try {
$this->segmentsRepository->verifyDynamicSegmentExists($filterSegmentId); $this->segmentsRepository->verifyDynamicSegmentExists($filterSegmentId);
} catch (InvalidStateException $exception) { } catch (InvalidStateException $exception) {
return 0; return;
} }
} }
$staticSegmentIds = []; $staticSegmentIds = [];
@@ -101,7 +101,6 @@ class SubscribersFinder {
if ($count > 0) { if ($count > 0) {
$this->entityManager->refresh($task); $this->entityManager->refresh($task);
} }
return $count;
} }
/** /**

59
mailpoet/tests/integration/Segments/SubscribersFinderTest.php
View File

@@ -17,13 +17,13 @@ use MailPoetVendor\Carbon\Carbon;
use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\MockObject\MockObject;
class SubscribersFinderTest extends \MailPoetTest { class SubscribersFinderTest extends \MailPoetTest {
public $scheduledTask; private ScheduledTaskEntity $scheduledTask;
public $subscriber3; private SubscriberEntity $subscriber3;
public $subscriber2; private SubscriberEntity $subscriber2;
public $subscriber1; private SubscriberEntity $subscriber1;
public $segment3; private SegmentEntity $segment3;
public $segment2; private SegmentEntity $segment2;
public $segment1; private SegmentEntity $segment1;
/** @var SubscribersFinder */ /** @var SubscribersFinder */
private $subscribersFinder; private $subscribersFinder;
@@ -96,26 +96,28 @@ class SubscribersFinderTest extends \MailPoetTest {
} }
public function testItAddsSubscribersToTaskFromStaticSegments() { public function testItAddsSubscribersToTaskFromStaticSegments() {
$subscribersCount = $this->subscribersFinder->addSubscribersToTaskFromSegments( $this->subscribersFinder->addSubscribersToTaskFromSegments(
$this->scheduledTask, $this->scheduledTask,
[ [
$this->segment1->getId(), (int)$this->segment1->getId(),
$this->segment2->getId(), (int)$this->segment2->getId(),
] ]
); );
$subscribersCount = $this->scheduledTask->getSubscribers()->count();
verify($subscribersCount)->equals(1); verify($subscribersCount)->equals(1);
$subscribersIds = $this->getScheduledTasksSubscribers($this->scheduledTask->getId()); $subscribersIds = $this->getScheduledTasksSubscribers((int)$this->scheduledTask->getId());
verify($subscribersIds)->equals([$this->subscriber2->getId()]); verify($subscribersIds)->equals([$this->subscriber2->getId()]);
} }
public function testItDoesNotAddSubscribersToTaskFromNoSegment() { public function testItDoesNotAddSubscribersToTaskFromNoSegment() {
$this->segment3->setType('Invalid type'); $this->segment3->setType('Invalid type');
$subscribersCount = $this->subscribersFinder->addSubscribersToTaskFromSegments( $this->subscribersFinder->addSubscribersToTaskFromSegments(
$this->scheduledTask, $this->scheduledTask,
[ [
$this->segment3->getId(), (int)$this->segment3->getId(),
] ]
); );
$subscribersCount = $this->scheduledTask->getSubscribers()->count();
verify($subscribersCount)->equals(0); verify($subscribersCount)->equals(0);
} }
@@ -129,14 +131,15 @@ class SubscribersFinderTest extends \MailPoetTest {
$this->segment2->setType(SegmentEntity::TYPE_DYNAMIC); $this->segment2->setType(SegmentEntity::TYPE_DYNAMIC);
$finder = new SubscribersFinder($mock, $this->segmentsRepository, $this->entityManager); $finder = new SubscribersFinder($mock, $this->segmentsRepository, $this->entityManager);
$subscribersCount = $finder->addSubscribersToTaskFromSegments( $finder->addSubscribersToTaskFromSegments(
$this->scheduledTask, $this->scheduledTask,
[ [
$this->segment2->getId(), (int)$this->segment2->getId(),
] ]
); );
$subscribersCount = $this->scheduledTask->getSubscribers()->count();
verify($subscribersCount)->equals(1); verify($subscribersCount)->equals(1);
$subscribersIds = $this->getScheduledTasksSubscribers($this->scheduledTask->getId()); $subscribersIds = $this->getScheduledTasksSubscribers((int)$this->scheduledTask->getId());
verify($subscribersIds)->equals([$this->subscriber1->getId()]); verify($subscribersIds)->equals([$this->subscriber1->getId()]);
} }
@@ -150,17 +153,17 @@ class SubscribersFinderTest extends \MailPoetTest {
$this->segment3->setType(SegmentEntity::TYPE_DYNAMIC); $this->segment3->setType(SegmentEntity::TYPE_DYNAMIC);
$finder = new SubscribersFinder($mock, $this->segmentsRepository, $this->entityManager); $finder = new SubscribersFinder($mock, $this->segmentsRepository, $this->entityManager);
$subscribersCount = $finder->addSubscribersToTaskFromSegments( $finder->addSubscribersToTaskFromSegments(
$this->scheduledTask, $this->scheduledTask,
[ [
$this->segment1->getId(), (int)$this->segment1->getId(),
$this->segment2->getId(), (int)$this->segment2->getId(),
$this->segment3->getId(), (int)$this->segment3->getId(),
] ]
); );
$subscribersCount = $this->scheduledTask->getSubscribers()->count();
verify($subscribersCount)->equals(1); verify($subscribersCount)->equals(1);
$subscribersIds = $this->getScheduledTasksSubscribers($this->scheduledTask->getId()); $subscribersIds = $this->getScheduledTasksSubscribers((int)$this->scheduledTask->getId());
verify($subscribersIds)->equals([$this->subscriber2->getId()]); verify($subscribersIds)->equals([$this->subscriber2->getId()]);
} }
@@ -179,20 +182,24 @@ class SubscribersFinderTest extends \MailPoetTest {
// Without filtering // Without filtering
$task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon()); $task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon());
$staticCount = $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$staticSegment->getId()]); $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$staticSegment->getId()]);
$staticCount = $task->getSubscribers()->count();
verify($staticCount)->equals(2); verify($staticCount)->equals(2);
$task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon()); $task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon());
$dynamicCount = $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$dynamicSegment->getId()]); $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$dynamicSegment->getId()]);
$dynamicCount = $task->getSubscribers()->count();
verify($dynamicCount)->equals(4); verify($dynamicCount)->equals(4);
// With filtering // With filtering
$task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon()); $task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon());
$staticCount = $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$staticSegment->getId()], $filterSegment->getId()); $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$staticSegment->getId()], $filterSegment->getId());
$staticCount = $task->getSubscribers()->count();
verify($staticCount)->equals(1); verify($staticCount)->equals(1);
$task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon()); $task = (new ScheduledTaskFactory())->create(SendingQueue::TASK_TYPE, ScheduledTaskEntity::STATUS_SCHEDULED, new Carbon());
$dynamicCount = $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$dynamicSegment->getId()], $filterSegment->getId()); $this->subscribersFinder->addSubscribersToTaskFromSegments($task, [$dynamicSegment->getId()], $filterSegment->getId());
$dynamicCount = $task->getSubscribers()->count();
verify($dynamicCount)->equals(2); verify($dynamicCount)->equals(2);
} }