From 1efe54e896da3a11ad4ad69b208ebe00c5d25368 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Thu, 17 Aug 2017 15:42:18 +0200
Subject: [PATCH] Fix #1286, Show proper error messages for OS.File errors

---
 chrome/content/zotero/xpcom/file.js           | 75 ++-----------------
 .../zotero/xpcom/storage/storageLocal.js      |  2 +-
 test/tests/fileTest.js                        | 23 ++++++
 3 files changed, 32 insertions(+), 68 deletions(-)

diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js
index 9405af4c7..6e2755b03 100644
--- a/chrome/content/zotero/xpcom/file.js
+++ b/chrome/content/zotero/xpcom/file.js
@@ -1168,76 +1168,17 @@ Zotero.File = new function(){
 		
 		if (e.name == 'NS_ERROR_FILE_ACCESS_DENIED' || e.name == 'NS_ERROR_FILE_IS_LOCKED'
 				// These show up on some Windows systems
-				|| e.name == 'NS_ERROR_FAILURE' || e.name == 'NS_ERROR_FILE_NOT_FOUND') {
-			str = str + " " + Zotero.getString('file.accessError.cannotBe') + " " + opWord + ".";
-			var checkFileWindows = Zotero.getString('file.accessError.message.windows');
-			var checkFileOther = Zotero.getString('file.accessError.message.other');
-			var msg = str + "\n\n"
-					+ (Zotero.isWin ? checkFileWindows : checkFileOther)
-					+ "\n\n"
-					+ Zotero.getString('file.accessError.restart');
-			
-			var e = new Zotero.Error(
-				msg,
-				0,
-				{
-					dialogButtonText: Zotero.getString('file.accessError.showParentDir'),
-					dialogButtonCallback: function () {
-						try {
-							file.parent.QueryInterface(Components.interfaces.nsILocalFile);
-							file.parent.reveal();
-						}
-						// Unsupported on some platforms
-						catch (e2) {
-							Zotero.launchFile(file.parent);
-						}
-					}
-				}
-			);
-		}
-		
-		throw (e);
-	}
-	
-	
-	this.checkPathAccessError = function (e, path, operation) {
-		var str = 'file.accessError.';
-		if (path) {
-			str += 'theFile'
-		}
-		else {
-			str += 'aFile'
-		}
-		str += 'CannotBe';
-		
-		switch (operation) {
-			case 'create':
-				str += 'Created';
-				break;
-				
-			case 'delete':
-				str += 'Deleted';
-				break;
-				
-			default:
-				str += 'Updated';
-		}
-		str = Zotero.getString(str, path ? path : undefined);
-		
-		Zotero.debug(path);
-		Zotero.debug(e, 1);
-		Components.utils.reportError(e);
-		
-		// TODO: Check for specific errors?
-		if (e instanceof OS.File.Error) {
+				|| e.name == 'NS_ERROR_FAILURE' || e.name == 'NS_ERROR_FILE_NOT_FOUND'
+				// OS.File.Error
+				|| e.becauseAccessDenied || e.becauseNoSuchFile) {
 			let checkFileWindows = Zotero.getString('file.accessError.message.windows');
 			let checkFileOther = Zotero.getString('file.accessError.message.other');
-			var msg = str + "\n\n"
+			let msg = str + "\n\n"
 					+ (Zotero.isWin ? checkFileWindows : checkFileOther)
 					+ "\n\n"
 					+ Zotero.getString('file.accessError.restart');
 			
-			var e = new Zotero.Error(
+			e = new Zotero.Error(
 				msg,
 				0,
 				{
@@ -1248,7 +1189,7 @@ Zotero.File = new function(){
 							file.parent.reveal();
 						}
 						// Unsupported on some platforms
-						catch (e2) {
+						catch (e) {
 							Zotero.launchFile(file.parent);
 						}
 					}
@@ -1258,8 +1199,8 @@ Zotero.File = new function(){
 		
 		throw e;
 	}
-
-
+	
+	
 	this.isDropboxDirectory = function(path) {
 		return path.toLowerCase().indexOf('dropbox') != -1;
 	}
diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js
index 04a289500..f37822a77 100644
--- a/chrome/content/zotero/xpcom/storage/storageLocal.js
+++ b/chrome/content/zotero/xpcom/storage/storageLocal.js
@@ -367,7 +367,7 @@ Zotero.Sync.Storage.Local = {
 					yield OS.File.setDates(path, null, mtime);
 				}
 				catch (e) {
-					Zotero.File.checkPathAccessError(e, path, 'update');
+					Zotero.File.checkFileAccessError(e, path, 'update');
 				}
 				return false;
 			}
diff --git a/test/tests/fileTest.js b/test/tests/fileTest.js
index 8568d7f81..3969ae4aa 100644
--- a/test/tests/fileTest.js
+++ b/test/tests/fileTest.js
@@ -188,4 +188,27 @@ describe("Zotero.File", function () {
 			assert.propertyVal(files, 'sub/b.txt', 'B');
 		});
 	});
+	
+	
+	describe("#checkFileAccessError()", function () {
+		it("should catch OS.File access-denied errors", function* () {
+			// We can't modify a real OS.File.Error, but we also don't do an instanceof check in
+			// checkFileAccessError, so just set the expected properties.
+			var e = {
+				operation: 'open',
+				becauseAccessDenied: true,
+				path: '/tmp/test'
+			};
+			try {
+				Zotero.File.checkFileAccessError(e, e.path, 'create');
+			}
+			catch (e) {
+				if (e instanceof Zotero.Error) {
+					return;
+				}
+				throw e;
+			}
+			throw new Error("Error not thrown");
+		});
+	});
 })