[PATCH v2 2/3] Include the responsible actor in applicable events
Daniel Axtens
dja at axtens.net
Wed Oct 9 17:04:33 AEDT 2019
Johan Herland <johan at herland.net> writes:
> 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 Patch-based events (patch-created, patch-state-changed,
> patch-delegated, patch-completed), 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).
>
> The series-completed event is also triggered by Patch.save(), so uses
> the same trick as above.
Aren't patch-created and series-completed usually triggered by incoming
emails also? (like with cover-created)
What happens if you use parsemail or parsearchive to load mail in? Is
the actor always NULL for patch-created in this case?
Regards,
Daniel
>
> For the check-created event the current user always happens to be the
> same as the .user field recorded in the Check object itself.
>
> The remaining events (cover-created, series-created) are both triggered
> by incoming emails, hence have no real actor as such, so we simply leave
> the actor as None/NULL.
>
> Closes: #73
> 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 | 10 ++++++++--
> patchwork/tests/test_events.py | 7 +++++++
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/patchwork/models.py b/patchwork/models.py
> index b43c15a..f37572e 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..987cc5a 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,
> + actor=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,
> + actor=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,
> + actor=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,
> + actor=getattr(patch, '_edited_by', None),
> patch=patch,
> series=patch.series)
>
> @@ -184,6 +188,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)
>
> @@ -219,10 +224,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, actor):
> return Event.objects.create(
> category=Event.CATEGORY_SERIES_COMPLETED,
> project=series.project,
> + actor=actor,
> series=series)
>
> # don't trigger for items loaded from fixtures, new items or items that
> @@ -241,4 +247,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..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])
>
>
> --
> 2.19.2
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list