undomodule: disallow undoing "clear authorship colors"

Clearing the authorship colors of a document with at least two authors, and then
undoing that action caused a disconnect from the pad.
This change disallows undoing clearing authorship colors in order to prevent
the problem from affecting users, and adds the relative test coverage.

This is a change of behaviour, and is documented in the changelog.

Fixes #2802 (sidestepping it).

CHANGELOG.md

@@ -1,3 +1,6 @@
+# 1.8.3
+*BREAKING CHANGE*: undoing the "clear authorship colors" command is no longer supported (see https://github.com/ether/etherpad-lite/issues/2802)
+
 # 1.8
 * SECURITY: change referrer policy so that Etherpad addresses aren't leaked when links are clicked (discussion: https://github.com/ether/etherpad-lite/pull/3636)
 * SECURITY: set the "secure" flag for the session cookies when served over SSL. From now on it will not be possible to serve the same instance both in cleartext and over SSL

src/locales/en.json

@@ -154,7 +154,7 @@
   "pad.userlist.guest": "Guest",
   "pad.userlist.deny": "Deny",
   "pad.userlist.approve": "Approve",
-  "pad.editbar.clearcolors": "Clear authorship colors on entire document?",
+  "pad.editbar.clearcolors": "Clear authorship colors on entire document? This cannot be undone",
 
   "pad.impexp.importbutton": "Import Now",
   "pad.impexp.importing": "Importing...",

src/locales/qqq.json

@@ -113,7 +113,7 @@
 	"pad.userlist.guest": "Preceded by the link text which is labeled {{msg-etherpadlite|Pad.userlist.approve}}.\n{{Identical|Guest}}",
 	"pad.userlist.deny": "Used as link text.\n\nFollowed by the link which is labeled {{msg-etherpadlite|Pad.userlist.approve}}.",
 	"pad.userlist.approve": "Used as link text.\n\nPreceded by the link which is labeled {{msg-etherpadlite|Pad.userlist.deny}}.\n\nFollowed by the message {{msg-etherpadlite|Pad.userlist.guest}}.\n{{Identical|Approve}}",
-	"pad.editbar.clearcolors": "Used as confirmation message (JavaScript <code>confirm()</code> function).\n\nThis message means \"Are you sure you want to clear authorship colors on entire document?\".",
+	"pad.editbar.clearcolors": "Used as confirmation message (JavaScript <code>confirm()</code> function).\n\nThis message means \"Are you sure you want to clear authorship colors on entire document? This cannot be undone\".",
 	"pad.impexp.importbutton": "Used as label for the Submit button.",
 	"pad.impexp.importing": "Used to indicate that the file is being imported.\n{{Identical|Importing}}",
 	"pad.impexp.confirmimport": "Used as confirmation message (JavaScript <code>confirm()</code> function).",

src/node/handler/PadMessageHandler.js

@@ -932,7 +932,7 @@ async function handleClientReady(client, message)
   await Promise.all(authors.map(authorId => {
     return authorManager.getAuthor(authorId).then(author => {
       if (!author) {
-        messageLogger.error("There is no author for authorId:", authorId);
+        messageLogger.error("There is no author for authorId: ", authorId, ". This is possibly related to https://github.com/ether/etherpad-lite/issues/2802");
       } else {
         historicalAuthorData[authorId] = { name: author.name, colorId: author.colorId }; // Filter author attribs (e.g. don't send author's pads to all clients)
       }

src/static/js/Changeset.js

@@ -791,6 +791,9 @@ exports.textLinesMutator = function (lines) {
         }
       } else {
         var sline = putCurLineInSplice();
+        if (!curSplice[sline]) {
+          console.error("curSplice[sline] not populated, actual curSplice contents is ", curSplice, ". Possibly related to https://github.com/ether/etherpad-lite/issues/2802");
+        }
         curSplice[sline] = curSplice[sline].substring(0, curCol) + text + curSplice[sline].substring(curCol);
         curCol += text.length;
       }

src/static/js/ace2_inner.js

@@ -292,6 +292,7 @@ function Ace2Inner(){
   {
     if ((typeof author) != "string")
     {
+      top.console.error("Going to throw new error, potentially caused by: https://github.com/ether/etherpad-lite/issues/2802");
       throw new Error("setAuthorInfo: author (" + author + ") is not a string");
     }
     if (!info)

src/static/js/undomodule.js

@@ -253,8 +253,16 @@ var undoModule = (function()
       }
       if (!merged)
       {
+        /*
+         * Push the event on the undo stack only if it exists, and if it's
+         * not a "clearauthorship". This disallows undoing the removal of the
+         * authorship colors, but is a necessary stopgap measure against
+         * https://github.com/ether/etherpad-lite/issues/2802
+         */
+        if (event && (event.eventType !== "clearauthorship")) {
           stack.pushEvent(event);
         }
+      }
       undoPtr = 0;
     }
 

tests/frontend/specs/clear_authorship_colors.js

@@ -47,6 +47,76 @@ describe("clear authorship colors button", function(){
       var hasAuthorClass = inner$("div").first().attr("class").indexOf("author") !== -1;
       expect(hasAuthorClass).to.be(false);
 
+      setTimeout(function(){
+        var disconnectVisible = chrome$("div.disconnected").attr("class").indexOf("visible") === -1
+        expect(disconnectVisible).to.be(true);
+      },1000);
+
+      done();
+    });
+
+  });
+
+  it("makes text clear authorship colors and checks it can't be undone", function(done) {
+    var inner$ = helper.padInner$;
+    var chrome$ = helper.padChrome$;
+
+    // override the confirm dialogue functioon
+    helper.padChrome$.window.confirm = function(){
+      return true;
+    }
+
+    //get the first text element out of the inner iframe
+    var $firstTextElement = inner$("div").first();
+
+    // Get the original text
+    var originalText = inner$("div").first().text();
+
+    // Set some new text
+    var sentText = "Hello";
+
+    //select this text element
+    $firstTextElement.sendkeys('{selectall}');
+    $firstTextElement.sendkeys(sentText);
+    $firstTextElement.sendkeys('{rightarrow}');
+
+    helper.waitFor(function(){
+      return inner$("div span").first().attr("class").indexOf("author") !== -1; // wait until we have the full value available
+    }).done(function(){
+      //IE hates you if you don't give focus to the inner frame bevore you do a clearAuthorship
+      inner$("div").first().focus();
+
+      //get the clear authorship colors button and click it
+      var $clearauthorshipcolorsButton = chrome$(".buttonicon-clearauthorship");
+      $clearauthorshipcolorsButton.click();
+
+      // does the first divs span include an author class?
+      console.log(inner$("div span").first().attr("class"));
+      var hasAuthorClass = inner$("div span").first().attr("class").indexOf("author") !== -1;
+      //expect(hasAuthorClass).to.be(false);
+
+      // does the first div include an author class?
+      var hasAuthorClass = inner$("div").first().attr("class").indexOf("author") !== -1;
+      expect(hasAuthorClass).to.be(false);
+
+      var e = inner$.Event(helper.evtType);
+      e.ctrlKey = true; // Control key
+      e.which = 90; // z
+      inner$("#innerdocbody").trigger(e); // shouldn't od anything
+
+      // does the first div include an author class?
+      hasAuthorClass = inner$("div").first().attr("class").indexOf("author") !== -1;
+      expect(hasAuthorClass).to.be(false);
+
+      // get undo and redo buttons
+      var $undoButton = chrome$(".buttonicon-undo");
+
+      // click the button
+      $undoButton.click(); // shouldn't do anything
+      hasAuthorClass = inner$("div").first().attr("class").indexOf("author") !== -1;
+      expect(hasAuthorClass).to.be(false);
+
+
       setTimeout(function(){
         var disconnectVisible = chrome$("div.disconnected").attr("class").indexOf("visible") === -1
         expect(disconnectVisible).to.be(true);
@@ -57,3 +127,4 @@ describe("clear authorship colors button", function(){
 
   });
 });
+