From d9acb9ed876d9814e468081d799f06ffd631580f Mon Sep 17 00:00:00 2001 From: Bernhard Posselt Date: Wed, 14 May 2014 18:08:52 +0200 Subject: add serverside possibility to order by oldest first --- businesslayer/itembusinesslayer.php | 42 +++++++++--------- controller/itemcontroller.php | 7 ++- controller/pagecontroller.php | 42 +++++++++--------- db/itemmapper.php | 26 ++++++----- tests/unit/businesslayer/ItemBusinessLayerTest.php | 8 ++-- tests/unit/controller/ItemControllerTest.php | 22 +++++++--- tests/unit/controller/PageControllerTest.php | 32 ++++++++++++-- tests/unit/db/ItemMapperTest.php | 51 ++++++++++++++++++++-- 8 files changed, 160 insertions(+), 70 deletions(-) diff --git a/businesslayer/itembusinesslayer.php b/businesslayer/itembusinesslayer.php index 23f6bcd2d..82d445161 100644 --- a/businesslayer/itembusinesslayer.php +++ b/businesslayer/itembusinesslayer.php @@ -56,19 +56,18 @@ class ItemBusinessLayer extends BusinessLayer { switch($type){ case FeedType::FEED: - $items = $this->itemMapper->findAllNewFeed($id, $updatedSince, - $status, $userId); - break; + return $this->itemMapper->findAllNewFeed( + $id, $updatedSince, $status, $userId + ); case FeedType::FOLDER: - $items = $this->itemMapper->findAllNewFolder($id, $updatedSince, - $status, $userId); - break; + return $this->itemMapper->findAllNewFolder( + $id, $updatedSince, $status, $userId + ); default: - $items = $this->itemMapper->findAllNew($updatedSince, $status, - $userId); + return $this->itemMapper->findAllNew( + $updatedSince, $status, $userId + ); } - - return $items; } @@ -80,26 +79,27 @@ class ItemBusinessLayer extends BusinessLayer { * @param int $offset only items lower than this id are returned, 0 for no offset * @param boolean $showAll if unread items should also be returned * @param string $userId the name of the user + * @param boolean $oldestFirst if it should be ordered by oldest first * @return array of items */ - public function findAll($id, $type, $limit, $offset, $showAll, $userId){ + public function findAll($id, $type, $limit, $offset, $showAll, $userId, + $oldestFirst=false){ $status = $this->statusFlag->typeToStatus($type, $showAll); switch($type){ case FeedType::FEED: - $items = $this->itemMapper->findAllFeed($id, $limit, $offset, - $status, $userId); - break; + return $this->itemMapper->findAllFeed( + $id, $limit, $offset,$status, $userId, $oldestFirst + ); case FeedType::FOLDER: - $items = $this->itemMapper->findAllFolder($id, $limit, $offset, - $status, $userId); - break; + return $this->itemMapper->findAllFolder( + $id, $limit, $offset, $status, $userId, $oldestFirst + ); default: - $items = $this->itemMapper->findAll($limit, $offset, $status, - $userId); + return $this->itemMapper->findAll( + $limit, $offset, $status, $userId, $oldestFirst + ); } - - return $items; } diff --git a/controller/itemcontroller.php b/controller/itemcontroller.php index dfc059505..0f590950e 100644 --- a/controller/itemcontroller.php +++ b/controller/itemcontroller.php @@ -57,6 +57,8 @@ class ItemController extends Controller { public function index($type, $id, $limit, $offset=0) { $showAll = $this->settings->getUserValue($this->userId, $this->appName, 'showAll') === '1'; + $oldestFirst = $this->settings->getUserValue($this->userId, $this->appName, + 'oldestFirst') === '1'; $this->settings->setUserValue($this->userId, $this->appName, 'lastViewedFeedId', $id); @@ -77,8 +79,9 @@ class ItemController extends Controller { $params['starred'] = $this->itemBusinessLayer->starredCount($this->userId); } - $params['items'] = $this->itemBusinessLayer->findAll($id, $type, $limit, - $offset, $showAll, $this->userId); + $params['items'] = $this->itemBusinessLayer->findAll( + $id, $type, $limit, $offset, $showAll, $this->userId, $oldestFirst + ); // this gets thrown if there are no items // in that case just return an empty array diff --git a/controller/pagecontroller.php b/controller/pagecontroller.php index 88154803f..db0fa48bf 100644 --- a/controller/pagecontroller.php +++ b/controller/pagecontroller.php @@ -51,17 +51,17 @@ class PageController extends Controller { * @NoAdminRequired */ public function settings() { - $showAll = $this->settings->getUserValue($this->userId, $this->appName, - 'showAll'); - $compact = $this->settings->getUserValue($this->userId, $this->appName, - 'compact'); - $language = $this->l10n->getLanguageCode(); - - return [ - 'showAll' => $showAll === '1', - 'compact' => $compact === '1', - 'language' => $language - ]; + $settings = ['showAll', 'compact', 'readOnScroll', 'oldestFirst']; + + $result = ['language' => $this->l10n->getLanguageCode()]; + + foreach ($settings as $setting) { + $result[$setting] = $this->settings->getUserValue( + $this->userId, $this->appName, $setting + ) === '1'; + } + + return $result; } @@ -70,17 +70,19 @@ class PageController extends Controller { * * @param bool $showAll * @param bool $compact + * @param bool $readOnScroll + * @param bool $oldestFirst */ - public function updateSettings($showAll, $compact) { - if($showAll !== null) { - $this->settings->setUserValue($this->userId, $this->appName, - 'showAll', $showAll); - } - - if($compact !== null) { - $this->settings->setUserValue($this->userId, $this->appName, - 'compact', $compact); + public function updateSettings($showAll, $compact, $readOnScroll, $oldestFirst) { + $settings = ['showAll', 'compact', 'readOnScroll', 'oldestFirst']; + + foreach ($settings as $setting) { + if(${$setting} !== null) { + $this->settings->setUserValue($this->userId, $this->appName, + $setting, ${$setting}); + } } } + } \ No newline at end of file diff --git a/db/itemmapper.php b/db/itemmapper.php index 201022735..ff17596c2 100644 --- a/db/itemmapper.php +++ b/db/itemmapper.php @@ -24,7 +24,13 @@ class ItemMapper extends Mapper implements IMapper { } - private function makeSelectQuery($prependTo){ + private function makeSelectQuery($prependTo, $oldestFirst=false){ + if($oldestFirst) { + $ordering = 'ASC'; + } else { + $ordering = 'DESC'; + } + return 'SELECT `items`.* FROM `*PREFIX*news_items` `items` '. 'JOIN `*PREFIX*news_feeds` `feeds` ' . 'ON `feeds`.`id` = `items`.`feed_id` '. @@ -35,10 +41,10 @@ class ItemMapper extends Mapper implements IMapper { 'ON `folders`.`id` = `feeds`.`folder_id` ' . 'WHERE `feeds`.`folder_id` = 0 ' . 'OR `folders`.`deleted_at` = 0 ' . - 'ORDER BY `items`.`id` DESC'; + 'ORDER BY `items`.`id` ' . $ordering; } - private function makeSelectQueryStatus($prependTo, $status) { + private function makeSelectQueryStatus($prependTo, $status, $oldestFirst=false) { // Hi this is Ray and you're watching Jack Ass // Now look closely: this is how we adults handle weird bugs in our // code: we take them variables and we cast the shit out of them @@ -57,7 +63,7 @@ class ItemMapper extends Mapper implements IMapper { // SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT. // think twice when changing this 'AND ((`items`.`status` & ' . $status . ') = ' . $status . ') ' . - $prependTo + $prependTo, $oldestFirst ); } @@ -161,38 +167,38 @@ class ItemMapper extends Mapper implements IMapper { } - public function findAllFeed($id, $limit, $offset, $status, $userId){ + public function findAllFeed($id, $limit, $offset, $status, $userId, $oldestFirst=false){ $params = [$userId, $id]; $sql = 'AND `items`.`feed_id` = ? '; if($offset !== 0){ $sql .= 'AND `items`.`id` < ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status); + $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst); return $this->findEntities($sql, $params, $limit); } - public function findAllFolder($id, $limit, $offset, $status, $userId){ + public function findAllFolder($id, $limit, $offset, $status, $userId, $oldestFirst=false){ $params = [$userId, $id]; $sql = 'AND `feeds`.`folder_id` = ? '; if($offset !== 0){ $sql .= 'AND `items`.`id` < ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status); + $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst); return $this->findEntities($sql, $params, $limit); } - public function findAll($limit, $offset, $status, $userId){ + public function findAll($limit, $offset, $status, $userId, $oldestFirst=false){ $params = [$userId]; $sql = ''; if($offset !== 0){ $sql .= 'AND `items`.`id` < ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status); + $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst); return $this->findEntities($sql, $params, $limit); } diff --git a/tests/unit/businesslayer/ItemBusinessLayerTest.php b/tests/unit/businesslayer/ItemBusinessLayerTest.php index 1b31963c1..48c077460 100644 --- a/tests/unit/businesslayer/ItemBusinessLayerTest.php +++ b/tests/unit/businesslayer/ItemBusinessLayerTest.php @@ -127,7 +127,8 @@ class ItemBusinessLayerTest extends \PHPUnit_Framework_TestCase { $this->equalTo($this->limit), $this->equalTo($this->offset), $this->equalTo($this->status), - $this->equalTo($this->user)) + $this->equalTo($this->user), + $this->equalTo(false)) ->will($this->returnValue($this->response)); $result = $this->itemBusinessLayer->findAll( @@ -164,13 +165,14 @@ class ItemBusinessLayerTest extends \PHPUnit_Framework_TestCase { ->with( $this->equalTo($this->limit), $this->equalTo($this->offset), $this->equalTo($this->status), - $this->equalTo($this->user)) + $this->equalTo($this->user), + $this->equalTo(true)) ->will($this->returnValue($this->response)); $result = $this->itemBusinessLayer->findAll( $this->id, $type, $this->limit, $this->offset, $this->showAll, - $this->user); + $this->user, true); $this->assertEquals($this->response, $result); } diff --git a/tests/unit/controller/ItemControllerTest.php b/tests/unit/controller/ItemControllerTest.php index b4f87a0e9..1ebcdce55 100644 --- a/tests/unit/controller/ItemControllerTest.php +++ b/tests/unit/controller/ItemControllerTest.php @@ -188,20 +188,26 @@ class ItemControllerTest extends \PHPUnit_Framework_TestCase { } - private function itemsApiExpects($id, $type){ - $this->settings->expects($this->once()) + private function itemsApiExpects($id, $type, $oldestFirst='1'){ + $this->settings->expects($this->at(0)) ->method('getUserValue') ->with($this->equalTo($this->user), $this->equalTo($this->appName), $this->equalTo('showAll')) ->will($this->returnValue('1')); $this->settings->expects($this->at(1)) + ->method('getUserValue') + ->with($this->equalTo($this->user), + $this->equalTo($this->appName), + $this->equalTo('oldestFirst')) + ->will($this->returnValue($oldestFirst)); + $this->settings->expects($this->at(2)) ->method('setUserValue') ->with($this->equalTo($this->user), $this->equalTo($this->appName), $this->equalTo('lastViewedFeedId'), $this->equalTo($id)); - $this->settings->expects($this->at(2)) + $this->settings->expects($this->at(3)) ->method('setUserValue') ->with($this->equalTo($this->user), $this->equalTo($this->appName), @@ -219,7 +225,7 @@ class ItemControllerTest extends \PHPUnit_Framework_TestCase { 'starred' => 3111 ]; - $this->itemsApiExpects(2, FeedType::FEED); + $this->itemsApiExpects(2, FeedType::FEED, '0'); $this->feedBusinessLayer->expects($this->once()) ->method('findAll') @@ -244,7 +250,8 @@ class ItemControllerTest extends \PHPUnit_Framework_TestCase { $this->equalTo(3), $this->equalTo(0), $this->equalTo(true), - $this->equalTo($this->user)) + $this->equalTo($this->user), + $this->equalTo(false)) ->will($this->returnValue($result['items'])); $response = $this->controller->index(FeedType::FEED, 2, 3); @@ -264,13 +271,14 @@ class ItemControllerTest extends \PHPUnit_Framework_TestCase { $this->equalTo(3), $this->equalTo(10), $this->equalTo(true), - $this->equalTo($this->user)) + $this->equalTo($this->user), + $this->equalTo(true)) ->will($this->returnValue($result['items'])); $this->feedBusinessLayer->expects($this->never()) ->method('findAll'); - $response = $this->controller->index(FeedType::FEED, 2, 3, 10); + $response = $this->controller->index(FeedType::FEED, 2, 3, 10, true); $this->assertEquals($result, $response); } diff --git a/tests/unit/controller/PageControllerTest.php b/tests/unit/controller/PageControllerTest.php index d4c53e468..1e898c146 100644 --- a/tests/unit/controller/PageControllerTest.php +++ b/tests/unit/controller/PageControllerTest.php @@ -60,7 +60,9 @@ class PageControllerTest extends \PHPUnit_Framework_TestCase { $result = [ 'showAll' => true, 'compact' => true, - 'language' => 'de' + 'readOnScroll' => true, + 'oldestFirst' => true, + 'language' => 'de', ]; $this->l10n->expects($this->once()) @@ -78,6 +80,18 @@ class PageControllerTest extends \PHPUnit_Framework_TestCase { $this->equalTo($this->appName), $this->equalTo('compact')) ->will($this->returnValue('1')); + $this->settings->expects($this->at(2)) + ->method('getUserValue') + ->with($this->equalTo($this->user), + $this->equalTo($this->appName), + $this->equalTo('readOnScroll')) + ->will($this->returnValue('1')); + $this->settings->expects($this->at(3)) + ->method('getUserValue') + ->with($this->equalTo($this->user), + $this->equalTo($this->appName), + $this->equalTo('oldestFirst')) + ->will($this->returnValue('1')); $response = $this->controller->settings(); $this->assertEquals($result, $response); @@ -97,7 +111,19 @@ class PageControllerTest extends \PHPUnit_Framework_TestCase { $this->equalTo($this->appName), $this->equalTo('compact'), $this->equalTo(true)); - $this->controller->updateSettings(true, true); + $this->settings->expects($this->at(2)) + ->method('setUserValue') + ->with($this->equalTo($this->user), + $this->equalTo($this->appName), + $this->equalTo('readOnScroll'), + $this->equalTo(true)); + $this->settings->expects($this->at(3)) + ->method('setUserValue') + ->with($this->equalTo($this->user), + $this->equalTo($this->appName), + $this->equalTo('oldestFirst'), + $this->equalTo(true)); + $this->controller->updateSettings(true, true, true, true); } @@ -113,7 +139,7 @@ class PageControllerTest extends \PHPUnit_Framework_TestCase { $this->equalTo('showAll'), $this->equalTo(true)); - $this->controller->updateSettings(true, null); + $this->controller->updateSettings(true, null, null, null); } } \ No newline at end of file diff --git a/tests/unit/db/ItemMapperTest.php b/tests/unit/db/ItemMapperTest.php index 15379f485..7a855b7bb 100644 --- a/tests/unit/db/ItemMapperTest.php +++ b/tests/unit/db/ItemMapperTest.php @@ -63,7 +63,12 @@ class ItemMapperTest extends \OCP\AppFramework\Db\MapperTestUtility { } - private function makeSelectQuery($prependTo){ + private function makeSelectQuery($prependTo, $oldestFirst=false){ + if($oldestFirst) { + $ordering = 'ASC'; + } else { + $ordering = 'DESC'; + } return 'SELECT `items`.* FROM `*PREFIX*news_items` `items` '. 'JOIN `*PREFIX*news_feeds` `feeds` ' . 'ON `feeds`.`id` = `items`.`feed_id` '. @@ -74,15 +79,15 @@ class ItemMapperTest extends \OCP\AppFramework\Db\MapperTestUtility { 'ON `folders`.`id` = `feeds`.`folder_id` ' . 'WHERE `feeds`.`folder_id` = 0 ' . 'OR `folders`.`deleted_at` = 0 ' . - 'ORDER BY `items`.`id` DESC'; + 'ORDER BY `items`.`id` ' . $ordering; } - private function makeSelectQueryStatus($prependTo, $status) { + private function makeSelectQueryStatus($prependTo, $status, $oldestFirst=false) { $status = (int) $status; return $this->makeSelectQuery( 'AND ((`items`.`status` & ' . $status . ') = ' . $status . ') ' . - $prependTo + $prependTo, $oldestFirst ); } @@ -227,6 +232,19 @@ class ItemMapperTest extends \OCP\AppFramework\Db\MapperTestUtility { } + public function testFindAllFeedOldestFirst(){ + $sql = 'AND `items`.`feed_id` = ? ' . + 'AND `items`.`id` < ? '; + $sql = $this->makeSelectQueryStatus($sql, $this->status, true); + $params = [$this->user, $this->id, $this->offset]; + $this->setMapperResult($sql, $params, $this->rows); + $result = $this->mapper->findAllFeed($this->id, $this->limit, + $this->offset, $this->status, $this->user, true); + + $this->assertEquals($this->items, $result); + } + + public function testFindAllFeedOffsetZero(){ $sql = 'AND `items`.`feed_id` = ? '; $sql = $this->makeSelectQueryStatus($sql, $this->status); @@ -252,6 +270,19 @@ class ItemMapperTest extends \OCP\AppFramework\Db\MapperTestUtility { } + public function testFindAllFolderOldestFirst(){ + $sql = 'AND `feeds`.`folder_id` = ? ' . + 'AND `items`.`id` < ? '; + $sql = $this->makeSelectQueryStatus($sql, $this->status, true); + $params = [$this->user, $this->id, $this->offset]; + $this->setMapperResult($sql, $params, $this->rows); + $result = $this->mapper->findAllFolder($this->id, $this->limit, + $this->offset, $this->status, $this->user, true); + + $this->assertEquals($this->items, $result); + } + + public function testFindAllFolderOffsetZero(){ $sql = 'AND `feeds`.`folder_id` = ? '; $sql = $this->makeSelectQueryStatus($sql, $this->status); @@ -276,6 +307,18 @@ class ItemMapperTest extends \OCP\AppFramework\Db\MapperTestUtility { } + public function testFindAllOldestFirst(){ + $sql = 'AND `items`.`id` < ? '; + $sql = $this->makeSelectQueryStatus($sql, $this->status, true); + $params = [$this->user, $this->offset]; + $this->setMapperResult($sql, $params, $this->rows); + $result = $this->mapper->findAll($this->limit, + $this->offset, $this->status, $this->user, true); + + $this->assertEquals($this->items, $result); + } + + public function testFindAllOffsetZero(){ $sql = $this->makeSelectQueryStatus('', $this->status); $params = [$this->user]; -- cgit v1.2.3