Reuse StepRunLogger to avoid overwrites

The SendEmailAction step needs to log arbitrary data to allow
retry-runs, through a StepRunLogger. We previously instanciated a logger
from the action, but this data will be overwritten by the handler right
after the action is run. This is because the handler holds a reference
to a logger already, and will overwrite any data the action tries to
write.

To work around this issue, the step handler now passes its logger
instance to the step's controller, allowing safe access from
SendEmailAction.

[MAILPOET-6176]

mailpoet/lib/Automation/Engine/Control/StepHandler.php

@@ -148,7 +148,7 @@ class StepHandler {
     }, $subjectEntries));
 
     $step->validate($validationArgs);
-    $step->run($args, $this->stepRunControllerFactory->createController($args));
+    $step->run($args, $this->stepRunControllerFactory->createController($args, $logger));
 
     // check if run is not completed by now (e.g., one of if/else branches is empty)
     $automationRun = $this->automationRunStorage->getAutomationRun($runId);

mailpoet/lib/Automation/Engine/Control/StepRunController.php

@@ -2,6 +2,7 @@
 
 namespace MailPoet\Automation\Engine\Control;
 
+use MailPoet\Automation\Engine\Control\StepRunLogger;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 
 class StepRunController {
@@ -11,12 +12,17 @@ class StepRunController {
   /** @var StepRunArgs */
   private $stepRunArgs;
 
+  /** @var StepRunLogger */
+  private $stepRunLogger;
+
   public function __construct(
     StepScheduler $stepScheduler,
-    StepRunArgs $stepRunArgs
+    StepRunArgs $stepRunArgs,
+    StepRunLogger $stepRunLogger
   ) {
     $this->stepScheduler = $stepScheduler;
     $this->stepRunArgs = $stepRunArgs;
+    $this->stepRunLogger = $stepRunLogger;
   }
 
   public function scheduleProgress(int $timestamp = null): int {
@@ -34,4 +40,8 @@ class StepRunController {
   public function hasScheduledNextStep(): bool {
     return $this->stepScheduler->hasScheduledNextStep($this->stepRunArgs);
   }
+
+  public function getRunLog(): StepRunLogger {
+    return $this->stepRunLogger;
+  }
 }

mailpoet/lib/Automation/Engine/Control/StepRunControllerFactory.php

@@ -2,6 +2,7 @@
 
 namespace MailPoet\Automation\Engine\Control;
 
+use MailPoet\Automation\Engine\Control\StepRunLogger;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 
 class StepRunControllerFactory {
@@ -14,7 +15,7 @@ class StepRunControllerFactory {
     $this->stepScheduler = $stepScheduler;
   }
 
-  public function createController(StepRunArgs $args): StepRunController {
+  public function createController(StepRunArgs $args, StepRunLogger $logger): StepRunController {
-    return new StepRunController($this->stepScheduler, $args);
+    return new StepRunController($this->stepScheduler, $args, $logger);
   }
 }

mailpoet/lib/Automation/Engine/Control/StepRunLogger.php

@@ -100,7 +100,15 @@ class StepRunLogger {
     $this->automationRunLogStorage->updateAutomationRunLog($log);
   }
 
-  private function getLog(): AutomationRunLog {
+  public function saveLogData(array $data): void {
+    $log = $this->getLog();
+    foreach ($data as $key => $value) {
+      $log->setData($key, $value);
+    }
+    $this->automationRunLogStorage->updateAutomationRunLog($log);
+  }
+
+  public function getLog(): AutomationRunLog {
     if (!$this->log) {
       $this->log = $this->automationRunLogStorage->getAutomationRunLogByRunAndStepId($this->runId, $this->stepId);
     }

mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php

@@ -6,14 +6,12 @@ use MailPoet\AutomaticEmails\WooCommerce\Events\AbandonedCart;
 use MailPoet\Automation\Engine\Control\AutomationController;
 use MailPoet\Automation\Engine\Control\StepRunController;
 use MailPoet\Automation\Engine\Data\Automation;
-use MailPoet\Automation\Engine\Data\AutomationRunLog;
 use MailPoet\Automation\Engine\Data\Step;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Exceptions\NotFoundException;
 use MailPoet\Automation\Engine\Integration\Action;
 use MailPoet\Automation\Engine\Integration\ValidationException;
-use MailPoet\Automation\Engine\Storage\AutomationRunLogStorage;
 use MailPoet\Automation\Integrations\MailPoet\Payloads\SegmentPayload;
 use MailPoet\Automation\Integrations\MailPoet\Payloads\SubscriberPayload;
 use MailPoet\Automation\Integrations\WooCommerce\Payloads\AbandonedCartPayload;
@@ -194,7 +192,7 @@ class SendEmailAction implements Action {
       }
 
       if ($this->isOptInRequired($newsletter, $subscriber)) {
-        $this->updateRunLogData($args, [self::WAIT_OPTIN => 1]);
+        $controller->getRunLog()->saveLogData([self::WAIT_OPTIN => 1]);
         $this->rerunLater($args->getRunNumber(), $controller, $newsletter, $subscriber);
         return;
       }
@@ -202,7 +200,8 @@ class SendEmailAction implements Action {
       $this->scheduleEmail($args, $newsletter, $subscriber);
     } else {
       // Re-running for opt-in?
-      $state = $this->getRunLogData($args);
+      $state = $this->getRunLogData($controller);
+
       if (array_key_exists(self::WAIT_OPTIN, $state) && $state[self::WAIT_OPTIN] === 1) {
         if ($this->isOptInRequired($newsletter, $subscriber)) {
           $this->rerunLater($args->getRunNumber(), $controller, $newsletter, $subscriber);
@@ -210,7 +209,7 @@ class SendEmailAction implements Action {
         }
 
         // Subscriber is now confirmed, so we can schedule an email.
-        $this->updateRunLogData($args, [
+        $controller->getRunLog()->saveLogData([
           self::WAIT_OPTIN => 0,
           self::OPTIN_RETRIES => $args->getRunNumber(),
         ]);
@@ -227,7 +226,7 @@ class SendEmailAction implements Action {
     // At this point, we're re-running to check sending status. We need
     // to offset opt-in reruns count from sending reruns.
     $runNumber = $args->getRunNumber();
-    $state = $state ?? $this->getRunLogData($args);
+    $state = $state ?? $this->getRunLogData($controller);
     $optinRetryCount = $state[self::OPTIN_RETRIES] ?? 0;
     $runNumber -= $optinRetryCount;
     $this->rerunLater($runNumber, $controller, $newsletter, $subscriber);
@@ -242,35 +241,8 @@ class SendEmailAction implements Action {
     }
   }
 
-  private function updateRunLogData(StepRunArgs $args, array $data): void {
+  private function getRunLogData(StepRunController $controller): array {
-    $run = $args->getAutomationRun();
+    $runLog = $controller->getRunLog()->getLog();
-    $step = $args->getStep();
-    $storage = new AutomationRunLogStorage();
-
-    $runLog = $storage->getAutomationRunLogByRunAndStepId($run->getId(), $step->getId());
-    if ($runLog === null) {
-      $runLog = new AutomationRunLog($run->getId(), $step->getId(), $step->getType());
-    }
-    foreach ($data as $key => $value) {
-      $runLog->setData($key, $value);
-    }
-
-    if ($runLog->getId() !== null) {
-      $storage->updateAutomationRunLog($runLog);
-    } else {
-      $storage->createAutomationRunLog($runLog);
-    }
-  }
-
-  private function getRunLogData(StepRunArgs $args): array {
-    $run = $args->getAutomationRun();
-    $step = $args->getStep();
-    $storage = new AutomationRunLogStorage();
-    $runLog = $storage->getAutomationRunLogByRunAndStepId($run->getId(), $step->getId());
-
-    if ($runLog === null) {
-      return [];
-    }
     return $runLog->getData();
   }
 

mailpoet/tests/integration/Automation/Integrations/Core/Actions/IfElseActionTest.php

@@ -5,6 +5,7 @@ namespace MailPoet\Test\Automation\Integrations\Core\Actions;
 use ActionScheduler_Action;
 use ActionScheduler_Store;
 use MailPoet\Automation\Engine\Control\StepRunControllerFactory;
+use MailPoet\Automation\Engine\Control\StepRunLoggerFactory;
 use MailPoet\Automation\Engine\Data\Automation;
 use MailPoet\Automation\Engine\Data\AutomationRun;
 use MailPoet\Automation\Engine\Data\Field;
@@ -96,7 +97,8 @@ class IfElseActionTest extends MailPoetTest {
     // run
     $run = new AutomationRun(1, 1, 'trigger-key', [$subject], 1);
     $args = new StepRunArgs($automation, $run, $step, [new SubjectEntry($this->subscriberSubject, $subject)], 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
     $this->action->run($args, $controller);
 
     // check

mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php

@@ -7,6 +7,7 @@ use MailPoet\Automation\Engine\Builder\UpdateAutomationController;
 use MailPoet\Automation\Engine\Control\ActionScheduler;
 use MailPoet\Automation\Engine\Control\AutomationController;
 use MailPoet\Automation\Engine\Control\StepRunControllerFactory;
+use MailPoet\Automation\Engine\Control\StepRunLoggerFactory;
 use MailPoet\Automation\Engine\Data\Automation;
 use MailPoet\Automation\Engine\Data\AutomationRun;
 use MailPoet\Automation\Engine\Data\NextStep;
@@ -130,7 +131,8 @@ class SendEmailActionTest extends \MailPoetTest {
 
     // first run
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
     $this->action->run($args, $controller);
 
     // scheduled task
@@ -151,7 +153,8 @@ class SendEmailActionTest extends \MailPoetTest {
 
     // progress — won't throw an exception when the email was sent (= step will be completed)
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 2);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 2);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
     $this->action->run($args, $controller);
   }
 
@@ -170,7 +173,8 @@ class SendEmailActionTest extends \MailPoetTest {
 
     // progress run step args
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 2);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 2);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     // email was never scheduled
     $this->assertThrowsExceptionWithMessage(
@@ -265,7 +269,7 @@ class SendEmailActionTest extends \MailPoetTest {
 
     $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', ['email_id' => $email->getId()], []);
     $automation = new Automation('some-automation', [$step->getId() => $step], new \WP_User());
-    $run = new AutomationRun(1, 1, 'trigger-key', $subjects);
+    $run = new AutomationRun(1, 1, 'trigger-key', $subjects, 1);
 
     $scheduled = $this->scheduledTasksRepository->findByNewsletterAndSubscriberId($email, (int)$subscriber->getId());
     verify($scheduled)->arrayCount(0);
@@ -273,7 +277,8 @@ class SendEmailActionTest extends \MailPoetTest {
     $this->segmentsRepository->bulkDelete([$segment->getId()]);
     $action = ContainerWrapper::getInstance()->get(SendEmailAction::class);
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     try {
       $action->run($args, $controller);
@@ -296,7 +301,7 @@ class SendEmailActionTest extends \MailPoetTest {
 
     $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', ['email_id' => $email->getId()], []);
     $automation = new Automation('some-automation', [$step->getId() => $step], new \WP_User());
-    $run = new AutomationRun(1, 1, 'trigger-key', $subjects);
+    $run = new AutomationRun(1, 1, 'trigger-key', $subjects, 1);
 
     $scheduled = $this->scheduledTasksRepository->findByNewsletterAndSubscriberId($email, (int)$subscriber->getId());
     verify($scheduled)->arrayCount(0);
@@ -304,7 +309,8 @@ class SendEmailActionTest extends \MailPoetTest {
     $this->subscribersRepository->bulkDelete([$subscriber->getId()]);
     $action = ContainerWrapper::getInstance()->get(SendEmailAction::class);
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     try {
       $action->run($args, $controller);
@@ -336,7 +342,7 @@ class SendEmailActionTest extends \MailPoetTest {
 
       $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', ['email_id' => $email->getId()], []);
       $automation = new Automation('some-automation', [$step->getId() => $step], new \WP_User());
-      $run = new AutomationRun(1, 1, 'trigger-key', $subjects);
+      $run = new AutomationRun(1, 1, 'trigger-key', $subjects, 1);
 
       $scheduled = $this->scheduledTasksRepository->findByNewsletterAndSubscriberId($email, (int)$subscriber->getId());
       verify($scheduled)->arrayCount(0);
@@ -344,7 +350,8 @@ class SendEmailActionTest extends \MailPoetTest {
       $this->subscribersRepository->bulkDelete([$subscriber->getId()]);
       $action = ContainerWrapper::getInstance()->get(SendEmailAction::class);
       $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-      $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+      $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+      $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
       try {
         $action->run($args, $controller);
@@ -381,7 +388,9 @@ class SendEmailActionTest extends \MailPoetTest {
 
     // Prepare action run.
     $args = new StepRunArgs($automation, $run, end($steps), $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $step = end($steps);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     // First run: with no opt-in => schedule a retry
     $actionScheduler = $this->diContainer->get(ActionScheduler::class);
@@ -425,7 +434,9 @@ class SendEmailActionTest extends \MailPoetTest {
 
     // Prepare action run.
     $args = new StepRunArgs($automation, $run, end($steps), $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $step = end($steps);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     // First run: with no opt-in => schedule a retry
     $actionScheduler = $this->diContainer->get(ActionScheduler::class);
@@ -455,14 +466,15 @@ class SendEmailActionTest extends \MailPoetTest {
 
     $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', ['email_id' => $email->getId()], []);
     $automation = new Automation('some-automation', [$step->getId() => $step], new \WP_User());
-    $run = new AutomationRun(1, 1, 'trigger-key', $subjects);
+    $run = new AutomationRun(1, 1, 'trigger-key', $subjects, 1);
 
     $scheduled = $this->scheduledTasksRepository->findByNewsletterAndSubscriberId($email, (int)$subscriber->getId());
     verify($scheduled)->arrayCount(0);
 
     $action = ContainerWrapper::getInstance()->get(SendEmailAction::class);
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     try {
       $action->run($args, $controller);
@@ -502,7 +514,8 @@ class SendEmailActionTest extends \MailPoetTest {
 
     $action = ContainerWrapper::getInstance()->get(SendEmailAction::class);
     $args = new StepRunArgs($automation, $run, $step, $this->getSubjectEntries($subjects), 1);
-    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args);
+    $logger = $this->diContainer->get(StepRunLoggerFactory::class)->createLogger($run->getId(), $step->getId(), $step->getType(), 1);
+    $controller = $this->diContainer->get(StepRunControllerFactory::class)->createController($args, $logger);
 
     $action->run($args, $controller);
     $scheduled = $this->scheduledTasksRepository->findByNewsletterAndSubscriberId($email, (int)$subscriber->getId());