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: [PATCH] Remove user reference in tag

Fix #1543
---
 .../Controller/WallabagRestController.php     | 17 +++++---
 .../Controller/WallabagRestControllerTest.php |  3 +-
 .../CoreBundle/Controller/TagController.php   | 12 +++---
 .../DataFixtures/ORM/LoadEntryData.php        | 19 ++-------
 .../DataFixtures/ORM/LoadTagData.php          | 41 +++++++++++++++++++
 src/Wallabag/CoreBundle/Entity/Entry.php      |  2 +-
 src/Wallabag/CoreBundle/Entity/Tag.php        | 17 +-------
 .../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 +++---
 src/Wallabag/UserBundle/Entity/User.php       | 27 ------------
 13 files changed, 123 insertions(+), 99 deletions(-)
 create mode 100644 src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php

diff --git a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php
index 74bfe4dcd..587013f68 100644
--- a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php
+++ b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php
@@ -27,7 +27,7 @@ class WallabagRestController extends FOSRestController
                 ->findOneByLabel($label);
 
             if (is_null($tagEntity)) {
-                $tagEntity = new Tag($this->getUser());
+                $tagEntity = new Tag();
                 $tagEntity->setLabel($label);
             }
 
@@ -74,8 +74,7 @@ class WallabagRestController extends FOSRestController
         $perPage = (int) $request->query->get('perPage', 30);
         $tags = $request->query->get('tags', []);
 
-        $pager = $this
-            ->getDoctrine()
+        $pager = $this->getDoctrine()
             ->getRepository('WallabagCoreBundle:Entry')
             ->findEntries($this->getUser()->getId(), $isArchived, $isStarred, $sort, $order);
 
@@ -311,7 +310,12 @@ class WallabagRestController extends FOSRestController
     public function getTagsAction()
     {
         $this->validateAuthentication();
-        $json = $this->get('serializer')->serialize($this->getUser()->getTags(), 'json');
+
+        $tags = $this->getDoctrine()
+            ->getRepository('WallabagCoreBundle:Tag')
+            ->findAllTags($this->getUser()->getId());
+
+        $json = $this->get('serializer')->serialize($tags, 'json');
 
         return $this->renderJsonResponse($json);
     }
@@ -328,7 +332,10 @@ class WallabagRestController extends FOSRestController
     public function deleteTagAction(Tag $tag)
     {
         $this->validateAuthentication();
-        $this->validateUserAccess($tag->getUser()->getId());
+
+        $this->getDoctrine()
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->removeTag($this->getUser()->getId(), $tag);
 
         $em = $this->getDoctrine()->getManager();
         $em->remove($tag);
diff --git a/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php b/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php
index bdd36e0c6..a7120e835 100644
--- a/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php
+++ b/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php
@@ -208,7 +208,7 @@ class WallabagRestControllerTest extends WallabagApiTestCase
 
         $tags = array();
         foreach ($entry->getTags() as $tag) {
-            $tags[] = array('id' => $tag->getId(), 'label' => $tag->getLabel());
+            $tags[] = array('id' => $tag->getId(), 'label' => $tag->getLabel(), 'slug' => $tag->getSlug());
         }
 
         $this->client->request('GET', '/api/entries/'.$entry->getId().'/tags');
@@ -309,5 +309,6 @@ class WallabagRestControllerTest extends WallabagApiTestCase
 
         $this->assertArrayHasKey('label', $content);
         $this->assertEquals($tag['label'], $content['label']);
+        $this->assertEquals($tag['slug'], $content['slug']);
     }
 }
diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php
index fd2069e03..64d53f0c7 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 0513cdb89..6c6a331a6 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 000000000..8553dced4
--- /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 2813c9442..b413c489c 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 4ed588be8..0689c7b31 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 41ef25b8c..991c9a56b 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 57bf8024c..9ff80d6ef 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 ac3145a1d..c4aeb5949 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 7b32354f8..89ca31e29 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 1de134b8a..cddc8b086 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()
diff --git a/src/Wallabag/UserBundle/Entity/User.php b/src/Wallabag/UserBundle/Entity/User.php
index e3b9a519c..e65284203 100644
--- a/src/Wallabag/UserBundle/Entity/User.php
+++ b/src/Wallabag/UserBundle/Entity/User.php
@@ -13,7 +13,6 @@ use JMS\Serializer\Annotation\Expose;
 use FOS\UserBundle\Model\User as BaseUser;
 use Wallabag\CoreBundle\Entity\Config;
 use Wallabag\CoreBundle\Entity\Entry;
-use Wallabag\CoreBundle\Entity\Tag;
 
 /**
  * User.
@@ -69,11 +68,6 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf
      */
     protected $config;
 
-    /**
-     * @ORM\OneToMany(targetEntity="Wallabag\CoreBundle\Entity\Tag", mappedBy="user", cascade={"remove"})
-     */
-    protected $tags;
-
     /**
      * @ORM\Column(type="integer", nullable=true)
      */
@@ -94,7 +88,6 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf
     {
         parent::__construct();
         $this->entries = new ArrayCollection();
-        $this->tags = new ArrayCollection();
         $this->roles = array('ROLE_USER');
     }
 
@@ -171,26 +164,6 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf
         return $this->entries;
     }
 
-    /**
-     * @param Entry $entry
-     *
-     * @return User
-     */
-    public function addTag(Tag $tag)
-    {
-        $this->tags[] = $tag;
-
-        return $this;
-    }
-
-    /**
-     * @return ArrayCollection<Tag>
-     */
-    public function getTags()
-    {
-        return $this->tags;
-    }
-
     public function isEqualTo(UserInterface $user)
     {
         return $this->username === $user->getUsername();