Refactor Loggable system #16707

uberbrady · 2025-04-15T12:34:57Z

This does....a LOT.

For "Action Type" we've been using strings and while we haven't run into problems with that before, I thought something a little safer would be to use PHP Enums instead. This means you can't accidentally create an action log with type 'updatedblork' accidentally.

I moved the Loggable trait to the Traits directory.

The loggable trait now has several private properties, and setters and getters for those properties. Those properties determine how the actual log entry will be generated. This reads a little clearer than some of the longer function signatures we have used.

Much of how the Loggable trait operates now is via various built-in events, such as 'restored' or 'saved' or 'created' - automatically creating the appropriate log entries when those events occur. That's explicitly laid out directly within the Loggable trait, which is the key file that powers much of what we have here.

I have removed several Events that we'd been using to implement functionality. In my opinions, events should be the results of actions, they shouldn't cause actions.

Instead of using the LogListener class to handle doing log things, that's now injected directly into the Trait, which makes it easier to see what exactly is happening when logging occurs.

In the Asset class, we now associate asset-specific listeners via the Boot method rather than in a separate listener file. This makes it clearer to see which effects are firing in the context of Assets.

Similarly with Consumables; the boot method handles what we had been doing in the separate listener.

And so you'll see that several observers have been removed, and their effects are directly integrated into the boot methods of the things that were being observed. Again, the thinking here is that things that affect class Foo should be in the file that has class Foo.

The biggest problem I've run into with this is that it's just so damned BIG - @snipe and I have been discussing ways to implement pieces of this in smaller, less dangerous ways in order to get it live faster. Instead of biting off more than I can chew, are there ways of shrinking this down and still making things cleaner, without boiling the ocean? We shall see.

First pass at huge Actionlog refactor Yank observers from AppServiceProvider Only 3 tests left to go! All but one test passing All tests now pass. Cleanups, renames, clearing out unneeded changes, unused log messages Cleanup, removing unnecessary log types on restores Added some comments and switched main Loggable interface method WIP - moving to new logging system and changing checkin/checkout match Start to move asset checkout methods to 'new-style' Small fixes; new helpers, cleanups Got Checkout tests working for UI and API Begin work on Accessory CheckIn refactor More progress on Accessories checkin/checkout Got Licenses handled now on checkin/checkout, API and GUI I think that's all of 'Licenses' now That looks like Components handled now. Have all of the main classes - minus quantities on Consumables - working Getting more and more tests passing Some work on fixing tests, but I'm getting stuck Reworked NoteAdded to fit in new scheme get rid of logUpload method More cleanups of ActionLogs usage

what-the-diff · 2025-04-15T12:36:20Z

PR Summary

Introduced ActionType Enum: A structure was added to help define various actions, like 'Requested', 'RequestCanceled', and 'Merged', for better categorization and identification.
Refactored Logging Logic: The mechanism for recording important actions was restructured and simplified across multiple areas of the application, to enhance clarity and improve maintainability.
Removed Redundant Event Classes: Certain classes, like CheckoutAccepted, CheckoutDeclined, and UserMerged, that were not necessary, are deleted, with their functionalities integrated elsewhere.
Enhanced Checkout and Check-in Logic: Improved the ways in which we handle logging and state changes during activities like check-in/check-out, ensuring they're more effective.
Simplified Logging in Various Actions: Logging for cancellation and creation requests was simplified, making the code easier to read and maintain.
Removed Duplicate Logging Code: Improvised uniformity by replacing several areas of manual logging with new logging techniques.
Clarified comments regarding potential future refactoring: Included suggestions in the code to hint at areas for future improvements or clarifications.
Controller-level Updates: Enhancements and simplifications in the logic and logging mechanism across various Controllers (AssetsController, ComponentsController, ConsumablesController etc.) for better data recording and code readability.
File Handling Improvements: Refined file upload handling in various file controllers including AssetModelsFilesController and ComponentFilesController, enhancing consistency in logging.
Deletion and Refactoring: Removed unnecessary classes (LogListener.php) and refactored several classes (Accessory, Asset, Component, LicenseSeat, etc.) to improve checkout/check-in operations and logging.
Model-level Changes: Added several functionalities such as asset tag incrementing logic, checkout/check-in methods, and logging functions to various models (Asset, Component, Consumable, etc.)
Removed Observers: Eliminated the need for certain Observer files and removed their references from AppServiceProvider and EventServiceProvider.
Updated Test Suites: Adjusted/improved test cases across multiple suites for enhanced testing accuracy.
Removed Unnecessary Code Files: Deleted several files like Loggable.php and some Observer files which were no longer required.

These changes aimed at enhancing the overall maintainability and readability of the code, while also ensuring that the actions were logged and recorded more effectively.

