[PATCH 2/3] Include the responsible user in patch-related events
Stephen Finucane
stephen at that.guru
Tue Oct 8 03:06:41 AEDT 2019
On Mon, 2019-10-07 at 00:57 +0200, Johan Herland wrote:
> 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.
I ran into the same issue when trying to resolve this (tracked via [1],
which you could probably refer to in your commit messages)...
> 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).
We _could_ do this but, as you note, it is a hack. Before we commit,
it's probably worth exploring the other options we have. From my
investigation into this in the distant past, there seemed to be two
potential solutions: using middleware instead of events to intercept
the request and create events, adding 'last_modified_by' fields to
various models and saving the user there for consumption when creating
the event, and modifying the 'save' methods of the various models. The
first option was the one I pursued but I ended up not following through
with it because (a) it was hard to read/test and (b) it had to be
enabled by the admin of the instance (via adding it to 'MIDDLEWARE' in
settings). I don't recall trying out the other two approaches but the
latter one (modifying 'save' methods) seems like it would do the trick,
at the expense of making the 'models.py' file even longer. Did you
check this approach out and, if so, how did it work?
Thanks for taking a look into this. Much appreciated :)
Stephen
[1] https://github.com/getpatchwork/patchwork/issues/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 | 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()
More information about the Patchwork
mailing list