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]
This commit is contained in:
committed by
Aschepikov
parent
38b3fbe6fc
commit
e5ab65f28e
@@ -157,10 +157,11 @@ class SendEmailAction implements Action {
|
|||||||
public function run(StepRunArgs $args, StepRunController $controller): void {
|
public function run(StepRunArgs $args, StepRunController $controller): void {
|
||||||
$newsletter = $this->getEmailForStep($args->getStep());
|
$newsletter = $this->getEmailForStep($args->getStep());
|
||||||
$subscriber = $this->getSubscriber($args);
|
$subscriber = $this->getSubscriber($args);
|
||||||
|
$runId = $args->getAutomationRun()->getId();
|
||||||
|
|
||||||
// sync sending status with the automation step
|
// sync sending status with the automation step
|
||||||
if (!$args->isFirstRun()) {
|
if (!$args->isFirstRun()) {
|
||||||
$this->checkSendingStatus($newsletter, $subscriber);
|
$this->checkSendingStatus($newsletter, $subscriber, $runId);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -210,8 +211,8 @@ class SendEmailAction implements Action {
|
|||||||
$this->automationController->enqueueProgress($runId, $stepId);
|
$this->automationController->enqueueProgress($runId, $stepId);
|
||||||
}
|
}
|
||||||
|
|
||||||
private function checkSendingStatus(NewsletterEntity $newsletter, SubscriberEntity $subscriber): void {
|
private function checkSendingStatus(NewsletterEntity $newsletter, SubscriberEntity $subscriber, int $runId): void {
|
||||||
$scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($newsletter, $subscriber);
|
$scheduledTaskSubscriber = $this->automationEmailScheduler->getScheduledTaskSubscriber($newsletter, $subscriber, $runId);
|
||||||
if (!$scheduledTaskSubscriber) {
|
if (!$scheduledTaskSubscriber) {
|
||||||
throw InvalidStateException::create()->withMessage('Email failed to schedule.');
|
throw InvalidStateException::create()->withMessage('Email failed to schedule.');
|
||||||
}
|
}
|
||||||
|
@@ -64,8 +64,8 @@ class AutomationEmailScheduler {
|
|||||||
return $task;
|
return $task;
|
||||||
}
|
}
|
||||||
|
|
||||||
public function getScheduledTaskSubscriber(NewsletterEntity $email, SubscriberEntity $subscriber): ?ScheduledTaskSubscriberEntity {
|
public function getScheduledTaskSubscriber(NewsletterEntity $email, SubscriberEntity $subscriber, int $runId): ?ScheduledTaskSubscriberEntity {
|
||||||
$result = $this->entityManager->createQueryBuilder()
|
$results = $this->entityManager->createQueryBuilder()
|
||||||
->select('sts')
|
->select('sts')
|
||||||
->from(ScheduledTaskSubscriberEntity::class, 'sts')
|
->from(ScheduledTaskSubscriberEntity::class, 'sts')
|
||||||
->join('sts.task', 'st')
|
->join('sts.task', 'st')
|
||||||
@@ -75,7 +75,19 @@ class AutomationEmailScheduler {
|
|||||||
->setParameter('newsletter', $email)
|
->setParameter('newsletter', $email)
|
||||||
->setParameter('subscriber', $subscriber)
|
->setParameter('subscriber', $subscriber)
|
||||||
->getQuery()
|
->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;
|
return $result instanceof ScheduledTaskSubscriberEntity ? $result : null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@@ -174,6 +174,7 @@ class SendEmailActionTest extends \MailPoetTest {
|
|||||||
|
|
||||||
// create scheduled task & subscriber
|
// create scheduled task & subscriber
|
||||||
$scheduledTask = (new ScheduledTask())->create('sending', ScheduledTaskEntity::STATUS_SCHEDULED);
|
$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);
|
(new SendingQueue())->create($scheduledTask, $email);
|
||||||
$scheduledTaskSubscriber = (new ScheduledTaskSubscriber())->createFailed($scheduledTask, $subscriber, 'Test error');
|
$scheduledTaskSubscriber = (new ScheduledTaskSubscriber())->createFailed($scheduledTask, $subscriber, 'Test error');
|
||||||
|
|
||||||
|
@@ -0,0 +1,60 @@
|
|||||||
|
<?php declare(strict_types = 1);
|
||||||
|
|
||||||
|
namespace MailPoet\Newsletter\Scheduler;
|
||||||
|
|
||||||
|
use MailPoet\Entities\NewsletterEntity;
|
||||||
|
use MailPoet\Entities\ScheduledTaskEntity;
|
||||||
|
use MailPoet\Entities\ScheduledTaskSubscriberEntity;
|
||||||
|
use MailPoet\Entities\SubscriberEntity;
|
||||||
|
use MailPoet\Test\DataFactories\Newsletter;
|
||||||
|
use MailPoet\Test\DataFactories\Subscriber;
|
||||||
|
|
||||||
|
class AutomationEmailSchedulerTest extends \MailPoetTest {
|
||||||
|
|
||||||
|
/** @var AutomationEmailScheduler */
|
||||||
|
private $automationEmailScheduler;
|
||||||
|
|
||||||
|
/** @var NewsletterEntity */
|
||||||
|
private $newsletter;
|
||||||
|
|
||||||
|
/** @var SubscriberEntity */
|
||||||
|
private $subscriber;
|
||||||
|
|
||||||
|
public function _before() {
|
||||||
|
parent::_before();
|
||||||
|
$this->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]];
|
||||||
|
}
|
||||||
|
}
|
Reference in New Issue
Block a user