Some minor cleanup on the study status and automatic events. I wanted to avoid having one database model automatically generating other database models as a side effect. The study service now has full responsiblity to recording study events.

To help in running tests, adding __init__.py methods to all the test directories.

crc/api/study.py

@@ -23,14 +23,12 @@ def add_study(body):
                              primary_investigator_id=body['primary_investigator_id'],
                              last_updated=datetime.now(),
                              status=StudyStatus.in_progress)
-
-    study_model.update_event(
-        status=StudyStatus.in_progress,
-        event_type=StudyEventType.automatic,
-        user_uid=g.user.uid
-    )
-
     session.add(study_model)
+    StudyService.add_study_update_event(study_model,
+                                        status=StudyStatus.in_progress,
+                                        event_type=StudyEventType.user,
+                                        user_uid=g.user.uid)
+
     errors = StudyService._add_all_workflow_specs_to_study(study_model)
     session.commit()
     study = StudyService().get_study(study_model.id)
@@ -40,6 +38,7 @@ def add_study(body):
 
 
 def update_study(study_id, body):
+    """Pretty limited, but allows manual modifications to the study status """
     if study_id is None:
         raise ApiError('unknown_study', 'Please provide a valid Study ID.')
 
@@ -48,7 +47,20 @@ def update_study(study_id, body):
         raise ApiError('unknown_study', 'The study "' + study_id + '" is not recognized.')
 
     study: Study = StudyForUpdateSchema().load(body)
-    study.update_model(study_model)
+
+    status = StudyStatus(study.status)
+    study_model.last_updated = datetime.now()
+
+    if study_model.status != status:
+        study_model.status = status
+        StudyService.add_study_update_event(study_model, status, StudyEventType.user,
+                                            user_uid=UserService.current_user().uid if UserService.has_user() else None,
+                                            comment='' if not hasattr(study, 'comment') else study.comment,
+                                            )
+
+    if status == StudyStatus.open_for_enrollment:
+        study_model.enrollment_date = study.enrollment_date
+
     session.add(study_model)
     session.commit()
     # Need to reload the full study to return it to the frontend

crc/models/study.py

@@ -61,36 +61,6 @@ class StudyModel(db.Model):
 
         self.irb_status = IrbStatus.incomplete_in_protocol_builder
         self.status = StudyStatus.in_progress
-        self.update_event(
-            status=StudyStatus.in_progress,
-            event_type=StudyEventType.automatic,
-            user_uid=self.user_uid
-        )
-
-        if pbs.HSRNUMBER:
-            self.irb_status = IrbStatus.hsr_assigned
-            self.status = StudyStatus.open_for_enrollment
-            self.update_event(
-                status=StudyStatus.open_for_enrollment,
-                event_type=StudyEventType.automatic,
-                user_uid=self.user_uid
-            )
-        if self.on_hold:
-            self.status = StudyStatus.hold
-            self.update_event(
-                status=StudyStatus.hold,
-                event_type=StudyEventType.automatic,
-                user_uid=self.user_uid
-            )
-
-    def update_event(self, status, event_type, user_uid, comment=''):
-        study_event = StudyEvent(study=self,
-                                 status=status,
-                                 event_type=event_type,
-                                 user_uid=user_uid,
-                                 comment=comment)
-        db.session.add(study_event)
-        db.session.commit()
 
 
 class StudyEvent(db.Model):
@@ -202,24 +172,6 @@ class Study(object):
         instance = cls(**args)
         return instance
 
-    def update_model(self, study_model: StudyModel):
-        """As the case for update was very reduced, it's mostly and specifically
-        updating only the study status and generating a history record
-        """
-        status = StudyStatus(self.status)
-        study_model.last_updated = datetime.datetime.now()
-        study_model.status = status
-
-        if status == StudyStatus.open_for_enrollment:
-            study_model.enrollment_date = self.enrollment_date
-
-        study_model.update_event(
-            status=status,
-            comment='' if not hasattr(self, 'comment') else self.comment,
-            event_type=StudyEventType.user,
-            user_uid=UserService.current_user().uid if UserService.has_user() else None
-        )
-
     def model_args(self):
         """Arguments that can be passed into the Study Model to update it."""
         self_dict = self.__dict__.copy()

crc/services/study_service.py

@@ -12,7 +12,8 @@ from crc.api.common import ApiError
 from crc.models.file import FileModel, FileModelSchema, File
 from crc.models.ldap import LdapSchema
 from crc.models.protocol_builder import ProtocolBuilderStudy, ProtocolBuilderStatus
-from crc.models.study import StudyModel, Study, StudyStatus, Category, WorkflowMetadata, StudyEventType
+from crc.models.study import StudyModel, Study, StudyStatus, Category, WorkflowMetadata, StudyEventType, StudyEvent, \
+    IrbStatus
 from crc.models.task_event import TaskEventModel, TaskEvent
 from crc.models.workflow import WorkflowSpecCategoryModel, WorkflowModel, WorkflowSpecModel, WorkflowState, \
     WorkflowStatus
@@ -229,9 +230,9 @@ class StudyService(object):
     @staticmethod
     def get_protocol(study_id):
         """Returns the study protocol, if it has been uploaded."""
-        file = db.session.query(FileModel)\
+        file = db.session.query(FileModel) \
-            .filter_by(study_id=study_id)\
+            .filter_by(study_id=study_id) \
-            .filter_by(form_field_key='Study_Protocol_Document')\
+            .filter_by(form_field_key='Study_Protocol_Document') \
             .first()
 
         return FileModelSchema().dump(file)
