From 0c47d09972291cf518658b53e573cdd32a79e926 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 4 Jan 2021 10:53:21 -0500 Subject: [PATCH 1/7] Ticket 143. Test whether file is uploaded. --- crc/scripts/is_file_uploaded.py | 23 +++++++++++++++ tests/test_is_file_uploaded_script.py | 41 +++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 crc/scripts/is_file_uploaded.py create mode 100644 tests/test_is_file_uploaded_script.py diff --git a/crc/scripts/is_file_uploaded.py b/crc/scripts/is_file_uploaded.py new file mode 100644 index 00000000..ac525713 --- /dev/null +++ b/crc/scripts/is_file_uploaded.py @@ -0,0 +1,23 @@ +from crc.scripts.script import Script +from crc.services.file_service import FileService + + +class IsFileUploaded(Script): + + def get_description(self): + return """Test whether a file is uploaded for a study. + Pass in the IRB Doc Code for the file.""" + + def do_task_validate_only(self, task, study_id, workflow_id, *args, **kwargs): + doc_code = args[0] + files = FileService.get_files_for_study(study_id) + + def do_task(self, task, study_id, workflow_id, *args, **kwargs): + + files = FileService.get_files_for_study(study_id) + if len(files) > 0: + doc_code = args[0] + for file in files: + if doc_code == file.irb_doc_code: + return True + return False diff --git a/tests/test_is_file_uploaded_script.py b/tests/test_is_file_uploaded_script.py new file mode 100644 index 00000000..3a6d15f5 --- /dev/null +++ b/tests/test_is_file_uploaded_script.py @@ -0,0 +1,41 @@ +from tests.base_test import BaseTest +from crc.services.file_service import FileService +from crc.scripts.is_file_uploaded import IsFileUploaded + + +class TestIsFileUploaded(BaseTest): + + def test_file_uploaded_pass(self): + self.load_example_data() + irb_code_1 = 'UVACompl_PRCAppr' + irb_code_2 = 'Study_App_Doc' + + workflow = self.create_workflow('empty_workflow') + first_task = self.get_workflow_api(workflow).next_task + study_id = workflow.study_id + + # We shouldn't have any files yet. + files = FileService.get_files_for_study(study_id) + self.assertEqual(0, len(files)) + self.assertEqual(False, IsFileUploaded.do_task(IsFileUploaded, first_task, study_id, workflow.id, irb_code_1)) + + # Add a file + FileService.add_workflow_file(workflow_id=workflow.id, + name="something.png", content_type="text", + binary_data=b'1234', irb_doc_code=irb_code_1) + + # Make sure we find the file + files = FileService.get_files_for_study(study_id) + self.assertEqual(1, len(files)) + self.assertEqual(True, IsFileUploaded.do_task(IsFileUploaded, first_task, study_id, workflow.id, irb_code_1)) + + # Add second file + FileService.add_workflow_file(workflow_id=workflow.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code=irb_code_2) + + # Make sure we find both files. + files = FileService.get_files_for_study(study_id) + self.assertEqual(2, len(files)) + self.assertEqual(True, IsFileUploaded.do_task(IsFileUploaded, first_task, study_id, workflow.id, irb_code_1)) + self.assertEqual(True, IsFileUploaded.do_task(IsFileUploaded, first_task, study_id, workflow.id, irb_code_2)) From b0dc8346825d3591cadf145a61833989dda7d358 Mon Sep 17 00:00:00 2001 From: Dan Date: Mon, 4 Jan 2021 13:41:57 -0500 Subject: [PATCH 2/7] Don't error out on autocomplete_num --- crc/models/api_models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/crc/models/api_models.py b/crc/models/api_models.py index 90991dd5..1ff623f7 100644 --- a/crc/models/api_models.py +++ b/crc/models/api_models.py @@ -28,6 +28,7 @@ class Task(object): # Autocomplete field FIELD_TYPE_AUTO_COMPLETE = "autocomplete" + FIELD_TYPE_AUTO_COMPLETE_MAX = "autocomplete_num" # Not used directly, passed in from the front end. # Required field FIELD_CONSTRAINT_REQUIRED = "required" From 0ae4448fbe837faa6722c6d444de9fde979822f8 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 4 Jan 2021 13:48:34 -0500 Subject: [PATCH 3/7] Fixed ApiError call. Now includes task_id and task_name. --- crc/services/workflow_service.py | 4 +++- tests/workflow/test_workflow_form_field_type.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index a30622a2..e9aec982 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -130,7 +130,9 @@ class WorkflowService(object): # Assure we have a field type if field.type is None: raise ApiError(code='invalid_form_data', - message='Field type is None. A field type must be provided.') + message=f'Type is missing for field "{field.id}". A field type must be provided.', + task_id=task.id, + task_name=task.get_name()) # Assure field has valid properties WorkflowService.check_field_properties(field, task) diff --git a/tests/workflow/test_workflow_form_field_type.py b/tests/workflow/test_workflow_form_field_type.py index 406d2669..48ec6533 100644 --- a/tests/workflow/test_workflow_form_field_type.py +++ b/tests/workflow/test_workflow_form_field_type.py @@ -10,5 +10,5 @@ class TestFormFieldType(BaseTest): json_data = json.loads(rv.get_data(as_text=True)) self.assertEqual(json_data[0]['message'], - 'When populating all fields ... Field type is None. A field type must be provided.') + 'When populating all fields ... Type is missing for field "name". A field type must be provided.') # print('TestFormFieldType: Good Form') From c9a4a9685f6a4e18b34c875dc55e7b740ce80336 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 4 Jan 2021 15:47:45 -0500 Subject: [PATCH 4/7] Users cannot upload a workflow_spec_file that already exists. We raise an error that is displayed on the fron end. --- crc/services/file_service.py | 22 +++++++++---- .../test_duplicate_workflow_spec_file.py | 32 +++++++++++++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) create mode 100644 tests/workflow/test_duplicate_workflow_spec_file.py diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 1bb42794..d39a45fc 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -35,14 +35,22 @@ class FileService(object): def add_workflow_spec_file(workflow_spec: WorkflowSpecModel, name, content_type, binary_data, primary=False, is_status=False): """Create a new file and associate it with a workflow spec.""" - file_model = FileModel( - workflow_spec_id=workflow_spec.id, - name=name, - primary=primary, - is_status=is_status, - ) + # Raise ApiError if the file already exists + if session.query(FileModel)\ + .filter(FileModel.workflow_spec_id == workflow_spec.id)\ + .filter(FileModel.name == name).first(): - return FileService.update_file(file_model, binary_data, content_type) + raise ApiError(code="Duplicate File", + message='If you want to replace the file, use the update mechanism.') + else: + file_model = FileModel( + workflow_spec_id=workflow_spec.id, + name=name, + primary=primary, + is_status=is_status, + ) + + return FileService.update_file(file_model, binary_data, content_type) @staticmethod def is_allowed_document(code): diff --git a/tests/workflow/test_duplicate_workflow_spec_file.py b/tests/workflow/test_duplicate_workflow_spec_file.py new file mode 100644 index 00000000..0e4d82fc --- /dev/null +++ b/tests/workflow/test_duplicate_workflow_spec_file.py @@ -0,0 +1,32 @@ +from tests.base_test import BaseTest +from crc import session +from crc.api.common import ApiError +from crc.models.workflow import WorkflowSpecModel +from crc.services.file_service import FileService + + +class TestDuplicateWorkflowSpecFile(BaseTest): + + def test_duplicate_workflow_spec_file(self): + # We want this to fail. + # Users should not be able to upload a file that already exists. + + self.load_example_data() + spec = session.query(WorkflowSpecModel).first() + + # Add a file + file_model = FileService.add_workflow_spec_file(spec, + name="something.png", + content_type="text", + binary_data=b'1234') + self.assertEqual(file_model.name, 'something.png') + self.assertEqual(file_model.content_type, 'text') + + # Try to add it again + try: + FileService.add_workflow_spec_file(spec, + name="something.png", + content_type="text", + binary_data=b'5678') + except ApiError as ae: + self.assertEqual(ae.message, 'If you want to replace the file, use the update mechanism.') From d274813ae59326c99bada3807182048b00cd10f5 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 7 Jan 2021 11:10:31 -0500 Subject: [PATCH 5/7] We now check form field IDs so they play well with Python and Javascript. --- crc/services/workflow_service.py | 16 ++++ .../workflow_form_field_name.bpmn | 86 +++++++++++++++++++ .../workflow/test_workflow_form_field_name.py | 13 +++ 3 files changed, 115 insertions(+) create mode 100644 tests/data/workflow_form_field_name/workflow_form_field_name.bpmn create mode 100644 tests/workflow/test_workflow_form_field_name.py diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index a30622a2..ac81ac71 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -131,6 +131,10 @@ class WorkflowService(object): if field.type is None: raise ApiError(code='invalid_form_data', message='Field type is None. A field type must be provided.') + # Assure we have valid ids + if not WorkflowService.check_field_id(field.id): + raise ApiError(code='invalid_form_id', + message="A field ID must begin with a letter, and can only contain letters, numbers, and '_'") # Assure field has valid properties WorkflowService.check_field_properties(field, task) @@ -179,6 +183,18 @@ class WorkflowService(object): task.data = {} task.data.update(form_data) + @staticmethod + def check_field_id(id): + """Assures that field names are valid Python and Javascript names.""" + if not id[0].isalpha(): + return False + for char in id[1:len(id)]: + if char.isalnum() or char == '_': + pass + else: + return False + return True + @staticmethod def check_field_properties(field, task): """Assures that all properties are valid on a given workflow.""" diff --git a/tests/data/workflow_form_field_name/workflow_form_field_name.bpmn b/tests/data/workflow_form_field_name/workflow_form_field_name.bpmn new file mode 100644 index 00000000..2794adec --- /dev/null +++ b/tests/data/workflow_form_field_name/workflow_form_field_name.bpmn @@ -0,0 +1,86 @@ + + + + + Flow_0dbfi6t + + + + <H1>Hello</H1> + Flow_0dbfi6t + Flow_02rje6r + + + + + + + + + Flow_02rje6r + Flow_1iphrck + + + + + + + + + Flow_1iphrck + Flow_0cxh51h + + + + Flow_0cxh51h + Flow_0hbiuz4 + print('Hello', name) + + + Flow_0hbiuz4 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_form_field_name.py b/tests/workflow/test_workflow_form_field_name.py new file mode 100644 index 00000000..8bb33f7a --- /dev/null +++ b/tests/workflow/test_workflow_form_field_name.py @@ -0,0 +1,13 @@ +import json +from tests.base_test import BaseTest + + +class TestFormFieldName(BaseTest): + + def test_form_field_name(self): + spec_model = self.load_test_spec('workflow_form_field_name') + 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.assertEqual(json_data[0]['message'], + "When populating all fields ... A field ID must begin with a letter, and can only contain letters, numbers, and '_'") From 72a73c1fc4b4b066f1b7b7e968ac4df90bd4f249 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 8 Jan 2021 13:23:01 -0500 Subject: [PATCH 6/7] Allow for synch to work even if the local set of workflow specifications are completely empty. --- crc/__init__.py | 7 +++++++ crc/api/workflow_sync.py | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/crc/__init__.py b/crc/__init__.py index ed69f70a..6fd1ebbd 100644 --- a/crc/__init__.py +++ b/crc/__init__.py @@ -99,6 +99,13 @@ def load_example_rrt_data(): ExampleDataLoader.clean_db() ExampleDataLoader().load_rrt() + +@app.cli.command() +def load_reference_files(): + """Load example data into the database.""" + from example_data import ExampleDataLoader + ExampleDataLoader().load_reference_documents() + @app.cli.command() def clear_db(): """Load example data into the database.""" diff --git a/crc/api/workflow_sync.py b/crc/api/workflow_sync.py index 8edeaeac..cd87e2e7 100644 --- a/crc/api/workflow_sync.py +++ b/crc/api/workflow_sync.py @@ -37,6 +37,14 @@ def get_changed_workflows(remote,as_df=False): # get the local thumbprints & make sure that 'workflow_spec_id' is a column, not an index local = get_all_spec_state_dataframe().reset_index() + if local.empty: + # return the list as a dict, let swagger convert it to json + remote_workflows['new'] = True + if as_df: + return remote_workflows + else: + return remote_workflows.reset_index().to_dict(orient='records') + # merge these on workflow spec id and hash - this will # make two different date columns date_x and date_y different = remote_workflows.merge(local, @@ -44,8 +52,7 @@ def get_changed_workflows(remote,as_df=False): left_on=['workflow_spec_id','md5_hash'], how = 'outer' , indicator=True).loc[lambda x : x['_merge']!='both'] - if len(different)==0: - return [] + # each line has a tag on it - if was in the left or the right, # label it so we know if that was on the remote or local machine different.loc[different['_merge']=='left_only','location'] = 'remote' @@ -74,6 +81,8 @@ def get_changed_workflows(remote,as_df=False): changedfiles.loc[changedfiles.index.isin(left['workflow_spec_id']), 'new'] = True output = changedfiles[~changedfiles.index.isin(right['workflow_spec_id'])] + + # return the list as a dict, let swagger convert it to json if as_df: return output @@ -295,6 +304,10 @@ def get_all_spec_state_dataframe(): 'date_created':file.date_created}) df = pd.DataFrame(filelist) + # If the file list is empty, return an empty data frame + if df.empty: + return df + # get a distinct list of file_model_id's with the most recent file_data retained df = df.sort_values('date_created').drop_duplicates(['file_model_id'],keep='last').copy() From 336c093bf8faebd5bb36d6fe62e025b7e85c6e69 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 8 Jan 2021 15:15:24 -0500 Subject: [PATCH 7/7] Catch errors when building enumeration lists. --- crc/services/workflow_service.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index cb6903d5..82a4610d 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -586,6 +586,13 @@ class WorkflowService(object): items = data_model.items() if isinstance(data_model, dict) else data_model options = [] for item in items: + if value_column not in item: + raise ApiError.from_task("invalid_enum", f"The value column '{value_column}' does not exist for item {item}", + task=spiff_task) + if label_column not in item: + 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}) return options