Fix 'since' for real

'since=[n]' should work as expected, since=0 should be allowed, and the absence of 'since' for /deleted should be an error. Follow-up to 34722eb and 8d0c0ca
VivienDelmon · Jan 21, 2020 · 48e17d6 · 48e17d6
1 parent 8d0c0ca
commit 48e17d6
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 21 deletions.
diff --git a/controllers/DeletedController.php b/controllers/DeletedController.php
@@ -41,7 +41,7 @@ public function deleted() {
 		$this->libraryVersion = Zotero_Libraries::getUpdatedVersion($this->objectLibraryID);
 
 		// TEMP: sync transition
-		if (!empty($this->queryParams['sincetime'])) {
+		if ($this->queryParams['sincetime'] !== false) {
 			$deleted = array(
 				"collections" => Zotero_Collections::getDeleteLogKeys(
 					$this->objectLibraryID, $this->queryParams['sincetime'], true
@@ -63,7 +63,7 @@ public function deleted() {
 			$this->end();
 		}
 
-		if (empty($this->queryParams['since'])) {
+		if ($this->queryParams['since'] === false) {
 			$this->e400("'since' parameter must be provided");
 		}
 

diff --git a/model/API.inc.php b/model/API.inc.php
@@ -88,8 +88,8 @@ class Zotero_API {
 		'searchKey' => [],
 		'tag' => '',
 		'tagType' => '',
-		'since' => 0,
-		'sincetime' => 0,
+		'since' => false,
+		'sincetime' => false,
 
 		// Tags within items
 		'itemQ' => '',
@@ -206,19 +206,13 @@ public static function parseQueryParams($queryString, $action, $singleObject, $a
 			unset($queryParams['order']);
 		}
 
-		// Handle deprecated (in v3) 'newer' and 'newertime' parameters
+		// Handle deprecated (in v3) 'newer' parameter
 		if (isset($queryParams['newer'])) {
 			if (!isset($queryParams['since'])) {
 				$queryParams['since'] = $queryParams['newer'];
 			}
 			unset($queryParams['newer']);
 		}
-		if (isset($queryParams['newertime'])) {
-			if (!isset($queryParams['sincetime'])) {
-				$queryParams['sincetime'] = $queryParams['newertime'];
-			}
-			unset($queryParams['newertime']);
-		}
 
 		foreach (self::resolveDefaultParams($action, self::$defaultParams, $queryParams) as $key => $value) {
 			// Don't overwrite field if already set (either above or derived from another field)

diff --git a/tests/remote/tests/API/3/ObjectTest.php b/tests/remote/tests/API/3/ObjectTest.php
@@ -158,6 +158,12 @@ public function testDeleted() {
 		$tempLibraryVersion = $func('item', $tempLibraryVersion);
 		$libraryVersion3 = $func('search', $tempLibraryVersion);
 
+		// /deleted without 'since' should be an error
+		$response = API::userGet(
+			self::$config['userID'],
+			"deleted?key=" . self::$config['apiKey']
+		);
+		$this->assert400($response);
 
 		// Request all deleted objects
 		$response = API::userGet(
@@ -171,14 +177,34 @@ public function testDeleted() {
 		$this->assertContentType("application/json", $response);
 
 		// Make sure 'newer' is equivalent
-		$responseNewer = API::userGet(
+		$responseAlt = API::userGet(
 			self::$config['userID'],
 			"deleted?key=" . self::$config['apiKey'] . "&newer=$libraryVersion1"
 		);
-		$this->assertEquals($response->getStatus(), $responseNewer->getStatus());
-		$this->assertEquals($response->getBody(), $responseNewer->getBody());
-		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseNewer->getHeader('Last-Modified-Version'));
-		$this->assertEquals($response->getHeader('Content-Type'), $responseNewer->getHeader('Content-Type'));
+		$this->assert200($responseAlt);
+		$this->assertEquals($response->getBody(), $responseAlt->getBody());
+		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseAlt->getHeader('Last-Modified-Version'));
+		$this->assertEquals($response->getHeader('Content-Type'), $responseAlt->getHeader('Content-Type'));
+
+		// Make sure 'since=0' is equivalent
+		$responseAlt = API::userGet(
+			self::$config['userID'],
+			"deleted?key=" . self::$config['apiKey'] . "&since=0"
+		);
+		$this->assert200($responseAlt);
+		$this->assertEquals($response->getBody(), $responseAlt->getBody());
+		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseAlt->getHeader('Last-Modified-Version'));
+		$this->assertEquals($response->getHeader('Content-Type'), $responseAlt->getHeader('Content-Type'));
+
+		// Make sure 'newer=0' is equivalent
+		$responseAlt = API::userGet(
+			self::$config['userID'],
+			"deleted?key=" . self::$config['apiKey'] . "&newer=0"
+		);
+		$this->assert200($responseAlt);
+		$this->assertEquals($response->getBody(), $responseAlt->getBody());
+		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseAlt->getHeader('Last-Modified-Version'));
+		$this->assertEquals($response->getHeader('Content-Type'), $responseAlt->getHeader('Content-Type'));
 
 		// Verify keys
 		$func = function ($json, $objectType, $objectKeys) use ($self) {
@@ -208,14 +234,14 @@ public function testDeleted() {
 		$this->assertContentType("application/json", $response);
 
 		// Make sure 'newer' is equivalent
-		$responseNewer = API::userGet(
+		$responseAlt = API::userGet(
 			self::$config['userID'],
 			"deleted?key=" . self::$config['apiKey'] . "&newer=$libraryVersion2"
 		);
-		$this->assertEquals($response->getStatus(), $responseNewer->getStatus());
-		$this->assertEquals($response->getBody(), $responseNewer->getBody());
-		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseNewer->getHeader('Last-Modified-Version'));
-		$this->assertEquals($response->getHeader('Content-Type'), $responseNewer->getHeader('Content-Type'));
+		$this->assert200($responseAlt);
+		$this->assertEquals($response->getBody(), $responseAlt->getBody());
+		$this->assertEquals($response->getHeader('Last-Modified-Version'), $responseAlt->getHeader('Last-Modified-Version'));
+		$this->assertEquals($response->getHeader('Content-Type'), $responseAlt->getHeader('Content-Type'));
 
 		// Verify keys
 		$func = function ($json, $objectType, $objectKeys) use ($self) {