From fc73222723c7a0c9b577805d3ef51eb96b124b92 Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Tue, 29 Dec 2015 14:50:52 +0100
Subject: Remove user reference in tag

Fix #1543
---
 .../CoreBundle/Controller/TagController.php        | 12 +++----
 .../CoreBundle/DataFixtures/ORM/LoadEntryData.php  | 19 +++-------
 .../CoreBundle/DataFixtures/ORM/LoadTagData.php    | 41 ++++++++++++++++++++++
 src/Wallabag/CoreBundle/Entity/Entry.php           |  2 +-
 src/Wallabag/CoreBundle/Entity/Tag.php             | 17 ++-------
 src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php | 13 ++++---
 .../CoreBundle/Repository/EntryRepository.php      | 19 ++++++++++
 .../CoreBundle/Repository/TagRepository.php        | 36 ++++++++++++-------
 .../Tests/Controller/ConfigControllerTest.php      |  4 +--
 .../Tests/Helper/RuleBasedTaggerTest.php           | 12 +++----
 10 files changed, 109 insertions(+), 66 deletions(-)
 create mode 100644 src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php

(limited to 'src/Wallabag/CoreBundle')

diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php
index fd2069e0..64d53f0c 100644
--- a/src/Wallabag/CoreBundle/Controller/TagController.php
+++ b/src/Wallabag/CoreBundle/Controller/TagController.php
@@ -20,25 +20,23 @@ class TagController extends Controller
      */
     public function addTagFormAction(Request $request, Entry $entry)
     {
-        $tag = new Tag($this->getUser());
+        $tag = new Tag();
         $form = $this->createForm(new NewTagType(), $tag);
         $form->handleRequest($request);
 
         if ($form->isValid()) {
             $existingTag = $this->getDoctrine()
                 ->getRepository('WallabagCoreBundle:Tag')
-                ->findOneByLabelAndUserId($tag->getLabel(), $this->getUser()->getId());
+                ->findOneByLabel($tag->getLabel());
 
             $em = $this->getDoctrine()->getManager();
 
             if (is_null($existingTag)) {
                 $entry->addTag($tag);
                 $em->persist($tag);
-            } else {
-                if (!$existingTag->hasEntry($entry)) {
-                    $entry->addTag($existingTag);
-                    $em->persist($existingTag);
-                }
+            } elseif (!$existingTag->hasEntry($entry)) {
+                $entry->addTag($existingTag);
+                $em->persist($existingTag);
             }
 
             $em->flush();
diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php
index 0513cdb8..6c6a331a 100644
--- a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php
+++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php
@@ -6,7 +6,6 @@ use Doctrine\Common\DataFixtures\AbstractFixture;
 use Doctrine\Common\DataFixtures\OrderedFixtureInterface;
 use Doctrine\Common\Persistence\ObjectManager;
 use Wallabag\CoreBundle\Entity\Entry;
-use Wallabag\CoreBundle\Entity\Tag;
 
 class LoadEntryData extends AbstractFixture implements OrderedFixtureInterface
 {
@@ -50,13 +49,8 @@ class LoadEntryData extends AbstractFixture implements OrderedFixtureInterface
         $entry3->setContent('This is my content /o/');
         $entry3->setLanguage('en');
 
-        $tag1 = new Tag($this->getReference('bob-user'));
-        $tag1->setLabel('foo');
-        $tag2 = new Tag($this->getReference('bob-user'));
-        $tag2->setLabel('bar');
-
-        $entry3->addTag($tag1);
-        $entry3->addTag($tag2);
+        $entry3->addTag($this->getReference('foo-tag'));
+        $entry3->addTag($this->getReference('bar-tag'));
 
         $manager->persist($entry3);
 
@@ -71,13 +65,8 @@ class LoadEntryData extends AbstractFixture implements OrderedFixtureInterface
         $entry4->setContent('This is my content /o/');
         $entry4->setLanguage('en');
 
-        $tag1 = new Tag($this->getReference('admin-user'));
-        $tag1->setLabel('foo');
-        $tag2 = new Tag($this->getReference('admin-user'));
-        $tag2->setLabel('bar');
-
-        $entry4->addTag($tag1);
-        $entry4->addTag($tag2);
+        $entry4->addTag($this->getReference('foo-tag'));
+        $entry4->addTag($this->getReference('bar-tag'));
 
         $manager->persist($entry4);
 
diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php
new file mode 100644
index 00000000..8553dced
--- /dev/null
+++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.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\Tag;
+
+class LoadTagData extends AbstractFixture implements OrderedFixtureInterface
+{
+    /**
+     * {@inheritdoc}
+     */
+    public function load(ObjectManager $manager)
+    {
+        $tag1 = new Tag();
+        $tag1->setLabel('foo');
+
+        $manager->persist($tag1);
+
+        $this->addReference('foo-tag', $tag1);
+
+        $tag2 = new Tag();
+        $tag2->setLabel('bar');
+
+        $manager->persist($tag2);
+
+        $this->addReference('bar-tag', $tag2);
+
+        $manager->flush();
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getOrder()
+    {
+        return 25;
+    }
+}
diff --git a/src/Wallabag/CoreBundle/Entity/Entry.php b/src/Wallabag/CoreBundle/Entity/Entry.php
index 2813c944..b413c489 100644
--- a/src/Wallabag/CoreBundle/Entity/Entry.php
+++ b/src/Wallabag/CoreBundle/Entity/Entry.php
@@ -465,7 +465,7 @@ class Entry
         // 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()) {
+            if ($existingTag->getLabel() === $tag->getLabel()) {
                 return;
             }
         }
diff --git a/src/Wallabag/CoreBundle/Entity/Tag.php b/src/Wallabag/CoreBundle/Entity/Tag.php
index 4ed588be..0689c7b3 100644
--- a/src/Wallabag/CoreBundle/Entity/Tag.php
+++ b/src/Wallabag/CoreBundle/Entity/Tag.php
@@ -38,6 +38,7 @@ class Tag
     private $label;
 
     /**
+     * @Expose
      * @Gedmo\Slug(fields={"label"})
      * @ORM\Column(length=128, unique=true)
      */
@@ -48,14 +49,8 @@ class Tag
      */
     private $entries;
 
-    /**
-     * @ORM\ManyToOne(targetEntity="Wallabag\UserBundle\Entity\User", inversedBy="tags")
-     */
-    private $user;
-
-    public function __construct(\Wallabag\UserBundle\Entity\User $user)
+    public function __construct()
     {
-        $this->user = $user;
         $this->entries = new ArrayCollection();
     }
 
@@ -112,12 +107,4 @@ class Tag
     {
         return $this->entries->contains($entry);
     }
-
-    /**
-     * @return User
-     */
-    public function getUser()
-    {
-        return $this->user;
-    }
 }
diff --git a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
index 41ef25b8..991c9a56 100644
--- a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
+++ b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php
@@ -37,7 +37,7 @@ class RuleBasedTagger
             }
 
             foreach ($rule->getTags() as $label) {
-                $tag = $this->getTag($entry->getUser(), $label);
+                $tag = $this->getTag($label);
 
                 $entry->addTag($tag);
             }
@@ -62,7 +62,7 @@ class RuleBasedTagger
 
             foreach ($entries as $entry) {
                 foreach ($rule->getTags() as $label) {
-                    $tag = $this->getTag($user, $label);
+                    $tag = $this->getTag($label);
 
                     $entry->addTag($tag);
                 }
@@ -73,19 +73,18 @@ class RuleBasedTagger
     }
 
     /**
-     * Fetch a tag for a user.
+     * Fetch a tag.
      *
-     * @param User   $user
      * @param string $label The tag's label.
      *
      * @return Tag
      */
-    private function getTag(User $user, $label)
+    private function getTag($label)
     {
-        $tag = $this->tagRepository->findOneByLabelAndUserId($label, $user->getId());
+        $tag = $this->tagRepository->findOneByLabel($label);
 
         if (!$tag) {
-            $tag = new Tag($user);
+            $tag = new Tag();
             $tag->setLabel($label);
         }
 
diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
index 57bf8024..9ff80d6e 100644
--- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php
+++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
@@ -5,6 +5,7 @@ namespace Wallabag\CoreBundle\Repository;
 use Doctrine\ORM\EntityRepository;
 use Pagerfanta\Adapter\DoctrineORMAdapter;
 use Pagerfanta\Pagerfanta;
+use Wallabag\CoreBundle\Entity\Tag;
 
 class EntryRepository extends EntityRepository
 {
@@ -179,4 +180,22 @@ class EntryRepository extends EntityRepository
             ->getQuery()
             ->getSingleResult();
     }
+
+    /**
+     * Remove a tag from all user entries.
+     * We are using a native SQL query because Doctrine doesn't know EntryTag entity because it's a ManyToMany relation.
+     * Instead of that SQL query we should loop on every entry and remove the tag, could be really long ...
+     *
+     * @param int $userId
+     * @param Tag $tag
+     */
+    public function removeTag($userId, Tag $tag)
+    {
+        $sql = 'DELETE et FROM entry_tag et WHERE et.entry_id IN ( SELECT e.id FROM entry e WHERE e.user_id = :userId ) AND et.tag_id = :tagId';
+        $stmt = $this->getEntityManager()->getConnection()->prepare($sql);
+        $stmt->execute([
+            'userId' => $userId,
+            'tagId' => $tag->getId(),
+        ]);
+    }
 }
diff --git a/src/Wallabag/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php
index ac3145a1..c4aeb594 100644
--- a/src/Wallabag/CoreBundle/Repository/TagRepository.php
+++ b/src/Wallabag/CoreBundle/Repository/TagRepository.php
@@ -9,16 +9,29 @@ use Pagerfanta\Pagerfanta;
 class TagRepository extends EntityRepository
 {
     /**
-     * Find Tags.
+     * Return only the QueryBuilder to retrieve all tags for a given user.
      *
      * @param int $userId
      *
-     * @return array
+     * @return QueryBuilder
+     */
+    private function getQbForAllTags($userId)
+    {
+        return $this->createQueryBuilder('t')
+            ->leftJoin('t.entries', 'e')
+            ->where('e.user = :userId')->setParameter('userId', $userId);
+    }
+
+    /**
+     * Find Tags and return a Pager.
+     *
+     * @param int $userId
+     *
+     * @return Pagerfanta
      */
     public function findTags($userId)
     {
-        $qb = $this->createQueryBuilder('t')
-            ->where('t.user =:userId')->setParameter('userId', $userId);
+        $qb = $this->getQbForAllTags($userId);
 
         $pagerAdapter = new DoctrineORMAdapter($qb);
 
@@ -26,19 +39,16 @@ class TagRepository extends EntityRepository
     }
 
     /**
-     * Find a tag by its label and its owner.
+     * Find Tags.
      *
-     * @param string $label
-     * @param int    $userId
+     * @param int $userId
      *
-     * @return Tag|null
+     * @return array
      */
-    public function findOneByLabelAndUserId($label, $userId)
+    public function findAllTags($userId)
     {
-        return $this->createQueryBuilder('t')
-            ->where('t.label = :label')->setParameter('label', $label)
-            ->andWhere('t.user = :user_id')->setParameter('user_id', $userId)
+        return $this->getQbForAllTags($userId)
             ->getQuery()
-            ->getOneOrNullResult();
+            ->getResult();
     }
 }
diff --git a/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php b/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php
index 7b32354f..89ca31e2 100644
--- a/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php
+++ b/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php
@@ -44,7 +44,7 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $form = $crawler->filter('button[id=config_save]')->form();
 
         $data = array(
-            'config[theme]' => 0,
+            'config[theme]' => 'baggy',
             'config[items_per_page]' => '30',
             'config[language]' => 'en',
         );
@@ -63,7 +63,7 @@ class ConfigControllerTest extends WallabagCoreTestCase
     {
         return array(
             array(array(
-                'config[theme]' => 0,
+                'config[theme]' => 'baggy',
                 'config[items_per_page]' => '',
                 'config[language]' => 'en',
             )),
diff --git a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
index 1de134b8..cddc8b08 100644
--- a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
+++ b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
@@ -69,9 +69,7 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
 
         $tags = $entry->getTags();
         $this->assertSame('foo', $tags[0]->getLabel());
-        $this->assertSame($user, $tags[0]->getUser());
         $this->assertSame('bar', $tags[1]->getLabel());
-        $this->assertSame($user, $tags[1]->getUser());
     }
 
     public function testTagWithAMixOfMatchingRules()
@@ -92,7 +90,6 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
 
         $tags = $entry->getTags();
         $this->assertSame('foo', $tags[0]->getLabel());
-        $this->assertSame($user, $tags[0]->getUser());
     }
 
     public function testWhenTheTagExists()
@@ -100,7 +97,8 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $taggingRule = $this->getTaggingRule('rule as string', array('foo'));
         $user = $this->getUser([$taggingRule]);
         $entry = new Entry($user);
-        $tag = new Tag($user);
+        $tag = new Tag();
+        $tag->setLabel('foo');
 
         $this->rulerz
             ->expects($this->once())
@@ -110,7 +108,9 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
 
         $this->tagRepository
             ->expects($this->once())
-            ->method('findOneByLabelAndUserId')
+            // the method `findOneByLabel` doesn't exist, EntityRepository will then call `_call` method
+            // to magically call the `findOneBy` with ['label' => 'foo']
+            ->method('__call')
             ->willReturn($tag);
 
         $this->tagger->tag($entry);
@@ -118,7 +118,7 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($entry->getTags()->isEmpty());
 
         $tags = $entry->getTags();
-        $this->assertSame($tag, $tags[0]);
+        $this->assertSame($tag->getLabel(), $tags[0]->getLabel());
     }
 
     public function testSameTagWithDifferentfMatchingRules()
