Merge pull request #15264 from snipe/fixes/fixed_admin_ordering_on_report

Fixed #15252 - admin ordering on activity report

app/Http/Controllers/Api/ReportsController.php

@@ -83,11 +83,19 @@ class ReportsController extends Controller
         $offset = ($request->input('offset') > $total) ? $total : app('api_offset_value');
         $limit = app('api_limit_value');
 
-        $sort = in_array($request->input('sort'), $allowed_columns) ? e($request->input('sort')) : 'created_at';
         $order = ($request->input('order') == 'asc') ? 'asc' : 'desc';
 
+        switch ($request->input('sort')) {
+            case 'admin':
+                $actionlogs->OrderAdmin($order);
+                break;
+            default:
+                $sort = in_array($request->input('sort'), $allowed_columns) ? e($request->input('sort')) : 'created_at';
+                $actionlogs = $actionlogs->orderBy($sort, $order);
+                break;
+        }
 
-        $actionlogs = $actionlogs->orderBy($sort, $order)->skip($offset)->take($limit)->get();
+        $actionlogs = $actionlogs->skip($offset)->take($limit)->get();
 
         return response()->json((new ActionlogsTransformer)->transformActionlogs($actionlogs, $total), 200, ['Content-Type' => 'application/json;charset=utf8'], JSON_UNESCAPED_UNICODE);
     }

app/Models/Actionlog.php

@@ -7,7 +7,6 @@ use App\Presenters\Presentable;
 use Carbon\Carbon;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\SoftDeletes;
-use Illuminate\Support\Facades\Auth;
 
 /**
  * Model for the Actionlog (the table that keeps a historical log of
@@ -17,10 +16,12 @@ use Illuminate\Support\Facades\Auth;
  */
 class Actionlog extends SnipeModel
 {
+    use CompanyableTrait;
     use HasFactory;
 
     // This is to manually set the source (via setActionSource()) for determineActionSource()
     protected ?string $source = null;
+    protected $with = ['admin'];
 
     protected $presenter = \App\Presenters\ActionlogPresenter::class;
     use SoftDeletes;
@@ -66,7 +67,7 @@ class Actionlog extends SnipeModel
         'company' => ['name'],
         'admin' => ['first_name','last_name','username', 'email'],
         'user'  => ['first_name','last_name','username', 'email'],
-        'assets'  => ['asset_tag','name'],
+        'assets'  => ['asset_tag','name','model', 'model_number'],
     ];
 
     /**
@@ -372,4 +373,9 @@ class Actionlog extends SnipeModel
     {
         $this->source = $source;
     }
+
+    public function scopeOrderAdmin($query, $order)
+    {
+        return $query->leftJoin('users as admin_sort', 'action_logs.user_id', '=', 'admin_sort.id')->select('action_logs.*')->orderBy('admin_sort.first_name', $order)->orderBy('admin_sort.last_name', $order);
+    }
 }

app/Models/Asset.php

@@ -2,7 +2,6 @@
 
 namespace App\Models;
 
-use App\Events\AssetCheckedOut;
 use App\Events\CheckoutableCheckedOut;
 use App\Exceptions\CheckoutNotAllowed;
 use App\Helpers\Helper;
@@ -31,6 +30,7 @@ class Asset extends Depreciable
 {
 
     protected $presenter = AssetPresenter::class;
+    protected $with = ['model', 'admin'];
 
     use CompanyableTrait;
     use HasFactory, Loggable, Requestable, Presentable, SoftDeletes, ValidatingTrait, UniqueUndeletedTrait;
@@ -716,7 +716,7 @@ class Asset extends Depreciable
      * @since [v1.0]
      * @return \Illuminate\Database\Eloquent\Relations\Relation
      */
-    public function adminuser()
+    public function admin()
     {
         return $this->belongsTo(\App\Models\User::class, 'user_id');
     }

resources/views/reports/activity.blade.php

@@ -52,7 +52,7 @@
                             <th class="col-sm-3" data-field="item.serial" data-visible="false">{{ trans('admin/hardware/table.serial') }}</th>
                             <th class="col-sm-3" data-field="item" data-formatter="polymorphicItemFormatter">{{ trans('general.item') }}</th>
                             <th class="col-sm-2" data-field="target" data-formatter="polymorphicItemFormatter">{{ trans('general.to') }}</th>
-                            <th class="col-sm-1" data-field="note">{{ trans('general.notes') }}</th>
+                            <th class="col-sm-1" data-field="note" data-sortable="true">{{ trans('general.notes') }}</th>
                             <th class="col-sm-2" data-field="log_meta" data-visible="false" data-formatter="changeLogFormatter">{{ trans('general.changed') }}</th>
                             <th data-field="remote_ip" data-visible="false" data-sortable="true">{{ trans('admin/settings/general.login_ip') }}</th>
                             <th data-field="user_agent" data-visible="false" data-sortable="true">{{ trans('admin/settings/general.login_user_agent') }}</th>

tests/Feature/Assets/Api/StoreAssetTest.php

