| Message ID | 20191016224442.9211-3-johan@herland.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Store the 'actor' responsible for events | expand |
On 17/10/19 9:44 am, Johan Herland wrote: > 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. > > 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. How is cover-created different from patch-created? The rest of this seems reasonable.
On Thu, Nov 14, 2019 at 6:31 AM Andrew Donnellan <ajd@linux.ibm.com> wrote: > On 17/10/19 9:44 am, Johan Herland wrote: > > 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. > > > > 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. > > How is cover-created different from patch-created? In practice, it turns out there is no difference, really: The patch-created event is triggered by a signal from the Patch model which potentially carries the ._edited_by attribute. However, AFAICS, when patches are created, there is no preceding call to Patch.is_editable(), hence Patch._edited_by is not set, and we end up passing actor=None to Event.objects.create(). The cover-created event is triggered by a signal from CoverLetter which has no ._edited_by, hence we pass no actor to Events.objects.create() and it defaults to None. I still figured I'd wire up the logic for patch-created, just in case we at some point were to start setting ._edited_by on Patch objects when they are first created. Hope this helps, ...Johan
On 15/11/19 9:37 am, Johan Herland wrote:>>> 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. >> >> How is cover-created different from patch-created? > > In practice, it turns out there is no difference, really: > > The patch-created event is triggered by a signal from the Patch model > which potentially carries the ._edited_by attribute. However, AFAICS, > when patches are created, there is no preceding call to > Patch.is_editable(), hence Patch._edited_by is not set, and we end up > passing actor=None to Event.objects.create(). > > The cover-created event is triggered by a signal from CoverLetter > which has no ._edited_by, hence we pass no actor to > Events.objects.create() and it defaults to None. > > I still figured I'd wire up the logic for patch-created, just in case > we at some point were to start setting ._edited_by on Patch objects > when they are first created. OK. Personally I'd rather we not wire it up, as it's confusing to see something like that wired up and wonder why the field isn't being set. It's also not clear how you should set the actor on email-triggered events as not every sender has a Patchwork account. Andrew
On Thu, 2019-11-14 at 23:37 +0100, Johan Herland wrote: > On Thu, Nov 14, 2019 at 6:31 AM Andrew Donnellan <ajd@linux.ibm.com> wrote: > > On 17/10/19 9:44 am, Johan Herland wrote: > > > 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. > > > > > > 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. > > > > How is cover-created different from patch-created? > > In practice, it turns out there is no difference, really: > > The patch-created event is triggered by a signal from the Patch model > which potentially carries the ._edited_by attribute. However, AFAICS, > when patches are created, there is no preceding call to > Patch.is_editable(), hence Patch._edited_by is not set, and we end up > passing actor=None to Event.objects.create(). > > The cover-created event is triggered by a signal from CoverLetter > which has no ._edited_by, hence we pass no actor to > Events.objects.create() and it defaults to None. > > I still figured I'd wire up the logic for patch-created, just in case > we at some point were to start setting ._edited_by on Patch objects > when they are first created. Doh. I just replied to the cover letter questioning this and here you are saying things are actually working as I expected them to :') I agree with Andrew, that we shouldn't wire up patch-created because (or series-completed, for that matter) since we can't actually support them yet. The commit message should also reflect this. As noted on the cover letter though, I have no issue to exposing the unset 'actor' field for these event types though. It's consistent at least, even if it is mostly useless. Can you respin to drop this? I'm happy to merge this once that's done. Stephen
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])
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. 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+samsung@kernel.org> Signed-off-by: Johan Herland <johan@herland.net> --- patchwork/models.py | 6 +++++- patchwork/signals.py | 10 ++++++++-- patchwork/tests/test_events.py | 7 +++++++ 3 files changed, 20 insertions(+), 3 deletions(-)