From fc031e5706acf89ff21f22ca8004ddc7f9b43089 Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Sun, 27 Dec 2015 22:26:49 +0100
Subject: [PATCH 1/2] Avoid multiple tag creation

When a new tag is created but not yet persisted, it can be duplicated.
It could happen when multiple rules match the content and at least 2 of them should attach same new tag.

Fix #1528
---
 .../DataFixtures/ORM/LoadTaggingRuleData.php  | 41 +++++++++++++++++++
 src/Wallabag/CoreBundle/Entity/Entry.php      |  8 ++++
 .../Tests/Helper/RuleBasedTaggerTest.php      | 20 +++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTaggingRuleData.php

diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTaggingRuleData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTaggingRuleData.php
new file mode 100644
index 000000000..09a08bb20
--- /dev/null
+++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTaggingRuleData.php
@@ -0,0 +1,41 @@
+<?php
+
+namespace Wallabag\CoreBundle\DataFixtures\ORM;
+
+use Doctrine\Common\DataFixtures\AbstractFixture;
+use Doctrine\Common\DataFixtures\OrderedFixtureInterface;
+use Doctrine\Common\Persistence\ObjectManager;
+use Wallabag\CoreBundle\Entity\TaggingRule;
+
+class LoadTaggingRuleData extends AbstractFixture implements OrderedFixtureInterface
+{
+    /**
+     * {@inheritdoc}
+     */
+    public function load(ObjectManager $manager)
+    {
+        $tr1 = new TaggingRule();
+        $tr1->setRule('content matches "spurs"');
+        $tr1->setTags(array('sport'));
+        $tr1->setConfig($this->getReference('admin-config'));
+
+        $manager->persist($tr1);
+
+        $tr2 = new TaggingRule();
+        $tr2->setRule('content matches "basket"');
+        $tr2->setTags(array('sport'));
+        $tr2->setConfig($this->getReference('admin-config'));
+
+        $manager->persist($tr2);
+
+        $manager->flush();
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getOrder()
+    {
+        return 40;
+    }
+}
diff --git a/src/Wallabag/CoreBundle/Entity/Entry.php b/src/Wallabag/CoreBundle/Entity/Entry.php
index 608ed2f0d..2813c9442 100644
--- a/src/Wallabag/CoreBundle/Entity/Entry.php
+++ b/src/Wallabag/CoreBundle/Entity/Entry.php
@@ -462,6 +462,14 @@ class Entry
             return;
         }
 
+        // check if tag already exist but has not yet be persisted
+        // it seems that the previous condition with `contains()` doesn't check that case
+        foreach ($this->tags as $existingTag) {
+            if ($existingTag->getUser() !== $tag->getUser() || $existingTag->getLabel() === $tag->getLabel()) {
+                return;
+            }
+        }
+
         $this->tags[] = $tag;
         $tag->addEntry($this);
     }
diff --git a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
index 37e137bfc..70951d46f 100644
--- a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
+++ b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
@@ -121,6 +121,26 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $this->assertSame($tag, $tags[0]);
     }
 
+    public function testSameTagWithDifferentfMatchingRules()
+    {
+        $taggingRule = $this->getTaggingRule('bla bla', array('hey'));
+        $otherTaggingRule = $this->getTaggingRule('rule as string', array('hey'));
+
+        $user = $this->getUser([$taggingRule, $otherTaggingRule]);
+        $entry = new Entry($user);
+
+        $this->rulerz
+            ->method('satisfies')
+            ->willReturn(true);
+
+        $this->tagger->tag($entry);
+
+        $this->assertFalse($entry->getTags()->isEmpty());
+
+        $tags = $entry->getTags();
+        $this->assertCount(1, $tags);
+    }
+
     private function getUser(array $taggingRules = [])
     {
         $user = new User();

From e9fa8c40aaf1c4fc356057bc7b248ce80c0766b0 Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Sun, 27 Dec 2015 23:48:57 +0100
Subject: [PATCH 2/2] Add test on tagAllForUser

And fix multiplication of entries returned by `tagAllForUser`.
---
 .../CoreBundle/Helper/RuleBasedTagger.php     |  1 -
 .../Tests/Helper/RuleBasedTaggerTest.php      | 26 +++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
index fb2d1f87d..41ef25b8c 100644
--- a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
+++ b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
@@ -65,7 +65,6 @@ class RuleBasedTagger
                     $tag = $this->getTag($user, $label);
 
                     $entry->addTag($tag);
-                    $entries[] = $entry;
                 }
             }
         }
diff --git a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
index 70951d46f..1de134b8a 100644
--- a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
+++ b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
@@ -141,6 +141,32 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $this->assertCount(1, $tags);
     }
 
+    public function testTagAllEntriesForAUser()
+    {
+        $taggingRule = $this->getTaggingRule('bla bla', array('hey'));
+
+        $user = $this->getUser([$taggingRule]);
+
+        $this->rulerz
+            ->method('satisfies')
+            ->willReturn(true);
+
+        $this->rulerz
+            ->method('filter')
+            ->willReturn(array(new Entry($user), new Entry($user)));
+
+        $entries = $this->tagger->tagAllForUser($user);
+
+        $this->assertCount(2, $entries);
+
+        foreach ($entries as $entry) {
+            $tags = $entry->getTags();
+
+            $this->assertCount(1, $tags);
+            $this->assertEquals('hey', $tags[0]->getLabel());
+        }
+    }
+
     private function getUser(array $taggingRules = [])
     {
         $user = new User();