Merge pull request #1626 from openhealthcare/1578-active-in-category

1578 active in category

changelog.md

@@ -1,5 +1,19 @@
 ### 0.13.0 (Major Release)
 
+#### Episode.active
+
+The field `Episode.active` was previously implicitly set when calling `.set_tag_names()` to
+something equivalent to the value of `bool(len(tag_names) > 0)`.
+
+As of 0.13.0 the value of `Episode.active` is checked whenever `.save()` is called, prior
+to the database call. The correct value is looked up via `Episode.category.is_active()`.
+
+The default calculation of `.active` has also changed to be roughly equivalent to
+` bool(self.episode.end is None)`.
+
+Applications are now able to easily _change_ this behaviour by overriding the `.is_active`
+method of the relevant `EpisodeCategory`.
+
 #### Coding systems for lookuplists
 
 Lookuplist entries may now have an associated coding system and code value stored against them.

doc/docs/guides/episodes.md

@@ -54,25 +54,6 @@ class Application(application.OpalApplication):
     default_episode_category = MyCategory.display_name
 ```
 
-## Episode stages
-
-An Episode will frequently consist of a number of possible stages. For instance,
-for an inpatient episode, a patient will first be an inpatient, and then an
-be discharged, with an optional interim follow up stage for inpatients who have been
-discharged but requrie further follow up.
-
-Opal stores the stage of an episode as a string in the `stage` property of an
-`Episode`. The valid possible stages for a category are accessed from the
-`get_stages` method of the category.
-
-```
-episode.category.get_stages()
-# ['Inpatient', 'Followup', 'Discharged']
-
-episode.category.has_stage('Followup')
-# True
-```
-
 ## Defining your own EpisodeCategory
 
 As EpisodeCategory is a [discoverable](discoverable) we can define our own to
@@ -92,3 +73,55 @@ class DropInClinicEpisode(episodes.EpisodeCategory):
     detail_template = "detail/drop_in.html"
 
 ```
+
+## Episode.active
+
+The field `.active` is used to distinguish Episodes which are ongoing. This field
+is set implicitly by the `Episode.save()` method, and there will generally be no
+need to set this field directly as part of application code.
+
+Whether an Episode is considered active is determined by the `.is_active()` method
+of the relevant EpisodeCategory.
+
+The default implementation considers any Episode without an `.end` date to be active
+and any Episode with one to be inactive.
+
+Applications may customise this behaviour by overriding the `.is_active()` method.
+
+For instance, to create a category which considered any Episode older than 2 weeks to
+be inactive, one might override the method as follows:
+
+```python
+# yourapp/episode_categories.py
+import datetime
+from opal.core import episodes
+
+
+class TwoWeeksAndStopCaringEpisode(episodes.EpisodeCategory):
+    def is_active(self):
+        delta = datetime.date.today() - self.episode.start
+        return bool(delta >= datetime.timedelta(days=14))
+
+```
+
+(Note that this would not alter the value in the database immediately after those 2
+weeks, but would alter the value the next time the `Episode.save()` method was called.)
+
+## Episode stages
+
+An Episode will frequently consist of a number of possible stages. For instance,
+for an inpatient episode, a patient will first be an inpatient, and then an
+be discharged, with an optional interim follow up stage for inpatients who have been
+discharged but requrie further follow up.
+
+Opal stores the stage of an episode as a string in the `stage` property of an
+`Episode`. The valid possible stages for a category are accessed from the
+`get_stages` method of the category.
+
+```
+episode.category.get_stages()
+# ['Inpatient', 'Followup', 'Discharged']
+
+episode.category.has_stage('Followup')
+# True
+```

doc/docs/reference/episode_categories.md

