Merge pull request #2086 from kcaran/tags_unique

2084: Ensure tags are unique when renaming
shaarli · Nov 15, 2024 · 3ba5297 · 3ba5297
2 parents 2169815 + 2db759f
commit 3ba5297
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 50 deletions.
diff --git a/application/bookmark/Bookmark.php b/application/bookmark/Bookmark.php
@@ -505,15 +505,19 @@ public function getAdditionalContentEntry(string $key, $default = null)
     }
 
     /**
-     * Rename a tag in tags list.
+     * Rename a tag in tags list. If the new tag already exists, merge them
      *
      * @param string $fromTag
      * @param string $toTag
      */
     public function renameTag(string $fromTag, string $toTag): void
     {
         if (($pos = array_search($fromTag, $this->tags ?? [])) !== false) {
-            $this->tags[$pos] = trim($toTag);
+            if (in_array($toTag, $this->tags ?? []) !== false) {
+                $this->deleteTag($fromTag);
+            } else {
+                $this->tags[$pos] = trim($toTag);
+            }
         }
     }
 

diff --git a/tests/bookmark/BookmarkTest.php b/tests/bookmark/BookmarkTest.php
@@ -374,6 +374,9 @@ public function testSetTags()
     public function testRenameTag()
     {
         $bookmark = new Bookmark();
+        $bookmark->renameTag('chair', 'table');
+        $this->assertEquals([], $bookmark->getTags());
+
         $bookmark->setTags(['tag1', 'tag2', 'chair']);
         $bookmark->renameTag('chair', 'table');
         $this->assertEquals(['tag1', 'tag2', 'table'], $bookmark->getTags());
@@ -396,6 +399,17 @@ public function testRenameTagNotExists()
         $this->assertEquals(['tag1', 'tag2', 'chair'], $bookmark->getTags());
     }
 
+    /**
+     * Test renameTag() avoid duplicating existing tag
+     */
+    public function testRenameTagNotDuplicated()
+    {
+        $bookmark = new Bookmark();
+        $bookmark->setTags(['tag1', 'tag2', 'chair']);
+        $bookmark->renameTag('tag1', 'chair');
+        $this->assertEquals(['tag2', 'chair'], $bookmark->getTags());
+    }
+
     /**
      * Test deleteTag()
      */

diff --git a/tests/front/controller/admin/ManageTagControllerTest.php b/tests/front/controller/admin/ManageTagControllerTest.php
@@ -13,6 +13,12 @@
 use Shaarli\TestCase;
 use Slim\Http\Request;
 use Slim\Http\Response;
