pull-requests: expose version browsing of pull requests.

- show nav for browsing each version of pr
- show detailed changes for each of version
- update comment template to indicate the changes were due to update of a pr

rhodecode/controllers/pullrequests.py

@@ -21,11 +21,13 @@
 """
 pull requests controller for rhodecode for initializing pull requests
 """
+import types
 
 import peppercorn
 import formencode
 import logging
 
+
 from webob.exc import HTTPNotFound, HTTPForbidden, HTTPBadRequest
 from pylons import request, tmpl_context as c, url
 from pylons.controllers.util import redirect
@@ -46,8 +48,9 @@ from rhodecode.lib.channelstream import channelstream_request
 from rhodecode.lib.compat import OrderedDict
 from rhodecode.lib.utils import jsonify
 from rhodecode.lib.utils2 import (
-    safe_int, safe_str, str2bool, safe_unicode, StrictAttributeDict)
-from rhodecode.lib.vcs.backends.base import EmptyCommit, UpdateFailureReason
+    safe_int, safe_str, str2bool, safe_unicode)
+from rhodecode.lib.vcs.backends.base import (
+    EmptyCommit, UpdateFailureReason, EmptyRepository)
 from rhodecode.lib.vcs.exceptions import (
     EmptyRepositoryError, CommitDoesNotExistError, RepositoryRequirementError,
     NodeDoesNotExistError)
@@ -680,7 +683,13 @@ class PullrequestsController(BaseRepoController):
     def _get_pr_version(self, pull_request_id, version=None):
         pull_request_id = safe_int(pull_request_id)
         at_version = None
-        if version:
+
+        if version and version == 'latest':
+            pull_request_ver = PullRequest.get(pull_request_id)
+            pull_request_obj = pull_request_ver
+            _org_pull_request_obj = pull_request_obj
+            at_version = 'latest'
+        elif version:
             pull_request_ver = PullRequestVersion.get_or_404(version)
             pull_request_obj = pull_request_ver
             _org_pull_request_obj = pull_request_ver.pull_request
@@ -688,57 +697,58 @@ class PullrequestsController(BaseRepoController):
         else:
             _org_pull_request_obj = pull_request_obj = PullRequest.get_or_404(pull_request_id)
 
-        class PullRequestDisplay(object):
-            """
-            Special object wrapper for showing PullRequest data via Versions
-            It mimics PR object as close as possible. This is read only object
-            just for display
-            """
-            def __init__(self, attrs):
-                self.attrs = attrs
-                # internal have priority over the given ones via attrs
-                self.internal = ['versions']
-
-            def __getattr__(self, item):
-                if item in self.internal:
-                    return getattr(self, item)
-                try:
-                    return self.attrs[item]
-                except KeyError:
-                    raise AttributeError(
-                        '%s object has no attribute %s' % (self, item))
-
-            def versions(self):
-                return pull_request_obj.versions.order_by(
-                    PullRequestVersion.pull_request_version_id).all()
-
-            def is_closed(self):
-                return pull_request_obj.is_closed()
-
-        attrs = StrictAttributeDict(pull_request_obj.get_api_data())
-
-        attrs.author = StrictAttributeDict(
-            pull_request_obj.author.get_api_data())
-        if pull_request_obj.target_repo:
-            attrs.target_repo = StrictAttributeDict(
-                pull_request_obj.target_repo.get_api_data())
-            attrs.target_repo.clone_url = pull_request_obj.target_repo.clone_url
-
-        if pull_request_obj.source_repo:
-            attrs.source_repo = StrictAttributeDict(
-                pull_request_obj.source_repo.get_api_data())
-            attrs.source_repo.clone_url = pull_request_obj.source_repo.clone_url
-
-        attrs.source_ref_parts = pull_request_obj.source_ref_parts
-        attrs.target_ref_parts = pull_request_obj.target_ref_parts
-
-        attrs.shadow_merge_ref = _org_pull_request_obj.shadow_merge_ref
-
-        pull_request_display_obj = PullRequestDisplay(attrs)
-
+        pull_request_display_obj = PullRequest.get_pr_display_object(
+            pull_request_obj, _org_pull_request_obj)
         return _org_pull_request_obj, pull_request_obj, \
                pull_request_display_obj, at_version
 
