From 79a3efb63a3ac2dea0382a4d6ec1b675a7b154c9 Mon Sep 17 00:00:00 2001 From: Johan Herland Date: Sun, 1 Dec 2019 02:49:53 +0100 Subject: 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 Reviewed-by: Stephen Finucane Cc: Mauro Carvalho Chehab --- patchwork/models.py | 6 +++++- patchwork/signals.py | 3 +++ patchwork/tests/test_events.py | 7 +++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/patchwork/models.py b/patchwork/models.py index b4f3bef..7f0efd4 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..73ddfa5 100644 --- 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 e5c40b5..415237f 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() + 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]) -- cgit v1.2.3