diff --git a/mailpoet/lib/Newsletter/Scheduler/WelcomeScheduler.php b/mailpoet/lib/Newsletter/Scheduler/WelcomeScheduler.php index fc18e450c7..56c4abf956 100644 --- a/mailpoet/lib/Newsletter/Scheduler/WelcomeScheduler.php +++ b/mailpoet/lib/Newsletter/Scheduler/WelcomeScheduler.php @@ -4,6 +4,7 @@ namespace MailPoet\Newsletter\Scheduler; use MailPoet\Entities\NewsletterEntity; use MailPoet\Entities\NewsletterOptionFieldEntity; +use MailPoet\Entities\ScheduledTaskEntity; use MailPoet\Entities\SegmentEntity; use MailPoet\Entities\SendingQueueEntity; use MailPoet\Entities\SubscriberEntity; @@ -126,6 +127,19 @@ class WelcomeScheduler { $newsletter->getOptionValue(NewsletterOptionFieldEntity::NAME_AFTER_TIME_TYPE), $newsletter->getOptionValue(NewsletterOptionFieldEntity::NAME_AFTER_TIME_NUMBER) ); - return $sendingTask->save(); + + $savedSendingTask = $sendingTask->save(); + + // Refreshing this entity here is needed while we are still using Paris to create the scheduled tasks and queues + // in the code above using \MailPoet\Tasks\Sending class. Doing this should avoid bugs where the loaded entity contain + // stale data after the corresponding entry in the database is updated using Paris. This code can be removed once + // https://mailpoet.atlassian.net/browse/MAILPOET-4375 is finished. Currently, if this code is removed a few integration + // tests fail (see https://app.circleci.com/pipelines/github/mailpoet/mailpoet/14806/workflows/0d441848-16db-461a-88ec-87bed101fe36/jobs/251385/tests#failed-test-0). + $scheduledTaskEntity = $this->scheduledTasksRepository->findOneScheduledByNewsletterAndSubscriber($newsletter, $subscriber); + if ($scheduledTaskEntity instanceof ScheduledTaskEntity) { + $this->scheduledTasksRepository->refresh($scheduledTaskEntity); + } + + return $savedSendingTask; } } diff --git a/mailpoet/lib/Newsletter/Sending/ScheduledTaskSubscribersRepository.php b/mailpoet/lib/Newsletter/Sending/ScheduledTaskSubscribersRepository.php index 3e4c4845c9..cf3014afac 100644 --- a/mailpoet/lib/Newsletter/Sending/ScheduledTaskSubscribersRepository.php +++ b/mailpoet/lib/Newsletter/Sending/ScheduledTaskSubscribersRepository.php @@ -125,6 +125,17 @@ class ScheduledTaskSubscribersRepository extends Repository { ->execute(); } + public function setSubscribers(ScheduledTaskEntity $task, array $subscriberIds): void { + $this->deleteByScheduledTask($task); + + foreach ($subscriberIds as $subscriberId) { + $this->createOrUpdate([ + 'task_id' => $task->getId(), + 'subscriber_id' => $subscriberId, + ]); + } + } + private function checkCompleted(ScheduledTaskEntity $task): void { $count = $this->countBy(['task' => $task, 'processed' => ScheduledTaskSubscriberEntity::STATUS_UNPROCESSED]); if ($count === 0) { diff --git a/mailpoet/lib/Tasks/Sending.php b/mailpoet/lib/Tasks/Sending.php index 09d5edfd0e..e0adfb09f7 100644 --- a/mailpoet/lib/Tasks/Sending.php +++ b/mailpoet/lib/Tasks/Sending.php @@ -4,6 +4,7 @@ namespace MailPoet\Tasks; use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueAlias; use MailPoet\DI\ContainerWrapper; +use MailPoet\Entities\ScheduledTaskEntity; use MailPoet\Entities\ScheduledTaskSubscriberEntity; use MailPoet\Entities\SendingQueueEntity; use MailPoet\InvalidStateException; @@ -11,6 +12,7 @@ use MailPoet\Logging\LoggerFactory; use MailPoet\Models\ScheduledTask; use MailPoet\Models\ScheduledTaskSubscriber; use MailPoet\Models\SendingQueue; +use MailPoet\Newsletter\Sending\ScheduledTasksRepository; use MailPoet\Newsletter\Sending\ScheduledTaskSubscribersRepository; use MailPoet\Newsletter\Sending\SendingQueuesRepository; use MailPoet\Util\Helpers; @@ -58,6 +60,12 @@ class Sending { 'deleted_at', ]; + /** @var ScheduledTaskSubscribersRepository */ + private $scheduledTaskSubscribersRepository; + + /** @var ScheduledTasksRepository */ + private $scheduledTasksRepository; + private function __construct( ScheduledTask $task = null, SendingQueue $queue = null @@ -81,6 +89,8 @@ class Sending { $this->task = $task; $this->queue = $queue; $this->taskSubscribers = new Subscribers($task); + $this->scheduledTaskSubscribersRepository = ContainerWrapper::getInstance()->get(ScheduledTaskSubscribersRepository::class); + $this->scheduledTasksRepository = ContainerWrapper::getInstance()->get(ScheduledTasksRepository::class); } public static function create(ScheduledTask $task = null, SendingQueue $queue = null) { @@ -214,16 +224,14 @@ class Sending { } public function getSubscribers($processed = null) { - $scheduledTaskSubscribersRepository = ContainerWrapper::getInstance()->get(ScheduledTaskSubscribersRepository::class); - if (is_null($processed)) { - $subscribers = $scheduledTaskSubscribersRepository->findBy(['task' => $this->task->id]); + $subscribers = $this->scheduledTaskSubscribersRepository->findBy(['task' => $this->task->id]); } else if ($processed) { - $subscribers = $scheduledTaskSubscribersRepository->findBy( + $subscribers = $this->scheduledTaskSubscribersRepository->findBy( ['task' => $this->task->id, 'processed' => ScheduledTaskSubscriberEntity::STATUS_PROCESSED] ); } else { - $subscribers = $scheduledTaskSubscribersRepository->findBy( + $subscribers = $this->scheduledTaskSubscribersRepository->findBy( ['task' => $this->task->id, 'processed' => ScheduledTaskSubscriberEntity::STATUS_UNPROCESSED] ); } @@ -237,8 +245,12 @@ class Sending { } public function setSubscribers(array $subscriberIds) { - $this->taskSubscribers->setSubscribers($subscriberIds); - $this->updateCount(); + $scheduledTaskEntity = $this->scheduledTasksRepository->findOneById($this->task->id); + + if ($scheduledTaskEntity instanceof ScheduledTaskEntity) { + $this->scheduledTaskSubscribersRepository->setSubscribers($scheduledTaskEntity, $subscriberIds); + $this->updateCount(); + } } public function removeSubscribers(array $subscriberIds) { diff --git a/mailpoet/lib/Tasks/Subscribers.php b/mailpoet/lib/Tasks/Subscribers.php index 9554da888d..a7986a260b 100644 --- a/mailpoet/lib/Tasks/Subscribers.php +++ b/mailpoet/lib/Tasks/Subscribers.php @@ -14,10 +14,6 @@ class Subscribers { $this->task = $task; } - public function setSubscribers(array $subscriberIds) { - ScheduledTaskSubscriber::setSubscribers($this->task->id, $subscriberIds); - } - public function getSubscribers() { return ScheduledTaskSubscriber::where('task_id', $this->task->id); } diff --git a/mailpoet/tests/integration/Newsletter/Sending/ScheduledTaskSubscribersRepositoryTest.php b/mailpoet/tests/integration/Newsletter/Sending/ScheduledTaskSubscribersRepositoryTest.php new file mode 100644 index 0000000000..9be9275c15 --- /dev/null +++ b/mailpoet/tests/integration/Newsletter/Sending/ScheduledTaskSubscribersRepositoryTest.php @@ -0,0 +1,73 @@ +repository = $this->diContainer->get(ScheduledTaskSubscribersRepository::class); + $scheduledTaskFactory = new ScheduledTaskFactory(); + $this->subscriberFactory = new SubscriberFactory(); + $taskSubscriberFactory = new TaskSubscriberFactory(); + + $this->subscriberUnprocessed = $this->subscriberFactory->withEmail('subscriberUprocessed@email.com')->create(); + $this->subscriberProcessed = $this->subscriberFactory->withEmail('subscriberProcessed@email.com')->create(); + $subscriberFailed = $this->subscriberFactory->withEmail('subscriberFailed@email.com')->create(); + $this->subscriberFactory->withEmail('subscriberNotIncluded@email.com')->create(); + + $this->scheduledTask1 = $scheduledTaskFactory->create('sending', ScheduledTaskEntity::STATUS_COMPLETED, Carbon::now()->subDay()); + $this->scheduledTask2 = $scheduledTaskFactory->create('sending', ScheduledTaskEntity::STATUS_COMPLETED, Carbon::now()->subDay()); + + $taskSubscriberFactory->createUnprocessed($this->scheduledTask1, $this->subscriberUnprocessed); + $taskSubscriberFactory->createProcessed($this->scheduledTask1, $this->subscriberProcessed); + $taskSubscriberFactory->createFailed($this->scheduledTask1, $subscriberFailed, 'Error Message'); + + $taskSubscriberFactory->createUnprocessed($this->scheduledTask2, $this->subscriberUnprocessed); + $taskSubscriberFactory->createProcessed($this->scheduledTask2, $this->subscriberProcessed); + } + + public function testItSetsSubscribers() { + $subscriber = $this->subscriberFactory->withEmail('newsubscriber@email.com')->create(); + + $this->assertCount(3, $this->repository->findBy(['task' => $this->scheduledTask1])); + $this->repository->setSubscribers($this->scheduledTask1, [$subscriber->getId()]); + $task1Subscribers = $this->repository->findBy(['task' => $this->scheduledTask1]); + $this->assertCount(1, $task1Subscribers); + $this->assertInstanceOf(SubscriberEntity::class, $task1Subscribers[0]->getSubscriber()); + $this->assertEquals($subscriber->getId(), $task1Subscribers[0]->getSubscriber()->getId()); + + // check that setSubscribers() does not delete subscribers from other tasks + $task2Subscribers = $this->repository->findBy(['task' => $this->scheduledTask2]); + $this->assertCount(2, $task2Subscribers); + $this->assertInstanceOf(SubscriberEntity::class, $task2Subscribers[0]->getSubscriber()); + $this->assertInstanceOf(SubscriberEntity::class, $task2Subscribers[1]->getSubscriber()); + $this->assertEquals($this->subscriberUnprocessed->getId(), $task2Subscribers[0]->getSubscriber()->getId()); + $this->assertEquals($this->subscriberProcessed->getId(), $task2Subscribers[1]->getSubscriber()->getId()); + } +} diff --git a/mailpoet/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserRendererTest.php b/mailpoet/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserRendererTest.php index cbb7e5447b..1e36532bb8 100644 --- a/mailpoet/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserRendererTest.php +++ b/mailpoet/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserRendererTest.php @@ -4,11 +4,13 @@ namespace MailPoet\Newsletter\ViewInBrowser; use Codeception\Stub\Expected; use MailPoet\Entities\NewsletterEntity; +use MailPoet\Entities\ScheduledTaskEntity; use MailPoet\Entities\SendingQueueEntity; use MailPoet\Entities\SubscriberEntity; use MailPoet\Newsletter\Links\Links; use MailPoet\Newsletter\NewslettersRepository; use MailPoet\Newsletter\Renderer\Renderer; +use MailPoet\Newsletter\Sending\ScheduledTasksRepository; use MailPoet\Newsletter\Sending\SendingQueuesRepository; use MailPoet\Newsletter\Shortcodes\Shortcodes; use MailPoet\Router\Router; @@ -127,6 +129,10 @@ class ViewInBrowserRendererTest extends \MailPoetTest { $queue->setSubscribers([$subscriber->getId()]); $this->sendingTask = $queue->save(); $this->newsletterRepository->refresh($newsletter); + $scheduledTasksRepository = $this->diContainer->get(ScheduledTasksRepository::class); + $scheduledTask = $scheduledTasksRepository->findOneById($this->sendingTask->task()->id); + $this->assertInstanceOf(ScheduledTaskEntity::class, $scheduledTask); + $scheduledTasksRepository->refresh($scheduledTask); $this->newsletter = $newsletter; // create newsletter link associations diff --git a/mailpoet/tests/integration/Statistics/Track/UnsubscribesTest.php b/mailpoet/tests/integration/Statistics/Track/UnsubscribesTest.php index 95450f53fd..cca0102eac 100644 --- a/mailpoet/tests/integration/Statistics/Track/UnsubscribesTest.php +++ b/mailpoet/tests/integration/Statistics/Track/UnsubscribesTest.php @@ -2,8 +2,10 @@ namespace MailPoet\Test\Statistics\Track; +use MailPoet\Entities\ScheduledTaskEntity; use MailPoet\Entities\StatisticsUnsubscribeEntity; use MailPoet\Entities\SubscriberEntity; +use MailPoet\Newsletter\Sending\ScheduledTasksRepository; use MailPoet\Statistics\StatisticsUnsubscribesRepository; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Tasks\Sending as SendingTask; @@ -43,6 +45,11 @@ class UnsubscribesTest extends \MailPoetTest { $queue->setSubscribers([$this->subscriber->getId()]); $queue->updateProcessedSubscribers([$this->subscriber->getId()]); $this->queue = $queue->save(); + $scheduledTasksRepository = $this->diContainer->get(ScheduledTasksRepository::class); + $scheduledTask = $scheduledTasksRepository->findOneById($this->queue->task()->id); + $this->assertInstanceOf(ScheduledTaskEntity::class, $scheduledTask); + $scheduledTasksRepository->refresh($scheduledTask); + // instantiate class $this->unsubscribes = $this->diContainer->get(Unsubscribes::class); $this->statisticsUnsubscribesRepository = $this->diContainer->get(StatisticsUnsubscribesRepository::class); diff --git a/mailpoet/tests/integration/Tasks/SendingTest.php b/mailpoet/tests/integration/Tasks/SendingTest.php index 26564f8f36..56bacb6d9e 100644 --- a/mailpoet/tests/integration/Tasks/SendingTest.php +++ b/mailpoet/tests/integration/Tasks/SendingTest.php @@ -37,13 +37,16 @@ class SendingTest extends \MailPoetTest { /** SubscriberEntity */ private $subscriber2; + /** @var SubscriberFactory */ + private $subscriberFactory; + public function _before() { parent::_before(); $this->newsletterFactory = new NewsletterFactory(); $this->newsletter = $this->newsletterFactory->create(); - $subscriberFactory = new SubscriberFactory(); - $this->subscriber1 = $subscriberFactory->withEmail('subscriber1@test.com')->create(); - $this->subscriber2 = $subscriberFactory->withEmail('subscriber2@test.com')->create(); + $this->subscriberFactory = new SubscriberFactory(); + $this->subscriber1 = $this->subscriberFactory->withEmail('subscriber1@test.com')->create(); + $this->subscriber2 = $this->subscriberFactory->withEmail('subscriber2@test.com')->create(); $this->task = $this->createNewScheduledTask(); $this->queue = $this->createNewSendingQueue([ 'newsletter' => $this->newsletter, @@ -155,10 +158,17 @@ class SendingTest extends \MailPoetTest { } public function testItSetsSubscribers() { - $subscriberIds = [$this->subscriber1->getId(), $this->subscriber2->getId()]; + $subscriber3 = $this->subscriberFactory->withEmail('subscriber3@test.com')->create(); + $subscriber4 = $this->subscriberFactory->withEmail('subscriber4@test.com')->create(); + $subscriber5 = $this->subscriberFactory->withEmail('subscriber5@test.com')->create(); + + $subscriberIds = [$subscriber3->getId(), $subscriber4->getId(), $subscriber5->getId()]; $this->sending->setSubscribers($subscriberIds); + verify($this->sending->getSubscribers())->equals($subscriberIds); verify($this->sending->count_total)->equals(count($subscriberIds)); + verify($this->sending->count_processed)->equals(0); + verify($this->sending->count_to_process)->equals(3); } public function testItRemovesSubscribers() {