Add error messages to action validation, use step validation args

[MAILPOET-4659]

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

@@ -6,6 +6,7 @@ use Exception;
 use MailPoet\Automation\Engine\Control\Steps\ActionStepRunner;
 use MailPoet\Automation\Engine\Data\Step;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Data\SubjectEntry;
 use MailPoet\Automation\Engine\Data\WorkflowRun;
 use MailPoet\Automation\Engine\Data\WorkflowRunLog;
@@ -135,7 +136,10 @@ class StepHandler {
         $requiredSubjects = $step instanceof Action ? $step->getSubjectKeys() : [];
         $subjectEntries = $this->getSubjectEntries($workflowRun, $requiredSubjects);
         $args = new StepRunArgs($workflow, $workflowRun, $step, $subjectEntries);
-        $this->stepRunners[$stepType]->run($args);
+        $validationArgs = new StepValidationArgs($workflow, $step, array_map(function (SubjectEntry $entry) {
+          return $entry->getSubject();
+        }, $subjectEntries));
+        $this->stepRunners[$stepType]->run($args, $validationArgs);
         $log->markCompletedSuccessfully();
       } catch (Throwable $e) {
         $log->markFailed();

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

@@ -3,7 +3,8 @@
 namespace MailPoet\Automation\Engine\Control;
 
 use MailPoet\Automation\Engine\Data\StepRunArgs;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 
 interface StepRunner {
-  public function run(StepRunArgs $args): void;
+  public function run(StepRunArgs $runArgs, StepValidationArgs $validationArgs): void;
 }

mailpoet/lib/Automation/Engine/Control/Steps/ActionStepRunner.php

@@ -4,6 +4,7 @@ namespace MailPoet\Automation\Engine\Control\Steps;
 
 use MailPoet\Automation\Engine\Control\StepRunner;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Exceptions\InvalidStateException;
 use MailPoet\Automation\Engine\Registry;
 
@@ -17,14 +18,13 @@ class ActionStepRunner implements StepRunner {
     $this->registry = $registry;
   }
 
-  public function run(StepRunArgs $args): void {
-    $action = $this->registry->getAction($args->getStep()->getKey());
+  public function run(StepRunArgs $runArgs, StepValidationArgs $validationArgs): void {
+    $action = $this->registry->getAction($runArgs->getStep()->getKey());
     if (!$action) {
       throw new InvalidStateException();
     }
-    if (!$action->isValid($args->getWorkflowRun()->getSubjects(), $args->getStep(), $args->getWorkflow())) {
-      throw new InvalidStateException();
-    }
-    $action->run($args);
+
+    $action->validate($validationArgs);
+    $action->run($runArgs);
   }
 }

mailpoet/lib/Automation/Engine/Integration/Action.php

@@ -2,12 +2,11 @@
 
 namespace MailPoet\Automation\Engine\Integration;
 
-use MailPoet\Automation\Engine\Data\Step as StepData;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
-use MailPoet\Automation\Engine\Data\Workflow;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 
 interface Action extends Step {
-  public function isValid(array $subjects, StepData $step, Workflow $workflow): bool;
-
   public function run(StepRunArgs $args): void;
+
+  public function validate(StepValidationArgs $args): void;
 }

mailpoet/lib/Automation/Engine/Integration/ValidationException.php

@@ -0,0 +1,8 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Automation\Engine\Integration;
+
+use MailPoet\Automation\Engine\Exceptions\UnexpectedValueException;
+
+class ValidationException extends UnexpectedValueException {
+}

mailpoet/lib/Automation/Integrations/Core/Actions/DelayAction.php

@@ -5,9 +5,10 @@ namespace MailPoet\Automation\Integrations\Core\Actions;
 use MailPoet\Automation\Engine\Control\ActionScheduler;
 use MailPoet\Automation\Engine\Data\Step;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
-use MailPoet\Automation\Engine\Data\Workflow;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Hooks;
 use MailPoet\Automation\Engine\Integration\Action;
+use MailPoet\Automation\Engine\Integration\ValidationException;
 use MailPoet\Validator\Builder;
 use MailPoet\Validator\Schema\ObjectSchema;
 
@@ -40,6 +41,16 @@ class DelayAction implements Action {
     return [];
   }
 
+  public function validate(StepValidationArgs $args): void {
+    $seconds = $this->calculateSeconds($args->getStep());
+    if ($seconds <= 0) {
+      throw new ValidationException(__('A delay must have a positive value', 'mailpoet'));
+    }
+    if ($seconds > 2 * YEAR_IN_SECONDS) {
+      throw new ValidationException(__("A delay can't be longer than two years", 'mailpoet'));
+    }
+  }
+
   public function run(StepRunArgs $args): void {
     $step = $args->getStep();
     $nextStep = $step->getNextSteps()[0] ?? null;
@@ -53,12 +64,6 @@ class DelayAction implements Action {
     // TODO: call a step complete ($id) hook instead?
   }
 
-  public function isValid(array $subjects, Step $step, Workflow $workflow): bool {
-    $seconds = $this->calculateSeconds($step);
-
-    return $seconds > 0 && $seconds < 2 * YEAR_IN_SECONDS;
-  }
-
   private function calculateSeconds(Step $step): int {
     $delay = (int)($step->getArgs()['delay'] ?? null);
     switch ($step->getArgs()['delay_type']) {

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

@@ -4,13 +4,11 @@ namespace MailPoet\Automation\Integrations\MailPoet\Actions;
 
 use MailPoet\Automation\Engine\Data\Step;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
-use MailPoet\Automation\Engine\Data\Workflow;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Integration\Action;
-use MailPoet\Automation\Engine\Integration\Subject;
+use MailPoet\Automation\Engine\Integration\ValidationException;
 use MailPoet\Automation\Integrations\MailPoet\Payloads\SegmentPayload;
 use MailPoet\Automation\Integrations\MailPoet\Payloads\SubscriberPayload;
-use MailPoet\Automation\Integrations\MailPoet\Subjects\SegmentSubject;
-use MailPoet\Automation\Integrations\MailPoet\Subjects\SubscriberSubject;
 use MailPoet\Entities\NewsletterEntity;
 use MailPoet\Entities\SubscriberEntity;
 use MailPoet\InvalidStateException;
@@ -81,21 +79,16 @@ class SendEmailAction implements Action {
     ];
   }
 
-  public function isValid(array $subjects, Step $step, Workflow $workflow): bool {
+  public function validate(StepValidationArgs $args): void {
     try {
-      $this->getEmailForStep($step);
+      $this->getEmailForStep($args->getStep());
     } catch (InvalidStateException $exception) {
-      return false;
+      $emailId = $args->getStep()->getArgs()['email_id'] ?? '';
+      throw new ValidationException(
+        // translators: %s is the ID of email.
+        sprintf(__("Automation email with ID '%s' not found.", 'mailpoet'), $emailId)
+      );
     }
-
-    $segmentSubjects = array_filter($subjects, function (Subject $subject) {
-      return $subject->getKey() === SegmentSubject::KEY;
-    });
-    $subscriberSubjects = array_filter($subjects, function (Subject $subject) {
-      return $subject->getKey() === SubscriberSubject::KEY;
-    });
-
-    return count($segmentSubjects) === 1 && count($subscriberSubjects) === 1;
   }
 
   public function run(StepRunArgs $args): void {
@@ -164,7 +157,10 @@ class SendEmailAction implements Action {
       'type' => NewsletterEntity::TYPE_AUTOMATION,
     ]);
     if (!$email) {
-      throw InvalidStateException::create()->withMessage(sprintf("Automation email with ID '%s' not found.", $emailId));
+      throw InvalidStateException::create()->withMessage(
+        // translators: %s is the ID of email.
+        sprintf(__("Automation email with ID '%s' not found.", 'mailpoet'), $emailId)
+      );
     }
     return $email;
   }

mailpoet/tests/integration/Automation/Engine/Data/WorkflowRunLogTest.php

@@ -5,6 +5,7 @@ namespace MailPoet\Test\Automation\Engine\Data;
 use MailPoet\Automation\Engine\Control\StepHandler;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 use MailPoet\Automation\Engine\Data\Step;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Data\Workflow;
 use MailPoet\Automation\Engine\Data\WorkflowRun;
 use MailPoet\Automation\Engine\Data\WorkflowRunLog;
@@ -291,8 +292,7 @@ class TestAction implements Action {
     return [];
   }
 
-  public function isValid(array $subjects, Step $step, Workflow $workflow): bool {
-    return true;
+  public function validate(StepValidationArgs $args): void {
   }
 
   public function run(StepRunArgs $args): void {

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

@@ -6,9 +6,11 @@ use MailPoet\Automation\Engine\Control\ActionScheduler;
 use MailPoet\Automation\Engine\Data\NextStep;
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 use MailPoet\Automation\Engine\Data\Step;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Data\Workflow;
 use MailPoet\Automation\Engine\Data\WorkflowRun;
 use MailPoet\Automation\Engine\Hooks;
+use MailPoet\Automation\Engine\Integration\ValidationException;
 use MailPoet\Automation\Integrations\Core\Actions\DelayAction;
 
 class DelayActionTest extends \MailPoetTest {
@@ -82,8 +84,7 @@ class DelayActionTest extends \MailPoetTest {
   /**
    * @dataProvider dataForTestDelayActionInvalidatesOutsideOfBoundaries
    */
-  public function testDelayActionInvalidatesOutsideOfBoundaries(int $delay, bool $expectation) {
-
+  public function testDelayActionInvalidatesOutsideOfBoundaries(int $delay, ?string $expectation) {
     $step = new Step(
       '1',
       'core:delay',
@@ -96,31 +97,36 @@ class DelayActionTest extends \MailPoetTest {
     );
     $workflow = $this->createMock(Workflow::class);
     $actionScheduler = $this->createMock(ActionScheduler::class);
+
+    if ($expectation) {
+      $this->expectException(ValidationException::class);
+      $this->expectExceptionMessage($expectation);
+    }
     $testee = new DelayAction($actionScheduler);
-    $this->assertEquals($expectation, $testee->isValid([], $step, $workflow));
+    $testee->validate(new StepValidationArgs($workflow, $step, []));
   }
 
   public function dataForTestDelayActionInvalidatesOutsideOfBoundaries() : array {
     return [
       'zero' => [
         0,
-        false,
+        'A delay must have a positive value',
       ],
       'minus_one' => [
         -1,
-        false,
+        'A delay must have a positive value',
       ],
       'one' => [
         1,
-        true,
+        null,
       ],
       'two_years' => [
-        2*8760,
-        false,
+        2*8760 + 1,
+        'A delay can\'t be longer than two years',
       ],
       'below_two_years' => [
-        2*8760-1,
-        true,
+        2*8760,
+        null,
       ],
     ];
   }

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

@@ -4,10 +4,12 @@ namespace MailPoet\Test\Automation\Integrations\MailPoet\Actions;
 
 use MailPoet\Automation\Engine\Data\StepRunArgs;
 use MailPoet\Automation\Engine\Data\Step;
+use MailPoet\Automation\Engine\Data\StepValidationArgs;
 use MailPoet\Automation\Engine\Data\Subject;
 use MailPoet\Automation\Engine\Data\SubjectEntry;
 use MailPoet\Automation\Engine\Data\Workflow;
 use MailPoet\Automation\Engine\Data\WorkflowRun;
+use MailPoet\Automation\Engine\Integration\ValidationException;
 use MailPoet\Automation\Integrations\MailPoet\Actions\SendEmailAction;
 use MailPoet\Automation\Integrations\MailPoet\Subjects\SegmentSubject;
 use MailPoet\Automation\Integrations\MailPoet\Subjects\SubscriberSubject;
@@ -48,15 +50,9 @@ class SendEmailActionTest extends \MailPoetTest {
   /** @var SegmentSubject */
   private $segmentSubject;
 
-  /** @var Step */
-  private $step;
-
   /** @var Workflow */
   private $workflow;
 
-  /** @var NewsletterEntity */
-  private $email;
-
   public function _before() {
     parent::_before();
     $this->cleanup();
@@ -68,33 +64,28 @@ class SendEmailActionTest extends \MailPoetTest {
     $this->subscriberSubject = $this->diContainer->get(SubscriberSubject::class);
     $this->segmentSubject = $this->diContainer->get(SegmentSubject::class);
 
-    $this->email = (new Newsletter())->withAutomationType()->create();
-    $this->step = new Step('step-id', Step::TYPE_ACTION, 'step-key',['email_id' => $this->email->getId()], []);
     $this->workflow = new Workflow('test-workflow', [], new \WP_User());
   }
 
-  public function testItKnowsWhenItHasAllRequiredSubjects() {
-    expect($this->action->isValid([], $this->step, $this->workflow))->false();
-    expect($this->action->isValid($this->getSubjects(), $this->step, $this->workflow))->true();
-  }
-
-  public function testItRequiresASubscriberSubject() {
-    expect($this->action->isValid([$this->segmentSubject], $this->step, $this->workflow))->false();
-  }
-
-  public function testItRequiresASegmentSubject() {
-    expect($this->action->isValid([$this->subscriberSubject], $this->step, $this->workflow))->false();
+  public function testItReturnsRequiredSubjects() {
+    $this->assertSame(['mailpoet:segment', 'mailpoet:subscriber'], $this->action->getSubjectKeys());
   }
 
   public function testItIsNotValidIfStepHasNoEmail(): void {
     $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', [], []);
-    expect($this->action->isValid($this->getSubjects(), $step, $this->workflow))->false();
+
+    $this->expectException(ValidationException::class);
+    $this->expectExceptionMessage("Automation email with ID '' not found.");
+    $this->action->validate(new StepValidationArgs($this->workflow, $step, []));
   }
 
   public function testItRequiresAutomationEmailType(): void {
     $newsletter = (new Newsletter())->withPostNotificationsType()->create();
     $step = new Step('step-id', Step::TYPE_ACTION, 'step-key', ['email_id' => $newsletter->getId()], []);
-    expect($this->action->isValid($this->getSubjects(), $step, $this->workflow))->false();
+
+    $this->expectExceptionMessage("Automation email with ID '{$newsletter->getId()}' not found.");
+    $this->action->validate(new StepValidationArgs($this->workflow, $step, []));
+    $this->action->validate(new StepValidationArgs($this->workflow, $step, []));
   }
 
   public function testHappyPath() {