-- 
cgit v1.2.3


From 1bb1939ab76cfbf1cdb5fa1dccbdd15ba17cdfb0 Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Tue, 29 Dec 2015 15:04:46 +0100
Subject: Cleanup tests

- WallabagRestController: remove the tag deletion from the API since we can't remove a tag now, we only remove reference to entries
- RuleBasedTaggerTest: remove workaround for asserting tag are equal since problem was related to mock expects (_call instead of findOneByLabel which was removed from the tag repository)
---
 src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

(limited to 'src/Wallabag/CoreBundle')

diff --git a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
index cddc8b08..dee17d65 100644
--- a/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
+++ b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
@@ -98,7 +98,6 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $user = $this->getUser([$taggingRule]);
         $entry = new Entry($user);
         $tag = new Tag();
-        $tag->setLabel('foo');
 
         $this->rulerz
             ->expects($this->once())
@@ -118,7 +117,7 @@ class RuleBasedTaggerTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($entry->getTags()->isEmpty());
 
         $tags = $entry->getTags();
-        $this->assertSame($tag->getLabel(), $tags[0]->getLabel());
+        $this->assertSame($tag, $tags[0]);
     }
 
     public function testSameTagWithDifferentfMatchingRules()
-- 
cgit v1.2.3


From 4059a061c0c8cc787f71e96aef2ab01599d3241d Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Tue, 29 Dec 2015 15:08:33 +0100
Subject: Fix the way to remove a tag from all user entries

