diff --git a/lib/Cron/CronWorkerRunner.php b/lib/Cron/CronWorkerRunner.php index 0ee10f7b94..262f75fd46 100644 --- a/lib/Cron/CronWorkerRunner.php +++ b/lib/Cron/CronWorkerRunner.php @@ -3,6 +3,7 @@ namespace MailPoet\Cron; use MailPoet\Models\ScheduledTask; +use MailPoet\Newsletter\Sending\ScheduledTasksRepository; use MailPoet\WP\Functions as WPFunctions; use MailPoetVendor\Carbon\Carbon; @@ -23,15 +24,20 @@ class CronWorkerRunner { /** @var WPFunctions */ private $wp; + /** @var ScheduledTasksRepository */ + private $scheduledTasksRepository; + public function __construct( CronHelper $cronHelper, CronWorkerScheduler $cronWorkerScheduler, - WPFunctions $wp + WPFunctions $wp, + ScheduledTasksRepository $scheduledTasksRepository ) { $this->timer = microtime(true); $this->cronHelper = $cronHelper; $this->cronWorkerScheduler = $cronWorkerScheduler; $this->wp = $wp; + $this->scheduledTasksRepository = $scheduledTasksRepository; } public function run(CronWorkerInterface $worker) { @@ -42,7 +48,8 @@ class CronWorkerRunner { if (!$worker->checkProcessingRequirements()) { foreach (array_merge($dueTasks, $runningTasks) as $task) { - $task->delete(); + $this->scheduledTasksRepository->remove($task); + $this->scheduledTasksRepository->flush(); } return false; } @@ -75,11 +82,11 @@ class CronWorkerRunner { } private function getDueTasks(CronWorkerInterface $worker) { - return ScheduledTask::findDueByType($worker->getTaskType(), self::TASK_BATCH_SIZE); + return $this->scheduledTasksRepository->findDueByType($worker->getTaskType(), self::TASK_BATCH_SIZE); } private function getRunningTasks(CronWorkerInterface $worker) { - return ScheduledTask::findRunningByType($worker->getTaskType(), self::TASK_BATCH_SIZE); + return $this->scheduledTasksRepository->findRunningByType($worker->getTaskType(), self::TASK_BATCH_SIZE); } private function prepareTask(CronWorkerInterface $worker, ScheduledTask $task) { diff --git a/lib/DI/ContainerConfigurator.php b/lib/DI/ContainerConfigurator.php index 11e75e13a3..651f23e5da 100644 --- a/lib/DI/ContainerConfigurator.php +++ b/lib/DI/ContainerConfigurator.php @@ -360,7 +360,7 @@ class ContainerConfigurator implements IContainerConfigurator { $container->autowire(\MailPoet\Newsletter\Statistics\NewsletterStatisticsRepository::class)->setPublic(true); $container->autowire(\MailPoet\Newsletter\Scheduler\WelcomeScheduler::class)->setPublic(true); $container->autowire(\MailPoet\Newsletter\Scheduler\PostNotificationScheduler::class)->setPublic(true); - $container->autowire(\MailPoet\Newsletter\Sending\ScheduledTasksRepository::class); + $container->autowire(\MailPoet\Newsletter\Sending\ScheduledTasksRepository::class)->setPublic(true); $container->autowire(\MailPoet\Newsletter\Sending\ScheduledTaskSubscribersRepository::class)->setPublic(true); $container->autowire(\MailPoet\Newsletter\Sending\SendingQueuesRepository::class)->setPublic(true); $container->autowire(\MailPoet\Newsletter\ViewInBrowser\ViewInBrowserController::class)->setPublic(true); diff --git a/lib/Models/ScheduledTask.php b/lib/Models/ScheduledTask.php index 8ec936cabf..aec9d454f1 100644 --- a/lib/Models/ScheduledTask.php +++ b/lib/Models/ScheduledTask.php @@ -164,14 +164,6 @@ class ScheduledTask extends Model { ->findOne() ?: null; } - public static function findDueByType($type, $limit = null) { - return self::findByTypeAndStatus($type, ScheduledTask::STATUS_SCHEDULED, $limit); - } - - public static function findRunningByType($type, $limit = null) { - return self::findByTypeAndStatus($type, null, $limit); - } - public static function findFutureScheduledByType($type, $limit = null) { return self::findByTypeAndStatus($type, ScheduledTask::STATUS_SCHEDULED, $limit, true); } diff --git a/lib/Newsletter/Sending/ScheduledTasksRepository.php b/lib/Newsletter/Sending/ScheduledTasksRepository.php index 9f313b7635..025535ea1e 100644 --- a/lib/Newsletter/Sending/ScheduledTasksRepository.php +++ b/lib/Newsletter/Sending/ScheduledTasksRepository.php @@ -7,6 +7,8 @@ use MailPoet\Entities\NewsletterEntity; use MailPoet\Entities\ScheduledTaskEntity; use MailPoet\Entities\ScheduledTaskSubscriberEntity; use MailPoet\Entities\SendingQueueEntity; +use MailPoet\WP\Functions as WPFunctions; +use MailPoetVendor\Carbon\Carbon; use MailPoetVendor\Doctrine\ORM\Query\Expr\Join; /** @@ -87,6 +89,45 @@ class ScheduledTasksRepository extends Repository { ->getOneOrNullResult(); } + public function findDueByType($type, $limit = null) { + return $this->findByTypeAndStatus($type, ScheduledTaskEntity::STATUS_SCHEDULED, $limit); + } + + public function findRunningByType($type, $limit = null) { + return $this->findByTypeAndStatus($type, null, $limit); + } + + protected function findByTypeAndStatus($type, $status, $limit = null, $future = false) { + $queryBuilder = $this->doctrineRepository->createQueryBuilder('st') + ->select('st') + ->where('st.type = :type') + ->setParameter('type', $type) + ->andWhere('st.deletedAt IS NULL'); + + if (is_null($status)) { + $queryBuilder->andWhere('st.status IS NULL'); + } else { + $queryBuilder + ->andWhere('st.status = :status') + ->setParameter('status', $status); + } + + if ($future) { + $queryBuilder->andWhere('st.scheduledAt >= :now'); + } else { + $queryBuilder->andWhere('st.scheduledAt <= :now'); + } + + $now = Carbon::createFromTimestamp(WPFunctions::get()->currentTime('timestamp')); + $queryBuilder->setParameter('now', $now); + + if ($limit) { + $queryBuilder->setMaxResults($limit); + } + + return $queryBuilder->getQuery()->getResult(); + } + protected function getEntityClassName() { return ScheduledTaskEntity::class; } diff --git a/tests/integration/Models/ScheduledTaskTest.php b/tests/integration/Models/ScheduledTaskTest.php index e241a10a04..614ba7cb6f 100644 --- a/tests/integration/Models/ScheduledTaskTest.php +++ b/tests/integration/Models/ScheduledTaskTest.php @@ -181,74 +181,6 @@ class ScheduledTaskTest extends \MailPoetTest { expect($timeout)->equals(ScheduledTask::MAX_RESCHEDULE_TIMEOUT); } - public function testItCanGetDueTasks() { - // due (scheduled in past) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => ScheduledTask::STATUS_SCHEDULED, - 'scheduled_at' => Carbon::now()->subDay(), - ]); - - // deleted (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => ScheduledTask::STATUS_SCHEDULED, - 'scheduled_at' => Carbon::now()->subDay(), - 'deleted_at' => Carbon::now(), - ]); - - // scheduled in future (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => ScheduledTask::STATUS_SCHEDULED, - 'scheduled_at' => Carbon::now()->addDay(), - ]); - - // wrong status (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => null, - 'scheduled_at' => Carbon::now()->subDay(), - ]); - - $tasks = ScheduledTask::findDueByType('test', 10); - expect($tasks)->count(1); - } - - public function testItCanGetRunningTasks() { - // running (scheduled in past) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => null, - 'scheduled_at' => Carbon::now()->subDay(), - ]); - - // deleted (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => null, - 'scheduled_at' => Carbon::now()->subDay(), - 'deleted_at' => Carbon::now(), - ]); - - // scheduled in future (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => null, - 'scheduled_at' => Carbon::now()->addDay(), - ]); - - // wrong status (should not be fetched) - ScheduledTask::createOrUpdate([ - 'type' => 'test', - 'status' => ScheduledTask::STATUS_COMPLETED, - 'scheduled_at' => Carbon::now()->subDay(), - ]); - - $tasks = ScheduledTask::findRunningByType('test', 10); - expect($tasks)->count(1); - } - public function testItCanGetCompletedTasks() { // completed (scheduled in past) ScheduledTask::createOrUpdate([ diff --git a/tests/integration/Newsletter/Sending/ScheduledTasksRepositoryTest.php b/tests/integration/Newsletter/Sending/ScheduledTasksRepositoryTest.php new file mode 100644 index 0000000000..044475b76b --- /dev/null +++ b/tests/integration/Newsletter/Sending/ScheduledTasksRepositoryTest.php @@ -0,0 +1,60 @@ +cleanup(); + $this->repository = $this->diContainer->get(ScheduledTasksRepository::class); + } + + public function testItCanGetDueTasks() { + $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_SCHEDULED, Carbon::now()->subDay(), Carbon::now()); // deleted (should not be fetched) + $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_SCHEDULED, Carbon::now()->addDay()); // scheduled in future (should not be fetched) + $this->createScheduledTask('test', '', Carbon::now()->subDay()); // wrong status (should not be fetched) + $expectedResult[] = $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_SCHEDULED, Carbon::now()->subDay()); // due (scheduled in past) + $expectedResult[] = $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_SCHEDULED, Carbon::now()->subDay()); // due (scheduled in past) + $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_SCHEDULED, Carbon::now()->subDay()); // due (scheduled in past) + + $tasks = $this->repository->findDueByType('test', 2); + $this->assertCount(2, $tasks); + $this->assertSame($expectedResult, $tasks); + } + + public function testItCanGetRunningTasks() { + $expectedResult[] = $this->createScheduledTask('test', null, Carbon::now()->subDay()); // running (scheduled in past) + $this->createScheduledTask('test', null, Carbon::now()->subDay(), Carbon::now()); // deleted (should not be fetched) + $this->createScheduledTask('test', null, Carbon::now()->addDay()); // scheduled in future (should not be fetched) + $this->createScheduledTask('test', ScheduledTaskEntity::STATUS_COMPLETED, Carbon::now()->subDay()); // wrong status (should not be fetched) + + $tasks = $this->repository->findRunningByType('test', 10); + $this->assertSame($expectedResult, $tasks); + } + + public function cleanup() { + $this->truncateEntity(ScheduledTaskEntity::class); + } + + private function createScheduledTask(string $type, ?string $status, \DateTimeInterface $scheduledAt, \DateTimeInterface $deletedAt = null) { + $task = new ScheduledTaskEntity(); + $task->setType($type); + $task->setStatus($status); + $task->setScheduledAt($scheduledAt); + + if ($deletedAt) { + $task->setDeletedAt($deletedAt); + } + + $this->entityManager->persist($task); + $this->entityManager->flush(); + + return $task; + } +}