Skip to content

Conversation

uberbrady
Copy link
Member

@uberbrady uberbrady commented Jul 24, 2025

This is a heavily cleaned up take of #16839 - repeating the notes here:

Updated

This is now getting pretty close. The previously-made updates to tests have definitely helped to catch bugs in this implementation, which I'm pretty happy with.

Right now, we should see that there are still a few failing tests. These all boil down to different choices of implementation. Before this PR we weren't quite too rigid about logging things separately when they were created versus checked out. And we weren't too rigid about making sure that the API worked exactly the same as the GUI in these cases. But, should we be?

My opinion - creating an asset and immediately checking it out via GUI or API should both do the same thing - log a 'created' action_log and then immediately a 'checkout' action_log. I think the extra action_log entries make it clearer and easier to 'replay' the history of an object.

To-Do

  • I recently introduced - or, rather, sorta re-introduced, the Loggable method saveWithLogAction() (which I think is a dumb name, I should fix it). I should probably apply that across the code base for consistency.
  • This probably needs some extensive testing. It touches a lot of the 'heart' of Snipe-IT - the action_log. While it does try to ensure that some type of action_log happens any time the object-in-question gets modified, or if the various logging methods are called, it's possible that something could have been missed, or there are still subtle bugs around.
  • We could probably stand to do another squash - as there are a ton of commits. Unfortunately, the vrious 'squash' attempts I've made in the past seem to obscure the fact that the Loggable trait got moved into the Traits sub-directory, which has made later merges much, much harder.
  • Because of how git works, some changes that have happened to some of the files that have been deleted keep making those files pop back up when we try and re-merge develop into this branch. It's not always easy to see what the changes were to merge them back into wherever they belong. I've done my fair share of archeology to try and figure that out, but I could've easily missed something.

Next Steps

  1. I have two PR's waiting on this one, which will need to be re-pointed and updated to match this new 'style' of handling the action_logs:
  1. I think once all that is done and everything is merged, it will be the case that the events that we're firing are really only notification-triggers. If that's true, we might be able to fully "de-event" the notifications, and trigger them all directly with ->notify(). if that makes sense. I feel like the Slack/webhook notifications and the email notifications are sufficiently different that I would like to see if it's possible to split them off - but that might only work with the event'ed notifications, and not with the ->notify() based ones.
  2. I think this sets us up to do the 'quantitizable' changes that we've been pondering - having one grand-unified method of making all action_log entries makes it much easier to start to treat "quantities" as first-class fields and not simple repetitions.
  3. I think this also sets up some of the "Order" and "Purchase Order" additions that we've been considering simpler as well.

@uberbrady uberbrady requested a review from snipe as a code owner July 24, 2025 20:57
@snipe
Copy link
Member

snipe commented Aug 10, 2025

Looks like we've got conflicts again, since I deleted some of those controllers in a previous PR.

@snipe snipe marked this pull request as draft August 18, 2025 01:46
@uberbrady
Copy link
Member Author

Two additional PR's (and an implicit third) are stacked against this one:

Furthermore, once both of those two PR's are merged, we should be able to completely delete the LogListener at that point (as it will be mostly empty) - which would be another PR, but that's easy enough.

@uberbrady uberbrady marked this pull request as ready for review August 28, 2025 17:56
@snipe
Copy link
Member

snipe commented Sep 3, 2025

Can you resolve the new conflicts and we can go over this today?

$logAction->logaction('add seats');
$cleanlicense = $license->fresh(); //we have to do this to avoid repeating the change that already happened
// this is a *brand new change* that just shows the increase in license seats
\Log::error("THIS IS THE MAIN WAY WE ADD SEATS YEAH? CHANGE IS $change");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix me stray error

$license = License::factory()->create();
$oldUser = User::factory()->create();

Log::error("Right after create");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray error (and next line) and further below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants