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

Proposed by Riccardo Padovani
Status: Merged
Merged at revision: 17681
Proposed branch: lp:~rpadovani/launchpad/link-revision-merged-mp
Merge into: lp:launchpad
Diff against target: 208 lines (+66/-1)
11 files modified
lib/lp/code/browser/branchmergeproposal.py (+6/-0)
lib/lp/code/browser/tests/test_branchmergeproposal.py (+14/-0)
lib/lp/code/interfaces/branch.py (+3/-0)
lib/lp/code/interfaces/gitref.py (+3/-0)
lib/lp/code/interfaces/gitrepository.py (+3/-0)
lib/lp/code/model/branch.py (+5/-0)
lib/lp/code/model/gitref.py (+4/-0)
lib/lp/code/model/gitrepository.py (+5/-0)
lib/lp/code/model/tests/test_branch.py (+10/-0)
lib/lp/code/model/tests/test_gitrepository.py (+9/-0)
lib/lp/code/templates/branchmergeproposal-link-summary.pt (+4/-1)
To merge this branch: bzr merge lp:~rpadovani/launchpad/link-revision-merged-mp
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Kit Randel (community) Needs Fixing
Review via email: mp+264654@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Kit Randel (blr) wrote :

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
Revision history for this message
Kit Randel (blr) wrote :

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.

Revision history for this message
Colin Watson (cjwatson) wrote :

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
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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?

Revision history for this message
Colin Watson (cjwatson) wrote :

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

review: Needs Fixing
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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

Revision history for this message
Colin Watson (cjwatson) wrote :

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.

Revision history for this message
Colin Watson (cjwatson) wrote :

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.

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

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.

Revision history for this message
Colin Watson (cjwatson) wrote :

