From eb3011ae5f21e6f600d0159969dc33d51b35cde0 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 24 May 2022 15:28:13 -0400 Subject: [PATCH 1/9] Don't let the master workflow change the state of workflows in the admin sandbox. I.e., when category.admin is True If a workflow is in the admin sandbox, we set the state to 'optional', no matter what the master workflow says Added a method to get the category for a workflow spec. --- crc/services/workflow_service.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index d3168bf2..2a83c53d 100755 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -1146,8 +1146,25 @@ class WorkflowService(object): if WorkflowState.has_value(workflow_state): # Get the workflow from our dictionary and set the state workflow = wf_by_workflow_spec_id[workflow_spec_id] - workflow.state = workflow_state - workflow.state_message = workflow_state_message + # Don't let the master workflow change the state for admin workflows + if WorkflowService.is_in_admin_category(workflow_spec_id): + workflow.state = 'optional' + workflow.state_message = 'This is an Admin workflow' + else: + workflow.state = workflow_state + workflow.state_message = workflow_state_message session.add(workflow) session.commit() + + @staticmethod + def get_workflow_spec_category(workflow_spec_id): + workflow_spec = WorkflowSpecService().get_spec(workflow_spec_id) + category_id = workflow_spec.category_id + category = WorkflowSpecService().get_category(category_id) + return category + + @staticmethod + def is_in_admin_category(workflow_spec_id): + category = WorkflowService.get_workflow_spec_category(workflow_spec_id) + return category.admin From 3bd591b2ce04fdd536e0c86a15dc4c48c972146d Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 24 May 2022 15:32:51 -0400 Subject: [PATCH 2/9] Allow us to set the `admin` bit when creating a test WorkflowSpecCategory This allows us to create an "admin sandbox" category in a test --- tests/base_test.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/base_test.py b/tests/base_test.py index 27c68c17..718e3952 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -169,12 +169,14 @@ class BaseTest(unittest.TestCase): self.workflow_spec_service.add_category(category) return category - def assure_category_exists(self, category_id=None): + def assure_category_exists(self, category_id=None, display_name="Test Workflows", admin=False): category = None if category_id is not None: category = self.workflow_spec_service.get_category(category_id) if category is None: - category = WorkflowSpecCategory(id="test_category", display_name="Test Workflows", admin=False, display_order=0) + if category_id is None: + category_id = 'test_category' + category = WorkflowSpecCategory(id=category_id, display_name=display_name, admin=admin, display_order=0) self.workflow_spec_service.add_category(category) return category From 132a140d5ae3ff5de0a95194d33182b4dbfd7965 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 24 May 2022 15:33:56 -0400 Subject: [PATCH 3/9] Make sure the Master Workflow doesn't change the state for workflows in an Admin Sandbox --- tests/study/test_study_api.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/study/test_study_api.py b/tests/study/test_study_api.py index b95254ec..4d6cdaf8 100644 --- a/tests/study/test_study_api.py +++ b/tests/study/test_study_api.py @@ -102,6 +102,24 @@ class TestStudyApi(BaseTest): self.assertEqual('Completion of this workflow is required.', workflows[0].state_message) self.assertEqual('This workflow is locked', workflows[1].state_message) + def test_admin_workflows_not_locked(self): + """Admin category workflows should not be locked by the master workflow""" + # Add an admin category + self.assure_category_exists('test_admin_category', admin=True) + # Add a workflow to the admin category + self.load_test_spec('empty_workflow', category_id='test_admin_category') + # Add a master workflow that tries to lock the workflow + self.load_test_spec('test_master_workflow', master_spec=True) + + study = self.add_test_study() + study_model = session.query(StudyModel).filter_by(id=study["id"]).first() + self.run_master_spec(study_model) + + workflows = session.query(WorkflowModel).all() + self.assertEqual(1, len(workflows)) + # The master workflow tries to lock this, but we don't lock if Admin category + self.assertEqual('optional', workflows[0].state) + def test_get_study_has_details_about_files(self): # Set up the study and attach a file to it. From 2c083f73a8132c79f01c5c9d170a4cdf11089e10 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 24 May 2022 15:57:08 -0400 Subject: [PATCH 4/9] Add `is_admin_workflow` attribute to WorkflowApi and WorkflowApiSchema Set is_admin_workflow when we instantiate WorkflowApi in WorkflowService.processor_to_workflow_api --- crc/models/api_models.py | 7 ++++--- crc/services/workflow_service.py | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/crc/models/api_models.py b/crc/models/api_models.py index 6049faf7..b96e8a7b 100644 --- a/crc/models/api_models.py +++ b/crc/models/api_models.py @@ -213,7 +213,7 @@ class DocumentDirectory(object): class WorkflowApi(object): def __init__(self, id, status, next_task, navigation, workflow_spec_id, total_tasks, completed_tasks, - last_updated, is_review, title, study_id, state): + last_updated, is_review, title, study_id, state, is_admin_workflow=False): self.id = id self.status = status self.next_task = next_task # The next task that requires user input. @@ -226,6 +226,7 @@ class WorkflowApi(object): self.is_review = is_review self.study_id = study_id or '' self.state = state + self.is_admin_workflow = is_admin_workflow class WorkflowApiSchema(ma.Schema): @@ -233,7 +234,7 @@ class WorkflowApiSchema(ma.Schema): model = WorkflowApi fields = ["id", "status", "next_task", "navigation", "workflow_spec_id", "total_tasks", "completed_tasks", - "last_updated", "is_review", "title", "study_id", "state"] + "last_updated", "is_review", "title", "study_id", "state", "is_admin_workflow"] unknown = INCLUDE status = EnumField(WorkflowStatus) @@ -245,7 +246,7 @@ class WorkflowApiSchema(ma.Schema): def make_workflow(self, data, **kwargs): keys = ['id', 'status', 'next_task', 'navigation', 'workflow_spec_id', "total_tasks", "completed_tasks", - "last_updated", "is_review", "title", "study_id", "state"] + "last_updated", "is_review", "title", "study_id", "state", "is_admin_workflow"] filtered_fields = {key: data[key] for key in keys} filtered_fields['next_task'] = TaskSchema().make_task(data['next_task']) return WorkflowApi(**filtered_fields) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index 2a83c53d..2a45115c 100755 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -664,6 +664,7 @@ class WorkflowService(object): WorkflowService.update_navigation(navigation, processor) spec_service = WorkflowSpecService() spec = spec_service.get_spec(processor.workflow_spec_id) + is_admin_workflow = WorkflowService.is_in_admin_category(processor.workflow_spec_id) workflow_api = WorkflowApi( id=processor.get_workflow_id(), status=processor.get_status(), @@ -676,7 +677,8 @@ class WorkflowService(object): is_review=spec.is_review, title=spec.display_name, study_id=processor.workflow_model.study_id or None, - state=processor.workflow_model.state + state=processor.workflow_model.state, + is_admin_workflow=is_admin_workflow ) if not next_task: # The Next Task can be requested to be a certain task, useful for parallel tasks. # This may or may not work, sometimes there is no next task to complete. From 4a1300f101a8fe9d1bcc5b37da9edd9e857b17db Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 25 May 2022 14:29:14 -0400 Subject: [PATCH 5/9] Add test that checks the is_admin_workflow attribute on WorkflowApi Add `from_workflow_model` helper method to the WorkflowApi class in api_models --- crc/models/api_models.py | 19 ++++++++++++++++++- tests/test_workflow_api.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/crc/models/api_models.py b/crc/models/api_models.py index b96e8a7b..61d70e46 100644 --- a/crc/models/api_models.py +++ b/crc/models/api_models.py @@ -6,7 +6,7 @@ from marshmallow import INCLUDE from marshmallow_enum import EnumField from crc import ma -from crc.models.workflow import WorkflowStatus +from crc.models.workflow import WorkflowStatus, WorkflowModel from crc.models.file import FileSchema class MultiInstanceType(enum.Enum): @@ -228,6 +228,23 @@ class WorkflowApi(object): self.state = state self.is_admin_workflow = is_admin_workflow + @classmethod + def from_workflow_model(cls, workflow: WorkflowModel): + instance = cls() + cls.id = workflow.id + cls.status = workflow.status + cls.next_task = workflow.next_task # The next task that requires user input. + cls.navigation = workflow.navigation + cls.workflow_spec_id = workflow.workflow_spec_id + cls.total_tasks = workflow.total_tasks + cls.completed_tasks = workflow.completed_tasks + cls.last_updated = workflow.last_updated + cls.title = workflow.title + cls.is_review = workflow.is_review + cls.study_id = workflow.study_id or '' + cls.state = workflow.state + cls.is_admin_workflow = workflow.is_admin_workflow + class WorkflowApiSchema(ma.Schema): class Meta: diff --git a/tests/test_workflow_api.py b/tests/test_workflow_api.py index ff8a93c2..07567482 100644 --- a/tests/test_workflow_api.py +++ b/tests/test_workflow_api.py @@ -1,6 +1,7 @@ from tests.base_test import BaseTest from crc import session +from crc.models.study import StudyStatus from crc.models.task_event import TaskEventModel from crc.models.user import UserModel from crc.services.workflow_service import WorkflowService @@ -132,4 +133,34 @@ class TestWorkflowApi(BaseTest): spec1 = self.workflow_spec_service.get_spec('hello_world') self.assertNotIn('hello_world_lib', spec1.libraries) + def test_workflow_api_model(self): + """Create 2 workflow specs, one of them in an admin sandbox + Make sure we pass the correct information through the api """ + self.assure_category_exists('test_admin_category', admin=True) + self.load_test_spec('simple_form') + self.load_test_spec('hello_world', category_id='test_admin_category') + + simple_form_workflow = self.create_workflow('simple_form') + hello_world_workflow = self.create_workflow('hello_world') + # Make sure both workflows use the same study + self.assertEqual(hello_world_workflow.study.id, simple_form_workflow.study.id) + study = hello_world_workflow.study + # Make sure the study status is in_progress + self.assertEqual(StudyStatus.in_progress, study.status) + + rv_simple_form_1 = self.app.get('/v1.0/workflow/%s' % simple_form_workflow.id, + follow_redirects=True, + content_type="application/json", + headers=self.logged_in_headers()) + self.assert_success(rv_simple_form_1) + json_data_simple_1 = json.loads(rv_simple_form_1.get_data(as_text=True)) + self.assertFalse(json_data_simple_1['is_admin_workflow']) + + rv_hello_world_1 = self.app.get('/v1.0/workflow/%s' % hello_world_workflow.id, + follow_redirects=True, + content_type="application/json", + headers=self.logged_in_headers()) + self.assert_success(rv_hello_world_1) + json_data_hello_1 = json.loads(rv_hello_world_1.get_data(as_text=True)) + self.assertTrue(json_data_hello_1['is_admin_workflow']) From 5b2b2f7b60e728c380da92a47803dcbc8f64a50a Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 25 May 2022 15:06:31 -0400 Subject: [PATCH 6/9] better wording --- crc/services/workflow_service.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index 2a45115c..e78e2ff1 100755 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -664,7 +664,7 @@ class WorkflowService(object): WorkflowService.update_navigation(navigation, processor) spec_service = WorkflowSpecService() spec = spec_service.get_spec(processor.workflow_spec_id) - is_admin_workflow = WorkflowService.is_in_admin_category(processor.workflow_spec_id) + is_admin_workflow = WorkflowService.is_admin_workflow(processor.workflow_spec_id) workflow_api = WorkflowApi( id=processor.get_workflow_id(), status=processor.get_status(), @@ -1149,7 +1149,7 @@ class WorkflowService(object): # Get the workflow from our dictionary and set the state workflow = wf_by_workflow_spec_id[workflow_spec_id] # Don't let the master workflow change the state for admin workflows - if WorkflowService.is_in_admin_category(workflow_spec_id): + if WorkflowService.is_admin_workflow(workflow_spec_id): workflow.state = 'optional' workflow.state_message = 'This is an Admin workflow' else: @@ -1167,6 +1167,6 @@ class WorkflowService(object): return category @staticmethod - def is_in_admin_category(workflow_spec_id): + def is_admin_workflow(workflow_spec_id): category = WorkflowService.get_workflow_spec_category(workflow_spec_id) return category.admin From 95c915a434d4a049215f698e00de859be07d10d4 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 26 May 2022 13:35:30 -0400 Subject: [PATCH 7/9] Remove code that doesn't let master workflow set state and state_message for admin sandbox workflows Remove test for this feature --- crc/services/workflow_service.py | 9 ++------- tests/study/test_study_api.py | 18 ------------------ 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index e78e2ff1..b077edcd 100755 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -1148,13 +1148,8 @@ class WorkflowService(object): if WorkflowState.has_value(workflow_state): # Get the workflow from our dictionary and set the state workflow = wf_by_workflow_spec_id[workflow_spec_id] - # Don't let the master workflow change the state for admin workflows - if WorkflowService.is_admin_workflow(workflow_spec_id): - workflow.state = 'optional' - workflow.state_message = 'This is an Admin workflow' - else: - workflow.state = workflow_state - workflow.state_message = workflow_state_message + workflow.state = workflow_state + workflow.state_message = workflow_state_message session.add(workflow) session.commit() diff --git a/tests/study/test_study_api.py b/tests/study/test_study_api.py index 4d6cdaf8..b95254ec 100644 --- a/tests/study/test_study_api.py +++ b/tests/study/test_study_api.py @@ -102,24 +102,6 @@ class TestStudyApi(BaseTest): self.assertEqual('Completion of this workflow is required.', workflows[0].state_message) self.assertEqual('This workflow is locked', workflows[1].state_message) - def test_admin_workflows_not_locked(self): - """Admin category workflows should not be locked by the master workflow""" - # Add an admin category - self.assure_category_exists('test_admin_category', admin=True) - # Add a workflow to the admin category - self.load_test_spec('empty_workflow', category_id='test_admin_category') - # Add a master workflow that tries to lock the workflow - self.load_test_spec('test_master_workflow', master_spec=True) - - study = self.add_test_study() - study_model = session.query(StudyModel).filter_by(id=study["id"]).first() - self.run_master_spec(study_model) - - workflows = session.query(WorkflowModel).all() - self.assertEqual(1, len(workflows)) - # The master workflow tries to lock this, but we don't lock if Admin category - self.assertEqual('optional', workflows[0].state) - def test_get_study_has_details_about_files(self): # Set up the study and attach a file to it. From d07df2610363c4776dbc47b6be981e6d1b348b10 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 26 May 2022 13:35:47 -0400 Subject: [PATCH 8/9] Remove unused method --- crc/models/api_models.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/crc/models/api_models.py b/crc/models/api_models.py index 61d70e46..f7217a20 100644 --- a/crc/models/api_models.py +++ b/crc/models/api_models.py @@ -228,23 +228,6 @@ class WorkflowApi(object): self.state = state self.is_admin_workflow = is_admin_workflow - @classmethod - def from_workflow_model(cls, workflow: WorkflowModel): - instance = cls() - cls.id = workflow.id - cls.status = workflow.status - cls.next_task = workflow.next_task # The next task that requires user input. - cls.navigation = workflow.navigation - cls.workflow_spec_id = workflow.workflow_spec_id - cls.total_tasks = workflow.total_tasks - cls.completed_tasks = workflow.completed_tasks - cls.last_updated = workflow.last_updated - cls.title = workflow.title - cls.is_review = workflow.is_review - cls.study_id = workflow.study_id or '' - cls.state = workflow.state - cls.is_admin_workflow = workflow.is_admin_workflow - class WorkflowApiSchema(ma.Schema): class Meta: From 251ff921ff29d5b2e38afc2f240afedf903c6ad2 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 26 May 2022 13:36:10 -0400 Subject: [PATCH 9/9] clean up imports --- tests/study/test_study_api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/study/test_study_api.py b/tests/study/test_study_api.py index b95254ec..d1351307 100644 --- a/tests/study/test_study_api.py +++ b/tests/study/test_study_api.py @@ -15,8 +15,6 @@ from crc.models.task_event import TaskEventModel from crc.models.study import StudyEvent, StudyModel, StudySchema, StudyStatus, StudyEventType, StudyAssociated from crc.models.workflow import WorkflowModel from crc.services.workflow_processor import WorkflowProcessor -from crc.services.workflow_service import WorkflowService -from crc.services.workflow_spec_service import WorkflowSpecService from crc.services.user_file_service import UserFileService