launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

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.
-- 
https://code.launchpad.net/~rpadovani/launchpad/link-revision-merged-mp/+merge/264654
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.