Shelves: Addressed book edits removing non-visible books

Tracks the non-visible existing books on change, to retain as part of
the assigned books sync.
Added test to cover.

For #5728

diff --git a/app/Entities/Repos/BookshelfRepo.php b/app/Entities/Repos/BookshelfRepo.php
index 8e60f58c42f0c4d1c735b90fe41b962e32347765..b870ec37747b1cce6e92a784db43f485eca93ff5 100644 (file)
--- a/app/Entities/Repos/BookshelfRepo.php
+++ b/app/Entities/Repos/BookshelfRepo.php
@@ -56,20 +56,37 @@ class BookshelfRepo
 
     /**
      * Update which books are assigned to this shelf by syncing the given book ids.
-     * Function ensures the books are visible to the current user and existing.
+     * Function ensures the managed books are visible to the current user and existing,
+     * and that the user does not alter the assignment of books that are not visible to them.
      */
-    protected function updateBooks(Bookshelf $shelf, array $bookIds)
+    protected function updateBooks(Bookshelf $shelf, array $bookIds): void
     {
         $numericIDs = collect($bookIds)->map(function ($id) {
             return intval($id);
         });
 
-        $syncData = $this->bookQueries->visibleForList()
+        $existingBookIds = $shelf->books()->pluck('id')->toArray();
+        $visibleExistingBookIds = $this->bookQueries->visibleForList()
+            ->whereIn('id', $existingBookIds)
+            ->pluck('id')
+            ->toArray();
+        $nonVisibleExistingBookIds = array_values(array_diff($existingBookIds, $visibleExistingBookIds));
+
+        $newIdsToAssign = $this->bookQueries->visibleForList()
             ->whereIn('id', $bookIds)
             ->pluck('id')
-            ->mapWithKeys(function ($bookId) use ($numericIDs) {
-                return [$bookId => ['order' => $numericIDs->search($bookId)]];
-            });
+            ->toArray();
+
+        $maxNewIndex = max($numericIDs->keys()->toArray() ?: [0]);
+
+        $syncData = [];
+        foreach ($newIdsToAssign as $id) {
+            $syncData[$id] = ['order' => $numericIDs->search($id)];
+        }
+
+        foreach ($nonVisibleExistingBookIds as $index => $id) {
+            $syncData[$id] = ['order' => $maxNewIndex + ($index + 1)];
+        }
 
         $shelf->books()->sync($syncData);
     }

diff --git a/tests/Entity/BookShelfTest.php b/tests/Entity/BookShelfTest.php
index fb9862931ae4c211cdc9a184fa533d2f41a00dc5..ad1d64e7126570c7e3388a0201e39c2adef66edd 100644 (file)
--- a/tests/Entity/BookShelfTest.php
+++ b/tests/Entity/BookShelfTest.php
@@ -259,6 +259,35 @@ class BookShelfTest extends TestCase
         $this->assertDatabaseHas('bookshelves_books', ['bookshelf_id' => $shelf->id, 'book_id' => $booksToInclude[1]->id]);
     }
 
+    public function test_shelf_edit_does_not_alter_books_we_dont_have_access_to()
+    {
+        $shelf = $this->entities->shelf();
+        $shelf->books()->detach();
+        $this->entities->book();
+        $this->entities->book();
+
+        $newBooks = [$this->entities->book(), $this->entities->book()];
+        $originalBooks = [$this->entities->book(), $this->entities->book()];
+        foreach ($originalBooks as $book) {
+            $this->permissions->disableEntityInheritedPermissions($book);
+            $shelf->books()->attach($book->id);
+        }
+
+        $this->asEditor()->put($shelf->getUrl(), [
+            'name' => $shelf->name,
+            'books' => "{$newBooks[0]->id},{$newBooks[1]->id}",
+        ])->assertRedirect($shelf->getUrl());
+
+        $resultingBooksById = $shelf->books()->get()->keyBy('id')->toArray();
+        $this->assertCount(4, $resultingBooksById);
+        foreach ($newBooks as $book) {
+            $this->assertArrayHasKey($book->id, $resultingBooksById);
+        }
+        foreach ($originalBooks as $book) {
+            $this->assertArrayHasKey($book->id, $resultingBooksById);
+        }
+    }
+
     public function test_shelf_create_new_book()
     {
         $shelf = $this->entities->shelf();