[PATCH 2/3] Include the responsible user in patch-related events

[Johan Herland]

We want to use the events as an audit log of patch state/delegate
changes. An important part of this is recording _who_ changed the
patch state/delegate.

To do this, we need to know the active user (request.user) at the
point where we create the Event instance. Event creation is currently
triggered by signals, but neither the signal handlers, nor the model
classes themselves have easy access to the active user.

However, for the patch-based Events that are relevant to us, we can do
the following hack:

The relevant events are created in signal handlers that are all hooked
up to either the pre_save or post_save signals sent by the save()
method of the Patch model. Before a Patch.save() can happen, naturally,
Patchwork must query Patch.is_editable() to ascertain whether the patch
can in fact be changed by the active user. Hence, we do in fact have
access to the active user (in Patch.is_editable()) before the Event is
created (as a result of the signals emitted by Patch.save()).

Thus, all we really need is a way to communicate this user instance
from Patch.is_editable() to the signal handlers that create the
resulting Events. The Patch object is available in both places, and is
the simplest place to put this information, so we simply add an
._edited_by attribute (which fortunately is not detected as a persistent
database field by Django).

Cc: Mauro Carvalho Chehab <mchehab at osg.samsung.com>
Signed-off-by: Johan Herland <johan at herland.net>
---
 patchwork/models.py            | 6 +++++-
 patchwork/signals.py           | 9 +++++++--
 patchwork/tests/test_events.py | 6 ++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/patchwork/models.py b/patchwork/models.py
index 3d4e222..8b18c6f 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -498,9 +498,13 @@ class Patch(Submission):
             return False
 
         if user in [self.submitter.user, self.delegate]:
+            self._edited_by = user
             return True
 
-        return self.project.is_editable(user)
+        if self.project.is_editable(user):
+            self._edited_by = user
+            return True
+        return False
 
     @property
     def combined_check_state(self):
diff --git a/patchwork/signals.py b/patchwork/signals.py
index 666199b..5aece0b 100644
--- a/patchwork/signals.py
+++ b/patchwork/signals.py
@@ -77,6 +77,7 @@ def create_patch_created_event(sender, instance, created, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_CREATED,
             project=patch.project,
+            user=getattr(patch, '_edited_by', None),
             patch=patch)
 
     # don't trigger for items loaded from fixtures or new items
@@ -93,6 +94,7 @@ def create_patch_state_changed_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_STATE_CHANGED,
             project=patch.project,
+            user=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_state=before,
             current_state=after)
@@ -116,6 +118,7 @@ def create_patch_delegated_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_DELEGATED,
             project=patch.project,
+            user=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_delegate=before,
             current_delegate=after)
@@ -139,6 +142,7 @@ def create_patch_completed_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_COMPLETED,
             project=patch.project,
+            user=getattr(patch, '_edited_by', None),
             patch=patch,
             series=patch.series)
 
@@ -219,10 +223,11 @@ def create_series_completed_event(sender, instance, raw, **kwargs):
     # exists in the wild ('PATCH 5/n'), so we probably want to retest a series
     # in that case.
 
-    def create_event(series):
+    def create_event(series, user):
         return Event.objects.create(
             category=Event.CATEGORY_SERIES_COMPLETED,
             project=series.project,
+            user=user,
             series=series)
 
     # don't trigger for items loaded from fixtures, new items or items that
@@ -241,4 +246,4 @@ def create_series_completed_event(sender, instance, raw, **kwargs):
     # we can't use "series.received_all" here since we haven't actually saved
     # the instance yet so we duplicate that logic here but with an offset
     if (instance.series.received_total + 1) >= instance.series.total:
-        create_event(instance.series)
+        create_event(instance.series, getattr(instance, '_edited_by', None))
diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
index e5c40b5..6af6374 100644
--- a/patchwork/tests/test_events.py
+++ b/patchwork/tests/test_events.py
@@ -110,8 +110,10 @@ class PatchChangedTest(_BaseTestCase):
         patch = utils.create_patch(series=None)
         old_state = patch.state
         new_state = utils.create_state()
+        maintainer = utils.create_maintainer(project=patch.project)
 
         patch.state = new_state
+        self.assertTrue(patch.is_editable(maintainer))
         patch.save()
 
         events = _get_events(patch=patch)
@@ -120,6 +122,7 @@ class PatchChangedTest(_BaseTestCase):
         self.assertEqual(events[1].category,
                          Event.CATEGORY_PATCH_STATE_CHANGED)
         self.assertEqual(events[1].project, patch.project)
+        self.assertEqual(events[1].user, maintainer)
         self.assertEventFields(events[1], previous_state=old_state,
                                current_state=new_state)
 
@@ -127,10 +130,12 @@ class PatchChangedTest(_BaseTestCase):
         # purposefully setting series to None to minimize additional events
         patch = utils.create_patch(series=None)
         delegate_a = utils.create_user()
+        maintainer = utils.create_maintainer(project=patch.project)
 
         # None -> Delegate A
 
         patch.delegate = delegate_a
+        self.assertTrue(patch.is_editable(maintainer))
         patch.save()
 
         events = _get_events(patch=patch)
@@ -139,6 +144,7 @@ class PatchChangedTest(_BaseTestCase):
         self.assertEqual(events[1].category,
                          Event.CATEGORY_PATCH_DELEGATED)
         self.assertEqual(events[1].project, patch.project)
+        self.assertEqual(events[1].user, maintainer)
         self.assertEventFields(events[1], current_delegate=delegate_a)
 
         delegate_b = utils.create_user()
-- 
2.19.2