launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

I won't mention it at each site, but please sort imports.  utilities/format-new-and-modified-imports can help with this (though it's worth double-checking what it does).  Note also that imports are normally sorted case-insensitively in LP, even though I personally find that a bit weird.

I think some more tests should be updated:

lib/lp/app/doc/displaying-paragraphs-of-text.txt
lib/lp/app/javascript/tests/test_lp_links.*

... and the comment in lib/lp/app/javascript/lp-links.js should probably be updated too.

Diff comments:

> === modified file 'lib/lp/app/browser/launchpad.py'
> --- lib/lp/app/browser/launchpad.py	2016-05-10 10:53:32 +0000
> +++ lib/lp/app/browser/launchpad.py	2016-06-20 21:23:40 +0000
> @@ -767,6 +770,70 @@
>  
>          return self.redirectSubTree(target_url)
>  
> +    @stepto('+code')
> +    def redirect_branch_or_repo(self):
> +        """Redirect /+code/<foo> to the branch or repository named 'foo'.
> +
> +        'foo' can be the unique name of the branch/repo, or any of the aliases
> +        for the branch/repo.
> +
> +        If 'foo' is invalid, or exists but the user does not have permission to
> +        view 'foo', a NotFoundError is raised, resulting in a 404 error.
> +
> +        Unlike +branch, this works for both git and bzr repositories/branches.
> +        """
> +        target_url = ''

None would be a little more idiomatic here, and likewise below.

> +        path = '/'.join(self.request.stepstogo)
> +
> +        # Try a Git repository lookup first, since the schema is simpler and
> +        # so it's quicker.
> +        try:
> +            repository, trailing = getUtility(IGitLookup).getByPath(path)
> +            if repository is not None:
> +                target_url = canonical_url(repository)
> +                if trailing:
> +                    target_url = urlappend(target_url, trailing)
> +        except (InvalidNamespace, InvalidProductName, NameLookupFailed,
> +                Unauthorized):
> +            # Either the git repository wasn't found, or it was found but we
> +            # lack authority to access it. In either case, attempt a bzr lookup
> +            # so don't set target_url
> +            pass
> +
> +        # Git lookup failed. If '+git' is in the path, there's no point
> +        # attempting a bzr lookup as well.
> +        if '+git' not in path:

This test has false positives: consider something like ~cjwatson/launchpad/add-bzr+git-support-to-answers.  You could perhaps check for '/+git/' instead, although I don't know if the optimisation is worth it as BranchLookup should be able to fail quite quickly once it works out that the URL structure doesn't fit the Bazaar model.

> +            try:
> +                branch, trailing = getUtility(IBranchLookup).getByLPPath(path)
> +                bzr_url = canonical_url(branch)
> +                if trailing != '':
> +                    bzr_url = urlappend(bzr_url, trailing)
> +
> +                if target_url:
> +                    # Project has both a bzr branch and a git repo. There's no
> +                    # right thing we can do here, so pretend we didn't see
> +                    # anything at all.
> +                    if branch.product.vcs is None:

In general, branch.product may be None.  I think you're right that it's impossible for that to actually be the case when we find both a branch and a repository, but I'd rather see a defensive check here.  Perhaps "if target_url and branch.product is not None:" above.

> +                        target_url = ''
> +                    # if it's set to BZR, then set this branch as the target
> +                    if branch.product.vcs == VCSType.BZR:
> +                        target_url = bzr_url
> +                else:
> +                    target_url = bzr_url
> +            except (NoLinkedBranch, CannotHaveLinkedBranch, InvalidNamespace,
> +                    InvalidProductName, NotFoundError):
> +                # No bzr branch found either.
> +                pass
> +
> +        # Either neither bzr nor git returned matches, or they did but we're
> +        # not authorised to view them, or they both did and the project has not
> +        # set it's 'vcs' property to indicate which one to prefer. In all cases

s/it's/its/

