From 693d5dff30be45e0f9f11e76e10022d114d72294 Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Tue, 30 Oct 2018 14:53:13 +0000
Subject: [PATCH 1/6] Stop implicitly setting Episode.active to
 len(get_tag_names()) > 0

refs #1578
---
 opal/models.py             | 18 +++++-------------
 opal/tests/test_episode.py | 12 ++++++++----
 opal/tests/test_patient.py |  3 ++-
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/opal/models.py b/opal/models.py
index f4f405d85..f164f7543 100644
--- a/opal/models.py
+++ b/opal/models.py
@@ -751,20 +751,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)
diff --git a/opal/tests/test_episode.py b/opal/tests/test_episode.py
index 83f8c9ba1..bc627b49e 100644
--- a/opal/tests/test_episode.py
+++ b/opal/tests/test_episode.py
@@ -85,13 +85,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)
diff --git a/opal/tests/test_patient.py b/opal/tests/test_patient.py
index 0d05a7179..6f6d98223 100644
--- a/opal/tests/test_patient.py
+++ b/opal/tests/test_patient.py
@@ -23,7 +23,8 @@ def test_can_create_episode(self):
     def test_get_active_episode(self):
         self.patient.create_episode()
         episode2 = self.patient.create_episode()
-        episode2.set_tag_names(['microbiology'], None)
+        episode2.active = True
+        episode2.save()
         self.assertEqual(episode2.id, self.patient.get_active_episode().id)
 
     def test_get_active_episode_with_no_episodes(self):

From 9a0c7d77745a7e7310ba3d69e2e6c52581e1229f Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Wed, 31 Oct 2018 15:21:20 +0000
Subject: [PATCH 2/6] Set Episode.active in the Episode.save() method. refs
 #1578

---
 opal/core/episodes.py            |  9 +++++++++
 opal/core/exceptions.py          |  4 ++++
 opal/models.py                   | 13 ++++++++++---
 opal/tests/test_core_episodes.py | 13 ++++++++++++-
 opal/tests/test_episode.py       | 23 +++++++++++++++++++++--
 opal/tests/test_models.py        | 17 ++++++++---------
 opal/tests/test_patient.py       |  6 ++++--
 7 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/opal/core/episodes.py b/opal/core/episodes.py
index 08b900726..1445df59b 100644
--- a/opal/core/episodes.py
+++ b/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 == None)
+
     def get_stages(self):
         """
         Return the list of string stages for this category
diff --git a/opal/core/exceptions.py b/opal/core/exceptions.py
index 9f2f05a87..1c152d6b8 100644
--- a/opal/core/exceptions.py
+++ b/opal/core/exceptions.py
@@ -37,3 +37,7 @@ class InitializationError(Error):
 
 class MissingTemplateError(Error):
     pass
+
+
+class UnexpectedEpisodeCategoryNameError(Error):
+    pass
diff --git a/opal/models.py b/opal/models.py
index f164f7543..84050b6f1 100644
--- a/opal/models.py
+++ b/opal/models.py
@@ -716,6 +716,7 @@ def __unicode__(self):
 
     def save(self, *args, **kwargs):
         created = not bool(self.id)
+        self.active = self.category.is_active()
         super(Episode, self).save(*args, **kwargs)
         if created:
             for subclass in episode_subrecords():
@@ -725,10 +726,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):
         """
diff --git a/opal/tests/test_core_episodes.py b/opal/tests/test_core_episodes.py
index 8c701da17..d72f4e75a 100644
--- a/opal/tests/test_core_episodes.py
+++ b/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',
diff --git a/opal/tests/test_episode.py b/opal/tests/test_episode.py
index bc627b49e..83a46fc53 100644
--- a/opal/tests/test_episode.py
+++ b/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,23 @@ def setUp(self):
     def test_singleton_subrecord_created(self):
         self.assertEqual(1, self.episode.episodename_set.count())
 
+    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)
+
     @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)
 
@@ -39,6 +53,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))
 
diff --git a/opal/tests/test_models.py b/opal/tests/test_models.py
index 24a7b29b7..fefdd74de 100644
--- a/opal/tests/test_models.py
+++ b/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()
diff --git a/opal/tests/test_patient.py b/opal/tests/test_patient.py
index 6f6d98223..aea7049d4 100644
--- a/opal/tests/test_patient.py
+++ b/opal/tests/test_patient.py
@@ -1,6 +1,8 @@
 """
 Unittests for Patients
 """
+import datetime
+
 from mock import patch
 from opal.core.test import OpalTestCase
 
@@ -31,6 +33,6 @@ def test_get_active_episode_with_no_episodes(self):
         self.assertIsNone(self.patient.get_active_episode())
 
     def test_get_active_episode_with_no_active_episodes(self):
-        self.patient.create_episode()
-        self.patient.create_episode()
+        self.patient.create_episode(end=datetime.date.today())
+        self.patient.create_episode(end=datetime.date.today())
         self.assertIsNone(self.patient.get_active_episode())

