comments: use unified aggregation of comments counters.

- use 1 source of count and display objects
- scroll to for hidden comments will show them before scroll
- properly show how many comments we're displaying for each version

rhodecode/controllers/pullrequests.py

@@ -807,49 +807,43 @@ class PullrequestsController(BaseRepoController):
             c.approval_msg = _('Reviewer approval is pending.')
             c.pr_merge_status = False
 
-        # inline comments
-        inline_comments = cc_model.get_inline_comments(
-            c.rhodecode_db_repo.repo_id, pull_request=pull_request_id)
-
-        _inline_cnt, c.inline_versions = cc_model.get_inline_comments_count(
-            inline_comments, version=at_version, include_aggregates=True)
-
         c.versions = pull_request_display_obj.versions()
+        c.at_version = at_version
         c.at_version_num = at_version if at_version and at_version != 'latest' else None
         c.at_version_pos = ChangesetComment.get_index_from_version(
             c.at_version_num, c.versions)
 
-        is_outdated = lambda co: \
+        # GENERAL COMMENTS with versions #
-            not c.at_version_num \
+        q = cc_model._all_general_comments_of_pull_request(pull_request_latest)
-            or co.pull_request_version_id <= c.at_version_num
+        general_comments = q.order_by(ChangesetComment.pull_request_version_id.asc())
 
-        # inline_comments_until_version
+        # pick comments we want to render at current version
-        if c.at_version_num:
+        c.comment_versions = cc_model.aggregate_comments(
-            # if we use version, then do not show later comments
+            general_comments, c.versions, c.at_version_num)
-            # than current version
+        c.comments = c.comment_versions[c.at_version_num]['until']
-            paths = collections.defaultdict(lambda: collections.defaultdict(list))
-            for fname, per_line_comments in inline_comments.iteritems():
-                for lno, comments in per_line_comments.iteritems():
-                    for co in comments:
-                        if co.pull_request_version_id and is_outdated(co):
-                            paths[co.f_path][co.line_no].append(co)
-            inline_comments = paths
 
-        # outdated comments
+        # INLINE COMMENTS with versions  #
-        c.outdated_cnt = 0
+        q = cc_model._all_inline_comments_of_pull_request(pull_request_latest)
-        if CommentsModel.use_outdated_comments(pull_request_latest):
+        inline_comments = q.order_by(ChangesetComment.pull_request_version_id.asc())
-            outdated_comments = cc_model.get_outdated_comments(
+        c.inline_versions = cc_model.aggregate_comments(
-                c.rhodecode_db_repo.repo_id,
+            inline_comments, c.versions, c.at_version_num, inline=True)
-                pull_request=pull_request_at_ver)
 
-            # Count outdated comments and check for deleted files
+        # if we use version, then do not show later comments
-            is_outdated = lambda co: \
+        # than current version
-                not c.at_version_num \
+        paths = collections.defaultdict(lambda: collections.defaultdict(list))
-                or co.pull_request_version_id < c.at_version_num
+        for co in inline_comments:
-            for file_name, lines in outdated_comments.iteritems():
+            if c.at_version_num:
-                for comments in lines.values():
+                # pick comments that are at least UPTO given version, so we
-                    comments = [comm for comm in comments if is_outdated(comm)]
+                # don't render comments for higher version
-                    c.outdated_cnt += len(comments)
+                should_render = co.pull_request_version_id and \
+                                co.pull_request_version_id <= c.at_version_num
+            else:
+                # showing all, for 'latest'
+                should_render = True
+
+            if should_render:
+                paths[co.f_path][co.line_no].append(co)
+        inline_comments = paths
 
         # load compare data into template context
         self._load_compare_data(pull_request_at_ver, inline_comments)
@@ -860,10 +854,6 @@ class PullrequestsController(BaseRepoController):
         # We need to swap that here to generate it properly on the html side
         c.target_repo = c.source_repo
 
-        # general comments
-        c.comments = cc_model.get_comments(
-            c.rhodecode_db_repo.repo_id, pull_request=pull_request_id)
-
         if c.allowed_to_update:
             force_close = ('forced_closed', _('Close Pull Request'))
             statuses = ChangesetStatus.STATUSES + [force_close]
@@ -874,7 +864,6 @@ class PullrequestsController(BaseRepoController):
         c.ancestor = None  # TODO: add ancestor here
         c.pull_request = pull_request_display_obj
         c.pull_request_latest = pull_request_latest
-        c.at_version = at_version
 
         c.changes = None
         c.file_changes = None

