From c2656f96d4776c86b13d8a4c93a78ee7c4d3824c Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Fri, 19 Feb 2016 14:22:20 +0100 Subject: Move assignTagsToEntry in ContentProxy helper --- .../Controller/WallabagRestController.php | 31 +----- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 40 +++++++- .../CoreBundle/Resources/config/services.yml | 1 + .../CoreBundle/Tests/Helper/ContentProxyTest.php | 106 ++++++++++++++++++++- src/Wallabag/ImportBundle/Import/PocketImport.php | 26 +---- .../ImportBundle/Import/WallabagV1Import.php | 28 ++---- .../ImportBundle/Tests/Import/PocketImportTest.php | 18 +--- 7 files changed, 158 insertions(+), 92 deletions(-) diff --git a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php index 84bc14a9..03990088 100644 --- a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php +++ b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php @@ -15,31 +15,6 @@ use Wallabag\CoreBundle\Entity\Tag; class WallabagRestController extends FOSRestController { - /** - * @param Entry $entry - * @param string $tags - */ - private function assignTagsToEntry(Entry $entry, $tags) - { - foreach (explode(',', $tags) as $label) { - $label = trim($label); - $tagEntity = $this - ->getDoctrine() - ->getRepository('WallabagCoreBundle:Tag') - ->findOneByLabel($label); - - if (is_null($tagEntity)) { - $tagEntity = new Tag(); - $tagEntity->setLabel($label); - } - - // only add the tag on the entry if the relation doesn't exist - if (!$entry->getTags()->contains($tagEntity)) { - $entry->addTag($tagEntity); - } - } - } - private function validateAuthentication() { if (false === $this->get('security.authorization_checker')->isGranted('IS_AUTHENTICATED_FULLY')) { @@ -140,7 +115,7 @@ class WallabagRestController extends FOSRestController $tags = $request->request->get('tags', ''); if (!empty($tags)) { - $this->assignTagsToEntry($entry, $tags); + $this->get('wallabag_core.content_proxy')->assignTagsToEntry($entry, $tags); } $em = $this->getDoctrine()->getManager(); @@ -192,7 +167,7 @@ class WallabagRestController extends FOSRestController $tags = $request->request->get('tags', ''); if (!empty($tags)) { - $this->assignTagsToEntry($entry, $tags); + $this->get('wallabag_core.content_proxy')->assignTagsToEntry($entry, $tags); } $em = $this->getDoctrine()->getManager(); @@ -270,7 +245,7 @@ class WallabagRestController extends FOSRestController $tags = $request->request->get('tags', ''); if (!empty($tags)) { - $this->assignTagsToEntry($entry, $tags); + $this->get('wallabag_core.content_proxy')->assignTagsToEntry($entry, $tags); } $em = $this->getDoctrine()->getManager(); diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index bd8b993a..ba90b731 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -5,7 +5,9 @@ namespace Wallabag\CoreBundle\Helper; use Graby\Graby; use Psr\Log\LoggerInterface as Logger; use Wallabag\CoreBundle\Entity\Entry; +use Wallabag\CoreBundle\Entity\Tag; use Wallabag\CoreBundle\Tools\Utils; +use Wallabag\CoreBundle\Repository\TagRepository; /** * This kind of proxy class take care of getting the content from an url @@ -16,12 +18,14 @@ class ContentProxy protected $graby; protected $tagger; protected $logger; + protected $tagRepository; - public function __construct(Graby $graby, RuleBasedTagger $tagger, Logger $logger) + public function __construct(Graby $graby, RuleBasedTagger $tagger, TagRepository $tagRepository, Logger $logger) { $this->graby = $graby; $this->tagger = $tagger; $this->logger = $logger; + $this->tagRepository = $tagRepository; } /** @@ -75,4 +79,38 @@ class ContentProxy return $entry; } + + /** + * Assign some tags to an entry. + * + * @param Entry $entry + * @param array|string $tags An array of tag or a string coma separated of tag + */ + public function assignTagsToEntry(Entry $entry, $tags) + { + if (!is_array($tags)) { + $tags = explode(',', $tags); + } + + foreach ($tags as $label) { + $label = trim($label); + + // avoid empty tag + if (0 === strlen($label)) { + continue; + } + + $tagEntity = $this->tagRepository->findOneByLabel($label); + + if (is_null($tagEntity)) { + $tagEntity = new Tag(); + $tagEntity->setLabel($label); + } + + // only add the tag on the entry if the relation doesn't exist + if (false === $entry->getTags()->contains($tagEntity)) { + $entry->addTag($tagEntity); + } + } + } } diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml index a8796fe4..1aa66be1 100644 --- a/src/Wallabag/CoreBundle/Resources/config/services.yml +++ b/src/Wallabag/CoreBundle/Resources/config/services.yml @@ -50,6 +50,7 @@ services: arguments: - "@wallabag_core.graby" - "@wallabag_core.rule_based_tagger" + - "@wallabag_core.tag_repository" - "@logger" wallabag_core.rule_based_tagger: diff --git a/src/Wallabag/CoreBundle/Tests/Helper/ContentProxyTest.php b/src/Wallabag/CoreBundle/Tests/Helper/ContentProxyTest.php index d29984e9..f58b5828 100644 --- a/src/Wallabag/CoreBundle/Tests/Helper/ContentProxyTest.php +++ b/src/Wallabag/CoreBundle/Tests/Helper/ContentProxyTest.php @@ -3,8 +3,9 @@ namespace Wallabag\CoreBundle\Tests\Helper; use Psr\Log\NullLogger; -use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Helper\ContentProxy; +use Wallabag\CoreBundle\Entity\Entry; +use Wallabag\CoreBundle\Entity\Tag; use Wallabag\UserBundle\Entity\User; class ContentProxyTest extends \PHPUnit_Framework_TestCase @@ -30,7 +31,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase 'language' => '', )); - $proxy = new ContentProxy($graby, $tagger, $this->getLogger()); + $proxy = new ContentProxy($graby, $tagger, $this->getTagRepositoryMock(), $this->getLogger()); $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0'); $this->assertEquals('http://0.0.0.0', $entry->getUrl()); @@ -68,7 +69,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ), )); - $proxy = new ContentProxy($graby, $tagger, $this->getLogger()); + $proxy = new ContentProxy($graby, $tagger, $this->getTagRepositoryMock(), $this->getLogger()); $entry = $proxy->updateEntry(new Entry(new User()), 'http://domain.io'); $this->assertEquals('http://domain.io', $entry->getUrl()); @@ -107,7 +108,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ), )); - $proxy = new ContentProxy($graby, $tagger, $this->getLogger()); + $proxy = new ContentProxy($graby, $tagger, $this->getTagRepositoryMock(), $this->getLogger()); $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0'); $this->assertEquals('http://1.1.1.1', $entry->getUrl()); @@ -120,6 +121,96 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $this->assertEquals('1.1.1.1', $entry->getDomainName()); } + public function testAssignTagsWithArrayAndExtraSpaces() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $entry = new Entry(new User()); + + $proxy->assignTagsToEntry($entry, array(' tag1', 'tag2 ')); + + $this->assertCount(2, $entry->getTags()); + $this->assertEquals('tag1', $entry->getTags()[0]->getLabel()); + $this->assertEquals('tag2', $entry->getTags()[1]->getLabel()); + } + + public function testAssignTagsWithString() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $entry = new Entry(new User()); + + $proxy->assignTagsToEntry($entry, 'tag1, tag2'); + + $this->assertCount(2, $entry->getTags()); + $this->assertEquals('tag1', $entry->getTags()[0]->getLabel()); + $this->assertEquals('tag2', $entry->getTags()[1]->getLabel()); + } + + public function testAssignTagsWithEmptyArray() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $entry = new Entry(new User()); + + $proxy->assignTagsToEntry($entry, array()); + + $this->assertCount(0, $entry->getTags()); + } + + public function testAssignTagsWithEmptyString() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $entry = new Entry(new User()); + + $proxy->assignTagsToEntry($entry, ''); + + $this->assertCount(0, $entry->getTags()); + } + + public function testAssignTagsAlreadyAssigned() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $tagEntity = new Tag(); + $tagEntity->setLabel('tag1'); + + $entry = new Entry(new User()); + $entry->addTag($tagEntity); + + $proxy->assignTagsToEntry($entry, 'tag1, tag2'); + + $this->assertCount(2, $entry->getTags()); + $this->assertEquals('tag1', $entry->getTags()[0]->getLabel()); + $this->assertEquals('tag2', $entry->getTags()[1]->getLabel()); + } + private function getTaggerMock() { return $this->getMockBuilder('Wallabag\CoreBundle\Helper\RuleBasedTagger') @@ -128,6 +219,13 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->getMock(); } + private function getTagRepositoryMock() + { + return $this->getMockBuilder('Wallabag\CoreBundle\Repository\TagRepository') + ->disableOriginalConstructor() + ->getMock(); + } + private function getLogger() { return new NullLogger(); diff --git a/src/Wallabag/ImportBundle/Import/PocketImport.php b/src/Wallabag/ImportBundle/Import/PocketImport.php index 22932238..5dfd098c 100644 --- a/src/Wallabag/ImportBundle/Import/PocketImport.php +++ b/src/Wallabag/ImportBundle/Import/PocketImport.php @@ -9,7 +9,6 @@ use GuzzleHttp\Client; use GuzzleHttp\Exception\RequestException; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Wallabag\CoreBundle\Entity\Entry; -use Wallabag\CoreBundle\Entity\Tag; use Wallabag\CoreBundle\Helper\ContentProxy; use Craue\ConfigBundle\Util\Config; @@ -177,26 +176,6 @@ class PocketImport implements ImportInterface $this->client = $client; } - private function assignTagsToEntry(Entry $entry, $tags) - { - foreach ($tags as $tag) { - $label = trim($tag['tag']); - $tagEntity = $this->em - ->getRepository('WallabagCoreBundle:Tag') - ->findOneByLabel($label); - - if (is_object($tagEntity)) { - $entry->addTag($tagEntity); - } else { - $newTag = new Tag(); - $newTag->setLabel($label); - - $entry->addTag($newTag); - } - $this->em->flush(); - } - } - /** * @see https://getpocket.com/developer/docs/v3/retrieve * @@ -246,7 +225,10 @@ class PocketImport implements ImportInterface } if (isset($pocketEntry['tags']) && !empty($pocketEntry['tags'])) { - $this->assignTagsToEntry($entry, $pocketEntry['tags']); + $this->contentProxy->assignTagsToEntry( + $entry, + array_keys($pocketEntry['tags']) + ); } $this->em->persist($entry); diff --git a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php index bbac6eaf..05bdb401 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php +++ b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php @@ -6,7 +6,6 @@ use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Doctrine\ORM\EntityManager; use Wallabag\CoreBundle\Entity\Entry; -use Wallabag\CoreBundle\Entity\Tag; use Wallabag\UserBundle\Entity\User; use Wallabag\CoreBundle\Tools\Utils; use Wallabag\CoreBundle\Helper\ContentProxy; @@ -144,6 +143,7 @@ class WallabagV1Import implements ImportInterface // @see ContentProxy->updateEntry $entry = new Entry($this->user); $entry->setUrl($importedEntry['url']); + if (in_array($importedEntry['title'], $untitled)) { $entry = $this->contentProxy->updateEntry($entry, $importedEntry['url']); } else { @@ -152,10 +152,14 @@ class WallabagV1Import implements ImportInterface $entry->setReadingTime(Utils::getReadingTime($importedEntry['content'])); $entry->setDomainName(parse_url($importedEntry['url'], PHP_URL_HOST)); } + if (array_key_exists('tags', $importedEntry) && $importedEntry['tags'] != '') { - $tags = explode(',', $importedEntry['tags']); - $this->assignTagsToEntry($entry, $tags); + $this->contentProxy->assignTagsToEntry( + $entry, + $importedEntry['tags'] + ); } + $entry->setArchived($importedEntry['is_read']); $entry->setStarred($importedEntry['is_fav']); @@ -171,22 +175,4 @@ class WallabagV1Import implements ImportInterface $this->em->flush(); } - - private function assignTagsToEntry(Entry $entry, $tags) - { - foreach ($tags as $tag) { - $label = trim($tag); - $tagEntity = $this->em - ->getRepository('WallabagCoreBundle:Tag') - ->findOneByLabel($label); - if (is_object($tagEntity)) { - $entry->addTag($tagEntity); - } else { - $newTag = new Tag(); - $newTag->setLabel($label); - $entry->addTag($newTag); - } - $this->em->flush(); - } - } } diff --git a/src/Wallabag/ImportBundle/Tests/Import/PocketImportTest.php b/src/Wallabag/ImportBundle/Tests/Import/PocketImportTest.php index 25359d56..f44786b1 100644 --- a/src/Wallabag/ImportBundle/Tests/Import/PocketImportTest.php +++ b/src/Wallabag/ImportBundle/Tests/Import/PocketImportTest.php @@ -260,24 +260,10 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase ->method('findByUrlAndUserId') ->will($this->onConsecutiveCalls(false, true)); - $tag = $this->getMockBuilder('Wallabag\CoreBundle\Entity\Tag') - ->disableOriginalConstructor() - ->getMock(); - - $tagRepo = $this->getMockBuilder('Wallabag\CoreBundle\Repository\TagRepository') - ->disableOriginalConstructor() - ->getMock(); - - $tagRepo->expects($this->exactly(2)) - // the method `findOneByLabel` doesn't exist, EntityRepository will then call `_call` method - // to magically call the `findOneBy` with ['label' => 'foo'] - ->method('__call') - ->will($this->onConsecutiveCalls(false, $tag)); - $this->em - ->expects($this->any()) + ->expects($this->exactly(2)) ->method('getRepository') - ->will($this->onConsecutiveCalls($entryRepo, $tagRepo, $tagRepo, $entryRepo)); + ->willReturn($entryRepo); $entry = $this->getMockBuilder('Wallabag\CoreBundle\Entity\Entry') ->disableOriginalConstructor() -- cgit v1.2.3