From 5ce427366394a280b84d0488498d1469f3d150de Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Wed, 31 Oct 2018 15:49:00 +0000
Subject: [PATCH 3/6] Fixup: Linter insists on x is None syntax.

---
 opal/core/episodes.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/opal/core/episodes.py b/opal/core/episodes.py
index 1445df59b..a4ed3e1ac 100644
--- a/opal/core/episodes.py
+++ b/opal/core/episodes.py
@@ -57,7 +57,7 @@ def is_active(self):
         The default implementation looks to see whether an end date has
         been set on the episode.
         """
-        return bool(self.episode.end == None)
+        return bool(self.episode.end is None)
 
     def get_stages(self):
         """

From 8e59805220c45a0a5c06aa5497702924bba5639f Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Wed, 31 Oct 2018 16:01:40 +0000
Subject: [PATCH 4/6] If code has set Episode.active() to be different to the
 value set by Episode.save() raise an exception rather than silently change
 it.

refs #1578
---
 opal/models.py                   | 29 ++++++++++++++++++++++++++++-
 opal/tests/test_episode.py       |  9 +++++++++
 opal/tests/test_patient_lists.py |  5 +++--
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/opal/models.py b/opal/models.py
index 84050b6f1..108e66c71 100644
--- a/opal/models.py
+++ b/opal/models.py
@@ -700,6 +700,10 @@ 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):
         try:
             demographics = self.patient.demographics()
@@ -716,8 +720,31 @@ def __unicode__(self):
 
     def save(self, *args, **kwargs):
         created = not bool(self.id)
-        self.active = self.category.is_active()
+
+        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:
diff --git a/opal/tests/test_episode.py b/opal/tests/test_episode.py
index 83a46fc53..9e73d2e4b 100644
--- a/opal/tests/test_episode.py
+++ b/opal/tests/test_episode.py
@@ -28,6 +28,10 @@ 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
@@ -39,6 +43,11 @@ def test_save_sets_active_when_category_is_active_is_false(self):
         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')
     @patch('opal.core.episodes.EpisodeCategory')
     def test_default_category_name(self, category, getter):
diff --git a/opal/tests/test_patient_lists.py b/opal/tests/test_patient_lists.py
index e97c8536f..23fa89ab6 100644
--- a/opal/tests/test_patient_lists.py
+++ b/opal/tests/test_patient_lists.py
@@ -1,6 +1,7 @@
 """
 Unittests for opal.core.patient_lists
 """
+import datetime
 import os
 
 from django.core.urlresolvers import reverse
@@ -268,7 +269,7 @@ def test_to_dict_passes_queryset(self, serialised):
 
     def test_to_dict_inactive_episodes(self):
         p, e = self.new_patient_and_episode_please()
-        e.active = False
+        e.end = datetime.date.today()
         e.save()
 
         class All(patient_lists.PatientList):
@@ -454,7 +455,7 @@ def test_to_dict_inactive_episodes(self):
         p, e = self.new_patient_and_episode_please()
         e.set_tag_names(['carnivore'], self.user)
         self.assertEqual(e.pk, TaggingTestNotSubTag().to_dict(self.user)[0]['id'])
-        e.active = False
+        e.end = datetime.date.today()
         e.save()
         self.assertEqual(1, len(TaggingTestNotSubTag().to_dict(self.user)))
 

From 487c95904351ee7259ffeba1fe4482465740ac04 Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Wed, 31 Oct 2018 16:18:02 +0000
Subject: [PATCH 5/6] Add Documentation: Changelog note for Episode.active
 alteration in 0.13 Reference docs for EpisodeCategory.is_active refs #1578

---
 changelog.md                             | 14 ++++++++++++++
 doc/docs/reference/episode_categories.md | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/changelog.md b/changelog.md
index c748a8632..b05032df2 100644
--- a/changelog.md
+++ b/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`.
+
 #### New date display format helpers
 
 Introduces two new Angular filters: `displayDate` and `displayDateTime`. These format a date
diff --git a/doc/docs/reference/episode_categories.md b/doc/docs/reference/episode_categories.md
index a432fde3c..0010eaa47 100644
--- a/doc/docs/reference/episode_categories.md
+++ b/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)
 

From 13abc302f29092c59564d3c7d93cd249676a266b Mon Sep 17 00:00:00 2001
From: David Miller <david@deadpansincerity.com>
Date: Wed, 31 Oct 2018 16:33:39 +0000
Subject: [PATCH 6/6] Documentation for episode.active and how to override refs
 #1578

---
 doc/docs/guides/episodes.md | 71 +++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/doc/docs/guides/episodes.md b/doc/docs/guides/episodes.md
index d7cae2ebd..b69ebb9cb 100644
--- a/doc/docs/guides/episodes.md
+++ b/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
+```