+    def _get_pr_version_changes(self, version, pull_request_latest):
+        """
+        Generate changes commits, and diff data based on the current pr version
+        """
+
+        #TODO(marcink): save those changes as JSON metadata for chaching later.
+
+        # fake the version to add the "initial" state object
+        pull_request_initial = PullRequest.get_pr_display_object(
+            pull_request_latest, pull_request_latest,
+            internal_methods=['get_commit', 'versions'])
+        pull_request_initial.revisions = []
+        pull_request_initial.source_repo.get_commit = types.MethodType(
+            lambda *a, **k: EmptyCommit(), pull_request_initial)
+        pull_request_initial.source_repo.scm_instance = types.MethodType(
+            lambda *a, **k: EmptyRepository(), pull_request_initial)
+
+        _changes_versions = [pull_request_latest] + \
+                            list(reversed(c.versions)) + \
+                            [pull_request_initial]
+
+        if version == 'latest':
+            index = 0
+        else:
+            for pos, prver in enumerate(_changes_versions):
+                ver = getattr(prver, 'pull_request_version_id', -1)
+                if ver == safe_int(version):
+                    index = pos
+                    break
+            else:
+                index = 0
+
+        cur_obj = _changes_versions[index]
+        prev_obj = _changes_versions[index + 1]
+
+        old_commit_ids = set(prev_obj.revisions)
+        new_commit_ids = set(cur_obj.revisions)
+
+        changes = PullRequestModel()._calculate_commit_id_changes(
+            old_commit_ids, new_commit_ids)
+
+        old_diff_data, new_diff_data = PullRequestModel()._generate_update_diffs(
+            cur_obj, prev_obj)
+        file_changes = PullRequestModel()._calculate_file_changes(
+            old_diff_data, new_diff_data)
+        return changes, file_changes
+
     @LoginRequired()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
                                    'repository.admin')
@@ -763,7 +773,7 @@ class PullrequestsController(BaseRepoController):
             pull_request_at_ver)
 
         pr_closed = pull_request_latest.is_closed()
-        if at_version:
+        if at_version and not at_version == 'latest':
             c.allowed_to_change_status = False
             c.allowed_to_update = False
             c.allowed_to_merge = False
@@ -840,11 +850,21 @@ class PullrequestsController(BaseRepoController):
             statuses = ChangesetStatus.STATUSES
         c.commit_statuses = statuses
 
-        c.ancestor = None # TODO: add ancestor here
+        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.versions = pull_request_display_obj.versions()
+        c.changes = None
+        c.file_changes = None
+
+        c.show_version_changes = 1
+
+        if at_version and c.show_version_changes:
+            c.changes, c.file_changes = self._get_pr_version_changes(
+                version, pull_request_latest)
+
         return render('/pullrequests/pullrequest_show.html')
 
     @LoginRequired()

rhodecode/lib/utils2.py

@@ -665,7 +665,8 @@ class StrictAttributeDict(dict):
         try:
             return self[attr]
         except KeyError:
-            raise AttributeError('%s object has no attribute %s' % (self, attr))
+            raise AttributeError('%s object has no attribute %s' % (
+                self.__class__, attr))
     __setattr__ = dict.__setitem__
     __delattr__ = dict.__delitem__
 

rhodecode/lib/vcs/backends/base.py

@@ -1442,6 +1442,15 @@ class EmptyChangeset(EmptyCommit):
             self.idx = value
 
 
+class EmptyRepository(BaseRepository):
+    def __init__(self, repo_path=None, config=None, create=False, **kwargs):
+        pass
+
+    def get_diff(self, *args, **kwargs):
+        from rhodecode.lib.vcs.backends.git.diff import GitDiff
+        return GitDiff('')
+
+
 class CollectionGenerator(object):
 
     def __init__(self, repo, commit_ids, collection_size=None, pre_load=None):

rhodecode/model/db.py

@@ -53,7 +53,7 @@ from rhodecode.lib.vcs.backends.base import EmptyCommit, Reference
 from rhodecode.lib.utils2 import (
     str2bool, safe_str, get_commit_safe, safe_unicode, md5_safe,
     time_to_datetime, aslist, Optional, safe_int, get_clone_url, AttributeDict,
-    glob2re)
+    glob2re, StrictAttributeDict)
 from rhodecode.lib.jsonalchemy import MutationObj, MutationList, JsonType
 from rhodecode.lib.ext_json import json
 from rhodecode.lib.caching_query import FromCache
@@ -3213,6 +3213,64 @@ class PullRequest(Base, _PullRequestBase):
                             cascade="all, delete, delete-orphan",
                             lazy='dynamic')
 
