From db448827339cbf6f6cdffbf358dc9ee086eadbba Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 1 Jul 2021 15:38:45 -0400 Subject: [PATCH 01/14] When we encounter a name error running a script, look in the task data and see if there is a variable with a similar name. If a variable with a similar name exists, add it to the ApiError as a hint --- crc/services/workflow_processor.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/crc/services/workflow_processor.py b/crc/services/workflow_processor.py index 884c12c2..fe6c8824 100644 --- a/crc/services/workflow_processor.py +++ b/crc/services/workflow_processor.py @@ -30,6 +30,8 @@ from crc.services.file_service import FileService from crc import app from crc.services.user_service import UserService +from difflib import SequenceMatcher + class CustomBpmnScriptEngine(BpmnScriptEngine): """This is a custom script processor that can be easily injected into Spiff Workflow. It will execute python code read in from the bpmn. It will also make any scripts in the @@ -64,9 +66,21 @@ class CustomBpmnScriptEngine(BpmnScriptEngine): f'please correct the following:' f' {script}, {e.msg}') except NameError as e: - raise ApiError('name_error', - f'something you are referencing does not exist:' - f' {script}, {e}') + def get_most_similar(task_data, name_error): + bad_variable = str(name_error)[6:-16] + highest_ratio = 0 + most_similar = None + for item in task_data: + ratio = SequenceMatcher(None, item, bad_variable).ratio() + if ratio > highest_ratio: + most_similar = item + highest_ratio = ratio + return most_similar, int(highest_ratio*100) + most_similar, highest_ratio = get_most_similar(data, e) + error_message = f'something you are referencing does not exist: {script}, {e}.' + if highest_ratio > 50: + error_message += f' Did you mean \'{most_similar}\'?' + raise ApiError('name_error', error_message) def evaluate_expression(self, task, expression): From 2a4323012163eae6dbc03c1cc9829f643d15b19b Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 1 Jul 2021 15:39:25 -0400 Subject: [PATCH 02/14] Test and workflow for the new name error hint code --- .../script_with_name_error.bpmn | 57 +++++++++++++++++++ .../workflow/test_workflow_name_error_hint.py | 12 ++++ 2 files changed, 69 insertions(+) create mode 100644 tests/data/script_with_name_error/script_with_name_error.bpmn create mode 100644 tests/workflow/test_workflow_name_error_hint.py diff --git a/tests/data/script_with_name_error/script_with_name_error.bpmn b/tests/data/script_with_name_error/script_with_name_error.bpmn new file mode 100644 index 00000000..a90aed4e --- /dev/null +++ b/tests/data/script_with_name_error/script_with_name_error.bpmn @@ -0,0 +1,57 @@ + + + + + Flow_13jyds8 + + + + Flow_18kybym + + + + Flow_1jqzan6 + Flow_18kybym + print(ham) + + + + + + + + + + Flow_13jyds8 + Flow_1jqzan6 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_name_error_hint.py b/tests/workflow/test_workflow_name_error_hint.py new file mode 100644 index 00000000..8c09fca9 --- /dev/null +++ b/tests/workflow/test_workflow_name_error_hint.py @@ -0,0 +1,12 @@ +from tests.base_test import BaseTest +import json + + +class TestNameErrorHint(BaseTest): + + def test_name_error_hint(self): + self.load_example_data() + spec_model = self.load_test_spec('script_with_name_error') + rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) + json_data = json.loads(rv.get_data(as_text=True)) + self.assertIn('Did you mean \'spam\'', json_data[0]['message']) From cd26654b3a78ec30c8f7f7343eebce3429ad197b Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 2 Jul 2021 15:21:35 -0400 Subject: [PATCH 03/14] Script to call new pb mock api endpoint `check_study` --- crc/scripts/check_study.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 crc/scripts/check_study.py diff --git a/crc/scripts/check_study.py b/crc/scripts/check_study.py new file mode 100644 index 00000000..e0c086ab --- /dev/null +++ b/crc/scripts/check_study.py @@ -0,0 +1,23 @@ +from crc.scripts.script import Script +from crc.api.common import ApiError +from crc.services.protocol_builder import ProtocolBuilderService + + +class CheckStudy(Script): + + pb = ProtocolBuilderService() + + def get_description(self): + pass + + def do_task_validate_only(self, task, study_id, workflow_id, *args, **kwargs): + pass + + def do_task(self, task, study_id, workflow_id, *args, **kwargs): + check_study = self.pb.check_study(study_id) + if check_study: + return check_study + else: + raise ApiError.from_task(code='missing_check_study', + message='There was a problem checking information for this study.', + task=task) From 9690c69b6cd0b735d9d9ba9df0a2e490112618c0 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 2 Jul 2021 15:25:33 -0400 Subject: [PATCH 04/14] added check_study method to protocol_builder service for new endpoint --- config/default.py | 1 + crc/services/protocol_builder.py | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/config/default.py b/config/default.py index fe96d8f5..47d5e7a0 100644 --- a/config/default.py +++ b/config/default.py @@ -63,6 +63,7 @@ PB_REQUIRED_DOCS_URL = environ.get('PB_REQUIRED_DOCS_URL', default=PB_BASE_URL + PB_STUDY_DETAILS_URL = environ.get('PB_STUDY_DETAILS_URL', default=PB_BASE_URL + "study?studyid=%i") PB_SPONSORS_URL = environ.get('PB_SPONSORS_URL', default=PB_BASE_URL + "sponsors?studyid=%i") PB_IRB_INFO_URL = environ.get('PB_IRB_INFO_URL', default=PB_BASE_URL + "current_irb_info/%i") +PB_CHECK_STUDY_URL = environ.get('PB_CHECK_STUDY_URL', default=PB_BASE_URL + "check_study/%i") # Ldap Configuration LDAP_URL = environ.get('LDAP_URL', default="ldap.virginia.edu").strip('/') # No trailing slash or http:// diff --git a/crc/services/protocol_builder.py b/crc/services/protocol_builder.py index 6107280b..6806204d 100644 --- a/crc/services/protocol_builder.py +++ b/crc/services/protocol_builder.py @@ -15,6 +15,7 @@ class ProtocolBuilderService(object): STUDY_DETAILS_URL = app.config['PB_STUDY_DETAILS_URL'] SPONSORS_URL = app.config['PB_SPONSORS_URL'] IRB_INFO_URL = app.config['PB_IRB_INFO_URL'] + CHECK_STUDY_URL = app.config['PB_CHECK_STUDY_URL'] @staticmethod def is_enabled(): @@ -64,6 +65,10 @@ class ProtocolBuilderService(object): def get_sponsors(study_id) -> {}: return ProtocolBuilderService.__make_request(study_id, ProtocolBuilderService.SPONSORS_URL) + @staticmethod + def check_study(study_id) -> {}: + return ProtocolBuilderService.__make_request(study_id, ProtocolBuilderService.CHECK_STUDY_URL) + @staticmethod def __enabled_or_raise(): if not ProtocolBuilderService.is_enabled(): From 2cb2874a49f7084e11c2da5d96326dcc0e7a1a4f Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 2 Jul 2021 15:26:39 -0400 Subject: [PATCH 05/14] Added test and json data for new check_study pb mock api endpoint --- tests/data/pb_responses/check_study.json | 3 +++ tests/test_protocol_builder.py | 10 ++++++++++ 2 files changed, 13 insertions(+) create mode 100644 tests/data/pb_responses/check_study.json diff --git a/tests/data/pb_responses/check_study.json b/tests/data/pb_responses/check_study.json new file mode 100644 index 00000000..eb701b75 --- /dev/null +++ b/tests/data/pb_responses/check_study.json @@ -0,0 +1,3 @@ +[ + {"DETAIL": "Passed validation.", "STATUS": "No Error"} +] \ No newline at end of file diff --git a/tests/test_protocol_builder.py b/tests/test_protocol_builder.py index 8bd1f66f..c64f83e5 100644 --- a/tests/test_protocol_builder.py +++ b/tests/test_protocol_builder.py @@ -72,3 +72,13 @@ class TestProtocolBuilder(BaseTest): self.assertEqual('IRB Event 1', response[0]["IRBEVENT"]) self.assertEqual('IRB Event 2', response[1]["IRBEVENT"]) self.assertEqual('IRB Event 3', response[2]["IRBEVENT"]) + + @patch('crc.services.protocol_builder.requests.get') + def test_check_study(self, mock_get): + app.config['PB_ENABLED'] = True + mock_get.return_value.ok = True + mock_get.return_value.text = self.protocol_builder_response('check_study.json') + response = ProtocolBuilderService.check_study(self.test_study_id) + self.assertIsNotNone(response) + self.assertIn('DETAIL', response[0].keys()) + self.assertIn('STATUS', response[0].keys()) From f647390e1c9139cd7124e2f0790c6022cfec59f0 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 2 Jul 2021 15:51:24 -0400 Subject: [PATCH 06/14] Added description and validate_only --- crc/scripts/check_study.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crc/scripts/check_study.py b/crc/scripts/check_study.py index e0c086ab..68237421 100644 --- a/crc/scripts/check_study.py +++ b/crc/scripts/check_study.py @@ -1,6 +1,7 @@ from crc.scripts.script import Script from crc.api.common import ApiError from crc.services.protocol_builder import ProtocolBuilderService +from crc.services.study_service import StudyService class CheckStudy(Script): @@ -8,10 +9,16 @@ class CheckStudy(Script): pb = ProtocolBuilderService() def get_description(self): - pass + return """Returns the Check Study data for a Study""" def do_task_validate_only(self, task, study_id, workflow_id, *args, **kwargs): - pass + study = StudyService.get_study(study_id) + if study: + return {"DETAIL": "Passed validation.", "STATUS": "No Error"} + else: + raise ApiError.from_task(code='bad_study', + message=f'No study for study_id {study_id}', + task=task) def do_task(self, task, study_id, workflow_id, *args, **kwargs): check_study = self.pb.check_study(study_id) From 8145ff9025dcd1267357c954fed495a72a769aa7 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 2 Jul 2021 16:14:19 -0400 Subject: [PATCH 07/14] Tests for calling the script --- .../check_study_script.bpmn | 53 +++++++++++++++++++ tests/scripts/test_check_study.py | 25 +++++++++ 2 files changed, 78 insertions(+) create mode 100644 tests/data/check_study_script/check_study_script.bpmn create mode 100644 tests/scripts/test_check_study.py diff --git a/tests/data/check_study_script/check_study_script.bpmn b/tests/data/check_study_script/check_study_script.bpmn new file mode 100644 index 00000000..91cfc790 --- /dev/null +++ b/tests/data/check_study_script/check_study_script.bpmn @@ -0,0 +1,53 @@ + + + + + Flow_17nzcku + + + + Flow_17nzcku + Flow_0oozrfg + check_study = check_study() + + + + # Check Study +<div><span>{{check_study}}</span></div> + Flow_0oozrfg + Flow_10sc31i + + + Flow_10sc31i + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/scripts/test_check_study.py b/tests/scripts/test_check_study.py new file mode 100644 index 00000000..e05afbd2 --- /dev/null +++ b/tests/scripts/test_check_study.py @@ -0,0 +1,25 @@ +from tests.base_test import BaseTest +from crc import app +from unittest.mock import patch + + +class TestCheckStudy(BaseTest): + + def test_check_study_script_validation(self): + self.load_example_data() + spec_model = self.load_test_spec('check_study_script') + rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) + self.assertEqual([], rv.json) + + @patch('crc.services.protocol_builder.requests.get') + def test_check_study(self, mock_get): + app.config['PB_ENABLED'] = True + app.config['PB_ENABLED'] = True + mock_get.return_value.ok = True + mock_get.return_value.text = self.protocol_builder_response('check_study.json') + workflow = self.create_workflow('check_study_script') + workflow_api = self.get_workflow_api(workflow) + task = workflow_api.next_task + + self.assertIn('DETAIL', task.documentation) + self.assertIn('STATUS', task.documentation) From 9803a04d6dd65a54e591822e4ad841d3c0b04e46 Mon Sep 17 00:00:00 2001 From: Kelly McDonald Date: Tue, 6 Jul 2021 11:46:47 -0400 Subject: [PATCH 08/14] Make a change so that anything that has a waiting event is labeled in the database as waiting, even if it is sitting around waiting on a user input task that is ready --- crc/services/workflow_processor.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crc/services/workflow_processor.py b/crc/services/workflow_processor.py index 96e3ef40..344314fa 100644 --- a/crc/services/workflow_processor.py +++ b/crc/services/workflow_processor.py @@ -7,7 +7,7 @@ import shlex from datetime import datetime from typing import List -from SpiffWorkflow import Task as SpiffTask, WorkflowException +from SpiffWorkflow import Task as SpiffTask, WorkflowException, Task from SpiffWorkflow.bpmn.BpmnScriptEngine import BpmnScriptEngine from SpiffWorkflow.bpmn.parser.ValidationException import ValidationException from SpiffWorkflow.bpmn.serializer.BpmnSerializer import BpmnSerializer @@ -343,6 +343,9 @@ class WorkflowProcessor(object): if bpmn_workflow.is_completed(): return WorkflowStatus.complete user_tasks = bpmn_workflow.get_ready_user_tasks() + waiting_tasks = bpmn_workflow.get_tasks(Task.WAITING) + if len(waiting_tasks) > 0: + return WorkflowStatus.waiting if len(user_tasks) > 0: return WorkflowStatus.user_input_required else: From 23be257db023c787db1c2384251642dca4e29720 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 6 Jul 2021 17:07:47 -0400 Subject: [PATCH 09/14] Add user_uid column to file_data table --- crc/models/file.py | 1 + migrations/versions/30e017a03948_.py | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 migrations/versions/30e017a03948_.py diff --git a/crc/models/file.py b/crc/models/file.py index f52b499d..6379f22d 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -69,6 +69,7 @@ class FileDataModel(db.Model): date_created = db.Column(db.DateTime(timezone=True), server_default=func.now()) file_model_id = db.Column(db.Integer, db.ForeignKey('file.id')) file_model = db.relationship("FileModel", foreign_keys=[file_model_id]) + user_uid = db.Column(db.String, db.ForeignKey('user.uid'), nullable=True) diff --git a/migrations/versions/30e017a03948_.py b/migrations/versions/30e017a03948_.py new file mode 100644 index 00000000..248b3db4 --- /dev/null +++ b/migrations/versions/30e017a03948_.py @@ -0,0 +1,27 @@ +"""add user_uid column to file_data table + +Revision ID: 30e017a03948 +Revises: bbf064082623 +Create Date: 2021-07-06 10:39:04.661704 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '30e017a03948' +down_revision = 'bbf064082623' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('file_data', sa.Column('user_uid', sa.String(), nullable=True)) + op.create_foreign_key(None, 'file_data', 'user', ['user_uid'], ['uid']) + + +def downgrade(): + # op.drop_constraint('file_data_user_uid_fkey', 'file_data', type_='foreignkey') + # op.execute("update file_data set user_uid = NULL WHERE user_uid IS NOT NULL") + op.drop_column('file_data', 'user_uid') From 894377a607fe10629494d65872ef6110c580604c Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 6 Jul 2021 17:09:00 -0400 Subject: [PATCH 10/14] Add user_uid to FileDataModel calls --- crc/services/file_service.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 0ca94b0c..f3971a7c 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -21,6 +21,7 @@ from crc.models.data_store import DataStoreModel from crc.models.file import FileType, FileDataModel, FileModel, LookupFileModel, LookupDataModel from crc.models.workflow import WorkflowSpecModel, WorkflowModel, WorkflowSpecDependencyFile from crc.services.cache_service import cache +from crc.services.user_service import UserService import re @@ -229,10 +230,14 @@ class FileService(object): except XMLSyntaxError as xse: raise ApiError("invalid_xml", "Failed to parse xml: " + str(xse), file_name=file_model.name) + try: + user_uid = UserService.current_user().uid + except ApiError as ae: + user_uid = None new_file_data_model = FileDataModel( data=binary_data, file_model_id=file_model.id, file_model=file_model, version=version, md5_hash=md5_checksum, date_created=datetime.utcnow(), - size=size + size=size, user_uid=user_uid ) session.add_all([file_model, new_file_data_model]) session.commit() From c11b5cfb29fd04fe053d4c0f3129666641f19d02 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 7 Jul 2021 08:18:02 -0400 Subject: [PATCH 11/14] Add user_uid to api output --- crc/models/file.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crc/models/file.py b/crc/models/file.py index 6379f22d..89646015 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -116,6 +116,7 @@ class File(object): instance.last_modified = data_model.date_created instance.latest_version = data_model.version instance.size = data_model.size + instance.user_uid = data_model.user_uid else: instance.last_modified = None instance.latest_version = None @@ -143,7 +144,7 @@ class FileSchema(Schema): fields = ["id", "name", "is_status", "is_reference", "content_type", "primary", "primary_process_id", "workflow_spec_id", "workflow_id", "irb_doc_code", "last_modified", "latest_version", "type", "size", "data_store", - "document"] + "document", "user_uid"] unknown = INCLUDE type = EnumField(FileType) From 621c11fe247166dfebb909c2d3797fe515466fe5 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 7 Jul 2021 09:39:01 -0400 Subject: [PATCH 12/14] Add test for user_uid --- tests/files/test_files_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/files/test_files_api.py b/tests/files/test_files_api.py index c71cd4b3..ee075625 100644 --- a/tests/files/test_files_api.py +++ b/tests/files/test_files_api.py @@ -121,6 +121,7 @@ class TestFilesApi(BaseTest): self.assertEqual(FileType.xls, file.type) self.assertTrue(file.is_reference) self.assertEqual("application/vnd.ms-excel", file.content_type) + self.assertEqual('dhf8r', json_data['user_uid']) def test_set_reference_file_bad_extension(self): file_name = FileService.DOCUMENT_LIST From ac19c3e3c67f1d6f7c7f31babd2ea15a915f0c45 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 7 Jul 2021 10:33:30 -0400 Subject: [PATCH 13/14] Add users first --- tests/base_test.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/base_test.py b/tests/base_test.py index 5876e503..ea0e7c16 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -147,13 +147,6 @@ class BaseTest(unittest.TestCase): otherwise it depends on a small setup for running tests.""" from example_data import ExampleDataLoader ExampleDataLoader.clean_db() - if use_crc_data: - ExampleDataLoader().load_all() - elif use_rrt_data: - ExampleDataLoader().load_rrt() - else: - ExampleDataLoader().load_test_data() - # If in production mode, only add the first user. if app.config['PRODUCTION']: session.add(UserModel(**self.users[0])) @@ -161,6 +154,13 @@ class BaseTest(unittest.TestCase): for user_json in self.users: session.add(UserModel(**user_json)) + if use_crc_data: + ExampleDataLoader().load_all() + elif use_rrt_data: + ExampleDataLoader().load_rrt() + else: + ExampleDataLoader().load_test_data() + session.commit() for study_json in self.studies: study_model = StudyModel(**study_json) From bef35e4bec8888b8c55ff1bbcae2cd78f2fd21be Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 7 Jul 2021 11:34:33 -0400 Subject: [PATCH 14/14] Upgrading Spiffworkflow, just incorporating some additional tests. --- Pipfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Pipfile.lock b/Pipfile.lock index 554434bb..429d082e 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -979,7 +979,7 @@ }, "spiffworkflow": { "git": "https://github.com/sartography/SpiffWorkflow.git", - "ref": "66555b92ef1d8d9ce117b6f2ccf6aa248df9835f" + "ref": "59d12e25e5313977b83e7d65b6deb65572dee71c" }, "sqlalchemy": { "hashes": [