launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-commits-link-mps into lp:launchpad with lp:~cjwatson/launchpad/git-ref-commits as a prerequisite.

Commit message:
Show associated merge proposals in Git commit listings.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-commits-link-mps/+merge/294653

Show associated merge proposals in Git commit listings.

For Bazaar, we do some of this in BranchCollection.getExtendedRevisionDetails, but I thought that was far too peculiar a place for it, and it makes no sense to have the extra level of nesting given that we aren't using BranchRevision here.  It's easier to just add an extra key to the commit information dictionary.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-commits-link-mps into lp:launchpad.

=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py	2016-05-13 14:30:57 +0000
+++ lib/lp/code/browser/gitref.py	2016-05-13 14:30:57 +0000
@@ -169,7 +169,8 @@
 
     @cachedproperty
     def commit_infos(self):
-        return self.context.getLatestCommits()
+        return self.context.getLatestCommits(
+            extended_details=True, user=self.user)
 
     @property
     def recipes_link(self):

=== modified file 'lib/lp/code/browser/tests/test_gitref.py'
--- lib/lp/code/browser/tests/test_gitref.py	2016-05-13 14:30:57 +0000
+++ lib/lp/code/browser/tests/test_gitref.py	2016-05-13 14:30:57 +0000
@@ -9,12 +9,14 @@
 import hashlib
 import re
 
+from BeautifulSoup import BeautifulSoup
 import pytz
 import soupmatchers
 from zope.component import getUtility
 
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     admin_logged_in,
     BrowserTestCase,
@@ -133,6 +135,26 @@
         self.assertEqual(
             expected_urls, [detail.a["href"] for detail in details])
 
+    def test_recent_commits_with_merge(self):
+        [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/branch"])
+        log = self.makeCommitLog()
+        self.hosting_client.getLog.result = list(reversed(log))
+        mp = self.factory.makeBranchMergeProposalForGit(target_ref=ref)
+        mp.markAsMerged(merged_revision_id=log[0]["sha1"])
+        view = create_initialized_view(ref, "+index")
+        soup = BeautifulSoup(view())
+        details = soup.findAll(
+            attrs={"class": re.compile(r"commit-details|commit-comment")})
+        expected_texts = list(reversed([
+            "%.7s...\nby\n%s\non 2015-01-%02d" % (
+                log[i]["sha1"], log[i]["author"]["name"], i + 1)
+            for i in range(5)]))
+        expected_texts.append(
+            "Merged branch\n%s" % mp.merge_source.display_name)
+        self.assertEqual(
+            expected_texts, [extract_text(detail) for detail in details])
+        self.assertEqual(canonical_url(mp), details[5].a["href"])
+
     def test_all_commits_link(self):
         [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/branch"])
         self.hosting_client.getLog.result = self.makeCommitLog()

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2016-05-13 14:30:57 +0000
+++ lib/lp/code/model/gitref.py	2016-05-13 14:30:57 +0000
@@ -297,8 +297,22 @@
             revisions.append(parsed_commit)
         return revisions
 
-    def getLatestCommits(self, quantity=10):
-        return self.getCommits(self.commit_sha1, limit=quantity)
+    def getLatestCommits(self, quantity=10, extended_details=False, user=None):
+        commits = self.getCommits(self.commit_sha1, limit=quantity)
+        if extended_details:
+            # XXX cjwatson 2016-05-09: Add support for linked bugtasks once
+            # those are supported for Git.
+            collection = getUtility(IAllGitRepositories).visibleByUser(user)
+            merge_proposals = collection.getMergeProposals(
+                target_repository=self.repository, target_path=self.path,
+                merged_revision_ids=[commit["sha1"] for commit in commits],
+                statuses=[BranchMergeProposalStatus.MERGED])
+            merge_proposal_commits = {
+                mp.merged_revision_id: mp for mp in merge_proposals}
+            for commit in commits:
+                commit["merge_proposal"] = merge_proposal_commits.get(
+                    commit["sha1"])
+        return commits
 
     @property
     def has_commits(self):

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2016-05-13 14:30:57 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2016-05-13 14:30:57 +0000
@@ -17,6 +17,7 @@
     ContainsDict,
     EndsWith,
     Equals,
+    Is,
     MatchesListwise,
     MatchesStructure,
     )
@@ -212,6 +213,22 @@
             ContainsDict({"sha1": Equals(self.sha1_root)}),
             ]))
 
+    def test_extended_details_with_merge(self):
+        mp = self.factory.makeBranchMergeProposalForGit(target_ref=self.ref)
+        mp.markAsMerged(merged_revision_id=self.sha1_tip)
+        revisions = self.ref.getLatestCommits(
+            self.sha1_tip, extended_details=True, user=self.ref.owner)
+        self.assertThat(revisions, MatchesListwise([
+            ContainsDict({
+                "sha1": Equals(self.sha1_tip),
+                "merge_proposal": Equals(mp),
+                }),
+            ContainsDict({
+                "sha1": Equals(self.sha1_root),
+                "merge_proposal": Is(None),
+                }),
+            ]))
+
 
 class TestGitRefWebservice(TestCaseWithFactory):
     """Tests for the webservice."""

=== modified file 'lib/lp/code/templates/git-macros.pt'
--- lib/lp/code/templates/git-macros.pt	2016-05-13 14:30:57 +0000
+++ lib/lp/code/templates/git-macros.pt	2016-05-13 14:30:57 +0000
@@ -104,7 +104,7 @@
     This macro requires the following defined variables:
       ref - the ref that has the revisions
       commit_infos - a list of dicts of commit information (sha1, author,
-                     author_date, commit_message)
+                     author_date, commit_message, merge_proposal)
   </tal:comment>
   <style type="text/css">
     .subordinate {
@@ -124,7 +124,8 @@
   <tal:comment condition="nothing">
     This macro requires the following defined variable:
       commit_info - a dict of the commit information (sha1, author,
-                    author_date, commit_message) to be displayed.
+                    author_date, commit_message, merge_proposal) to be
+                    displayed
 
     It is expected that this macro is called from within a definition list
     (<dl></dl>).
@@ -152,6 +153,14 @@
       replace="structure commit_message/fmt:obfuscate-email/fmt:text-to-html" />
   </dd>
 
+  <div tal:define="merge_proposal python:commit_info['merge_proposal']"
+       tal:condition="merge_proposal">
+    <dd class="subordinate commit-comment">
+      <a tal:attributes="href merge_proposal/fmt:url">Merged</a> branch
+      <a tal:replace="structure merge_proposal/merge_source/fmt:link">source branch</a>
+    </dd>
+  </div>
+
 </metal:commit-text>
 
 <metal:no-commit-message define-macro="no-commit-message">