Spike: Rationalize logging event-types

[iaindillingham]

It's likely that we'll be asked to log more event-types than we do at present. (We're talking specifically about the audit log, rather than logging in general.)

https://github.com/opensafely-core/job-server/blob/935ae263364a2bf0af34f05ff7c3375b5706ef4b/jobserver/models/auditable_event.py#L14-L19

Before we do, we should consider whether:

• we can reduce the amount of code that's needed to log more event-types
• we can make logging more event-types safer (i.e. whether we can make it harder to bypass Model.save, QuerySet.create, etc.)
• we can be more consistent with our use of fixtures and factories in our tests (we have a project_membership fixture but an OrgMembershipFactory)

@lucyb and I feel that having a commands module makes sense. We also feel that calling it actions, to avoid confusion with Django's management commands and for consistency with opencodelists (see opencodelists/opencodelists/actions.py) makes sense, too. @inglesp has written up some design decisions, which will be helpful, as will this Slack thread, where @bloodearnest points out the similarity with CQRS.

[iaindillingham]

I've spent some time thinking about how best to structure the commands in jobserver.commands.

One idea was to replace each module with a class, such that the functions in each module became the methods of each class. The classes would inherit from an ABC (an Abstract Base Class), meaning they would implement a common set of methods. If they didn't, then a TypeError would be raised at runtime. Having read "Interfaces, Protocols, and ABCs" in Fluent Python, I felt that some of these methods could be concrete, which would reduce the repetition within jobserver.commands.[^1]

For example, the org_members module, which has an update_roles function, would be replaced by an org_members class, which would have an update_roles method. Because this method would be static, and with a small change to jobserver.commands.__init__, client code would be unaware of the replacement: calling org_members.update_roles (module/function) would be the same as calling org_members.update_roles (class/method).

Unfortunately, this idea wasn't viable. Changing jobserver.commands.__init__ was hacky and left me with more questions than answers about how the import system works (even after reading "The import system" several times). Consequently, client code would be aware of the replacement. Furthermore, testing that a class implements a common set of methods happens when that class is instantiated. In this case, the classes wouldn't be instantiated: their methods would be static. Consequently, the benefits of inheriting from an ABC were lost. Of course, I could have written a test to instantiate the classes, but doing so would have added more cost for less benefit.

Ultimately, this idea would have resulted in changes to all the modules in jobserver.commands, all the modules that import them, and a test that wouldn't have used the modules in jobserver.commands as they were used elsewhere 🙁.

[^1]: It seems that the Python community has decided that adding concrete methods to an ABC is okay, but adding them to another class isn't.

[iaindillingham]

How would we log more event-types? We would...

1. add choices to AuditableEvent.Type (see below)
2. add a module corresponding to the thing being audited to jobserver.auditing.presenters. This module should contain presenters (functions) corresponding to the events being audited
3. add templates corresponding to the choices to staff/auditable_events
4. add choices/presenters (functions) corresponding to the events being audited to jobserver.auditing.presenters.lookup.get_presenter
5. add/update commands (functions) in jobserver.commands
6. make the model being audited immutable by inheriting from ImmutableModelMixin and by using the ImmutableManager custom manager
7. ensure that the (newly) immutable model is only mutated by commands in jobserver.commands

Of the above, 1 and 5 concern the implementation of the audit log; 2, 3, and 4 concern the presentation of the audit log; and 6 and 7 ensure the audit log isn't (inadvertently) bypassed.

Each choice in AuditableEvent.Type has two components. The first component is the thing being audited. This similar to, but not the same as, the model being audited. For example, the thing being audited for the ProjectMembership model is the project member. The second component is the event being audited. There are three events: added, removed, and updated roles. Currently, AuditableEvent.Type has three choices: PROJECT_MEMBER_ADDED, PROJECT_MEMBER_REMOVED, and PROJECT_MEMBER_UPDATED_ROLES.

[iaindillingham]

Having spent a couple of days thinking about this issue, and having got as far as how best to structure the commands in jobserver.commands but not having made much progress, I think it's time to close this issue @lucyb. I feel I have contributed:

• a null result (ABCs aren't viable)
• a description of how we would log more event types

There's a possible third implicit contribution: the conclusion that the audit log is far from scalable. This is my perspective, clearly. However, it seems as though adding a new event-type would require an awful lot of code.