launchpad-reviewers team mailing list archive

[Riccardo Padovani <riccardo@xxxxxxxxxxxxx>]

Riccardo Padovani has proposed merging lp:~rpadovani/launchpad/link-revision-merged-mp into lp:launchpad.

Commit message:
Link to revision for merged MPs 

Add link to the revision in the summary of merge proposal page
Fix #892259 

Requested reviews:
  Colin Watson (cjwatson)
  Bayard 'kit' Randel (blr)
Related bugs:
  Bug #892259 in Launchpad itself: "Link to revision for merged MPs"
  https://bugs.launchpad.net/launchpad/+bug/892259

For more details, see:
https://code.launchpad.net/~rpadovani/launchpad/link-revision-merged-mp/+merge/264654

Summary
Fix bug #892259

"Merged into lp:pkgme at revision 86". "Merged" is a link; "lp:pkgme" is a link, but "revision 86" is not.

Proposed fix
Add link to the revision

Pre-implementation notes
My first patch to lp itself :-)

Implementation details
I just changed the template of the merge proposal summary page.

Tests
I didn't write any. Should I?

Demo and Q/A
Just run it locally and go on a summary of a branch merged in another branch
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2015-07-09 20:06:17 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2015-07-21 17:08:20 +0000
@@ -207,6 +207,12 @@
                 formatter.displaydate())
         return result
 
+    @property
+    def link_to_branch_target_commit(self):
+        """The link to the code browser at the merged commit."""
+        revision = self.context.merged_revision
+        return self.context.merge_target.getCodebrowseUrlForRevision(revision)
+
 
 class BranchMergeProposalMenuMixin:
     """Mixin class for merge proposal menus."""

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-06-12 02:29:10 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-07-21 17:08:20 +0000
@@ -11,6 +11,7 @@
     timedelta,
     )
 from difflib import unified_diff
+from urllib import quote_plus
 
 from lazr.restful.interfaces import IJSONRequestCache
 import pytz
@@ -154,6 +155,19 @@
             normalize_whitespace(extract_text(find_tag_by_id(
                 browser.contents, 'landing-targets'))))
 
+    def test_link_to_merged_revno(self):
+        bmp = self.makeBranchMergeProposal()
+        login_person(bmp.registrant)
+        bmp.markAsMerged(merge_reporter=bmp.registrant)
+        browser = self.getViewBrowser(bmp, '+merged', user=bmp.registrant)
+        browser.getControl(self.merged_revision_text).value = str(
+            self.arbitrary_revisions[2])
+        browser.getControl('Mark as Merged').click()
+        browser = self.getViewBrowser(bmp.merge_source, '+index')
+        revision_link = bmp.merge_target.getCodebrowseUrlForRevision(
+            self.arbitrary_revisions[2])
+        revision_number = Tag('Revision number', 'a', {'href': revision_link})
+        self.assertThat(browser.contents, HTMLContains(revision_number))
 
 class TestBranchMergeProposalMergedViewBzr(
     TestBranchMergeProposalMergedViewMixin, BrowserTestCase):

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2015-05-26 12:18:12 +0000
+++ lib/lp/code/interfaces/branch.py	2015-07-21 17:08:20 +0000
@@ -692,6 +692,9 @@
     browse_source_url = Attribute(
         "The URL of the source browser for this branch.")
 
