From 3a8357cb95c42e342a08d8761def2f8e50b428e7 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Mon, 27 Mar 2017 20:42:28 -0400
Subject: [PATCH] Fix renaming and clearing of colored tags

---
 .../content/zotero/bindings/tagselector.xml   | 12 +++++++--
 chrome/content/zotero/xpcom/data/tags.js      | 15 +++++++++--
 test/tests/tagSelectorTest.js                 | 27 ++++++++++++++++++-
 test/tests/tagsTest.js                        | 17 +++++++++++-
 4 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/chrome/content/zotero/bindings/tagselector.xml b/chrome/content/zotero/bindings/tagselector.xml
index feb78cf897..80104e5fb6 100644
--- a/chrome/content/zotero/bindings/tagselector.xml
+++ b/chrome/content/zotero/bindings/tagselector.xml
@@ -772,8 +772,16 @@
 							if (!color) {
 								throw new Error("Can't rename missing tag");
 							}
-							yield Zotero.Tags.setColor(this.libraryID, oldName, false);
-							yield Zotero.Tags.setColor(this.libraryID, newName, color);
+							
+							yield Zotero.DB.executeTransaction(function* () {
+								yield Zotero.Tags.setColor(this.libraryID, oldName, false);
+								yield Zotero.Tags.setColor(
+									this.libraryID,
+									newName.value,
+									color.color,
+									color.position
+								);
+							}.bind(this));
 						}
 						
 						if (wasSelected) {
diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js
index d1af5c1598..59db23a3d3 100644
--- a/chrome/content/zotero/xpcom/data/tags.js
+++ b/chrome/content/zotero/xpcom/data/tags.js
@@ -501,7 +501,17 @@ Zotero.Tags = new function() {
 		
 		var tagColors = Zotero.SyncedSettings.get(libraryID, 'tagColors');
 		
-		tagColors = tagColors || [];
+		// Sanitize -- shouldn't be necessary, but just in case a bad value makes it into the setting
+		if (!Array.isArray(tagColors)) {
+			tagColors = [];
+		}
+		tagColors = tagColors.filter(color => {
+			if (typeof color != 'object' || typeof color.name != 'string' || typeof color.color != 'string') {
+				Zotero.logError("Skipping invalid colored tag: " + JSON.stringify(color));
+				return false;
+			}
+			return true;
+		});
 		
 		_libraryColors[libraryID] = tagColors;
 		_libraryColorsByName[libraryID] = new Map;
@@ -539,7 +549,8 @@ Zotero.Tags = new function() {
 				return;
 			}
 			
-			tagColors = tagColors.filter(val => val.name != name);
+			_libraryColors[libraryID] = tagColors = tagColors.filter(val => val.name != name);
+			_libraryColorsByName[libraryID].delete(name);
 		}
 		else {
 			// Get current position if present
diff --git a/test/tests/tagSelectorTest.js b/test/tests/tagSelectorTest.js
index 20ae5532ed..3e184ecfb8 100644
--- a/test/tests/tagSelectorTest.js
+++ b/test/tests/tagSelectorTest.js
@@ -356,7 +356,32 @@ describe("Tag Selector", function () {
 			
 			var tags = getRegularTags();
 			assert.include(tags, newTag);
-		})
+		});
+		
+		it("should rename a non-matching colored tag and update the tag selector", function* () {
+			yield selectLibrary(win);
+			
+			var oldTag = Zotero.Utilities.randomString();
+			var newTag = Zotero.Utilities.randomString();
+			
+			var libraryID = Zotero.Libraries.userLibraryID;
+			var promise = waitForTagSelector(win);
+			yield Zotero.Tags.setColor(libraryID, oldTag, "#F3F3F3");
+			yield promise;
+			
+			var tagSelector = doc.getElementById('zotero-tag-selector');
+			promise = waitForTagSelector(win);
+			var promptPromise = waitForWindow("chrome://global/content/commonDialog.xul", function (dialog) {
+				dialog.document.getElementById('loginTextbox').value = newTag;
+				dialog.document.documentElement.acceptDialog();
+			})
+			yield tagSelector.rename(oldTag);
+			yield promise;
+			
+			var tags = getColoredTags();
+			assert.notInclude(tags, oldTag);
+			assert.include(tags, newTag);
+		});
 	})
 	
 	describe("#_openColorPickerWindow()", function () {
diff --git a/test/tests/tagsTest.js b/test/tests/tagsTest.js
index c0f9d482cb..46d7c340eb 100644
--- a/test/tests/tagsTest.js
+++ b/test/tests/tagsTest.js
@@ -69,7 +69,7 @@ describe("Zotero.Tags", function () {
 	describe("#setColor()", function () {
 		var libraryID;
 		
-		before(function* () {
+		beforeEach(function* () {
 			libraryID = Zotero.Libraries.userLibraryID;
 			
 			// Clear library tag colors
@@ -97,5 +97,20 @@ describe("Zotero.Tags", function () {
 			assert.lengthOf(o, 2);
 			assert.sameMembers(o.map(c => c.color), [aColor, bColor]);
 		});
+		
+		it("should clear color for a tag", function* () {
+			var aColor = '#ABCDEF';
+			yield Zotero.Tags.setColor(libraryID, "A", aColor);
+			var o = Zotero.Tags.getColor(libraryID, "A")
+			assert.equal(o.color, aColor);
+			assert.equal(o.position, 0);
+			
+			yield Zotero.Tags.setColor(libraryID, "A", false);
+			assert.equal(Zotero.Tags.getColors(libraryID).size, 0);
+			assert.isFalse(Zotero.Tags.getColor(libraryID, "A"));
+			
+			var o = Zotero.SyncedSettings.get(libraryID, 'tagColors');
+			assert.isNull(o);
+		});
 	});
 })