> +        # raise a 404:
> +        if not target_url:
> +            raise NotFoundError
> +
> +        return self.redirectSubTree(target_url)
> +
>      @stepto('+builds')
>      def redirect_buildfarm(self):
>          """Redirect old /+builds requests to new URL, /builders."""
> 
> === modified file 'lib/lp/app/browser/tests/test_launchpad.py'
> --- lib/lp/app/browser/tests/test_launchpad.py	2016-04-11 06:38:48 +0000
> +++ lib/lp/app/browser/tests/test_launchpad.py	2016-06-20 21:23:40 +0000
> @@ -332,6 +335,253 @@
>          self.assertNotFound("_foo", use_default_referer=False)
>  
>  
> +class TestCodeTraversal(TestCaseWithFactory, TraversalMixin):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def traverse(self, path, **kwargs):
> +        return super(TestCodeTraversal, self).traverse(
> +            path, '+code', **kwargs)
> +
> +    def test_project_bzr_branch(self):
> +        branch = self.factory.makeAnyBranch()
> +        self.assertRedirects(branch.unique_name, canonical_url(branch))
> +
> +    def test_project_git_branch(self):
> +        git_repo = self.factory.makeGitRepository()
> +        self.assertRedirects(git_repo.unique_name, canonical_url(git_repo))
> +
> +    def test_no_such_bzr_unique_name(self):
> +        branch = self.factory.makeAnyBranch()
> +        bad_name = branch.unique_name + 'wibble'
> +        self.assertNotFound(bad_name)
> +
> +    def test_no_such_git_unique_name(self):
> +        repo = self.factory.makeGitRepository()
> +        bad_name = repo.unique_name + 'wibble'
> +        self.assertNotFound(bad_name)
> +
> +    def test_private_bzr_branch(self):
> +        branch = self.factory.makeProductBranch(
> +            information_type=InformationType.USERDATA)
> +        branch_unique_name = removeSecurityProxy(branch).unique_name
> +        login(ANONYMOUS)
> +        self.assertNotFound(branch_unique_name)
> +
> +    def test_private_git_branch(self):
> +        git_repo = self.factory.makeGitRepository(
> +            information_type=InformationType.USERDATA)
> +        repo_unique_name = removeSecurityProxy(git_repo).unique_name
> +        login(ANONYMOUS)
> +        self.assertNotFound(repo_unique_name)
> +
> +    def test_product_alias_bzr(self):
> +        branch = self.factory.makeProductBranch()
> +        naked_product = removeSecurityProxy(branch.product)
> +        ICanHasLinkedBranch(naked_product).setBranch(branch)
> +        self.assertRedirects(naked_product.name, canonical_url(branch))
> +
> +    def test_product_alias_git(self):
> +        project = self.factory.makeProduct()
> +        repo = self.factory.makeGitRepository(target=project)
> +        naked_project = removeSecurityProxy(project)
> +        with person_logged_in(repo.target.owner):
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                repo.target, repo)
> +        self.assertRedirects(naked_project.name, canonical_url(repo))
> +
> +    def test_private_bzr_branch_for_product(self):
> +        branch = self.factory.makeProductBranch()
> +        naked_product = removeSecurityProxy(branch.product)
> +        ICanHasLinkedBranch(naked_product).setBranch(branch)
> +        removeSecurityProxy(branch).information_type = (
> +            InformationType.USERDATA)
> +        login(ANONYMOUS)
> +        self.assertNotFound(naked_product.name)
> +
> +    def test_private_git_branch_for_product(self):
> +        project = self.factory.makeProduct()
> +        repo = self.factory.makeGitRepository(target=project)
> +        with person_logged_in(repo.target.owner):
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                repo.target, repo)
> +
> +        removeSecurityProxy(repo).information_type = (
> +            InformationType.USERDATA)
> +        login(ANONYMOUS)
> +
> +        naked_project = removeSecurityProxy(project)
> +        self.assertNotFound(naked_project.name)
> +
> +    def test_nonexistent_product(self):
> +        non_existent = 'non-existent'
> +        self.assertNotFound(non_existent)
> +
> +    def test_product_without_dev_focus(self):
> +        product = self.factory.makeProduct()
> +        self.assertNotFound(product.name)
> +
> +    def test_distro_package_alias_bzr(self):
> +        sourcepackage = self.factory.makeSourcePackage()
> +        branch = self.factory.makePackageBranch(sourcepackage=sourcepackage)
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        registrant = distro_package.distribution.owner
> +        target = ICanHasLinkedBranch(distro_package)
> +        with person_logged_in(registrant):
> +            target.setBranch(branch, registrant)
> +        self.assertRedirects("%s" % target.bzr_path, canonical_url(branch))
> +
> +    def test_distro_package_alias_git(self):
> +        sourcepackage = self.factory.makeSourcePackage()
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        repo = self.factory.makeGitRepository(target=distro_package)
> +
> +        with admin_logged_in():
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                distro_package, repo)
> +
> +        self.assertRedirects("%s" % repo.shortened_path, canonical_url(repo))
> +
> +    def test_private_branch_for_distro_package_bzr(self):
> +        sourcepackage = self.factory.makeSourcePackage()
> +        branch = self.factory.makePackageBranch(
> +            sourcepackage=sourcepackage,
> +            information_type=InformationType.USERDATA)
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        registrant = distro_package.distribution.owner
> +        with person_logged_in(registrant):
> +            ICanHasLinkedBranch(distro_package).setBranch(branch, registrant)
> +        login(ANONYMOUS)
> +        path = ICanHasLinkedBranch(distro_package).bzr_path
> +        self.assertNotFound(path)
> +
> +    def test_private_branch_for_distro_package_git(self):
> +        sourcepackage = self.factory.makeSourcePackage()
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        repo = self.factory.makeGitRepository(
> +            target=distro_package,
> +            information_type=InformationType.USERDATA)
> +        with admin_logged_in():
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                distro_package, repo)
> +        login(ANONYMOUS)
> +        path = removeSecurityProxy(repo).shortened_path
> +        self.assertNotFound(path)
> +
> +    def test_trailing_path_redirect_bzr(self):
> +        branch = self.factory.makeAnyBranch()
> +        path = urlappend(branch.unique_name, '+edit')
> +        self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
> +
> +    def test_trailing_path_redirect_git(self):
> +        repo = self.factory.makeGitRepository()
> +        path = urlappend(repo.unique_name, '+edit')
> +        self.assertRedirects(path, canonical_url(repo, view_name='+edit'))
> +
> +    def test_alias_trailing_path_redirect_bzr(self):
> +        branch = self.factory.makeProductBranch()
> +        with person_logged_in(branch.product.owner):
> +            branch.product.development_focus.branch = branch
> +        path = '%s/+edit' % branch.product.name
> +        self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
> +
> +    def test_alias_trailing_path_redirect_git(self):
> +        project = self.factory.makeProduct()
> +        repo = self.factory.makeGitRepository(target=project)
> +        with admin_logged_in():
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                project, repo)
> +        path = '%s/+edit' % project.name
> +        self.assertRedirects(path, canonical_url(repo, view_name='+edit'))
> +
> +    def test_product_series_redirect_bzr(self):
> +        branch = self.factory.makeBranch()
> +        series = self.factory.makeProductSeries(branch=branch)
> +        self.assertRedirects(
> +            ICanHasLinkedBranch(series).bzr_path, canonical_url(branch))
> +
> +    def test_no_branch_for_series(self):
> +        # If there's no branch for a product series, display a
> +        # message telling the user there is no linked branch.
> +        series = self.factory.makeProductSeries()
> +        path = ICanHasLinkedBranch(series).bzr_path
> +        self.assertNotFound(path)
> +
> +    def test_private_branch_for_series(self):
> +        # If the development focus of a product series is private, display a
> +        # message telling the user there is no linked branch.
> +        branch = self.factory.makeBranch(
> +            information_type=InformationType.USERDATA)
> +        series = self.factory.makeProductSeries(branch=branch)
> +        login(ANONYMOUS)
> +        path = ICanHasLinkedBranch(series).bzr_path
> +        self.assertNotFound(path)
> +
> +    def test_too_short_branch_name(self):
> +        owner = self.factory.makePerson()
> +        self.assertNotFound('~%s' % owner.name)
> +
> +    def test_invalid_product_name(self):
> +        self.assertNotFound('_foo')
> +
> +    def test_invalid_product_name_without_referer(self):
> +        self.assertNotFound("_foo", use_default_referer=False)
> +
> +    def test_ambigous_project_default_repo_bzr(self):

