Updated activities table format

Renamed some columns to be more generic and applicable. Removed now redundant book_id column. Allowed nullable entity morph columns for non-entity activity. Ran tests and made required changes.
2020-11-08 00:03:19 +00:00 · 2020-11-08 00:03:19 +00:00 · 712ccd23c4
commit 712ccd23c4
parent ee7e1122d3
12 changed files with 106 additions and 50 deletions
--- a/app/Actions/Activity.php
+++ b/app/Actions/Activity.php
@ -5,16 +5,16 @@ namespace BookStack\Actions;
 use BookStack\Auth\User;
 use BookStack\Entities\Entity;
 use BookStack\Model;
+use Illuminate\Database\Eloquent\Relations\BelongsTo;

 /**
- * @property string $key
+ * @property string $type
 * @property User $user
 * @property Entity $entity
- * @property string $extra
+ * @property string $detail
 * @property string $entity_type
 * @property int $entity_id
 * @property int $user_id
- * @property int $book_id
 */
 class Activity extends Model
 {
@ -32,20 +32,18 @@ class Activity extends Model

    /**
     * Get the user this activity relates to.
-     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
-    public function user()
+    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

    /**
-     * Returns text from the language files, Looks up by using the
-     * activity key.
+     * Returns text from the language files, Looks up by using the activity key.
     */
-    public function getText()
+    public function getText(): string
    {
-        return trans('activities.' . $this->key);
+        return trans('activities.' . $this->type);
    }

    /**
@ -53,6 +51,6 @@ class Activity extends Model
     */
    public function isSimilarTo(Activity $activityB): bool
    {
-        return [$this->key, $this->entity_type, $this->entity_id] === [$activityB->key, $activityB->entity_type, $activityB->entity_id];
+        return [$this->type, $this->entity_type, $this->entity_id] === [$activityB->type, $activityB->entity_type, $activityB->entity_id];
    }
 }
