Entity Repo & Controller Refactor (#1690)

* Started mass-refactoring of the current entity repos * Rewrote book tree logic - Now does two simple queries instead of one really complex one. - Extracted logic into its own class. - Remove model-level akward union field listing. - Logic now more readable than being large separate query and compilation functions. * Extracted and split book sort logic * Finished up Book controller/repo organisation * Refactored bookshelves controllers and repo parts * Fixed issues found via phpunit * Refactored Chapter controller * Updated Chapter export controller * Started Page controller/repo refactor * Refactored another chunk of PageController * Completed initial pagecontroller refactor pass * Fixed tests and continued reduction of old repos * Removed old page remove and further reduced entity repo * Removed old entity repo, split out page controller * Ran phpcbf and split out some page content methods * Tidied up some EntityProvider elements * Fixed issued caused by viewservice change
2025-08-07 23:03:00 +03:00 · 2019-10-05 12:55:01 +01:00
parent 7cd956b24b
commit 31f5786e01
72 changed files with 2705 additions and 2751 deletions
--- a/tests/CommandsTest.php
+++ b/tests/CommandsTest.php
@@ -2,7 +2,6 @@

 use BookStack\Auth\Permissions\JointPermission;
 use BookStack\Entities\Page;
-use BookStack\Entities\Repos\EntityRepo;
 use BookStack\Auth\User;
 use BookStack\Entities\Repos\PageRepo;

@@ -56,7 +55,7 @@ class CommandsTest extends TestCase
        $this->asEditor();
        $pageRepo = app(PageRepo::class);
        $page = Page::first();
-        $pageRepo->updatePage($page, $page->book_id, ['name' => 'updated page', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
+        $pageRepo->update($page, ['name' => 'updated page', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
        $pageRepo->updatePageDraft($page, ['name' => 'updated page', 'html' => '<p>new content in draft</p>', 'summary' => 'page revision testing']);

        $this->assertDatabaseHas('page_revisions', [
--- a/tests/Entity/EntityTest.php
+++ b/tests/Entity/EntityTest.php
@@ -3,7 +3,6 @@
 use BookStack\Entities\Book;
 use BookStack\Entities\Chapter;
 use BookStack\Entities\Page;
-use BookStack\Entities\Repos\EntityRepo;
 use BookStack\Auth\UserRepo;
 use BookStack\Entities\Repos\PageRepo;
 use Carbon\Carbon;
@@ -192,7 +191,7 @@ class EntityTest extends BrowserKitTest
        $entities = $this->createEntityChainBelongingToUser($creator, $updater);
        $this->actingAs($creator);
        app(UserRepo::class)->destroy($creator);
-        app(PageRepo::class)->savePageRevision($entities['page']);
+        app(PageRepo::class)->update($entities['page'], ['html' => '<p>hello!</p>>']);

        $this->checkEntitiesViewable($entities);
    }
@@ -205,7 +204,7 @@ class EntityTest extends BrowserKitTest
        $entities = $this->createEntityChainBelongingToUser($creator, $updater);
        $this->actingAs($updater);
        app(UserRepo::class)->destroy($updater);
-        app(PageRepo::class)->savePageRevision($entities['page']);
+        app(PageRepo::class)->update($entities['page'], ['html' => '<p>Hello there!</p>']);

        $this->checkEntitiesViewable($entities);
    }
@@ -273,8 +272,7 @@ class EntityTest extends BrowserKitTest

    public function test_slug_multi_byte_lower_casing()
    {
-        $entityRepo = app(EntityRepo::class);
-        $book = $entityRepo->createFromInput('book', [
+        $book = $this->newBook([
            'name' => 'КНИГА'
        ]);

@@ -284,8 +282,7 @@ class EntityTest extends BrowserKitTest

    public function test_slug_format()
    {
-        $entityRepo = app(EntityRepo::class);
-        $book = $entityRepo->createFromInput('book', [
+        $book = $this->newBook([
            'name' => 'PartA / PartB / PartC'
        ]);

--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -1,8 +1,7 @@
 <?php namespace Tests;

+use BookStack\Entities\Managers\PageContent;
 use BookStack\Entities\Page;
-use BookStack\Entities\Repos\EntityRepo;
-use BookStack\Entities\Repos\PageRepo;

 class PageContentTest extends TestCase
 {
@@ -242,4 +241,66 @@ class PageContentTest extends TestCase
        $updatedPage = Page::where('id', '=', $page->id)->first();
        $this->assertEquals(substr_count($updatedPage->html, "bkmrk-test\""), 1);
    }
+
+    public function test_get_page_nav_sets_correct_properties()
+    {
+        $content = '<h1 id="testa">Hello</h1><h2 id="testb">There</h2><h3 id="testc">Donkey</h3>';
+        $pageContent = new PageContent(new Page(['html' => $content]));
+        $navMap = $pageContent->getNavigation($content);
+
+        $this->assertCount(3, $navMap);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h1',
+            'link' => '#testa',
+            'text' => 'Hello',
+            'level' => 1,
+        ], $navMap[0]);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h2',
+            'link' => '#testb',
+            'text' => 'There',
+            'level' => 2,
+        ], $navMap[1]);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h3',
+            'link' => '#testc',
+            'text' => 'Donkey',
+            'level' => 3,
+        ], $navMap[2]);
+    }
+
+    public function test_get_page_nav_does_not_show_empty_titles()
+    {
+        $content = '<h1 id="testa">Hello</h1><h2 id="testb">&nbsp;</h2><h3 id="testc"></h3>';
+        $pageContent = new PageContent(new Page(['html' => $content]));
+        $navMap = $pageContent->getNavigation($content);
+
+        $this->assertCount(1, $navMap);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h1',
+            'link' => '#testa',
+            'text' => 'Hello'
+        ], $navMap[0]);
+    }
+
+    public function test_get_page_nav_shifts_headers_if_only_smaller_ones_are_used()
+    {
+        $content = '<h4 id="testa">Hello</h4><h5 id="testb">There</h5><h6 id="testc">Donkey</h6>';
+        $pageContent = new PageContent(new Page(['html' => $content]));
+        $navMap = $pageContent->getNavigation($content);
+
+        $this->assertCount(3, $navMap);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h4',
+            'level' => 1,
+        ], $navMap[0]);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h5',
+            'level' => 2,
+        ], $navMap[1]);
+        $this->assertArrayMapIncludes([
+            'nodeName' => 'h6',
+            'level' => 3,
+        ], $navMap[2]);
+    }
 }