---
 .../CoreBundle/Repository/EntryRepository.php      | 47 ++++++++++++++++++----
 1 file changed, 39 insertions(+), 8 deletions(-)

(limited to 'src/Wallabag/CoreBundle')

diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
index 9ff80d6e..e658a359 100644
--- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php
+++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
@@ -183,19 +183,50 @@ class EntryRepository extends EntityRepository
 
     /**
      * Remove a tag from all user entries.
-     * We are using a native SQL query because Doctrine doesn't know EntryTag entity because it's a ManyToMany relation.
-     * Instead of that SQL query we should loop on every entry and remove the tag, could be really long ...
+     *
+     * We need to loop on each entry attached to the given tag to remove it, since Doctrine doesn't know EntryTag entity because it's a ManyToMany relation.
+     * It could be faster with one query but I don't know how to retrieve the table name `entry_tag` which can have a prefix.
      *
      * @param int $userId
      * @param Tag $tag
      */
     public function removeTag($userId, Tag $tag)
     {
-        $sql = 'DELETE et FROM entry_tag et WHERE et.entry_id IN ( SELECT e.id FROM entry e WHERE e.user_id = :userId ) AND et.tag_id = :tagId';
-        $stmt = $this->getEntityManager()->getConnection()->prepare($sql);
-        $stmt->execute([
-            'userId' => $userId,
-            'tagId' => $tag->getId(),
-        ]);
+        $entries = $this->getBuilderByUser($userId)
+            ->innerJoin('e.tags', 't')
+            ->andWhere('t.id = :tagId')->setParameter('tagId', $tag->getId())
+            ->getQuery()
+            ->getResult();
+
+        foreach ($entries as $entry) {
+            $entry->removeTag($tag);
+        }
+
+        $this->getEntityManager()->flush();
+
+        // An other solution can be to use raw query but I can't find a way to retrieve the `entry_tag` table name since it can be prefixed....
+        // $sql = 'DELETE et FROM entry_tag et WHERE et.entry_id IN ( SELECT e.id FROM entry e WHERE e.user_id = :userId ) AND et.tag_id = :tagId';
+        // $stmt = $this->getEntityManager()->getConnection()->prepare($sql);
+        // $stmt->execute([
+        //     'userId' => $userId,
+        //     'tagId' => $tag->getId(),
+        // ]);
+    }
+
+    /**
+     * Find all entries that are attached to a give tag id.
+     *
+     * @param int $userId
+     * @param int $tagId
+     *
+     * @return array
+     */
+    public function findAllByTagId($userId, $tagId)
+    {
+        return $this->getBuilderByUser($userId)
+            ->innerJoin('e.tags', 't')
+            ->andWhere('t.id = :tagId')->setParameter('tagId', $tagId)
+            ->getQuery()
+            ->getResult();
     }
 }
