launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20649
[Merge] lp:~thomir/launchpad/devel-fix-git-links into lp:launchpad
Thomi Richards has proposed merging lp:~thomir/launchpad/devel-fix-git-links into lp:launchpad.
Commit message:
Links to git repositories are now linked correctly.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~thomir/launchpad/devel-fix-git-links/+merge/297963
Make links to git repositories work in launchpad.
There are several changes here:
* Links to bzr branches or git repos in the form lp:<path> now generate expanded links in the form: /+code/<path> instead of /+branch/<path>. The +branch form still exists, as it's used by bzr clients, but it only deals with bazaar branches.
* The link checker API method now deals with +code links, instead of +branch links.
* +code links now traverse correctly to git repositories or bzr branches.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thomir/launchpad/devel-fix-git-links into lp:launchpad.
=== 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
@@ -78,6 +78,7 @@
)
from lp.app.errors import (
GoneError,
+ NameLookupFailed,
NotFoundError,
POSTToNonCanonicalURL,
)
@@ -104,8 +105,10 @@
from lp.code.interfaces.codehosting import IBazaarApplication
from lp.code.interfaces.codeimport import ICodeImportSet
from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.code.interfaces.gitlookup import IGitLookup
from lp.hardwaredb.interfaces.hwdb import IHWDBApplication
from lp.layers import WebServiceLayer
+from lp.registry.enums import VCSType
from lp.registry.interfaces.announcement import IAnnouncementSet
from lp.registry.interfaces.codeofconduct import ICodeOfConductSet
from lp.registry.interfaces.distribution import IDistributionSet
@@ -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 = ''
+ 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:
+ 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:
+ 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
+ # 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/linkchecker.py'
--- lib/lp/app/browser/linkchecker.py 2013-01-07 02:40:55 +0000
+++ lib/lp/app/browser/linkchecker.py 2016-06-20 21:23:40 +0000
@@ -19,6 +19,7 @@
NoSuchBranch,
)
from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.gitlookup import IGitLookup
from lp.registry.interfaces.product import InvalidProductName
from lp.services.searchbuilder import any
from lp.services.webapp import LaunchpadView
@@ -34,7 +35,7 @@
something appropriate.
This initial implementation supports processing links like the following:
- /+branch/foo/bar
+ /+code/foo/bar
The implementation can easily be extended to handle other forms by
providing a method to handle the link type extracted from the json
@@ -68,17 +69,19 @@
return simplejson.dumps(result)
def check_branch_links(self, links):
- """Check links of the form /+branch/foo/bar"""
+ """Check links of the form /+code/foo/bar"""
invalid_links = {}
- branch_lookup = getUtility(IBranchLookup)
+ bzr_branch_lookup = getUtility(IBranchLookup)
+ git_branch_lookup = getUtility(IGitLookup)
for link in links:
- path = link.strip('/')[len('+branch/'):]
- try:
- branch_lookup.getByLPPath(path)
- except (CannotHaveLinkedBranch, InvalidNamespace,
- InvalidProductName, NoLinkedBranch, NoSuchBranch,
- NotFoundError) as e:
- invalid_links[link] = self._error_message(e)
+ path = link.strip('/')[len('+code/'):]
+ if git_branch_lookup.getByPath(path)[0] is None:
+ try:
+ bzr_branch_lookup.getByLPPath(path)
+ except (CannotHaveLinkedBranch, InvalidNamespace,
+ InvalidProductName, NoLinkedBranch, NoSuchBranch,
+ NotFoundError) as e:
+ invalid_links[link] = self._error_message(e)
return {'invalid': invalid_links}
def check_bug_links(self, links):
=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py 2015-10-26 14:54:43 +0000
+++ lib/lp/app/browser/stringformatter.py 2016-06-20 21:23:40 +0000
@@ -455,7 +455,7 @@
if path.isdigit():
return FormattersAPI._linkify_bug_number(
lp_url, path, trailers)
- url = '/+branch/%s' % path
+ url = '/+code/%s' % path
# Mark the links with a 'branch-short-link' class so they can be
# harvested and validated when the page is rendered.
return structured(
=== 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
@@ -22,9 +22,11 @@
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.app.interfaces.services import IService
from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
+from lp.code.interfaces.gitrepository import IGitRepositorySet
from lp.registry.enums import (
PersonVisibility,
SharingPermission,
+ VCSType,
)
from lp.registry.interfaces.person import IPersonSet
from lp.services.identity.interfaces.account import AccountStatus
@@ -38,6 +40,7 @@
from lp.services.webapp.url import urlappend
from lp.testing import (
ANONYMOUS,
+ admin_logged_in,
celebrity_logged_in,
login,
login_person,
@@ -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):
+ 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())
+ 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,
]
def make_invalid_branch_links(self):
Follow ups
