From 860c475b29120497226bc1dc2f271e8dcb35c337 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Sat, 30 May 2020 15:37:04 -0400 Subject: [PATCH] Fill out repeating sections during validation process. Also, when returning error messages, attempt to include the task data for the task that caused the error. Also, when attempting to delete any file, respond with an API error explaining the issue, and log the details. --- crc/api/common.py | 7 +- crc/models/api_models.py | 1 + crc/services/file_service.py | 27 +++-- crc/services/study_service.py | 3 + crc/services/workflow_service.py | 117 +++++++++++++-------- tests/data/repeat_form/repeat_form.bpmn | 47 +++++++++ tests/test_workflow_spec_validation_api.py | 16 ++- 7 files changed, 159 insertions(+), 59 deletions(-) create mode 100644 tests/data/repeat_form/repeat_form.bpmn diff --git a/crc/api/common.py b/crc/api/common.py index 2cd09522..b89dd8d5 100644 --- a/crc/api/common.py +++ b/crc/api/common.py @@ -3,7 +3,7 @@ from crc import ma, app class ApiError(Exception): def __init__(self, code, message, status_code=400, - file_name="", task_id="", task_name="", tag=""): + file_name="", task_id="", task_name="", tag="", task_data = {}): self.status_code = status_code self.code = code # a short consistent string describing the error. self.message = message # A detailed message that provides more information. @@ -11,6 +11,7 @@ class ApiError(Exception): self.task_name = task_name or "" # OPTIONAL: The name of the task in the BPMN Diagram. self.file_name = file_name or "" # OPTIONAL: The file that caused the error. self.tag = tag or "" # OPTIONAL: The XML Tag that caused the issue. + self.task_data = task_data or "" # OPTIONAL: A snapshot of data connected to the task when error ocurred. Exception.__init__(self, self.message) @classmethod @@ -20,6 +21,7 @@ class ApiError(Exception): instance.task_id = task.task_spec.name or "" instance.task_name = task.task_spec.description or "" instance.file_name = task.workflow.spec.file or "" + instance.task_data = task.data return instance @classmethod @@ -35,7 +37,8 @@ class ApiError(Exception): class ApiErrorSchema(ma.Schema): class Meta: - fields = ("code", "message", "workflow_name", "file_name", "task_name", "task_id") + fields = ("code", "message", "workflow_name", "file_name", "task_name", "task_id", + "task_data") @app.errorhandler(ApiError) diff --git a/crc/models/api_models.py b/crc/models/api_models.py index 4b279965..d53e43bc 100644 --- a/crc/models/api_models.py +++ b/crc/models/api_models.py @@ -31,6 +31,7 @@ class NavigationItem(object): class Task(object): + PROP_OPTIONS_REPEAT = "repeat" PROP_OPTIONS_FILE = "spreadsheet.name" PROP_OPTIONS_VALUE_COLUMN = "spreadsheet.value.column" PROP_OPTIONS_LABEL_COL = "spreadsheet.label.column" diff --git a/crc/services/file_service.py b/crc/services/file_service.py index beb22831..273460c1 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -5,11 +5,13 @@ from datetime import datetime from uuid import UUID from xml.etree import ElementTree +import flask from SpiffWorkflow.bpmn.parser.ValidationException import ValidationException from pandas import ExcelFile from sqlalchemy import desc +from sqlalchemy.exc import IntegrityError -from crc import session +from crc import session, app from crc.api.common import ApiError from crc.models.file import FileType, FileDataModel, FileModel, LookupFileModel, LookupDataModel from crc.models.workflow import WorkflowSpecModel, WorkflowModel, WorkflowSpecDependencyFile @@ -295,12 +297,17 @@ class FileService(object): @staticmethod def delete_file(file_id): - data_models = session.query(FileDataModel).filter_by(file_model_id=file_id).all() - for dm in data_models: - lookup_files = session.query(LookupFileModel).filter_by(file_data_model_id=dm.id).all() - for lf in lookup_files: - session.query(LookupDataModel).filter_by(lookup_file_model_id=lf.id).delete() - session.query(LookupFileModel).filter_by(id=lf.id).delete() - session.query(FileDataModel).filter_by(file_model_id=file_id).delete() - session.query(FileModel).filter_by(id=file_id).delete() - session.commit() + try: + data_models = session.query(FileDataModel).filter_by(file_model_id=file_id).all() + for dm in data_models: + lookup_files = session.query(LookupFileModel).filter_by(file_data_model_id=dm.id).all() + for lf in lookup_files: + session.query(LookupDataModel).filter_by(lookup_file_model_id=lf.id).delete() + session.query(LookupFileModel).filter_by(id=lf.id).delete() + session.query(FileDataModel).filter_by(file_model_id=file_id).delete() + session.query(FileModel).filter_by(id=file_id).delete() + session.commit() + except IntegrityError as ie: + app.logger.error("Failed to delete file: %i, due to %s" % (file_id, str(ie))) + raise ApiError('file_integrity_error', "You are attempting to delete a file that is " + "required by other records in the system.") \ No newline at end of file diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 98a8d15a..424a911f 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -4,6 +4,7 @@ from typing import List import requests from SpiffWorkflow import WorkflowException +from SpiffWorkflow.exceptions import WorkflowTaskExecException from ldap3.core.exceptions import LDAPSocketOpenError from crc import db, session, app @@ -309,6 +310,8 @@ class StudyService(object): for workflow_spec in new_specs: try: StudyService._create_workflow_model(study_model, workflow_spec) + except WorkflowTaskExecException as wtee: + errors.append(ApiError.from_task("workflow_execution_exception", str(wtee), wtee.task)) except WorkflowException as we: errors.append(ApiError.from_task_spec("workflow_execution_exception", str(we), we.sender)) return errors diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index c6cb8638..cf40b84d 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -7,8 +7,9 @@ from SpiffWorkflow import Task as SpiffTask, WorkflowException from SpiffWorkflow.bpmn.specs.ManualTask import ManualTask from SpiffWorkflow.bpmn.specs.ScriptTask import ScriptTask from SpiffWorkflow.bpmn.specs.UserTask import UserTask -from SpiffWorkflow.bpmn.workflow import BpmnWorkflow +from SpiffWorkflow.camunda.specs.UserTask import EnumFormField from SpiffWorkflow.dmn.specs.BusinessRuleTask import BusinessRuleTask +from SpiffWorkflow.exceptions import WorkflowTaskExecException from SpiffWorkflow.specs import CancelTask, StartTask from flask import g from jinja2 import Template @@ -17,7 +18,6 @@ from crc import db, app from crc.api.common import ApiError from crc.models.api_models import Task, MultiInstanceType from crc.models.file import LookupDataModel -from crc.models.protocol_builder import ProtocolBuilderStatus from crc.models.stats import TaskEventModel from crc.models.study import StudyModel from crc.models.user import UserModel @@ -39,7 +39,9 @@ class WorkflowService(object): the workflow Processor should be hidden behind this service. This will help maintain a structure that avoids circular dependencies. But for now, this contains tools for converting spiff-workflow models into our - own API models with additional information and capabilities.""" + own API models with additional information and capabilities and + handles the testing of a workflow specification by completing it with + random selections, attempting to mimic a front end as much as possible. """ @staticmethod def make_test_workflow(spec_id): @@ -61,17 +63,25 @@ class WorkflowService(object): for study in db.session.query(StudyModel).filter(StudyModel.user_uid=="test"): StudyService.delete_study(study.id) db.session.commit() - db.session.query(UserModel).filter_by(uid="test").delete() + + user = db.session.query(UserModel).filter_by(uid="test").first() + if user: + db.session.delete(user) @staticmethod def test_spec(spec_id): - """Runs a spec through it's paces to see if it results in any errors. Not fool-proof, but a good - sanity check.""" + """Runs a spec through it's paces to see if it results in any errors. + Not fool-proof, but a good sanity check. Returns the final data + output form the last task if successful. """ workflow_model = WorkflowService.make_test_workflow(spec_id) try: processor = WorkflowProcessor(workflow_model, validate_only=True) + except WorkflowTaskExecException as wtee: + WorkflowService.delete_test_data() + raise ApiError.from_task("workflow_execution_exception", str(wtee), + wtee.task) except WorkflowException as we: WorkflowService.delete_test_data() raise ApiError.from_task_spec("workflow_execution_exception", str(we), @@ -87,11 +97,17 @@ class WorkflowService(object): add_docs_and_forms=True) # Assure we try to process the documenation, and raise those errors. WorkflowService.populate_form_with_random_data(task, task_api) task.complete() + except WorkflowTaskExecException as wtee: + WorkflowService.delete_test_data() + raise ApiError.from_task("workflow_execution_exception", str(wtee), + wtee.task) except WorkflowException as we: WorkflowService.delete_test_data() raise ApiError.from_task_spec("workflow_execution_exception", str(we), we.sender) + WorkflowService.delete_test_data() + return processor.bpmn_workflow.last_task.data @staticmethod def populate_form_with_random_data(task, task_api): @@ -101,22 +117,35 @@ class WorkflowService(object): form_data = {} for field in task_api.form.fields: - if field.type == "enum": - if len(field.options) > 0: - random_choice = random.choice(field.options) - if isinstance(random_choice, dict): - form_data[field.id] = random.choice(field.options)['id'] - else: - # fixme: why it is sometimes an EnumFormFieldOption, and other times not? - form_data[field.id] = random_choice.id ## Assume it is an EnumFormFieldOption + if field.has_property(Task.PROP_OPTIONS_REPEAT): + group = field.get_property(Task.PROP_OPTIONS_REPEAT) + if group not in form_data: + form_data[group] = [{},{},{}] + for i in range(3): + form_data[group][i][field.id] = WorkflowService.get_random_data_for_field(field, task) + else: + form_data[field.id] = WorkflowService.get_random_data_for_field(field, task) + if task.data is None: + task.data = {} + task.data.update(form_data) + + @staticmethod + def get_random_data_for_field(field, task): + if field.type == "enum": + if len(field.options) > 0: + random_choice = random.choice(field.options) + if isinstance(random_choice, dict): + return random.choice(field.options)['id'] else: - raise ApiError.from_task("invalid_enum", "You specified an enumeration field (%s)," - " with no options" % field.id, - task) - elif field.type == "autocomplete": - lookup_model = LookupService.get_lookup_model(task, field) - if field.has_property(Task.PROP_LDAP_LOOKUP): - form_data[field.id] = { + # fixme: why it is sometimes an EnumFormFieldOption, and other times not? + return random_choice.id ## Assume it is an EnumFormFieldOption + else: + raise ApiError.from_task("invalid_enum", "You specified an enumeration field (%s)," + " with no options" % field.id, task) + elif field.type == "autocomplete": + lookup_model = LookupService.get_lookup_model(task, field) + if field.has_property(Task.PROP_LDAP_LOOKUP): # All ldap records get the same person. + return { "label": "dhf8r", "value": "Dan Funk", "data": { @@ -126,32 +155,30 @@ class WorkflowService(object): "email_address": "dhf8r@virginia.edu", "department": "Depertment of Psychocosmographictology", "affiliation": "Rousabout", - "sponsor_type": "Staff" + "sponsor_type": "Staff"} } - } - elif lookup_model: - data = db.session.query(LookupDataModel).filter( - LookupDataModel.lookup_file_model == lookup_model).limit(10).all() - options = [] - for d in data: - options.append({"id": d.value, "name": d.label}) - form_data[field.id] = random.choice(options) - else: - raise ApiError.from_task("invalid_autocomplete", "The settings for this auto complete field " - "are incorrect: %s " % field.id, task) - elif field.type == "long": - form_data[field.id] = random.randint(1, 1000) - elif field.type == 'boolean': - form_data[field.id] = random.choice([True, False]) - elif field.type == 'file': - form_data[field.id] = random.randint(1, 100) - elif field.type == 'files': - form_data[field.id] = random.randrange(1, 100) + elif lookup_model: + data = db.session.query(LookupDataModel).filter( + LookupDataModel.lookup_file_model == lookup_model).limit(10).all() + options = [] + for d in data: + options.append({"id": d.value, "name": d.label}) + return random.choice(options) else: - form_data[field.id] = WorkflowService._random_string() - if task.data is None: - task.data = {} - task.data.update(form_data) + raise ApiError.from_task("invalid_autocomplete", "The settings for this auto complete field " + "are incorrect: %s " % field.id, task) + elif field.type == "long": + return random.randint(1, 1000) + elif field.type == 'boolean': + return random.choice([True, False]) + elif field.type == 'file': + # fixme: produce some something sensible for files. + return random.randint(1, 100) + # fixme: produce some something sensible for files. + elif field.type == 'files': + return random.randrange(1, 100) + else: + return WorkflowService._random_string() def __get_options(self): pass diff --git a/tests/data/repeat_form/repeat_form.bpmn b/tests/data/repeat_form/repeat_form.bpmn new file mode 100644 index 00000000..f0e3f922 --- /dev/null +++ b/tests/data/repeat_form/repeat_form.bpmn @@ -0,0 +1,47 @@ + + + + + SequenceFlow_0lvudp8 + + + + SequenceFlow_02vev7n + + + + + + + + + + + + + SequenceFlow_0lvudp8 + SequenceFlow_02vev7n + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/test_workflow_spec_validation_api.py b/tests/test_workflow_spec_validation_api.py index 9e581874..d46746dc 100644 --- a/tests/test_workflow_spec_validation_api.py +++ b/tests/test_workflow_spec_validation_api.py @@ -3,17 +3,16 @@ from unittest.mock import patch from tests.base_test import BaseTest -from crc.services.protocol_builder import ProtocolBuilderService 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.services.workflow_service import WorkflowService class TestWorkflowSpecValidation(BaseTest): def validate_workflow(self, workflow_name): - self.load_example_data() spec_model = self.load_test_spec(workflow_name) rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) self.assert_success(rv) @@ -22,6 +21,7 @@ class TestWorkflowSpecValidation(BaseTest): def test_successful_validation_of_test_workflows(self): app.config['PB_ENABLED'] = False # Assure this is disabled. + self.load_example_data() self.assertEqual(0, len(self.validate_workflow("parallel_tasks"))) self.assertEqual(0, len(self.validate_workflow("decision_table"))) self.assertEqual(0, len(self.validate_workflow("docx"))) @@ -60,6 +60,7 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual(0, len(errors), json.dumps(errors)) def test_invalid_expression(self): + self.load_example_data() errors = self.validate_workflow("invalid_expression") self.assertEqual(1, len(errors)) self.assertEqual("workflow_execution_exception", errors[0]['code']) @@ -68,8 +69,11 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual("invalid_expression.bpmn", errors[0]['file_name']) self.assertEqual('ExclusiveGateway_003amsm: Error evaluating expression \'this_value_does_not_exist==true\', ' 'name \'this_value_does_not_exist\' is not defined', errors[0]["message"]) + self.assertIsNotNone(errors[0]['task_data']) + self.assertIn("has_bananas", errors[0]['task_data']) def test_validation_error(self): + self.load_example_data() errors = self.validate_workflow("invalid_spec") self.assertEqual(1, len(errors)) self.assertEqual("workflow_validation_error", errors[0]['code']) @@ -77,6 +81,7 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual("invalid_spec.bpmn", errors[0]['file_name']) def test_invalid_script(self): + self.load_example_data() errors = self.validate_workflow("invalid_script") self.assertEqual(1, len(errors)) self.assertEqual("workflow_execution_exception", errors[0]['code']) @@ -84,3 +89,10 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual("Invalid_Script_Task", errors[0]['task_id']) self.assertEqual("An Invalid Script Reference", errors[0]['task_name']) self.assertEqual("invalid_script.bpmn", errors[0]['file_name']) + + def test_repeating_sections_correctly_populated(self): + self.load_example_data() + spec_model = self.load_test_spec('repeat_form') + final_data = WorkflowService.test_spec(spec_model.id) + self.assertIsNotNone(final_data) + self.assertIn('cats', final_data) \ No newline at end of file