2601 allow non user audit actors

[rorymckinley]

Description

This allows audit events to be created with non-user actors. This is in preparation for the auditing of snapshots, where the actor may be a ProjectRepoConnection or a Trigger.

There are a fair number of changes in this PR, but they mostly consist of:

• Adding extra test coverage
• Swapping out a user id for the user struct.

I have tried to group related changes in commits to make reviewing easier.

#2601

Validation steps

As the intent of this change is to have everything remain unchanged, I would suggest the following:

• On main make some retention period changes for a project.
• Take a screenshot of the Audit UI.
• Switch to this branch.
• Make similar changes to the changes on main
• Compare the audit screen to your screenshot.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

• Code generation (copilot but not intellisense)
• Learning or fact checking
• Strategy / design
• Optimisation / refactoring
• Translation / spellchecking / doc gen
• Other
• I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

• I have performed a self-review of my code.
• I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
• I have updated the changelog.
• I have ticked a box in "AI usage" in this PR

[rorymckinley]

The next iteration of this query is likely to be a Union - some experimentation with a union and a select merge that maps to a Struct produced issues. So, for now, I am going with a dumber version that will hopefully be more acceptable once the union is implemented.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.88%. Comparing base (d67c136) to head (023abfb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2638   +/-   ##
=======================================
  Coverage   90.87%   90.88%           
=======================================
  Files         320      320           
  Lines       11257    11252    -5     
=======================================
- Hits        10230    10226    -4     
+ Misses       1027     1026    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jyeshe]

Nice abstraction Sir @rorymckinley ! Nit picking you again with pipes and I would like to propose keeping the existing virtual actor map unless we are sure that the View is very stable and unlikely to change according to multiple actor types available.

[jyeshe]

Model exists to support some view at least at some point in time (internally or externally) so I guess where you're heading to but we might end up having more flexibility if we keep/preserve the actor map and use the same trick you did but assigning to the virtual map:

import Ecto.Query; 
query = 
  from a in Lightning.Auditing.Audit, 
    left_join: u in Lightning.Accounts.User, 
    on: [id: a.actor_id], 
    select_merge: %{actor: u}
Repo.all(query) 

Then whatever change we might have on the View, for any of the three actor types, we wouldn't need to change the model again.

[rorymckinley]

@jyeshe My initial intent was to retain the actor map, but when I started experimenting with unions, I ran into the following error:

** (Ecto.SubQueryError) the following exception happened when compiling a subquery.

         ** (Ecto.QueryError) atoms, structs, maps, lists, tuples and sources are not allowed as map values in subquery, got: `%{actor: &1}` in query:
         
         from a0 in Lightning.Auditing.Audit,
           prefix: "public",
           left_join: u1 in Lightning.Accounts.User,
           on: u1.id == a0.actor_id,
           where: a0.actor_type == "Lightning.Accounts.User",
           union_all: (from a0 in Lightning.Auditing.Audit,
           prefix: "public",
           left_join: p1 in Lightning.VersionControl.ProjectRepoConnection,
           on: p1.id == a0.actor_id,
           where: a0.actor_type == "Lightning.VersionControl.ProjectRepoConnection",
           select: merge(a0, %{actor: p1})),
           select: merge(a0, %{actor: u1})

There are ways to correct this and still retain the actor map but I am not sure that the benefit outweighs the cost currently, as my understanding of the deliverable is that the primary intent is to ensure that the data is captured.

Therefore, my preference is to apply a less elegant and less flexible approach to satisfy the current view requirements so that I can maintain momentum on the capturing of audit events. I am more than willing to revisit this if (a) one of the new events demands it or (b) there is time to do so once the primary objectives have been met.

[jyeshe]

Got it, in this case I would do just a single query with select: %{audit: a, user: u, trigger: t, provision: p}
It would allow keeping a generic model and use a single select on the SQL side.
To set the actor would be as clean as:

Enum.map(audits, &Map.put(&1.audit, :actor, &1.user || &1.trigger || &1.provision))

[jyeshe]

But again if we are sure of what we want on the presentation layer, if it's a stable view, I am fine with the only two virtuals.

[jyeshe]

if there are only 3 types we can enumerate them with a |:
Lightning.Accounts.User.t() | Lightning.Workflows.Trigger.t() | ...

[rorymckinley]

Ah, nice - fixed. Thanks @jyeshe

[jyeshe]

well done getting the module type mr Rory, this is nice trick. Please remember that the |> are for chaining values and not much for simple function calls. Sometimes we do:

socket
|> assign(...)

but there is a purpose of making it easy to add other assigns. If we do piping for all function calls it produces extra work for the compiler on each compilation.

[rorymckinley]

Thanks @jyeshe - cleaned up.

[jyeshe]

Similar to the function call above.

[rorymckinley]

Thanks - cleaned up.

[jyeshe]

like

[jyeshe]

❤️

[rorymckinley]

Thanks for the feedback @jyeshe , I have addressed it in two follow-up commits.