@@ -65,6 +65,17 @@ InpatientEpisode(episode).has_stage('Inpatient')
 # -> True
 ```
 
+### EpisodeCategory.is_active()
+
+Predicate function to determine whether this episode is active.
+
+The default implementation looks to see whether an end date has
+been set on the episode.
+
+```python
+InpatientEpisode(Episode()).is_active()
+# -> True
+```
 
 ### EpisodeCategory.set_stage(stage, user, data)
 

opal/core/episodes.py

@@ -50,6 +50,15 @@ def episode_visible_to(kls, episode, user):
     def __init__(self, episode):
         self.episode = episode
 
+    def is_active(self):
+        """
+        Predicate function to determine whether this episode is active.
+
+        The default implementation looks to see whether an end date has
+        been set on the episode.
+        """
+        return bool(self.episode.end is None)
+
     def get_stages(self):
         """
         Return the list of string stages for this category

opal/core/exceptions.py

@@ -39,5 +39,9 @@ class MissingTemplateError(Error):
     pass
 
 
+class UnexpectedEpisodeCategoryNameError(Error):
+    pass
+
+
 class InvalidDataError(Error):
     pass

opal/models.py

@@ -689,14 +689,42 @@ class Episode(UpdatesFromDictMixin, TrackedModel):
 
     objects = managers.EpisodeQueryset.as_manager()
 
+    def __init__(self, *args, **kwargs):
+        super(Episode, self).__init__(*args, **kwargs)
+        self.__original_active = self.active
+
     def __unicode__(self):
         return 'Episode {0}: {1} - {2}'.format(
             self.pk, self.start, self.end
         )
 
     def save(self, *args, **kwargs):
         created = not bool(self.id)
+
+        current_active_value = self.active
+        category_active_value = self.category.is_active()
+
+        if current_active_value != category_active_value:  # Disagreement
+            if current_active_value != self.__original_active:
+                # The value of self.active has been set by some code somewhere
+                # not by __init__() e.g. the original database value at the
+                # time of instance initalization.
+                #
+                # Rather than overriding this silently we should raise a
+                # ValueError.
+                msg = "Value of Episode.active has been set to {} but " \
+                      "category.is_active() returns {}"
+                raise ValueError(
+                    msg.format(current_active_value, category_active_value)
+                )
+
+        self.active = category_active_value
         super(Episode, self).save(*args, **kwargs)
+
+        # Re-set this in case we changed it once post initialization and then
+        # the user subsequently saves this instance again
+        self.__original_active = self.active
+
         if created:
             for subclass in episode_subrecords():
                 if subclass._is_singleton:
@@ -705,10 +733,16 @@ def save(self, *args, **kwargs):
     @property
     def category(self):
         from opal.core import episodes
-        category = episodes.EpisodeCategory.filter(
+        categories = episodes.EpisodeCategory.filter(
             display_name=self.category_name
-        )[0]
-        return category(self)
+        )
+        if len(categories) == 0:
+            msg = "Unable to find EpisodeCategory for category name {0}"
+            msg = msg.format(self.category_name)
+            raise exceptions.UnexpectedEpisodeCategoryNameError(msg)
+        else:
+            category = categories[0]
+            return category(self)
 
     def visible_to(self, user):
         """
@@ -731,20 +765,12 @@ def set_stage(self, stage, user, data):
 
     def set_tag_names(self, tag_names, user):
         """
-        1. Set the episode.active status
-        2. Special case mine
-        3. Archive dangling tags not in our current list.
-        4. Add new tags.
-        5. Ensure that we're setting the parents of child tags
-        6. There is no step 6.
+        1. Special case mine
+        2. Archive dangling tags not in our current list.
+        3. Add new tags.
+        4. Ensure that we're setting the parents of child tags
+        5. There is no step 6.
         """
-        if len(tag_names) and not self.active:
-            self.active = True
-            self.save()
-        elif not len(tag_names) and self.active:
-            self.active = False
-            self.save()
-
         if "mine" not in tag_names:
             self.tagging_set.filter(user=user,
                                     value='mine').update(archived=True)

opal/tests/test_core_episodes.py

@@ -1,6 +1,8 @@
 """
 Unittests for opal.core.episodes
 """
+import datetime
+
 from django.contrib.auth.models import User
 
 from opal.core import test
@@ -17,7 +19,7 @@ def setUp(self):
         )
         self.patient = Patient.objects.create()
         self.inpatient_episode = self.patient.create_episode(
-            category_name=episodes.InpatientEpisode
+            category_name=episodes.InpatientEpisode.display_name
         )
 
     def test_episode_categories(self):
@@ -42,6 +44,15 @@ def test_for_category(self):
         self.assertEqual(episodes.InpatientEpisode,
                          episodes.EpisodeCategory.get('inpatient'))
 
+    def test_is_active(self):
+        category = episodes.InpatientEpisode(self.inpatient_episode)
+        self.assertTrue(category.is_active())
+
+    def test_is_active_end_date_set(self):
+        self.inpatient_episode.end = datetime.date.today()
+        category = episodes.InpatientEpisode(self.inpatient_episode)
+        self.assertFalse(category.is_active())
+
     def test_get_stages(self):
         stages = [
             'Inpatient',

opal/tests/test_episode.py

@@ -7,10 +7,11 @@
 
 from django.contrib.auth.models import User
 
-from opal.core import application, subrecords
+from opal.core import application, exceptions, subrecords
 from opal.core.episodes import InpatientEpisode
 from opal.core.test import OpalTestCase
 from opal.models import Patient, Episode, Tagging, UserProfile
+from opal import models
 
 from opal.tests import test_patient_lists # ensure the lists are loaded
 from opal.tests.models import (
@@ -27,10 +28,32 @@ def setUp(self):
     def test_singleton_subrecord_created(self):
         self.assertEqual(1, self.episode.episodename_set.count())
 
+    def test_init_sets_original_active_value(self):
+        episode = models.Episode()
+        self.assertEqual(episode.active, episode._Episode__original_active)
+
+    def test_save_sets_active_when_category_is_active_is_true(self):
+        with patch.object(self.episode.category, 'is_active') as activep:
+            activep.return_value = True
+            self.episode.save()
+            self.assertTrue(self.episode.active)
+
+    def test_save_sets_active_when_category_is_active_is_false(self):
+        self.episode.end = datetime.date.today()
+        self.episode.save()
+        self.assertFalse(self.episode.active)
+
+    def test_user_value_of_active_disagrees_with_category(self):
+        self.episode.active = False
+        with self.assertRaises(ValueError):
+            self.episode.save()
+
     @patch('opal.models.application.get_app')
-    def test_default_category_name(self, getter):
+    @patch('opal.core.episodes.EpisodeCategory')
+    def test_default_category_name(self, category, getter):
         mock_app = getter.return_value
         mock_app.default_episode_category = 'MyEpisodeCategory'
+        category.filter.return_value = [InpatientEpisode]
         episode = self.patient.create_episode()
         self.assertEqual('MyEpisodeCategory', episode.category_name)
 
@@ -43,6 +66,11 @@ def test_category(self):
         self.assertEqual(self.episode.category.__class__, InpatientEpisode)
         self.assertEqual(self.episode.category.episode, self.episode)
 
+    def test_category_name_not_found(self):
+        self.episode.category_name = 'Not A Real Category'
+        with self.assertRaises(exceptions.UnexpectedEpisodeCategoryNameError):
+            cat = self.episode.category
+
     def test_visible_to(self):
         self.assertTrue(self.episode.visible_to(self.user))
 
@@ -89,13 +117,17 @@ def test_user_cannot_see_other_users_mine_tag(self):
         self.episode.set_tag_names(['carnivore', 'mine'], self.user)
         self.assertEqual(['carnivore'], list(self.episode.get_tag_names(other_user)))
 
-    def test_active_if_tagged_by_non_mine_tag(self):
+    def test_active_unchanged_if_tagged_by_non_mine_tag(self):
+        # Regression test - see github.com/openhealthcare/opal#1578 for details
+        before = self.episode.active
         self.episode.set_tag_names(['carnivore'], self.user)
-        self.assertTrue(self.episode.active)
+        self.assertEqual(before, self.episode.active)
 
-    def test_active_if_only_tagged_by_mine_tag(self):
+    def test_active_unchanged_if_only_tagged_by_mine_tag(self):
+        # Regression test - see github.com/openhealthcare/opal#1578 for details
+        before = self.episode.active
         self.episode.set_tag_names(['mine'], self.user)
-        self.assertTrue(self.episode.active)
+        self.assertEqual(before, self.episode.active)
 
     def test_set_tag_names_from_tagging_dict(self):
         self.episode.set_tag_names_from_tagging_dict({'inpatient': True}, self.user)

opal/tests/test_models.py

@@ -9,7 +9,7 @@
 from django.utils import timezone
 
 from opal import models
-from opal.core import exceptions, subrecords
+from opal.core import application, exceptions, subrecords
 from opal.models import (
     Subrecord, Tagging, Patient, InpatientAdmission, Symptom,
 )
@@ -64,18 +64,17 @@ def test_to_dict_episode_identical_to_episode_to_dict(self):
         episode_dict = episode.to_dict(self.user)
         self.assertEqual(episode_dict, patient.to_dict(self.user)['episodes'][episode.id])
 
-    @patch("opal.models.application.get_app")
-    def test_created_with_the_default_episode(self, get_app):
-        test_app = MagicMock()
-        test_app.default_episode_category ="testcategory"
-        get_app.return_value = test_app
+    def test_created_with_the_default_episode(self):
         _, episode = self.new_patient_and_episode_please()
-        self.assertEqual(episode.category_name, "testcategory")
+        self.assertEqual(
+            application.get_app().default_episode_category,
+            episode.category_name
+        )
 
     def test_create_episode_category(self):
         patient = models.Patient.objects.create()
-        e = patient.create_episode(category_name='testcategory')
-        self.assertEqual('testcategory', e.category_name)
+        e = patient.create_episode(category_name='Outpatient')
+        self.assertEqual('Outpatient', e.category_name)
 
     def test_bulk_update_patient_subrecords(self):
         original_patient = models.Patient()