| Message ID | 20191016224442.9211-1-johan@herland.net |
|---|---|
| Headers | show |
| Series | Store the 'actor' responsible for events | expand |
Johan Herland <johan@herland.net> writes: > The V4L project (https://patchwork.linuxtv.org) uses patch states and > delegates extensively to track progress. We want an audit log to keep > track of the changes made to these patch fields. The Event model already > records this information, but leaves out one crucial detail: which > maintainer/user actually updated the patch state/delegate. The need for > this enhancement is also documented in Issue #73. > > This patch series adds an 'actor' field to the Event model, and - for > applicable events - stores the user responsible for that event (i.e. the > current/active user, if any) into this field. > > This applies to the following events: > - patch-created > - patch-completed > - patch-state-changed > - patch-delegated > - series-completed > - check-created > I keep going back and forth about the sets of events. I still think patch-created is an odd event to try to audit, but OTOH I think setting a precedent of including every event in the audit trail will make it easier to (remember to) extend this in future - I'm particularly thinking about the upcoming relations stuff, which we will definitely want to include in the audit trail. Stephen, what are your thoughts? Regards, Daniel > For the other events (cover-created and series-created), I could not > easily determine if there is a responsible user at all, as these events > are typically generated in response to incoming emails. In these cases > (and any other scenario where we cannot find the active/current user) > the Event.actor field is left as null/None. > > Finally, the new Event.actor field is exposed in the events view of the > REST API (as of API version 1.2). > > Changes since v2: > - Update docs/usage/overview.rst > - Acked-by: Daniel > > > Have fun! > ...Johan > > > Johan Herland (3): > models.Event: Add the user responsible for the event > Include the responsible actor in applicable events > /api/events: Add 'actor' field to generated JSON > > docs/api/schemas/latest/patchwork.yaml | 6 ++++++ > docs/api/schemas/patchwork.j2 | 8 ++++++++ > docs/api/schemas/v1.2/patchwork.yaml | 6 ++++++ > docs/usage/overview.rst | 3 +++ > patchwork/api/event.py | 10 +++++++--- > patchwork/migrations/0037_event_actor.py | 21 +++++++++++++++++++++ > patchwork/models.py | 10 +++++++++- > patchwork/signals.py | 10 ++++++++-- > patchwork/tests/api/test_event.py | 24 ++++++++++++++++++++++++ > patchwork/tests/test_events.py | 7 +++++++ > 10 files changed, 99 insertions(+), 6 deletions(-) > create mode 100644 patchwork/migrations/0037_event_actor.py > > -- > 2.19.2 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Fri, Oct 18, 2019 at 8:29 AM Daniel Axtens <dja@axtens.net> wrote: > Johan Herland <johan@herland.net> writes: > > > The V4L project (https://patchwork.linuxtv.org) uses patch states and > > delegates extensively to track progress. We want an audit log to keep > > track of the changes made to these patch fields. The Event model already > > records this information, but leaves out one crucial detail: which > > maintainer/user actually updated the patch state/delegate. The need for > > this enhancement is also documented in Issue #73. > > > > This patch series adds an 'actor' field to the Event model, and - for > > applicable events - stores the user responsible for that event (i.e. the > > current/active user, if any) into this field. > > > > This applies to the following events: > > - patch-created > > - patch-completed > > - patch-state-changed > > - patch-delegated > > - series-completed > > - check-created > > > > I keep going back and forth about the sets of events. I still think > patch-created is an odd event to try to audit, but OTOH I think setting > a precedent of including every event in the audit trail will make it > easier to (remember to) extend this in future - I'm particularly > thinking about the upcoming relations stuff, which we will definitely > want to include in the audit trail. > > Stephen, what are your thoughts? Ping? Regards, ...Johan
On Mon, 2019-11-11 at 19:30 +0100, Johan Herland wrote: > On Fri, Oct 18, 2019 at 8:29 AM Daniel Axtens <dja@axtens.net> wrote: > > Johan Herland <johan@herland.net> writes: > > > > > The V4L project (https://patchwork.linuxtv.org) uses patch states and > > > delegates extensively to track progress. We want an audit log to keep > > > track of the changes made to these patch fields. The Event model already > > > records this information, but leaves out one crucial detail: which > > > maintainer/user actually updated the patch state/delegate. The need for > > > this enhancement is also documented in Issue #73. > > > > > > This patch series adds an 'actor' field to the Event model, and - for > > > applicable events - stores the user responsible for that event (i.e. the > > > current/active user, if any) into this field. > > > > > > This applies to the following events: > > > - patch-created > > > - patch-completed > > > - patch-state-changed > > > - patch-delegated > > > - series-completed > > > - check-created > > > > > > > I keep going back and forth about the sets of events. I still think > > patch-created is an odd event to try to audit, but OTOH I think setting > > a precedent of including every event in the audit trail will make it > > easier to (remember to) extend this in future - I'm particularly > > thinking about the upcoming relations stuff, which we will definitely > > want to include in the audit trail. > > > > Stephen, what are your thoughts? > > Ping? Sorry, I was on vacation and have been swamped with $dayjob for the past few weeks. Only catching up now. I'm a little confused with patch two. You've said there that only the 'cover-created' or 'series-created' events don't have an 'actor', but I'm not sure how 'patch-created', 'patch-completed' or 'series- completed' could possibly have one. It's not currently possible for a user to manually create a patch, cover letter or series via the web UI or APIs, so these will always be created by the parsemail management command which means a user clearly isn't doing something. How were you expecting this to work? FWIW, I have no real issue with exposing the 'actor' field on all events, even if it's useless for some of them. I just want to make sure you're not going to be suprised by IRL not mapping to the mental model you have of things. Stephen