Include the responsible actor in applicable events

We want to use the events as an audit log. An important part of this is
recording _who_ made the changes that the events represent.

To accomplish this, we need to know the current user (aka. 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 request.user.

For some Patch-based events (patch-state-changed, patch-delegated), 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 Patch.save(). But before calling Patch.save(),
Patchwork must naturally query Patch.is_editable() to ascertain whether
the patch can in fact be changed by the current user. Thus, we only
need a way to communicate the current user from Patch.is_editable()
to the signal handlers that create the resulting Events. The Patch
object itself is available in both places, so we simply add an
'_edited_by' attribute to the instance (which fortunately is not
detected as a persistent db field by Django).

For the check-created event the current user always happens to be the
same as the 'user' field recorded in the Check object itself.

For the other Patch-based events (patch-created, patch-completed, and
series-completed), although they are also triggered by Patch.save(),
they are triggered as a result of incoming emails, hence have no real
actor as such, so we simply leave the actor as None/NULL. The same
argument also applies to the cover-created and series-created events.

Signed-off-by: Johan Herland <johan@herland.net>
Reviewed-by: Stephen Finucane <stephen@that.guru>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/patchwork/models.py b/patchwork/models.py
index b4f3bef685fbff309164f96d201f29865b5dcd75..7f0efd489ae3710ec8054f587dc9a02e0a5612cf 100644 (file)
--- 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 666199b68161f8e63d4b258c62f961e7ba77e9ea..73ddfa5e35eebe60e0963be7140028f0c3262261 100644 (file)
--- a/patchwork/signals.py
+++ b/patchwork/signals.py
@@ -93,6 +93,7 @@ def create_patch_state_changed_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_STATE_CHANGED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_state=before,
             current_state=after)
@@ -116,6 +117,7 @@ def create_patch_delegated_event(sender, instance, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_DELEGATED,
             project=patch.project,
+            actor=getattr(patch, '_edited_by', None),
             patch=patch,
             previous_delegate=before,
             current_delegate=after)
@@ -184,6 +186,7 @@ def create_check_created_event(sender, instance, created, raw, **kwargs):
         return Event.objects.create(
             category=Event.CATEGORY_CHECK_CREATED,
             project=check.patch.project,
+            actor=check.user,
             patch=check.patch,
             created_check=check)
 

diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
index e5c40b558cf1a5c8a6c523089db94a50765bb4ed..415237f9787ff649c55006bb1090d8c742fa200b 100644 (file)
--- 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()
+        actor = utils.create_maintainer(project=patch.project)
 
         patch.state = new_state
+        self.assertTrue(patch.is_editable(actor))
         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].actor, actor)
         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()
+        actor = utils.create_maintainer(project=patch.project)
 
         # None -> Delegate A
 
         patch.delegate = delegate_a
+        self.assertTrue(patch.is_editable(actor))
         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].actor, actor)
         self.assertEventFields(events[1], current_delegate=delegate_a)
 
         delegate_b = utils.create_user()
@@ -175,6 +181,7 @@ class CheckCreatedTest(_BaseTestCase):
         self.assertEqual(events.count(), 1)
         self.assertEqual(events[0].category, Event.CATEGORY_CHECK_CREATED)
         self.assertEqual(events[0].project, check.patch.project)
+        self.assertEqual(events[0].actor, check.user)
         self.assertEventFields(events[0])