ep_comments_page - npm Package Compare versions

Comparing version 0.1.26 to 0.1.27

commentManager.js

@@ -5,5 +5,8 @@ /* global exports, require */
 var db = require('ep_etherpad-lite/node/db/DB');
+const log4js = require('ep_etherpad-lite/node_modules/log4js');
 var randomString = require('ep_etherpad-lite/static/js/pad_utils').randomString;
 var shared = require('./static/js/shared');
 
+const logger = log4js.getLogger('ep_comments_page');
+
 exports.getComments = async (padId) => {
@@ -19,8 +22,11 @@ // Not sure if we will encouter race conditions here..  Be careful.
 exports.deleteComment = async (padId, commentId, authorId) => {
-  let comments = await db.get('comments:' + padId);
-  // the entry doesn't exist so far, let's create it
-  if (comments == null) comments = {};
-  // get the entry
+  const comments = await db.get(`comments:${padId}`);
+  if (comments == null || comments[commentId] == null) {
+    logger.debug(`ignoring attempt to delete non-existent comment ${commentId}`);
+    throw new Error('no_such_comment');
+  }
   if (comments[commentId].author !== authorId) {
-    return 'unauth';
+    logger.debug(`author ${authorId} attempted to delete comment ${commentId} ` +
+                 `belonging to author ${comments[commentId].author}`);
+    throw new Error('unauth');
   }
@@ -176,3 +182,6 @@ delete comments[commentId];
 exports.changeCommentText = async (padId, commentId, commentText, authorId) => {
-  if (commentText.length <= 0) return true;
+  if (commentText.length <= 0) {
+    logger.debug(`ignoring attempt to change comment ${commentId} to the empty string`);
+    throw new Error('comment_cannot_be_empty');
+  }
 
@@ -189,4 +198,10 @@ // Given a comment we update the comment text
   const comments = await db.get(prefix + padId);
+  if (comments == null || comments[commentId] == null) {
+    logger.debug(`ignoring attempt to edit non-existent comment ${commentId}`);
+    throw new Error('no_such_comment');
+  }
   if (comments[commentId].author !== authorId) {
-    return 'unauth';
+    logger.debug(`author ${authorId} attempted to edit comment ${commentId} ` +
+                 `belonging to author ${comments[commentId].author}`);
+    throw new Error('unauth');
   }
@@ -198,4 +213,2 @@ // update the comment text
   await db.set(prefix + padId, comments);
-
-  return null;
 };

index.js

@@ -70,10 +70,10 @@ /* global exports, require */
     socket.on('deleteComment', async (data, respond) => {
-      const {padId} = await readOnlyManager.getIds(data.padId);
-      // delete the comment on the database
-      const failed = await commentManager.deleteComment(padId, data.commentId, data.authorId);
-      // Broadcast to all other users that this comment was deleted
-      if (!failed) {
+      try {
+        const {padId} = await readOnlyManager.getIds(data.padId);
+        await commentManager.deleteComment(padId, data.commentId, data.authorId);
         socket.broadcast.to(padId).emit('commentDeleted', data.commentId);
+        respond('');
+      } catch (err) {
+        respond(err.message || err.toString());
       }
-      respond(failed);
     });
@@ -112,11 +112,11 @@
     socket.on('updateCommentText', async (data, respond) => {
-      const {padId} = await readOnlyManager.getIds(data.padId);
-      // Broadcast to all other users that the comment text was changed.
-      // Note that commentId here can either be the commentId or replyId..
-      var commentId = data.commentId;
-      var commentText = data.commentText;
-      var authorId = data.authorId;
-      const failed = await commentManager.changeCommentText(padId, commentId, commentText, authorId);
-      if (!failed) socket.broadcast.to(padId).emit('textCommentUpdated', commentId, commentText);
-      respond(failed);
+      try {
+        const {commentId, commentText, authorId} = data;
+        const {padId} = await readOnlyManager.getIds(data.padId);
+        await commentManager.changeCommentText(padId, commentId, commentText, authorId);
+        socket.broadcast.to(padId).emit('textCommentUpdated', commentId, commentText);
+        respond('');
+      } catch (err) {
+        respond(err.message || err.toString());
+      }
     });
@@ -123,0 +123,0 @@

package.json

 {
   "description": "Adds comments on sidebar and link it to the text.  For no-skin use ep_page_view.",
   "name": "ep_comments_page",
-  "version": "0.1.26",
+  "version": "0.1.27",
   "author": {
@@ -6,0 +6,0 @@ "name": "Nicolas Lescop",

static/js/index.js

@@ -123,39 +123,41 @@ /* global clearTimeout, clientVars, exports, html10n, pad, require, setTimeout */
   // All this does is remove the comment attr on the selection
-  this.container.parent().on("click", ".comment-delete", function(){
+  this.container.parent().on('click', '.comment-delete', async function () {
     var commentId = $(this).closest('.comment-container')[0].id;
-    self.socket.emit('deleteComment', {padId: self.padId, commentId: commentId, authorId: clientVars.userId}, function (err){
-      if (!err) {
-        self.deleteComment(commentId);
-        var padOuter = $('iframe[name="ace_outer"]').contents();
-        var padInner = padOuter.find('iframe[name="ace_inner"]');
-        var selector = "."+commentId;
-        var ace = self.ace;
+    try {
+      await new Promise((resolve, reject) => {
+        self.socket.emit('deleteComment', {
+          padId: self.padId,
+          commentId,
+          authorId: clientVars.userId,
+        }, (errMsg) => errMsg ? reject(new Error(errMsg)) : resolve());
+      });
+    } catch (err) {
+      if (err.message !== 'unauth') throw err; // Let the uncaught error handler handle it.
+      $.gritter.add({
+        title: html10n.translations['ep_comments_page.error'] || 'Error',
+        text: html10n.translations['ep_comments_page.error.delete_unauth'] ||
+          'You cannot delete other users comments!',
+        class_name: 'error',
+      });
+      return;
+    }
+    self.deleteComment(commentId);
+    const padOuter = $('iframe[name="ace_outer"]').contents();
+    const padInner = padOuter.find('iframe[name="ace_inner"]');
+    const selector = `.${commentId}`;
+    const ace = self.ace;
 
-        ace.callWithAce(function(aceTop){
-          var repArr = aceTop.ace_getRepFromSelector(selector, padInner);
-          // rep is an array of reps..  I will need to iterate over each to do something meaningful..
-          $.each(repArr, function(index, rep){
-            // I don't think we need this nested call
-            ace.callWithAce(function (ace){
-              ace.ace_performSelectionChange(rep[0],rep[1],true);
-              ace.ace_setAttributeOnSelection('comment', 'comment-deleted');
-              // Note that this is the correct way of doing it, instead of there being
-              // a commentId we now flag it as "comment-deleted"
-            });
-          });
-        },'deleteCommentedSelection', true);
-      }
-
-      if (err === 'unauth') {
-        $.gritter.add({title: html10n.translations["ep_comments_page.error"] || "Error", text: html10n.translations["ep_comments_page.error.delete_unauth"] || "You cannot delete other users comments!",  class_name: "error"})
-      } else {
-        $.gritter.add({
-          title: "Error",
-          text: err,
-          sticky: true,
-          class_name: "error"
-        })
-      }
-    });
-
+    ace.callWithAce((aceTop) => {
+      const repArr = aceTop.ace_getRepFromSelector(selector, padInner);
+      // rep is an array of reps.. I will need to iterate over each to do something meaningful..
+      $.each(repArr, (index, rep) => {
+        // I don't think we need this nested call
+        ace.callWithAce((ace) => {
+          ace.ace_performSelectionChange(rep[0], rep[1], true);
+          ace.ace_setAttributeOnSelection('comment', 'comment-deleted');
+          // Note that this is the correct way of doing it, instead of there being
+          // a commentId we now flag it as "comment-deleted"
+        });
+      });
+    }, 'deleteCommentedSelection', true);
   });
@@ -185,3 +187,3 @@
   // submit the edition on the text and update the comment text
-  this.container.parent().on("click", ".comment-edit-submit", function(e){
+  this.container.parent().on('click', '.comment-edit-submit', async function (e) {
     e.preventDefault();
@@ -199,25 +201,24 @@ e.stopPropagation();
 
-    self.socket.emit('updateCommentText', data, function (err){
-      if(!err) {
-        $commentForm.remove();
-        $commentBox.removeClass('editing');
-        self.updateCommentBoxText(commentId, commentText);
+    try {
+      await new Promise((resolve, reject) => {
+        self.socket.emit('updateCommentText', data,
+          (errMsg) => errMsg ? reject(new Error(errMsg)) : resolve());
+      });
+    } catch (err) {
+      if (err.message !== 'unauth') throw err; // Let the uncaught error handler handle it.
+      $.gritter.add({
+        title: html10n.translations['ep_comments_page.error'] || 'Error',
+        text: html10n.translations['ep_comments_page.error.edit_unauth'] ||
+          'You cannot edit other users comments!',
+        class_name: 'error',
+      });
+      return;
+    }
+    $commentForm.remove();
+    $commentBox.removeClass('editing');
+    self.updateCommentBoxText(commentId, commentText);
 
-        // although the comment or reply was saved on the data base successfully, it needs
-        // to update the comment or comment reply variable with the new text saved
-        self.setCommentOrReplyNewText(commentId, commentText);
-      }
-
-      if (err === 'unauth') {
-        $.gritter.add({title: html10n.translations["ep_comments_page.error"] || "Error", text: html10n.translations["ep_comments_page.error.edit_unauth"] || "You cannot edit other users comments!",  class_name: "error"})
-      } else {
-        $.gritter.add({
-          title: "Error",
-          text: err,
-          sticky: true,
-          class_name: "error"
-        })
-      }
-
-    });
+    // although the comment or reply was saved on the data base successfully, it needs
+    // to update the comment or comment reply variable with the new text saved
+    self.setCommentOrReplyNewText(commentId, commentText);
   });
@@ -224,0 +225,0 @@

static/tests/frontend/specs/commentDelete.js

@@ -34,22 +34,17 @@ describe('ep_comments_page - Comment Delete', function(){
   context('when user presses the delete button on other users comment', function(){
-    it("should not delete comment", function(done){
+    it('should not delete comment', async function () {
       var outer$ = helper.padOuter$;
-      setTimeout(function () {
-        helper.newPad({}, helperFunctions.padId);
-        helper.waitFor(function () {
-          outer$ = helper.padOuter$;
-          return !!outer$ && outer$('.comment-delete').length;
-        }).done(function () {
-            outer$('.comment-delete').click();
-            helper.waitFor(function () {
-              var chrome$ = helper.padChrome$;
-
-              return chrome$('#gritter-container').find('.error').length > 0;
-            }).done(function () {
-              var inner$ = helper.padInner$;
-              if(inner$('.comment').length === 0) throw new Error("Error deleting comment");
-              done();
-            });
-        });
-      }, 500);
+      await new Promise((resolve) => setTimeout(resolve, 500));
+      await new Promise((resolve) => helper.newPad(resolve, helperFunctions.padId));
+      await helper.waitForPromise(() => {
+        outer$ = helper.padOuter$;
+        return !!outer$ && outer$('.comment-delete').length;
+      });
+      outer$('.comment-delete').click();
+      await helper.waitForPromise(() => {
+        const chrome$ = helper.padChrome$;
+        return chrome$('#gritter-container').find('.error').length > 0;
+      });
+      const inner$ = helper.padInner$;
+      if (inner$('.comment').length === 0) throw new Error('Comment should not have been deleted');
     });
@@ -56,0 +51,0 @@ });

static/tests/frontend/specs/commentEdit.js

@@ -112,32 +112,26 @@ describe('ep_comments_page - Comment Edit', function(){
 
-      it('should not update the comment text', function (done) {
+      it('should not update the comment text', async function () {
         var outer$ = helper.padOuter$;
-        setTimeout(function () {
-          helper.newPad({}, helperFunctions.padId);
-          helper.waitFor(function () {
-            outer$ = helper.padOuter$;
-            return !!outer$ && outer$('#comments').find('.comment-edit').length;
-          }).done(function () {
-            helperFunctions.clickEditCommentButton();
-            helperFunctions.writeCommentText(updatedText2)
-            helperFunctions.pressSave();
+        await new Promise((resolve) => setTimeout(resolve, 500));
+        await new Promise((resolve) => helper.newPad(resolve, helperFunctions.padId));
+        await helper.waitForPromise(() => {
+          outer$ = helper.padOuter$;
+          return !!outer$ && outer$('#comments').find('.comment-edit').length;
+        });
+        helperFunctions.clickEditCommentButton();
+        helperFunctions.writeCommentText(updatedText2);
+        helperFunctions.pressSave();
 
-            helper.waitFor(function () {
-              //Error message is shown
-              var chrome$ = helper.padChrome$;
-              return chrome$('#gritter-container').find('.error').length > 0;
-            }).done(function () {
-              helper.waitFor(function () {
-                outer$ = helper.padOuter$;
-                var commentText = outer$('.comment-text').first().text();
-                return (commentText !== updatedText2);
-              }).done(function(){
-                var commentText = outer$('.comment-text').first().text();
-                expect(commentText).to.not.be(updatedText2);
-                done();
-              });
-            });
-
-          });
-        }, 500);
+        await helper.waitForPromise(() => {
+          // Error message is shown
+          const chrome$ = helper.padChrome$;
+          return chrome$('#gritter-container').find('.error').length > 0;
+        });
+        await helper.waitForPromise(() => {
+          outer$ = helper.padOuter$;
+          const commentText = outer$('.comment-text').first().text();
+          return (commentText !== updatedText2);
+        });
+        const commentText = outer$('.comment-text').first().text();
+        expect(commentText).to.not.be(updatedText2);
       });
@@ -144,0 +138,0 @@ });

No alert changes

Improved metrics

Total package byte prevSize

618084

increased by0.08%

Lines of code

6186

increased by0.15%

No dependency changes