launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00780
[Merge] lp:~mwhudson/launchpad/cross-product-spec-links-bug-3552 into lp:launchpad/devel
Michael Hudson has proposed merging lp:~mwhudson/launchpad/cross-product-spec-links-bug-3552 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#3552 No way to cross-link specs across products
https://bugs.launchpad.net/bugs/3552
Hi,
This branch changes the way the vocabulary for specification dependency candidates works. In particular, it allows you to create cross-pillar dependencies by entering the URL of a specification.
I had a pre-imp chat with Tim.
Cheers,
mwh
--
https://code.launchpad.net/~mwhudson/launchpad/cross-product-spec-links-bug-3552/+merge/33866
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/cross-product-spec-links-bug-3552 into lp:launchpad/devel.
=== modified file 'lib/lp/blueprints/browser/specificationdependency.py'
--- lib/lp/blueprints/browser/specificationdependency.py 2010-08-26 03:02:29 +0000
+++ lib/lp/blueprints/browser/specificationdependency.py 2010-08-27 04:42:08 +0000
@@ -37,7 +37,9 @@
"If another blueprint needs to be fully implemented "
"before this feature can be started, then specify that "
"dependency here so Launchpad knows about it and can "
- "give you an accurate project plan."))
+ "give you an accurate project plan. You can enter the "
+ "name of a blueprint that has the same target, or the "
+ "URL of any blueprint."))
class SpecificationDependencyAddView(LaunchpadFormView):
=== added file 'lib/lp/blueprints/browser/tests/test_specificationdependency.py'
--- lib/lp/blueprints/browser/tests/test_specificationdependency.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationdependency.py 2010-08-27 04:42:08 +0000
@@ -0,0 +1,30 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""XXX: Module docstring goes here."""
+
+__metaclass__ = type
+
+import unittest
+
+from canonical.launchpad.webapp import canonical_url
+from canonical.testing import DatabaseFunctionalLayer
+from lp.testing import BrowserTestCase
+
+
+class TestAddDependency(BrowserTestCase):
+ layer = DatabaseFunctionalLayer
+
+ def test_add_dependency_by_url(self):
+ # It is possible to use the URL of a specification in the "Depends On"
+ # field of the form to add a dependency to a spec.
+ spec = self.factory.makeSpecification(owner=self.user)
+ dependency = self.factory.makeSpecification()
+ browser = self.getViewBrowser(spec, '+linkdependency')
+ browser.getControl('Depends On').value = canonical_url(dependency)
+ browser.getControl('Continue').click()
+ self.assertIn(dependency, spec.dependencies)
+
+
+def test_suite():
+ return unittest.TestLoader().loadTestsFromName(__name__)
=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
--- lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-26 03:30:31 +0000
+++ lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-27 04:42:08 +0000
@@ -6,11 +6,16 @@
__metaclass__ = type
__all__ = ['SpecificationDepCandidatesVocabulary']
+from zope.component import getUtility
from zope.interface import implements
from zope.schema.vocabulary import SimpleTerm
from canonical.database.sqlbase import quote_like
from canonical.launchpad.helpers import shortlist
+from canonical.launchpad.webapp import (
+ canonical_url,
+ urlparse,
+ )
from canonical.launchpad.webapp.vocabulary import (
CountableIterator,
IHugeVocabulary,
@@ -19,15 +24,26 @@
from lp.blueprints.interfaces.specification import SpecificationFilter
from lp.blueprints.model.specification import Specification
-
+from lp.registry.interfaces.pillar import IPillarNameSet
class SpecificationDepCandidatesVocabulary(SQLObjectVocabularyBase):
"""Specifications that could be dependencies of this spec.
- This includes only those specs that are not blocked by this spec
- (directly or indirectly), unless they are already dependencies.
-
- The current spec is not included.
+ This includes only those specs that are not blocked by this spec (directly
+ or indirectly), unless they are already dependencies.
+
+ This vocabulary has a bit of a split personality.
+
+ Tokens are *either*:
+
+ - the name of a spec, in which case it must be a spec on the same target
+ as the context, or
+ - the full URL of the spec, in which case it can be any spec at all.
+
+ For the purposes of enumeration and searching we only consider the first
+ sort of spec for now. The URL form of token only matches precisely,
+ searching only looks for specs on the current target if the search term is
+ not a URL.
"""
implements(IHugeVocabulary)
@@ -36,8 +52,8 @@
_orderBy = 'name'
displayname = 'Select a blueprint'
- def _filter_specs(self, specs):
- """Filter `specs` to remove invalid candidates.
+ def _is_valid_candidate(self, spec, check_target=False):
+ """Is `spec` a valid candidate spec for self.context?
Invalid candidates are:
@@ -47,27 +63,64 @@
Preventing the last category prevents loops in the dependency graph.
"""
+ if check_target and spec.target != self.context.target:
+ return False
+ return spec != self.context and spec not in self.context.all_blocked
+
+ def _filter_specs(self, specs, check_target=False):
+ """Filter `specs` to remove invalid candidates.
+
+ See `_is_valid_candidate` for what an invalid candidate is.
+ """
# XXX intellectronica 2007-07-05: is 100 a reasonable count before
# starting to warn?
- speclist = shortlist(specs, 100)
- return [spec for spec in speclist
- if (spec != self.context and
- spec.target == self.context.target
- and spec not in self.context.all_blocked)]
+ return [spec for spec in shortlist(specs, 100)
+ if self._is_valid_candidate(spec, check_target)]
def toTerm(self, obj):
- return SimpleTerm(obj, obj.name, obj.title)
+ if obj.target == self.context.target:
+ token = obj.name
+ else:
+ token = canonical_url(obj)
+ return SimpleTerm(obj, token, obj.title)
+
+ def _spec_from_url(self, url):
+ """If `url` is the URL of a specification, return it.
+
+ This implementation is a little fuzzy and will return specs for URLs
+ that, for example, don't have the host name right. This seems
+ unlikely to cause confusion in practice, and being too anal probably
+ would be confusing (e.g. not accepting edge URLs on lpnet).
+ """
+ scheme, netloc, path, params, args, fragment = urlparse(url)
+ if not scheme or not netloc:
+ # Not enough like a URL
+ return None
+ path_segments = path.strip('/').split('/')
+ if len(path_segments) != 3:
+ # Can't be a spec url
+ return None
+ pillar_name, plus_spec, spec_name = path_segments
+ if plus_spec != '+spec':
+ # Can't be a spec url
+ return None
+ pillar = getUtility(IPillarNameSet).getByName(
+ pillar_name, ignore_inactive=True)
+ if pillar is None:
+ return None
+ return pillar.getSpecification(spec_name)
def getTermByToken(self, token):
"""See `zope.schema.interfaces.IVocabularyTokenized`.
- The tokens for specifications are just the name of the spec.
+ The tokens for specifications are either the name of a spec on the
+ same target or a URL for a spec.
"""
- spec = self.context.target.getSpecification(token)
- if spec is not None:
- filtered = self._filter_specs([spec])
- if len(filtered) > 0:
- return self.toTerm(filtered[0])
+ spec = self._spec_from_url(token)
+ if spec is None:
+ spec = self.context.target.getSpecification(token)
+ if spec and self._is_valid_candidate(spec):
+ return self.toTerm(spec)
raise LookupError(token)
def search(self, query):
@@ -79,6 +132,9 @@
"""
if not query:
return CountableIterator(0, [])
+ spec = self._spec_from_url(query)
+ if spec is not None and self._is_valid_candidate(spec):
+ return CountableIterator(1, [spec])
quoted_query = quote_like(query)
sql_query = ("""
(Specification.name LIKE %s OR
@@ -87,18 +143,18 @@
"""
% (quoted_query, quoted_query, quoted_query))
all_specs = Specification.select(sql_query, orderBy=self._orderBy)
- candidate_specs = self._filter_specs(all_specs)
+ candidate_specs = self._filter_specs(all_specs, check_target=True)
return CountableIterator(len(candidate_specs), candidate_specs)
@property
def _all_specs(self):
return self.context.target.specifications(
- filter=[SpecificationFilter.ALL],
- prejoin_people=False)
+ filter=[SpecificationFilter.ALL], prejoin_people=False)
def __iter__(self):
- return (self.toTerm(spec)
- for spec in self._filter_specs(self._all_specs))
+ return (
+ self.toTerm(spec) for spec in self._filter_specs(self._all_specs))
def __contains__(self, obj):
- return obj in self._all_specs and len(self._filter_specs([obj])) > 0
+ return self._is_valid_candidate(obj)
+
=== modified file 'lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt'
--- lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt 2010-08-25 05:39:32 +0000
+++ lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt 2010-08-27 04:42:08 +0000
@@ -22,26 +22,24 @@
>>> sorted([spec.name for spec in specced_product.specifications()])
[u'spec-a', u'spec-b', u'spec-c']
-The dependency candidates for spec_a are all blueprints for
-specced_product except for spec_a itself.
+
+Iterating over the vocabulary gives all blueprints for specced_product
+except for spec_a itself.
>>> vocab = vocabulary_registry.get(
... spec_a, "SpecificationDepCandidates")
>>> sorted([term.value.name for term in vocab])
[u'spec-b', u'spec-c']
-Dependency candidate come only from the same product of the blueprint
-they depend on.
+Blueprints for other targets are considered to be 'in' the vocabulary
+though.
>>> unrelated_spec = factory.makeSpecification(
... product=factory.makeProduct())
>>> vocab = vocabulary_registry.get(
... spec_a, "SpecificationDepCandidates")
>>> unrelated_spec in vocab
- False
- >>> [term.value.product for term in vocab
- ... if term.value.product != specced_product]
- []
+ True
We mark spec_b as a dependency of spec_a and spec_c as a dependency of
spec_b.
=== modified file 'lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py'
--- lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2010-08-26 22:07:41 +0000
+++ lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2010-08-27 04:42:08 +0000
@@ -10,6 +10,7 @@
from zope.schema.vocabulary import getVocabularyRegistry
+from canonical.launchpad.webapp import canonical_url
from canonical.testing import DatabaseFunctionalLayer
from lp.testing import TestCaseWithFactory
@@ -24,7 +25,7 @@
return getVocabularyRegistry().get(
spec, name='SpecificationDepCandidates')
- def test_getTermByToken_product(self):
+ def test_getTermByToken_by_name_for_product(self):
# Calling getTermByToken for a dependency vocab for a spec from a
# product with the name of an acceptable candidate spec returns the
# term for the candidate
@@ -35,7 +36,7 @@
self.assertEqual(
candidate, vocab.getTermByToken(candidate.name).value)
- def test_getTermByToken_distro(self):
+ def test_getTermByToken_by_name_for_distro(self):
# Calling getTermByToken for a dependency vocab for a spec from a
# distribution with the name of an acceptable candidate spec returns
# the term for the candidate
@@ -46,7 +47,55 @@
self.assertEqual(
candidate, vocab.getTermByToken(candidate.name).value)
- def test_getTermByToken_disallows_blocked(self):
+ def test_getTermByToken_by_url_for_product(self):
+ # Calling getTermByToken with the full URL for a spec on a product
+ # returns that spec, irrespective of the context's target.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification(
+ product=self.factory.makeProduct())
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertEqual(
+ candidate, vocab.getTermByToken(canonical_url(candidate)).value)
+
+ def test_getTermByToken_by_url_for_distro(self):
+ # Calling getTermByToken with the full URL for a spec on a
+ # distribution returns that spec, irrespective of the context's
+ # target.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification(
+ distribution=self.factory.makeDistribution())
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertEqual(
+ candidate, vocab.getTermByToken(canonical_url(candidate)).value)
+
+ def test_getTermByToken_lookup_error_on_nonsense(self):
+ # getTermByToken with the a string that does not name a spec raises
+ # LookupError.
+ product = self.factory.makeProduct()
+ spec = self.factory.makeSpecification(product=product)
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertRaises(
+ LookupError, vocab.getTermByToken, self.factory.getUniqueString())
+
+ def test_getTermByToken_lookup_error_on_url_with_invalid_pillar(self):
+ # getTermByToken with the a string that looks like a blueprint URL but
+ # has an invalid pillar name raises LookupError.
+ spec = self.factory.makeSpecification()
+ url = canonical_url(spec).replace(
+ spec.target.name, self.factory.getUniqueString())
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertRaises(LookupError, vocab.getTermByToken, url)
+
+ def test_getTermByToken_lookup_error_on_url_with_invalid_spec_name(self):
+ # getTermByToken with the a string that looks like a blueprint URL but
+ # has an invalid spec name raises LookupError.
+ spec = self.factory.makeSpecification()
+ url = canonical_url(spec).replace(
+ spec.name, self.factory.getUniqueString())
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertRaises(LookupError, vocab.getTermByToken, url)
+
+ def test_getTermByToken_by_name_disallows_blocked(self):
# getTermByToken with the name of an candidate spec that is blocked by
# the vocab's context raises LookupError.
product = self.factory.makeProduct()
@@ -56,17 +105,74 @@
vocab = self.getVocabularyForSpec(spec)
self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)
- def test_getTermByToken_disallows_context(self):
+ def test_getTermByToken_by_url_disallows_blocked(self):
+ # getTermByToken with the URL of an candidate spec that is blocked by
+ # the vocab's context raises LookupError.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification()
+ candidate.createDependency(spec)
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertRaises(
+ LookupError, vocab.getTermByToken, canonical_url(candidate))
+
+ def test_getTermByToken_by_name_disallows_context(self):
# getTermByToken with the name of the vocab's context raises
# LookupError.
spec = self.factory.makeSpecification()
vocab = self.getVocabularyForSpec(spec)
self.assertRaises(LookupError, vocab.getTermByToken, spec.name)
- def test_getTermByToken_disallows_spec_for_other_target(self):
+ def test_getTermByToken_by_url_disallows_context(self):
+ # getTermByToken with the URL of the vocab's context raises
+ # LookupError.
+ spec = self.factory.makeSpecification()
+ vocab = self.getVocabularyForSpec(spec)
+ self.assertRaises(
+ LookupError, vocab.getTermByToken, canonical_url(spec))
+
+ def test_getTermByToken_by_name_disallows_spec_for_other_target(self):
# getTermByToken with the name of a spec with a different target
# raises LookupError.
spec = self.factory.makeSpecification()
candidate = self.factory.makeSpecification()
vocab = self.getVocabularyForSpec(spec)
self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)
+
+ def test_searchForTerms_by_url(self):
+ # Calling searchForTerms with the URL of a valid candidate spec
+ # returns just that spec.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification()
+ vocab = self.getVocabularyForSpec(spec)
+ results = vocab.searchForTerms(canonical_url(candidate))
+ self.assertEqual(1, len(results))
+ self.assertEqual(candidate, list(results)[0].value)
+
+ def test_searchForTerms_by_url_rejects_invalid(self):
+ # Calling searchForTerms with the URL of a invalid candidate spec
+ # returns an empty iterator.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification()
+ candidate.createDependency(spec)
+ vocab = self.getVocabularyForSpec(spec)
+ results = vocab.searchForTerms(canonical_url(candidate))
+ self.assertEqual(0, len(results))
+
+ def test_token_for_same_target_dep_is_name(self):
+ # The 'token' part of the term for a dependency candidate that has the
+ # same target is just the name of the candidate.
+ product = self.factory.makeProduct()
+ spec = self.factory.makeSpecification(product=product)
+ candidate = self.factory.makeSpecification(product=product)
+ vocab = self.getVocabularyForSpec(spec)
+ term = vocab.getTermByToken(candidate.name)
+ self.assertEqual(term.token, candidate.name)
+
+ def test_token_for_different_target_dep_is_url(self):
+ # The 'token' part of the term for a dependency candidate that has a
+ # different target is the canonical url of the candidate.
+ spec = self.factory.makeSpecification()
+ candidate = self.factory.makeSpecification()
+ vocab = self.getVocabularyForSpec(spec)
+ term = vocab.getTermByToken(canonical_url(candidate))
+ self.assertEqual(term.token, canonical_url(candidate))
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-08-26 22:44:30 +0000
+++ lib/lp/testing/factory.py 2010-08-27 04:42:08 +0000
@@ -1496,7 +1496,7 @@
return mail
def makeSpecification(self, product=None, title=None, distribution=None,
- name=None, summary=None,
+ name=None, summary=None, owner=None,
status=SpecificationDefinitionStatus.NEW):
"""Create and return a new, arbitrary Blueprint.
@@ -1511,13 +1511,15 @@
summary = self.getUniqueString('summary')
if title is None:
title = self.getUniqueString('title')
+ if owner is None:
+ owner = self.makePerson()
return getUtility(ISpecificationSet).new(
name=name,
title=title,
specurl=None,
summary=summary,
definition_status=status,
- owner=self.makePerson(),
+ owner=owner,
product=product,
distribution=distribution)
