comment-history: fixes/ui changes

- show explicit version conflicts
- use new select2 based dropdown with nicer formatting
- enabled editing of content for TODO comments

rhodecode/apps/repository/tests/test_repo_commit_comments.py

@@ -378,9 +378,9 b' class TestRepoCommitCommentsView(TestCon'
                 'text': test_text_v2,
                 'version': '0',
             },
-            status=404,
+            status=409,
         )
-        assert response.status_int == 404
+        assert response.status_int == 409
 
         text_form_db = ChangesetComment.query().filter(
             ChangesetComment.comment_id == comment_id).first().text

rhodecode/apps/repository/tests/test_repo_pullrequests.py

@@ -480,9 +480,7 b' class TestPullrequestsView(object):'
         )
         assert response.status_int == 404
 
-    def test_comment_and_try_edit_already_edited(
-            self, pr_util, csrf_token, xhr_header
-    ):
+    def test_comment_and_try_edit_already_edited(self, pr_util, csrf_token, xhr_header):
         pull_request = pr_util.create_pull_request()
         response = self.app.post(
             route_path(
@@ -498,8 +496,9 b' class TestPullrequestsView(object):'
         assert response.json
         comment_id = response.json.get('comment_id', None)
         assert comment_id
+
         test_text = 'test'
-        response = self.app.post(
+        self.app.post(
             route_path(
                 'pullrequest_comment_edit',
                 repo_name=pull_request.target_repo.scm_instance().name,
@@ -528,9 +527,9 b' class TestPullrequestsView(object):'
                 'text': test_text_v2,
                 'version': '0',
             },
-            status=404,
+            status=409,
         )
-        assert response.status_int == 404
+        assert response.status_int == 409
 
         text_form_db = ChangesetComment.query().filter(
             ChangesetComment.comment_id == comment_id).first().text

rhodecode/apps/repository/views/repo_commits.py

@@ -20,9 +20,9 b''
 
 
 import logging
-import collections
 
-from pyramid.httpexceptions import HTTPNotFound, HTTPBadRequest, HTTPFound, HTTPForbidden
+from pyramid.httpexceptions import (
+    HTTPNotFound, HTTPBadRequest, HTTPFound, HTTPForbidden, HTTPConflict)
 from pyramid.view import view_config
 from pyramid.renderers import render
 from pyramid.response import Response
@@ -39,7 +39,7 b' from rhodecode.lib.compat import Ordered'
 from rhodecode.lib.diffs import (
     cache_diff, load_cached_diff, diff_cache_exist, get_diff_context,
     get_diff_whitespace_flag)
-from rhodecode.lib.exceptions import StatusChangeOnClosedPullRequestError
+from rhodecode.lib.exceptions import StatusChangeOnClosedPullRequestError, CommentVersionMismatch
 import rhodecode.lib.helpers as h
 from rhodecode.lib.utils2 import safe_unicode, str2bool
 from rhodecode.lib.vcs.backends.base import EmptyCommit
@@ -595,6 +595,8 b' class RepoCommitsView(RepoAppView):'
         route_name='repo_commit_comment_edit', request_method='POST',
         renderer='json_ext')
     def repo_commit_comment_edit(self):
+        self.load_default_context()
+
         comment_id = self.request.matchdict['comment_id']
         comment = ChangesetComment.get_or_404(comment_id)
 
@@ -615,11 +617,12 b' class RepoCommitsView(RepoAppView):'
                 log.warning(
                     'Comment(repo): '
                     'Trying to create new version '
-                    'of existing comment {}'.format(
+                    'with the same comment body {}'.format(
                         comment_id,
                     )
                 )
                 raise HTTPNotFound()
+
             if version.isdigit():
                 version = int(version)
             else:
@@ -633,19 +636,28 b' class RepoCommitsView(RepoAppView):'
                 )
                 raise HTTPNotFound()
 
+            try:
             comment_history = CommentsModel().edit(
                 comment_id=comment_id,
                 text=text,
                 auth_user=self._rhodecode_user,
                 version=version,
             )
+            except CommentVersionMismatch:
+                raise HTTPConflict()
+
             if not comment_history:
                 raise HTTPNotFound()
+
             Session().commit()
             return {
                 'comment_history_id': comment_history.comment_history_id,
                 'comment_id': comment.comment_id,
                 'comment_version': comment_history.version,
+                'comment_author_username': comment_history.author.username,
+                'comment_author_gravatar': h.gravatar_url(comment_history.author.email, 16),
+                'comment_created_on': h.age_component(comment_history.created_on,
+                                                      time_is_local=True),
             }
         else:
             log.warning('No permissions for user %s to edit comment_id: %s',

rhodecode/apps/repository/views/repo_pull_requests.py

@@ -25,7 +25,7 b' import formencode'
 import formencode.htmlfill
 import peppercorn
 from pyramid.httpexceptions import (
-    HTTPFound, HTTPNotFound, HTTPForbidden, HTTPBadRequest)
+    HTTPFound, HTTPNotFound, HTTPForbidden, HTTPBadRequest, HTTPConflict)
 from pyramid.view import view_config
 from pyramid.renderers import render
 
@@ -34,6 +34,7 b' from rhodecode.apps._base import RepoApp'
 from rhodecode.lib import helpers as h, diffs, codeblocks, channelstream
 from rhodecode.lib.base import vcs_operation_context
 from rhodecode.lib.diffs import load_cached_diff, cache_diff, diff_cache_exist
+from rhodecode.lib.exceptions import CommentVersionMismatch
 from rhodecode.lib.ext_json import json
 from rhodecode.lib.auth import (
     LoginRequired, HasRepoPermissionAny, HasRepoPermissionAnyDecorator,
@@ -1528,6 +1529,8 b' class RepoPullRequestsView(RepoAppView, '
         route_name='pullrequest_comment_edit', request_method='POST',
         renderer='json_ext')
     def pull_request_comment_edit(self):
+        self.load_default_context()
+
         pull_request = PullRequest.get_or_404(
             self.request.matchdict['pull_request_id']
         )
@@ -1566,11 +1569,12 b' class RepoPullRequestsView(RepoAppView, '
                 log.warning(
                     'Comment(PR): '
                     'Trying to create new version '
-                    'of existing comment {}'.format(
+                    'with the same comment body {}'.format(
                         comment_id,
                     )
                 )
                 raise HTTPNotFound()
+
             if version.isdigit():
                 version = int(version)
             else:
@@ -1584,24 +1588,30 b' class RepoPullRequestsView(RepoAppView, '
                 )
                 raise HTTPNotFound()
 
+            try:
             comment_history = CommentsModel().edit(
                 comment_id=comment_id,
                 text=text,
                 auth_user=self._rhodecode_user,
                 version=version,
             )
+            except CommentVersionMismatch:
+                raise HTTPConflict()
+
             if not comment_history:
                 raise HTTPNotFound()
+
             Session().commit()
             return {
                 'comment_history_id': comment_history.comment_history_id,
                 'comment_id': comment.comment_id,
                 'comment_version': comment_history.version,
+                'comment_author_username': comment_history.author.username,
+                'comment_author_gravatar': h.gravatar_url(comment_history.author.email, 16),
+                'comment_created_on': h.age_component(comment_history.created_on,
+                                                      time_is_local=True),
             }
         else:
-            log.warning(
-                'No permissions for user {} to edit comment_id: {}'.format(
-                    self._rhodecode_db_user, comment_id
-                )
-            )
+            log.warning('No permissions for user %s to edit comment_id: %s',
+                        self._rhodecode_db_user, comment_id)
             raise HTTPNotFound()

rhodecode/lib/exceptions.py

@@ -177,3 +177,7 b' class ArtifactMetadataDuplicate(ValueErr'
 
 class ArtifactMetadataBadValueType(ValueError):
     pass
+
+
+class CommentVersionMismatch(ValueError):
+    pass

rhodecode/lib/helpers.py

@@ -24,6 +24,7 b' Helper functions'
 Consists of functions to typically be used within templates, but also
 available to Controllers. This module is available to both as 'h'.
 """
+import base64
 
 import os
 import random
@@ -1352,7 +1353,7 b' class InitialsGravatar(object):'
 
     def generate_svg(self, svg_type=None):
         img_data = self.get_img_data(svg_type)
-        return "data:image/svg+xml;base64,%s" % img_data.encode('base64')
+        return "data:image/svg+xml;base64,%s" % base64.b64encode(img_data)
 
 
 def initials_gravatar(email_address, first_name, last_name, size=30):

rhodecode/model/comment.py

@@ -33,6 +33,7 b' from sqlalchemy.sql.functions import coa'
 
 from rhodecode.lib import helpers as h, diffs, channelstream, hooks_utils
 from rhodecode.lib import audit_logger
+from rhodecode.lib.exceptions import CommentVersionMismatch
 from rhodecode.lib.utils2 import extract_mentioned_users, safe_str
 from rhodecode.model import BaseModel
 from rhodecode.model.db import (
@@ -507,13 +508,13 b' class CommentsModel(BaseModel):'
         comment_version = ChangesetCommentHistory.get_version(comment_id)
         if (comment_version - version) != 1:
             log.warning(
-                'Version mismatch, skipping... '
-                'version {} but should be {}'.format(
-                    (version - 1),
+                'Version mismatch comment_version {} submitted {}, skipping'.format(
                     comment_version,
+                    version
                 )
             )
-            return
+            raise CommentVersionMismatch()
+
         comment_history = ChangesetCommentHistory()
         comment_history.comment_id = comment_id
         comment_history.version = comment_version

rhodecode/public/css/select2.less

@@ -148,6 +148,38 b' select.select2{height:28px;visibility:hi'
   margin: 0;
 }
 
+
+.drop-menu-comment-history {
+    .drop-menu-core;
+    border: none;
+    padding: 0 6px 0 0;
+    width: auto;
+    min-width: 0;
+    margin: 0;
+    position: relative;
+    display: inline-block;
+    line-height: 1em;
+    z-index: 2;
+    cursor: pointer;
+
+    a {
+        display:block;
+        padding: 0;
+        position: relative;
+
+        &:after {
+            position: absolute;
+            content: "\00A0\25BE";
+            right: -0.80em;
+            line-height: 1em;
+            top: -0.20em;
+            width: 1em;
+            font-size: 16px;
+        }
+    }
+
+}
+
 .field-sm .drop-menu {
     padding: 1px 0 0 0;
     a {

rhodecode/public/js/src/rhodecode/comments.js

@@ -496,6 +496,43 b' var _submitAjaxPOST = function(url, post'
     return CommentForm;
 });
 
+/* selector for comment versions */
+var initVersionSelector = function(selector, initialData) {
+
+    var formatResult = function(result, container, query, escapeMarkup) {
+
+        return renderTemplate('commentVersion', {
+            show_disabled: true,
+            version: result.comment_version,
+            user_name: result.comment_author_username,
+            gravatar_url: result.comment_author_gravatar,
+            size: 16,
+            timeago_component: result.comment_created_on,
+        })
+    };
+
+    $(selector).select2({
+        placeholder: "Edited",
+        containerCssClass: "drop-menu-comment-history",
+        dropdownCssClass: "drop-menu-dropdown",
+        dropdownAutoWidth: true,
+        minimumResultsForSearch: -1,
+        data: initialData,
+        formatResult: formatResult,
+    });
+
+    $(selector).on('select2-selecting', function (e) {
+        // hide the mast as we later do preventDefault()
+        $("#select2-drop-mask").click();
+        e.preventDefault();
+        e.choice.action();
+    });
+
+    $(selector).on("select2-open", function() {
+        timeagoActivate();
+    });
+};
+
 /* comments controller */
 var CommentsController = function() {
   var mainComment = '#text';
@@ -521,21 +558,8 b' var CommentsController = function() {'
       return false;
   };
 
-   this.showVersion = function (node) {
-       var $node = $(node);
-       var selectedIndex = $node.context.selectedIndex;
-       var option = $node.find('option[value="'+ selectedIndex +'"]');
-       var zero_option = $node.find('option[value="0"]');
-       if (!option){
-           return;
-       }
+   this.showVersion = function (comment_id, comment_history_id) {
 
-       // little trick to cheat onchange and allow to display the same version again
-       $node.context.selectedIndex = 0;
-       zero_option.text(selectedIndex);
-
-       var comment_history_id = option.attr('data-comment-history-id');
-       var comment_id = option.attr('data-comment-id');
        var historyViewUrl = pyroutes.url(
            'repo_commit_comment_history_view',
            {
@@ -557,7 +581,8 b' var CommentsController = function() {'
            });
        };
        _submitAjaxPOST(
-           historyViewUrl, {'csrf_token': CSRF_TOKEN}, successRenderCommit,
+           historyViewUrl, {'csrf_token': CSRF_TOKEN},
+           successRenderCommit,
            failRenderCommit
        );
    };
@@ -863,6 +888,7 b' var CommentsController = function() {'
 
       return commentForm;
   };
+
   this.editComment = function(node) {
       var $node = $(node);
       var $comment = $(node).closest('.comment');
@@ -917,15 +943,16 b' var CommentsController = function() {'
                lineno: lineno,
                f_path: f_path}
            );
+
            // set a CUSTOM submit handler for inline comments.
             commentForm.setHandleFormSubmit(function(o) {
               var text = commentForm.cm.getValue();
               var commentType = commentForm.getCommentType();
-              var resolvesCommentId = commentForm.getResolvesId();
 
               if (text === "") {
                 return;
               }
+
               if (old_comment_text == text) {
                   SwalNoAnimation.fire({
                       title: 'Unable to edit comment',
@@ -937,19 +964,22 b' var CommentsController = function() {'
               var submitEvent = true;
               commentForm.setActionButtonsDisabled(true, excludeCancelBtn, submitEvent);
               commentForm.cm.setOption("readOnly", true);
-              var dropDown = $('#comment_history_for_comment_'+comment_id);
 
-              var version = dropDown.children().last().val()
+              // Read last version known
+              var versionSelector = $('#comment_versions_{0}'.format(comment_id));
+              var version = versionSelector.data('lastVersion');
+
               if(!version){
                   version = 0;
               }
+
               var postData = {
                   'text': text,
                   'f_path': f_path,
                   'line': lineno,
                   'comment_type': commentType,
-                  'csrf_token': CSRF_TOKEN,
                   'version': version,
+                  'csrf_token': CSRF_TOKEN
               };
 
               var submitSuccessCallback = function(json_data) {
@@ -961,30 +991,59 b' var CommentsController = function() {'
                     'csrf_token': CSRF_TOKEN
                 };
 
+                /* Inject new edited version selector */
                 var updateCommentVersionDropDown = function () {
-                    var dropDown = $('#comment_history_for_comment_'+comment_id);
+                    var versionSelectId = '#comment_versions_'+comment_id;
+                    var preLoadVersionData = [
+                        {
+                            id: json_data['comment_version'],
+                            text: "v{0}".format(json_data['comment_version']),
+                            action: function () {
+                                Rhodecode.comments.showVersion(
+                                    json_data['comment_id'],
+                                    json_data['comment_history_id']
+                                )
+                            },
+                            comment_version: json_data['comment_version'],
+                            comment_author_username: json_data['comment_author_username'],
+                            comment_author_gravatar: json_data['comment_author_gravatar'],
+                            comment_created_on: json_data['comment_created_on'],
+                        },
+                    ]
+
+
+                    if ($(versionSelectId).data('select2')) {
+                        var oldData = $(versionSelectId).data('select2').opts.data.results;
+                        $(versionSelectId).select2("destroy");
+                        preLoadVersionData = oldData.concat(preLoadVersionData)
+                    }
+
+                    initVersionSelector(versionSelectId, {results: preLoadVersionData});
+
                     $comment.attr('data-comment-text', btoa(text));
-                    var version = json_data['comment_version']
-                    var option = new Option(version, version);
-                    var $option = $(option);
-                    $option.attr('data-comment-history-id', json_data['comment_history_id']);
-                    $option.attr('data-comment-id', json_data['comment_id']);
-                    dropDown.append(option);
-                    dropDown.parent().show();
+
+                    var versionSelector = $('#comment_versions_'+comment_id);
+
+                    // set lastVersion so we know our last edit version
+                    versionSelector.data('lastVersion', json_data['comment_version'])
+                    versionSelector.parent().show();
                 }
                 updateCommentVersionDropDown();
+
                 // by default we reset state of comment preserving the text
                 var failRenderCommit = function(jqXHR, textStatus, errorThrown) {
-                    var prefix = "Error while editing of comment.\n"
+                    var prefix = "Error while editing this comment.\n"
                     var message = formatErrorMessage(jqXHR, textStatus, errorThrown, prefix);
                     ajaxErrorSwal(message);
+                };
 
-                };
                 var successRenderCommit = function(o){
                     $comment.show();
                     $comment[0].lastElementChild.innerHTML = o;
-                }
-                var previewUrl = pyroutes.url('repo_commit_comment_preview',
+                };
+
+                var previewUrl = pyroutes.url(
+                    'repo_commit_comment_preview',
                 {'repo_name': templateContext.repo_name,
                 'commit_id': templateContext.commit_data.commit_id});
 
@@ -1000,11 +1059,6 b' var CommentsController = function() {'
 
                   $comments.find('.cb-comment-add-button').before(html);
 
-                  //mark visually which comment was resolved
-                  if (resolvesCommentId) {
-                      commentForm.markCommentResolved(resolvesCommentId);
-                  }
-
                  //  run global callback on submit
                   commentForm.globalSubmitSuccessCallback();
 
@@ -1029,16 +1083,25 b' var CommentsController = function() {'
               var submitFailCallback = function(jqXHR, textStatus, errorThrown) {
                   var prefix = "Error while editing comment.\n"
                   var message = formatErrorMessage(jqXHR, textStatus, errorThrown, prefix);
+                  if (jqXHR.status == 409){
+                    message = 'This comment was probably changed somewhere else. Please reload the content of this comment.'
+                    ajaxErrorSwal(message, 'Comment version mismatch.');
+                  } else {
                   ajaxErrorSwal(message);
+                  }
+
                   commentForm.resetCommentFormState(text)
               };
               commentForm.submitAjaxPOST(
-                  commentForm.submitUrl, postData, submitSuccessCallback, submitFailCallback);
+                  commentForm.submitUrl, postData,
+                  submitSuccessCallback,
+                  submitFailCallback);
             });
       }
 
       $form.addClass('comment-inline-form-open');
   };
+
   this.createComment = function(node, resolutionComment) {
       var resolvesCommentId = resolutionComment || null;
       var $node = $(node);

rhodecode/public/js/src/rhodecode/utils/ajax.js

@@ -130,10 +130,13 b' function formatErrorMessage(jqXHR, textS'
     }
 }
 
-function ajaxErrorSwal(message) {
+function ajaxErrorSwal(message, title) {
+
+    var title = (typeof title !== 'undefined') ? title : _gettext('Ajax Request Error');
+
     SwalNoAnimation.fire({
         icon: 'error',
-        title: _gettext('Ajax Request Error'),
+        title: title,
         html: '<span style="white-space: pre-line">{0}</span>'.format(message),
         showClass: {
             popup: 'swal2-noanimation',

rhodecode/templates/changeset/changeset_file_comment.mako

@@ -66,31 +66,47 b''
           <div class="date">
               ${h.age_component(comment.modified_at, time_is_local=True)}
           </div>
+          <%
+          comment_version_selector = 'comment_versions_{}'.format(comment.comment_id)
+          %>
+
           % if comment.history:
               <div class="date">
-                  <span class="comment-area-text">${_('Edited')}:</span>
-                  <select class="comment-version-select" id="comment_history_for_comment_${comment.comment_id}"
-                          onchange="return Rhodecode.comments.showVersion(this)"
-                          name="comment_type">
 
-                      <option style="display: none" value="0">---</option>
+                 <input id="${comment_version_selector}" name="${comment_version_selector}"
+                        type="hidden"
+                        data-last-version="${comment.history[-1].version}">
+
+                 <script type="text/javascript">
+
+                    var preLoadVersionData = [
                       % for comment_history in comment.history:
-                          <option data-comment-history-id="${comment_history.comment_history_id}"
-                                  data-comment-id="${comment.comment_id}"
-                                  value="${comment_history.version}">
-                              ${comment_history.version}
-                          </option>
+                            {
+                                id: ${comment_history.comment_history_id},
+                                text: 'v${comment_history.version}',
+                                action: function () {
+                                    Rhodecode.comments.showVersion(
+                                        "${comment.comment_id}",
+                                        "${comment_history.comment_history_id}"
+                                    )
+                                },
+                                comment_version: "${comment_history.version}",
+                                comment_author_username: "${comment_history.author.username}",
+                                comment_author_gravatar: "${h.gravatar_url(comment_history.author.email, 16)}",
+                                comment_created_on: '${h.age_component(comment_history.created_on, time_is_local=True)}',
+                            },
                       % endfor
-                  </select>
+                        ]
+                    initVersionSelector("#${comment_version_selector}", {results: preLoadVersionData});
+
+                  </script>
+
               </div>
           % else:
               <div class="date" style="display: none">
-                  <span class="comment-area-text">${_('Edited')}</span>
-                  <select class="comment-version-select" id="comment_history_for_comment_${comment.comment_id}"
-                          onchange="return Rhodecode.comments.showVersion(this)"
-                          name="comment_type">
-                      <option style="display: none" value="0">---</option>
-                  </select>
+                <input id="${comment_version_selector}" name="${comment_version_selector}"
+                       type="hidden"
+                       data-last-version="0">
               </div>
           %endif
           % if inline:
@@ -169,12 +185,8 b''
             %if not outdated_at_ver and (not comment.pull_request or (comment.pull_request and not comment.pull_request.is_closed())):
                ## permissions to delete
                %if comment.immutable is False and (c.is_super_admin or h.HasRepoPermissionAny('repository.admin')(c.repo_name) or comment.author.user_id == c.rhodecode_user.user_id):
-                    %if comment.comment_type == 'note':
                        <a onclick="return Rhodecode.comments.editComment(this);"
                           class="edit-comment">${_('Edit')}</a>
-                    %else:
-                        <a class="tooltip edit-comment link-disabled" disabled="disabled" title="${_('Action unavailable')}">${_('Edit')}</a>
-                    %endif
                        | <a onclick="return Rhodecode.comments.deleteComment(this);"
                           class="delete-comment">${_('Delete')}</a>
                %else:

rhodecode/templates/ejs_templates/templates.html

@@ -130,6 +130,34 b' var CG = new ColorGenerator();'
 </script>
 
 
+<script id="ejs_commentVersion" type="text/template" class="ejsTemplate">
+
+<%
+if (size > 16) {
+    var gravatar_class = 'gravatar gravatar-large';
+} else {
+    var gravatar_class = 'gravatar';
+}
+
+%>
+
+<%
+if (show_disabled) {
+    var user_cls = 'user user-disabled';
+} else {
+    var user_cls = 'user';
+}
+
+%>
+
+<div style='line-height: 20px'>
+    <img style="margin: -3px 0" class="<%= gravatar_class %>" height="<%= size %>" width="<%= size %>" src="<%- gravatar_url -%>">
+    <strong><%- user_name -%></strong>, <code>v<%- version -%></code> edited <%- timeago_component -%>
+</div>
+
+</script>
+
+
 </div>
 
 <script>