From 78b1d2ee35ed56420bfb4f34b73d9971365e5e71 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 9 Dec 2016 04:36:42 -0500
Subject: [PATCH] Import translation improvements

- Don't block the UI with a progress meter during imports. Instead, show
  a popup in the bottom right when the import is done that shows how
  many items were saved.
- Fix hang when importing some files
- Fix various problems with asynchronous operations/transactions
    - Use the save queue for imports instead of creating concurrent
      transactions that can time out
    - Wait for the save to finish before returning from the translate()
      promise. All save modes now use the save queue, so code that
      handled the non-save-queue process can probably be removed.
    - Serialize child attachments instead of running them concurrently.
      This might make multi-attachment saves a little slower, since they
      can't download at the same time, but it avoids problems with
      concurrent transactions. We might be able to improve this to allow
      concurrent downloads, or allow concurrent saves for a limited
      number of items (e.g., from web saving) if not for larger imports.
- Change collection handling during import, since UI is now active
  - Select the root collection at the beginning of the import
  - Assign items and collections to the root during the import instead
    of at the end
  - Don't select other collections
- Change a few ItemSaver functions to use promises and remove
  unnecessary callbacks. (This includes some connector code that needs
  to be tested.)
- Change some `parentID` variables in ItemSaver to `parentItemID` for
  clarity, since collections are now handled in more places

To-do:

- Save items in smaller batches instead of doing all in the same
  transaction
- Show progress meter in a bottom-right popup during the import
---
 chrome/content/zotero/fileInterface.js        |  50 ++---
 .../zotero/xpcom/connector/translate_item.js  |  62 +++---
 .../content/zotero/xpcom/server_connector.js  |  40 ++--
 .../zotero/xpcom/translation/translate.js     | 126 ++++++-----
 .../xpcom/translation/translate_item.js       | 204 ++++++++++--------
 chrome/locale/en-US/zotero/zotero.properties  |   3 +-
 test/tests/translateTest.js                   |  86 ++++++++
 7 files changed, 327 insertions(+), 244 deletions(-)

diff --git a/chrome/content/zotero/fileInterface.js b/chrome/content/zotero/fileInterface.js
index 4c446ccdf0..4a36066a55 100644
--- a/chrome/content/zotero/fileInterface.js
+++ b/chrome/content/zotero/fileInterface.js
@@ -320,44 +320,40 @@ var Zotero_File_Interface = new function() {
 		}
 
 		translation.setTranslator(translators[0]);
-		translation.setHandler("itemDone",  function () {
+		// TODO: Restore a progress meter
+		/*translation.setHandler("itemDone",  function () {
 			Zotero.updateZoteroPaneProgressMeter(translation.getProgress());
-		});
-
-		// show progress indicator
-		Zotero_File_Interface.Progress.show(
-			Zotero.getString("fileInterface.itemsImported")
-		);
+		});*/
 
 		yield Zotero.Promise.delay(0);
 
 		let failed = false;
 		try {
-			yield translation.translate(libraryID);
+			yield translation.translate({
+				libraryID,
+				collections: importCollection ? [importCollection.id] : null
+			});
 		} catch(e) {
 			Zotero.logError(e);
-			failed = true;
-		}
-		Zotero_File_Interface.Progress.close();
-
-		// Add items to import collection
-		if(importCollection) {
-			yield Zotero.DB.executeTransaction(function* () {
-				yield importCollection.addItems(translation.newItems.map(item => item.id));
-				for(let i=0; i<translation.newCollections.length; i++) {
-					let collection = translation.newCollections[i];
-					collection.parent = importCollection.id;
-					yield collection.save();
-				}
-			});
-			// 	// TODO: yield or change to .queue()
-			// 	Zotero.Notifier.trigger('refresh', 'collection', importCollection.id);
-		}
-
-		if(failed) {
 			window.alert(Zotero.getString("fileInterface.importError"));
 			return;
 		}
+		
+		// Show popup on completion
+		var numItems = translation.newItems.length;
+		translation.newItems.forEach(item => numItems += item.numChildren());
+		var progressWin = new Zotero.ProgressWindow();
+		progressWin.changeHeadline(Zotero.getString('fileInterface.importComplete'));
+		if (numItems == 1) {
+			var icon = translation.newItems[0].getImageSrc();
+		}
+		else {
+			var icon = 'chrome://zotero/skin/treesource-unfiled' + (Zotero.hiDPI ? "@2x" : "") + '.png';
+		}
+		var title = Zotero.getString(`fileInterface.itemsWereImported`, numItems, numItems);
+		progressWin.addLines(title, icon)
+		progressWin.show();
+		progressWin.startCloseTimer();
 	});
 	
 	/*
diff --git a/chrome/content/zotero/xpcom/connector/translate_item.js b/chrome/content/zotero/xpcom/connector/translate_item.js
index d9012a494c..a3b9339493 100644
--- a/chrome/content/zotero/xpcom/connector/translate_item.js
+++ b/chrome/content/zotero/xpcom/connector/translate_item.js
@@ -73,16 +73,12 @@ Zotero.Translate.ItemSaver.prototype = {
 	/**
 	 * Saves items to Standalone or the server
 	 * @param items Items in Zotero.Item.toArray() format
-	 * @param {Function} callback A callback to be executed when saving is complete. If saving
-	 *    succeeded, this callback will be passed true as the first argument and a list of items
-	 *    saved as the second. If saving failed, the callback will be passed false as the first
-	 *    argument and an error object as the second
 	 * @param {Function} [attachmentCallback] A callback that receives information about attachment
 	 *     save progress. The callback will be called as attachmentCallback(attachment, false, error)
 	 *     on failure or attachmentCallback(attachment, progressPercent) periodically during saving.
 	 */
