Slugs: Fixed storage bugs, added testing coverage

app/Entities/Repos/BaseRepo.php

@@ -165,10 +165,7 @@ class BaseRepo
      */
     public function refreshSlug(Entity $entity): void
     {
-        if ($entity->id) {
+        $this->slugHistory->recordForEntity($entity);
-            $this->slugHistory->recordForEntity($entity);
-        }
-
         $this->slugGenerator->regenerateForEntity($entity);
     }
 }

app/Entities/Tools/SlugHistory.php

@@ -2,6 +2,7 @@
 
 namespace BookStack\Entities\Tools;
 
+use BookStack\Entities\Models\BookChild;
 use BookStack\Entities\Models\Entity;
 use Illuminate\Support\Facades\DB;
 
@@ -12,16 +13,25 @@ class SlugHistory
      */
     public function recordForEntity(Entity $entity): void
     {
+        if (!$entity->id || !$entity->slug) {
+            return;
+        }
+
         $latest = $this->getLatestEntryForEntity($entity);
         if ($latest && $latest->slug === $entity->slug && $latest->parent_slug === $entity->getParent()?->slug) {
             return;
         }
 
+        $parentSlug = null;
+        if ($entity instanceof BookChild) {
+            $parentSlug = $entity->book()->first()?->slug;
+        }
+
         $info = [
             'sluggable_type' => $entity->getMorphClass(),
             'sluggable_id'   => $entity->id,
             'slug'           => $entity->slug,
-            'parent_slug'    => $entity->getParent()?->slug,
+            'parent_slug'    => $parentSlug,
             'created_at'     => now(),
             'updated_at'     => now(),
         ];

tests/Entity/BookTest.php

@@ -238,30 +238,6 @@ class BookTest extends TestCase
         $this->assertEquals('list', setting()->getUser($editor, 'books_view_type'));
     }
 
-    public function test_slug_multi_byte_url_safe()
-    {
-        $book = $this->entities->newBook([
-            'name' => 'информация',
-        ]);
-
-        $this->assertEquals('informaciia', $book->slug);
-
-        $book = $this->entities->newBook([
-            'name' => '¿Qué?',
-        ]);
-
-        $this->assertEquals('que', $book->slug);
-    }
-
-    public function test_slug_format()
-    {
-        $book = $this->entities->newBook([
-            'name' => 'PartA / PartB / PartC',
-        ]);
-
-        $this->assertEquals('parta-partb-partc', $book->slug);
-    }
-
     public function test_description_limited_to_specific_html()
     {
         $book = $this->entities->book();

tests/Entity/PageTest.php

@@ -269,28 +269,6 @@ class PageTest extends TestCase
         ]);
     }
 
-    public function test_old_page_slugs_redirect_to_new_pages()
-    {
-        $page = $this->entities->page();
-
-        // Need to save twice since revisions are not generated in seeder.
-        $this->asAdmin()->put($page->getUrl(), [
-            'name' => 'super test',
-            'html' => '<p></p>',
-        ]);
-
-        $page->refresh();
-        $pageUrl = $page->getUrl();
-
-        $this->put($pageUrl, [
-            'name' => 'super test page',
-            'html' => '<p></p>',
-        ]);
-
-        $this->get($pageUrl)
-            ->assertRedirect("/books/{$page->book->slug}/page/super-test-page");
-    }
-
     public function test_page_within_chapter_deletion_returns_to_chapter()
     {
         $chapter = $this->entities->chapter();

tests/Entity/SlugTest.php

@@ -0,0 +1,131 @@
+<?php
+
+namespace Tests\Entity;
+
+use Tests\TestCase;
+
+class SlugTest extends TestCase
+{
+    public function test_slug_multi_byte_url_safe()
+    {
+        $book = $this->entities->newBook([
+            'name' => 'информация',
+        ]);
+
+        $this->assertEquals('informaciia', $book->slug);
+
+        $book = $this->entities->newBook([
+            'name' => '¿Qué?',
+        ]);
+
+        $this->assertEquals('que', $book->slug);
+    }
+
+    public function test_slug_format()
+    {
+        $book = $this->entities->newBook([
+            'name' => 'PartA / PartB / PartC',
+        ]);
+
+        $this->assertEquals('parta-partb-partc', $book->slug);
+    }
+
+    public function test_old_page_slugs_redirect_to_new_pages()
+    {
+        $page = $this->entities->page();
+
+        // Need to save twice since revisions are not generated in seeder.
+        $this->asAdmin()->put($page->getUrl(), [
+            'name' => 'super test',
+            'html' => '<p></p>',
+        ]);
+
+        $page->refresh();
+        $pageUrl = $page->getUrl();
+
+        $this->put($pageUrl, [
+            'name' => 'super test page',
+            'html' => '<p></p>',
+        ]);
+
+        $this->get($pageUrl)
+            ->assertRedirect("/books/{$page->book->slug}/page/super-test-page");
+    }
+
+    public function test_slugs_recorded_in_history_on_page_update()
+    {
+        $page = $this->entities->page();
+        $this->asAdmin()->put($page->getUrl(), [
+            'name' => 'new slug',
+            'html' => '<p></p>',
+        ]);
+
+        $oldSlug = $page->slug;
+        $page->refresh();
+        $this->assertNotEquals($oldSlug, $page->slug);
+
+        $this->assertDatabaseHas('slug_history', [
+            'sluggable_id' => $page->id,
+            'sluggable_type' => 'page',
+            'slug' => $oldSlug,
+            'parent_slug' => $page->book->slug,
+        ]);
+    }
+
+    public function test_slugs_recorded_in_history_on_chapter_update()
+    {
+        $chapter = $this->entities->chapter();
+        $this->asAdmin()->put($chapter->getUrl(), [
+            'name' => 'new slug',
+        ]);
+
+        $oldSlug = $chapter->slug;
+        $chapter->refresh();
+        $this->assertNotEquals($oldSlug, $chapter->slug);
+
+        $this->assertDatabaseHas('slug_history', [
+            'sluggable_id' => $chapter->id,
+            'sluggable_type' => 'chapter',
+            'slug' => $oldSlug,
+            'parent_slug' => $chapter->book->slug,
+        ]);
+    }
+
+    public function test_slugs_recorded_in_history_on_book_update()
+    {
+        $book = $this->entities->book();
+        $this->asAdmin()->put($book->getUrl(), [
+            'name' => 'new slug',
+        ]);
+
+        $oldSlug = $book->slug;
+        $book->refresh();
+        $this->assertNotEquals($oldSlug, $book->slug);
+
+        $this->assertDatabaseHas('slug_history', [
+            'sluggable_id' => $book->id,
+            'sluggable_type' => 'book',
+            'slug' => $oldSlug,
+            'parent_slug' => null,
+        ]);
+    }
+
+    public function test_slugs_recorded_in_history_on_shelf_update()
+    {
+        $shelf = $this->entities->shelf();
+        $this->asAdmin()->put($shelf->getUrl(), [
+            'name' => 'new slug',
+        ]);
+
+        $oldSlug = $shelf->slug;
+        $shelf->refresh();
+        $this->assertNotEquals($oldSlug, $shelf->slug);
+
+        $this->assertDatabaseHas('slug_history', [
+            'sluggable_id' => $shelf->id,
+            'sluggable_type' => 'bookshelf',
+            'slug' => $oldSlug,
+            'parent_slug' => null,
+        ]);
+    }
+}