-- 
cgit v1.2.3


From 6be9750155fa731d75898b4973a126a090345c2d Mon Sep 17 00:00:00 2001
From: Jeremy Benoist <jeremy.benoist@gmail.com>
Date: Tue, 29 Dec 2015 21:59:34 +0100
Subject: Removed comment

And move the SQL query inside the php doc
---
 src/Wallabag/CoreBundle/Repository/EntryRepository.php | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

(limited to 'src/Wallabag/CoreBundle')

diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
index e658a359..ca71970b 100644
--- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php
+++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php
@@ -185,7 +185,9 @@ class EntryRepository extends EntityRepository
      * Remove a tag from all user entries.
      *
      * We need to loop on each entry attached to the given tag to remove it, since Doctrine doesn't know EntryTag entity because it's a ManyToMany relation.
-     * It could be faster with one query but I don't know how to retrieve the table name `entry_tag` which can have a prefix.
+     * It could be faster with one query but I don't know how to retrieve the table name `entry_tag` which can have a prefix:
+     *
+     * DELETE et FROM entry_tag et WHERE et.entry_id IN ( SELECT e.id FROM entry e WHERE e.user_id = :userId ) AND et.tag_id = :tagId
      *
      * @param int $userId
      * @param Tag $tag
@@ -203,14 +205,6 @@ class EntryRepository extends EntityRepository
         }
 
         $this->getEntityManager()->flush();
-
-        // An other solution can be to use raw query but I can't find a way to retrieve the `entry_tag` table name since it can be prefixed....
-        // $sql = 'DELETE et FROM entry_tag et WHERE et.entry_id IN ( SELECT e.id FROM entry e WHERE e.user_id = :userId ) AND et.tag_id = :tagId';
-        // $stmt = $this->getEntityManager()->getConnection()->prepare($sql);
-        // $stmt->execute([
-        //     'userId' => $userId,
-        //     'tagId' => $tag->getId(),
-        // ]);
     }
 
     /**
-- 
cgit v1.2.3