@@ -61,7 +61,7 @@ class StoreAssetTest extends TestCase
 
         $asset = Asset::find($response['payload']['id']);
 
-        $this->assertTrue($asset->adminuser->is($user));
+        $this->assertTrue($asset->admin->is($user));
 
         $this->assertEquals('2024-06-02', $asset->asset_eol_date);
         $this->assertEquals('random_string', $asset->asset_tag);
@@ -456,7 +456,7 @@ class StoreAssetTest extends TestCase
 
         $asset = Asset::find($response['payload']['id']);
 
-        $this->assertTrue($asset->adminuser->is($user));
+        $this->assertTrue($asset->admin->is($user));
         $this->assertTrue($asset->checkedOutToUser());
         $this->assertTrue($asset->assignedTo->is($userAssigned));
     }
@@ -482,7 +482,7 @@ class StoreAssetTest extends TestCase
 
         $asset = Asset::find($response['payload']['id']);
 
-        $this->assertTrue($asset->adminuser->is($user));
+        $this->assertTrue($asset->admin->is($user));
         $this->assertTrue($asset->checkedOutToLocation());
         $this->assertTrue($asset->location->is($location));
     }
@@ -508,7 +508,7 @@ class StoreAssetTest extends TestCase
 
         $apiAsset = Asset::find($response['payload']['id']);
 
-        $this->assertTrue($apiAsset->adminuser->is($user));
+        $this->assertTrue($apiAsset->admin->is($user));
         $this->assertTrue($apiAsset->checkedOutToAsset());
         // I think this makes sense, but open to a sanity check
         $this->assertTrue($asset->assignedAssets()->find($response['payload']['id'])->is($apiAsset));

tests/Feature/Assets/Ui/BulkEditAssetsTest.php

@@ -165,7 +165,7 @@ class BulkEditAssetsTest extends TestCase
         });
     }
 
-    public function testBulkEditAssetsRequiresAdminUserToUpdateEncryptedCustomFields()
+    public function testBulkEditAssetsRequiresadminToUpdateEncryptedCustomFields()
     {
         $this->markIncompleteIfMySQL('Custom Fields tests do not work on mysql');
         $edit_user = User::factory()->editAssets()->create();

tests/Feature/Reporting/ActivityReportTest.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Tests\Feature\Reporting;
+
+use App\Models\Asset;
+use App\Models\Company;
+use App\Models\User;
+use App\Models\Actionlog;
+use Database\Factories\ActionlogFactory;
+use Illuminate\Testing\Fluent\AssertableJson;
+use Illuminate\Testing\TestResponse;
+use League\Csv\Reader;
+use PHPUnit\Framework\Assert;
+use Tests\TestCase;
+
+class ActivityReportTest extends TestCase
+{
+    public function testRequiresPermissionToViewActivity()
+    {
+        $this->actingAsForApi(User::factory()->create())
+            ->getJson(route('api.activity.index'))
+            ->assertForbidden();
+    }
+
+    public function testRecordsAreScopedToCompanyWhenMultipleCompanySupportEnabled()
+    {
+        $this->markTestIncomplete('This test returns strange results. Need to figure out why.');
+        $this->settings->enableMultipleFullCompanySupport();
+
+        $companyA = Company::factory()->create();
+        $companyB = Company::factory()->create();
+
+        $superUser = User::factory()->superuser()->make();
+
+        $userInCompanyA = User::factory()
+            ->viewUsers()
+            ->viewAssets()
+            ->canViewReports()
+            ->create(['company_id' => $companyA->id]);
+
+        $userInCompanyB = User::factory()
+            ->viewUsers()
+            ->viewAssets()
+            ->canViewReports()
+            ->create(['company_id' => $companyB->id]);
+
+        Asset::factory()->count(5)->create(['company_id' => $companyA->id]);
+        Asset::factory()->count(4)->create(['company_id' => $companyB->id]);
+        Asset::factory()->count(3)->create();
+
+        Actionlog::factory()->userUpdated()->count(5)->create(['company_id' => $companyA->id]);
+        Actionlog::factory()->userUpdated()->count(4)->create(['company_id' => $companyB->id]);
+        Actionlog::factory()->userUpdated()->count(3)->create(['company_id' => $companyB->id]);
+
+        // I don't love this, since it doesn't test that we're actually storing the company ID appropriately
+        // but it's better than what we had
+        $response = $this->actingAsForApi($userInCompanyA)
+            ->getJson(route('api.activity.index'))
+            ->assertOk()
+            ->assertJsonStructure([
+                'rows',
+            ])
+            ->assertJson(fn(AssertableJson $json) => $json->has('rows', 5)->etc());
+
+
+        $this->actingAsForApi($userInCompanyB)
+            ->getJson(
+                route('api.activity.index'))
+            ->assertOk()
+            ->assertJsonStructure([
+                'rows',
+            ])
+            ->assertJson(fn(AssertableJson $json) => $json->has('rows', 7)->etc());
+
+
+
+
+
+    }
+
+}