+
+    @classmethod
+    def get_pr_display_object(cls, pull_request_obj, org_pull_request_obj,
+                              internal_methods=None):
+
+        class PullRequestDisplay(object):
+            """
+            Special object wrapper for showing PullRequest data via Versions
+            It mimics PR object as close as possible. This is read only object
+            just for display
+            """
+
+            def __init__(self, attrs, internal=None):
+                self.attrs = attrs
+                # internal have priority over the given ones via attrs
+                self.internal = internal or ['versions']
+
+            def __getattr__(self, item):
+                if item in self.internal:
+                    return getattr(self, item)
+                try:
+                    return self.attrs[item]
+                except KeyError:
+                    raise AttributeError(
+                        '%s object has no attribute %s' % (self, item))
+
+            def __repr__(self):
+                return '<DB:PullRequestDisplay #%s>' % self.attrs.get('pull_request_id')
+
+            def versions(self):
+                return pull_request_obj.versions.order_by(
+                    PullRequestVersion.pull_request_version_id).all()
+
+            def is_closed(self):
+                return pull_request_obj.is_closed()
+
+        attrs = StrictAttributeDict(pull_request_obj.get_api_data())
+
+        attrs.author = StrictAttributeDict(
+            pull_request_obj.author.get_api_data())
+        if pull_request_obj.target_repo:
+            attrs.target_repo = StrictAttributeDict(
+                pull_request_obj.target_repo.get_api_data())
+            attrs.target_repo.clone_url = pull_request_obj.target_repo.clone_url
+
+        if pull_request_obj.source_repo:
+            attrs.source_repo = StrictAttributeDict(
+                pull_request_obj.source_repo.get_api_data())
+            attrs.source_repo.clone_url = pull_request_obj.source_repo.clone_url
+
+        attrs.source_ref_parts = pull_request_obj.source_ref_parts
+        attrs.target_ref_parts = pull_request_obj.target_ref_parts
+        attrs.revisions = pull_request_obj.revisions
+
+        attrs.shadow_merge_ref = org_pull_request_obj.shadow_merge_ref
+
+        return PullRequestDisplay(attrs, internal=internal_methods)
+
     def is_closed(self):
         return self.status == self.STATUS_CLOSED
 

rhodecode/public/css/main.less

@@ -1382,6 +1382,16 @@ table.integrations {
     }
 }
 