+// These are declared for the bookmark service
+use malkusch\lock\mutex\NoMutex;
+use Shaarli\History;
+use Shaarli\Plugin\PluginManager;
+use Shaarli\Tests\Utils\FakeBookmarkService;
+use Shaarli\Tests\Utils\ReferenceLinkDB;
 
 class ManageTagControllerTest extends TestCase
 {
@@ -21,11 +27,46 @@ class ManageTagControllerTest extends TestCase
     /** @var ManageTagController */
     protected $controller;
 
+    /** @var string Path of test data store */
+    protected static $testDatastore = 'sandbox/datastore.php';
+
+    /** @var BookmarkServiceInterface instance */
+    protected $bookmarkService;
+
+    /** @var BookmarkFilter instance */
+    protected $linkFilter;
+
+    /** @var ReferenceLinkDB instance */
+    protected static $refDB;
+
+    /** @var PluginManager */
+    protected static $pluginManager;
+
     public function setUp(): void
     {
         $this->createContainer();
 
         $this->controller = new ManageTagController($this->container);
+
+        $mutex = new NoMutex();
+        $conf = new ConfigManager('tests/utils/config/configJson');
+        $conf->set('resource.datastore', self::$testDatastore);
+        static::$pluginManager = new PluginManager($conf);
+        self::$refDB = new ReferenceLinkDB();
+        self::$refDB->write(self::$testDatastore);
+        $history = new History('sandbox/history.php');
+        $this->container->bookmarkService = new FakeBookmarkService(
+            $conf,
+            static::$pluginManager,
+            $history,
+            $mutex,
+            true
+        );
+        $this->linkFilter = new BookmarkFilter(
+            $this->container->bookmarkService->getBookmarks(),
+            $conf,
+            static::$pluginManager
+        );
     }
 
     /**
@@ -80,10 +121,23 @@ public function testSaveRenameTagValid(): void
         $session = [];
         $this->assignSessionVars($session);
 
+        $this->assertEquals(
+            3,
+            count($this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'cartoon'))
+        );
+        $this->assertEquals(
+            4,
+            count($this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'web'))
+        );
+        $this->assertEquals(
+            2,
+            count($this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'cartoon web'))
+        );
+
         $requestParameters = [
             'renametag' => 'rename',
-            'fromtag' => 'old-tag',
-            'totag' => 'new-tag',
+            'fromtag' => 'cartoon',
+            'totag' => 'web',
         ];
         $request = $this->createMock(Request::class);
         $request
@@ -94,36 +148,33 @@ public function testSaveRenameTagValid(): void
             })
         ;
         $response = new Response();
-
-        $bookmark1 = $this->createMock(Bookmark::class);
-        $bookmark2 = $this->createMock(Bookmark::class);
-        $this->container->bookmarkService
-            ->expects(static::once())
-            ->method('search')
-            ->with(['searchtags' => 'old-tag'], BookmarkFilter::$ALL, true)
-            ->willReturnCallback(function () use ($bookmark1, $bookmark2): SearchResult {
-                $bookmark1->expects(static::once())->method('renameTag')->with('old-tag', 'new-tag');
-                $bookmark2->expects(static::once())->method('renameTag')->with('old-tag', 'new-tag');
-
-                return SearchResult::getSearchResult([$bookmark1, $bookmark2]);
-            })
-        ;
-        $this->container->bookmarkService
-            ->expects(static::exactly(2))
-            ->method('set')
-            ->withConsecutive([$bookmark1, false], [$bookmark2, false])
-        ;
-        $this->container->bookmarkService->expects(static::once())->method('save');
-
         $result = $this->controller->save($request, $response);
-
         static::assertSame(302, $result->getStatusCode());
-        static::assertSame(['/subfolder/?searchtags=new-tag'], $result->getHeader('location'));
+        static::assertSame(['/subfolder/?searchtags=web'], $result->getHeader('location'));
 
         static::assertArrayNotHasKey(SessionManager::KEY_ERROR_MESSAGES, $session);
         static::assertArrayNotHasKey(SessionManager::KEY_WARNING_MESSAGES, $session);
         static::assertArrayHasKey(SessionManager::KEY_SUCCESS_MESSAGES, $session);
-        static::assertSame(['The tag was renamed in 2 bookmarks.'], $session[SessionManager::KEY_SUCCESS_MESSAGES]);
+        static::assertSame(['The tag was renamed in 3 bookmarks.'], $session[SessionManager::KEY_SUCCESS_MESSAGES]);
+
+        $this->assertEquals(
+            0,
+            count($this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'cartoon'))
+        );
+        $new = $this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'web');
+        $this->assertEquals(
+            5,
+            count($new)
+        );
+
+        // Make sure there are no duplicate tags
+        foreach ($new as $bookmark) {
+            $tags = $bookmark->getTags();
+            $this->assertEquals(
+                count($tags),
+                count(array_unique($tags))
+            );
+        }
     }
 
     /**
@@ -134,10 +185,16 @@ public function testSaveDeleteTagValid(): void
         $session = [];
         $this->assignSessionVars($session);
 
+        $this->assertEquals(
+            3,
+            count($this->linkFilter->filter(BookmarkFilter::$FILTER_TAG, 'cartoon'))
+        );
+
         $requestParameters = [
             'deletetag' => 'delete',
-            'fromtag' => 'old-tag',
+            'fromtag' => 'cartoon',
         ];
+
         $request = $this->createMock(Request::class);
         $request
             ->expects(static::atLeastOnce())
@@ -148,26 +205,6 @@ public function testSaveDeleteTagValid(): void
         ;
         $response = new Response();
 
-        $bookmark1 = $this->createMock(Bookmark::class);
-        $bookmark2 = $this->createMock(Bookmark::class);
-        $this->container->bookmarkService
-            ->expects(static::once())
-            ->method('search')
-            ->with(['searchtags' => 'old-tag'], BookmarkFilter::$ALL, true)
-            ->willReturnCallback(function () use ($bookmark1, $bookmark2): SearchResult {
-                $bookmark1->expects(static::once())->method('deleteTag')->with('old-tag');
-                $bookmark2->expects(static::once())->method('deleteTag')->with('old-tag');
-
-                return SearchResult::getSearchResult([$bookmark1, $bookmark2]);
-            })
-        ;
-        $this->container->bookmarkService
-            ->expects(static::exactly(2))
-            ->method('set')
-            ->withConsecutive([$bookmark1, false], [$bookmark2, false])
-        ;
-        $this->container->bookmarkService->expects(static::once())->method('save');
-
         $result = $this->controller->save($request, $response);
 
         static::assertSame(302, $result->getStatusCode());
@@ -176,7 +213,7 @@ public function testSaveDeleteTagValid(): void
         static::assertArrayNotHasKey(SessionManager::KEY_ERROR_MESSAGES, $session);
         static::assertArrayNotHasKey(SessionManager::KEY_WARNING_MESSAGES, $session);
         static::assertArrayHasKey(SessionManager::KEY_SUCCESS_MESSAGES, $session);
-        static::assertSame(['The tag was removed from 2 bookmarks.'], $session[SessionManager::KEY_SUCCESS_MESSAGES]);
+        static::assertSame(['The tag was removed from 3 bookmarks.'], $session[SessionManager::KEY_SUCCESS_MESSAGES]);
     }
 
     /**