-	"saveItems":function(items, callback, attachmentCallback) {
-		var me = this;
+	saveItems: function (items, attachmentCallback) {
+		var deferred = Zotero.Promise.defer();
 		// first try to save items via connector
 		var payload = {"items":items};
 		Zotero.Connector.setCookiesThenSaveItems(payload, function(data, status) {
@@ -100,14 +96,15 @@ Zotero.Translate.ItemSaver.prototype = {
 						}
 					}
 				}
-				callback(true, items);
-				if(haveAttachments) me._pollForProgress(items, attachmentCallback);
+				deferred.resolve(items);
+				if (haveAttachments) this._pollForProgress(items, attachmentCallback);
 			} else if(Zotero.isFx) {
-				callback(false, new Error("Save via Standalone failed with "+status));
+				deferred.reject(new Error("Save via Standalone failed with " + status));
 			} else {
-				me._saveToServer(items, callback, attachmentCallback);
+				deferred.resolve(this._saveToServer(items, attachmentCallback));
 			}
-		});
+		}.bind(this));
+		return deferred.promise;
 	},
 	
 	/**
@@ -169,16 +166,12 @@ Zotero.Translate.ItemSaver.prototype = {
 	/**
 	 * Saves items to server
 	 * @param items Items in Zotero.Item.toArray() format
-	 * @param {Function} callback A callback to be executed when saving is complete. If saving
-	 *    succeeded, this callback will be passed true as the first argument and a list of items
-	 *    saved as the second. If saving failed, the callback will be passed false as the first
-	 *    argument and an error object as the second
 	 * @param {Function} attachmentCallback A callback that receives information about attachment
 	 *     save progress. The callback will be called as attachmentCallback(attachment, false, error)
 	 *     on failure or attachmentCallback(attachment, progressPercent) periodically during saving.
 	 *     attachmentCallback() will be called with all attachments that will be saved 
 	 */