s/ambigous/ambiguous/ throughout this file.

> +        project = self.factory.makeProduct()
> +        bzr_branch = self.factory.makeBranch(target=project)
> +        self.factory.makeGitRepository(target=project)
> +        with person_logged_in(project.owner):
> +            ICanHasLinkedBranch(project).setBranch(bzr_branch, project.owner)
> +
> +        self.assertRedirects(project.name, canonical_url(bzr_branch))
> +
> +    def test_ambigous_project_without_vcs_set(self):
> +        project = self.factory.makeProduct()
> +        bzr_branch = self.factory.makeBranch(target=project)
> +        repo = self.factory.makeGitRepository(target=project)
> +        with person_logged_in(project.owner):
> +            ICanHasLinkedBranch(project).setBranch(bzr_branch, project.owner)
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                project, repo)
> +
> +        self.assertNotFound(project.name)
> +
> +    def test_ambigous_project_with_vcs_set_to_git(self):
> +        project = self.factory.makeProduct()
> +        bzr_branch = self.factory.makeBranch(target=project)
> +        repo = self.factory.makeGitRepository(target=project)
> +        with person_logged_in(project.owner):
> +            ICanHasLinkedBranch(project).setBranch(bzr_branch, project.owner)
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                project, repo)
> +            project.vcs = VCSType.GIT
> +
> +        self.assertRedirects(project.name, canonical_url(repo))
> +
> +    def test_ambigous_project_with_vcs_set_to_bzr(self):
> +        project = self.factory.makeProduct()
> +        bzr_branch = self.factory.makeBranch(target=project)
> +        repo = self.factory.makeGitRepository(target=project)
> +        with person_logged_in(project.owner):
> +            ICanHasLinkedBranch(project).setBranch(bzr_branch, project.owner)
> +            getUtility(IGitRepositorySet).setDefaultRepository(
> +                project, repo)
> +            project.vcs = VCSType.BZR
> +
> +        self.assertRedirects(project.name, canonical_url(bzr_branch))
> +
> +    def test_personal_branch_bzr(self):
> +        person = self.factory.makePerson()
> +        branch = self.factory.makePersonalBranch(owner=person)
> +        self.assertRedirects(branch.unique_name, canonical_url(branch))
> +
> +    def test_personal_branch_git(self):
> +        person = self.factory.makePerson()
> +        repo = self.factory.makeGitRepository(owner=person, target=person)
> +        self.assertRedirects(repo.unique_name, canonical_url(repo))
> +
> +
>  class TestPersonTraversal(TestCaseWithFactory, TraversalMixin):
>  
>      layer = DatabaseFunctionalLayer
> 
> === modified file 'lib/lp/app/browser/tests/test_linkchecker.py'
> --- lib/lp/app/browser/tests/test_linkchecker.py	2012-09-17 15:19:10 +0000
> +++ lib/lp/app/browser/tests/test_linkchecker.py	2016-06-20 21:23:40 +0000
> @@ -43,9 +43,22 @@
>          removeSecurityProxy(product).development_focus.branch = product_branch
>          valid_product_url = self.BRANCH_URL_TEMPLATE % product.name
>  
> +        # git branches are a thing now as well!
> +        project_git_repo = removeSecurityProxy(
> +            self.factory.makeGitRepository())

target=product here, I think.

> +        dsp = self.factory.makeDistributionSourcePackage()
> +        dsp_git_repo = removeSecurityProxy(
> +            self.factory.makeGitRepository(target=dsp))
> +        person = self.factory.makePerson()
> +        person_git_repo = removeSecurityProxy(
> +            self.factory.makeGitRepository(target=person))
> +
>          return [
>              valid_branch_url,
>              valid_product_url,
> +            self.BRANCH_URL_TEMPLATE % project_git_repo.unique_name,
> +            self.BRANCH_URL_TEMPLATE % dsp_git_repo.unique_name,
> +            self.BRANCH_URL_TEMPLATE % person_git_repo.unique_name,

I noted while looking at context for this that BRANCH_URL_TEMPLATE should probably be changed from '/+branch/%s' to '/+code/%s'.  I'm not quite sure why the tests apparently succeeded for you given that, so this might be worth looking into.

>          ]
>  
>      def make_invalid_branch_links(self):


-- 
https://code.launchpad.net/~thomir/launchpad/devel-fix-git-links/+merge/297963
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.