--- a/tests/Entity/PageDraftTest.php
+++ b/tests/Entity/PageDraftTest.php
@@ -1,11 +1,14 @@
 <?php namespace Tests;

-
 use BookStack\Entities\Repos\PageRepo;

 class PageDraftTest extends BrowserKitTest
 {
    protected $page;
+
+    /**
+     * @var PageRepo
+     */
    protected $pageRepo;

    public function setUp(): void
@@ -85,11 +88,11 @@ class PageDraftTest extends BrowserKitTest
        $newUser = $this->getEditor();

        $this->actingAs($newUser)->visit('/')
-            ->visit($book->getUrl() . '/create-page')
-            ->visit($chapter->getUrl() . '/create-page')
+            ->visit($book->getUrl('/create-page'))
+            ->visit($chapter->getUrl('/create-page'))
            ->visit($book->getUrl())
            ->seeInElement('.book-contents', 'New Page');
-        
+
        $this->asAdmin()
            ->visit($book->getUrl())
            ->dontSeeInElement('.book-contents', 'New Page')
--- a/tests/Entity/PageRevisionTest.php
+++ b/tests/Entity/PageRevisionTest.php
@@ -12,7 +12,7 @@ class PageRevisionTest extends TestCase

        $pageRepo = app(PageRepo::class);
        $page = Page::first();
-        $pageRepo->updatePage($page, $page->book_id, ['name' => 'updated page', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
+        $pageRepo->update($page, ['name' => 'updated page', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
        $pageRevision = $page->revisions->last();

        $revisionView = $this->get($page->getUrl() . '/revisions/' . $pageRevision->id);
@@ -30,8 +30,8 @@ class PageRevisionTest extends TestCase

        $pageRepo = app(PageRepo::class);
        $page = Page::first();
-        $pageRepo->updatePage($page, $page->book_id, ['name' => 'updated page abc123', 'html' => '<p>new contente def456</p>', 'summary' => 'initial page revision testing']);
-        $pageRepo->updatePage($page, $page->book_id, ['name' => 'updated page again', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
+        $pageRepo->update($page, ['name' => 'updated page abc123', 'html' => '<p>new contente def456</p>', 'summary' => 'initial page revision testing']);
+        $pageRepo->update($page, ['name' => 'updated page again', 'html' => '<p>new content</p>', 'summary' => 'page revision testing']);
        $page =  Page::find($page->id);


@@ -98,7 +98,7 @@ class PageRevisionTest extends TestCase
        $beforeRevisionCount = $page->revisions->count();
        $currentRevision = $page->getCurrentRevision();
        $resp = $this->asEditor()->delete($currentRevision->getUrl('/delete/'));
-        $resp->assertStatus(400);
+        $resp->assertRedirect($page->getUrl('/revisions'));

        $page = Page::find($page->id);
        $afterRevisionCount = $page->revisions->count();
--- a/tests/Entity/SortTest.php
+++ b/tests/Entity/SortTest.php
@@ -1,6 +1,5 @@
 <?php namespace Tests;

-use BookStack\Auth\Role;
 use BookStack\Entities\Book;
 use BookStack\Entities\Chapter;
 use BookStack\Entities\Page;
@@ -20,7 +19,7 @@ class SortTest extends TestCase
    {
        $this->asAdmin();
        $pageRepo = app(PageRepo::class);
-        $draft = $pageRepo->getDraftPage($this->book);
+        $draft = $pageRepo->getNewDraftPage($this->book);

        $resp = $this->get($this->book->getUrl());
        $resp->assertSee($draft->name);
@@ -214,7 +213,6 @@ class SortTest extends TestCase
            'entity_selection' => 'book:' . $newBook->id,
            'name' => 'My copied test page'
        ]);
-
        $pageCopy = Page::where('name', '=', 'My copied test page')->first();

        $movePageResp->assertRedirect($pageCopy->getUrl());
--- a/tests/Permissions/RestrictionsTest.php
+++ b/tests/Permissions/RestrictionsTest.php
@@ -5,14 +5,13 @@ use BookStack\Entities\Bookshelf;
 use BookStack\Entities\Chapter;
 use BookStack\Entities\Entity;
 use BookStack\Auth\User;
-use BookStack\Entities\Repos\EntityRepo;
 use BookStack\Entities\Page;

 class RestrictionsTest extends BrowserKitTest
 {

    /**
-     * @var \BookStack\Auth\User
+     * @var User
     */
    protected $user;

@@ -327,7 +326,7 @@ class RestrictionsTest extends BrowserKitTest

    public function test_page_view_restriction()
    {
-        $page = \BookStack\Entities\Page::first();
+        $page = Page::first();

        $pageUrl = $page->getUrl();
        $this->actingAs($this->user)
@@ -367,7 +366,7 @@ class RestrictionsTest extends BrowserKitTest

    public function test_page_delete_restriction()
    {
-        $page = \BookStack\Entities\Page::first();
+        $page = Page::first();

        $pageUrl = $page->getUrl();
        $this->actingAs($this->user)
@@ -438,7 +437,7 @@ class RestrictionsTest extends BrowserKitTest

    public function test_page_restriction_form()
    {
-        $page = \BookStack\Entities\Page::first();
+        $page = Page::first();
        $this->asAdmin()->visit($page->getUrl() . '/permissions')
            ->see('Page Permissions')
            ->check('restricted')
@@ -665,10 +664,8 @@ class RestrictionsTest extends BrowserKitTest
        $this->setEntityRestrictions($firstBook, ['view', 'update']);
        $this->setEntityRestrictions($secondBook, ['view']);

-        $firstBookChapter = $this->app[EntityRepo::class]->createFromInput('chapter',
-                ['name' => 'first book chapter'], $firstBook);
-        $secondBookChapter = $this->app[EntityRepo::class]->createFromInput('chapter',
-                ['name' => 'second book chapter'], $secondBook);
+        $firstBookChapter = $this->newChapter(['name' => 'first book chapter'], $firstBook);
+        $secondBookChapter = $this->newChapter(['name' => 'second book chapter'], $secondBook);

        // Create request data
        $reqData = [
--- a/tests/SharedTestHelpers.php
+++ b/tests/SharedTestHelpers.php
@@ -1,9 +1,14 @@
 <?php namespace Tests;

+use BookStack\Auth\User;
 use BookStack\Entities\Book;
+use BookStack\Entities\Bookshelf;
+use BookStack\Entities\Chapter;
 use BookStack\Entities\Entity;
 use BookStack\Entities\Page;
-use BookStack\Entities\Repos\EntityRepo;
+use BookStack\Entities\Repos\BookRepo;
+use BookStack\Entities\Repos\BookshelfRepo;
+use BookStack\Entities\Repos\ChapterRepo;
 use BookStack\Auth\Permissions\PermissionsRepo;
 use BookStack\Auth\Role;
 use BookStack\Auth\Permissions\PermissionService;
@@ -11,6 +16,8 @@ use BookStack\Entities\Repos\PageRepo;
 use BookStack\Settings\SettingService;
 use BookStack\Uploads\HttpFetcher;
 use Illuminate\Support\Env;
+use Mockery;
+use Throwable;

 trait SharedTestHelpers
 {
@@ -68,7 +75,7 @@ trait SharedTestHelpers
     */
    protected function getViewer($attributes = [])
    {
-        $user = \BookStack\Auth\Role::getRole('viewer')->users()->first();
+        $user = Role::getRole('viewer')->users()->first();
        if (!empty($attributes)) $user->forceFill($attributes)->save();
        return $user;
    }
@@ -76,7 +83,7 @@ trait SharedTestHelpers
    /**
     * Regenerate the permission for an entity.
     * @param Entity $entity
-     * @throws \Throwable
+     * @throws Throwable
     */
    protected function regenEntityPermissions(Entity $entity)
    {
@@ -87,10 +94,10 @@ trait SharedTestHelpers
    /**
     * Create and return a new bookshelf.
     * @param array $input
-     * @return \BookStack\Entities\Bookshelf
+     * @return Bookshelf
     */
    public function newShelf($input = ['name' => 'test shelf', 'description' => 'My new test shelf']) {
-        return app(EntityRepo::class)->createFromInput('bookshelf', $input);
+        return app(BookshelfRepo::class)->create($input, []);
    }

    /**
@@ -99,30 +106,30 @@ trait SharedTestHelpers
     * @return Book
     */
    public function newBook($input = ['name' => 'test book', 'description' => 'My new test book']) {
-        return app(EntityRepo::class)->createFromInput('book', $input);
+        return app(BookRepo::class)->create($input);
    }

    /**
     * Create and return a new test chapter
     * @param array $input
     * @param Book $book
-     * @return \BookStack\Entities\Chapter
+     * @return Chapter
     */
    public function newChapter($input = ['name' => 'test chapter', 'description' => 'My new test chapter'], Book $book) {
-        return app(EntityRepo::class)->createFromInput('chapter', $input, $book);
+        return app(ChapterRepo::class)->create($input, $book);
    }

    /**
     * Create and return a new test page
     * @param array $input
     * @return Page
-     * @throws \Throwable
+     * @throws Throwable
     */
    public function newPage($input = ['name' => 'test page', 'html' => 'My new test page']) {
        $book = Book::first();
        $pageRepo = app(PageRepo::class);
-        $draftPage = $pageRepo->getDraftPage($book);
-        return $pageRepo->publishPageDraft($draftPage, $input);
+        $draftPage = $pageRepo->getNewDraftPage($book);
+        return $pageRepo->publishDraft($draftPage, $input);
    }

    /**
@@ -167,10 +174,10 @@ trait SharedTestHelpers

    /**
     * Give the given user some permissions.
-     * @param \BookStack\Auth\User $user
+     * @param User $user
     * @param array $permissions
     */
-    protected function giveUserPermissions(\BookStack\Auth\User $user, $permissions = [])
+    protected function giveUserPermissions(User $user, $permissions = [])
    {
        $newRole = $this->createNewRole($permissions);
        $user->attachRole($newRole);
@@ -198,7 +205,7 @@ trait SharedTestHelpers
     */
    protected function mockHttpFetch($returnData, int $times = 1)
    {
-        $mockHttp = \Mockery::mock(HttpFetcher::class);
+        $mockHttp = Mockery::mock(HttpFetcher::class);
        $this->app[HttpFetcher::class] = $mockHttp;
        $mockHttp->shouldReceive('fetch')
            ->times($times)
--- a/tests/Unit/PageRepoTest.php
+++ b/tests/Unit/PageRepoTest.php
@@ -1,78 +0,0 @@
-<?php
-namespace Tests;
-
-use BookStack\Entities\Repos\PageRepo;
-
-class PageRepoTest extends TestCase
-{
-    /**
-     * @var PageRepo $pageRepo
-     */
-    protected $pageRepo;
-
-    protected function setUp(): void
-    {
-        parent::setUp();
-        $this->pageRepo = app()->make(PageRepo::class);
-    }
-
-    public function test_get_page_nav_sets_correct_properties()
-    {
-        $content = '<h1 id="testa">Hello</h1><h2 id="testb">There</h2><h3 id="testc">Donkey</h3>';
-        $navMap = $this->pageRepo->getPageNav($content);
-
-        $this->assertCount(3, $navMap);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h1',
-            'link' => '#testa',
-            'text' => 'Hello',
-            'level' => 1,
-        ], $navMap[0]);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h2',
-            'link' => '#testb',
-            'text' => 'There',
-            'level' => 2,
-        ], $navMap[1]);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h3',
-            'link' => '#testc',
-            'text' => 'Donkey',
-            'level' => 3,
-        ], $navMap[2]);
-    }
-
-    public function test_get_page_nav_does_not_show_empty_titles()
-    {
-        $content = '<h1 id="testa">Hello</h1><h2 id="testb">&nbsp;</h2><h3 id="testc"></h3>';
-        $navMap = $this->pageRepo->getPageNav($content);
-
-        $this->assertCount(1, $navMap);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h1',
-            'link' => '#testa',
-            'text' => 'Hello'
-        ], $navMap[0]);
-    }
-
-    public function test_get_page_nav_shifts_headers_if_only_smaller_ones_are_used()
-    {
-        $content = '<h4 id="testa">Hello</h4><h5 id="testb">There</h5><h6 id="testc">Donkey</h6>';
-        $navMap = $this->pageRepo->getPageNav($content);
-
-        $this->assertCount(3, $navMap);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h4',
-            'level' => 1,
-        ], $navMap[0]);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h5',
-            'level' => 2,
-        ], $navMap[1]);
-        $this->assertArrayMapIncludes([
-            'nodeName' => 'h6',
-            'level' => 3,
-        ], $navMap[2]);
-    }
-
-}
--- a/tests/Uploads/AttachmentTest.php
+++ b/tests/Uploads/AttachmentTest.php
@@ -223,7 +223,7 @@ class AttachmentTest extends TestCase
    {
        $admin = $this->getAdmin();
        $viewer = $this->getViewer();
-        $page = Page::first();
+        $page = Page::first(); /** @var Page $page */

        $this->actingAs($admin);
        $fileName = 'permission_test.txt';
@@ -233,7 +233,7 @@ class AttachmentTest extends TestCase
        $page->restricted = true;
        $page->permissions()->delete();
        $page->save();
-        $this->app[PermissionService::class]->buildJointPermissionsForEntity($page);
+        $page->rebuildPermissions();
        $page->load('jointPermissions');

        $this->actingAs($viewer);
--- a/tests/Uploads/ImageTest.php
+++ b/tests/Uploads/ImageTest.php
@@ -367,7 +367,7 @@ class ImageTest extends TestCase
        $image = Image::where('type', '=', 'gallery')->first();

        $pageRepo = app(PageRepo::class);
-        $pageRepo->updatePage($page, $page->book_id, [
+        $pageRepo->update($page, [
            'name' => $page->name,
            'html' => $page->html . "<img src=\"{$image->url}\">",
            'summary' => ''
@@ -379,7 +379,7 @@ class ImageTest extends TestCase
        $this->assertCount(0, $toDelete);

        // Save a revision of our page without the image;
-        $pageRepo->updatePage($page, $page->book_id, [
+        $pageRepo->update($page, [
            'name' => $page->name,
            'html' => "<p>Hello</p>",
            'summary' => ''