@@ -253,30 +254,54 @@ class StudyService(object):
             db_studies = session.query(StudyModel).filter_by(user_uid=user.uid).all()
 
             # Update all studies from the protocol builder, create new studies as needed.
-            # Futher assures that every active study (that does exist in the protocol builder)
+            # Further assures that every active study (that does exist in the protocol builder)
             # has a reference to every available workflow (though some may not have started yet)
             for pb_study in pb_studies:
+                new_status = None
                 db_study = next((s for s in db_studies if s.id == pb_study.STUDYID), None)
                 if not db_study:
                     db_study = StudyModel(id=pb_study.STUDYID)
+                    db_study.status = None  # Force a new sa
+                    new_status = StudyStatus.in_progress
                     session.add(db_study)
                     db_studies.append(db_study)
+
+                if pb_study.HSRNUMBER:
+                    db_study.irb_status = IrbStatus.hsr_assigned
+                    if db_study.status != StudyStatus.open_for_enrollment:
+                        new_status = StudyStatus.open_for_enrollment
+
                 db_study.update_from_protocol_builder(pb_study)
                 StudyService._add_all_workflow_specs_to_study(db_study)
 
+                # If there is a new automatic status change and there isn't a manual change in place, record it.
+                if new_status and db_study.status != StudyStatus.hold:
+                    db_study.status = new_status
+                    StudyService.add_study_update_event(db_study,
+                                                        status=new_status,
+                                                        event_type=StudyEventType.automatic)
+
             # Mark studies as inactive that are no longer in Protocol Builder
             for study in db_studies:
                 pb_study = next((pbs for pbs in pb_studies if pbs.STUDYID == study.id), None)
-                if not pb_study:
+                if not pb_study and study.status != StudyStatus.abandoned:
                     study.status = StudyStatus.abandoned
-                    study.update_event(
+                    StudyService.add_study_update_event(study,
                                                         status=StudyStatus.abandoned,
-                        event_type=StudyEventType.automatic,
+                                                        event_type=StudyEventType.automatic)
-                        user_uid=study.user_uid
-                    )
 
             db.session.commit()
 
+    @staticmethod
+    def add_study_update_event(study, status, event_type, user_uid=None, comment=''):
+        study_event = StudyEvent(study=study,
+                                 status=status,
+                                 event_type=event_type,
+                                 user_uid=user_uid,
+                                 comment=comment)
+        db.session.add(study_event)
+        db.session.commit()
+
     @staticmethod
     def __update_status_of_workflow_meta(workflow_metas, status):
         # Update the status on each workflow
@@ -322,7 +347,7 @@ class StudyService(object):
         return WorkflowProcessor.run_master_spec(master_specs[0], study_model)
 
     @staticmethod
-    def _add_all_workflow_specs_to_study(study_model:StudyModel):
+    def _add_all_workflow_specs_to_study(study_model: StudyModel):
         existing_models = session.query(WorkflowModel).filter(WorkflowModel.study == study_model).all()
         existing_specs = list(m.workflow_spec_id for m in existing_models)
         new_specs = session.query(WorkflowSpecModel). \

tests/study/test_study_api.py

@@ -135,7 +135,7 @@ class TestStudyApi(BaseTest):
         study_event = session.query(StudyEvent).first()
         self.assertIsNotNone(study_event)
         self.assertEqual(study_event.status, StudyStatus.in_progress)
-        self.assertEqual(study_event.event_type, StudyEventType.automatic)
+        self.assertEqual(study_event.event_type, StudyEventType.user)
         self.assertFalse(study_event.comment)
         self.assertEqual(study_event.user_uid, self.test_uid)
 
@@ -145,7 +145,7 @@ class TestStudyApi(BaseTest):
         study: StudyModel = session.query(StudyModel).first()
         study.title = "Pilot Study of Fjord Placement for Single Fraction Outcomes to Cortisol Susceptibility"
         study_schema = StudySchema().dump(study)
-        study_schema['status'] = StudyStatus.in_progress.value
+        study_schema['status'] = StudyStatus.hold.value
         study_schema['comment'] = update_comment
         rv = self.app.put('/v1.0/study/%i' % study.id,
                           content_type="application/json",
@@ -159,7 +159,7 @@ class TestStudyApi(BaseTest):
         # Making sure events history is being properly recorded
         study_event = session.query(StudyEvent).first()
         self.assertIsNotNone(study_event)
-        self.assertEqual(study_event.status, StudyStatus.in_progress)
+        self.assertEqual(study_event.status, StudyStatus.hold)
         self.assertEqual(study_event.event_type, StudyEventType.user)
         self.assertEqual(study_event.comment, update_comment)
         self.assertEqual(study_event.user_uid, self.test_uid)
@@ -221,7 +221,7 @@ class TestStudyApi(BaseTest):
 
         # Automatic events check
         in_progress_events = session.query(StudyEvent).filter_by(status=StudyStatus.in_progress)
-        self.assertEqual(in_progress_events.count(), 3)  # 3 studies were started
+        self.assertEqual(in_progress_events.count(), 1)  # 1 study is in progress
 
         abandoned_events = session.query(StudyEvent).filter_by(status=StudyStatus.abandoned)
         self.assertEqual(abandoned_events.count(), 1)  # 1 study has been abandoned