-	"_saveToServer":function(items, callback, attachmentCallback) {
+	_saveToServer: function (items, attachmentCallback) {
 		var newItems = [], itemIndices = [], typedArraysSupported = false;
 		try {
 			typedArraysSupported = !!(new Uint8Array(1) && new Blob());
@@ -197,41 +190,42 @@ Zotero.Translate.ItemSaver.prototype = {
 			}
 		}
 		
-		var me = this;
+		var deferred = Zotero.Promise.defer();
 		Zotero.API.createItem({"items":newItems}, function(statusCode, response) {
 			if(statusCode !== 200) {
-				callback(false, new Error("Save to server failed with "+statusCode+" "+response));
+				deferred.reject(new Error("Save to server failed with " + statusCode + " " + response));
 				return;
 			}
 
 			try {
 				var resp = JSON.parse(response);
 			} catch(e) {
-				callback(false, new Error("Unexpected response received from server"));
+				deferred.reject(new Error("Unexpected response received from server"));
 				return;
 			}
 			for(var i in resp.failed) {
-				callback(false, new Error("Save to server failed with "+statusCode+" "+response));
+				deferred.reject(new Error("Save to server failed with " + statusCode + " " + response));
 				return;
 			}
 			
 			Zotero.debug("Translate: Save to server complete");
-			Zotero.Prefs.getCallback(["downloadAssociatedFiles", "automaticSnapshots"],
-			function(prefs) {
-
-				if(typedArraysSupported) {
-					for(var i=0; i<items.length; i++) {
-						var item = items[i], key = resp.success[itemIndices[i]];
-						if(item.attachments && item.attachments.length) {
-							me._saveAttachmentsToServer(key, me._getFileBaseNameFromItem(item),
-								item.attachments, prefs, attachmentCallback);
+			Zotero.Prefs.getCallback(
+				["downloadAssociatedFiles", "automaticSnapshots"],
+				function (prefs) {
+					if(typedArraysSupported) {
+						for(var i=0; i<items.length; i++) {
+							var item = items[i], key = resp.success[itemIndices[i]];
+							if(item.attachments && item.attachments.length) {
+								this._saveAttachmentsToServer(key, this._getFileBaseNameFromItem(item),
+									item.attachments, prefs, attachmentCallback);
+							}
 						}
 					}
-				}
-				
-				callback(true, items);
-			});
-		});
+					deferred.resolve(items);
+				}.bind(this)
+			);
+		}.bind(this));
+		return deferred.promise;
 	},
 	
 	/**
diff --git a/chrome/content/zotero/xpcom/server_connector.js b/chrome/content/zotero/xpcom/server_connector.js
index ede4d09dfc..6a945d05fc 100644
--- a/chrome/content/zotero/xpcom/server_connector.js
+++ b/chrome/content/zotero/xpcom/server_connector.js
@@ -379,31 +379,27 @@ Zotero.Server.Connector.SaveItem.prototype = {
 			forceTagType: 1,
 			cookieSandbox
 		});
-		var deferred = Zotero.Promise.defer();
-		itemSaver.saveItems(data.items, function(returnValue, items) {
-			if(returnValue) {
-				try {
-					// Remove attachments not being saved from item.attachments
-					for(var i=0; i<data.items.length; i++) {
-						var item = data.items[i];
-						for(var j=0; j<item.attachments.length; j++) {
-							if(!Zotero.Server.Connector.AttachmentProgressManager.has(item.attachments[j])) {
-								item.attachments.splice(j--, 1);
-							}
-						}
+		try {
+			let items = yield itemSaver.saveItems(
+				data.items,
+				Zotero.Server.Connector.AttachmentProgressManager.onProgress
+			);
+			// Remove attachments not being saved from item.attachments
+			for(var i=0; i<data.items.length; i++) {
+				var item = data.items[i];
+				for(var j=0; j<item.attachments.length; j++) {
+					if(!Zotero.Server.Connector.AttachmentProgressManager.has(item.attachments[j])) {
+						item.attachments.splice(j--, 1);
 					}
-					
-					deferred.resolve([201, "application/json", JSON.stringify({items: data.items})]);
-				} catch(e) {
-					Zotero.logError(e);
-					deferred.resolve(500);
 				}
-			} else {
-				Zotero.logError(items);
-				deferred.resolve(500);
 			}
-		}, Zotero.Server.Connector.AttachmentProgressManager.onProgress);
-		return deferred.promise;
+			
+			return [201, "application/json", JSON.stringify({items: data.items})];
+		}
+		catch (e) {
+			Zotero.logError(e);
+			return 500;
+		}
 	})
 }
 
diff --git a/chrome/content/zotero/xpcom/translation/translate.js b/chrome/content/zotero/xpcom/translation/translate.js
index c5f58f5fc7..d2c560f003 100644
--- a/chrome/content/zotero/xpcom/translation/translate.js
+++ b/chrome/content/zotero/xpcom/translation/translate.js
@@ -199,13 +199,9 @@ Zotero.Translate.Sandbox = {
 			// Fire itemSaving event
 			translate._runHandler("itemSaving", item);
 			
-			if(translate instanceof Zotero.Translate.Web) {
-				// For web translators, we queue saves
-				translate.saveQueue.push(item);
-			} else {
-				// Save items
-				translate._saveItems([item]);
-			}
+			// TODO: This used to only be used for some modes. Since it's now used for everything with
+			// async saving, there's probably a bunch of code for the non-queued mode that can be removed.
+			translate.saveQueue.push(item);
 		},
 		
 		/**
@@ -1480,8 +1476,7 @@ Zotero.Translate.Base.prototype = {
 			if(returnValue) {
 				if(this.saveQueue.length) {
 					this._waitingForSave = true;
-					this._saveItems(this.saveQueue);
-					this.saveQueue = [];
+					this._saveItems(this.saveQueue).then(() => this.saveQueue = []);
 					return;
 				}
 				this._debug("Translation successful");
@@ -1540,69 +1535,70 @@ Zotero.Translate.Base.prototype = {
 	 * Saves items to the database, taking care to defer attachmentProgress notifications
 	 * until after save
 	 */
-	"_saveItems":function(items) {
-		var me = this,
-			itemDoneEventsDispatched = false,
-			deferredProgress = [],
-			attachmentsWithProgress = [];
+	_saveItems: function (items) {
+		var itemDoneEventsDispatched = false;
+		var deferredProgress = [];
+		var attachmentsWithProgress = [];
 		
 		this._savingItems++;
-		this._itemSaver.saveItems(items.slice(), function(returnValue, newItems) {	
-			if(returnValue) {
-				// Remove attachments not being saved from item.attachments
-				for(var i=0; i<items.length; i++) {
-					var item = items[i];
-					for(var j=0; j<item.attachments.length; j++) {
-						if(attachmentsWithProgress.indexOf(item.attachments[j]) === -1) {
-							item.attachments.splice(j--, 1);
-						}
+		return this._itemSaver.saveItems(
+			items.slice(),
+			function (attachment, progress, error) {
+				var attachmentIndex = this._savingAttachments.indexOf(attachment);
+				if(progress === false || progress === 100) {
+					if(attachmentIndex !== -1) {
+						this._savingAttachments.splice(attachmentIndex, 1);
+					}
+				} else if(attachmentIndex === -1) {
+					this._savingAttachments.push(attachment);
+				}
+				
+				if(itemDoneEventsDispatched) {
+					// itemDone event has already fired, so we can fire attachmentProgress
+					// notifications
+					this._runHandler("attachmentProgress", attachment, progress, error);
+					this._checkIfDone();
+				} else {
+					// Defer until after we fire the itemDone event
+					deferredProgress.push([attachment, progress, error]);
+					attachmentsWithProgress.push(attachment);
+				}
+			}.bind(this)
+		)
+		.then(function (newItems) {
+			// Remove attachments not being saved from item.attachments
+			for(var i=0; i<items.length; i++) {
+				var item = items[i];
+				for(var j=0; j<item.attachments.length; j++) {
+					if(attachmentsWithProgress.indexOf(item.attachments[j]) === -1) {
+						item.attachments.splice(j--, 1);
 					}
 				}
-				
-				// Trigger itemDone events
-				for(var i=0, nItems = items.length; i<nItems; i++) {
-					me._runHandler("itemDone", newItems[i], items[i]);
-				}
-				
-				// Specify that itemDone event was dispatched, so that we don't defer
-				// attachmentProgress notifications anymore
-				itemDoneEventsDispatched = true;
-				
-				// Run deferred attachmentProgress notifications
-				for(var i=0; i<deferredProgress.length; i++) {
-					me._runHandler("attachmentProgress", deferredProgress[i][0],
-						deferredProgress[i][1], deferredProgress[i][2]);
-				}
-				
-				me.newItems = me.newItems.concat(newItems);
-				me._savingItems--;
-				me._checkIfDone();
-			} else {
-				Zotero.logError(newItems);
-				me.complete(returnValue, newItems);
-			}
-		},
-		function(attachment, progress, error) {
-			var attachmentIndex = me._savingAttachments.indexOf(attachment);
-			if(progress === false || progress === 100) {
-				if(attachmentIndex !== -1) {
-					me._savingAttachments.splice(attachmentIndex, 1);
-				}
-			} else if(attachmentIndex === -1) {
-				me._savingAttachments.push(attachment);
 			}
 			
-			if(itemDoneEventsDispatched) {
-				// itemDone event has already fired, so we can fire attachmentProgress
-				// notifications
-				me._runHandler("attachmentProgress", attachment, progress, error);
-				me._checkIfDone();
-			} else {
-				// Defer until after we fire the itemDone event
-				deferredProgress.push([attachment, progress, error]);
-				attachmentsWithProgress.push(attachment);
+			// Trigger itemDone events
+			for(var i=0, nItems = items.length; i<nItems; i++) {
+				this._runHandler("itemDone", newItems[i], items[i]);
 			}
-		});
+			
+			// Specify that itemDone event was dispatched, so that we don't defer
+			// attachmentProgress notifications anymore
+			itemDoneEventsDispatched = true;
+			
+			// Run deferred attachmentProgress notifications
+			for(var i=0; i<deferredProgress.length; i++) {
+				this._runHandler("attachmentProgress", deferredProgress[i][0],
+					deferredProgress[i][1], deferredProgress[i][2]);
+			}
+			
+			this.newItems = this.newItems.concat(newItems);
+			this._savingItems--;
+			this._checkIfDone();
+		}.bind(this))
+		.catch(function (e) {
+			Zotero.logError(e);
+			this.complete(false, e);
+		}.bind(this));
 	},
 	
 	/**
diff --git a/chrome/content/zotero/xpcom/translation/translate_item.js b/chrome/content/zotero/xpcom/translation/translate_item.js
index f2d9caf3d0..d000dbdd47 100644
--- a/chrome/content/zotero/xpcom/translation/translate_item.js
+++ b/chrome/content/zotero/xpcom/translation/translate_item.js
@@ -74,94 +74,98 @@ Zotero.Translate.ItemSaver.prototype = {
 	/**
 	 * Saves items to Standalone or the server
 	 * @param items Items in Zotero.Item.toArray() format
-	 * @param {Function} callback A callback to be executed when saving is complete. If saving
-	 *    succeeded, this callback will be passed true as the first argument and a list of items
-	 *    saved as the second. If saving failed, the callback will be passed false as the first
-	 *    argument and an error object as the second
 	 * @param {Function} [attachmentCallback] A callback that receives information about attachment
 	 *     save progress. The callback will be called as attachmentCallback(attachment, false, error)
 	 *     on failure or attachmentCallback(attachment, progressPercent) periodically during saving.
 	 */
-	"saveItems": Zotero.Promise.coroutine(function* (items, callback, attachmentCallback) {
-		try {
-			let newItems = [], standaloneAttachments = [];
-			yield Zotero.DB.executeTransaction(function* () {
-				for (let iitem=0; iitem<items.length; iitem++) {
-					let item = items[iitem], newItem, myID;
-					// Type defaults to "webpage"
-					let type = (item.itemType ? item.itemType : "webpage");
+	saveItems: Zotero.Promise.coroutine(function* (items, attachmentCallback) {
+		let newItems = [], standaloneAttachments = [], childAttachments = [];
+		yield Zotero.DB.executeTransaction(function* () {
+			for (let iitem=0; iitem<items.length; iitem++) {
+				let item = items[iitem], newItem, myID;
+				// Type defaults to "webpage"
+				let type = (item.itemType ? item.itemType : "webpage");
+				
+				if (type == "note") {				// handle notes differently
+					newItem = yield this._saveNote(item);
+				}
+				// Handle standalone attachments differently
+				else if (type == "attachment") {
+					standaloneAttachments.push(items[iitem]);
+					attachmentCallback(items[iitem], 0);
+					continue;
+				} else {
+					newItem = new Zotero.Item(type);
+					newItem.libraryID = this._libraryID;
+					if(item.tags) item.tags = this._cleanTags(item.tags);
+
+					// Need to handle these specially. Put them in a separate object to
+					// avoid a warning from fromJSON()
+					let specialFields = {
+						attachments:item.attachments,
+						notes:item.notes,
+						seeAlso:item.seeAlso,
+						id:item.itemID || item.id
+					};
+					newItem.fromJSON(this._deleteIrrelevantFields(item));
 					
-					if (type == "note") {				// handle notes differently
-						newItem = yield this._saveNote(item);
-					} else if (type == "attachment") {	// handle attachments differently
-						standaloneAttachments.push(iitem);
-						continue;
-					} else {
-						newItem = new Zotero.Item(type);
-						newItem.libraryID = this._libraryID;
-						if(item.tags) item.tags = this._cleanTags(item.tags);
+					if (this._collections) {
+						newItem.setCollections(this._collections);
+					}
+					
+					// save item
+					myID = yield newItem.save();
 
-						// Need to handle these specially. Put them in a separate object to
-						// avoid a warning from fromJSON()
-						let specialFields = {
-							attachments:item.attachments,
-							notes:item.notes,
-							seeAlso:item.seeAlso,
-							id:item.itemID || item.id
-						};
-						newItem.fromJSON(this._deleteIrrelevantFields(item));
-						
-						if (this._collections) {
-							newItem.setCollections(this._collections);
+					// handle notes
+					if (specialFields.notes) {
+						for (let i=0; i<specialFields.notes.length; i++) {
+							yield this._saveNote(specialFields.notes[i], myID);
 						}
-						
-						// save item
-						myID = yield newItem.save();
-
-						// handle notes
-						if (specialFields.notes) {
-							for (let i=0; i<specialFields.notes.length; i++) {
-								yield this._saveNote(specialFields.notes[i], myID);
-							}
-						}
-
-						// handle attachments
-						if (specialFields.attachments) {
-							for (let i=0; i<specialFields.attachments.length; i++) {
-								let attachment = specialFields.attachments[i];
-								// Don't wait for the promise to resolve, since we want to
-								// signal completion as soon as the items are saved
-								this._saveAttachment(attachment, myID, attachmentCallback);
-							}
-							// Restore the attachments field, since we use it later in
-							// translation
-							item.attachments = specialFields.attachments;
-						}
-
-						// handle see also
-						this._handleRelated(specialFields, newItem);
 					}
 
-					// add to new item list
-					newItems.push(newItem);
+					// handle attachments
+					if (specialFields.attachments) {
+						for (let attachment of specialFields.attachments) {
+							childAttachments.push([attachment, myID]);
+							attachmentCallback(attachment, 0);
+						}
+						// Restore the attachments field, since we use it later in
+						// translation
+						item.attachments = specialFields.attachments;
+					}
+
+					// handle see also
+					this._handleRelated(specialFields, newItem);
 				}
-			}.bind(this));
 
-			// Handle standalone attachments outside of the transaction
-			for (let iitem of standaloneAttachments) {
-				let newItem = yield this._saveAttachment(items[iitem], null, attachmentCallback);
-				if (newItem) newItems.push(newItem);
+				// add to new item list
+				newItems.push(newItem);
 			}
+		}.bind(this));
 
-			callback(true, newItems);
-		} catch(e) {
-			callback(false, e);
+		// Handle standalone attachments outside of the transaction, because they can involve downloading
+		for (let item of standaloneAttachments) {
+			let newItem = yield this._saveAttachment(item, null, attachmentCallback);
+			if (newItem) newItems.push(newItem);
 		}
+		// Save attachments afterwards, since we want to signal completion as soon as the main items
+		// are saved
+		var promise = Zotero.Promise.delay(1);
+		for (let a of childAttachments) {
+			// Workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=449811 (fixed in Fx51?)
+			let [item, parentItemID] = a;
+			promise = promise.then(() => this._saveAttachment(item, parentItemID, attachmentCallback));
+		}
+
+		return newItems;
 	}),
 	
 	"saveCollections": Zotero.Promise.coroutine(function* (collections) {
 		var collectionsToProcess = collections.slice();
-		var parentIDs = [null];
+		// Use first collection passed to translate process as the root
+		var rootCollectionID = (this._collections && this._collections.length)
+			? this._collections[0] : null;
+		var parentIDs = collections.map(c => null);
 		var topLevelCollections = [];
 
 		yield Zotero.DB.executeTransaction(function* () {
@@ -175,9 +179,13 @@ Zotero.Translate.ItemSaver.prototype = {
 				if (parentID) {
 					newCollection.parentID = parentID;
 				}
-				yield newCollection.save();
-
-				if(parentID === null) topLevelCollections.push(newCollection);
+				else {
+					newCollection.parentID = rootCollectionID;
+					topLevelCollections.push(newCollection)
+				}
+				yield newCollection.save({
+					skipSelect: true
+				});
 
 				var toAdd = [];
 
@@ -222,7 +230,7 @@ Zotero.Translate.ItemSaver.prototype = {
 	 * Saves a translator attachment to the database
 	 *
 	 * @param {Translator Attachment} attachment
-	 * @param {Integer} parentID Item to attach to
+	 * @param {Integer} parentItemID - Item to attach to
 	 * @param {Function} attachmentCallback Callback function that takes three
 	 *   parameters: translator attachment object, percent completion (integer),
 	 *   and an optional error object
@@ -230,7 +238,7 @@ Zotero.Translate.ItemSaver.prototype = {
 	 * @return {Zotero.Primise<Zotero.Item|False} Flase is returned if attachment
 	 *   was not saved due to error or user settings.
 	 */
-	"_saveAttachment": Zotero.Promise.coroutine(function* (attachment, parentID, attachmentCallback) {
+	_saveAttachment: Zotero.Promise.coroutine(function* (attachment, parentItemID, attachmentCallback) {
 		try {
 			let newAttachment;
 
@@ -263,7 +271,7 @@ Zotero.Translate.ItemSaver.prototype = {
 		}
 	}),
 	
-	"_saveAttachmentFile": Zotero.Promise.coroutine(function* (attachment, parentID, attachmentCallback) {
+	_saveAttachmentFile: Zotero.Promise.coroutine(function* (attachment, parentItemID, attachmentCallback) {
 		Zotero.debug("Translate: Adding attachment", 4);
 		attachmentCallback(attachment, 0);
 		
@@ -319,9 +327,10 @@ Zotero.Translate.ItemSaver.prototype = {
 			attachment.linkMode = "linked_file";
 			newItem = yield Zotero.Attachments.linkFromURL({
 				url: attachment.url,
-				parentItemID: parentID,
+				parentItemID,
 				contentType: attachment.mimeType || undefined,
-				title: attachment.title || undefined
+				title: attachment.title || undefined,
+				collections: !parentItemID ? this._collections : undefined
 			});
 		} else {
 			if (attachment.url) {
@@ -332,14 +341,16 @@ Zotero.Translate.ItemSaver.prototype = {
 					title: attachment.title,
 					contentType: attachment.mimeType,
 					charset: attachment.charset,
-					parentItemID: parentID
+					parentItemID,
+					collections: !parentItemID ? this._collections : undefined
 				});
 			}
 			else {
 				attachment.linkMode = "imported_file";
 				newItem = yield Zotero.Attachments.importFromFile({
 					file: file,
-					parentItemID: parentID
+					parentItemID,
+					collections: !parentItemID ? this._collections : undefined
 				});
 				if (attachment.title) newItem.setField("title", attachment.title);
 			}
@@ -473,7 +484,7 @@ Zotero.Translate.ItemSaver.prototype = {
 		return false;
 	},
 	
-	"_saveAttachmentDownload": Zotero.Promise.coroutine(function* (attachment, parentID, attachmentCallback) {
+	_saveAttachmentDownload: Zotero.Promise.coroutine(function* (attachment, parentItemID, attachmentCallback) {
 		Zotero.debug("Translate: Adding attachment", 4);
 		
 		if(!attachment.url && !attachment.document) {
@@ -542,9 +553,10 @@ Zotero.Translate.ItemSaver.prototype = {
 			
 			return Zotero.Attachments.linkFromURL({
 				url: cleanURI,
-				parentItemID: parentID,
+				parentItemID,
 				contentType: mimeType,
-				title: title
+				title,
+				collections: !parentItemID ? this._collections : undefined
 			});
 		}
 		
@@ -558,16 +570,17 @@ Zotero.Translate.ItemSaver.prototype = {
 			return Zotero.Attachments.importFromDocument({
 				libraryID: this._libraryID,
 				document: attachment.document,
-				parentItemID: parentID,
-				title: title
+				parentItemID,
+				title,
+				collections: !parentItemID ? this._collections : undefined
 			});
 		}
 		
 		// Import from URL
 		let mimeType = attachment.mimeType ? attachment.mimeType : null;
 		let fileBaseName;
-		if (parentID) {
-			let parentItem = yield Zotero.Items.getAsync(parentID);
+		if (parentItemID) {
+			let parentItem = yield Zotero.Items.getAsync(parentItemID);
 			fileBaseName = Zotero.Attachments.getFileBaseNameFromItem(parentItem);
 		}
 		
@@ -579,19 +592,20 @@ Zotero.Translate.ItemSaver.prototype = {
 		return Zotero.Attachments.importFromURL({
 			libraryID: this._libraryID,
 			url: attachment.url,
-			parentItemID: parentID,
-			title: title,
-			fileBaseName: fileBaseName,
+			parentItemID,
+			title,
+			fileBaseName,
 			contentType: mimeType,
-			cookieSandbox: this._cookieSandbox
+			cookieSandbox: this._cookieSandbox,
+			collections: !parentItemID ? this._collections : undefined
 		});
 	}),
 	
-	"_saveNote":Zotero.Promise.coroutine(function* (note, parentID) {
+	"_saveNote":Zotero.Promise.coroutine(function* (note, parentItemID) {
 		var myNote = new Zotero.Item('note');
 		myNote.libraryID = this._libraryID;
-		if(parentID) {
-			myNote.parentID = parentID;
+		if (parentItemID) {
+			myNote.parentItemID = parentItemID;
 		}
 
 		if(typeof note == "object") {
@@ -601,7 +615,7 @@ Zotero.Translate.ItemSaver.prototype = {
 		} else {
 			myNote.setNote(note);
 		}
-		if (!parentID && this._collections) {
+		if (!parentItemID && this._collections) {
 			myNote.setCollections(this._collections);
 		}
 		yield myNote.save();
diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties
index 05a08ee456..9de9d9d287 100644
--- a/chrome/locale/en-US/zotero/zotero.properties
+++ b/chrome/locale/en-US/zotero/zotero.properties
@@ -649,7 +649,8 @@ zotero.preferences.advanced.debug.error = An error occurred sending debug output
 dragAndDrop.existingFiles = The following files already existed in the destination directory and were not copied:
 dragAndDrop.filesNotFound = The following files were not found and could not be copied:
 
-fileInterface.itemsImported		= Importing items…
+fileInterface.importComplete = Import Complete
+fileInterface.itemsWereImported = %1$S item was imported;%1$S items were imported
 fileInterface.itemsExported		= Exporting items…
 fileInterface.import			= Import
 fileInterface.export			= Export
diff --git a/test/tests/translateTest.js b/test/tests/translateTest.js
index 207f33a053..35edfaa5f6 100644
--- a/test/tests/translateTest.js
+++ b/test/tests/translateTest.js
@@ -596,6 +596,92 @@ describe("Zotero.Translate", function() {
 			Zotero.Translators.get.restore();
 		});
 	});
+	
+	describe("ItemSaver", function () {
+		describe("#saveCollections()", function () {
+			it("should add top-level collections to specified collection", function* () {
+				var collection = yield createDataObject('collection');
+				var collections = [
+					{
+						name: "Collection",
+						type: "collection",
+						children: []
+					}
+				];
+				var items = [
+					{
+						itemType: "book",
+						title: "Test"
+					}
+				];
+				
+				var translation = new Zotero.Translate.Import();
+				translation.setString("");
+				translation.setTranslator(buildDummyTranslator(
+					"import",
+					"function detectImport() {}\n"
+					+ "function doImport() {\n"
+					+ "	var json = JSON.parse('" + JSON.stringify(collections).replace(/['\\]/g, "\\$&") + "');\n"
+					+ "	for (let o of json) {"
+					+ "		var collection = new Zotero.Collection;\n"
+					+ "		for (let field in o) { collection[field] = o[field]; }\n"
+					+ "		collection.complete();\n"
+					+ "	}\n"
+					+ "	json = JSON.parse('" + JSON.stringify(items).replace(/['\\]/g, "\\$&") + "');\n"
+					+ "	for (let o of json) {"
+					+ "		var item = new Zotero.Item;\n"
+					+ "		for (let field in o) { item[field] = o[field]; }\n"
+					+ "		item.complete();\n"
+					+ "	}\n"
+					+ "}"
+				));
+				yield translation.translate({
+					collections: [collection.id]
+				});
+				assert.lengthOf(translation.newCollections, 1);
+				assert.isNumber(translation.newCollections[0].id);
+				assert.lengthOf(translation.newItems, 1);
+				assert.isNumber(translation.newItems[0].id);
+				var childCollections = Array.from(collection.getChildCollections(true));
+				assert.sameMembers(childCollections, translation.newCollections.map(c => c.id));
+			});
+		});
+		
+		describe("#_saveAttachment()", function () {
+			it("should save standalone attachment to collection", function* () {
+				var collection = yield createDataObject('collection');
+				var items = [
+					{
+						itemType: "attachment",
+						title: "Test",
+						mimeType: "text/html",
+						url: "http://example.com"
+					}
+				];
+				
+				var translation = new Zotero.Translate.Import();
+				translation.setString("");
+				translation.setTranslator(buildDummyTranslator(
+					"import",
+					"function detectImport() {}\n"
+					+ "function doImport() {\n"
+					+ "	var json = JSON.parse('" + JSON.stringify(items).replace(/['\\]/g, "\\$&") + "');\n"
+					+ "	for (var i=0; i<json.length; i++) {"
+					+ "		var item = new Zotero.Item;\n"
+					+ "		for (var field in json[i]) { item[field] = json[i][field]; }\n"
+					+ "		item.complete();\n"
+					+ "	}\n"
+					+ "}"
+				));
+				yield translation.translate({
+					collections: [collection.id]
+				});
+				assert.lengthOf(translation.newItems, 1);
+				assert.isNumber(translation.newItems[0].id);
+				assert.ok(collection.hasItem(translation.newItems[0].id));
+			});
+		});
+	});
 });
 
 describe("Zotero.Translate.ItemGetter", function() {