+.compare_view_commits_title {
+  .disabled {
+    cursor: inherit;
+    &:hover{
+      background-color: inherit;
+      color: inherit;
+    }
+  }
+}
+
 // new entry in group_members
 .td-author-new-entry {
   background-color: rgba(red(@alert1), green(@alert1), blue(@alert1), 0.3);

rhodecode/templates/pullrequests/pullrequest_show.html

@@ -45,7 +45,7 @@
       <div class="summary-details block-left">
         <%summary = lambda n:{False:'summary-short'}.get(n)%>
         <div class="pr-details-title">
-            ${_('Pull request #%s') % c.pull_request.pull_request_id} ${_('From')} ${h.format_date(c.pull_request.created_on)}
+            <a href="${h.url('pull_requests_global', pull_request_id=c.pull_request.pull_request_id)}">${_('Pull request #%s') % c.pull_request.pull_request_id}</a> ${_('From')} ${h.format_date(c.pull_request.created_on)}
             %if c.allowed_to_update:
               <div id="delete_pullrequest" class="pull-right action_button ${'' if c.allowed_to_delete else 'disabled' }" style="clear:inherit;padding: 0">
                   % if c.allowed_to_delete:
@@ -112,22 +112,26 @@
            </div>
 
             ## Link to the shadow repository.
-            %if not c.pull_request.is_closed() and c.pull_request.shadow_merge_ref:
-                <div class="field">
-                    <div class="label-summary">
-                        <label>Merge:</label>
-                    </div>
-                    <div class="input">
-                        <div class="pr-mergeinfo">
-                            %if h.is_hg(c.pull_request.target_repo):
-                                <input type="text"  value="hg clone -u ${c.pull_request.shadow_merge_ref.name} ${c.shadow_clone_url} pull-request-${c.pull_request.pull_request_id}" readonly="readonly">
-                            %elif h.is_git(c.pull_request.target_repo):
-                                <input type="text"  value="git clone --branch ${c.pull_request.shadow_merge_ref.name} ${c.shadow_clone_url} pull-request-${c.pull_request.pull_request_id}" readonly="readonly">
-                            %endif
-                        </div>
-                    </div>
+            <div class="field">
+                <div class="label-summary">
+                    <label>${_('Merge')}:</label>
                 </div>
-            %endif
+                <div class="input">
+                    % if not c.pull_request.is_closed() and c.pull_request.shadow_merge_ref:
+                    <div class="pr-mergeinfo">
+                        %if h.is_hg(c.pull_request.target_repo):
+                            <input type="text"  value="hg clone -u ${c.pull_request.shadow_merge_ref.name} ${c.shadow_clone_url} pull-request-${c.pull_request.pull_request_id}" readonly="readonly">
+                        %elif h.is_git(c.pull_request.target_repo):
+                            <input type="text"  value="git clone --branch ${c.pull_request.shadow_merge_ref.name} ${c.shadow_clone_url} pull-request-${c.pull_request.pull_request_id}" readonly="readonly">
+                        %endif
+                    </div>
+                    % else:
+                    <div class="">
+                        ${_('Shadow repository data not available')}.
+                    </div>
+                    % endif
+                </div>
+            </div>
 
            <div class="field">
             <div class="label-summary">
@@ -187,21 +191,23 @@
 
            <div class="field">
                 <div class="label-summary">
-                <label>${_('Versions')}:</label>
+                <label>${_('Versions')} (${len(c.versions)}):</label>
            </div>
+
            <div>
+           % if c.show_version_changes:
                <table>
                    <tr>
                        <td>
-                           % if c.at_version == None:
+                           % if c.at_version in [None, 'latest']:
                             <i class="icon-ok link"></i>
                            % endif
                        </td>
-                       <td><code><a href="${h.url.current()}">latest</a></code></td>
+                       <td><code><a href="${h.url.current(version='latest')}">latest</a></code></td>
                        <td>
                            <code>${c.pull_request_latest.source_ref_parts.commit_id[:6]}</code>
                        </td>
-                       <td>${_('created')} ${h.age_component(c.pull_request.created_on)}</td>
+                       <td>${_('created')} ${h.age_component(c.pull_request_latest.updated_on)}</td>
                    </tr>
                    % for ver in reversed(c.pull_request.versions()):
                    <tr>
@@ -214,10 +220,36 @@
                        <td>
                            <code>${ver.source_ref_parts.commit_id[:6]}</code>
                        </td>
-                       <td>${_('created')} ${h.age_component(ver.created_on)}</td>
+                       <td>${_('created')} ${h.age_component(ver.updated_on)}</td>
                    </tr>
                    % endfor
                </table>
+
+               % if c.at_version:
+<pre>
+  Changed commits:
+    * added: ${len(c.changes.added)}
+    * removed: ${len(c.changes.removed)}
+
+  % if not (c.file_changes.added+c.file_changes.modified+c.file_changes.removed):
+  No file changes found
+  % else:
+  Changed files:
+    %for file_name in c.file_changes.added:
+    * A <a href="#${'a_' + h.FID('', file_name)}">${file_name}</a>
+    %endfor
+    %for file_name in c.file_changes.modified:
+    * M <a href="#${'a_' + h.FID('', file_name)}">${file_name}</a>
+    %endfor
+    %for file_name in c.file_changes.removed:
+    * R ${file_name}
+    %endfor
+  % endif
+</pre>
+               % endif
+           % else:
+               ${_('Pull request versions not available')}.
+           % endif
            </div>
            </div>
 
@@ -329,9 +361,9 @@
               % endif
                 <div class="compare_view_commits_title">
                     % if c.allowed_to_update and not c.pull_request.is_closed():
-                      <button id="update_commits" class="btn pull-right">${_('Update commits')}</button>
+                      <a id="update_commits" class="btn btn-primary pull-right">${_('Update commits')}</a>
                     % else:
-                      <button class="btn disabled pull-right" disabled="disabled">${_('Update commits')}</button>
+                      <a class="tooltip btn disabled pull-right" disabled="disabled" title="${_('Update is disabled for current view')}">${_('Update commits')}</a>
                     % endif
                     % if len(c.commit_ranges):
                       <h2>${ungettext('Compare View: %s commit','Compare View: %s commits', len(c.commit_ranges)) % len(c.commit_ranges)}</h2>

rhodecode/templates/rst_templates/pull_request_update.mako

@@ -1,5 +1,5 @@
 ## -*- coding: utf-8 -*-
-Auto status change to |under_review|
+Pull request updated. Auto status change to |under_review|
 
 .. role:: added
 .. role:: removed

rhodecode/tests/lib/test_markup_renderer.py

@@ -95,7 +95,7 @@ def test_rst_xss_raw_directive():
 
 def test_render_rst_template_without_files():
     expected = u'''\
-Auto status change to |under_review|
+Pull request updated. Auto status change to |under_review|
 
 .. role:: added
 .. role:: removed
@@ -125,7 +125,7 @@ Auto status change to |under_review|
 
 def test_render_rst_template_with_files():
     expected = u'''\
-Auto status change to |under_review|
+Pull request updated. Auto status change to |under_review|
 
 .. role:: added
 .. role:: removed

rhodecode/tests/models/test_pullrequest.py

@@ -722,7 +722,7 @@ def test_update_adds_a_comment_to_the_pull_request_about_the_change(pr_util):
     # Expect to find a new comment about the change
     expected_message = textwrap.dedent(
         """\
-        Auto status change to |under_review|
+        Pull request updated. Auto status change to |under_review|
 
         .. role:: added
         .. role:: removed