From e5ab65f28eabcdbc1f3c30b70e5fe10aa98acb1e Mon Sep 17 00:00:00 2001 From: Rostislav Wolny Date: Wed, 24 Jul 2024 12:52:35 +0200 Subject: [PATCH] Fix send action checkSendingStatus to support multiple emails per subscriber. It is possible that one email (e.g., purchase in category) is sent multiple times to the same subscriber. AutomationEmailScheduler::getScheduledTaskSubscriber was selecting the task based on subscriber and newsletter. In the case of multiple emails sent to one subscriber, the method failed to pick ScheduledTaskSubsrciberEntity because the query was fetching multiple results, but getOneOrNullResult expects only one result. This commit fixes it by adding additional filtering by $runId to get the ScheduledTaskSubsriberEntity associated with the correct run. I did the filtering in PHP because an alternative would be using LIKE %% in the query. The meta column is text. [MAILPOET-6155] --- .../MailPoet/Actions/SendEmailAction.php | 7 ++- .../Scheduler/AutomationEmailScheduler.php | 18 +++++- .../MailPoet/Actions/SendEmailActionTest.php | 1 + .../AutomationEmailSchedulerTest.php | 60 +++++++++++++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 mailpoet/tests/integration/Newsletter/Scheduler/AutomationEmailSchedulerTest.php diff --git a/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php b/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php index cb8d19118c..018f694ce1 100644 --- a/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php +++ b/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php @@ -157,10 +157,11 @@ class SendEmailAction implements Action { public function run(StepRunArgs $args, StepRunController $controller): void { $newsletter = $this->getEmailForStep($args->getStep()); $subscriber = $this->getSubscriber($args); + $runId = $args->getAutomationRun()->getId(); // sync sending status with the automation step if (!$args->isFirstRun()) { - $this->checkSendingStatus($newsletter, $subscriber); + $this->checkSendingStatus($newsletter, $subscriber, $runId); return; } @@ -210,8 +211,8 @@ class SendEmailAction implements Action { $this->automationController->enqueueProgress($runId, $stepId); } - private function checkSendingStatus(NewsletterEntity $newsletter, SubscriberEntity $subscriber): void { - $scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($newsletter, $subscriber); + private function checkSendingStatus(NewsletterEntity $newsletter, SubscriberEntity $subscriber, int $runId): void { + $scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($newsletter, $subscriber, $runId); if (!$scheduledTaskSubscriber) { throw InvalidStateException::create()->withMessage('Email failed to schedule.'); } diff --git a/mailpoet/lib/Newsletter/Scheduler/AutomationEmailScheduler.php b/mailpoet/lib/Newsletter/Scheduler/AutomationEmailScheduler.php index 14b388474e..3a5701b90e 100644 --- a/mailpoet/lib/Newsletter/Scheduler/AutomationEmailScheduler.php +++ b/mailpoet/lib/Newsletter/Scheduler/AutomationEmailScheduler.php @@ -64,8 +64,8 @@ class AutomationEmailScheduler { return $task; } - public function getScheduledTaskSubscriber(NewsletterEntity $email, SubscriberEntity $subscriber): ?ScheduledTaskSubscriberEntity { - $result = $this->entityManager->createQueryBuilder() + public function getScheduledTaskSubscriber(NewsletterEntity $email, SubscriberEntity $subscriber, int $runId): ?ScheduledTaskSubscriberEntity { + $results = $this->entityManager->createQueryBuilder() ->select('sts') ->from(ScheduledTaskSubscriberEntity::class, 'sts') ->join('sts.task', 'st') @@ -75,7 +75,19 @@ class AutomationEmailScheduler { ->setParameter('newsletter', $email) ->setParameter('subscriber', $subscriber) ->getQuery() - ->getOneOrNullResult(); + ->getResult(); + $result = null; + foreach ($results as $scheduledTaskSubscriber) { + $task = $scheduledTaskSubscriber->getTask(); + if (!$task instanceof ScheduledTaskEntity) { + continue; + } + $meta = $task->getMeta(); + if (($meta['automation']['run_id'] ?? null) === $runId) { + $result = $scheduledTaskSubscriber; + break; + } + } return $result instanceof ScheduledTaskSubscriberEntity ? $result : null; } diff --git a/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php b/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php index e0d0d9d6a2..2efc7a3a2e 100644 --- a/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php +++ b/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php @@ -174,6 +174,7 @@ class SendEmailActionTest extends \MailPoetTest { // create scheduled task & subscriber $scheduledTask = (new ScheduledTask())->create('sending', ScheduledTaskEntity::STATUS_SCHEDULED); + $scheduledTask->setMeta(['automation' => ['run_id' => $run->getId(), 'step_id' => $step->getId(), 'run_number' => 1]]); (new SendingQueue())->create($scheduledTask, $email); $scheduledTaskSubscriber = (new ScheduledTaskSubscriber())->createFailed($scheduledTask, $subscriber, 'Test error'); diff --git a/mailpoet/tests/integration/Newsletter/Scheduler/AutomationEmailSchedulerTest.php b/mailpoet/tests/integration/Newsletter/Scheduler/AutomationEmailSchedulerTest.php new file mode 100644 index 0000000000..f27be8190e --- /dev/null +++ b/mailpoet/tests/integration/Newsletter/Scheduler/AutomationEmailSchedulerTest.php @@ -0,0 +1,60 @@ +automationEmailScheduler = $this->diContainer->get(AutomationEmailScheduler::class); + $this->newsletter = (new Newsletter())->withType(NewsletterEntity::TYPE_AUTOMATION)->create(); + $this->subscriber = (new Subscriber())->create(); + } + + public function testGetScheduledTaskSubscriberReturnsNullWhenNonExists() { + $scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($this->newsletter, $this->subscriber, 1); + verify($scheduledTaskSubscriber)->null(); + } + + public function testGetScheduledTaskSubscriberReturnsNullForUnknownRunId() { + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, []); + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, $this->getMeta(1)); + + $scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($this->newsletter, $this->subscriber, 2); + verify($scheduledTaskSubscriber)->null(); + } + + public function testGetScheduledTaskSubscriberReturnsProperEntityForRun() { + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, []); + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, $this->getMeta(1)); + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, $this->getMeta(2)); + $this->automationEmailScheduler->createSendingTask($this->newsletter, $this->subscriber, $this->getMeta(3)); + + $scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($this->newsletter, $this->subscriber, 1); + $this->assertInstanceOf(ScheduledTaskSubscriberEntity::class, $scheduledTaskSubscriber); + $task = $scheduledTaskSubscriber->getTask(); + $this->assertInstanceOf(ScheduledTaskEntity::class, $task); + $meta = $task->getMeta(); + verify($meta['automation']['run_id'] ?? null)->equals(1); + } + + private function getMeta(int $runId) { + return ['automation' => ['run_id' => $runId]]; + } +}