Merge pull request #1545 from wallabag/v2-user-tag

v2 – Remove user reference in tag

13 files changed, 159 insertions, 103 deletions

diff --git a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php
index 74bfe4dc..459c4172 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);
 
@@ -175,8 +174,8 @@ class WallabagRestController extends FOSRestController
         $this->validateUserAccess($entry->getUser()->getId());
 
         $title = $request->request->get('title');
-        $isArchived = $request->request->get('is_archived');
-        $isStarred = $request->request->get('is_starred');
+        $isArchived = $request->request->get('archive');
+        $isStarred = $request->request->get('star');
 
         if (!is_null($title)) {
             $entry->setTitle($title);
@@ -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,11 +332,10 @@ class WallabagRestController extends FOSRestController
     public function deleteTagAction(Tag $tag)
     {
         $this->validateAuthentication();
-        $this->validateUserAccess($tag->getUser()->getId());
 
-        $em = $this->getDoctrine()->getManager();
-        $em->remove($tag);
-        $em->flush();
+        $this->getDoctrine()
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->removeTag($this->getUser()->getId(), $tag);
 
         $json = $this->get('serializer')->serialize($tag, 'json');
 
diff --git a/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php b/src/Wallabag/ApiBundle/Tests/Controller/WallabagRestControllerTest.php
index bdd36e0c..22894a77 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,13 @@ class WallabagRestControllerTest extends WallabagApiTestCase
 
         $this->assertArrayHasKey('label', $content);
         $this->assertEquals($tag['label'], $content['label']);
+        $this->assertEquals($tag['slug'], $content['slug']);
+
+        $entries = $entry = $this->client->getContainer()
+            ->get('doctrine.orm.entity_manager')
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findAllByTagId($this->user->getId(), $tag['id']);
+
+        $this->assertCount(0, $entries);
     }
 }
diff --git a/src/Wallabag/ApiBundle/Tests/WallabagApiTestCase.php b/src/Wallabag/ApiBundle/Tests/WallabagApiTestCase.php
index 8a57fea2..a415c749 100644
--- a/src/Wallabag/ApiBundle/Tests/WallabagApiTestCase.php
+++ b/src/Wallabag/ApiBundle/Tests/WallabagApiTestCase.php
@@ -12,6 +12,11 @@ abstract class WallabagApiTestCase extends WebTestCase
      */
     protected $client = null;
 
+    /**
+     * @var \FOS\UserBundle\Model\UserInterface
+     */
+    protected $user;
+
     public function setUp()
     {
         $this->client = $this->createAuthorizedClient();
@@ -31,8 +36,8 @@ abstract class WallabagApiTestCase extends WebTestCase
         $loginManager = $container->get('fos_user.security.login_manager');
         $firewallName = $container->getParameter('fos_user.firewall_name');
 
-        $user = $userManager->findUserBy(array('username' => 'admin'));
-        $loginManager->loginUser($firewallName, $user);
+        $this->user = $userManager->findUserBy(array('username' => 'admin'));
+        $loginManager->loginUser($firewallName, $this->user);
 
         // save the login token into the session and put it in a cookie
         $container->get('session')->set('_security_'.$firewallName, serialize($container->get('security.token_storage')->getToken()));
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..ca71970b 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,47 @@ class EntryRepository extends EntityRepository
             ->getQuery()
             ->getSingleResult();
     }
+
+    /**
+     * 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:
+     *
+     * 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
+     */
+    public function removeTag($userId, Tag $tag)
+    {
+        $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();
+    }
+
+    /**
+     * 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();
+    }
 }
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/Helper/RuleBasedTaggerTest.php b/src/Wallabag/CoreBundle/Tests/Helper/RuleBasedTaggerTest.php
index 1de134b8..dee17d65 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,7 @@ 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();
 
         $this->rulerz
             ->expects($this->once())
@@ -110,7 +107,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);
diff --git a/src/Wallabag/UserBundle/Entity/User.php b/src/Wallabag/UserBundle/Entity/User.php
index e3b9a519..e6528420 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.
@@ -70,11 +69,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)
      */
     private $authCode;
@@ -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();