Reviewed recycle bin API PR and made changes

Made the following changes, many of these are just to align with
existing conventions.

- Updated urls to be hypenated, instead of underscored, to match other system endpoints.
- Updated URL parameter to be `deletionId` instead of `id`, and removed the ID-based comment on controller methods, so the required ID model is clear from the URL alone, since its not clear from the URL endpoint alone like existing endpoints. This follows the pattern used in the "web" routes.
- Added extra detail on some controller method comments, and copied permission comment to each method.
- Removed existing field visibility mechanisms to use simpler model-based visibility since we didn't need anything too special here (After some of my other changes).
- Allowed the "deletable" model to be shown in response to provide a little more detail on the main deleted item.
- Updated parent/child-count loading to be on the "deletable" model instead of additional properties which results in simpler controller logic and enforces the idea these are relations on the deletable, not the deletion itself. It also removes additional exposure of model namespacing.
- Updated (int) casts to intval, just since that's our most common conversion method in the codebase.
- Testing: Removed `actingAsAuthorizedUser` and used the admin user instead to prevent extra auth steps on each test.
- Testing: Cut logic/data-checks from tests if already covered by other tests.
- Testing: Added simple assertions for delete/restore response data.
- Examples: Updated list example to reflect changes.

Review of PR #3377
To be followed up with changes to polymorphic relations to hide
namespacing.

