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

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,