Refactor logging squashed rebased

[uberbrady]

(This has conflicts, which I will fix, but I wanted to get this up so we can look at it and talk about it).

This is my latest attempt at re-doing the actionlog system.

The most important changes are to the Loggable trait, which has been moved into the Traits directory. (I also moved Requestable there too; but that's not really related).

Loggable now hooks into the various Eloquent lifecycle events and ensures that actionlogs are written whenever an object is created, updated, deleted, or restored. Additionally, we can do other tasks that may or may not change the object in question, to create action logs for Requests, Checkins, checkouts, etc. This is powered by a bunch of Setters usually named "setLogSomething", and the actual log entry is created when we call "logAndSaveIfNeeded()" which takes an ActionType. That method is named like that because sometimes actions like Checkin and Checkout will actually modify the object itself, but sometimes they may not - and we need to make sure the log is inserted regardless. It's possible that a regular save() method-call will still fire the saving and saved events, even if the object is not dirty, which might mean we can simplify some of these method names, which could be nicer. These changes mean that, in general, you should not ever have to create a new ActionLog() and save it directly - the helpers and "logAndSaveIfNeeded()" methods should be enough.

ActionType is now an Enum, so we can avoid accidentally typo'ing an action_type - and in fact there was one such typo that was already in there which @snipe caught and I've fixed.

The individual Observers for the First Class objects are no longer needed since the Loggable Trait now handles many of those cases. In the cases where the Observer was also implementing some parts of the First Class Object's behavior (such as auto_increment on assets, etc), that functionality has been added to the appropriate First Class Object's boot() method. I prefer this locality-of-reference because it means when you're looking at, say, Asset, you can see the functionality that powers it all right there - without having to remember to look at another file somewhere else.

Additionally, we were powering many log entries by emitting an event, which was hard to trace through. Now, setting appropriate values and firing the logAndSaveIfNeeded() method has to be done explicitly, at the cost of a few extra lines of code. I feel like this is worth it and is more clear and explicit.

There were some spots in the code where we needed to disable the event dispatcher in order to avoid multiple log events; I think that's not needed anymore so I've removed those parts, but testing should help show if I'm right or not on this.

There were some parts of the code where we set a non-Eloquent value - checkout_qty and checkout_to - on things like Accessory (which don't have those values), specifically for the purpose of reporting those quantities to event listeners that fire off emails and other notifications. That has an unintended side-effect of making those objects, once modified, un-savable (a call to save() fails with "no such field checkout_qty"). I've replaced those with a ->setLogQuantity() method which sets private values. This will dovetail into better quantity management in a later PR.

In order to keep this change (which is pretty big!) as small as I could, there are still some bits of code powered by events, and still some direct action log manipulation. We should be able to fix those as well

TODO

• confirm or fix any of the remaining FIXME's
• There's a weird call to setLocation in app/Http/Controllers/ViewAssetsController.php - what is that, is it doing what it's supposed to?
• Make sure I've stripped out any Log::error() statements that I had during troubleshooting.
• Ensure that action_date is actually being set, and set correctly, for all log entries. I'm pretty sure I wrote this but I'm not sure.
• This is going to conflict a bit with what @snipe has been doing with some observer code.
• With all the similar, but not identical, code that has been all smooshed together, we need to make sure that any weird special-cases we were handling with log_meta and object-changes are all still working
• I'm not entirely convinced that encrypted Custom Fields are going to be properly redacted in the log_meta fields. I'd hate to add special-case code, but this might be a good place to do something like if ($this instanceof Asset) { ... to make sure that we're not putting secure information unencrypted (or, even encrypted) in the action_logs
• TEST TEST TEST TEST!

[marcusmoore]

Just starting to dive in but I'm not liking the logAndSaveIfNeeded() method but I believe I understand the issue.

That method is named like that because sometimes actions like Checkin and Checkout will actually modify the object itself, but sometimes they may not - and we need to make sure the log is inserted regardless

What if the API changed from (using CancelCheckoutRequestAction as example)

$asset->setLogTarget(auth()->user());
$asset->setLogLocationOverride($user->location_id);
$asset->logAndSaveIfNeeded(ActionType::RequestCanceled);

to something like this:

$asset->withLogging(function () use ($asset, $user) {
    $asset->setLogAction(ActionType::RequestCanceled);
    $asset->setLogTarget(auth()->user());
    $asset->setLogLocationOverride($user->location_id);
})->save();

It feels better to me to bundle that operation together in the callback and continue to use the native ->save() that Eloquent provides.

---

It's possible that a regular save() method-call will still fire the saving and saved events, even if the object is not dirty, which might mean we can simplify some of these method names, which could be nicer

FWIW, the following hits the saving method of the AssetObserver (on develop):

$asset = Asset::factory()->make();
$asset->save();
$asset->save();

---

Gonna dig deeper but wanted to drop an initial thought here first.

[marcusmoore]

A couple more thoughts before really diving into the code...

These changes mean that, in general, you should not ever have to create a new ActionLog() and save it directly

ActionType is now an Enum, so we can avoid accidentally typo'ing an action_type

The individual Observers for the First Class objects are no longer needed since the Loggable Trait now handles many of those cases...that functionality has been added to the appropriate First Class Object's boot() method. I prefer this locality-of-reference because it means when you're looking at, say, Asset, you can see the functionality that powers it all right there - without having to remember to look at another file somewhere else.

All three of these are wins in my book and I'd really like to push towards that goal. The last one I completely agree with.

---

I'm going to hold off on a full review until we talk more about my initial response since that would touch a lot of the files changed here.

[uberbrady]

Just starting to dive in but I'm not liking the logAndSaveIfNeeded() method but I believe I understand the issue.

That method is named like that because sometimes actions like Checkin and Checkout will actually modify the object itself, but sometimes they may not - and we need to make sure the log is inserted regardless

What if the API changed from (using CancelCheckoutRequestAction as example)

$asset->setLogTarget(auth()->user());
$asset->setLogLocationOverride($user->location_id);
$asset->logAndSaveIfNeeded(ActionType::RequestCanceled);

to something like this:

$asset->withLogging(function () use ($asset, $user) {
    $asset->setLogAction(ActionType::RequestCanceled);
    $asset->setLogTarget(auth()->user());
    $asset->setLogLocationOverride($user->location_id);
})->save();

It feels better to me to bundle that operation together in the callback and continue to use the native ->save() that Eloquent provides.

It's possible that a regular save() method-call will still fire the saving and saved events, even if the object is not dirty, which might mean we can simplify some of these method names, which could be nicer

FWIW, the following hits the saving method of the AssetObserver (on develop):

$asset = Asset::factory()->make();
$asset->save();
$asset->save();

Gonna dig deeper but wanted to drop an initial thought here first.

Thank you for your comments!

I feel similarly about logAndSaveIfNeeded() - it's a mouthful and it's weird. If you're right - and based on your test, I think you very much are - I think we can just make it be save() - even easier.

So you get:

$asset->setLogTarget(auth()->user());
$asset->setLogLocationOverride($user->location_id);
$asset->setLogAction(ActionType::RequestCanceled);
$asset->save();

I did like the way the save + Action stuff happened all in one line, but I am not married to that. This looks more 'standard' and should work. We might want to mess around a little bit with the implementation so a "blank save" doesn't create a 'pointless log entry'. The logic there could be something like - if there's no custom ActionType (in other words, it's about to be a simple ActionType::Updated), and there's no changed data in log_meta - then don't log the save. In general, I don't think we ever do pointless updates, so I don't imagine this is going to come up very often, but it might be nice to make sure we've handled that.

If you're okay with this new approach, I can throw in a chunk of fixes for this stuff - basically, finding every call to logAndSaveIfNeeded() and replacing that with setLogAction(...); save();. Once that's done, I suspect I can probably refactor some more of the code to simplify it further, too. Which will be nice :)

[uberbrady]

Just FYI - I also have two more 'stacked' PR's against this PR. They're here:

uberbrady#7
and here:
uberbrady#8

It's weird that they have to be on my version of the Snipe-IT repo, but, well, that's GitHub for you, I guess.

[marcusmoore]

I feel similarly about logAndSaveIfNeeded() - it's a mouthful and it's weird. If you're right - and based on your test, I think you very much are - I think we can just make it be save() - even easier.

So you get:

$asset->setLogTarget(auth()->user());
$asset->setLogLocationOverride($user->location_id);
$asset->setLogAction(ActionType::RequestCanceled);
$asset->save();

I did like the way the save + Action stuff happened all in one line, but I am not married to that. This looks more 'standard' and should work. We might want to mess around a little bit with the implementation so a "blank save" doesn't create a 'pointless log entry'. The logic there could be something like - if there's no custom ActionType (in other words, it's about to be a simple ActionType::Updated), and there's no changed data in log_meta - then don't log the save. In general, I don't think we ever do pointless updates, so I don't imagine this is going to come up very often, but it might be nice to make sure we've handled that.

If you're okay with this new approach, I can throw in a chunk of fixes for this stuff - basically, finding every call to logAndSaveIfNeeded() and replacing that with setLogAction(...); save();. Once that's done, I suspect I can probably refactor some more of the code to simplify it further, too. Which will be nice :)

Am I right in thinking that these logAndSaveIfNeeded()/ setLogX() calls would only need to happen in places where we're not doing a "normal" CRUD action and most of the time we would call save() without having to do any of that because the base Loggable would handle those cases? The following is written with that understanding:

I prefer the codeblock you wrote above instead of the original logAndSaveIfNeeded() but I also like the idea of wrapping the logging context in the callback:

$asset->withLogging(function () use ($asset, $user) {
    $asset->setLogAction(ActionType::RequestCanceled);
    $asset->setLogTarget(auth()->user());
    $asset->setLogLocationOverride($user->location_id);
})->save();

In my mind you would only need to call withLogging() if you are doing something outside of what Loggable does by default. It's more characters to type but I feel like it visually expresses the intention and could pair with a withoutLogging() to handle this situation:

We might want to mess around a little bit with the implementation so a "blank save" doesn't create a 'pointless log entry'. The logic there could be something like - if there's no custom ActionType (in other words, it's about to be a simple ActionType::Updated), and there's no changed data in log_meta - then don't log the save. In general, I don't think we ever do pointless updates, so I don't imagine this is going to come up very often, but it might be nice to make sure we've handled that.

$asset->withoutLogging()->save();

I imagine withoutLogging() would do something like setting $this->skipLogging in Loggable so that the static::creating|updating|etc would adhere to it.

I'm not super-invested in this idea but just wanted to express my viewpoint.

[uberbrady]

In my mind you would only need to call withLogging() if you are doing something outside of what Loggable does by default. It's more characters to type but I feel like it visually expresses the intention and could pair with a withoutLogging() to handle this situation:

$asset->withoutLogging()->save();

I imagine withoutLogging() would do something like setting $this->skipLogging in Loggable so that the static::creating|updating|etc would adhere to it.

I'm not super-invested in this idea but just wanted to express my viewpoint.

I'd agree that I do like the aesthetic look of the anonymous function/callback format, but I don't think it actually gains us anything. And honestly, the withoutLogging() option hasn't come up (yet) - the few places where we were disabling Eloquent events in order to avoid double-logging aren't even needed anymore. If for some reason we needed to make a change to an object and not log it - (maybe in an Artisan fixup command?) - we could probably just do saveQuietly().

The way it works now is that, if you create a new thing, the log entry gets added as ActionType::Created - if you edit a thing, ActionType::Updated, and similarly for soft-deleted and restored. But if you set an alternative setLogAction(), then that type will be used instead.

I'm proposing that any save() - even one that doesn't fundamentally change the object in question - should still fire Logs if there's enough information to do that. I think the bare minimum there would be at least a LogAction. But I want to add an option (and a corresponding test) for a 'blank save' - one where an object has no overridden logAction, and no changes are going to happen (e.g. the object is not ->isDirty()) - and make sure we don't add a log entry for that.

As a side note, I did notice that in the ActionType I'm using different parts of speech - "created" versus "restore", "note added" vs. "audit" - I should probably clean that up too.

[uberbrady]

I cleaned this up the way I mentioned in my last comment over in #16839 - I decided against fixing the "parts-of-speech" problem in ActionType since the IDE will fix that for you pretty easily and it auto-completes pretty well.

[marcusmoore]

I'd agree that I do like the aesthetic look of the anonymous function/callback format, but I don't think it actually gains us anything. And honestly, the withoutLogging() option hasn't come up (yet) - the few places where we were disabling Eloquent events in order to avoid double-logging aren't even needed anymore. If for some reason we needed to make a change to an object and not log it - (maybe in an Artisan fixup command?) - we could probably just do saveQuietly().

I see where you're coming from and since we don't need the withoutLogging() method I think skipping the closure is the approach to take.

Going to move comments to the new PR.