+    def getCodebrowseUrlForRevision(commit):
+        """The URL to the commit of the merge to the target branch"""
+
     # Really ICodeImport, but that would cause a circular import
     code_import = exported(
         Reference(

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2015-06-05 11:05:03 +0000
+++ lib/lp/code/interfaces/gitref.py	2015-07-21 17:08:20 +0000
@@ -134,6 +134,9 @@
     def getCodebrowseUrl():
         """Construct a browsing URL for this Git reference."""
 
+    def getCodebrowseUrlForRevision(commit):
+        """Construct a browsing URL for this Git at the given commit"""
+
     information_type = Attribute(
         "The type of information contained in the repository containing this "
         "reference.")

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2015-07-01 07:58:25 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2015-07-21 17:08:20 +0000
@@ -330,6 +330,9 @@
     def getCodebrowseUrl():
         """Construct a browsing URL for this Git repository."""
 
+    def getCodebrowseUrlForRevision(commit):
+        """The URL to the commit of the merge to the target branch"""
+
     def visibleByUser(user):
         """Can the specified user see this repository?"""
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/branch.py	2015-07-21 17:08:20 +0000
@@ -10,6 +10,7 @@
 
 from datetime import datetime
 import operator
+from urllib import quote_plus
 
 from bzrlib import urlutils
 from bzrlib.revision import NULL_REVISION
@@ -660,6 +661,10 @@
             root = config.codehosting.codebrowse_root
         return urlutils.join(root, self.unique_name, *extras)
 
+    def getCodebrowseUrlForRevision(self, revision):
+        """See `IBranch`."""
+        return self.getCodebrowseUrl("revision", quote_plus(str(revision)))
+
     @property
     def browse_source_url(self):
         return self.getCodebrowseUrl('files')

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/gitref.py	2015-07-21 17:08:20 +0000
@@ -101,6 +101,10 @@
         return "%s?h=%s" % (
             self.repository.getCodebrowseUrl(), quote_plus(self.name))
 
+    def getCodebrowseUrlForRevision(self, commit):
+        """See `IGitRef`."""
+        return self.repository.getCodebrowseUrlForRevision(commit)
+
     @property
     def information_type(self):
         """See `IGitRef`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-07-15 06:25:36 +0000
+++ lib/lp/code/model/gitrepository.py	2015-07-21 17:08:20 +0000
@@ -16,6 +16,7 @@
     groupby,
     )
 from operator import attrgetter
+from urllib import quote_plus
 
 from bzrlib import urlutils
 import pytz
@@ -351,6 +352,10 @@
         return urlutils.join(
             config.codehosting.git_browse_root, self.shortened_path)
 
+    def getCodebrowseUrlForRevision(self, commit):
+        return "%s/commit/?id=%s" % (
+            self.getCodebrowseUrl(), quote_plus(str(commit)))
+
     @property
     def git_https_url(self):
         """See `IGitRepository`."""

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2015-04-22 12:03:05 +0000
+++ lib/lp/code/model/tests/test_branch.py	2015-07-21 17:08:20 +0000
@@ -2169,6 +2169,16 @@
         since = branch.getRevisionsSince(mid_revision.revision.revision_date)
         self.assertEqual(revisions[:mid_sequence], list(since))
 
+    def test_getCodebrowseUrlForRevision(self):
+        # IBranch.getCodebrowseUrlForRevision gives the URL to the browser
+        # for a specific revision of the code
+        branch = self.factory.makeBranch()
+        revision = 42
+        self.factory.makeRevisionsForBranch(branch, count=revision)
+        urlByRevision = branch.getCodebrowseUrlForRevision(42)
+        url = branch.getCodebrowseUrl()
+        self.assertEqual(urlByRevision, "%s/revision/%s" % (url, revision))
+
     def test_top_ancestor_has_no_parents(self):
         # The top-most revision of a branch (i.e. the first one) has no
         # parents.

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-07-10 22:24:49 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-07-21 17:08:20 +0000
@@ -782,6 +782,15 @@
                 config.codehosting.git_ssh_root, repository.shortened_path)
             self.assertEqual(expected_url, repository.git_ssh_url)
 
+    def test_getCodebrowseUrlForRevision(self):
+        # IGitRepository.getCodebrowseUrlForRevision gives the URL to the
+        # browser for a specific commit of the code
+        repository = self.factory.makeGitRepository()
+        commit = u'0' * 40
+        urlByCommit = repository.getCodebrowseUrlForRevision(commit)
+        url = repository.getCodebrowseUrl()
+        self.assertEqual(urlByCommit, "%s/commit/?id=%s" % (url, commit))
+
 
 class TestGitRepositoryNamespace(TestCaseWithFactory):
     """Test `IGitRepository.namespace`."""

=== modified file 'lib/lp/code/templates/branchmergeproposal-link-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-link-summary.pt	2015-06-10 10:25:28 +0000
+++ lib/lp/code/templates/branchmergeproposal-link-summary.pt	2015-07-21 17:08:20 +0000
@@ -18,7 +18,10 @@
   </tal:source-branch>
   <tal:for-merging condition="context/queue_status/enumvalue:MERGED">
     <tal:have-revision condition="context/merged_revision">
-      at revision <tal:revision replace="context/merged_revision"/>
+      at
+      <a tal:attributes="href view/link_to_branch_target_commit">
+        revision <tal:revision replace="context/merged_revision"/>
+      </a>
     </tal:have-revision>
   </tal:for-merging>
 </tal:root>