diff --git a/app/Entities/Repos/BookshelfRepo.php b/app/Entities/Repos/BookshelfRepo.php index 8e60f58c4..b870ec377 100644 --- 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 fb9862931..ad1d64e71 100644 --- 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();