snipe · 2025-04-16T18:08:29Z

app/Actions/CheckoutRequests/CancelCheckoutRequestAction.php

-        $logaction->target_type = User::class;
-        $logaction->location_id = $user->location_id ?? null;
-        $logaction->logaction('request canceled');
+        $asset->setLogTarget(Auth::user());


Can we please use auth()->user here instead?

snipe · 2025-04-16T18:32:27Z

app/Http/Controllers/Api/AssetsController.php

        if (($request->filled('checkin_at')) && ($request->get('checkin_at') != date('Y-m-d'))) {
-            $originalValues['action_date'] = $checkin_at;
+            \Log::error("Setting checkin time of ".$request->get('checkin_at')." because that's not today which is: ".date('Y-m-d'));
+            $asset->setLogDate(new Carbon($request->get('checkin_at')));


If this only affects action_date, can we rename it to be more specific, like setLogActionDate?

snipe · 2025-04-16T18:42:15Z

app/Http/Controllers/Assets/AssetsController.php

+        $asset->setLogNote($request->input('note'));
+        // Create the image (if one was chosen.)
+        if ($request->hasFile('image')) {
+            $asset->setLogFilename($request->handleFile('private_uploads/audits/', 'audit-'.$asset->id, $request->file('image')));


I think we need to use handleFile() here

snipe · 2025-04-16T18:44:26Z

app/Http/Controllers/CheckInOutRequest.php


 trait CheckInOutRequest
 {
    /**


This file should probably be in the Traits directory?

snipe · 2025-04-16T18:48:26Z

app/Http/Controllers/Components/ComponentCheckinController.php

-
-            // If the checked-in qty is exactly the same as the assigned_qty,
-            // we can simply delete the associated components_assets record
-            if ($qty_remaining_in_checkout == 0) {


Where are we checking and and deleting the components_assets record now?

snipe · 2025-04-16T18:49:55Z

app/Http/Controllers/Components/ComponentCheckoutController.php

+        $component->setLogQuantity($request->input('assigned_qty', 1));
+        $component->setLogTarget($asset);
+        $component->setLogNote($request->input('note'));
+        $component->checkInAndSave();


I think this should be checkOutAndSave()

snipe · 2025-04-16T18:52:42Z

app/Http/Controllers/Licenses/LicenseCheckinController.php

-        }
-
        if($licenseSeat->assigned_to != null){
-            $return_to = User::find($licenseSeat->assigned_to);


I think $return_to here ties into the redirector that @Godmartinz wrote. Not sure we can pull it from here, but we'll need to test further.

snipe · 2025-04-16T18:54:43Z

app/Http/Controllers/Licenses/LicenseCheckinController.php

-        $licenseSeat->assigned_to = null;
-        $licenseSeat->asset_id = null;
-        $licenseSeat->notes = $request->input('notes');
+        $licenseSeat->notes = $request->input('notes'); //huh. Weird.


I think it's not so weird - I think the $licenseSeat->notes is the pivot table notes, versus the action_logs notes

snipe · 2025-04-16T18:57:01Z

app/Http/Controllers/Licenses/LicenseCheckoutController.php

+            $request->request->add(['assigned_user' => $target->id]);
            session()->put(['redirect_option' => $request->get('redirect_option'), 'checkout_to_type' => 'user']);
+        } else {
+            throw new \Exception("No valid checkout target for license");


This should maybe be handled in validation? Form request, or something else.

snipe · 2025-04-16T18:57:55Z

app/Http/Controllers/Licenses/LicenseCheckoutController.php

-
-
-        return redirect()->route('licenses.index')->with('error', trans('Something went wrong handling this checkout.'));
+        return redirect()->route('licenses.index')->with('error', trans('Something went wrong handling this checkout.')); //TODO - translate?


Yes, definitely should translate - this isn't a valid key. :(

snipe · 2025-04-16T18:59:56Z

app/Http/Controllers/Licenses/LicenseCheckoutController.php


        if (! $licenseSeat->license->is($license)) {
-            throw new \Illuminate\Http\Exceptions\HttpResponseException(redirect()->route('licenses.index')->with('error', trans('admin/licenses/message.checkout.mismatch')));
+            throw new HttpResponseException(redirect()->route('licenses.index')->with('error', trans('admin/licenses/message.checkout.mismatch')));


I think just handling this via regular redirect (and add withInput() if it makes sense should be fine

snipe · 2025-04-16T19:03:32Z

app/Http/Controllers/LocationsController.php

                return redirect()->back()->with('error', trans('general.not_deleted', ['item_type' => trans('general.location')]));
            }

            if ($location->restore()) {


The one thing that worries me here i that some FCOs use a special logAndSaveIfBlah() method, and some have the magic overrides in the save(), saving(), restoring() methods, and I'm just concerned that it could be confusing to figure out where the magic is.

snipe · 2025-04-16T19:05:56Z

app/Http/Controllers/Users/UserFilesController.php

+            $user->setLogNote($request->input('notes'));
+
+            if (!$user->logAndSaveIfNeeded(ActionType::Uploaded)) {
                return JsonResponse::create(['error' => 'Failed validation: '.print_r($logAction->getErrors(), true)], 500);


We should use the UploadedFilesTransformer for this, or at least our standard API JSON response formatter.

snipe · 2025-04-16T19:07:45Z

app/Importer/AssetImporter.php

+        //dump($item);
        if ($editingAsset) {
-            $asset->update($item);
+            $asset->update($item); // FIXME - I don't know why this is happening like that?


Not sure what this comment means. What is happening like what?

snipe · 2025-04-16T19:16:53Z

app/Importer/ItemImporter.php

        if (get_class($this) !== UserImporter::class) {
            // $this->item["user"] = $this->createOrFetchUser($row);
-            $this->item['checkout_target'] = $this->determineCheckout($row);
+            $this->item['checkout_target'] = $this->determineCheckout($row); //BERZINGOES?!


I don't know what this comment means

snipe · 2025-04-16T19:19:59Z

app/Models/Consumable.php

+
+            foreach ($uploads as $file) {
+                try {
+                    Storage::delete('private_uploads/consumables/'.$file->filename);


If we soft-delete and can possibly restore, we should do this in the purge instead. Otherwise a restored consumable's files will be broken.

snipe · 2025-04-16T19:22:53Z

app/Models/Traits/Loggable.php

+    }
+
+    // setter functions for private variables
+    public function setLogAction(ActionType $message)


Should we use the more modern accessor/mutator laravel syntax here?

snipe · 2025-04-16T19:23:40Z

app/Models/User.php

    protected $table = 'users';
    protected $injectUniqueIdentifier = true;

+    public static array $hide_changes = ['password', 'remember_token', 'two_factor_secret', 'reset_password_code', 'persist_code'];


snipe · 2025-04-16T19:25:22Z

tests/Feature/Checkins/Ui/AssetCheckinTest.php


        Event::assertDispatched(function (CheckoutableCheckedIn $event) {
-            return $event->action_date === '2023-01-02' && $event->note === 'hello';
+            return $event->action_date->isSameDay(new Carbon('2023-01-02')) && $event->note === 'hello';


Let's confirm this conforms with the slightly changed newer Carbon syntax.

snipe · 2025-04-16T19:27:23Z

tests/Feature/Importing/Api/ImportAssetsTest.php

+        at the factories. So the asset's location should be changed from its rtd_location_id to the User's location_id - which is definitively null. So I think this test
+        might be wrong? But, at the same time, if that's something we've been doing for years, we might have to keep doing that for years. Which is weird. Very, very weird.
+        Not sure what to do on this one.
+        */


I think this might have already handled that? #16682

uberbrady · 2025-04-21T19:09:01Z

A lot of what I did here is done, in a smaller way, in #16779 - which does a lot of the groundwork used here, without going quite as deep as this change goes. I suspect we can and will probably go in this specific direction, but not until we've worked out any/all bugs with the aforementioned PR - and then, these changes will be 'smaller' and more reviewable, relative to that.

uberbrady added 4 commits April 2, 2025 15:10

Got everything passing tests except when the tests are weird

0651989

Switching everything over to the new way, fixing tests to match

b403bd7

WIP getting the last few tests to pass

528f8ac

probot-autolabeler bot added api backend cli / artisan consumables labels Apr 15, 2025

snipe reviewed Apr 16, 2025

View reviewed changes

app/Http/Controllers/CheckInOutRequest.php

trait CheckInOutRequest

{

/**

Copy link

Member

snipe Apr 16, 2025

Choose a reason for hiding this comment

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

This file should probably be in the Traits directory?

snipe reviewed Apr 16, 2025

View reviewed changes

snipe mentioned this pull request Apr 18, 2025

WIP - Use anonymous blade component for history #16769

Closed

uberbrady closed this Apr 21, 2025

snipe mentioned this pull request May 26, 2025

[Feature request] Key information missing from reports #16647

Closed



		return redirect()->route('licenses.index')->with('error', trans('Something went wrong handling this checkout.'));
		return redirect()->route('licenses.index')->with('error', trans('Something went wrong handling this checkout.')); //TODO - translate?

Uh oh!

Refactor Loggable system #16707

Refactor Loggable system #16707

Uh oh!

Conversation

uberbrady commented Apr 15, 2025

Uh oh!

what-the-diff bot commented Apr 15, 2025

PR Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uberbrady commented Apr 21, 2025

Uh oh!

Uh oh!