diff --git a/mailpoet/lib/Automation/Engine/Exceptions.php b/mailpoet/lib/Automation/Engine/Exceptions.php index 6e444ca9e2..a8e62e4ffc 100644 --- a/mailpoet/lib/Automation/Engine/Exceptions.php +++ b/mailpoet/lib/Automation/Engine/Exceptions.php @@ -28,6 +28,7 @@ class Exceptions { private const WORKFLOW_STRUCTURE_NOT_VALID = 'mailpoet_automation_workflow_structure_not_valid'; private const WORKFLOW_STEP_MODIFIED_WHEN_UNKNOWN = 'mailpoet_automation_workflow_step_modified_when_unknown'; private const WORKFLOW_NOT_VALID = 'mailpoet_automation_workflow_not_valid'; + private const MISSING_REQUIRED_SUBJECTS = 'mailpoet_automation_missing_required_subjects'; public function __construct() { throw new InvalidStateException( @@ -190,4 +191,17 @@ class Exceptions { ->withMessage(sprintf(__("Workflow validation failed: %s", 'mailpoet'), $detail)) ->withErrors($errors); } + + public static function missingRequiredSubjects(Step $step, array $missingSubjectKeys): UnexpectedValueException { + return UnexpectedValueException::create() + ->withErrorCode(self::MISSING_REQUIRED_SUBJECTS) + // translators: %1$s is the key of the step, %2$s are the missing subject keys. + ->withMessage( + sprintf( + __("Step with ID '%1\$s' is missing required subjects with keys: %2\$s", 'mailpoet'), + $step->getId(), + implode(', ', $missingSubjectKeys) + ) + ); + } } diff --git a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepArgsRule.php b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepArgsRule.php index 1edf26455f..a25d96ab67 100644 --- a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepArgsRule.php +++ b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepArgsRule.php @@ -3,7 +3,6 @@ namespace MailPoet\Automation\Engine\Validation\WorkflowRules; use MailPoet\Automation\Engine\Data\Workflow; -use MailPoet\Automation\Engine\Exceptions; use MailPoet\Automation\Engine\Registry; use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNode; use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor; @@ -16,9 +15,6 @@ class ValidStepArgsRule implements WorkflowNodeVisitor { /** @var Validator */ private $validator; - /** @var array */ - private $errors = []; - public function __construct( Registry $registry, Validator $validator @@ -28,7 +24,6 @@ class ValidStepArgsRule implements WorkflowNodeVisitor { } public function initialize(Workflow $workflow): void { - $this->errors = []; } public function visitNode(Workflow $workflow, WorkflowNode $node): void { @@ -39,25 +34,11 @@ class ValidStepArgsRule implements WorkflowNodeVisitor { } // validate args schema only for active workflows - if ($workflow->getStatus() !== Workflow::STATUS_ACTIVE) { - return; - } - - try { + if ($workflow->getStatus() === Workflow::STATUS_ACTIVE) { $this->validator->validate($registryStep->getArgsSchema(), $step->getArgs()); - } catch (Throwable $e) { - $this->errors[$step->getId()] = [ - 'step_id' => $step->getId(), - 'message' => $e instanceof ValidationException - ? $e->getWpError()->get_error_message() - : __('Unknown error.', 'mailpoet'), - ]; } } public function complete(Workflow $workflow): void { - if ($this->errors) { - throw Exceptions::workflowNotValid(__('Some steps have invalid arguments', 'mailpoet'), $this->errors); - } } } diff --git a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepOrderRule.php b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepOrderRule.php index 3163277a39..5900d0bfe9 100644 --- a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepOrderRule.php +++ b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepOrderRule.php @@ -13,9 +13,6 @@ class ValidStepOrderRule implements WorkflowNodeVisitor { /** @var Registry */ private $registry; - /** @var array */ - private $errors = []; - public function __construct( Registry $registry ) { @@ -23,7 +20,6 @@ class ValidStepOrderRule implements WorkflowNodeVisitor { } public function initialize(Workflow $workflow): void { - $this->errors = []; } public function visitNode(Workflow $workflow, WorkflowNode $node): void { @@ -51,22 +47,11 @@ class ValidStepOrderRule implements WorkflowNodeVisitor { $subjectKeys = $this->collectSubjectKeys($workflow, $node->getParents()); $missingSubjectKeys = array_diff($requiredSubjectKeys, $subjectKeys); if (count($missingSubjectKeys) > 0) { - $missingKeysString = implode(', ', $missingSubjectKeys); - $this->errors[$step->getId()] = [ - 'step_id' => $step->getId(), - 'message' => sprintf( - // translators: %s are the missing subject keys. - __("Missing required subjects with keys: %s", 'mailpoet'), - $missingKeysString - ), - ]; + throw Exceptions::missingRequiredSubjects($step, $missingSubjectKeys); } } public function complete(Workflow $workflow): void { - if ($this->errors) { - throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors); - } } /** diff --git a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepRule.php b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepRule.php new file mode 100644 index 0000000000..6ca6171d4d --- /dev/null +++ b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepRule.php @@ -0,0 +1,54 @@ + */ + private $errors = []; + + /** @param WorkflowNodeVisitor[] $rules */ + public function __construct( + array $rules + ) { + $this->rules = $rules; + } + + public function initialize(Workflow $workflow): void { + foreach ($this->rules as $rule) { + $rule->initialize($workflow); + } + } + + public function visitNode(Workflow $workflow, WorkflowNode $node): void { + foreach ($this->rules as $rule) { + $stepId = $node->getStep()->getId(); + try { + $rule->visitNode($workflow, $node); + } catch (UnexpectedValueException $e) { + $this->errors[$stepId] = ['step_id' => $stepId, 'message' => $e->getMessage()]; + } catch (Throwable $e) { + $this->errors[$stepId] = ['step_id' => $stepId, 'message' => __('Unknown error.', 'mailpoet')]; + } + } + } + + public function complete(Workflow $workflow): void { + foreach ($this->rules as $rule) { + $rule->complete($workflow); + } + + if ($this->errors) { + throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors); + } + } +} diff --git a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepValidationRule.php b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepValidationRule.php index 4ed237e53a..8bfee6618e 100644 --- a/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepValidationRule.php +++ b/mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepValidationRule.php @@ -8,19 +8,14 @@ use MailPoet\Automation\Engine\Data\Workflow; use MailPoet\Automation\Engine\Exceptions; use MailPoet\Automation\Engine\Integration\Payload; use MailPoet\Automation\Engine\Integration\Subject; -use MailPoet\Automation\Engine\Integration\ValidationException; use MailPoet\Automation\Engine\Registry; use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNode; use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor; -use Throwable; class ValidStepValidationRule implements WorkflowNodeVisitor { /** @var Registry */ private $registry; - /** @var array */ - private $errors = []; - public function __construct( Registry $registry ) { @@ -28,7 +23,6 @@ class ValidStepValidationRule implements WorkflowNodeVisitor { } public function initialize(Workflow $workflow): void { - $this->errors = []; } public function visitNode(Workflow $workflow, WorkflowNode $node): void { @@ -43,22 +37,12 @@ class ValidStepValidationRule implements WorkflowNodeVisitor { return; } - try { - $subjects = $this->collectSubjects($workflow, $node->getParents()); - $args = new StepValidationArgs($workflow, $step, $subjects); - $registryStep->validate($args); - } catch (Throwable $e) { - $this->errors[$step->getId()] = [ - 'step_id' => $step->getId(), - 'message' => $e instanceof ValidationException ? $e->getMessage() : __('Unknown error.', 'mailpoet'), - ]; - } + $subjects = $this->collectSubjects($workflow, $node->getParents()); + $args = new StepValidationArgs($workflow, $step, $subjects); + $registryStep->validate($args); } public function complete(Workflow $workflow): void { - if ($this->errors) { - throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors); - } } /** diff --git a/mailpoet/lib/Automation/Engine/Validation/WorkflowValidator.php b/mailpoet/lib/Automation/Engine/Validation/WorkflowValidator.php index e75ec7177b..8b9926e489 100644 --- a/mailpoet/lib/Automation/Engine/Validation/WorkflowValidator.php +++ b/mailpoet/lib/Automation/Engine/Validation/WorkflowValidator.php @@ -14,6 +14,7 @@ use MailPoet\Automation\Engine\Validation\WorkflowRules\TriggersUnderRootRule; use MailPoet\Automation\Engine\Validation\WorkflowRules\UnknownStepRule; use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepArgsRule; use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepOrderRule; +use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepRule; use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepValidationRule; class WorkflowValidator { @@ -56,9 +57,11 @@ class WorkflowValidator { new NoJoinRule(), new NoSplitRule(), $this->unknownStepRule, - $this->validStepArgsRule, - $this->validStepOrderRule, - $this->validStepValidationRule, + new ValidStepRule([ + $this->validStepArgsRule, + $this->validStepOrderRule, + $this->validStepValidationRule, + ]), ]); } } diff --git a/mailpoet/tests/unit/Automation/Engine/Validation/WorkflowRules/ValidStepRuleTest.php b/mailpoet/tests/unit/Automation/Engine/Validation/WorkflowRules/ValidStepRuleTest.php new file mode 100644 index 0000000000..9d34bc746d --- /dev/null +++ b/mailpoet/tests/unit/Automation/Engine/Validation/WorkflowRules/ValidStepRuleTest.php @@ -0,0 +1,109 @@ +getWorkflow(); + $workflow->setStatus(Workflow::STATUS_ACTIVE); + + $rule = new ValidStepRule([ + $this->makeEmpty(WorkflowNodeVisitor::class, [ + 'initialize' => Expected::once(), + 'visitNode' => Expected::once(), + 'complete' => Expected::once(), + ]) + ]); + (new WorkflowWalker())->walk($workflow, [$rule]); + } + + public function testItCollectsRecognizedErrors(): void { + $workflow = $this->getWorkflow(); + $workflow->setStatus(Workflow::STATUS_ACTIVE); + + $rule = new ValidStepRule([ + $this->makeEmpty(WorkflowNodeVisitor::class, [ + 'visitNode' => Expected::once(function () { + throw UnexpectedValueException::create()->withMessage('Test error'); + }), + ]), + ]); + + try { + (new WorkflowWalker())->walk($workflow, [$rule]); + } catch (UnexpectedValueException $e) { + $this->assertSame( + [ + 'root' => [ + 'step_id' => 'root', + 'message' => 'Test error', + ], + ], + $e->getErrors() + ); + return; + } + $this->fail(sprintf("Exception of class '%s' was not thrown.", UnexpectedValueException::class)); + } + + + public function testItCollectsUnrecognizedErrorsWithAGenericMessage(): void { + $workflow = $this->getWorkflow(); + $workflow->setStatus(Workflow::STATUS_ACTIVE); + + $rule = new ValidStepRule([ + $this->makeEmpty(WorkflowNodeVisitor::class, [ + 'visitNode' => Expected::once(function () { + throw new Exception(' Unknown test error'); + }), + ]), + ]); + + try { + (new WorkflowWalker())->walk($workflow, [$rule]); + } catch (UnexpectedValueException $e) { + $this->assertSame( + [ + 'root' => [ + 'step_id' => 'root', + 'message' => 'Unknown error.', + ], + ], + $e->getErrors() + ); + return; + } + $this->fail(sprintf("Exception of class '%s' was not thrown.", UnexpectedValueException::class)); + } + + public function testItValidatesOnlyActiveWorkflows(): void { + $workflow = $this->getWorkflow(); + $workflow->setStatus(Workflow::STATUS_DRAFT); + $rule = new ValidStepRule([ + $this->makeEmpty(WorkflowNodeVisitor::class, [ + 'initialize' => Expected::never(), + 'visitNode' => Expected::never(), + 'complete' => Expected::never(), + ]) + ]); + (new WorkflowWalker())->walk($workflow, [$rule]); + } + + private function getWorkflow(): Workflow { + return $this->make(Workflow::class, [ + 'steps' => [ + 'root' => new Step('root', 'root', 'core:root', [], []), + ], + ]); + } +}