-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Complete refactor of the 'simpler logging' action_log changes #17463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Complete refactor of the 'simpler logging' action_log changes #17463
Conversation
app/Http/Controllers/Accessories/AccessoriesFilesController.php
Outdated
Show resolved
Hide resolved
Looks like we've got conflicts again, since I deleted some of those controllers in a previous PR. |
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. |
Can you resolve the new conflicts and we can go over this today? |
app/Models/License.php
Outdated
$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"); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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
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
saveWithLogAction()
(which I think is a dumb name, I should fix it). I should probably apply that across the code base for consistency.action_log
. While it does try to ensure that some type ofaction_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.Loggable
trait got moved into theTraits
sub-directory, which has made later merges much, much harder.Next Steps
->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.action_log
entries makes it much easier to start to treat "quantities" as first-class fields and not simple repetitions.