Download remotely updated files in "as needed" file sync mode

Previously, files updated remotely wouldn't be downloaded in "as needed"
mode if a copy of the file already existed locally and could only be
re-downloaded by deleting the file via Show File.

This causes remotely modified files that exist locally to be downloaded
at sync time, even in "as needed" mode, by marking them as
"force_download". While this might not be ideal for people who use "as
needed" to limit data transfer, it's better for people who use it simply
to limit local storage, and ending up with an outdated file while
offline seems worse than a little bit of extra data transfer.

In the future, we'll likely also provide ways to explicitly download and
remove files, so keeping chosen files in sync makes sense.

Files modified remotely before this change (which were marked as
"to_download" instead of "force_download") won't be downloaded as sync
time in "as needed" mode, but they'll now be re-downloaded on open.

Fixes #1322

chrome/content/zotero/xpcom/storage/storageLocal.js

@@ -314,7 +314,7 @@ Zotero.Sync.Storage.Local = {
 	}),
 	
 	
-	_checkForUpdatedFile: Zotero.Promise.coroutine(function* (item, attachmentData, remoteModTime) {
+	_checkForUpdatedFile: Zotero.Promise.coroutine(function* (item, attachmentData) {
 		var lk = item.libraryKey;
 		Zotero.debug("Checking attachment file for item " + lk, 4);
 		
@@ -350,25 +350,7 @@ Zotero.Sync.Storage.Local = {
 			//Zotero.debug("Stored mtime is " + attachmentData.mtime);
 			//Zotero.debug("File mtime is " + fmtime);
 			
-			//BAIL AFTER DOWNLOAD MARKING MODE, OR CHECK LOCAL?
 			let mtime = attachmentData ? attachmentData.mtime : false;
-			
-			// Download-marking mode
-			if (remoteModTime) {
-				Zotero.debug(`Remote mod time for item ${lk} is ${remoteModTime}`);
-				
-				// Ignore attachments whose stored mod times haven't changed
-				mtime = mtime !== false ? mtime : item.attachmentSyncedModificationTime;
-				if (mtime == remoteModTime) {
-					Zotero.debug(`Synced mod time (${mtime}) hasn't changed for item ${lk}`);
-					return false;
-				}
-				
-				Zotero.debug(`Marking attachment ${lk} for download (stored mtime: ${mtime})`);
-				// DEBUG: Always set here, or allow further steps?
-				return this.SYNC_STATE_FORCE_DOWNLOAD;
-			}
-			
 			var same = !this.checkFileModTime(item, fmtime, mtime);
 			if (same) {
 				Zotero.debug("File has not changed");

chrome/content/zotero/xpcom/sync/syncLocal.js

@@ -1129,8 +1129,9 @@ Zotero.Sync.Data.Local = {
 	 * Check whether an attachment's file mod time matches the given mod time, and mark the file
 	 * for download if not (or if this is a new attachment)
 	 */
-	_checkAttachmentForDownload: Zotero.Promise.coroutine(function* (item, mtime, isNewObject) {
+	_checkAttachmentForDownload: async function (item, mtime, isNewObject) {
-		var markToDownload = false;
+		var markToDownload = true;
+		var fileExists = false;
 		if (!isNewObject) {
 			// Convert previously used Unix timestamps to ms-based timestamps
 			if (mtime < 10000000000) {
@@ -1139,7 +1140,7 @@ Zotero.Sync.Data.Local = {
 			}
 			var fmtime = null;
 			try {
-				fmtime = yield item.attachmentModificationTime;
+				fmtime = await item.attachmentModificationTime;
 			}
 			catch (e) {
 				// This will probably fail later too, but ignore it for now
@@ -1147,21 +1148,20 @@ Zotero.Sync.Data.Local = {
 			}
 			if (fmtime) {
 				let state = Zotero.Sync.Storage.Local.checkFileModTime(item, fmtime, mtime);
-				if (state !== false) {
+				if (state === false) {
-					markToDownload = true;
+					markToDownload = false;
 				}
+				fileExists = true;
 			}
-			else {
-				markToDownload = true;
-			}
-		}
-		else {
-			markToDownload = true;
 		}
 		if (markToDownload) {
-			item.attachmentSyncState = "to_download";
+			// If file already exists locally, download it even in "as needed" mode. While we could
+			// just check whether a download is necessary at file open, these are files that people
+			// have previously downloaded, and avoiding opening an outdated version seems more
+			// important than avoiding a little bit of extra data transfer.
+			item.attachmentSyncState = fileExists ? "force_download" : "to_download";
 		}
-	}),
+	},
 	
 	
 	/**

chrome/content/zotero/zoteroPane.js

@@ -4013,6 +4013,8 @@ var ZoteroPane = new function()
 				throw new Error("Item " + itemID + " is not an attachment");
 			}
 			
+			Zotero.debug("Viewing attachment " + item.libraryKey);
+			
 			if (item.attachmentLinkMode == Zotero.Attachments.LINK_MODE_LINKED_URL) {
 				this.loadURI(item.getField('url'), event);
 				continue;
@@ -4087,15 +4089,29 @@ var ZoteroPane = new function()
 				}
 			}
 			
-			if (fileExists) {
+			let fileSyncingEnabled = Zotero.Sync.Storage.Local.getEnabledForLibrary(item.libraryID);
+			let redownload = false;
+			
+			// TEMP: If file is queued for download, download first. Starting in 5.0.85, files
+			// modified remotely get marked as SYNC_STATE_FORCE_DOWNLOAD, causing them to get
+			// downloaded at sync time even in download-as-needed mode, but this causes files
+			// modified previously to be downloaded on open.
+			if (fileExists
+					&& !isLinkedFile
+					&& fileSyncingEnabled
+					&& (item.attachmentSyncState == Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD)) {
+				Zotero.debug("File exists but is queued for download -- re-downloading");
+				redownload = true;
+			}
+			
+			if (fileExists && !redownload) {
 				Zotero.debug("Opening " + path);
 				Zotero.Notifier.trigger('open', 'file', item.id);
-				
 				launchFile(path, item.attachmentContentType);
 				continue;
 			}
 			
-			if (isLinkedFile || !Zotero.Sync.Storage.Local.getEnabledForLibrary(item.libraryID)) {
+			if (isLinkedFile || !fileSyncingEnabled) {
 				this.showAttachmentNotFoundDialog(
 					itemID,
 					path,
@@ -4109,7 +4125,10 @@ var ZoteroPane = new function()
 			}
 			
 			try {
-				await Zotero.Sync.Runner.downloadFile(item);
+				let results = await Zotero.Sync.Runner.downloadFile(item);
+				if (!results || !results.localChanges) {
+					Zotero.debug("Download failed -- opening existing file");
+				}
 			}
 			catch (e) {
 				// TODO: show error somewhere else
@@ -4130,12 +4149,11 @@ var ZoteroPane = new function()
 				return;
 			}
 			
-			// check if unchanged?
-			// maybe not necessary, since we'll get an error if there's an error
-			
 			Zotero.Notifier.trigger('redraw', 'item', []);
-			// Retry after download
+			
-			i--;
+			Zotero.debug("Opening " + path);
+			Zotero.Notifier.trigger('open', 'file', item.id);
+			launchFile(path, item.attachmentContentType);
 		}
 	});
 	

test/tests/syncLocalTest.js

@@ -745,7 +745,7 @@ describe("Zotero.Sync.Data.Local", function() {
 			assert.isTrue(library.storageDownloadNeeded);
 		})
 		
-		it("should mark updated attachment items for download", function* () {
+		it("should mark remotely updated attachment item for forced download", function* () {
 			var library = Zotero.Libraries.userLibrary;
 			var libraryID = library.id;
 			Zotero.Sync.Storage.Local.setModeForLibrary(libraryID, 'zfs');
@@ -771,10 +771,43 @@ describe("Zotero.Sync.Data.Local", function() {
 				'item', libraryID, [json], { stopOnError: true }
 			);
 			
-			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD);
+			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_FORCE_DOWNLOAD);
 			assert.isTrue(library.storageDownloadNeeded);
 		})
 		
+		it("should mark remotely updated attachment item with missing file for download", async function () {
+			var library = Zotero.Libraries.userLibrary;
+			var libraryID = library.id;
+			Zotero.Sync.Storage.Local.setModeForLibrary(libraryID, 'zfs');
+			
+			var item = await importFileAttachment('test.png');
+			item.version = 5;
+			item.synced = true;
+			await item.saveTx();
+			
+			// Set file as synced
+			item.attachmentSyncedModificationTime = await item.attachmentModificationTime;
+			item.attachmentSyncedHash = await item.attachmentHash;
+			item.attachmentSyncState = "in_sync";
+			await item.saveTx({ skipAll: true });
+			
+			// Delete file
+			await OS.File.remove(item.getFilePath());
+			
+			// Simulate download of version with updated attachment
+			var json = item.toResponseJSON();
+			json.version = 10;
+			json.data.version = 10;
+			json.data.md5 = '57f8a4fda823187b91e1191487b87fe6';
+			json.data.mtime = new Date().getTime() + 10000;
+			await Zotero.Sync.Data.Local.processObjectsFromJSON(
+				'item', libraryID, [json], { stopOnError: true }
+			);
+			
+			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD);
+			assert.isTrue(library.storageDownloadNeeded);
+		});
+		
 		it("should ignore attachment metadata when resolving metadata conflict", function* () {
 			var libraryID = Zotero.Libraries.userLibraryID;
 			Zotero.Sync.Storage.Local.setModeForLibrary(libraryID, 'zfs');
@@ -806,7 +839,7 @@ describe("Zotero.Sync.Data.Local", function() {
 			);
 			
 			assert.equal(item.getField('title'), newTitle);
-			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD);
+			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_FORCE_DOWNLOAD);
 		})
 		
 		it("should roll back partial object changes on error", function* () {

test/tests/zoteroPaneTest.js

@@ -236,6 +236,100 @@ describe("ZoteroPane", function() {
 			yield downloadOnDemand();
 		});
 		
+		// As noted in viewAttachment(), this is only necessary for files modified before 5.0.85
+		it("should re-download a remotely modified attachment in as-needed mode", async function () {
+			await setup();
+			Zotero.Sync.Storage.Local.downloadAsNeeded(Zotero.Libraries.userLibraryID, true);
+			
+			var item = await importFileAttachment('test.txt');
+			item.attachmentSyncState = "to_download";
+			await item.saveTx();
+			
+			var text = Zotero.Utilities.randomString();
+			var mtime = "1441252524000";
+			var md5 = Zotero.Utilities.Internal.md5(text)
+			
+			var s3Path = `pretend-s3/${item.key}`;
+			httpd.registerPathHandler(
+				`/users/1/items/${item.key}/file`,
+				{
+					handle: function (request, response) {
+						response.setStatusLine(null, 302, "Found");
+						response.setHeader("Zotero-File-Modification-Time", mtime, false);
+						response.setHeader("Zotero-File-MD5", md5, false);
+						response.setHeader("Zotero-File-Compressed", "No", false);
+						response.setHeader("Location", baseURL + s3Path, false);
+					}
+				}
+			);
+			httpd.registerPathHandler(
+				"/" + s3Path,
+				{
+					handle: function (request, response) {
+						response.setStatusLine(null, 200, "OK");
+						response.write(text);
+					}
+				}
+			);
+			
+			// Disable loadURI() so viewAttachment() doesn't trigger translator loading
+			var downloadSpy = sinon.spy(Zotero.Sync.Runner, "downloadFile");
+			var launchFileStub = sinon.stub(Zotero, "launchFile");
+			
+			await zp.viewAttachment(item.id);
+			
+			assert.ok(downloadSpy.calledOnce);
+			assert.ok(launchFileStub.calledOnce);
+			assert.ok(launchFileStub.calledWith(item.getFilePath()));
+			downloadSpy.restore();
+			launchFileStub.restore();
+			
+			assert.equal(await item.attachmentHash, md5);
+			assert.equal(await item.attachmentModificationTime, mtime);
+			var path = await item.getFilePathAsync();
+			assert.equal(await Zotero.File.getContentsAsync(path), text);
+		});
+		
+		it("should handle a 404 when re-downloading a remotely modified attachment in as-needed mode", async function () {
+			await setup();
+			Zotero.Sync.Storage.Local.downloadAsNeeded(Zotero.Libraries.userLibraryID, true);
+			
+			var item = await importFileAttachment('test.txt');
+			item.attachmentSyncState = "to_download";
+			await item.saveTx();
+			
+			var mtime = await item.attachmentModificationTime;
+			var md5 = await item.attachmentHash;
+			var text = await Zotero.File.getContentsAsync(item.getFilePath());
+			
+			httpd.registerPathHandler(
+				`/users/1/items/${item.key}/file`,
+				{
+					handle: function (request, response) {
+						response.setStatusLine(null, 404, "Not Found");
+					}
+				}
+			);
+			
+			// Disable loadURI() so viewAttachment() doesn't trigger translator loading
+			var downloadSpy = sinon.spy(Zotero.Sync.Runner, "downloadFile");
+			var launchFileStub = sinon.stub(Zotero, "launchFile");
+			
+			await zp.viewAttachment(item.id);
+			
+			assert.ok(downloadSpy.calledOnce);
+			assert.ok(launchFileStub.calledOnce);
+			assert.ok(launchFileStub.calledWith(item.getFilePath()));
+			downloadSpy.restore();
+			launchFileStub.restore();
+			
+			// File shouldn't have changed
+			assert.equal(await item.attachmentModificationTime, mtime);
+			assert.equal(await item.attachmentHash, md5);
+			var path = await item.getFilePathAsync();
+			assert.equal(await Zotero.File.getContentsAsync(path), text);
+		});
+		
 		it("should download an attachment on-demand in at-sync-time mode", function* () {
 			yield setup();
 			Zotero.Sync.Storage.Local.downloadOnSync(Zotero.Libraries.userLibraryID, true);