launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

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

Diff comments:

> === 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-14 23:01:25 +0000
> @@ -207,6 +207,19 @@
>                  formatter.displaydate())
>          return result
>  
> +    def link_to_branch_target_commit(self):

This should have @property above it, since it's essentially just a computed attribute.

> +        """The link to the code browser at the merged commit."""
> +        url = ''
> +        revision = self.context.merged_revision
> +        if IBranch.providedBy(self.context.merge_source):
> +            url = self.context.merge_source.getCodebrowseUrl(

This should be in the target branch, so merge_target in both places, not merge_source.  In the Bazaar case, the source branch probably doesn't have the merged revision.

> +                "revision/%s" % revision)
> +        else:
> +            url = "%s/commit/?id=%s" % (
> +                self.context.repository.target_git_repository, revision)
> +
> +        return url

Having thought about it a little more, I think this whole thing would be clearer if you pushed some of this code down a level to the model object (the context).  The model for both Branch and GitRepository (lib/lp/code/model/) could reasonably have a getCodebrowseUrlForRevision method.  In the Branch case, that would be:

  def getCodebrowseUrlForRevision(self, revision):
      return self.getCodebrowseUrl("revision", quote_plus(revision))

... while in the GitRepository case, that would be:

  def getCodebrowseUrlForRevision(self, revision):
      return "%s/commit/?id=%s" % (self.getCodebrowseUrl(), quote_plus(revision))

("from urllib import quote_plus" in both cases.)

Remember to add corresponding declarations under lib/lp/code/interfaces/.  If you also add a trivial forwarding method to GitRef (see getCodebrowseUrl, except in this case it can just forward to the repository directly rather than doing the ?h= thing), then the view code can be really simple:

  @property
  def link_to_branch_target_commit(self):
      return self.context.merge_target.getCodebrowseUrlForRevision(
          self.context.merged_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-14 23:01:25 +0000
> @@ -171,6 +186,9 @@
>      def getBranchRevisionValues(self, branch):
>          return {'merged_revno': branch.revision_count}
>  
> +    def getCodebrowseUrl(self, bmp, revision):
> +        return "%s/revision/%s" % (bmp.merge_target, revision)

This, and the corresponding method for Git, are incorrect, as they're substituting in a stringification of the target branch rather than any kind of URL.  But if you take the approach recommended above, then you can drop these methods entirely: just make the test do "revision_link = bmp.merge_target.getCodebrowseUrlForRevision(self.arbitrary_revisions[2])".

Of course, as well as testing that the HTML is put together correctly, you also need to test the underlying getCodebrowseUrlForRevision methods and make sure that they return sensible URLs.  Those tests would go in lib/lp/code/model/tests/test_{branch,gitrepository}.py.

> +
>  
>  class TestBranchMergeProposalMergedViewGit(
>      TestBranchMergeProposalMergedViewMixin, BrowserTestCase):


-- 
https://code.launchpad.net/~rpadovani/launchpad/link-revision-merged-mp/+merge/264654
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.