From 712ccd23c4738e6a59a10f31ff654743fbc61879 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 8 Nov 2020 00:03:19 +0000 Subject: [PATCH] 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. --- app/Actions/Activity.php | 18 +++--- app/Actions/ActivityService.php | 16 +++-- app/Entities/BookChild.php | 3 - app/Http/Controllers/AuditLogController.php | 6 +- app/Http/Controllers/BookSortController.php | 4 -- ...11_07_232321_simplify_activities_table.php | 58 +++++++++++++++++++ resources/views/settings/audit.blade.php | 10 ++-- tests/AuditLogTest.php | 19 ++++-- tests/CommandsTest.php | 4 +- tests/Entity/SortTest.php | 2 +- tests/RecycleBinTest.php | 12 ++-- tests/TestCase.php | 4 +- 12 files changed, 106 insertions(+), 50 deletions(-) create mode 100644 database/migrations/2020_11_07_232321_simplify_activities_table.php diff --git a/app/Actions/Activity.php b/app/Actions/Activity.php index 035a9cc75..63eda5917 100644 --- 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]; } } diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php index be9f656c3..fb4a739cd 100644 --- 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); diff --git a/app/Entities/BookChild.php b/app/Entities/BookChild.php index 6eac4375d..042b56e28 100644 --- 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) { diff --git a/app/Http/Controllers/AuditLogController.php b/app/Http/Controllers/AuditLogController.php index fad4e8d38..eb6eecc94 100644 --- 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, ]); } } diff --git a/app/Http/Controllers/BookSortController.php b/app/Http/Controllers/BookSortController.php index fb9308b46..9375b618a 100644 --- 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; diff --git a/database/migrations/2020_11_07_232321_simplify_activities_table.php b/database/migrations/2020_11_07_232321_simplify_activities_table.php new file mode 100644 index 000000000..828dbc656 --- /dev/null +++ b/database/migrations/2020_11_07_232321_simplify_activities_table.php @@ -0,0 +1,58 @@ +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'); + }); + } +} diff --git a/resources/views/settings/audit.blade.php b/resources/views/settings/audit.blade.php index 47a2355d1..7bbf0ed1a 100644 --- a/resources/views/settings/audit.blade.php +++ b/resources/views/settings/audit.blade.php @@ -19,8 +19,8 @@ @@ -62,7 +62,7 @@ @include('partials.table-user', ['user' => $activity->user, 'user_id' => $activity->user_id]) - {{ $activity->key }} + {{ $activity->type }} @if($activity->entity) @@ -71,10 +71,10 @@ {{ $activity->entity->name }} - @elseif($activity->extra) + @elseif($activity->detail)
{{ trans('settings.audit_deleted_item') }}
- {{ trans('settings.audit_deleted_item_name', ['name' => $activity->extra]) }} + {{ trans('settings.audit_deleted_item_name', ['name' => $activity->detail]) }}
@endif diff --git a/tests/AuditLogTest.php b/tests/AuditLogTest.php index 94eb02599..efe842aa1 100644 --- 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')); diff --git a/tests/CommandsTest.php b/tests/CommandsTest.php index 69233e287..ca90bf055 100644 --- 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' ]); } diff --git a/tests/Entity/SortTest.php b/tests/Entity/SortTest.php index 28c3adf31..d510a20ca 100644 --- 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'); diff --git a/tests/RecycleBinTest.php b/tests/RecycleBinTest.php index b6a9dc791..505ee6939 100644 --- 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, ]); } diff --git a/tests/TestCase.php b/tests/TestCase.php index 1f1d5ece7..b5c773037 100644 --- 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();