From d8ae719489bc74f4877232c6b9ea6fcc96b44f22 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Fri, 6 May 2022 14:13:02 -0400 Subject: [PATCH] We don't need to run _update_status_of_workflow_meta any more But, it returned warnings to the front end for debugging Ticket 733 was created to address this. Fix the warnings and return them again. This also broke the tests in test_study_status_message, so we skip them for now --- crc/services/study_service.py | 93 ++++++++++++------------ tests/study/test_study_status_message.py | 9 ++- 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 8d9e94e7..6ccbddca 100755 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -104,8 +104,8 @@ class StudyService(object): workflow_metas = StudyService._get_workflow_metas(study_id, category) category_meta = [] if master_workflow_results: - study.warnings = StudyService._update_status_of_workflow_meta(workflow_metas, - master_workflow_results) + # study.warnings = StudyService._update_status_of_workflow_meta(workflow_metas, + # master_workflow_results) category_meta = StudyService._update_status_of_category_meta(master_workflow_results, category) category.workflows = workflow_metas category.meta = category_meta @@ -503,51 +503,50 @@ class StudyService(object): if unused_statuses.get(cat.id)['message'] else '' return cat_meta - - @staticmethod - def _update_status_of_workflow_meta(workflow_metas, status): - # TODO: Remove - # This code is moving to WorkflowService.update_workflow_state_from_master_workflow - - # Update the status on each workflow - warnings = [] - unused_statuses = status.copy() # A list of all the statuses that are not used. - for wfm in workflow_metas: - unused_statuses.pop(wfm.workflow_spec_id, None) - wfm.state_message = '' - # do we have a status for you - if wfm.workflow_spec_id not in status.keys(): - warnings.append(ApiError("missing_status", - "No status information provided about workflow %s" % wfm.workflow_spec_id)) - continue - if not isinstance(status[wfm.workflow_spec_id], dict): - warnings.append(ApiError(code='invalid_status', - message=f'Status must be a dictionary with "status" and "message" keys. ' - f'Name is {wfm.workflow_spec_id}. Status is {status[wfm.workflow_spec_id]}')) - continue - if 'message' in status[wfm.workflow_spec_id].keys(): - wfm.state_message = status[wfm.workflow_spec_id]['message'] - if 'status' not in status[wfm.workflow_spec_id].keys(): - warnings.append(ApiError("missing_status_key", - "Workflow '%s' is present in master workflow, but doesn't have a status" % wfm.workflow_spec_id)) - continue - if not WorkflowState.has_value(status[wfm.workflow_spec_id]['status']): - warnings.append(ApiError("invalid_state", - "Workflow '%s' can not be set to '%s', should be one of %s" % ( - wfm.workflow_spec_id, status[wfm.workflow_spec_id]['status'], - ",".join(WorkflowState.list()) - ))) - continue - - wfm.state = WorkflowState[status[wfm.workflow_spec_id]['status']] - - for status in unused_statuses: - if isinstance(unused_statuses[status], dict) and 'status' in unused_statuses[status]: - warnings.append(ApiError("unmatched_status", "The master workflow provided a status for '%s' a " - "workflow that doesn't seem to exist." % - status)) - - return warnings + # @staticmethod + # def _update_status_of_workflow_meta(workflow_metas, status): + # # TODO: Remove + # # This code is moving to WorkflowService.update_workflow_state_from_master_workflow + # + # # Update the status on each workflow + # warnings = [] + # unused_statuses = status.copy() # A list of all the statuses that are not used. + # for wfm in workflow_metas: + # unused_statuses.pop(wfm.workflow_spec_id, None) + # wfm.state_message = '' + # # do we have a status for you + # if wfm.workflow_spec_id not in status.keys(): + # warnings.append(ApiError("missing_status", + # "No status information provided about workflow %s" % wfm.workflow_spec_id)) + # continue + # if not isinstance(status[wfm.workflow_spec_id], dict): + # warnings.append(ApiError(code='invalid_status', + # message=f'Status must be a dictionary with "status" and "message" keys. ' + # f'Name is {wfm.workflow_spec_id}. Status is {status[wfm.workflow_spec_id]}')) + # continue + # if 'message' in status[wfm.workflow_spec_id].keys(): + # wfm.state_message = status[wfm.workflow_spec_id]['message'] + # if 'status' not in status[wfm.workflow_spec_id].keys(): + # warnings.append(ApiError("missing_status_key", + # "Workflow '%s' is present in master workflow, but doesn't have a status" % wfm.workflow_spec_id)) + # continue + # if not WorkflowState.has_value(status[wfm.workflow_spec_id]['status']): + # warnings.append(ApiError("invalid_state", + # "Workflow '%s' can not be set to '%s', should be one of %s" % ( + # wfm.workflow_spec_id, status[wfm.workflow_spec_id]['status'], + # ",".join(WorkflowState.list()) + # ))) + # continue + # + # wfm.state = WorkflowState[status[wfm.workflow_spec_id]['status']] + # + # for status in unused_statuses: + # if isinstance(unused_statuses[status], dict) and 'status' in unused_statuses[status]: + # warnings.append(ApiError("unmatched_status", "The master workflow provided a status for '%s' a " + # "workflow that doesn't seem to exist." % + # status)) + # + # return warnings @staticmethod def add_all_workflow_specs_to_study(study_model: StudyModel, specs: List[WorkflowSpecInfo]): diff --git a/tests/study/test_study_status_message.py b/tests/study/test_study_status_message.py index 34abdecb..ede35c6c 100644 --- a/tests/study/test_study_status_message.py +++ b/tests/study/test_study_status_message.py @@ -4,6 +4,7 @@ from crc.models.study import StudyModel from crc.models.workflow import WorkflowState from crc.services.study_service import StudyService from crc.services.workflow_spec_service import WorkflowSpecService +from unittest import skip class TestStudyStatusMessage(BaseTest): @@ -21,10 +22,11 @@ class TestStudyStatusMessage(BaseTest): warnings = StudyService._update_status_of_workflow_meta(workflow_metas, status) return workflow_metas, warnings + @skip("We don't currently return warnings. Ticket 733 addresses this. Delete or fix these then.") def test_study_status_message(self): # these are the passing tests # we loop through each Workflow state - # (hidden,disabled,required,optional) + # (hidden,disabled,required,optional,locked) for state in WorkflowState: # use state.value to set status['status'], status = {'random_fact': @@ -39,6 +41,7 @@ class TestStudyStatusMessage(BaseTest): self.assertEqual(state, workflow_metas[0].state) self.assertEqual('This is my status message!', workflow_metas[0].state_message) + @skip("We don't currently return warnings. Ticket 733 addresses this. Delete or fix these then.") 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!'}} @@ -51,6 +54,7 @@ class TestStudyStatusMessage(BaseTest): self.assertEqual('The master workflow provided a status for \'bad_name\' a workflow that doesn\'t' ' seem to exist.', warnings[1].message) + @skip("We don't currently return warnings. Ticket 733 addresses this. Delete or fix these then.") 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!'} @@ -61,6 +65,7 @@ class TestStudyStatusMessage(BaseTest): 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) + @skip("We don't currently return warnings. Ticket 733 addresses this. Delete or fix these then.") def test_study_status_message_bad_state(self): # you have an invalid state # I.e., not in (hidden,disabled,required,optional) @@ -68,5 +73,5 @@ class TestStudyStatusMessage(BaseTest): 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', + self.assertEqual('Workflow \'random_fact\' can not be set to \'hide\', should be one of hidden,disabled,required,optional,locked', warnings[0].message)