diff options
-rw-r--r-- | bl/feedbl.php | 30 | ||||
-rw-r--r-- | bl/itembl.php | 4 | ||||
-rw-r--r-- | controller/itemcontroller.php | 3 | ||||
-rw-r--r-- | db/itemmapper.php | 15 | ||||
-rw-r--r-- | js/app/controllers/feedcontroller.coffee | 4 | ||||
-rw-r--r-- | js/app/services/models/itemmodel.coffee | 38 | ||||
-rw-r--r-- | js/public/app.js | 39 | ||||
-rw-r--r-- | js/tests/controllers/feedcontrollerSpec.coffee | 8 | ||||
-rw-r--r-- | js/tests/services/models/itemmodelSpec.coffee | 29 | ||||
-rw-r--r-- | tests/bl/FeedBlTest.php | 53 | ||||
-rw-r--r-- | tests/bl/ItemBlTest.php | 7 | ||||
-rw-r--r-- | tests/controller/ItemControllerTest.php | 7 | ||||
-rw-r--r-- | tests/db/ItemMapperTest.php | 16 |
13 files changed, 201 insertions, 52 deletions
diff --git a/bl/feedbl.php b/bl/feedbl.php index a77f9bdd2..3f9708234 100644 --- a/bl/feedbl.php +++ b/bl/feedbl.php @@ -71,8 +71,10 @@ class FeedBl extends Bl { $feed->setUserId($userId); $feed = $this->mapper->insert($feed); - // insert items - foreach($items as $item){ + // insert items in reverse order because the first one is usually the + // newest item + for($i=count($items)-1; $i>=0; $i--){ + $item = $items[$i]; $item->setFeedId($feed->getId()); $this->itemMapper->insert($item); } @@ -106,8 +108,10 @@ class FeedBl extends Bl { try { list($feed, $items) = $this->feedFetcher->fetch($feed->getUrl()); - // update items - foreach($items as $item){ + // insert items in reverse order because the first one is usually the + // newest item + for($i=count($items)-1; $i>=0; $i--){ + $item = $items[$i]; // if a database exception is being thrown the unique constraint // on the item guid hash is being violated and we need to update @@ -116,10 +120,24 @@ class FeedBl extends Bl { $item->setFeedId($feed->getId()); $this->itemMapper->insert($item); } catch(\DatabaseException $ex){ + + // in case of an update the existing item has to be deleted + // if the pub_date changed because we sort by id on the + // client side since this is the only reliable way to do it + // to not get weird behaviour $existing = $this->itemMapper->findByGuidHash( $item->getGuidHash(), $feedId, $userId); - $item->setId($existing->getId()); - $this->itemMapper->update($item); + + if($existing->getPubDate() !== $item->getPubDate()){ + + // because the item is being replaced we need to keep + // status flags + $item->setStatus($existing->getStatus()); + $item->setUnread(); + + $this->itemMapper->insert($item); + } + } } diff --git a/bl/itembl.php b/bl/itembl.php index 332a51774..3c8649201 100644 --- a/bl/itembl.php +++ b/bl/itembl.php @@ -112,8 +112,8 @@ class ItemBl extends Bl { } - public function readFeed($feedId, $userId){ - $this->mapper->readFeed($feedId, $userId); + public function readFeed($feedId, $highestItemId, $userId){ + $this->mapper->readFeed($feedId, $highestItemId, $userId); } diff --git a/controller/itemcontroller.php b/controller/itemcontroller.php index 47061a14c..2c00fb1bd 100644 --- a/controller/itemcontroller.php +++ b/controller/itemcontroller.php @@ -163,8 +163,9 @@ class ItemController extends Controller { public function readFeed(){ $userId = $this->api->getUserId(); $feedId = (int) $this->params('feedId'); + $highestItemId = (int) $this->params('highestItemId'); - $this->itemBl->readFeed($feedId, $userId); + $this->itemBl->readFeed($feedId, $highestItemId, $userId); } diff --git a/db/itemmapper.php b/db/itemmapper.php index 0208472b3..f4702a626 100644 --- a/db/itemmapper.php +++ b/db/itemmapper.php @@ -92,14 +92,15 @@ class ItemMapper extends Mapper implements IMapper { } - public function readFeed($feedId, $userId){ + public function readFeed($feedId, $highestItemId, $userId){ $sql = 'UPDATE `*PREFIX*news_feeds` `feeds` ' . 'JOIN `*PREFIX*news_items` `items` ' . 'ON `items`.`feed_id` = `feeds`.`id` ' . 'AND `feeds`.`user_id` = ? ' . - 'SET `items`.`status` = (`items`.`status` & ?) ' . - 'WHERE `items`.`id` = ?'; - $params = array(~StatusFlag::UNREAD, $userId, $feedId); + 'AND `feeds`.`id` = ? ' . + 'AND `items`.`id` <= ? ' . + 'SET `items`.`status` = (`items`.`status` & ?) '; + $params = array($userId, $feedId, $highestItemId, ~StatusFlag::UNREAD); $this->execute($sql, $params); } @@ -107,7 +108,7 @@ class ItemMapper extends Mapper implements IMapper { public function findAllNewFeed($id, $updatedSince, $status, $userId){ $sql = 'AND `items`.`feed_id` = ? ' . - 'AND `items`.`last_modified` >= ?'; + 'AND `items`.`id` >= ?'; $sql = $this->makeSelectQueryStatus($sql); $params = array($userId, $status, $id, $updatedSince); return $this->findAllRows($sql, $params); @@ -116,7 +117,7 @@ class ItemMapper extends Mapper implements IMapper { public function findAllNewFolder($id, $updatedSince, $status, $userId){ $sql = 'AND `feeds`.`folder_id` = ? ' . - 'AND `items`.`last_modified` >= ?'; + 'AND `items`.`id` >= ?'; $sql = $this->makeSelectQueryStatus($sql); $params = array($userId, $status, $id, $updatedSince); return $this->findAllRows($sql, $params); @@ -124,7 +125,7 @@ class ItemMapper extends Mapper implements IMapper { public function findAllNew($updatedSince, $status, $userId){ - $sql = $this->makeSelectQueryStatus('AND `items`.`last_modified` >= ?'); + $sql = $this->makeSelectQueryStatus('AND `items`.`id` >= ?'); $params = array($userId, $status, $updatedSince); return $this->findAllRows($sql, $params); } diff --git a/js/app/controllers/feedcontroller.coffee b/js/app/controllers/feedcontroller.coffee index 1c04cd92e..14e1aea5b 100644 --- a/js/app/controllers/feedcontroller.coffee +++ b/js/app/controllers/feedcontroller.coffee @@ -179,7 +179,7 @@ angular.module('News').factory '_FeedController', -> @_persistence.getItems(type, id, 0) @_active.handle({id: id, type: type}) else - lastModified = @_itemModel.getLastModified() + lastModified = @_itemModel.getHighestId() @_persistence.getItems(type, id, 0, null, lastModified) @@ -208,6 +208,8 @@ angular.module('News').factory '_FeedController', -> feed = @_feedModel.getById(id) if angular.isDefined(feed) feed.unreadCount = 0 + # TODO: also update items in the right field if id is the + # the same highestItemId = @_itemModel.getHighestId() @_persistence.setFeedRead(id, highestItemId) when @_feedType.Folder diff --git a/js/app/services/models/itemmodel.coffee b/js/app/services/models/itemmodel.coffee index 18af373d9..16250ecf7 100644 --- a/js/app/services/models/itemmodel.coffee +++ b/js/app/services/models/itemmodel.coffee @@ -27,14 +27,40 @@ angular.module('News').factory '_ItemModel', class ItemModel extends _Model - getLastModified: -> - query = new _MaximumQuery('lastModified') - lastModified = @get(query) + constructor: -> + @_guidFeedIdHash = {} + super() + + + clear: -> + @_guidFeedIdHash = {} + super() + + + # items have two unique fields: feed_id and guidhash + # in case we get updated items with the same two fields we + # also need to update the field + add: (data, clearCache=true) -> + hash = data.feedId + '_' + data.guidHash + entry = @_guidFeedIdHash[hash] + + # update entry if exists with same feedid and guidhash + if angular.isDefined(entry) + + # first update id that could have changed + delete @_dataMap[entry.id] + @_dataMap[data.id] = entry + + # now copy over the elements data attrs + for key, value of data + if key == 'feedId' or key == 'guidHash' + continue + else + entry[key] = value - if angular.isDefined(lastModified) - return lastModified.lastModified else - return null + @_guidFeedIdHash[hash] = data + super(data, clearCache) getHighestId: -> diff --git a/js/public/app.js b/js/public/app.js index 9efc9aece..d2ff687ae 100644 --- a/js/public/app.js +++ b/js/public/app.js @@ -372,7 +372,7 @@ License along with this library. If not, see <http://www.gnu.org/licenses/>. type: type }); } else { - lastModified = this._itemModel.getLastModified(); + lastModified = this._itemModel.getHighestId(); return this._persistence.getItems(type, id, 0, null, lastModified); } }; @@ -824,17 +824,38 @@ License along with this library. If not, see <http://www.gnu.org/licenses/>. __extends(ItemModel, _super); function ItemModel() { - return ItemModel.__super__.constructor.apply(this, arguments); + this._guidFeedIdHash = {}; + ItemModel.__super__.constructor.call(this); } - ItemModel.prototype.getLastModified = function() { - var lastModified, query; - query = new _MaximumQuery('lastModified'); - lastModified = this.get(query); - if (angular.isDefined(lastModified)) { - return lastModified.lastModified; + ItemModel.prototype.clear = function() { + this._guidFeedIdHash = {}; + return ItemModel.__super__.clear.call(this); + }; + + ItemModel.prototype.add = function(data, clearCache) { + var entry, hash, key, value, _results; + if (clearCache == null) { + clearCache = true; + } + hash = data.feedId + '_' + data.guidHash; + entry = this._guidFeedIdHash[hash]; + if (angular.isDefined(entry)) { + delete this._dataMap[entry.id]; + this._dataMap[data.id] = entry; + _results = []; + for (key in data) { + value = data[key]; + if (key === 'feedId' || key === 'guidHash') { + continue; + } else { + _results.push(entry[key] = value); + } + } + return _results; } else { - return null; + this._guidFeedIdHash[hash] = data; + return ItemModel.__super__.add.call(this, data, clearCache); } }; diff --git a/js/tests/controllers/feedcontrollerSpec.coffee b/js/tests/controllers/feedcontrollerSpec.coffee index 8ef621030..6ec75e920 100644 --- a/js/tests/controllers/feedcontrollerSpec.coffee +++ b/js/tests/controllers/feedcontrollerSpec.coffee @@ -152,7 +152,7 @@ describe '_FeedController', -> @ActiveFeed.handle({id: 3, type: 3}) @scope.loadFeed(3, 3) - expect(@persistence.getItems).toHaveBeenCalledWith(3, 3, 0, null, 323) + expect(@persistence.getItems).toHaveBeenCalledWith(3, 3, 0, null, 6) it 'should send a get all items query when feed changed', => @@ -232,9 +232,9 @@ describe '_FeedController', -> it 'should mark feed as read', => @persistence.setFeedRead = jasmine.createSpy('setFeedRead') @FeedModel.add({id: 5, unreadCount:2, folderId: 2}) - @ItemModel.add({id: 6}) - @ItemModel.add({id: 3}) - @ItemModel.add({id: 2}) + @ItemModel.add({id: 6, feedId: 5, guidHash: 'a'}) + @ItemModel.add({id: 3, feedId: 5, guidHash: 'a1'}) + @ItemModel.add({id: 2, feedId: 5, guidHash: 'a2'}) @scope.markAllRead(@FeedType.Feed, 5) expect(@persistence.setFeedRead).toHaveBeenCalledWith(5, 6) diff --git a/js/tests/services/models/itemmodelSpec.coffee b/js/tests/services/models/itemmodelSpec.coffee index 061f84f01..7d929b223 100644 --- a/js/tests/services/models/itemmodelSpec.coffee +++ b/js/tests/services/models/itemmodelSpec.coffee @@ -29,4 +29,31 @@ describe '_ItemModel', -> it 'should extend model', => - expect(new @_ItemModel instanceof @_Model).toBeTruthy()
\ No newline at end of file + expect(new @_ItemModel instanceof @_Model).toBeTruthy() + + + it 'should also update items with the same feed id and guidhash', => + model = new @_ItemModel() + item1 = {id: 4, guidHash: 'abc', feedId: 3} + model.add(item1) + + expect(model.getById(4)).toBe(item1) + + # normal id update + item2 = {id: 4, guidHash: 'abc', feedId: 4} + model.add(item2) + expect(model.size()).toBe(1) + + # new feeds should be added normally if different + item3 = {id: 5, guidHash: 'abc', feedId: 6} + model.add(item3) + expect(model.size()).toBe(2) + + # feed should be updated when guidhash and feedid the same + item4 = {id: 3, guidHash: 'abc', feedId: 6} + model.add(item4) + expect(model.getById(3).guidHash).toBe(item4.guidHash) + expect(model.getById(3).feedId).toBe(item4.feedId) + expect(model.getById(3).id).toBe(item4.id) + expect(model.getById(5)).toBe(undefined) + expect(model.size()).toBe(2)
\ No newline at end of file diff --git a/tests/bl/FeedBlTest.php b/tests/bl/FeedBlTest.php index b43060112..c970ac837 100644 --- a/tests/bl/FeedBlTest.php +++ b/tests/bl/FeedBlTest.php @@ -118,10 +118,10 @@ class FeedBlTest extends \OCA\AppFramework\Utility\TestUtility { ->will($this->returnValue($createdFeed)); $this->itemMapper->expects($this->at(0)) ->method('insert') - ->with($this->equalTo($return[1][0])); + ->with($this->equalTo($return[1][1])); $this->itemMapper->expects($this->at(1)) ->method('insert') - ->with($this->equalTo($return[1][1])); + ->with($this->equalTo($return[1][0])); $feed = $this->bl->create($url, $folderId, $this->user); @@ -168,7 +168,7 @@ class FeedBlTest extends \OCA\AppFramework\Utility\TestUtility { } - public function testUpdateUpdatesEntry(){ + public function testUpdateUpdatesEntryNotWhenPubDateSame(){ $feed = new Feed(); $feed->setId(3); $feed->getUrl('test'); @@ -176,6 +176,7 @@ class FeedBlTest extends \OCA\AppFramework\Utility\TestUtility { $item = new Item(); $item->setGuidHash(md5('hi')); + $item->setPubDate(3333); $items = array( $item ); @@ -200,13 +201,57 @@ class FeedBlTest extends \OCA\AppFramework\Utility\TestUtility { $this->equalTo($feed->getId()), $this->equalTo($this->user)) ->will($this->returnValue($item)); + + $this->bl->update($feed->getId(), $this->user); + } + + + + public function testUpdateUpdatesEntry(){ + $feed = new Feed(); + $feed->setId(3); + $feed->getUrl('test'); + $ex = new \DatabaseException(''); + + $item = new Item(); + $item->setGuidHash(md5('hi')); + $item->setPubDate(3333); + $items = array( + $item + ); + + $item2 = new Item(); + $item2->setPubDate(111); + + $fetchReturn = array($feed, $items); + + $this->mapper->expects($this->once()) + ->method('find') + ->with($this->equalTo($feed->getId()), + $this->equalTo($this->user)) + ->will($this->returnValue($feed)); + $this->fetcher->expects($this->once()) + ->method('fetch') + ->will($this->returnValue($fetchReturn)); + $this->itemMapper->expects($this->at(0)) + ->method('insert') + ->with($this->equalTo($items[0])) + ->will($this->throwException($ex)); $this->itemMapper->expects($this->once()) - ->method('update') + ->method('findByGuidHash') + ->with($this->equalTo($item->getGuidHash()), + $this->equalTo($feed->getId()), + $this->equalTo($this->user)) + ->will($this->returnValue($item2)); + $this->itemMapper->expects($this->at(2)) + ->method('insert') ->with($this->equalTo($item)); $this->bl->update($feed->getId(), $this->user); + $this->assertTrue($item->isUnread()); } + public function testCreateUpdateFails(){ $feed = new Feed(); $feed->setId(3); diff --git a/tests/bl/ItemBlTest.php b/tests/bl/ItemBlTest.php index dcc208705..225435b7d 100644 --- a/tests/bl/ItemBlTest.php +++ b/tests/bl/ItemBlTest.php @@ -230,12 +230,15 @@ class ItemBlTest extends \OCA\AppFramework\Utility\TestUtility { public function testReadFeed(){ $feedId = 3; + $highestItemId = 6; $this->mapper->expects($this->once()) ->method('readFeed') - ->with($this->equalTo($feedId), $this->equalTo($this->user)); + ->with($this->equalTo($feedId), + $this->equalTo($highestItemId), + $this->equalTo($this->user)); - $this->bl->readFeed($feedId, $this->user); + $this->bl->readFeed($feedId, $highestItemId, $this->user); } } diff --git a/tests/controller/ItemControllerTest.php b/tests/controller/ItemControllerTest.php index 0c9899d4f..3ab3de145 100644 --- a/tests/controller/ItemControllerTest.php +++ b/tests/controller/ItemControllerTest.php @@ -181,14 +181,17 @@ class ItemControllerTest extends ControllerTestUtility { $url = array( 'feedId' => 4 ); - $this->controller = $this->getPostController(array(), $url); + $post = array( + 'highestItemId' => 5 + ); + $this->controller = $this->getPostController($post, $url); $this->api->expects($this->once()) ->method('getUserId') ->will($this->returnValue($this->user)); $this->bl->expects($this->once()) ->method('readFeed') - ->with($url['feedId'], $this->user); + ->with($url['feedId'], $post['highestItemId'], $this->user); $this->controller->readFeed(); } diff --git a/tests/db/ItemMapperTest.php b/tests/db/ItemMapperTest.php index d8b76722b..ea0feadd8 100644 --- a/tests/db/ItemMapperTest.php +++ b/tests/db/ItemMapperTest.php @@ -119,15 +119,17 @@ class ItemMapperTest extends \OCA\AppFramework\Utility\MapperTestUtility { 'JOIN `*PREFIX*news_items` `items` ' . 'ON `items`.`feed_id` = `feeds`.`id` ' . 'AND `feeds`.`user_id` = ? ' . - 'SET `items`.`status` = (`items`.`status` & ?) ' . - 'WHERE `items`.`id` = ?'; - $this->setMapperResult($sql, array(~StatusFlag::UNREAD, $this->user, 3)); - $this->mapper->readFeed(3, $this->user); + 'AND `feeds`.`id` = ? ' . + 'AND `items`.`id` <= ? ' . + 'SET `items`.`status` = (`items`.`status` & ?) '; + $params = array($this->user, 3, 6, ~StatusFlag::UNREAD); + $this->setMapperResult($sql, $params); + $this->mapper->readFeed(3, 6, $this->user); } public function testFindAllNew(){ - $sql = 'AND `items`.`last_modified` >= ?'; + $sql = 'AND `items`.`id` >= ?'; $sql = $this->makeSelectQueryStatus($sql); $params = array($this->user, $this->status, $this->updatedSince); @@ -141,7 +143,7 @@ class ItemMapperTest extends \OCA\AppFramework\Utility\MapperTestUtility { public function testFindAllNewFeed(){ $sql = 'AND `items`.`feed_id` = ? ' . - 'AND `items`.`last_modified` >= ?'; + 'AND `items`.`id` >= ?'; $sql = $this->makeSelectQueryStatus($sql); $params = array($this->user, $this->status, $this->id, $this->updatedSince); @@ -155,7 +157,7 @@ class ItemMapperTest extends \OCA\AppFramework\Utility\MapperTestUtility { public function testFindAllNewFolder(){ $sql = 'AND `feeds`.`folder_id` = ? ' . - 'AND `items`.`last_modified` >= ?'; + 'AND `items`.`id` >= ?'; $sql = $this->makeSelectQueryStatus($sql); $params = array($this->user, $this->status, $this->id, |