From 5c4993832ef8708b09d5e010179b1ee8b1e4b6de Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Wed, 2 Mar 2022 19:11:32 +0100 Subject: [PATCH 1/2] Fix tagging rule match when user a custom reading speed By default, we assume the reading speed is 200 word per minute (WPM) when we save an entry. User can change that value in the config and the rendering is properly performed with the user reading speed. BUT, when the matching rule is applied, it uses the default reading time defined in the entry without applying the custom reading speed of the user. This should fix that bug. Also update the `wallabag:tag:all` to fix the bug when tagging all entries. --- .../CoreBundle/Command/TagAllCommand.php | 2 +- .../DataFixtures/ConfigFixtures.php | 2 +- .../DataFixtures/TaggingRuleFixtures.php | 14 +++++++ .../CoreBundle/Helper/RuleBasedTagger.php | 37 +++++++++++++++---- .../Controller/EntryControllerTest.php | 36 ++++++++++++++++++ 5 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/Wallabag/CoreBundle/Command/TagAllCommand.php b/src/Wallabag/CoreBundle/Command/TagAllCommand.php index 4afac2d45..666654c02 100644 --- a/src/Wallabag/CoreBundle/Command/TagAllCommand.php +++ b/src/Wallabag/CoreBundle/Command/TagAllCommand.php @@ -41,7 +41,7 @@ class TagAllCommand extends ContainerAwareCommand $entries = $tagger->tagAllForUser($user); - $io->text('Persist entries... '); + $io->text('Persist ' . \count($entries) . ' entries... '); $em = $this->getDoctrine()->getManager(); foreach ($entries as $entry) { diff --git a/src/Wallabag/CoreBundle/DataFixtures/ConfigFixtures.php b/src/Wallabag/CoreBundle/DataFixtures/ConfigFixtures.php index d76ba514d..6dde08e81 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/ConfigFixtures.php +++ b/src/Wallabag/CoreBundle/DataFixtures/ConfigFixtures.php @@ -45,7 +45,7 @@ class ConfigFixtures extends Fixture implements DependentFixtureInterface $emptyConfig = new Config($this->getReference('empty-user')); $emptyConfig->setTheme('material'); $emptyConfig->setItemsPerPage(10); - $emptyConfig->setReadingSpeed(200); + $emptyConfig->setReadingSpeed(100); $emptyConfig->setLanguage('en'); $emptyConfig->setPocketConsumerKey(null); $emptyConfig->setActionMarkAsRead(0); diff --git a/src/Wallabag/CoreBundle/DataFixtures/TaggingRuleFixtures.php b/src/Wallabag/CoreBundle/DataFixtures/TaggingRuleFixtures.php index b36224119..083c7bc98 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/TaggingRuleFixtures.php +++ b/src/Wallabag/CoreBundle/DataFixtures/TaggingRuleFixtures.php @@ -43,6 +43,20 @@ class TaggingRuleFixtures extends Fixture implements DependentFixtureInterface $manager->persist($tr4); + $tr5 = new TaggingRule(); + $tr5->setRule('readingTime <= 5'); + $tr5->setTags(['shortread']); + $tr5->setConfig($this->getReference('empty-config')); + + $manager->persist($tr5); + + $tr6 = new TaggingRule(); + $tr6->setRule('readingTime > 5'); + $tr6->setTags(['longread']); + $tr6->setConfig($this->getReference('empty-config')); + + $manager->persist($tr6); + $manager->flush(); } diff --git a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php index 0b18beaaa..4014c68e7 100644 --- a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php +++ b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php @@ -35,8 +35,10 @@ class RuleBasedTagger { $rules = $this->getRulesForUser($entry->getUser()); + $clonedEntry = $this->fixEntry($entry); + foreach ($rules as $rule) { - if (!$this->rulerz->satisfies($entry, $rule->getRule())) { + if (!$this->rulerz->satisfies($clonedEntry, $rule->getRule())) { continue; } @@ -61,14 +63,22 @@ class RuleBasedTagger public function tagAllForUser(User $user) { $rules = $this->getRulesForUser($user); - $entries = []; + $entriesToUpdate = []; $tagsCache = []; - foreach ($rules as $rule) { - $qb = $this->entryRepository->getBuilderForAllByUser($user->getId()); - $entries = $this->rulerz->filter($qb, $rule->getRule()); + $entries = $this->entryRepository + ->getBuilderForAllByUser($user->getId()) + ->getQuery() + ->getResult(); + + foreach ($entries as $entry) { + $clonedEntry = $this->fixEntry($entry); + + foreach ($rules as $rule) { + if (!$this->rulerz->satisfies($clonedEntry, $rule->getRule())) { + continue; + } - foreach ($entries as $entry) { foreach ($rule->getTags() as $label) { // avoid new tag duplicate by manually caching them if (!isset($tagsCache[$label])) { @@ -78,11 +88,13 @@ class RuleBasedTagger $tag = $tagsCache[$label]; $entry->addTag($tag); + + $entriesToUpdate[] = $entry; } } } - return $entries; + return $entriesToUpdate; } /** @@ -114,4 +126,15 @@ class RuleBasedTagger { return $user->getConfig()->getTaggingRules(); } + + /** + * Update reading time on the fly to match the proper words per minute from the user. + */ + private function fixEntry(Entry $entry) + { + $clonedEntry = clone $entry; + $clonedEntry->setReadingTime($entry->getReadingTime() / $entry->getUser()->getConfig()->getReadingSpeed() * 200); + + return $clonedEntry; + } } diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 4820fad09..13ab7aca2 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -175,6 +175,42 @@ class EntryControllerTest extends WallabagCoreTestCase $client->getContainer()->get('craue_config')->set('store_article_headers', 0); } + /** + * @group NetworkCalls + */ + public function testPostNewOkWithTaggingRules() + { + $this->logInAs('empty'); + $client = $this->getClient(); + + $crawler = $client->request('GET', '/new'); + + $this->assertSame(200, $client->getResponse()->getStatusCode()); + + $form = $crawler->filter('form[name=entry]')->form(); + + $data = [ + 'entry[url]' => $this->url, + ]; + + $client->submit($form, $data); + + $this->assertSame(302, $client->getResponse()->getStatusCode()); + + $content = $client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->findByUrlAndUserId($this->url, $this->getLoggedInUserId()); + + $tags = $content->getTagsLabel(); + + /* + * Without the custom reading speed of `empty` user, it'll be inversed + */ + $this->assertContains('longread', $tags); + $this->assertNotContains('shortread', $tags); + } + /** * @group NetworkCalls */ From 10d071a4f29c7bf23fc6235fe4fdeff27c18df47 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Wed, 2 Mar 2022 19:28:48 +0100 Subject: [PATCH 2/2] Fix tests --- .../CoreBundle/Helper/RuleBasedTaggerTest.php | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/Wallabag/CoreBundle/Helper/RuleBasedTaggerTest.php b/tests/Wallabag/CoreBundle/Helper/RuleBasedTaggerTest.php index 44d42a980..f85b8d07b 100644 --- a/tests/Wallabag/CoreBundle/Helper/RuleBasedTaggerTest.php +++ b/tests/Wallabag/CoreBundle/Helper/RuleBasedTaggerTest.php @@ -202,10 +202,29 @@ class RuleBasedTaggerTest extends TestCase ->method('satisfies') ->willReturn(true); - $this->rulerz - ->method('filter') + $query = $this->getMockBuilder('Doctrine\ORM\AbstractQuery') + ->disableOriginalConstructor() + ->getMock(); + + $query + ->expects($this->once()) + ->method('getResult') ->willReturn([new Entry($user), new Entry($user)]); + $qb = $this->getMockBuilder('Doctrine\ORM\QueryBuilder') + ->disableOriginalConstructor() + ->getMock(); + + $qb + ->expects($this->once()) + ->method('getQuery') + ->willReturn($query); + + $this->entryRepository + ->expects($this->once()) + ->method('getBuilderForAllByUser') + ->willReturn($qb); + $entries = $this->tagger->tagAllForUser($user); $this->assertCount(2, $entries); @@ -222,6 +241,7 @@ class RuleBasedTaggerTest extends TestCase { $user = new User(); $config = new Config($user); + $config->setReadingSpeed(200); $user->setConfig($config);