Complete refactor of the 'simpler logging' action_log changes - #17463
Closed
uberbrady wants to merge 18 commits into
Closed
Complete refactor of the 'simpler logging' action_log changes#17463uberbrady wants to merge 18 commits into
uberbrady wants to merge 18 commits into
Conversation
4 tasks
uberbrady
commented
Jul 28, 2025
uberbrady
commented
Jul 28, 2025
uberbrady
commented
Jul 28, 2025
uberbrady
commented
Jul 28, 2025
uberbrady
commented
Jul 28, 2025
uberbrady
commented
Jul 28, 2025
Member
|
Looks like we've got conflicts again, since I deleted some of those controllers in a previous PR. |
snipe
marked this pull request as draft
August 18, 2025 01:46
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
marked this pull request as ready for review
August 28, 2025 17:56
Member
|
Can you resolve the new conflicts and we can go over this today? |
uberbrady
commented
Sep 3, 2025
| $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"); |
uberbrady
commented
Sep 3, 2025
| $license = License::factory()->create(); | ||
| $oldUser = User::factory()->create(); | ||
|
|
||
| Log::error("Right after create"); |
Member
Author
There was a problem hiding this comment.
stray error (and next line) and further below
Member
|
Looks like you might have missed one:
|
Member
|
Closing this for now - you're free to re-open it though. (It's a bit old and there are a ton of conflicts.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_loghappens 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.Loggabletrait got moved into theTraitssub-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_logentries makes it much easier to start to treat "quantities" as first-class fields and not simple repetitions.