Fix checking for file sync downloads at sync time

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

@@ -120,15 +120,20 @@ Zotero.Sync.Storage.Engine.prototype.start = Zotero.Promise.coroutine(function*
 		yield this.controller.cacheCredentials();
 	}
 	
-	// Get library last-sync time for download-on-sync libraries.
 	var lastSyncTime = null;
 	var downloadAll = this.local.downloadOnSync(libraryID);
-	if (downloadAll) {
+	// After all library data is downloaded during a data sync, the library version is updated to match
-		if (!this.library.storageDownloadNeeded) {
+	// the server version. We track a library version for file syncing separately, so that even if Zotero
+	// is closed or interrupted between a data sync and a file sync, we know that file syncing has to be
+	// performed for any files marked for download during data sync (based on outdated mtime/md5). Files
+	// may be missing remotely, though, so it's only necessary to try to download them once every time
+	// there are remote storage changes, which we indicate with a flag set in syncLocal.
+	//
+	// TODO: If files are persistently missing, don't try to download them each time
+	if (downloadAll && !this.library.storageDownloadNeeded) {
 		this.library.storageVersion = this.library.libraryVersion;
 		yield this.library.saveTx();
 	}
-	}
 	
 	// Check for updated files to upload
 	if (!Zotero.Libraries.isFilesEditable(libraryID)) {

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

@@ -611,15 +611,6 @@ Zotero.Sync.Data.Local = {
 						if (obj) {
 							Zotero.debug("Matching local " + objectType + " exists", 4);
 							
-							// Local object has been modified since last sync
-							if (!obj.synced) {
-								Zotero.debug("Local " + objectType + " " + obj.libraryKey
-										+ " has been modified since last sync", 4);
-								
-								let cachedJSON = yield this.getCacheObject(
-									objectType, obj.libraryID, obj.key, obj.version
-								);
-								
 							let jsonDataLocal = obj.toJSON();
 							
 							// For items, check if mtime or file hash changed in metadata,
@@ -632,6 +623,15 @@ Zotero.Sync.Data.Local = {
 								}
 							}
 							
+							// Local object has been modified since last sync
+							if (!obj.synced) {
+								Zotero.debug("Local " + objectType + " " + obj.libraryKey
+										+ " has been modified since last sync", 4);
+								
+								let cachedJSON = yield this.getCacheObject(
+									objectType, obj.libraryID, obj.key, obj.version
+								);
+								
 								let result = this._reconcileChanges(
 									objectType,
 									cachedJSON.data,
@@ -701,7 +701,7 @@ Zotero.Sync.Data.Local = {
 						else {
 							Zotero.debug(ObjectType + " doesn't exist locally");
 							
-							isNewObject = true;
+							saveOptions.isNewObject = true;
 							
 							// Check if object has been deleted locally
 							let dateDeleted = yield this.getDateDeleted(

test/tests/syncLocalTest.js

@@ -275,8 +275,9 @@ describe("Zotero.Sync.Data.Local", function() {
 			assert.notInclude(versions, key);
 		});
 		
-		it("should mark new attachment items for download", function* () {
+		it("should mark new attachment items and library for download", function* () {
-			var libraryID = Zotero.Libraries.userLibraryID;
+			var library = Zotero.Libraries.userLibrary;
+			var libraryID = library.id;
 			Zotero.Sync.Storage.Local.setModeForLibrary(libraryID, 'zfs');
 			
 			var key = Zotero.DataObjectUtilities.generateKey();
@@ -299,10 +300,12 @@ describe("Zotero.Sync.Data.Local", function() {
 			);
 			var item = Zotero.Items.getByLibraryAndKey(libraryID, key);
 			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD);
+			assert.isTrue(library.storageDownloadNeeded);
 		})
 		
 		it("should mark updated attachment items for download", function* () {
-			var libraryID = Zotero.Libraries.userLibraryID;
+			var library = Zotero.Libraries.userLibrary;
+			var libraryID = library.id;
 			Zotero.Sync.Storage.Local.setModeForLibrary(libraryID, 'zfs');
 			
 			var item = yield importFileAttachment('test.png');
@@ -327,6 +330,7 @@ describe("Zotero.Sync.Data.Local", function() {
 			);
 			
 			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* () {