From 6acadf8e98cf6021a9019773df75bdb151865687 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 11:31:45 +0200 Subject: [PATCH] Rewrote code & fix tests --- .../CoreBundle/Helper/ContentProxy.php | 66 ++++++------------- .../ImportBundle/Import/AbstractImport.php | 7 +- .../Controller/TagControllerTest.php | 2 +- .../CoreBundle/Helper/ContentProxyTest.php | 37 +++++------ .../ImportBundle/Import/ChromeImportTest.php | 8 +-- .../ImportBundle/Import/FirefoxImportTest.php | 8 +-- .../Import/InstapaperImportTest.php | 8 +-- .../ImportBundle/Import/PocketImportTest.php | 10 +-- .../Import/ReadabilityImportTest.php | 8 +-- .../Import/WallabagV1ImportTest.php | 8 +-- .../Import/WallabagV2ImportTest.php | 10 +-- 11 files changed, 71 insertions(+), 101 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index e6292744..3024fdc5 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -7,7 +7,6 @@ use Psr\Log\LoggerInterface; use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Tools\Utils; use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser; -use Symfony\Component\Config\Definition\Exception\Exception; /** * This kind of proxy class take care of getting the content from an url @@ -32,57 +31,40 @@ class ContentProxy } /** - * Update existing entry by fetching from URL using Graby. + * Update entry using either fetched or provided content. * - * @param Entry $entry Entry to update - * @param string $url Url to grab content for + * @param Entry $entry Entry to update + * @param string $url Url of the content + * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url + * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby */ - public function updateEntry(Entry $entry, $url) + public function updateEntry(Entry $entry, $url, array $content = [], $disableContentUpdate = false) { - $content = $this->graby->fetchContent($url); - - // be sure to keep the url in case of error - // so we'll be able to refetch it in the future - $content['url'] = $content['url'] ?: $url; - - $this->stockEntry($entry, $content); - } - - /** - * Import entry using either fetched or provided content. - * - * @param Entry $entry Entry to update - * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url - * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby - */ - public function importEntry(Entry $entry, array $content, $disableContentUpdate = false) - { - try { - $this->validateContent($content); - } catch (\Exception $e) { - // validation failed but do we want to disable updating content? - if (true === $disableContentUpdate) { - throw $e; - } + if (!empty($content['html'])) { + $content['html'] = $this->graby->cleanupHtml($content['html'], $url); } - if (false === $disableContentUpdate) { + if ((empty($content) || false === $this->validateContent($content)) && false === $disableContentUpdate) { try { - $fetchedContent = $this->graby->fetchContent($content['url']); + $fetchedContent = $this->graby->fetchContent($url); } catch (\Exception $e) { $this->logger->error('Error while trying to fetch content from URL.', [ - 'entry_url' => $content['url'], + 'entry_url' => $url, 'error_msg' => $e->getMessage(), ]); } // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them - if ($fetchedContent['html'] !== $this->fetchingErrorMessage) { + if (empty($content) || $fetchedContent['html'] !== $this->fetchingErrorMessage) { $content = $fetchedContent; } } + // be sure to keep the url in case of error + // so we'll be able to refetch it in the future + $content['url'] = !empty($content['url']) ? $content['url'] : $url; + $this->stockEntry($entry, $content); } @@ -126,7 +108,7 @@ class ContentProxy try { $entry->setPublishedAt(new \DateTime($date)); } catch (\Exception $e) { - $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $url, 'date' => $content['date']]); + $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $content['url'], 'date' => $content['date']]); } } @@ -170,19 +152,11 @@ class ContentProxy * Validate that the given content has at least a title, an html and a url. * * @param array $content + * + * @return bool true if valid otherwise false */ private function validateContent(array $content) { - if (empty($content['title'])) { - throw new Exception('Missing title from imported entry!'); - } - - if (empty($content['url'])) { - throw new Exception('Missing URL from imported entry!'); - } - - if (empty($content['html'])) { - throw new Exception('Missing html from imported entry!'); - } + return !empty($content['title']) && !empty($content['html']) && !empty($content['url']); } } diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index bf568a1a..9f3d822a 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -115,14 +115,11 @@ abstract class AbstractImport implements ImportInterface */ protected function fetchContent(Entry $entry, $url, array $content = []) { - // be sure to set at least the given url - $content['url'] = isset($content['url']) ? $content['url'] : $url; - try { - $this->contentProxy->importEntry($entry, $content, $this->disableContentUpdate); + $this->contentProxy->updateEntry($entry, $url, $content, $this->disableContentUpdate); } catch (\Exception $e) { $this->logger->error('Error trying to import an entry.', [ - 'entry_url' => $content['url'], + 'entry_url' => $url, 'error_msg' => $e->getMessage(), ]); } diff --git a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php index f9bf7b87..af1ad7af 100644 --- a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php @@ -123,7 +123,7 @@ class TagControllerTest extends WallabagCoreTestCase $this->assertEquals(302, $client->getResponse()->getStatusCode()); $this->assertEquals($entryUri, $client->getResponse()->getTargetUrl()); - // re-retrieve the entry to be sure to get fresh data from database (mostly for tags) + // re-retrieve the entry to be sure to get fresh data from database (mostly for tags) $entry = $this->getEntityManager()->getRepository(Entry::class)->find($entry->getId()); $this->assertNotContains($this->tagName, $entry->getTags()); diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index be287d84..a3570125 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -11,8 +11,6 @@ use Wallabag\CoreBundle\Entity\Tag; use Wallabag\UserBundle\Entity\User; use Wallabag\CoreBundle\Helper\RuleBasedTagger; use Graby\Graby; -use Monolog\Handler\TestHandler; -use Monolog\Logger; class ContentProxyTest extends \PHPUnit_Framework_TestCase { @@ -259,12 +257,13 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $logHandler = new TestHandler(); - $logger = new Logger('test', array($logHandler)); + $logger = new Logger('test', [$logHandler]); $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage); $entry = new Entry(new User()); - $proxy->importEntry( + $proxy->updateEntry( $entry, + 'http://1.1.1.1', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -299,6 +298,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $entry = new Entry(new User()); $proxy->updateEntry( $entry, + 'http://1.1.1.1', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -326,26 +326,24 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase public function testTaggerThrowException() { - $graby = $this->getMockBuilder('Graby\Graby') - ->disableOriginalConstructor() - ->getMock(); - $tagger = $this->getTaggerMock(); $tagger->expects($this->once()) ->method('tag') ->will($this->throwException(new \Exception())); - $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - + $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); $entry = new Entry(new User()); - $content = array( - 'html' => str_repeat('this is my content', 325), - 'title' => 'this is my title', - 'url' => 'http://1.1.1.1', - 'content_type' => 'text/html', - 'language' => 'fr', + $proxy->updateEntry( + $entry, + 'http://1.1.1.1', + [ + 'html' => str_repeat('this is my content', 325), + 'title' => 'this is my title', + 'url' => 'http://1.1.1.1', + 'content_type' => 'text/html', + 'language' => 'fr', + ] ); - $proxy->importEntry($entry, $content, true); $this->assertCount(0, $entry->getTags()); } @@ -374,8 +372,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry( - new Entry(new User()), + $entry = new Entry(new User()); + $proxy->updateEntry( + $entry, 'http://1.1.1.1', [ 'html' => $html, diff --git a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php index 7a15e918..cec19534 100644 --- a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php @@ -89,7 +89,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $chromeImport->import(); @@ -118,7 +118,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php index 09abac57..c186c820 100644 --- a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php @@ -89,7 +89,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $firefoxImport->import(); @@ -118,7 +118,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php index 05844490..9158c8a2 100644 --- a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php @@ -104,7 +104,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(4)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $instapaperImport->import(); @@ -133,7 +133,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -173,7 +173,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -213,7 +213,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index f75e6bea..b81ebe15 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -282,7 +282,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -377,7 +377,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -450,7 +450,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -536,7 +536,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('ImportEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -621,7 +621,7 @@ JSON; $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->will($this->throwException(new \Exception())); $pocketImport->setClient($client); diff --git a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php index 1b0daa92..8f466d38 100644 --- a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php @@ -89,7 +89,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $readabilityImport->import(); @@ -118,7 +118,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php index 3b2375a1..834b7ef5 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php @@ -113,7 +113,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $wallabagV1Import->import(); @@ -142,7 +142,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -182,7 +182,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -222,7 +222,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index e1acf569..5cc04aa5 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -100,7 +100,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); $res = $wallabagV2Import->import(); @@ -129,7 +129,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -165,7 +165,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -201,7 +201,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -278,7 +278,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->will($this->throwException(new \Exception())); $res = $wallabagV2Import->import(); -- 2.41.0