From c05b879dd41265d8d6d44f2611f7e66da65721f4 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 16 Mar 2021 12:09:42 -0400 Subject: [PATCH 1/6] We want to modify the master workflow so that it returns both a status, and a message explaining the status for each workflow. I.e., from status = {'ids_full_submission': 'required', 'enter_core_info': 'disabled', ...} to status = {'ids_full_submission': {'status': 'required', 'message': 'required because...'}, 'enter_core_info': {'status': 'disabled', 'message': 'disabled because...'}, ...} Modified study_service.__update_status_of_workflow_meta to accommodate this change. --- crc/services/study_service.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index c4576578..99b42223 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -421,13 +421,15 @@ class StudyService(object): warnings = [] for wfm in workflow_metas: if wfm.name in status.keys(): - if not WorkflowState.has_value(status[wfm.name]): + if not WorkflowState.has_value(status[wfm.name]['status']): warnings.append(ApiError("invalid_status", "Workflow '%s' can not be set to '%s', should be one of %s" % ( wfm.name, status[wfm.name], ",".join(WorkflowState.list()) ))) else: - wfm.state = WorkflowState[status[wfm.name]] + wfm.state = WorkflowState[status[wfm.name]['status']] + if status[wfm.name]['message']: + wfm.state_message = status[wfm.name]['message'] else: warnings.append(ApiError("missing_status", "No status specified for workflow %s" % wfm.name)) return warnings From 47ff29e3ab9b85fd2f6d43d7e9490db39e224b23 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 18 Mar 2021 12:25:27 -0400 Subject: [PATCH 2/6] Added a test for dictionary output from a decision table. Will need dictionary output for master workflow status messages. --- .../decision_table_dictionary_output.bpmn | 71 +++++++++++++++++++ .../decision_table_dictionary_output.dmn | 69 ++++++++++++++++++ .../test_decision_table_dictionary_output.py | 15 ++++ 3 files changed, 155 insertions(+) create mode 100644 tests/data/decision_table_dictionary_output/decision_table_dictionary_output.bpmn create mode 100644 tests/data/decision_table_dictionary_output/decision_table_dictionary_output.dmn create mode 100644 tests/test_decision_table_dictionary_output.py diff --git a/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.bpmn b/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.bpmn new file mode 100644 index 00000000..1117386f --- /dev/null +++ b/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.bpmn @@ -0,0 +1,71 @@ + + + + Testing Workflow Status Messages + + SequenceFlow_0x4n744 + + + + + SequenceFlow_1o630oy + SequenceFlow_1foyag7 + + + + <div><span>Good Bye {{ dog.name }}</span></div> +<div><span>You are such a good {{ dog.breed }}</span></div> + + SequenceFlow_1foyag7 + SequenceFlow_1bc1ugw + + + SequenceFlow_1bc1ugw + + + + + + + + + SequenceFlow_0x4n744 + SequenceFlow_1o630oy + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.dmn b/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.dmn new file mode 100644 index 00000000..3f74accf --- /dev/null +++ b/tests/data/decision_table_dictionary_output/decision_table_dictionary_output.dmn @@ -0,0 +1,69 @@ + + + + + + + name + + + + + + + 'Layla' + + + 'Layla' + + + 'Aussie' + + + + + 'Mona' + + + 'Mona' + + + 'Aussie Mix' + + + + + 'Jerry' + + + 'Jerry' + + + 'Aussie Mix' + + + + + 'Zoey' + + + 'Zoey' + + + 'Healer' + + + + + 'Etta' + + + 'Etta' + + + 'Healer Mix' + + + + + diff --git a/tests/test_decision_table_dictionary_output.py b/tests/test_decision_table_dictionary_output.py new file mode 100644 index 00000000..a516825a --- /dev/null +++ b/tests/test_decision_table_dictionary_output.py @@ -0,0 +1,15 @@ +from tests.base_test import BaseTest + + +class TestDecisionTableDictionaryOutput(BaseTest): + + def test_decision_table_dictionary_output(self): + + workflow = self.create_workflow('decision_table_dictionary_output') + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + + result = self.complete_form(workflow, first_task, {'name': 'Mona'}) + self.assertIn('dog', result.next_task.data) + self.assertEqual('Mona', result.next_task.data['dog']['name']) + self.assertEqual('Aussie Mix', result.next_task.data['dog']['breed']) From a201e49d21d6f11c85c9801ce2b210bd7088b9d8 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 22 Mar 2021 17:46:39 -0400 Subject: [PATCH 3/6] The master workflow now generates a dictionary of dictionaries, rather than a dictionary of strings. Each entry in status is a dictionary with 'status' and 'message' keys. We updated the StudyService._update_status_of_workflow_meta method to accommodate the master workflow changes. Changed the order of some if statements so we move forward while we have good data, and run all the error states at the end. Added some comments to explain the cascading if statements We changed the names of private methods to begin with one underscore, so they work in the test environment. Changed some internal calls to accommodate the underscore change. --- crc/services/study_service.py | 39 ++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 99b42223..450c46e4 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -71,7 +71,7 @@ class StudyService(object): study.last_activity_user = LdapService.user_info(last_event.user_uid).display_name study.last_activity_date = last_event.date study.categories = StudyService.get_categories() - workflow_metas = StudyService.__get_workflow_metas(study_id) + workflow_metas = StudyService._get_workflow_metas(study_id) files = FileService.get_files_for_study(study.id) files = (File.from_models(model, FileService.get_file_data(model.id), FileService.get_doc_dictionary()) for model in files) @@ -83,8 +83,9 @@ class StudyService(object): # this line is taking 99% of the time that is used in get_study. # see ticket #196 if do_status: - status = StudyService.__get_study_status(study_model) - study.warnings = StudyService.__update_status_of_workflow_meta(workflow_metas, status) + # __get_study_status() runs the master workflow to generate the status dictionary + status = StudyService._get_study_status(study_model) + study.warnings = StudyService._update_status_of_workflow_meta(workflow_metas, status) # Group the workflows into their categories. for category in study.categories: @@ -416,26 +417,36 @@ class StudyService(object): db.session.commit() @staticmethod - def __update_status_of_workflow_meta(workflow_metas, status): + def _update_status_of_workflow_meta(workflow_metas, status): # Update the status on each workflow warnings = [] for wfm in workflow_metas: + # do we have a status for you if wfm.name in status.keys(): - if not WorkflowState.has_value(status[wfm.name]['status']): - warnings.append(ApiError("invalid_status", - "Workflow '%s' can not be set to '%s', should be one of %s" % ( - wfm.name, status[wfm.name], ",".join(WorkflowState.list()) - ))) + # is it a dictionary + if isinstance(status[wfm.name], dict): + if 'status' in status[wfm.name].keys(): + # is 'status' a valid WorkflowState + if WorkflowState.has_value(status[wfm.name]['status']): + wfm.state = WorkflowState[status[wfm.name]['status']] + if 'message' in status[wfm.name].keys(): + wfm.state_message = status[wfm.name]['message'] + else: + wfm.state_message = '' + else: + warnings.append(ApiError("invalid_state", + "Workflow '%s' can not be set to '%s', should be one of %s" % ( + wfm.name, status[wfm.name]['status'], ",".join(WorkflowState.list()) + ))) else: - wfm.state = WorkflowState[status[wfm.name]['status']] - if status[wfm.name]['message']: - wfm.state_message = status[wfm.name]['message'] + warnings.append(ApiError(code='invalid_status', + message=f'Status must be a dictionary with "status" and "message" keys. Name is {wfm.name}. Status is {status[wfm.name]}')) else: warnings.append(ApiError("missing_status", "No status specified for workflow %s" % wfm.name)) return warnings @staticmethod - def __get_workflow_metas(study_id): + def _get_workflow_metas(study_id): # Add in the Workflows for each category workflow_models = db.session.query(WorkflowModel). \ join(WorkflowSpecModel). \ @@ -448,7 +459,7 @@ class StudyService(object): return workflow_metas @staticmethod - def __get_study_status(study_model): + def _get_study_status(study_model): """Uses the Top Level Workflow to calculate the status of the study, and it's workflow models.""" master_specs = db.session.query(WorkflowSpecModel). \ From 3ee8109535c973c0906e7a2d507376841c071252 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 22 Mar 2021 17:52:14 -0400 Subject: [PATCH 4/6] Tests for the changes to master workflow status dictionary Test for each valid WorkflowState Test for all three failure states; not in status, not a dictionary, not a valid state. --- tests/study/test_study_status_message.py | 66 ++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 tests/study/test_study_status_message.py diff --git a/tests/study/test_study_status_message.py b/tests/study/test_study_status_message.py new file mode 100644 index 00000000..6e2fbee2 --- /dev/null +++ b/tests/study/test_study_status_message.py @@ -0,0 +1,66 @@ +from tests.base_test import BaseTest +from crc import db, session +from crc.models.study import StudyModel +from crc.models.workflow import WorkflowState +from crc.services.study_service import StudyService + + +class TestStudyStatusMessage(BaseTest): + + """The workflow runs with a workflow_meta.name of `random_fact` + Add an entry to `status` dictionary with a key of `random_fact`""" + + def run_update_status(self, status): + # shared code + self.load_example_data() + study_model = session.query(StudyModel).first() + workflow_metas = StudyService._get_workflow_metas(study_model.id) + warnings = StudyService._update_status_of_workflow_meta(workflow_metas, status) + return workflow_metas, warnings + + def test_study_status_message(self): + # these are the passing tests + # we loop through each Workflow state + # (hidden,disabled,required,optional) + for state in WorkflowState: + # use state.value to set status['status'], + status = {'random_fact': + {'status': state.value, + 'message': 'This is my status message!'}} + + # call run_update_status(), + workflow_metas, warnings = self.run_update_status(status) + + # and assert the values of workflow_metas[0].state and workflow_metas[0].state_message + self.assertEqual(0, len(warnings)) + self.assertEqual(state, workflow_metas[0].state) + self.assertEqual('This is my status message!', workflow_metas[0].state_message) + + def test_study_status_message_bad_name(self): + # we don't have an entry for you in the status dictionary + status = {'bad_name': {'status': 'hidden', 'message': 'This is my status message!'}} + workflow_metas, warnings = self.run_update_status(status) + + self.assertEqual(1, len(warnings)) + self.assertEqual('missing_status', warnings[0].code) + self.assertEqual('No status specified for workflow random_fact', warnings[0].message) + + def test_study_status_message_not_dict(self): + # your entry in the status dictionary is not a dictionary + status = {'random_fact': 'This is my status message!'} + workflow_metas, warnings = self.run_update_status(status) + + self.assertEqual(1, len(warnings)) + self.assertEqual('invalid_status', warnings[0].code) + self.assertEqual('Status must be a dictionary with "status" and "message" keys. Name is random_fact. Status is This is my status message!', + warnings[0].message) + + def test_study_status_message_bad_state(self): + # you have an invalid state + # I.e., not in (hidden,disabled,required,optional) + status = {'random_fact': {'status': 'hide', 'message': 'This is my status message!'}} + workflow_metas, warnings = self.run_update_status(status) + self.assertEqual(1, len(warnings)) + self.assertEqual('invalid_state', warnings[0].code) + self.assertEqual('Workflow \'random_fact\' can not be set to \'hide\', should be one of hidden,disabled,required,optional', + warnings[0].message) From b64b52e7cb37622952336a454ffbbd0aef12b0c0 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 26 Mar 2021 19:47:31 -0400 Subject: [PATCH 5/6] Assure that we properly return the new status message over the api calls. --- crc/models/study.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crc/models/study.py b/crc/models/study.py index d6209013..99d1d585 100644 --- a/crc/models/study.py +++ b/crc/models/study.py @@ -96,7 +96,7 @@ class WorkflowMetadata(object): def __init__(self, id, name = None, display_name = None, description = None, spec_version = None, category_id = None, category_display_name = None, state: WorkflowState = None, status: WorkflowStatus = None, total_tasks = None, completed_tasks = None, - is_review=None,display_order = None): + is_review=None,display_order = None, state_message = None): self.id = id self.name = name self.display_name = display_name @@ -105,6 +105,7 @@ class WorkflowMetadata(object): self.category_id = category_id self.category_display_name = category_display_name self.state = state + self.state_message = state_message self.status = status self.total_tasks = total_tasks self.completed_tasks = completed_tasks @@ -140,7 +141,7 @@ class WorkflowMetadataSchema(ma.Schema): model = WorkflowMetadata additional = ["id", "name", "display_name", "description", "total_tasks", "completed_tasks", "display_order", - "category_id", "is_review", "category_display_name"] + "category_id", "is_review", "category_display_name", "state_message"] unknown = INCLUDE From ded11ddd82b1e24f42168ca610a485b65e91825f Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 26 Mar 2021 20:25:34 -0400 Subject: [PATCH 6/6] fixing a code smell --- crc/api.yml | 1 - crc/services/study_service.py | 40 +++++++++++++++++------------------ 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/crc/api.yml b/crc/api.yml index dbc10fa4..833efd54 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -160,7 +160,6 @@ paths: parameters: - name: workflow_spec_id in: path - required: false description: The unique id of an existing workflow specification to modify. schema: type: string diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 450c46e4..2c1bf8fe 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -421,28 +421,28 @@ class StudyService(object): # Update the status on each workflow warnings = [] for wfm in workflow_metas: + wfm.state_message = '' # do we have a status for you - if wfm.name in status.keys(): - # is it a dictionary - if isinstance(status[wfm.name], dict): - if 'status' in status[wfm.name].keys(): - # is 'status' a valid WorkflowState - if WorkflowState.has_value(status[wfm.name]['status']): - wfm.state = WorkflowState[status[wfm.name]['status']] - if 'message' in status[wfm.name].keys(): - wfm.state_message = status[wfm.name]['message'] - else: - wfm.state_message = '' - else: - warnings.append(ApiError("invalid_state", - "Workflow '%s' can not be set to '%s', should be one of %s" % ( - wfm.name, status[wfm.name]['status'], ",".join(WorkflowState.list()) - ))) - else: - warnings.append(ApiError(code='invalid_status', - message=f'Status must be a dictionary with "status" and "message" keys. Name is {wfm.name}. Status is {status[wfm.name]}')) - else: + if wfm.name not in status.keys(): warnings.append(ApiError("missing_status", "No status specified for workflow %s" % wfm.name)) + continue + if not isinstance(status[wfm.name], dict): + warnings.append(ApiError(code='invalid_status', + message=f'Status must be a dictionary with "status" and "message" keys. Name is {wfm.name}. Status is {status[wfm.name]}')) + continue + if 'status' not in status[wfm.name].keys(): + warnings.append(ApiError("missing_status", + "Workflow '%s' does not have a status setting" % wfm.name)) + continue + if not WorkflowState.has_value(status[wfm.name]['status']): + warnings.append(ApiError("invalid_state", + "Workflow '%s' can not be set to '%s', should be one of %s" % ( + wfm.name, status[wfm.name]['status'], ",".join(WorkflowState.list()) + ))) + continue + wfm.state = WorkflowState[status[wfm.name]['status']] + if 'message' in status[wfm.name].keys(): + wfm.state_message = status[wfm.name]['message'] return warnings @staticmethod