From 2620ef0b57f8b8a02a7e83c39d827040a4533a91 Mon Sep 17 00:00:00 2001 From: David Remer Date: Mon, 24 Oct 2022 11:28:36 +0300 Subject: [PATCH] Do not handle workflow runs when workflow status is not active/deactivating [MAILPOET-4731] --- .../Automation/Engine/Control/StepHandler.php | 7 +- mailpoet/lib/Automation/Engine/Exceptions.php | 8 ++ .../Engine/Control/StepHandlerTest.php | 119 ++++++++++++++++++ 3 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php diff --git a/mailpoet/lib/Automation/Engine/Control/StepHandler.php b/mailpoet/lib/Automation/Engine/Control/StepHandler.php index 64b7746658..f71bb4753f 100644 --- a/mailpoet/lib/Automation/Engine/Control/StepHandler.php +++ b/mailpoet/lib/Automation/Engine/Control/StepHandler.php @@ -8,6 +8,7 @@ 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\Workflow; use MailPoet\Automation\Engine\Data\WorkflowRun; use MailPoet\Automation\Engine\Data\WorkflowRunLog; use MailPoet\Automation\Engine\Exceptions; @@ -98,7 +99,8 @@ class StepHandler { try { $this->handleStep($args); } catch (Throwable $e) { - $this->workflowRunStorage->updateStatus((int)$args['workflow_run_id'], WorkflowRun::STATUS_FAILED); + $status = $e instanceof InvalidStateException && $e->getErrorCode() === 'mailpoet_automation_workflow_not_active' ? WorkflowRun::STATUS_CANCELLED : WorkflowRun::STATUS_FAILED; + $this->workflowRunStorage->updateStatus((int)$args['workflow_run_id'], $status); if (!$e instanceof Exception) { throw new Exception($e->getMessage(), intval($e->getCode()), $e); } @@ -123,6 +125,9 @@ class StepHandler { if (!$workflow) { throw Exceptions::workflowVersionNotFound($workflowRun->getWorkflowId(), $workflowRun->getVersionId()); } + if (!in_array($workflow->getStatus(), [Workflow::STATUS_ACTIVE, Workflow::STATUS_DEACTIVATING], true)) { + throw Exceptions::workflowNotActive($workflowRun->getWorkflowId()); + } // complete workflow run if (!$stepId) { diff --git a/mailpoet/lib/Automation/Engine/Exceptions.php b/mailpoet/lib/Automation/Engine/Exceptions.php index 09df71b610..118b6f31eb 100644 --- a/mailpoet/lib/Automation/Engine/Exceptions.php +++ b/mailpoet/lib/Automation/Engine/Exceptions.php @@ -14,6 +14,7 @@ class Exceptions { private const JSON_NOT_OBJECT = 'mailpoet_automation_json_not_object'; private const WORKFLOW_NOT_FOUND = 'mailpoet_automation_workflow_not_found'; private const WORKFLOW_VERSION_NOT_FOUND = 'mailpoet_automation_workflow_version_not_found'; + private const WORKFLOW_NOT_ACTIVE = 'mailpoet_automation_workflow_not_active'; private const WORKFLOW_RUN_NOT_FOUND = 'mailpoet_automation_workflow_run_not_found'; private const WORKFLOW_STEP_NOT_FOUND = 'mailpoet_automation_workflow_step_not_found'; private const WORKFLOW_TRIGGER_NOT_FOUND = 'mailpoet_automation_workflow_trigger_not_found'; @@ -72,6 +73,13 @@ class Exceptions { ->withMessage(sprintf(__('Workflow with ID "%1$s" in version "%2$s" not found.', 'mailpoet'), $workflow, $version)); } + public static function workflowNotActive(int $workflow): InvalidStateException { + return InvalidStateException::create() + ->withErrorCode(self::WORKFLOW_NOT_ACTIVE) + // translators: %1$s is the ID of the workflow. + ->withMessage(sprintf(__('Workflow with ID "%1$s" in no longer active.', 'mailpoet'), $workflow)); + } + public static function workflowRunNotFound(int $id): NotFoundException { return NotFoundException::create() ->withErrorCode(self::WORKFLOW_RUN_NOT_FOUND) diff --git a/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php b/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php new file mode 100644 index 0000000000..8c7a650462 --- /dev/null +++ b/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php @@ -0,0 +1,119 @@ +testee = $this->diContainer->get(StepHandler::class); + $this->workflowStorage = $this->diContainer->get(WorkflowStorage::class); + $this->workflowRunStorage = $this->diContainer->get(WorkflowRunStorage::class); + } + + public function testItDoesOnlyProcessActiveAndDeactivatingWorkflows() { + $workflow = $this->createWorkflow(); + $this->assertInstanceOf(Workflow::class, $workflow); + $steps = $workflow->getSteps(); + $workflowRun = $this->createWorkflowRun($workflow); + $this->assertInstanceOf(WorkflowRun::class, $workflowRun); + + $currentStep = current($steps); + $this->assertInstanceOf(Step::class, $currentStep); + $runner = $this->createMock(StepRunner::class); + + $runner->expects(self::exactly(2))->method('run'); // The run method will be called twice: Once for the active workflow and once for the deactivating workflow. + + $this->testee->addStepRunner($currentStep->getType(), $runner); + $this->assertSame(Workflow::STATUS_ACTIVE, $workflow->getStatus()); + $this->testee->handle(['workflow_run_id' => $workflowRun->getId(), 'step_id' => $currentStep->getId()]); + // no exception thrown. + $newWorkflowRun = $this->workflowRunStorage->getWorkflowRun($workflowRun->getId()); + $this->assertInstanceOf(WorkflowRun::class, $newWorkflowRun); + $this->assertSame(WorkflowRun::STATUS_RUNNING, $newWorkflowRun->getStatus()); + + $workflow->setStatus(Workflow::STATUS_DEACTIVATING); + $this->workflowStorage->updateWorkflow($workflow); + $this->testee->handle(['workflow_run_id' => $workflowRun->getId(), 'step_id' => $currentStep->getId()]); + // no exception thrown. + $newWorkflowRun = $this->workflowRunStorage->getWorkflowRun($workflowRun->getId()); + $this->assertInstanceOf(WorkflowRun::class, $newWorkflowRun); + $this->assertSame(WorkflowRun::STATUS_RUNNING, $newWorkflowRun->getStatus()); + + $invalidStati = array_filter( + Workflow::STATUS_ALL, + function(string $status) : bool { + return !in_array($status, [Workflow::STATUS_ACTIVE, Workflow::STATUS_DEACTIVATING], true); + } + ); + + foreach ($invalidStati as $status) { + $workflow->setStatus($status); + $this->workflowStorage->updateWorkflow($workflow); + $workflowRun = $this->createWorkflowRun($workflow); + $this->assertInstanceOf(WorkflowRun::class, $workflowRun); + $error = null; + try { + $this->testee->handle(['workflow_run_id' => $workflowRun->getId(), 'step_id' => $currentStep->getId()]); + } catch (InvalidStateException $error) { + $this->assertSame('mailpoet_automation_workflow_not_active', $error->getErrorCode(), "Workflow with '$status' did not return expected error code."); + } + $this->assertInstanceOf(InvalidStateException::class, $error); + + $newWorkflowRun = $this->workflowRunStorage->getWorkflowRun($workflowRun->getId()); + $this->assertInstanceOf(WorkflowRun::class, $newWorkflowRun); + + $this->assertSame(WorkflowRun::STATUS_CANCELLED, $newWorkflowRun->getStatus()); + } + } + + private function createWorkflow(): ?Workflow { + $trigger = $this->diContainer->get(SomeoneSubscribesTrigger::class); + $delay = $this->diContainer->get(DelayAction::class); + $steps = [ + 'root' => new Step('root', Step::TYPE_ROOT, 'root', [], [new NextStep('someone-subscribes')]), + 'someone-subscribes' => new Step('someone-subscribes', Step::TYPE_TRIGGER, $trigger->getKey(), [], [new NextStep('a')]), + 'delay' => new Step('delay', Step::TYPE_ACTION, $delay->getKey(), [], []), + ]; + $workflow = new Workflow('test', $steps, wp_get_current_user()); + $workflow->setStatus(Workflow::STATUS_ACTIVE); + return $this->workflowStorage->getWorkflow($this->workflowStorage->createWorkflow($workflow)); + } + + private function createWorkflowRun(Workflow $workflow, $subjects = []) : ?WorkflowRun { + $trigger = array_filter($workflow->getSteps(), function(Step $step) : bool { return $step->getType() === Step::TYPE_TRIGGER;}); + $triggerKeys = array_map(function(Step $step) : string { return $step->getKey();}, $trigger); + $triggerKey = count($triggerKeys)>0?current($triggerKeys):''; + + $workflowRun = new WorkflowRun( + $workflow->getId(), + $workflow->getVersionId(), + $triggerKey, + $subjects + ); + return $this->workflowRunStorage->getWorkflowRun($this->workflowRunStorage->createWorkflowRun($workflowRun)); + } + +}