diff --git a/app/Entities/Models/Deletion.php b/app/Entities/Models/Deletion.php
index 181c9c5803d6441254bd89fc92e66cbddf27c4d0..101a138d1ce2185e2623fa03f664299252771f73 100644 (file)
--- a/app/Entities/Models/Deletion.php
+++ b/app/Entities/Models/Deletion.php
@@ -10,10 +10,16 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo;
 use Illuminate\Database\Eloquent\Relations\MorphTo;
 
 /**
 use Illuminate\Database\Eloquent\Relations\MorphTo;
 
 /**

+ * @property int $id
+ * @property int $deleted_by
+ * @property string $deletable_type
+ * @property int $deletable_id

  * @property Deletable $deletable
  */
 class Deletion extends Model implements Loggable
 {
  * @property Deletable $deletable
  */
 class Deletion extends Model implements Loggable
 {

+    protected $hidden = [];
+

     /**
      * Get the related deletable record.
      */
     /**
      * Get the related deletable record.
      */

diff --git a/app/Http/Controllers/Api/RecycleBinApiController.php b/app/Http/Controllers/Api/RecycleBinApiController.php
index a2176a2c8a34af3c0211dfba077f4f3b4e246717..bbe19bd86ec383658f28172ded1f29d9fa73b311 100644 (file)
--- a/app/Http/Controllers/Api/RecycleBinApiController.php
+++ b/app/Http/Controllers/Api/RecycleBinApiController.php
@@ -2,16 +2,16 @@
 
 namespace BookStack\Http\Controllers\Api;
 
 
 namespace BookStack\Http\Controllers\Api;
 

+use BookStack\Entities\Models\Book;
+use BookStack\Entities\Models\BookChild;
+use BookStack\Entities\Models\Chapter;

 use BookStack\Entities\Models\Deletion;
 use BookStack\Entities\Repos\DeletionRepo;
 use Closure;
 use BookStack\Entities\Models\Deletion;
 use BookStack\Entities\Repos\DeletionRepo;
 use Closure;

+use Illuminate\Database\Eloquent\Builder;

 
 class RecycleBinApiController extends ApiController
 {
 
 class RecycleBinApiController extends ApiController
 {

-    protected $fieldsToExpose = [
-        'id', 'deleted_by', 'created_at', 'updated_at', 'deletable_type', 'deletable_id',
-    ];
-

     public function __construct()
     {
         $this->middleware(function ($request, $next) {
     public function __construct()
     {
         $this->middleware(function ($request, $next) {


@@ -24,7 +24,11 @@ class RecycleBinApiController extends ApiController
 
     /**
      * Get a top-level listing of the items in the recycle bin.
 
     /**
      * Get a top-level listing of the items in the recycle bin.

-     * Requires the permission to manage settings and restrictions.
+     * The "deletable" property will reflect the main item deleted.
+     * For books and chapters, counts of child pages/chapters will
+     * be loaded within this "deletable" data.
+     * For chapters & pages, the parent item will be loaded within this "deletable" data.
+     * Requires permission to manage both system settings and permissions.

      */
     public function list()
     {
      */
     public function list()
     {


@@ -40,11 +44,11 @@ class RecycleBinApiController extends ApiController
 
     /**
      * Restore a single deletion from the recycle bin.
 
     /**
      * Restore a single deletion from the recycle bin.

-     * You must provide the deletion id, not the id of the corresponding deleted item.
+     * Requires permission to manage both system settings and permissions.

      */
      */

-    public function restore(DeletionRepo $deletionRepo, string $id)
+    public function restore(DeletionRepo $deletionRepo, string $deletionId)

     {
     {

-        $restoreCount = $deletionRepo->restore((int) $id);
+        $restoreCount = $deletionRepo->restore(intval($deletionId));

 
         return response()->json(['restore_count' => $restoreCount]);
     }
 
         return response()->json(['restore_count' => $restoreCount]);
     }


@@ -52,44 +56,35 @@ class RecycleBinApiController extends ApiController
     /**
      * Remove a single deletion from the recycle bin.
      * Use this endpoint carefully as it will entirely remove the underlying deleted items from the system.
     /**
      * Remove a single deletion from the recycle bin.
      * Use this endpoint carefully as it will entirely remove the underlying deleted items from the system.

-     * You must provide the deletion id, not the id of the corresponding deleted item.
+     * Requires permission to manage both system settings and permissions.

      */
      */

-    public function destroy(DeletionRepo $deletionRepo, string $id)
+    public function destroy(DeletionRepo $deletionRepo, string $deletionId)

     {
     {

-        $deleteCount = $deletionRepo->destroy((int) $id);
+        $deleteCount = $deletionRepo->destroy(intval($deletionId));

 
         return response()->json(['delete_count' => $deleteCount]);
     }
 
 
         return response()->json(['delete_count' => $deleteCount]);
     }
 

+    /**
+     * Load some related details for the deletion listing.
+     */

     protected function listFormatter(Deletion $deletion)
     {
     protected function listFormatter(Deletion $deletion)
     {

-        $deletion->makeVisible($this->fieldsToExpose);
-        $deletion->makeHidden('deletable');
-

         $deletable = $deletion->deletable;
         $deletable = $deletion->deletable;

-        $isBook = $deletion->deletable_type === "BookStack\Book";
-        $parent = null;
-        $children = null;
+        $withTrashedQuery = fn(Builder $query) => $query->withTrashed();

 
 

-        if ($isBook) {
-            $chapterCount = $deletable->chapters()->withTrashed()->count();
-            $children['BookStack\Chapter'] = $chapterCount;
+        if ($deletable instanceof BookChild) {
+            $parent = $deletable->getParent();
+            $parent->setAttribute('type', $parent->getType());
+            $deletable->setRelation('parent', $parent);

         }
 
         }
 

-        if ($isBook || $deletion->deletable_type === "BookStack\Chapter") {
-            $pageCount = $deletable->pages()->withTrashed()->count();
-            $children['BookStack\Page'] = $pageCount;
+        if ($deletable instanceof Book || $deletable instanceof Chapter) {
+            $countsToLoad = ['pages' => $withTrashedQuery];
+            if ($deletable instanceof Book) {
+                $countsToLoad['chapters'] = $withTrashedQuery;
+            }
+            $deletable->loadCount($countsToLoad);

         }
         }

-
-        $parentEntity = $deletable->getParent();
-        $parent = null;
-
-        if ($parentEntity) {
-            $parent['type'] = $parentEntity->getMorphClass();
-            $parent['id'] = $parentEntity->getKey();
-        }
-
-        $deletion->setAttribute('parent', $parent);
-        $deletion->setAttribute('children', $children);

     }
 }
     }
 }

diff --git a/dev/api/responses/recycle_bin-destroy.json b/dev/api/responses/recycle-bin-destroy.json
similarity index 100%
rename from dev/api/responses/recycle_bin-destroy.json
rename to dev/api/responses/recycle-bin-destroy.json

diff --git a/dev/api/responses/recycle-bin-list.json b/dev/api/responses/recycle-bin-list.json
new file mode 100644 (file)
index 0000000..8530708
--- /dev/null
+++ b/dev/api/responses/recycle-bin-list.json
@@ -0,0 +1,64 @@
+{
+  "data": [
+    {
+      "id": 18,
+      "deleted_by": 1,
+      "created_at": "2022-04-20T12:57:46.000000Z",
+      "updated_at": "2022-04-20T12:57:46.000000Z",
+      "deletable_type": "page",
+      "deletable_id": 2582,
+      "deletable": {
+        "id": 2582,
+        "book_id": 25,
+        "chapter_id": 0,
+        "name": "A Wonderful Page",
+        "slug": "a-wonderful-page",
+        "priority": 9,
+        "created_at": "2022-02-08T00:44:45.000000Z",
+        "updated_at": "2022-04-20T12:57:46.000000Z",
+        "created_by": 1,
+        "updated_by": 1,
+        "draft": false,
+        "revision_count": 1,
+        "template": false,
+        "owned_by": 1,
+        "editor": "wysiwyg",
+        "book_slug": "a-great-book",
+        "parent": {
+          "id": 25,
+          "name": "A Great Book",
+          "slug": "a-great-book",
+          "description": "",
+          "created_at": "2022-01-24T16:14:28.000000Z",
+          "updated_at": "2022-03-06T15:14:50.000000Z",
+          "created_by": 1,
+          "updated_by": 1,
+          "owned_by": 1,
+          "type": "book"
+        }
+      }
+    },
+    {
+      "id": 19,
+      "deleted_by": 1,
+      "created_at": "2022-04-25T16:07:46.000000Z",
+      "updated_at": "2022-04-25T16:07:46.000000Z",
+      "deletable_type": "book",
+      "deletable_id": 13,
+      "deletable": {
+        "id": 13,
+        "name": "A Big Book!",
+        "slug": "a-big-book",
+        "description": "This is a very large book with loads of cool stuff in it!",
+        "created_at": "2021-11-08T11:26:43.000000Z",
+        "updated_at": "2022-04-25T16:07:47.000000Z",
+        "created_by": 27,
+        "updated_by": 1,
+        "owned_by": 1,
+        "pages_count": 208,
+        "chapters_count": 50
+      }
+    }
+  ],
+  "total": 2
+}
\ No newline at end of file

diff --git a/dev/api/responses/recycle_bin-restore.json b/dev/api/responses/recycle-bin-restore.json
similarity index 100%
rename from dev/api/responses/recycle_bin-restore.json
rename to dev/api/responses/recycle-bin-restore.json

diff --git a/dev/api/responses/recycle_bin-list.json b/dev/api/responses/recycle_bin-list.json
deleted file mode 100644 (file)
index 025cc98..0000000
--- a/dev/api/responses/recycle_bin-list.json
+++ /dev/null
@@ -1,34 +0,0 @@
-{
-  "data": [
-    {
-      "id": 25,
-      "deleted_by": 1,
-      "created_at": "2022-04-24T07:59:34.000000Z",
-      "updated_at": "2022-04-24T07:59:34.000000Z",
-      "deletable_type": "BookStack\\Book",
-      "deletable_id": 4,
-      "parent": {
-        "type": "BookStack\\Book",
-        "id": 25
-      },
-      "children": {
-        "BookStack\\Chapter": 0,
-        "BookStack\\Page": 1
-      }
-    },
-    {
-      "id": 26,
-      "deleted_by": 1,
-      "created_at": "2022-04-24T07:59:35.000000Z",
-      "updated_at": "2022-04-24T07:59:35.000000Z",
-      "deletable_type": "BookStack\\Book",
-      "deletable_id": 3,
-      "parent": [],
-      "children": {
-        "BookStack\\Chapter": 1,
-        "BookStack\\Page": 1
-      }
-    }
-  ],
-  "total": 2
-}
\ No newline at end of file

diff --git a/routes/api.php b/routes/api.php
index 465f2392c831702f11cc1a3eadc41cae1475db91..20e167d70934c0d35104014986780a4c2160a510 100644 (file)
--- a/routes/api.php
+++ b/routes/api.php
@@ -74,6 +74,6 @@ Route::get('users/{id}', [UserApiController::class, 'read']);
 Route::put('users/{id}', [UserApiController::class, 'update']);
 Route::delete('users/{id}', [UserApiController::class, 'delete']);
 
 Route::put('users/{id}', [UserApiController::class, 'update']);
 Route::delete('users/{id}', [UserApiController::class, 'delete']);
 

-Route::get('recycle_bin', [RecycleBinApiController::class, 'list']);
-Route::put('recycle_bin/{id}', [RecycleBinApiController::class, 'restore']);
-Route::delete('recycle_bin/{id}', [RecycleBinApiController::class, 'destroy']);
+Route::get('recycle-bin', [RecycleBinApiController::class, 'list']);
+Route::put('recycle-bin/{deletionId}', [RecycleBinApiController::class, 'restore']);
+Route::delete('recycle-bin/{deletionId}', [RecycleBinApiController::class, 'destroy']);

diff --git a/tests/Api/RecycleBinApiTest.php b/tests/Api/RecycleBinApiTest.php
index 4849080b9489902bd90bb8cb3ce691ae92b79570..05c896bd9632a47f9da1e9f546e4f79b46cb5321 100644 (file)
--- a/tests/Api/RecycleBinApiTest.php
+++ b/tests/Api/RecycleBinApiTest.php
@@ -12,12 +12,12 @@ class RecycleBinApiTest extends TestCase
 {
     use TestsApi;
 
 {
     use TestsApi;
 

-    protected string $baseEndpoint = '/api/recycle_bin';
+    protected string $baseEndpoint = '/api/recycle-bin';

 
     protected array $endpointMap = [
 
     protected array $endpointMap = [

-        ['get', '/api/recycle_bin'],
-        ['put', '/api/recycle_bin/1'],
-        ['delete', '/api/recycle_bin/1'],
+        ['get', '/api/recycle-bin'],
+        ['put', '/api/recycle-bin/1'],
+        ['delete', '/api/recycle-bin/1'],

     ];
 
     public function test_settings_manage_permission_needed_for_all_endpoints()
     ];
 
     public function test_settings_manage_permission_needed_for_all_endpoints()


@@ -48,13 +48,12 @@ class RecycleBinApiTest extends TestCase
 
     public function test_index_endpoint_returns_expected_page()
     {
 
     public function test_index_endpoint_returns_expected_page()
     {

-        $this->actingAsAuthorizedUser();
+        $admin = $this->getAdmin();

 
         $page = Page::query()->first();
 
         $page = Page::query()->first();

-        $book = Book::query()->whereHas('pages')->whereHas('chapters')->withCount(['pages', 'chapters'])->first();
-        $editor = $this->getEditor();
-        $this->actingAs($editor)->delete($page->getUrl());
-        $this->actingAs($editor)->delete($book->getUrl());
+        $book = Book::query()->first();
+        $this->actingAs($admin)->delete($page->getUrl());
+        $this->delete($book->getUrl());

 
         $deletions = Deletion::query()->orderBy('id')->get();
 
 
         $deletions = Deletion::query()->orderBy('id')->get();
 


@@ -62,14 +61,17 @@ class RecycleBinApiTest extends TestCase
 
         $expectedData = $deletions
             ->zip([$page, $book])
 
         $expectedData = $deletions
             ->zip([$page, $book])

-            ->map(function (Collection $data) use ($editor) {
+            ->map(function (Collection $data) use ($admin) {

                 return [
                     'id'                => $data[0]->id,
                 return [
                     'id'                => $data[0]->id,

-                    'deleted_by'        => $editor->getKey(),
+                    'deleted_by'        => $admin->id,

                     'created_at'        => $data[0]->created_at->toJson(),
                     'updated_at'        => $data[0]->updated_at->toJson(),
                     'deletable_type'    => $data[1]->getMorphClass(),
                     'created_at'        => $data[0]->created_at->toJson(),
                     'updated_at'        => $data[0]->updated_at->toJson(),
                     'deletable_type'    => $data[1]->getMorphClass(),

-                    'deletable_id'      => $data[1]->getKey(),
+                    'deletable_id'      => $data[1]->id,
+                    'deletable'         => [
+                        'name' => $data[1]->name,
+                    ],

                 ];
             });
 
                 ];
             });
 


@@ -79,13 +81,12 @@ class RecycleBinApiTest extends TestCase
         ]);
     }
 
         ]);
     }
 

-    public function test_index_endpoint_returns_children()
+    public function test_index_endpoint_returns_children_count()

     {
     {

-        $this->actingAsAuthorizedUser();
+        $admin = $this->getAdmin();

 
         $book = Book::query()->whereHas('pages')->whereHas('chapters')->withCount(['pages', 'chapters'])->first();
 
         $book = Book::query()->whereHas('pages')->whereHas('chapters')->withCount(['pages', 'chapters'])->first();

-        $editor = $this->getEditor();
-        $this->actingAs($editor)->delete($book->getUrl());
+        $this->actingAs($admin)->delete($book->getUrl());

 
         $deletion = Deletion::query()->orderBy('id')->first();
 
 
         $deletion = Deletion::query()->orderBy('id')->first();
 


@@ -93,17 +94,11 @@ class RecycleBinApiTest extends TestCase
 
         $expectedData = [
             [
 
         $expectedData = [
             [

-                'id'                => $deletion->getKey(),
-                'deleted_by'        => $editor->getKey(),
-                'created_at'        => $deletion->created_at->toJson(),
-                'updated_at'        => $deletion->updated_at->toJson(),
-                'deletable_type'    => $book->getMorphClass(),
-                'deletable_id'      => $book->getKey(),
-                'children'          => [
-                    'BookStack\Page'    => $book->pages_count,
-                    'BookStack\Chapter' => $book->chapters_count,
+                'id'             => $deletion->id,
+                'deletable'      => [
+                    'pages_count'    => $book->pages_count,
+                    'chapters_count' => $book->chapters_count,

                 ],
                 ],

-                'parent' => null,

             ],
         ];
 
             ],
         ];
 


@@ -115,30 +110,24 @@ class RecycleBinApiTest extends TestCase
 
     public function test_index_endpoint_returns_parent()
     {
 
     public function test_index_endpoint_returns_parent()
     {

-        $this->actingAsAuthorizedUser();
-
+        $admin = $this->getAdmin();

         $page = Page::query()->whereHas('chapter')->with('chapter')->first();
 
         $page = Page::query()->whereHas('chapter')->with('chapter')->first();
 

-        $editor = $this->getEditor();
-        $this->actingAs($editor)->delete($page->getUrl());
-
+        $this->actingAs($admin)->delete($page->getUrl());

         $deletion = Deletion::query()->orderBy('id')->first();
 
         $resp = $this->getJson($this->baseEndpoint);
 
         $expectedData = [
             [
         $deletion = Deletion::query()->orderBy('id')->first();
 
         $resp = $this->getJson($this->baseEndpoint);
 
         $expectedData = [
             [

-                'id'                => $deletion->getKey(),
-                'deleted_by'        => $editor->getKey(),
-                'created_at'        => $deletion->created_at->toJson(),
-                'updated_at'        => $deletion->updated_at->toJson(),
-                'deletable_type'    => $page->getMorphClass(),
-                'deletable_id'      => $page->getKey(),
-                'parent'            => [
-                    'type'  => 'BookStack\Chapter',
-                    'id'    => $page->chapter->getKey(),
-                ],
-                'children' => null,
+                'id'             => $deletion->id,
+                'deletable'      => [
+                    'parent' => [
+                        'id'   => $page->chapter->id,
+                        'name' => $page->chapter->name,
+                        'type' => 'chapter'
+                    ]
+                ]

             ],
         ];
 
             ],
         ];
 


@@ -150,53 +139,46 @@ class RecycleBinApiTest extends TestCase
 
     public function test_restore_endpoint()
     {
 
     public function test_restore_endpoint()
     {

-        $this->actingAsAuthorizedUser();
-

         $page = Page::query()->first();
         $page = Page::query()->first();

-        $editor = $this->getEditor();
-        $this->actingAs($editor)->delete($page->getUrl());
+        $this->asAdmin()->delete($page->getUrl());

         $page->refresh();
 
         $deletion = Deletion::query()->orderBy('id')->first();
 
         $this->assertDatabaseHas('pages', [
         $page->refresh();
 
         $deletion = Deletion::query()->orderBy('id')->first();
 
         $this->assertDatabaseHas('pages', [

-            'id'            => $page->getKey(),
+            'id'            => $page->id,

             'deleted_at'    => $page->deleted_at,
         ]);
 
             'deleted_at'    => $page->deleted_at,
         ]);
 

-        $this->putJson($this->baseEndpoint . '/' . $deletion->getKey());
+        $resp = $this->putJson($this->baseEndpoint . '/' . $deletion->id);
+        $resp->assertJson([
+            'restore_count' => 1
+        ]);

 
         $this->assertDatabaseHas('pages', [
 
         $this->assertDatabaseHas('pages', [

-            'id'            => $page->getKey(),
+            'id'            => $page->id,

             'deleted_at'    => null,
         ]);
     }
 
     public function test_destroy_endpoint()
     {
             'deleted_at'    => null,
         ]);
     }
 
     public function test_destroy_endpoint()
     {

-        $this->actingAsAuthorizedUser();
-

         $page = Page::query()->first();
         $page = Page::query()->first();

-        $editor = $this->getEditor();
-        $this->actingAs($editor)->delete($page->getUrl());
+        $this->asAdmin()->delete($page->getUrl());

         $page->refresh();
 
         $deletion = Deletion::query()->orderBy('id')->first();
 
         $this->assertDatabaseHas('pages', [
         $page->refresh();
 
         $deletion = Deletion::query()->orderBy('id')->first();
 
         $this->assertDatabaseHas('pages', [

-            'id'            => $page->getKey(),
+            'id'            => $page->id,

             'deleted_at'    => $page->deleted_at,
         ]);
 
             'deleted_at'    => $page->deleted_at,
         ]);
 

-        $this->deleteJson($this->baseEndpoint . '/' . $deletion->getKey());
-        $this->assertDatabaseMissing('pages', ['id' => $page->getKey()]);
-    }
+        $resp = $this->deleteJson($this->baseEndpoint . '/' . $deletion->id);
+        $resp->assertJson([
+            'delete_count' => 1
+        ]);

 
 

-    private function actingAsAuthorizedUser()
-    {
-        $editor = $this->getEditor();
-        $this->giveUserPermissions($editor, ['restrictions-manage-all']);
-        $this->giveUserPermissions($editor, ['settings-manage']);
-        $this->actingAs($editor);
+        $this->assertDatabaseMissing('pages', ['id' => $page->id]);

     }
 }
     }
 }