rhodecode/model/comment.py

@@ -39,7 +39,7 @@ from rhodecode.lib.utils import action_logger
 from rhodecode.lib.utils2 import extract_mentioned_users
 from rhodecode.model import BaseModel
 from rhodecode.model.db import (
-    ChangesetComment, User, Notification, PullRequest)
+    ChangesetComment, User, Notification, PullRequest, AttributeDict)
 from rhodecode.model.notification import NotificationModel
 from rhodecode.model.meta import Session
 from rhodecode.model.settings import VcsSettingsModel
@@ -83,6 +83,52 @@ class CommentsModel(BaseModel):
             log.error(traceback.format_exc())
         return global_renderer
 
+    def aggregate_comments(self, comments, versions, show_version, inline=False):
+        # group by versions, and count until, and display objects
+
+        comment_groups = collections.defaultdict(list)
+        [comment_groups[
+             _co.pull_request_version_id].append(_co) for _co in comments]
+
+        def yield_comments(pos):
+            for co in comment_groups[pos]:
+                yield co
+
+        comment_versions = collections.defaultdict(
+            lambda: collections.defaultdict(list))
+        prev_prvid = -1
+        # fake last entry with None, to aggregate on "latest" version which
+        # doesn't have an pull_request_version_id
+        for ver in versions + [AttributeDict({'pull_request_version_id': None})]:
+            prvid = ver.pull_request_version_id
+            if prev_prvid == -1:
+                prev_prvid = prvid
+
+            for co in yield_comments(prvid):
+                comment_versions[prvid]['at'].append(co)
+
+            # save until
+            current = comment_versions[prvid]['at']
+            prev_until = comment_versions[prev_prvid]['until']
+            cur_until = prev_until + current
+            comment_versions[prvid]['until'].extend(cur_until)
+
+            # save outdated
+            if inline:
+                outdated = [x for x in cur_until
+                            if x.outdated_at_version(show_version)]
+            else:
+                outdated = [x for x in cur_until
+                            if x.older_than_version(show_version)]
+            display = [x for x in cur_until if x not in outdated]
+
+            comment_versions[prvid]['outdated'] = outdated
+            comment_versions[prvid]['display'] = display
+
+            prev_prvid = prvid
+
+        return comment_versions
+
     def create(self, text, repo, user, commit_id=None, pull_request=None,
                f_path=None, line_no=None, status_change=None,
                status_change_type=None, comment_type=None,
@@ -376,18 +422,14 @@ class CommentsModel(BaseModel):
         return self._group_comments_by_path_and_line_number(q)
 
     def get_inline_comments_count(self, inline_comments, skip_outdated=True,
-                                  version=None, include_aggregates=False):
+                                  version=None):
-        version_aggregates = collections.defaultdict(list)
         inline_cnt = 0
         for fname, per_line_comments in inline_comments.iteritems():
             for lno, comments in per_line_comments.iteritems():
                 for comm in comments:
-                    version_aggregates[comm.pull_request_version_id].append(comm)
                     if not comm.outdated_at_version(version) and skip_outdated:
                         inline_cnt += 1
 
-        if include_aggregates:
-            return inline_cnt, version_aggregates
         return inline_cnt
 
     def get_outdated_comments(self, repo_id, pull_request):
@@ -511,6 +553,13 @@ class CommentsModel(BaseModel):
             .filter(ChangesetComment.pull_request == pull_request)
         return comments
 
+    def _all_general_comments_of_pull_request(self, pull_request):
+        comments = Session().query(ChangesetComment)\
+            .filter(ChangesetComment.line_no == None)\
+            .filter(ChangesetComment.f_path == None)\
+            .filter(ChangesetComment.pull_request == pull_request)
+        return comments
+
     @staticmethod
     def use_outdated_comments(pull_request):
         settings_model = VcsSettingsModel(repo=pull_request.target_repo)

rhodecode/model/db.py

