From 02ae36eba33a5e0957defd4619d337bfdd0c178f Mon Sep 17 00:00:00 2001 From: Bernhard Posselt Date: Wed, 27 Mar 2013 12:26:04 +0100 Subject: fixed mark all unread serverside (was missing highestitemid, dont use lastmodified to compare for new versions but use the highest item id. if items are updated and the guidHash and feedId are the same then it will be deleted and newly inserted to make the lastmodified feasable --- bl/feedbl.php | 30 ++++++++++++--- bl/itembl.php | 4 +- controller/itemcontroller.php | 3 +- db/itemmapper.php | 15 ++++---- js/app/controllers/feedcontroller.coffee | 4 +- js/app/services/models/itemmodel.coffee | 38 +++++++++++++++--- js/public/app.js | 39 ++++++++++++++----- js/tests/controllers/feedcontrollerSpec.coffee | 8 ++-- js/tests/services/models/itemmodelSpec.coffee | 29 +++++++++++++- tests/bl/FeedBlTest.php | 53 ++++++++++++++++++++++++-- tests/bl/ItemBlTest.php | 7 +++- tests/controller/ItemControllerTest.php | 7 +++- 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 . 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 . __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, -- cgit v1.2.3