From c8f8888c172ecbc258c3dbbb681180a4553201ff Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 14 Jun 2021 14:51:16 -0400 Subject: [PATCH 1/8] Script to reset workflow. Requires workflow spec name. --- crc/scripts/reset_workflow.py | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 crc/scripts/reset_workflow.py diff --git a/crc/scripts/reset_workflow.py b/crc/scripts/reset_workflow.py new file mode 100644 index 00000000..ddd22651 --- /dev/null +++ b/crc/scripts/reset_workflow.py @@ -0,0 +1,46 @@ +from crc import session +from crc.api.common import ApiError +from crc.models.workflow import WorkflowModel, WorkflowSpecModel +from crc.scripts.script import Script +from crc.services.workflow_processor import WorkflowProcessor + + +class ResetWorkflow(Script): + + def get_description(self): + return """Reset a workflow. Run by master workflow. + Designed for completed workflows where we need to force rerunning the workflow. + I.e., a new PI""" + + def do_task_validate_only(self, task, study_id, workflow_id, *args, **kwargs): + return hasattr(kwargs, 'workflow_name') + + def do_task(self, task, study_id, workflow_id, *args, **kwargs): + + if 'workflow_name' in kwargs.keys(): + workflow_name = kwargs['workflow_name'] + workflow_spec: WorkflowSpecModel = session.query(WorkflowSpecModel).filter_by(name=workflow_name).first() + if workflow_spec: + workflow_model: WorkflowModel = session.query(WorkflowModel).filter_by( + workflow_spec_id=workflow_spec.id, + study_id=study_id).first() + if workflow_model: + try: + workflow_processor = WorkflowProcessor.reset(workflow_model, clear_data=False, delete_files=False) + except Exception as e: + raise ApiError(code='unknown_error', + message=f'An unknown error occurred: {e}') + else: + return True + else: + raise ApiError(code='missing_workflow_model', + message=f'No WorkflowModel returned. \ + workflow_spec_id: {workflow_spec.id} \ + study_id: {study_id}') + else: + raise ApiError(code='missing_workflow_spec', + message=f'No WorkflowSpecModel returned. \ + name: {workflow_name}') + else: + raise ApiError(code='missing_workflow_name', + message='Reset workflow requires a workflow name') From 9e20025f33705fcfde977655a3c8a826552ff790 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 15 Jun 2021 08:37:42 -0400 Subject: [PATCH 2/8] Test and workflow for reset_workflow --- tests/data/reset_workflow/reset_workflow.bpmn | 82 +++++++++++++++++++ tests/workflow/test_workflow_reset.py | 22 +++++ 2 files changed, 104 insertions(+) create mode 100644 tests/data/reset_workflow/reset_workflow.bpmn create mode 100644 tests/workflow/test_workflow_reset.py diff --git a/tests/data/reset_workflow/reset_workflow.bpmn b/tests/data/reset_workflow/reset_workflow.bpmn new file mode 100644 index 00000000..0a2fe9a9 --- /dev/null +++ b/tests/data/reset_workflow/reset_workflow.bpmn @@ -0,0 +1,82 @@ + + + + + SequenceFlow_1oykjju + + + + + + + + + + + + + + + SequenceFlow_1oykjju + SequenceFlow_0z8c3ob + + + + + + + + + + + + SequenceFlow_0z8c3ob + SequenceFlow_1jfrd7w + + + # Data +{{name}} is {{age}} years old. + SequenceFlow_1jfrd7w + SequenceFlow_0yjk26l + + + SequenceFlow_0yjk26l + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_reset.py b/tests/workflow/test_workflow_reset.py new file mode 100644 index 00000000..3bb5174c --- /dev/null +++ b/tests/workflow/test_workflow_reset.py @@ -0,0 +1,22 @@ +from tests.base_test import BaseTest +from crc.scripts.reset_workflow import ResetWorkflow + + +class TestWorkflowReset(BaseTest): + + def test_workflow_reset(self): + workflow = self.create_workflow('reset_workflow') + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + self.assertEqual('Task_GetName', first_task.name) + + self.complete_form(workflow, first_task, {'name': 'Mona'}) + workflow_api = self.get_workflow_api(workflow) + second_task = workflow_api.next_task + self.assertEqual('Task_GetAge', second_task.name) + + ResetWorkflow().do_task(second_task, workflow.study_id, workflow.id, workflow_name='reset_workflow') + + workflow_api = self.get_workflow_api(workflow) + task = workflow_api.next_task + self.assertEqual('Task_GetName', task.name) From addf1cab5b32f640baffa1c4db1d34197d0d7deb Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 15 Jun 2021 10:30:18 -0400 Subject: [PATCH 3/8] Added tests for failing conditions --- tests/workflow/test_workflow_reset.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/workflow/test_workflow_reset.py b/tests/workflow/test_workflow_reset.py index 3bb5174c..929301a2 100644 --- a/tests/workflow/test_workflow_reset.py +++ b/tests/workflow/test_workflow_reset.py @@ -1,5 +1,6 @@ from tests.base_test import BaseTest from crc.scripts.reset_workflow import ResetWorkflow +from crc.api.common import ApiError class TestWorkflowReset(BaseTest): @@ -20,3 +21,19 @@ class TestWorkflowReset(BaseTest): workflow_api = self.get_workflow_api(workflow) task = workflow_api.next_task self.assertEqual('Task_GetName', task.name) + + def test_workflow_reset_missing_name(self): + workflow = self.create_workflow('reset_workflow') + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + + with self.assertRaises(ApiError): + ResetWorkflow().do_task(first_task, workflow.study_id, workflow.id) + + def test_workflow_reset_bad_name(self): + workflow = self.create_workflow('reset_workflow') + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + + with self.assertRaises(ApiError): + ResetWorkflow().do_task(first_task, workflow.study_id, workflow.id, workflow_name='bad_workflow_name') From 02949dc6e27bb8463f56e112df9da9f9051684a8 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 15 Jun 2021 11:12:50 -0400 Subject: [PATCH 4/8] WorkflowProcessor handles this error --- crc/scripts/reset_workflow.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crc/scripts/reset_workflow.py b/crc/scripts/reset_workflow.py index ddd22651..2b3275ec 100644 --- a/crc/scripts/reset_workflow.py +++ b/crc/scripts/reset_workflow.py @@ -25,13 +25,8 @@ class ResetWorkflow(Script): workflow_spec_id=workflow_spec.id, study_id=study_id).first() if workflow_model: - try: - workflow_processor = WorkflowProcessor.reset(workflow_model, clear_data=False, delete_files=False) - except Exception as e: - raise ApiError(code='unknown_error', - message=f'An unknown error occurred: {e}') - else: - return True + workflow_processor = WorkflowProcessor.reset(workflow_model, clear_data=False, delete_files=False) + return workflow_processor else: raise ApiError(code='missing_workflow_model', message=f'No WorkflowModel returned. \ From 8db4199d73c0a0525e87e8512fad4222eadec274 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 15 Jun 2021 11:15:04 -0400 Subject: [PATCH 5/8] Separate workflows for `using` the script and `validating` the script. --- tests/data/reset_workflow/reset_workflow.bpmn | 112 +++++++++--------- tests/data/two_user_tasks/two_user_tasks.bpmn | 82 +++++++++++++ 2 files changed, 138 insertions(+), 56 deletions(-) create mode 100644 tests/data/two_user_tasks/two_user_tasks.bpmn diff --git a/tests/data/reset_workflow/reset_workflow.bpmn b/tests/data/reset_workflow/reset_workflow.bpmn index 0a2fe9a9..1f2eaccf 100644 --- a/tests/data/reset_workflow/reset_workflow.bpmn +++ b/tests/data/reset_workflow/reset_workflow.bpmn @@ -1,82 +1,82 @@ - - + + + Use this process to reset a workflow for the current study. You must enter the name of the workflow. I.e., lower case with underscores. - SequenceFlow_1oykjju + SequenceFlow_0i872g2 - - - - - - - - - - - - - - SequenceFlow_1oykjju - SequenceFlow_0z8c3ob - - - - - - - - - - - - SequenceFlow_0z8c3ob - SequenceFlow_1jfrd7w - - - # Data -{{name}} is {{age}} years old. - SequenceFlow_1jfrd7w - SequenceFlow_0yjk26l - - - SequenceFlow_0yjk26l + + + + + SequenceFlow_0yy50p2 - + + + + + + + + + + + + SequenceFlow_0i872g2 + SequenceFlow_1q2ton3 + + + SequenceFlow_1q2ton3 + SequenceFlow_0x127gc + value = reset_workflow(workflow_name=workflow_name) + + + # Reset Workflow +<div> +{% if value %} +<span>Workflow {{workflow_name}} was reset.</span> +{% else %} +<span>There was a problem resetting workflow {{workflow_name}}.</span> +{% endif %} +</div> + + SequenceFlow_0x127gc + SequenceFlow_0yy50p2 + - + - + - + - + - - - - - - - - - - + - + + + + + + + + + + diff --git a/tests/data/two_user_tasks/two_user_tasks.bpmn b/tests/data/two_user_tasks/two_user_tasks.bpmn new file mode 100644 index 00000000..0a2fe9a9 --- /dev/null +++ b/tests/data/two_user_tasks/two_user_tasks.bpmn @@ -0,0 +1,82 @@ + + + + + SequenceFlow_1oykjju + + + + + + + + + + + + + + + SequenceFlow_1oykjju + SequenceFlow_0z8c3ob + + + + + + + + + + + + SequenceFlow_0z8c3ob + SequenceFlow_1jfrd7w + + + # Data +{{name}} is {{age}} years old. + SequenceFlow_1jfrd7w + SequenceFlow_0yjk26l + + + SequenceFlow_0yjk26l + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 94e730d04eaf616e2a361cd7fa1e26ae0503ef7d Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 15 Jun 2021 11:15:31 -0400 Subject: [PATCH 6/8] Test script validation --- tests/workflow/test_workflow_reset.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/workflow/test_workflow_reset.py b/tests/workflow/test_workflow_reset.py index 929301a2..d8b5bef3 100644 --- a/tests/workflow/test_workflow_reset.py +++ b/tests/workflow/test_workflow_reset.py @@ -5,8 +5,14 @@ from crc.api.common import ApiError class TestWorkflowReset(BaseTest): + def test_workflow_reset_validation(self): + self.load_example_data() + spec_model = self.load_test_spec('reset_workflow') + rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) + self.assertEqual([], rv.json) + def test_workflow_reset(self): - workflow = self.create_workflow('reset_workflow') + workflow = self.create_workflow('two_user_tasks') workflow_api = self.get_workflow_api(workflow) first_task = workflow_api.next_task self.assertEqual('Task_GetName', first_task.name) @@ -16,14 +22,14 @@ class TestWorkflowReset(BaseTest): second_task = workflow_api.next_task self.assertEqual('Task_GetAge', second_task.name) - ResetWorkflow().do_task(second_task, workflow.study_id, workflow.id, workflow_name='reset_workflow') + ResetWorkflow().do_task(second_task, workflow.study_id, workflow.id, workflow_name='two_user_tasks') workflow_api = self.get_workflow_api(workflow) task = workflow_api.next_task self.assertEqual('Task_GetName', task.name) def test_workflow_reset_missing_name(self): - workflow = self.create_workflow('reset_workflow') + workflow = self.create_workflow('two_user_tasks') workflow_api = self.get_workflow_api(workflow) first_task = workflow_api.next_task @@ -31,7 +37,7 @@ class TestWorkflowReset(BaseTest): ResetWorkflow().do_task(first_task, workflow.study_id, workflow.id) def test_workflow_reset_bad_name(self): - workflow = self.create_workflow('reset_workflow') + workflow = self.create_workflow('two_user_tasks') workflow_api = self.get_workflow_api(workflow) first_task = workflow_api.next_task From acae6030f5e360425c781b1d6d1da74ba4371a7b Mon Sep 17 00:00:00 2001 From: Dan Date: Tue, 15 Jun 2021 16:17:15 -0400 Subject: [PATCH 7/8] The API for evaluating python expressions should not raise an error, it can simply return a result of false, and provide an error to explain the problem. In this way the front end doesn't error out when it's running the script tasks but doesn't have enough information to get a valid response back. The validation should take into account that repeating sections must be evaluated in the context of the data within the repeating section, not outside of it. --- crc/api/tools.py | 5 +---- crc/services/workflow_service.py | 13 ++++++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/crc/api/tools.py b/crc/api/tools.py index 7904dfe4..b78d9636 100644 --- a/crc/api/tools.py +++ b/crc/api/tools.py @@ -87,10 +87,7 @@ def evaluate_python_expression(body): result = script_engine.eval(body['expression'], body['data']) return {"result": result, "expression": body['expression'], "data": body['data']} except Exception as e: - raise ApiError("expression_error", f"Failed to evaluate the expression '%s'. %s" % - (body['expression'], str(e)), - task_data = body["data"]) - + return {"result": False, "expression": body['expression'], "data": body['data'], "error": str(e)} def send_test_email(subject, address, message, data=None): rendered, wrapped = EmailService().get_rendered_content(message, data) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index db1a359f..da529450 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -7,6 +7,7 @@ from typing import List import jinja2 from SpiffWorkflow import Task as SpiffTask, WorkflowException, NavItem +from SpiffWorkflow.bpmn.PythonScriptEngine import Box from SpiffWorkflow.bpmn.specs.EndEvent import EndEvent from SpiffWorkflow.bpmn.specs.ManualTask import ManualTask from SpiffWorkflow.bpmn.specs.MultiInstanceTask import MultiInstanceTask @@ -291,8 +292,14 @@ class WorkflowService(object): @staticmethod def evaluate_property(property_name, field, task): expression = field.get_property(property_name) + data = task.data + if field.has_property(Task.FIELD_PROP_REPEAT): + # Then you must evaluate the expression based on the data within the group only. + group = field.get_property(Task.FIELD_PROP_REPEAT) + if group in task.data: + data = task.data[group][0] try: - return task.workflow.script_engine.evaluate_expression(task, expression) + return task.workflow.script_engine.eval(expression, data) except Exception as e: message = f"The field {field.id} contains an invalid expression. {e}" raise ApiError.from_task(f'invalid_{property_name}', message, task=task) @@ -373,7 +380,7 @@ class WorkflowService(object): if len(field.options) > 0: random_choice = random.choice(field.options) if isinstance(random_choice, dict): - return {'value': random_choice['id'], 'label': random_choice['name']} + return {'value': random_choice['id'], 'label': random_choice['name'], 'data': random_choice['data']} else: # fixme: why it is sometimes an EnumFormFieldOption, and other times not? return {'value': random_choice.id, 'label': random_choice.name} @@ -694,7 +701,7 @@ class WorkflowService(object): raise ApiError.from_task("invalid_enum", f"The label column '{label_column}' does not exist for item {item}", task=spiff_task) - options.append({"id": item[value_column], "name": item[label_column], "data": item}) + options.append(Box({"id": item[value_column], "name": item[label_column], "data": item})) return options @staticmethod From a4caae8d64483acedf4509ba1a1c6127e710ff58 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 16 Jun 2021 14:40:20 -0400 Subject: [PATCH 8/8] when validating, we need to take every step to remove the workflows we create during the validation. --- crc/services/workflow_service.py | 75 +++++++++---------- .../test_workflow_spec_validation_api.py | 13 ++-- 2 files changed, 43 insertions(+), 45 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index da529450..5c3f8f34 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -82,11 +82,12 @@ class WorkflowService(object): return workflow_model @staticmethod - def delete_test_data(): + def delete_test_data(workflow: WorkflowModel): + db.session.delete(workflow) + # Also, delete any test study or user models that may have been created. for study in db.session.query(StudyModel).filter(StudyModel.user_uid == "test"): StudyService.delete_study(study.id) db.session.commit() - user = db.session.query(UserModel).filter_by(uid="test").first() if user: db.session.delete(user) @@ -102,48 +103,42 @@ class WorkflowService(object): """ workflow_model = WorkflowService.make_test_workflow(spec_id, validate_study_id) - try: processor = WorkflowProcessor(workflow_model, validate_only=True) + + count = 0 + while not processor.bpmn_workflow.is_completed(): + processor.bpmn_workflow.get_deep_nav_list() # Assure no errors with navigation. + processor.bpmn_workflow.do_engine_steps() + tasks = processor.bpmn_workflow.get_tasks(SpiffTask.READY) + for task in tasks: + if task.task_spec.lane is not None and task.task_spec.lane not in task.data: + raise ApiError.from_task("invalid_role", + f"This task is in a lane called '{task.task_spec.lane}', The " + f" current task data must have information mapping this role to " + f" a unique user id.", task) + task_api = WorkflowService.spiff_task_to_api_task( + task, + add_docs_and_forms=True) # Assure we try to process the documentation, and raise those errors. + # make sure forms have a form key + if hasattr(task_api, 'form') and task_api.form is not None and task_api.form.key == '': + raise ApiError(code='missing_form_key', + message='Forms must include a Form Key.', + task_id=task.id, + task_name=task.get_name()) + WorkflowService._process_documentation(task) + WorkflowService.populate_form_with_random_data(task, task_api, required_only) + processor.complete_task(task) + count += 1 + if count >= 100: + raise ApiError.from_task(code='validation_loop', + message=f'There appears to be an infinite loop in the validation. Task is {task.task_spec.description}', + task=task) + WorkflowService._process_documentation(processor.bpmn_workflow.last_task.parent.parent) except WorkflowException as we: - WorkflowService.delete_test_data() raise ApiError.from_workflow_exception("workflow_validation_exception", str(we), we) - - count = 0 - while not processor.bpmn_workflow.is_completed(): - if count < 100: # check for infinite loop - try: - processor.bpmn_workflow.get_deep_nav_list() # Assure no errors with navigation. - processor.bpmn_workflow.do_engine_steps() - tasks = processor.bpmn_workflow.get_tasks(SpiffTask.READY) - for task in tasks: - if task.task_spec.lane is not None and task.task_spec.lane not in task.data: - raise ApiError.from_task("invalid_role", - f"This task is in a lane called '{task.task_spec.lane}', The " - f" current task data must have information mapping this role to " - f" a unique user id.", task) - task_api = WorkflowService.spiff_task_to_api_task( - task, - add_docs_and_forms=True) # Assure we try to process the documentation, and raise those errors. - # make sure forms have a form key - if hasattr(task_api, 'form') and task_api.form is not None and task_api.form.key == '': - raise ApiError(code='missing_form_key', - message='Forms must include a Form Key.', - task_id=task.id, - task_name=task.get_name()) - WorkflowService.populate_form_with_random_data(task, task_api, required_only) - processor.complete_task(task) - count += 1 - except WorkflowException as we: - WorkflowService.delete_test_data() - raise ApiError.from_workflow_exception("workflow_validation_exception", str(we), we) - else: - raise ApiError.from_task(code='validation_loop', - message=f'There appears to be an infinite loop in the validation. Task is {task.task_spec.description}', - task=task) - - WorkflowService.delete_test_data() - WorkflowService._process_documentation(processor.bpmn_workflow.last_task.parent.parent) + finally: + WorkflowService.delete_test_data(workflow_model) return processor.bpmn_workflow.last_task.data @staticmethod diff --git a/tests/workflow/test_workflow_spec_validation_api.py b/tests/workflow/test_workflow_spec_validation_api.py index 688823b7..b7cb5af3 100644 --- a/tests/workflow/test_workflow_spec_validation_api.py +++ b/tests/workflow/test_workflow_spec_validation_api.py @@ -2,12 +2,14 @@ import json import unittest from unittest.mock import patch +from sqlalchemy import func + from tests.base_test import BaseTest from crc import session, app from crc.api.common import ApiErrorSchema from crc.models.protocol_builder import ProtocolBuilderStudySchema -from crc.models.workflow import WorkflowSpecModel +from crc.models.workflow import WorkflowSpecModel, WorkflowModel from crc.services.workflow_service import WorkflowService @@ -15,8 +17,11 @@ class TestWorkflowSpecValidation(BaseTest): def validate_workflow(self, workflow_name): spec_model = self.load_test_spec(workflow_name) + total_workflows = session.query(WorkflowModel).count() rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) self.assert_success(rv) + total_workflows_after = session.query(WorkflowModel).count() + self.assertEqual(total_workflows, total_workflows_after, "No rogue workflow exists after validation.") json_data = json.loads(rv.get_data(as_text=True)) return ApiErrorSchema(many=True).load(json_data) @@ -59,10 +64,7 @@ class TestWorkflowSpecValidation(BaseTest): workflows = session.query(WorkflowSpecModel).all() errors = [] for w in workflows: - rv = self.app.get('/v1.0/workflow-specification/%s/validate' % w.id, - headers=self.logged_in_headers()) - self.assert_success(rv) - json_data = json.loads(rv.get_data(as_text=True)) + json_data = self.validate_workflow(w.name) errors.extend(ApiErrorSchema(many=True).load(json_data)) self.assertEqual(0, len(errors), json.dumps(errors)) @@ -87,6 +89,7 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual("StartEvent_1", errors[0]['task_id']) self.assertEqual("invalid_spec.bpmn", errors[0]['file_name']) + def test_invalid_script(self): self.load_example_data() errors = self.validate_workflow("invalid_script")