From 7c04b7396a296e31bb11beadc19550396ee728a8 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 3 Aug 2017 12:46:20 +0200 Subject: Multiple tag search was broken from API First, the setParameter() were done on the same parameter which in fact just duplicated the condition in the SQL query (like `where t.label = 'test' and t.label = 'test'`. Changed the parameter doesn't help because the query was then wrong. Changing the way to match associated tags for an entry and it worked. --- .../CoreBundle/DataFixtures/ORM/LoadTagData.php | 9 ++++- .../CoreBundle/Repository/EntryRepository.php | 45 +++++++++++++++++++--- .../Controller/EntryRestControllerTest.php | 3 ++ .../CoreBundle/Controller/ExportControllerTest.php | 2 +- .../CoreBundle/Controller/RssControllerTest.php | 6 +-- 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php index 6de561e0..0ecfd18b 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php +++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php @@ -19,7 +19,7 @@ class LoadTagData extends AbstractFixture implements OrderedFixtureInterface $manager->persist($tag1); - $this->addReference('foo-tag', $tag1); + $this->addReference('foo-bar-tag', $tag1); $tag2 = new Tag(); $tag2->setLabel('bar'); @@ -35,6 +35,13 @@ class LoadTagData extends AbstractFixture implements OrderedFixtureInterface $this->addReference('baz-tag', $tag3); + $tag4 = new Tag(); + $tag4->setLabel('foo'); + + $manager->persist($tag4); + + $this->addReference('foo-tag', $tag4); + $manager->flush(); } diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php index ecc159fc..44f95dcb 100644 --- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php +++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php @@ -133,7 +133,7 @@ class EntryRepository extends EntityRepository { $qb = $this->createQueryBuilder('e') ->leftJoin('e.tags', 't') - ->where('e.user =:userId')->setParameter('userId', $userId); + ->where('e.user = :userId')->setParameter('userId', $userId); if (null !== $isArchived) { $qb->andWhere('e.isArchived = :isArchived')->setParameter('isArchived', (bool) $isArchived); @@ -152,8 +152,23 @@ class EntryRepository extends EntityRepository } if ('' !== $tags) { - foreach (explode(',', $tags) as $tag) { - $qb->andWhere('t.label = :label')->setParameter('label', $tag); + foreach (explode(',', $tags) as $i => $tag) { + $entryAlias = 'e' . $i; + $tagAlias = 't' . $i; + + // Complexe queries to ensure multiple tags is associated to an entry + // https://stackoverflow.com/a/6638146/569101 + $qb->andWhere($qb->expr()->in( + 'e.id', + $this->createQueryBuilder($entryAlias) + ->select($entryAlias . '.id') + ->leftJoin($entryAlias . '.tags', $tagAlias) + ->where($tagAlias . '.label = :label' . $i) + ->getDQL() + )); + + // bound parameter to the main query builder + $qb->setParameter('label' . $i, $tag); } } @@ -181,7 +196,7 @@ class EntryRepository extends EntityRepository ->innerJoin('e.tags', 't') ->innerJoin('e.user', 'u') ->addSelect('t', 'u') - ->where('e.user=:userId')->setParameter('userId', $userId) + ->where('e.user = :userId')->setParameter('userId', $userId) ; return $qb->getQuery()->getResult(); @@ -323,7 +338,27 @@ class EntryRepository extends EntityRepository { $qb = $this->createQueryBuilder('e') ->select('count(e)') - ->where('e.user=:userId')->setParameter('userId', $userId) + ->where('e.user = :userId')->setParameter('userId', $userId) + ; + + return (int) $qb->getQuery()->getSingleScalarResult(); + } + + /** + * Count all entries for a tag and a user. + * + * @param int $userId + * @param int $tagId + * + * @return int + */ + public function countAllEntriesByUserIdAndTagId($userId, $tagId) + { + $qb = $this->createQueryBuilder('e') + ->select('count(e.id)') + ->leftJoin('e.tags', 't') + ->where('e.user = :userId')->setParameter('userId', $userId) + ->andWhere('t.id = :tagId')->setParameter('tagId', $tagId) ; return (int) $qb->getQuery()->getSingleScalarResult(); diff --git a/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php index f4c8a630..fcec3f3b 100644 --- a/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php +++ b/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php @@ -292,6 +292,9 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertSame(1, $content['page']); $this->assertGreaterThanOrEqual(1, $content['pages']); + $this->assertContains('foo', array_column($content['_embedded']['items'][0]['tags'], 'label'), 'Entries tags should have "foo" tag'); + $this->assertContains('bar', array_column($content['_embedded']['items'][0]['tags'], 'label'), 'Entries tags should have "bar" tag'); + $this->assertArrayHasKey('_links', $content); $this->assertArrayHasKey('self', $content['_links']); $this->assertArrayHasKey('first', $content['_links']); diff --git a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php index 3e216381..02ad26ae 100644 --- a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php @@ -239,7 +239,7 @@ class ExportControllerTest extends WallabagCoreTestCase $this->assertSame($contentInDB->getLanguage(), $content[0]['language']); $this->assertSame($contentInDB->getReadingtime(), $content[0]['reading_time']); $this->assertSame($contentInDB->getDomainname(), $content[0]['domain_name']); - $this->assertSame(['foo bar', 'baz'], $content[0]['tags']); + $this->assertSame(['baz', 'foo'], $content[0]['tags']); } public function testXmlExport() diff --git a/tests/Wallabag/CoreBundle/Controller/RssControllerTest.php b/tests/Wallabag/CoreBundle/Controller/RssControllerTest.php index 6167fe2d..c6ca4937 100644 --- a/tests/Wallabag/CoreBundle/Controller/RssControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/RssControllerTest.php @@ -184,13 +184,13 @@ class RssControllerTest extends WallabagCoreTestCase $em->flush(); $client = $this->getClient(); - $client->request('GET', '/admin/SUPERTOKEN/tags/foo-bar.xml'); + $client->request('GET', '/admin/SUPERTOKEN/tags/foo.xml'); $this->assertSame(200, $client->getResponse()->getStatusCode()); - $this->validateDom($client->getResponse()->getContent(), 'tag (foo bar)', 'tags/foo-bar'); + $this->validateDom($client->getResponse()->getContent(), 'tag (foo)', 'tags/foo'); - $client->request('GET', '/admin/SUPERTOKEN/tags/foo-bar.xml?page=3000'); + $client->request('GET', '/admin/SUPERTOKEN/tags/foo.xml?page=3000'); $this->assertSame(302, $client->getResponse()->getStatusCode()); } } -- cgit v1.2.3 From 33264c2d02fdfe6e8321096491b9c7398cd10e9a Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 3 Aug 2017 16:20:49 +0200 Subject: Fix tests --- src/Wallabag/CoreBundle/Repository/EntryRepository.php | 2 +- tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php index 44f95dcb..47db4c07 100644 --- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php +++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php @@ -156,7 +156,7 @@ class EntryRepository extends EntityRepository $entryAlias = 'e' . $i; $tagAlias = 't' . $i; - // Complexe queries to ensure multiple tags is associated to an entry + // Complexe queries to ensure multiple tags are associated to an entry // https://stackoverflow.com/a/6638146/569101 $qb->andWhere($qb->expr()->in( 'e.id', diff --git a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php index 02ad26ae..ab7f23cc 100644 --- a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php @@ -239,7 +239,8 @@ class ExportControllerTest extends WallabagCoreTestCase $this->assertSame($contentInDB->getLanguage(), $content[0]['language']); $this->assertSame($contentInDB->getReadingtime(), $content[0]['reading_time']); $this->assertSame($contentInDB->getDomainname(), $content[0]['domain_name']); - $this->assertSame(['baz', 'foo'], $content[0]['tags']); + $this->assertContains('baz', $content[0]['tags']); + $this->assertContains('foo', $content[0]['tags']); } public function testXmlExport() -- cgit v1.2.3 From 9c4a7388da9a250b30ff83c380581fe146af5bb1 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Wed, 6 Sep 2017 22:58:34 +0200 Subject: Remove unused function Introduce after the rebase I guess --- .../CoreBundle/Repository/EntryRepository.php | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/Wallabag/CoreBundle/Repository/EntryRepository.php b/src/Wallabag/CoreBundle/Repository/EntryRepository.php index 47db4c07..05f0e0ba 100644 --- a/src/Wallabag/CoreBundle/Repository/EntryRepository.php +++ b/src/Wallabag/CoreBundle/Repository/EntryRepository.php @@ -344,26 +344,6 @@ class EntryRepository extends EntityRepository return (int) $qb->getQuery()->getSingleScalarResult(); } - /** - * Count all entries for a tag and a user. - * - * @param int $userId - * @param int $tagId - * - * @return int - */ - public function countAllEntriesByUserIdAndTagId($userId, $tagId) - { - $qb = $this->createQueryBuilder('e') - ->select('count(e.id)') - ->leftJoin('e.tags', 't') - ->where('e.user = :userId')->setParameter('userId', $userId) - ->andWhere('t.id = :tagId')->setParameter('tagId', $tagId) - ; - - return (int) $qb->getQuery()->getSingleScalarResult(); - } - /** * Remove all entries for a user id. * Used when a user want to reset all informations. -- cgit v1.2.3