diff --git a/crc/models/approval.py b/crc/models/approval.py index be83ba30..df433fac 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -57,23 +57,11 @@ class Approval(object): @classmethod def from_model(cls, model: ApprovalModel): - # TODO: Reduce the code by iterating over model's dict keys - instance = cls() - instance.id = model.id - instance.study_id = model.study_id - instance.workflow_id = model.workflow_id - instance.version = model.version - instance.approver_uid = model.approver_uid - instance.status = model.status - instance.message = model.message - instance.date_created = model.date_created - instance.date_approved = model.date_approved - instance.version = model.version - instance.title = '' + args = dict((k, v) for k, v in model.__dict__.items() if not k.startswith('_')) + instance = cls(**args) instance.related_approvals = [] + instance.title = model.study.title if model.study else '' - if model.study: - instance.title = model.study.title try: instance.approver = LdapService.user_info(model.approver_uid) instance.primary_investigator = LdapService.user_info(model.study.primary_investigator_id) diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index 19912207..28b97b6b 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -287,7 +287,7 @@ class ApprovalService(object): ) if mail_result: app.logger.error(mail_result, exc_info=True) - # TODO: Log update action by approver_uid - maybe ? + return db_approval @staticmethod @@ -299,11 +299,12 @@ class ApprovalService(object): pending approvals and create a new approval for the latest version of the workflow.""" - # Find any existing approvals for this workflow and approver. - latest_approval_request = db.session.query(ApprovalModel). \ + # Find any existing approvals for this workflow. + latest_approval_requests = db.session.query(ApprovalModel). \ filter(ApprovalModel.workflow_id == workflow_id). \ - filter(ApprovalModel.approver_uid == approver_uid). \ - order_by(desc(ApprovalModel.version)).first() + order_by(desc(ApprovalModel.version)) + + latest_approver_request = latest_approval_requests.filter(ApprovalModel.approver_uid == approver_uid).first() # Construct as hash of the latest files to see if things have changed since # the last approval. @@ -318,16 +319,20 @@ class ApprovalService(object): # If an existing approval request exists and no changes were made, do nothing. # If there is an existing approval request for a previous version of the workflow # then add a new request, and cancel any waiting/pending requests. - if latest_approval_request: - request_file_ids = list(file.file_data_id for file in latest_approval_request.approval_files) + if latest_approver_request: + request_file_ids = list(file.file_data_id for file in latest_approver_request.approval_files) current_data_file_ids.sort() request_file_ids.sort() + other_approver = latest_approval_requests.filter(ApprovalModel.approver_uid != approver_uid).first() if current_data_file_ids == request_file_ids: - return # This approval already exists. + return # This approval already exists or we're updating other approver. else: - latest_approval_request.status = ApprovalStatus.CANCELED.value - db.session.add(latest_approval_request) - version = latest_approval_request.version + 1 + for approval_request in latest_approval_requests: + if (approval_request.version == latest_approver_request.version and + approval_request.status != ApprovalStatus.CANCELED.value): + approval_request.status = ApprovalStatus.CANCELED.value + db.session.add(approval_request) + version = latest_approver_request.version + 1 else: version = 1 diff --git a/crc/services/email_service.py b/crc/services/email_service.py index 51886e3b..f800d900 100644 --- a/crc/services/email_service.py +++ b/crc/services/email_service.py @@ -13,7 +13,7 @@ class EmailService(object): """Provides common tools for working with an Email""" @staticmethod - def add_email(subject, sender, recipients, content, content_html, study_id): + def add_email(subject, sender, recipients, content, content_html, study_id=None): """We will receive all data related to an email and store it""" # Find corresponding study - if any diff --git a/crc/services/study_service.py b/crc/services/study_service.py index f07c82c8..ce283cfe 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -1,3 +1,4 @@ +from copy import copy from datetime import datetime import json from typing import List @@ -185,6 +186,7 @@ class StudyService(object): @staticmethod def get_investigators(study_id, all=False): + """Convert array of investigators from protocol builder into a dictionary keyed on the type. """ # Loop through all known investigator types as set in the reference file inv_dictionary = FileService.get_reference_data(FileService.INVESTIGATOR_LIST, 'code') @@ -192,18 +194,26 @@ class StudyService(object): # Get PB required docs pb_investigators = ProtocolBuilderService.get_investigators(study_id=study_id) - """Convert array of investigators from protocol builder into a dictionary keyed on the type""" + # It is possible for the same type to show up more than once in some circumstances, in those events + # append a counter to the name. + investigators = {} for i_type in inv_dictionary: - pb_data = next((item for item in pb_investigators if item['INVESTIGATORTYPE'] == i_type), None) - if pb_data: - inv_dictionary[i_type]['user_id'] = pb_data["NETBADGEID"] - inv_dictionary[i_type].update(StudyService.get_ldap_dict_if_available(pb_data["NETBADGEID"])) - else: - inv_dictionary[i_type]['user_id'] = None - + pb_data_entries = list(item for item in pb_investigators if item['INVESTIGATORTYPE'] == i_type) + entry_count = 0 + investigators[i_type] = copy(inv_dictionary[i_type]) + investigators[i_type]['user_id'] = None + for pb_data in pb_data_entries: + entry_count += 1 + if entry_count == 1: + t = i_type + else: + t = i_type + "_" + str(entry_count) + investigators[t] = copy(inv_dictionary[i_type]) + investigators[t]['user_id'] = pb_data["NETBADGEID"] + investigators[t].update(StudyService.get_ldap_dict_if_available(pb_data["NETBADGEID"])) if not all: - inv_dictionary = dict(filter(lambda elem: elem[1]['user_id'] is not None, inv_dictionary.items())) - return inv_dictionary + investigators = dict(filter(lambda elem: elem[1]['user_id'] is not None, investigators.items())) + return investigators @staticmethod def get_ldap_dict_if_available(user_id): diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index e89a6946..157c4c13 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -385,7 +385,8 @@ class WorkflowService(object): except TypeError as te: raise ApiError.from_task(code="template_error", message="Error processing template for task %s: %s" % (spiff_task.task_spec.name, str(te)), task=spiff_task) - # TODO: Catch additional errors and report back. + except Exception as e: + app.logger.error(str(e), exc_info=True) @staticmethod def process_options(spiff_task, field): diff --git a/tests/approval/test_approvals_api.py b/tests/approval/test_approvals_api.py index ed0f7c5d..03cd8622 100644 --- a/tests/approval/test_approvals_api.py +++ b/tests/approval/test_approvals_api.py @@ -217,27 +217,6 @@ class TestApprovals(BaseTest): total_counts = sum(counts[status] for status in statuses) self.assertEqual(total_counts, len(approvals), 'Total approval counts for user should match number of approvals for user') - def _create_study_workflow_approvals(self, user_uid, title, primary_investigator_id, approver_uids, statuses, - workflow_spec_name="random_fact"): - study = self.create_study(uid=user_uid, title=title, primary_investigator_id=primary_investigator_id) - workflow = self.create_workflow(workflow_name=workflow_spec_name, study=study) - approvals = [] - - for i in range(len(approver_uids)): - approvals.append(self.create_approval( - study=study, - workflow=workflow, - approver_uid=approver_uids[i], - status=statuses[i], - version=1 - )) - - return { - 'study': study, - 'workflow': workflow, - 'approvals': approvals, - } - def _add_lots_of_random_approvals(self, n=100, workflow_spec_name="random_fact"): num_studies_before = db.session.query(StudyModel).count() statuses = [name for name, value in ApprovalStatus.__members__.items()] diff --git a/tests/approval/test_approvals_service.py b/tests/approval/test_approvals_service.py index 34871fec..dae15eee 100644 --- a/tests/approval/test_approvals_service.py +++ b/tests/approval/test_approvals_service.py @@ -1,7 +1,7 @@ from tests.base_test import BaseTest from crc import db from crc.models.approval import ApprovalModel -from crc.services.approval_service import ApprovalService +from crc.services.approval_service import ApprovalService, ApprovalStatus from crc.services.file_service import FileService from crc.services.workflow_processor import WorkflowProcessor @@ -83,6 +83,34 @@ class TestApprovalsService(BaseTest): self.assertEqual(len(records), 2) + def test_new_approval_cancels_all_previous_approvals(self): + self.create_reference_document() + workflow = self.create_workflow("empty_workflow") + FileService.add_workflow_file(workflow_id=workflow.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code="UVACompl_PRCAppr" ) + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="lb3dp") + + current_count = ApprovalModel.query.count() + self.assertTrue(current_count, 2) + + FileService.add_workflow_file(workflow_id=workflow.id, + name="borderline.png", content_type="text", + binary_data=b'906090', irb_doc_code="AD_CoCAppr" ) + + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + + current_count = ApprovalModel.query.count() + canceled_count = ApprovalModel.query.filter(ApprovalModel.status == ApprovalStatus.CANCELED.value) + self.assertTrue(current_count, 2) + self.assertTrue(current_count, 3) + + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="lb3dp") + + current_count = ApprovalModel.query.count() + self.assertTrue(current_count, 4) + def test_new_approval_sends_proper_emails(self): self.assertEqual(1, 1) diff --git a/tests/base_test.py b/tests/base_test.py index 3bdae053..116df5a2 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -240,6 +240,29 @@ class BaseTest(unittest.TestCase): db.session.commit() return study + def _create_study_workflow_approvals(self, user_uid, title, primary_investigator_id, approver_uids, statuses, + workflow_spec_name="random_fact"): + study = self.create_study(uid=user_uid, title=title, primary_investigator_id=primary_investigator_id) + workflow = self.create_workflow(workflow_name=workflow_spec_name, study=study) + approvals = [] + + for i in range(len(approver_uids)): + approvals.append(self.create_approval( + study=study, + workflow=workflow, + approver_uid=approver_uids[i], + status=statuses[i], + version=1 + )) + + full_study = { + 'study': study, + 'workflow': workflow, + 'approvals': approvals, + } + + return full_study + def create_workflow(self, workflow_name, study=None, category_id=None): db.session.flush() spec = db.session.query(WorkflowSpecModel).filter(WorkflowSpecModel.name == workflow_name).first() diff --git a/tests/data/pb_responses/investigators.json b/tests/data/pb_responses/investigators.json index b0c1c38f..e476c453 100644 --- a/tests/data/pb_responses/investigators.json +++ b/tests/data/pb_responses/investigators.json @@ -13,5 +13,15 @@ "INVESTIGATORTYPE": "PI", "INVESTIGATORTYPEFULL": "Primary Investigator", "NETBADGEID": "dhf8r" + }, + { + "INVESTIGATORTYPE": "SI", + "INVESTIGATORTYPEFULL": "Sub Investigator", + "NETBADGEID": "ajl2j" + }, + { + "INVESTIGATORTYPE": "SI", + "INVESTIGATORTYPEFULL": "Sub Investigator", + "NETBADGEID": "cah3us" } -] \ No newline at end of file +] diff --git a/tests/emails/test_email_service.py b/tests/emails/test_email_service.py index e2bcd139..174dca13 100644 --- a/tests/emails/test_email_service.py +++ b/tests/emails/test_email_service.py @@ -31,4 +31,15 @@ class TestEmailService(BaseTest): self.assertEqual(email_model.content_html, content_html) self.assertEqual(email_model.study, study) - # TODO: Create email model without study + subject = 'Email Subject - Empty study' + EmailService.add_email(subject=subject, sender=sender, recipients=recipients, + content=content, content_html=content_html) + + email_model = EmailModel.query.order_by(EmailModel.id.desc()).first() + + self.assertEqual(email_model.subject, subject) + self.assertEqual(email_model.sender, sender) + self.assertEqual(email_model.recipients, str(recipients)) + self.assertEqual(email_model.content, content) + self.assertEqual(email_model.content_html, content_html) + self.assertEqual(email_model.study, None) diff --git a/tests/study/test_study_api.py b/tests/study/test_study_api.py index d3a76673..ea5a86e6 100644 --- a/tests/study/test_study_api.py +++ b/tests/study/test_study_api.py @@ -7,6 +7,7 @@ from unittest.mock import patch from crc import session, app from crc.models.protocol_builder import ProtocolBuilderStatus, \ ProtocolBuilderStudySchema +from crc.models.approval import ApprovalStatus from crc.models.stats import TaskEventModel from crc.models.study import StudyModel, StudySchema from crc.models.workflow import WorkflowSpecModel, WorkflowModel @@ -95,8 +96,21 @@ class TestStudyApi(BaseTest): # TODO: WRITE A TEST FOR STUDY FILES def test_get_study_has_details_about_approvals(self): - # TODO: WRITE A TEST FOR STUDY APPROVALS - pass + self.load_example_data() + full_study = self._create_study_workflow_approvals( + user_uid="dhf8r", title="first study", primary_investigator_id="lb3dp", + approver_uids=["lb3dp", "dhf8r"], statuses=[ApprovalStatus.PENDING.value, ApprovalStatus.PENDING.value] + ) + + api_response = self.app.get('/v1.0/study/%i' % full_study['study'].id, + headers=self.logged_in_headers(), content_type="application/json") + self.assert_success(api_response) + study = StudySchema().loads(api_response.get_data(as_text=True)) + + self.assertEqual(len(study.approvals), 2) + + for approval in study.approvals: + self.assertEqual(full_study['study'].title, approval['title']) def test_add_study(self): self.load_example_data() diff --git a/tests/study/test_study_service.py b/tests/study/test_study_service.py index 1eb020fa..b436835f 100644 --- a/tests/study/test_study_service.py +++ b/tests/study/test_study_service.py @@ -193,7 +193,7 @@ class TestStudyService(BaseTest): workflow = self.create_workflow('docx') # The workflow really doesnt matter in this case. investigators = StudyService().get_investigators(workflow.study_id, all=True) - self.assertEqual(9, len(investigators)) + self.assertEqual(10, len(investigators)) # dhf8r is in the ldap mock data. self.assertEqual("dhf8r", investigators['PI']['user_id']) @@ -219,10 +219,14 @@ class TestStudyService(BaseTest): workflow = self.create_workflow('docx') # The workflow really doesnt matter in this case. investigators = StudyService().get_investigators(workflow.study_id, all=False) - self.assertEqual(3, len(investigators)) + self.assertEqual(5, len(investigators)) # dhf8r is in the ldap mock data. self.assertEqual("dhf8r", investigators['PI']['user_id']) self.assertEqual("Dan Funk", investigators['PI']['display_name']) # Data from ldap self.assertEqual("Primary Investigator", investigators['PI']['label']) # Data from xls file. self.assertEqual("Always", investigators['PI']['display']) # Data from xls file. + + # Both Alex and Aaron are SI, and both should be returned. + self.assertEqual("ajl2j", investigators['SI']['user_id']) + self.assertEqual("cah3us", investigators['SI_2']['user_id']) diff --git a/tests/test_protocol_builder.py b/tests/test_protocol_builder.py index e5b75632..2a77ec05 100644 --- a/tests/test_protocol_builder.py +++ b/tests/test_protocol_builder.py @@ -24,7 +24,7 @@ class TestProtocolBuilder(BaseTest): mock_get.return_value.text = self.protocol_builder_response('investigators.json') response = ProtocolBuilderService.get_investigators(self.test_study_id) self.assertIsNotNone(response) - self.assertEqual(3, len(response)) + self.assertEqual(5, len(response)) self.assertEqual("DC", response[0]["INVESTIGATORTYPE"]) self.assertEqual("Department Contact", response[0]["INVESTIGATORTYPEFULL"]) self.assertEqual("asd3v", response[0]["NETBADGEID"]) diff --git a/tests/test_tasks_api.py b/tests/test_tasks_api.py index abbf8707..7a7f3c76 100644 --- a/tests/test_tasks_api.py +++ b/tests/test_tasks_api.py @@ -322,7 +322,7 @@ class TestTasksApi(BaseTest): self.assertEqual(4, len(navigation)) # Start task, form_task, multi_task, end task self.assertEqual("UserTask", workflow.next_task.type) self.assertEqual(MultiInstanceType.sequential.value, workflow.next_task.multi_instance_type) - self.assertEqual(3, workflow.next_task.multi_instance_count) + self.assertEqual(5, workflow.next_task.multi_instance_count) # Assure that the names for each task are properly updated, so they aren't all the same. self.assertEqual("Primary Investigator", workflow.next_task.properties['display_name']) @@ -480,15 +480,15 @@ class TestTasksApi(BaseTest): workflow = self.create_workflow('multi_instance_parallel') workflow_api = self.get_workflow_api(workflow) - self.assertEqual(6, len(workflow_api.navigation)) + self.assertEqual(8, len(workflow_api.navigation)) ready_items = [nav for nav in workflow_api.navigation if nav['state'] == "READY"] - self.assertEqual(3, len(ready_items)) + self.assertEqual(5, len(ready_items)) self.assertEqual("UserTask", workflow_api.next_task.type) self.assertEqual("MultiInstanceTask",workflow_api.next_task.name) self.assertEqual("Primary Investigator", workflow_api.next_task.title) - for i in random.sample(range(3), 3): + for i in random.sample(range(5), 5): task = TaskSchema().load(ready_items[i]['task']) rv = self.app.put('/v1.0/workflow/%i/task/%s/set_token' % (workflow.id, task.id), headers=self.logged_in_headers(),