From 39133eb796996701228501f898b4ef33af8e0fdb Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 4 Apr 2020 21:03:22 +0200 Subject: TagController: fix duplicated tags when renaming them The fix relies on a workaround available on TagsAssigner, see the AssignTagsToEntry() signature for detail. I replaced the findOneByLabel in the corresponding test to assert that there is no duplicate. Fixes #4216 Signed-off-by: Kevin Decherf --- .../CoreBundle/Controller/TagController.php | 7 +++- .../CoreBundle/Controller/TagControllerTest.php | 38 ++++++++++++++++++---- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php index a6ad131f..c228c27a 100644 --- a/src/Wallabag/CoreBundle/Controller/TagController.php +++ b/src/Wallabag/CoreBundle/Controller/TagController.php @@ -152,6 +152,10 @@ class TagController extends Controller $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { + $newTagLabel = $form->get('label')->getData(); + $newTag = new Tag(); + $newTag->setLabel($newTagLabel); + $entries = $this->get('wallabag_core.entry_repository')->findAllByTagId( $this->getUser()->getId(), $tag->getId() @@ -159,7 +163,8 @@ class TagController extends Controller foreach ($entries as $entry) { $this->get('wallabag_core.tags_assigner')->assignTagsToEntry( $entry, - $form->get('label')->getData() + $newTagLabel, + [$newTag] ); $entry->removeTag($tag); } diff --git a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php index 47c83a7b..20e60c32 100644 --- a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php @@ -179,15 +179,24 @@ class TagControllerTest extends WallabagCoreTestCase public function testRenameTagUsingTheFormInsideTagList() { + $newTagLabel = 'rename label'; + $this->logInAs('admin'); $client = $this->getClient(); $tag = new Tag(); $tag->setLabel($this->tagName); + $entry = new Entry($this->getLoggedInUser()); $entry->setUrl('http://0.0.0.0/foo'); $entry->addTag($tag); $this->getEntityManager()->persist($entry); + + $entry2 = new Entry($this->getLoggedInUser()); + $entry2->setUrl('http://0.0.0.0/bar'); + $entry2->addTag($tag); + $this->getEntityManager()->persist($entry); + $this->getEntityManager()->flush(); $this->getEntityManager()->clear(); @@ -196,7 +205,7 @@ class TagControllerTest extends WallabagCoreTestCase $form = $crawler->filter('#tag-' . $tag->getId() . ' form')->form(); $data = [ - 'tag[label]' => 'specific label', + 'tag[label]' => $newTagLabel, ]; $client->submit($form, $data); @@ -207,19 +216,34 @@ class TagControllerTest extends WallabagCoreTestCase ->getRepository('WallabagCoreBundle:Entry') ->find($entry->getId()); - $tags = $freshEntry->getTags()->toArray(); - foreach ($tags as $key => $item) { + $freshEntry2 = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->find($entry2->getId()); + + $tags = []; + + $tagsFromEntry = $freshEntry->getTags()->toArray(); + foreach ($tagsFromEntry as $key => $item) { $tags[$key] = $item->getLabel(); } - $this->assertFalse(array_search($tag->getLabel(), $tags, true), 'Previous tag is not attach to entry anymore.'); + $tagsFromEntry2 = $freshEntry2->getTags()->toArray(); + foreach ($tagsFromEntry2 as $key => $item) { + $tags[$key] = $item->getLabel(); + } + + $this->assertFalse(array_search($tag->getLabel(), $tags, true), 'Previous tag is not attach to entries anymore.'); $newTag = $client->getContainer() ->get('doctrine.orm.entity_manager') ->getRepository('WallabagCoreBundle:Tag') - ->findOneByLabel('specific label'); - $this->assertInstanceOf(Tag::class, $newTag, 'Tag "specific label" exists.'); - $this->assertTrue($newTag->hasEntry($freshEntry), 'Tag "specific label" is assigned to the entry.'); + ->findByLabel($newTagLabel); + + $this->assertCount(1, $newTag, 'New tag exists.'); + + $this->assertTrue($newTag[0]->hasEntry($freshEntry), 'New tag is assigned to the entry.'); + $this->assertTrue($newTag[0]->hasEntry($freshEntry2), 'New tag is assigned to the entry2.'); } public function testAddUnicodeTagLabel() -- cgit v1.2.3 From a19caf8a37dfd59a4e270507ec08e9fc259e3e1e Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 4 Apr 2020 22:08:08 +0200 Subject: TagController: prevent tag deletion when renaming a tag with the same label Signed-off-by: Kevin Decherf --- .../CoreBundle/Controller/TagController.php | 15 +++--- .../CoreBundle/Controller/TagControllerTest.php | 58 ++++++++++++++++++++++ 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php index c228c27a..f7b78f5d 100644 --- a/src/Wallabag/CoreBundle/Controller/TagController.php +++ b/src/Wallabag/CoreBundle/Controller/TagController.php @@ -151,7 +151,10 @@ class TagController extends Controller $form = $this->createForm(RenameTagType::class, new Tag()); $form->handleRequest($request); - if ($form->isSubmitted() && $form->isValid()) { + if ($form->isSubmitted() + && $form->isValid() + && $form->get('label')->getData() !== $tag->getLabel() + ) { $newTagLabel = $form->get('label')->getData(); $newTag = new Tag(); $newTag->setLabel($newTagLabel); @@ -171,12 +174,12 @@ class TagController extends Controller $em = $this->getDoctrine()->getManager(); $em->flush(); - } - $this->get('session')->getFlashBag()->add( - 'notice', - 'flashes.tag.notice.tag_renamed' - ); + $this->get('session')->getFlashBag()->add( + 'notice', + 'flashes.tag.notice.tag_renamed' + ); + } $redirectUrl = $this->get('wallabag_core.helper.redirect')->to($request->headers->get('referer'), '', true); diff --git a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php index 20e60c32..80903b95 100644 --- a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php @@ -211,6 +211,10 @@ class TagControllerTest extends WallabagCoreTestCase $client->submit($form, $data); $this->assertSame(302, $client->getResponse()->getStatusCode()); + $crawler = $client->followRedirect(); + + $this->assertContains('flashes.tag.notice.tag_renamed', $crawler->filter('body')->extract(['_text'])[0]); + $freshEntry = $client->getContainer() ->get('doctrine.orm.entity_manager') ->getRepository('WallabagCoreBundle:Entry') @@ -246,6 +250,60 @@ class TagControllerTest extends WallabagCoreTestCase $this->assertTrue($newTag[0]->hasEntry($freshEntry2), 'New tag is assigned to the entry2.'); } + public function testRenameTagWithSameLabel() + { + $tagLabel = 'same label'; + $this->logInAs('admin'); + $client = $this->getClient(); + + $tag = new Tag(); + $tag->setLabel($tagLabel); + + $entry = new Entry($this->getLoggedInUser()); + $entry->setUrl('http://0.0.0.0/foobar'); + $entry->addTag($tag); + $this->getEntityManager()->persist($entry); + + $this->getEntityManager()->flush(); + $this->getEntityManager()->clear(); + + // We make a first request to set an history and test redirection after tag deletion + $crawler = $client->request('GET', '/tag/list'); + $form = $crawler->filter('#tag-' . $tag->getId() . ' form')->form(); + + $data = [ + 'tag[label]' => $tagLabel, + ]; + + $client->submit($form, $data); + $this->assertSame(302, $client->getResponse()->getStatusCode()); + $this->assertNotContains('flashes.tag.notice.tag_renamed', $crawler->filter('body')->extract(['_text'])[0]); + + $freshEntry = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->find($entry->getId()); + + $tags = []; + + $tagsFromEntry = $freshEntry->getTags()->toArray(); + foreach ($tagsFromEntry as $key => $item) { + $tags[$key] = $item->getLabel(); + } + + $this->assertNotFalse(array_search($tag->getLabel(), $tags, true), 'Tag is still assigned to the entry.'); + + $newTag = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Tag') + ->findByLabel($tagLabel); + + $this->assertCount(1, $newTag); + $this->assertSame($tag->getId(), $newTag[0]->getId(), 'Tag is unchanged.'); + + $this->assertTrue($newTag[0]->hasEntry($freshEntry), 'Tag is still assigned to the entry.'); + } + public function testAddUnicodeTagLabel() { $this->logInAs('admin'); -- cgit v1.2.3 From 48f9a9632d2823be38883628ddfe62344cc282b1 Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Wed, 15 Apr 2020 22:41:03 +0200 Subject: TagController: support merging labels when renaming one with label of another Signed-off-by: Kevin Decherf --- .../CoreBundle/Controller/TagController.php | 27 +++-- .../CoreBundle/Controller/TagControllerTest.php | 127 +++++++++++++++++++++ 2 files changed, 143 insertions(+), 11 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php index f7b78f5d..16ded948 100644 --- a/src/Wallabag/CoreBundle/Controller/TagController.php +++ b/src/Wallabag/CoreBundle/Controller/TagController.php @@ -151,13 +151,21 @@ class TagController extends Controller $form = $this->createForm(RenameTagType::class, new Tag()); $form->handleRequest($request); - if ($form->isSubmitted() - && $form->isValid() - && $form->get('label')->getData() !== $tag->getLabel() - ) { - $newTagLabel = $form->get('label')->getData(); + $redirectUrl = $this->get('wallabag_core.helper.redirect')->to($request->headers->get('referer'), '', true); + + if ($form->isSubmitted() && $form->isValid()) { $newTag = new Tag(); - $newTag->setLabel($newTagLabel); + $newTag->setLabel($form->get('label')->getData()); + + if ($newTag->getLabel() === $tag->getLabel()) { + return $this->redirect($redirectUrl); + } + + $tagFromRepo = $this->get('wallabag_core.tag_repository')->findOneByLabel($newTag->getLabel()); + + if (null !== $tagFromRepo) { + $newTag = $tagFromRepo; + } $entries = $this->get('wallabag_core.entry_repository')->findAllByTagId( $this->getUser()->getId(), @@ -166,14 +174,13 @@ class TagController extends Controller foreach ($entries as $entry) { $this->get('wallabag_core.tags_assigner')->assignTagsToEntry( $entry, - $newTagLabel, + $newTag->getLabel(), [$newTag] ); $entry->removeTag($tag); } - $em = $this->getDoctrine()->getManager(); - $em->flush(); + $this->getDoctrine()->getManager()->flush(); $this->get('session')->getFlashBag()->add( 'notice', @@ -181,8 +188,6 @@ class TagController extends Controller ); } - $redirectUrl = $this->get('wallabag_core.helper.redirect')->to($request->headers->get('referer'), '', true); - return $this->redirect($redirectUrl); } } diff --git a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php index 80903b95..fb066632 100644 --- a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php @@ -304,6 +304,133 @@ class TagControllerTest extends WallabagCoreTestCase $this->assertTrue($newTag[0]->hasEntry($freshEntry), 'Tag is still assigned to the entry.'); } + public function testRenameTagWithSameLabelDifferentCase() + { + $tagLabel = 'same label'; + $newTagLabel = 'saMe labEl'; + $this->logInAs('admin'); + $client = $this->getClient(); + + $tag = new Tag(); + $tag->setLabel($tagLabel); + + $entry = new Entry($this->getLoggedInUser()); + $entry->setUrl('http://0.0.0.0/foobar'); + $entry->addTag($tag); + $this->getEntityManager()->persist($entry); + + $this->getEntityManager()->flush(); + $this->getEntityManager()->clear(); + + // We make a first request to set an history and test redirection after tag deletion + $crawler = $client->request('GET', '/tag/list'); + $form = $crawler->filter('#tag-' . $tag->getId() . ' form')->form(); + + $data = [ + 'tag[label]' => $newTagLabel, + ]; + + $client->submit($form, $data); + $this->assertSame(302, $client->getResponse()->getStatusCode()); + $this->assertNotContains('flashes.tag.notice.tag_renamed', $crawler->filter('body')->extract(['_text'])[0]); + + $freshEntry = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->find($entry->getId()); + + $tags = []; + + $tagsFromEntry = $freshEntry->getTags()->toArray(); + foreach ($tagsFromEntry as $key => $item) { + $tags[$key] = $item->getLabel(); + } + + $this->assertFalse(array_search($newTagLabel, $tags, true)); + + $tagFromRepo = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Tag') + ->findByLabel($tagLabel); + + $newTagFromRepo = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Tag') + ->findByLabel($newTagLabel); + + $this->assertCount(0, $newTagFromRepo); + $this->assertCount(1, $tagFromRepo); + + $this->assertSame($tag->getId(), $tagFromRepo[0]->getId(), 'Tag is unchanged.'); + + $this->assertTrue($tagFromRepo[0]->hasEntry($freshEntry), 'Tag is still assigned to the entry.'); + } + + public function testRenameTagWithExistingLabel() + { + $tagLabel = 'existing label'; + $previousTagLabel = 'previous label'; + $this->logInAs('admin'); + $client = $this->getClient(); + + $tag = new Tag(); + $tag->setLabel($tagLabel); + + $previousTag = new Tag(); + $previousTag->setLabel($previousTagLabel); + + $entry1 = new Entry($this->getLoggedInUser()); + $entry1->setUrl('http://0.0.0.0/foobar'); + $entry1->addTag($previousTag); + $this->getEntityManager()->persist($entry1); + + $entry2 = new Entry($this->getLoggedInUser()); + $entry2->setUrl('http://0.0.0.0/baz'); + $entry2->addTag($tag); + $this->getEntityManager()->persist($entry2); + + $this->getEntityManager()->flush(); + $this->getEntityManager()->clear(); + + // We make a first request to set an history and test redirection after tag deletion + $crawler = $client->request('GET', '/tag/list'); + $form = $crawler->filter('#tag-' . $previousTag->getId() . ' form')->form(); + + $data = [ + 'tag[label]' => $tagLabel, + ]; + + $client->submit($form, $data); + $this->assertSame(302, $client->getResponse()->getStatusCode()); + $this->assertNotContains('flashes.tag.notice.tag_renamed', $crawler->filter('body')->extract(['_text'])[0]); + + $freshEntry1 = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->find($entry1->getId()); + + $freshEntry2 = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->find($entry2->getId()); + + $tagFromRepo = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Tag') + ->findByLabel($tagLabel); + + $previousTagFromRepo = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Tag') + ->findByLabel($previousTagLabel); + + $this->assertCount(1, $tagFromRepo); + + $this->assertTrue($tagFromRepo[0]->hasEntry($freshEntry1)); + $this->assertTrue($tagFromRepo[0]->hasEntry($freshEntry2), 'Tag is assigned to the entry.'); + $this->assertFalse($previousTagFromRepo[0]->hasEntry($freshEntry1)); + } + public function testAddUnicodeTagLabel() { $this->logInAs('admin'); -- cgit v1.2.3