Merge lp:~rpadovani/launchpad/link-revision-merged-mp into lp:launchpad

[Riccardo Padovani]

Add link to the merged revision in the summary of merge proposal pages.

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
Create a new method in IGitRepository and IBranchRepository to create url to a commit/revision in the branch.
Use it to create the property for the template

Tests
1 test to check the html and 2 tests to check the new method

Demo and Q/A
Just run it locally and go on a summary of a branch merged in another branch

[Kit Randel (blr)]

Hi Riccardo, thanks for the patch!

This doesn't seem quite right however - currently this creates a link to code.launchpad.dev/project/branch/revno whereas I think we need (in the bazaar case), bazaar.launchpad.net/[project, ~user/project]/branch/revision/revno

review: Needs Fixing

[Kit Randel (blr)]

You might find lib/lp/code/templates/branch-macros.pt useful, as there is a relevant macro there used for "Recent revisions" on the branch index page.

[Colin Watson (cjwatson)]

You should definitely write a test, and the process of doing so would probably have exposed the problem Kit points out. By way of "bzr grep 'at revision'", it looks as though lib/lp/code/browser/tests/test_branchmergeproposal.py:TestBranchMergeProposalMergedViewMixin would be the right place to put the test, and you'd use 'bin/test -vvct TestBranchMergeProposalMergedView' to run it; before submitting you should also run 'bin/test -vvct lp.code.browser.tests.test_branchmergeproposal' to make sure you haven't caused a regression in the other tests there.

Getting the link right for both Bazaar and Git may be a bit fiddly to do in the page template (.pt) alone. Page templates have an associated view class, which you can look up in the relevant browser/configure.zcml file and in this case is BranchMergeCandidateView, and you can use attributes of the view as view/name_of_attribute. If it seems easier, you can add a property to the view as a helper for this; I'd probably make it be a method that returns the entire <a> element for the link, using structured(). In such a method, you can use "if IBranch.providedBy(self.context.merge_source):" to switch behaviour between Bazaar and Git.

review: Needs Fixing

[Riccardo Padovani (rpadovani)]

Thanks for your feedbacks :-)

I followed Colin's suggestions and I created a new attribute for the template, now I think it should work.

Then (actually, before) I started to write the test, but I'm stuck trying to creating the link. Any hint?

[Colin Watson (cjwatson)]

Certainly making progress, thanks. I think this advice should disentangle you a bit.

review: Needs Fixing

[Riccardo Padovani (rpadovani)]

Thanks for the awesome feedback Colin :-) Sorry for not replying before, I was busy at uni.

So, I started to work on your suggestions, and all seems to work now, but the test fails (only for bzr), with this error:

Error in test lp.code.browser.tests.test_branchmergeproposal.TestBranchMergeProposalMergedViewBzr.test_link_to_merged_revno
Traceback (most recent call last):
_StringException: Traceback (most recent call last):
  File "/home/rpadovani/launchpad/lp-branches/link-revision-merged-mp/lib/lp/code/browser/tests/test_branchmergeproposal.py", line 167, in test_link_to_merged_revno
    revision_link = bmp.merge_target.getCodebrowseUrlForRevision(
AttributeError: 'thread._local' object has no attribute 'interaction'

I tried to looking on the internet for this error, but I found nothingu useful. Also because in the file there is no reference to a thread, so I think is something more complicated.

Also another question:
you said to use quote_plus(revision) but if revision is an int then the function fails because int isn't interable. Explicit casting it to a string is the right way to go or there is another function to use?

I'm starting to work on other two tests

[Riccardo Padovani (rpadovani)]

Okay, I also created tests for getCodebrowseUrlForRevision methods. Hope they make sense!

[Colin Watson (cjwatson)]

https://dev.launchpad.net/ErrorExplanations#AttributeError:_.27thread._local.27_object_has_no_attribute_.27interaction.27

getViewBrowser calls logout(), so it's probably simplest to just move the assignments to revision_link and revision_number up to before you call getViewBrowser.

[Colin Watson (cjwatson)]

Oh, and as far as quote_plus goes, I'd forgotten that that would be an int in the Bazaar case. You can just use revision rather than quote_plus(revision) in that case, since an int will never require URL-quoting.

[Riccardo Padovani (rpadovani)]

Thanks for the link, I fixed the test.

Anyway, I used self.getCodebrowseUrl("revision", str(revision)) and not self.getCodebrowseUrl("revision", revision) because getCodebrowseUrl() requires *extras to be strings.

[Colin Watson (cjwatson)]

Thanks for the fixes!

review: Approve

=== 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:30:54 +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:30:54 +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)
+        revision_link = bmp.merge_target.getCodebrowseUrlForRevision(
+            self.arbitrary_revisions[2])
+        revision_number = Tag('Revision number', 'a', {'href': revision_link})
+        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')
+        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:30:54 +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:30:54 +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:30:54 +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:30:54 +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", 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:30:54 +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:30:54 +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:30:54 +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:30:54 +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:30:54 +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>