diff --git a/mailpoet/lib/Automation/Engine/Control/StepHandler.php b/mailpoet/lib/Automation/Engine/Control/StepHandler.php index fe3ec12b00..7d0b83a337 100644 --- a/mailpoet/lib/Automation/Engine/Control/StepHandler.php +++ b/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(); diff --git a/mailpoet/lib/Automation/Engine/Control/StepRunner.php b/mailpoet/lib/Automation/Engine/Control/StepRunner.php index 7bd89c7ca9..4b118f6d02 100644 --- a/mailpoet/lib/Automation/Engine/Control/StepRunner.php +++ b/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; } diff --git a/mailpoet/lib/Automation/Engine/Control/Steps/ActionStepRunner.php b/mailpoet/lib/Automation/Engine/Control/Steps/ActionStepRunner.php index 7512e4a049..9bad551654 100644 --- a/mailpoet/lib/Automation/Engine/Control/Steps/ActionStepRunner.php +++ b/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); } } diff --git a/mailpoet/lib/Automation/Engine/Integration/Action.php b/mailpoet/lib/Automation/Engine/Integration/Action.php index c44df5335e..76cc6551b8 100644 --- a/mailpoet/lib/Automation/Engine/Integration/Action.php +++ b/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; } diff --git a/mailpoet/lib/Automation/Engine/Integration/ValidationException.php b/mailpoet/lib/Automation/Engine/Integration/ValidationException.php new file mode 100644 index 0000000000..0abfd63141 --- /dev/null +++ b/mailpoet/lib/Automation/Engine/Integration/ValidationException.php @@ -0,0 +1,8 @@ +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']) { diff --git a/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php b/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php index 1b7227be7d..788b8a8ff3 100644 --- a/mailpoet/lib/Automation/Integrations/MailPoet/Actions/SendEmailAction.php +++ b/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; } diff --git a/mailpoet/tests/integration/Automation/Engine/Data/WorkflowRunLogTest.php b/mailpoet/tests/integration/Automation/Engine/Data/WorkflowRunLogTest.php index e7afec48f3..190e6631fa 100644 --- a/mailpoet/tests/integration/Automation/Engine/Data/WorkflowRunLogTest.php +++ b/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 { diff --git a/mailpoet/tests/integration/Automation/Integrations/Core/Actions/DelayActionTest.php b/mailpoet/tests/integration/Automation/Integrations/Core/Actions/DelayActionTest.php index 211669ab90..b82a5ed13f 100644 --- a/mailpoet/tests/integration/Automation/Integrations/Core/Actions/DelayActionTest.php +++ b/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, ], ]; } diff --git a/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php b/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php index 12b09bba07..ece0b4dcaf 100644 --- a/mailpoet/tests/integration/Automation/Integrations/MailPoet/Actions/SendEmailActionTest.php +++ b/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() {