--- a/app/Actions/ActivityService.php
+++ b/app/Actions/ActivityService.php
@ -12,14 +12,12 @@ use Illuminate\Support\Facades\Log;
 class ActivityService
 {
    protected $activity;
-    protected $user;
    protected $permissionService;

    public function __construct(Activity $activity, PermissionService $permissionService)
    {
        $this->activity = $activity;
        $this->permissionService = $permissionService;
-        $this->user = user();
    }

    /**
@ -38,8 +36,8 @@ class ActivityService
    protected function newActivityForUser(string $type): Activity
    {
        return $this->activity->newInstance()->forceFill([
-            'key'     => strtolower($type),
-            'user_id' => $this->user->id,
+            'type'     => strtolower($type),
+            'user_id' => user()->id,
        ]);
    }

@ -51,9 +49,9 @@ class ActivityService
    public function removeEntity(Entity $entity)
    {
        $entity->activity()->update([
-            'extra'       => $entity->name,
-            'entity_id'   => 0,
-            'entity_type' => '',
+            'detail'       => $entity->name,
+            'entity_id'   => null,
+            'entity_type' => null,
        ]);
    }

@ -150,9 +148,9 @@ class ActivityService
    /**
     * Flashes a notification message to the session if an appropriate message is available.
     */
-    protected function setNotification(string $activityKey)
+    protected function setNotification(string $type)
    {
-        $notificationTextKey = 'activities.' . $activityKey . '_notification';
+        $notificationTextKey = 'activities.' . $type . '_notification';
        if (trans()->has($notificationTextKey)) {
            $message = trans($notificationTextKey);
            session()->flash('success', $message);
--- a/app/Entities/BookChild.php
+++ b/app/Entities/BookChild.php
@ -45,9 +45,6 @@ class BookChild extends Entity
        $this->save();
        $this->refresh();

-        // Update related activity
-        $this->activity()->update(['book_id' => $newBookId]);
-
        // Update all child pages if a chapter
        if ($this instanceof Chapter) {
            foreach ($this->pages as $page) {
--- a/app/Http/Controllers/AuditLogController.php
+++ b/app/Http/Controllers/AuditLogController.php
@ -32,7 +32,7 @@ class AuditLogController extends Controller
            ->orderBy($listDetails['sort'], $listDetails['order']);

        if ($listDetails['event']) {
-            $query->where('key', '=', $listDetails['event']);
+            $query->where('type', '=', $listDetails['event']);
        }

        if ($listDetails['date_from']) {
@ -45,12 +45,12 @@ class AuditLogController extends Controller
        $activities = $query->paginate(100);
        $activities->appends($listDetails);

-        $keys = DB::table('activities')->select('key')->distinct()->pluck('key');
+        $types = DB::table('activities')->select('type')->distinct()->pluck('type');
        $this->setPageTitle(trans('settings.audit'));
        return view('settings.audit', [
            'activities' => $activities,
            'listDetails' => $listDetails,
-            'activityKeys' => $keys,
+            'activityTypes' => $types,
        ]);
    }
 }
--- a/app/Http/Controllers/BookSortController.php
+++ b/app/Http/Controllers/BookSortController.php
@ -15,10 +15,6 @@ class BookSortController extends Controller

    protected $bookRepo;

-    /**
-     * BookSortController constructor.
-     * @param $bookRepo
-     */
    public function __construct(BookRepo $bookRepo)
    {
        $this->bookRepo = $bookRepo;
--- a/database/migrations/2020_11_07_232321_simplify_activities_table.php
+++ b/database/migrations/2020_11_07_232321_simplify_activities_table.php
@ -0,0 +1,58 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+use Illuminate\Support\Facades\DB;
+
+class SimplifyActivitiesTable extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('activities', function (Blueprint $table) {
+            $table->renameColumn('key', 'type');
+            $table->renameColumn('extra', 'detail');
+            $table->dropColumn('book_id');
+            $table->integer('entity_id')->nullable()->change();
+            $table->string('entity_type', 191)->nullable()->change();
+        });
+
+        DB::table('activities')
+            ->where('entity_id', '=', 0)
+            ->update([
+                'entity_id' => null,
+                'entity_type' => null,
+            ]);
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        DB::table('activities')
+            ->whereNull('entity_id')
+            ->update([
+                'entity_id' => 0,
+                'entity_type' => '',
+            ]);
+
+        Schema::table('activities', function (Blueprint $table) {
+            $table->renameColumn('type', 'key');
+            $table->renameColumn('detail', 'extra');
+            $table->integer('book_id');
+
+            $table->integer('entity_id')->change();
+            $table->string('entity_type', 191)->change();
+
+            $table->index('book_id');
+        });
+    }
+}
--- a/resources/views/settings/audit.blade.php
+++ b/resources/views/settings/audit.blade.php
@ -19,8 +19,8 @@
                <button refs="dropdown@toggle" aria-haspopup="true" aria-expanded="false" aria-label="{{ trans('common.sort_options') }}" class="input-base text-left">{{ $listDetails['event'] ?: trans('settings.audit_event_filter_no_filter') }}</button>
                <ul refs="dropdown@menu" class="dropdown-menu">
                    <li @if($listDetails['event'] === '') class="active" @endif><a href="{{ sortUrl('/settings/audit', $listDetails, ['event' => '']) }}">{{ trans('settings.audit_event_filter_no_filter') }}</a></li>
-                    @foreach($activityKeys as $key)
-                        <li @if($key === $listDetails['event']) class="active" @endif><a href="{{ sortUrl('/settings/audit', $listDetails, ['event' => $key]) }}">{{ $key }}</a></li>
+                    @foreach($activityTypes as $type)
+                        <li @if($type === $listDetails['event']) class="active" @endif><a href="{{ sortUrl('/settings/audit', $listDetails, ['event' => $type]) }}">{{ $type }}</a></li>
                    @endforeach
                </ul>
            </div>
@ -62,7 +62,7 @@
                    <td>
                        @include('partials.table-user', ['user' => $activity->user, 'user_id' => $activity->user_id])
                    </td>
-                    <td>{{ $activity->key }}</td>
+                    <td>{{ $activity->type }}</td>
                    <td>
                        @if($activity->entity)
                            <a href="{{ $activity->entity->getUrl() }}" class="table-entity-item">
@ -71,10 +71,10 @@
                                    {{ $activity->entity->name }}
                                </div>
                            </a>
-                        @elseif($activity->extra)
+                        @elseif($activity->detail)
                            <div class="px-m">
                                {{ trans('settings.audit_deleted_item') }} <br>
-                                {{ trans('settings.audit_deleted_item_name', ['name' => $activity->extra]) }}
+                                {{ trans('settings.audit_deleted_item_name', ['name' => $activity->detail]) }}
                            </div>
                        @endif
                    </td>
--- a/tests/AuditLogTest.php
+++ b/tests/AuditLogTest.php
@ -2,6 +2,7 @@

 use BookStack\Actions\Activity;
 use BookStack\Actions\ActivityService;
+use BookStack\Actions\ActivityType;
 use BookStack\Auth\UserRepo;
 use BookStack\Entities\Managers\TrashCan;
 use BookStack\Entities\Page;
@ -10,6 +11,14 @@ use Carbon\Carbon;

 class AuditLogTest extends TestCase
 {
+    /** @var ActivityService  */
+    protected $activityService;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+        $this->activityService = app(ActivityService::class);
+    }

    public function test_only_accessible_with_right_permissions()
    {
@ -34,7 +43,7 @@ class AuditLogTest extends TestCase
        $admin = $this->getAdmin();
        $this->actingAs($admin);
        $page = Page::query()->first();
-        app(ActivityService::class)->add($page, 'page_create', $page->book->id);
+        $this->activityService->addForEntity($page, ActivityType::PAGE_CREATE);
        $activity = Activity::query()->orderBy('id', 'desc')->first();

        $resp = $this->get('settings/audit');
@ -49,7 +58,7 @@ class AuditLogTest extends TestCase
        $this->actingAs( $this->getAdmin());
        $page = Page::query()->first();
        $pageName = $page->name;
-        app(ActivityService::class)->add($page, 'page_create', $page->book->id);
+        $this->activityService->addForEntity($page, ActivityType::PAGE_CREATE);

        app(PageRepo::class)->destroy($page);
        app(TrashCan::class)->empty();
@ -64,7 +73,7 @@ class AuditLogTest extends TestCase
        $viewer = $this->getViewer();
        $this->actingAs($viewer);
        $page = Page::query()->first();
-        app(ActivityService::class)->add($page, 'page_create', $page->book->id);
+        $this->activityService->addForEntity($page, ActivityType::PAGE_CREATE);

        $this->actingAs($this->getAdmin());
        app(UserRepo::class)->destroy($viewer);
@ -77,7 +86,7 @@ class AuditLogTest extends TestCase
    {
        $this->actingAs($this->getAdmin());
        $page = Page::query()->first();
-        app(ActivityService::class)->add($page, 'page_create', $page->book->id);
+        $this->activityService->addForEntity($page, ActivityType::PAGE_CREATE);

        $resp = $this->get('settings/audit');
        $resp->assertSeeText($page->name);
@ -90,7 +99,7 @@ class AuditLogTest extends TestCase
    {
        $this->actingAs($this->getAdmin());
        $page = Page::query()->first();
-        app(ActivityService::class)->add($page, 'page_create', $page->book->id);
+        $this->activityService->addForEntity($page, ActivityType::PAGE_CREATE);

        $yesterday = (Carbon::now()->subDay()->format('Y-m-d'));
        $tomorrow = (Carbon::now()->addDay()->format('Y-m-d'));
--- a/tests/CommandsTest.php
+++ b/tests/CommandsTest.php
@ -41,7 +41,7 @@ class CommandsTest extends TestCase
        \Activity::addForEntity($page, ActivityType::PAGE_UPDATE);

        $this->assertDatabaseHas('activities', [
-            'key' => 'page_update',
+            'type' => 'page_update',
            'entity_id' => $page->id,
            'user_id' => $this->getEditor()->id
        ]);
@ -51,7 +51,7 @@ class CommandsTest extends TestCase


        $this->assertDatabaseMissing('activities', [
-            'key' => 'page_update'
+            'type' => 'page_update'
        ]);
    }

--- a/tests/Entity/SortTest.php
+++ b/tests/Entity/SortTest.php
@ -79,7 +79,7 @@ class SortTest extends TestCase
        $movePageResp = $this->actingAs($this->getEditor())->put($page->getUrl('/move'), [
            'entity_selection' => 'book:' . $newBook->id
        ]);
-        $page = Page::find($page->id);
+        $page->refresh();

        $movePageResp->assertRedirect($page->getUrl());
        $this->assertTrue($page->book->id == $newBook->id, 'Page parent is now the new book');
--- a/tests/RecycleBinTest.php
+++ b/tests/RecycleBinTest.php
@ -136,7 +136,7 @@ class RecycleBinTest extends TestCase
        $deletion = $page->deletions()->firstOrFail();

        $this->assertDatabaseHas('activities', [
-            'key' => 'page_delete',
+            'type' => 'page_delete',
            'entity_id' => $page->id,
            'entity_type' => $page->getMorphClass(),
        ]);
@ -144,16 +144,16 @@ class RecycleBinTest extends TestCase
        $this->asAdmin()->delete("/settings/recycle-bin/{$deletion->id}");

        $this->assertDatabaseMissing('activities', [
-            'key' => 'page_delete',
+            'type' => 'page_delete',
            'entity_id' => $page->id,
            'entity_type' => $page->getMorphClass(),
        ]);

        $this->assertDatabaseHas('activities', [
-            'key' => 'page_delete',
-            'entity_id' => 0,
-            'entity_type' => '',
-            'extra' => $page->name,
+            'type' => 'page_delete',
+            'entity_id' => null,
+            'entity_type' => null,
+            'detail' => $page->name,
        ]);
    }

--- a/tests/TestCase.php
+++ b/tests/TestCase.php
@ -53,9 +53,9 @@ abstract class TestCase extends BaseTestCase
     * Assert that an activity entry exists of the given key.
     * Checks the activity belongs to the given entity if provided.
     */
-    protected function assertActivityExists(string $key, Entity $entity = null)
+    protected function assertActivityExists(string $type, Entity $entity = null)
    {
-        $detailsToCheck = ['key' => $key];
+        $detailsToCheck = ['type' => $type];

        if ($entity) {
            $detailsToCheck['entity_type'] = $entity->getMorphClass();