@@ -2959,6 +2959,15 @@ class ChangesetComment(Base, BaseModel):
         """
         return self.outdated and self.pull_request_version_id != version
 
+    def older_than_version(self, version):
+        """
+        Checks if comment is made from previous version than given
+        """
+        if version is None:
+            return self.pull_request_version_id is not None
+
+        return self.pull_request_version_id < version
+
     @property
     def resolved(self):
         return self.resolved_by[0] if self.resolved_by else None
@@ -2973,9 +2982,9 @@ class ChangesetComment(Base, BaseModel):
 
     def __repr__(self):
         if self.comment_id:
-            return '<DB:ChangesetComment #%s>' % self.comment_id
+            return '<DB:Comment #%s>' % self.comment_id
         else:
-            return '<DB:ChangesetComment at %#x>' % id(self)
+            return '<DB:Comment at %#x>' % id(self)
 
 
 class ChangesetStatus(Base, BaseModel):

rhodecode/public/css/comments.less

@@ -51,7 +51,7 @@ tr.inline-comments div {
     float: left;
 
     padding: 0.4em 0.4em;
-    margin: 2px 5px 0px -10px;
+    margin: 3px 5px 0px -10px;
     display: inline-block;
     min-height: 0;
 

rhodecode/templates/changeset/changeset.mako

@@ -180,7 +180,7 @@
     <%namespace name="comment" file="/changeset/changeset_file_comment.mako"/>
 
     ## render comments
-    ${comment.generate_comments()}
+    ${comment.generate_comments(c.comments)}
 
     ## main comment form and it status
     ${comment.comments(h.url('changeset_comment', repo_name=c.repo_name, revision=c.commit.raw_id),

rhodecode/templates/changeset/changeset_file_comment.mako

@@ -6,8 +6,13 @@
 <%namespace name="base" file="/base/base.mako"/>
 
 <%def name="comment_block(comment, inline=False)">
-  <% outdated_at_ver = comment.outdated_at_version(getattr(c, 'at_version', None)) %>
   <% pr_index_ver = comment.get_index_version(getattr(c, 'versions', [])) %>
+  % if inline:
+      <% outdated_at_ver = comment.outdated_at_version(getattr(c, 'at_version_num', None)) %>
+  % else:
+      <% outdated_at_ver = comment.older_than_version(getattr(c, 'at_version_num', None)) %>
+  % endif
+
 
   <div class="comment
              ${'comment-inline' if inline else 'comment-general'}
@@ -83,16 +88,19 @@
           <div class="comment-links-block">
 
             % if inline:
-              % if outdated_at_ver:
                   <div class="pr-version-inline">
                     <a href="${h.url.current(version=comment.pull_request_version_id, anchor='comment-{}'.format(comment.comment_id))}">
-                    <code class="pr-version-num">
+                    % if outdated_at_ver:
-                        outdated ${'v{}'.format(pr_index_ver)}
+                        <code class="pr-version-num" title="${_('Outdated comment from pull request version {0}').format(pr_index_ver)}">
-                    </code>
+                            outdated ${'v{}'.format(pr_index_ver)} |
+                        </code>
+                    % elif pr_index_ver:
+                        <code class="pr-version-num" title="${_('Comment from pull request version {0}').format(pr_index_ver)}">
+                            ${'v{}'.format(pr_index_ver)} |
+                        </code>
+                    % endif
                     </a>
                   </div>
-                  |
-              % endif
             % else:
                 % if comment.pull_request_version_id and pr_index_ver:
                     |
@@ -102,7 +110,7 @@
                             ${_('Outdated comment from pull request version {}').format(pr_index_ver)}
                         </a>
                       % else:
-                        <div class="tooltip" title="${_('Comment from pull request version {0}').format(pr_index_ver)}">
+                        <div title="${_('Comment from pull request version {0}').format(pr_index_ver)}">
                             <a href="${h.url('pullrequest_show',repo_name=comment.pull_request.target_repo.repo_name,pull_request_id=comment.pull_request.pull_request_id, version=comment.pull_request_version_id)}">
                             <code class="pr-version-num">
                                 ${'v{}'.format(pr_index_ver)}
@@ -143,9 +151,9 @@
 </%def>
 
 ## generate main comments
-<%def name="generate_comments(include_pull_request=False, is_pull_request=False)">
+<%def name="generate_comments(comments, include_pull_request=False, is_pull_request=False)">
   <div id="comments">
-    %for comment in c.comments:
+    %for comment in comments:
         <div id="comment-tr-${comment.comment_id}">
           ## only render comments that are not from pull request, or from
           ## pull request and a status change

rhodecode/templates/pullrequests/pullrequest_show.mako

@@ -35,6 +35,7 @@
     templateContext.pull_request_data.pull_request_id = ${c.pull_request.pull_request_id};
 </script>
 <div class="box">
+
   <div class="title">
     ${self.repo_page_title(c.rhodecode_db_repo)}
   </div>
@@ -42,6 +43,7 @@
   ${self.breadcrumbs()}
 
   <div class="box pr-summary">
+
       <div class="summary-details block-left">
         <% summary = lambda n:{False:'summary-short'}.get(n) %>
         <div class="pr-details-title">
@@ -173,10 +175,13 @@
                        ## CURRENTLY SELECT PR VERSION
                        <tr class="version-pr" style="display: ${'' if c.at_version_num is None else 'none'}">
                            <td>
-                               % if c.at_version in [None, 'latest']:
+                               % if c.at_version_num is None:
                                 <i class="icon-ok link"></i>
                                % else:
-                                <i class="icon-comment"></i> <code>${len(c.inline_versions[None])}</code>
+                                <i class="icon-comment"></i>
+                                   <code>
+                                   ${len(c.comment_versions[None]['at'])}/${len(c.inline_versions[None]['at'])}
+                                   </code>
                                % endif
                            </td>
                            <td>
@@ -203,36 +208,40 @@
 
                        ## SHOW ALL VERSIONS OF PR
                        <% ver_pr = None %>
-                       % for data in reversed(list(enumerate(c.versions, 1))):
-                       <% ver_pos = data[0] %>
-                       <% ver = data[1] %>
-                       <% ver_pr = ver.pull_request_version_id %>
 
-                       <tr class="version-pr" style="display: ${'' if c.at_version == ver_pr else 'none'}">
+                       % for data in reversed(list(enumerate(c.versions, 1))):
-                           <td>
+                           <% ver_pos = data[0] %>
-                               % if c.at_version == ver_pr:
+                           <% ver = data[1] %>
-                                <i class="icon-ok link"></i>
+                           <% ver_pr = ver.pull_request_version_id %>
-                               % else:
+
-                                <i class="icon-comment"></i> <code>${len(c.inline_versions[ver_pr])}</code>
+                           <tr class="version-pr" style="display: ${'' if c.at_version_num == ver_pr else 'none'}">
-                               % endif
+                               <td>
-                           </td>
+                                   % if c.at_version_num == ver_pr:
-                           <td>
+                                    <i class="icon-ok link"></i>
-                                <code class="tooltip" title="${_('Comment from pull request version {0}').format(ver_pos)}">
+                                   % else:
-                                    <a href="${h.url.current(version=ver_pr)}">v${ver_pos}</a>
+                                    <i class="icon-comment"></i>
-                                </code>
+                                       <code class="tooltip" title="${_('Comment from pull request version {0}, general:{1} inline{2}').format(ver_pos, len(c.comment_versions[ver_pr]['at']), len(c.inline_versions[ver_pr]['at']))}">
-                           </td>
+                                           ${len(c.comment_versions[ver_pr]['at'])}/${len(c.inline_versions[ver_pr]['at'])}
-                           <td>
+                                       </code>
-                               <code>${ver.source_ref_parts.commit_id[:6]}</code>
+                                   % endif
-                           </td>
+                               </td>
-                           <td>
+                               <td>
-                               ${_('created')} ${h.age_component(ver.updated_on)}
+                                    <code>
-                           </td>
+                                        <a href="${h.url.current(version=ver_pr)}">v${ver_pos}</a>
-                           <td align="right">
+                                    </code>
-                               % if c.at_version == ver_pr:
+                               </td>
-                               <span id="show-pr-versions" class="btn btn-link" onclick="$('.version-pr').show(); $(this).hide(); return false">${_('Show all versions')}</span>
+                               <td>
-                               % endif
+                                   <code>${ver.source_ref_parts.commit_id[:6]}</code>
-                           </td>
+                               </td>
-                       </tr>
+                               <td>
+                                   ${_('created')} ${h.age_component(ver.updated_on)}
+                               </td>
+                               <td align="right">
+                                   % if c.at_version_num == ver_pr:
+                                   <span id="show-pr-versions" class="btn btn-link" onclick="$('.version-pr').show(); $(this).hide(); return false">${_('Show all versions')}</span>
+                                   % endif
+                               </td>
+                           </tr>
                        % endfor
 
                        ## show comment/inline comments summary
@@ -240,28 +249,39 @@
                            <td>
                            </td>
 
-                           <% inline_comm_count_ver = len(c.inline_versions[ver_pr])%>
                            <td colspan="4" style="border-top: 1px dashed #dbd9da">
-                            ${_('Comments for this version')}:
+                            <% outdated_comm_count_ver = len(c.inline_versions[c.at_version_num]['outdated']) %>
-                            %if c.comments:
+                            <% general_outdated_comm_count_ver = len(c.comment_versions[c.at_version_num]['outdated']) %>
-                                <a href="#comments">${_("%d General ") % len(c.comments)}</a>
+
+
+                            % if c.at_version:
+                                <% inline_comm_count_ver = len(c.inline_versions[c.at_version_num]['display']) %>
+                                <% general_comm_count_ver = len(c.comment_versions[c.at_version_num]['display']) %>
+                                ${_('Comments at this version')}:
+                            % else:
+                                <% inline_comm_count_ver = len(c.inline_versions[c.at_version_num]['until']) %>
+                                <% general_comm_count_ver = len(c.comment_versions[c.at_version_num]['until']) %>
+                                ${_('Comments for this pull request')}:
+                            % endif
+
+                            %if general_comm_count_ver:
+                                <a href="#comments">${_("%d General ") % general_comm_count_ver}</a>
                             %else:
-                                ${_("%d General ") % len(c.comments)}
+                                ${_("%d General ") % general_comm_count_ver}
                             %endif
 
-                            <% inline_comm_count_ver = len(c.inline_versions[c.at_version_num])%>
                             %if inline_comm_count_ver:
                                , <a href="#" onclick="return Rhodecode.comments.nextComment();" id="inline-comments-counter">${_("%d Inline") % inline_comm_count_ver}</a>
                             %else:
                                , ${_("%d Inline") % inline_comm_count_ver}
                             %endif
 
-                            %if c.outdated_cnt:
+                            %if outdated_comm_count_ver:
-                              , <a href="#" onclick="showOutdated(); Rhodecode.comments.nextOutdatedComment(); return false;">${_("%d Outdated") % c.outdated_cnt}</a>
+                              , <a href="#" onclick="showOutdated(); Rhodecode.comments.nextOutdatedComment(); return false;">${_("%d Outdated") % outdated_comm_count_ver}</a>
                                 <a href="#" class="showOutdatedComments" onclick="showOutdated(this); return false;"> | ${_('show outdated comments')}</a>
                                 <a href="#" class="hideOutdatedComments" style="display: none" onclick="hideOutdated(this); return false;"> | ${_('hide outdated comments')}</a>
                             %else:
-                              , ${_("%d Outdated") % c.outdated_cnt}
+                              , ${_("%d Outdated") % outdated_comm_count_ver}
                             %endif
                            </td>
                        </tr>
@@ -459,7 +479,24 @@ Changed files:
       <%namespace name="comment" file="/changeset/changeset_file_comment.mako"/>
 
       ## render general comments
-      ${comment.generate_comments(include_pull_request=True, is_pull_request=True)}
+
+      <div id="comment-tr-show">
+          <div class="comment">
+            <div class="meta">
+                % if general_outdated_comm_count_ver:
+                    % if general_outdated_comm_count_ver == 1:
+                        ${_('there is {num} general comment from older versions').format(num=general_outdated_comm_count_ver)},
+                        <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show it')}</a>
+                    % else:
+                        ${_('there are {num} general comments from older versions').format(num=general_outdated_comm_count_ver)},
+                        <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show them')}</a>
+                    % endif
+                % endif
+            </div>
+          </div>
+      </div>
+
+      ${comment.generate_comments(c.comments, include_pull_request=True, is_pull_request=True)}
 
       % if not c.pull_request.is_closed():
         ## main comment form and it status
@@ -472,11 +509,20 @@ Changed files:
       <script type="text/javascript">
         if (location.hash) {
           var result = splitDelimitedHash(location.hash);
-          var line = $('html').find(result.loc);
+            var line = $('html').find(result.loc);
+            // show hidden comments if we use location.hash
+            if (line.hasClass('comment-general')) {
+                $(line).show();
+            } else if (line.hasClass('comment-inline')) {
+                $(line).show();
+                var $cb = $(line).closest('.cb');
+                $cb.removeClass('cb-collapsed')
+            }
             if (line.length > 0){
                 offsetScroll(line, 70);
             }
         }
+
         $(function(){
             ReviewerAutoComplete('user');
             // custom code mirror
@@ -544,15 +590,14 @@ Changed files:
             ReviewersPanel.init();
 
             showOutdated = function(self){
-                $('.comment-outdated').show();
+                $('.comment-inline.comment-outdated').show();
                 $('.filediff-outdated').show();
                 $('.showOutdatedComments').hide();
                 $('.hideOutdatedComments').show();
-
             };
 
             hideOutdated = function(self){
-                $('.comment-outdated').hide();
+                $('.comment-inline.comment-outdated').hide();
                 $('.filediff-outdated').hide();
                 $('.hideOutdatedComments').hide();
                 $('.showOutdatedComments').show();