Code review comment for lp:~rpadovani/launchpad/link-revision-merged-mp

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

« Back to merge proposal