Thanks for the fixes!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
207 formatter.displaydate())207 formatter.displaydate())
208 return result208 return result
209209
210 @property
211 def link_to_branch_target_commit(self):
212 """The link to the code browser at the merged commit."""
213 revision = self.context.merged_revision
214 return self.context.merge_target.getCodebrowseUrlForRevision(revision)
215
210216
211class BranchMergeProposalMenuMixin:217class BranchMergeProposalMenuMixin:
212 """Mixin class for merge proposal menus."""218 """Mixin class for merge proposal menus."""
213219
=== 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 @@
11 timedelta,11 timedelta,
12 )12 )
13from difflib import unified_diff13from difflib import unified_diff
14from urllib import quote_plus
1415
15from lazr.restful.interfaces import IJSONRequestCache16from lazr.restful.interfaces import IJSONRequestCache
16import pytz17import pytz
@@ -154,6 +155,19 @@
154 normalize_whitespace(extract_text(find_tag_by_id(155 normalize_whitespace(extract_text(find_tag_by_id(
155 browser.contents, 'landing-targets'))))156 browser.contents, 'landing-targets'))))
156157
158 def test_link_to_merged_revno(self):
159 bmp = self.makeBranchMergeProposal()
160 login_person(bmp.registrant)
161 bmp.markAsMerged(merge_reporter=bmp.registrant)
162 revision_link = bmp.merge_target.getCodebrowseUrlForRevision(
163 self.arbitrary_revisions[2])
164 revision_number = Tag('Revision number', 'a', {'href': revision_link})
165 browser = self.getViewBrowser(bmp, '+merged', user=bmp.registrant)
166 browser.getControl(self.merged_revision_text).value = str(
167 self.arbitrary_revisions[2])
168 browser.getControl('Mark as Merged').click()
169 browser = self.getViewBrowser(bmp.merge_source, '+index')
170 self.assertThat(browser.contents, HTMLContains(revision_number))
157171
158class TestBranchMergeProposalMergedViewBzr(172class TestBranchMergeProposalMergedViewBzr(
159 TestBranchMergeProposalMergedViewMixin, BrowserTestCase):173 TestBranchMergeProposalMergedViewMixin, BrowserTestCase):
160174
=== 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 @@
692 browse_source_url = Attribute(692 browse_source_url = Attribute(
693 "The URL of the source browser for this branch.")693 "The URL of the source browser for this branch.")
694694
695 def getCodebrowseUrlForRevision(commit):
696 """The URL to the commit of the merge to the target branch"""
697
695 # Really ICodeImport, but that would cause a circular import698 # Really ICodeImport, but that would cause a circular import
696 code_import = exported(699 code_import = exported(
697 Reference(700 Reference(
698701
=== 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 @@
134 def getCodebrowseUrl():134 def getCodebrowseUrl():
135 """Construct a browsing URL for this Git reference."""135 """Construct a browsing URL for this Git reference."""
136136
137 def getCodebrowseUrlForRevision(commit):
138 """Construct a browsing URL for this Git at the given commit"""
139
137 information_type = Attribute(140 information_type = Attribute(
138 "The type of information contained in the repository containing this "141 "The type of information contained in the repository containing this "
139 "reference.")142 "reference.")
140143
=== 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 @@
330 def getCodebrowseUrl():330 def getCodebrowseUrl():
331 """Construct a browsing URL for this Git repository."""331 """Construct a browsing URL for this Git repository."""
332332
333 def getCodebrowseUrlForRevision(commit):
334 """The URL to the commit of the merge to the target branch"""
335
333 def visibleByUser(user):336 def visibleByUser(user):
334 """Can the specified user see this repository?"""337 """Can the specified user see this repository?"""
335338
336339
=== 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 @@
1010
11from datetime import datetime11from datetime import datetime
12import operator12import operator
13from urllib import quote_plus
1314
14from bzrlib import urlutils15from bzrlib import urlutils
15from bzrlib.revision import NULL_REVISION16from bzrlib.revision import NULL_REVISION
@@ -660,6 +661,10 @@
660 root = config.codehosting.codebrowse_root661 root = config.codehosting.codebrowse_root
661 return urlutils.join(root, self.unique_name, *extras)662 return urlutils.join(root, self.unique_name, *extras)
662663
664 def getCodebrowseUrlForRevision(self, revision):
665 """See `IBranch`."""
666 return self.getCodebrowseUrl("revision", str(revision))
667
663 @property668 @property
664 def browse_source_url(self):669 def browse_source_url(self):
665 return self.getCodebrowseUrl('files')670 return self.getCodebrowseUrl('files')
666671
=== 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 @@
101 return "%s?h=%s" % (101 return "%s?h=%s" % (
102 self.repository.getCodebrowseUrl(), quote_plus(self.name))102 self.repository.getCodebrowseUrl(), quote_plus(self.name))
103103
104 def getCodebrowseUrlForRevision(self, commit):
105 """See `IGitRef`."""
106 return self.repository.getCodebrowseUrlForRevision(commit)
107
104 @property108 @property
105 def information_type(self):109 def information_type(self):
106 """See `IGitRef`."""110 """See `IGitRef`."""
107111
=== 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 @@
16 groupby,16 groupby,
17 )17 )
18from operator import attrgetter18from operator import attrgetter
19from urllib import quote_plus
1920
20from bzrlib import urlutils21from bzrlib import urlutils
21import pytz22import pytz
@@ -351,6 +352,10 @@
351 return urlutils.join(352 return urlutils.join(
352 config.codehosting.git_browse_root, self.shortened_path)353 config.codehosting.git_browse_root, self.shortened_path)
353354
355 def getCodebrowseUrlForRevision(self, commit):
356 return "%s/commit/?id=%s" % (
357 self.getCodebrowseUrl(), quote_plus(str(commit)))
358
354 @property359 @property
355 def git_https_url(self):360 def git_https_url(self):
356 """See `IGitRepository`."""361 """See `IGitRepository`."""
357362
=== 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 @@
2169 since = branch.getRevisionsSince(mid_revision.revision.revision_date)2169 since = branch.getRevisionsSince(mid_revision.revision.revision_date)
2170 self.assertEqual(revisions[:mid_sequence], list(since))2170 self.assertEqual(revisions[:mid_sequence], list(since))
21712171
2172 def test_getCodebrowseUrlForRevision(self):
2173 # IBranch.getCodebrowseUrlForRevision gives the URL to the browser
2174 # for a specific revision of the code
2175 branch = self.factory.makeBranch()
2176 revision = 42
2177 self.factory.makeRevisionsForBranch(branch, count=revision)
2178 urlByRevision = branch.getCodebrowseUrlForRevision(42)
2179 url = branch.getCodebrowseUrl()
2180 self.assertEqual(urlByRevision, "%s/revision/%s" % (url, revision))
2181
2172 def test_top_ancestor_has_no_parents(self):2182 def test_top_ancestor_has_no_parents(self):
2173 # The top-most revision of a branch (i.e. the first one) has no2183 # The top-most revision of a branch (i.e. the first one) has no
2174 # parents.2184 # parents.
21752185
=== 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 @@
782 config.codehosting.git_ssh_root, repository.shortened_path)782 config.codehosting.git_ssh_root, repository.shortened_path)
783 self.assertEqual(expected_url, repository.git_ssh_url)783 self.assertEqual(expected_url, repository.git_ssh_url)
784784
785 def test_getCodebrowseUrlForRevision(self):
786 # IGitRepository.getCodebrowseUrlForRevision gives the URL to the
787 # browser for a specific commit of the code
788 repository = self.factory.makeGitRepository()
789 commit = u'0' * 40
790 urlByCommit = repository.getCodebrowseUrlForRevision(commit)
791 url = repository.getCodebrowseUrl()
792 self.assertEqual(urlByCommit, "%s/commit/?id=%s" % (url, commit))
793
785794
786class TestGitRepositoryNamespace(TestCaseWithFactory):795class TestGitRepositoryNamespace(TestCaseWithFactory):
787 """Test `IGitRepository.namespace`."""796 """Test `IGitRepository.namespace`."""
788797
=== 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 @@
18 </tal:source-branch>18 </tal:source-branch>
19 <tal:for-merging condition="context/queue_status/enumvalue:MERGED">19 <tal:for-merging condition="context/queue_status/enumvalue:MERGED">
20 <tal:have-revision condition="context/merged_revision">20 <tal:have-revision condition="context/merged_revision">
21 at revision <tal:revision replace="context/merged_revision"/>21 at
22 <a tal:attributes="href view/link_to_branch_target_commit">
23 revision <tal:revision replace="context/merged_revision"/>
24 </a>
22 </tal:have-revision>25 </tal:have-revision>
23 </tal:for-merging>26 </tal:for-merging>
24</tal:root>27</tal:root>