Merge lp:~thumper/launchpad/blueprint-dependency-vocabulary into lp:launchpad

[Tim Penhey]

[r=mwhudson][bug=707111] Allow any blueprint to be searched for when adding dependencies.

Allow the vocabulary to work with any dependency.

[Michael Hudson-Doyle (mwhudson)]

Hi Tim,

This basically looks fine. The only comments I have are:

 * I think the ISpecification check is a bit over the top (already said this on IRC),
 * I think _order_by.__doc__ could do with expansion (or deletion -- the current docstring doesn't really add anything)
 * I think "_exclude_blocked_query" would be a better name than "_blocked_subselect" (the latter sounds a bit like it returns the blocked specs)
 * "Not" is imported but not used in ./lib/lp/blueprints/vocabularies/specificationdependency.py

"make lint" also reports a few long lines, up to you if you care :)

review: Approve

[Tim Penhey (thumper)]

On Tue, 15 Mar 2011 12:13:40 you wrote:
> Review: Approve
> Hi Tim,
>
> This basically looks fine. The only comments I have are:
>
> * I think the ISpecification check is a bit over the top (already said
> this on IRC), * I think _order_by.__doc__ could do with expansion (or
> deletion -- the current docstring doesn't really add anything) * I think
> "_exclude_blocked_query" would be a better name than "_blocked_subselect"
> (the latter sounds a bit like it returns the blocked specs) * "Not" is
> imported but not used in
> ./lib/lp/blueprints/vocabularies/specificationdependency.py
>
> "make lint" also reports a few long lines, up to you if you care :)

Addressed, and thanks.

=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
--- lib/lp/blueprints/vocabularies/specificationdependency.py	2011-03-14 21:25:44 +0000
+++ lib/lp/blueprints/vocabularies/specificationdependency.py	2011-03-15 08:46:24 +0000
@@ -11,12 +11,12 @@
 
 from operator import attrgetter
 
+from storm.locals import SQL, Store
 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.database.sqlbase import quote
 from canonical.launchpad.webapp import (
     canonical_url,
     urlparse,
@@ -27,9 +27,11 @@
     NamedSQLObjectVocabulary,
     SQLObjectVocabularyBase,
     )
-
-from lp.blueprints.enums import SpecificationFilter
-from lp.blueprints.model.specification import Specification
+from lp.blueprints.interfaces.specification import ISpecification
+from lp.blueprints.model.specification import (
+    recursive_blocked_query,
+    Specification,
+    )
 from lp.registry.interfaces.pillar import IPillarNameSet
 
 
@@ -47,10 +49,10 @@
        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.
+    For the purposes of enumeration and searching we look at all the possible
+    specifications, but order those of the same target first.  If there is an
+    associated series as well, then those are shown before other matches not
+    linked to the same series.
     """
 
     implements(IHugeVocabulary)
@@ -60,30 +62,60 @@
     displayname = 'Select a blueprint'
     step_title = 'Search'
 
-    def _is_valid_candidate(self, spec, check_target=False):
+    def _is_valid_candidate(self, spec):
         """Is `spec` a valid candidate spec for self.context?
 
         Invalid candidates are:
 
-         * The spec that we're adding a depdency to,
-         * Specs for a different target, and
-         * Specs that depend on this one.
+         * None
+         * The spec that we're adding a depdency to
+         * Specs that depend on this one
 
         Preventing the last category prevents loops in the dependency graph.
         """
-        if check_target and spec.target != self.context.target:
+        if spec is None or spec == self.context:
             return False
-        return spec != self.context and spec not in set(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.
+        return spec not in set(self.context.all_blocked)
+
+    def _order_by(self):
+        """Look at the context to provide grouping.
+
+        If the blueprint is for a project, then matching results for that
+        project should be first.  If the blueprint is set for a series, then
+        that series should come before others for the project.  Similarly for
+        the distribution, and the series goal for the distribution.
+
+        If all else is equal, the ordering is by name, then database id as a
+        final uniqueness resolver.
         """
-        # XXX intellectronica 2007-07-05: is 100 a reasonable count before
-        # starting to warn?
-        return [spec for spec in shortlist(specs, 100)
-                if self._is_valid_candidate(spec, check_target)]
+        order_statements = []
+        spec = self.context
+        if spec.product is not None:
+            order_statements.append(
+                "(CASE Specification.product WHEN %s THEN 0 ELSE 1 END)" %
+                spec.product.id)
+            if spec.productseries is not None:
+                order_statements.append(
+                    "(CASE Specification.productseries"
+                    " WHEN %s THEN 0 ELSE 1 END)" %
+                    spec.productseries.id)
+        elif spec.distribution is not None:
+            order_statements.append(
+                "(CASE Specification.distribution WHEN %s THEN 0 ELSE 1 END)"
+                % spec.distribution.id)
+            if spec.distroseries is not None:
+                order_statements.append(
+                    "(CASE Specification.distroseries"
+                    " WHEN %s THEN 0 ELSE 1 END)" %
+                    spec.distroseries.id)
+        order_statements.append("Specification.name")
+        order_statements.append("Specification.id")
+        return SQL(', '.join(order_statements))
+
+    def _exclude_blocked_query(self):
+        """Return the select statement to exclude already blocked specs."""
+        return SQL("Specification.id not in (WITH %s select id from blocked)"
+                   % recursive_blocked_query(self.context))
 
     def toTerm(self, obj):
         if obj.target == self.context.target:
@@ -127,7 +159,7 @@
         spec = self._spec_from_url(token)
         if spec is None:
             spec = self.context.target.getSpecification(token)
-        if spec and self._is_valid_candidate(spec):
+        if self._is_valid_candidate(spec):
             return self.toTerm(spec)
         raise LookupError(token)
 
@@ -141,27 +173,18 @@
         if not query:
             return CountableIterator(0, [])
         spec = self._spec_from_url(query)
-        if spec is not None and self._is_valid_candidate(spec):
+        if self._is_valid_candidate(spec):
             return CountableIterator(1, [spec])
-        quoted_query = quote_like(query)
-        sql_query = ("""
-            (Specification.name LIKE %s OR
-             Specification.title LIKE %s OR
-             fti @@ ftq(%s))
-            """
-            % (quoted_query, quoted_query, quoted_query))
-        all_specs = Specification.select(sql_query, orderBy=self._orderBy)
-        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)
+        return Store.of(self.context).find(
+            Specification,
+            SQL('Specification.fti @@ ftq(%s)' % quote(query)),
+            self._exclude_blocked_query(),
+            ).order_by(self._order_by())
 
     def __iter__(self):
-        return (
-            self.toTerm(spec) for spec in self._filter_specs(self._all_specs))
+        # We don't ever want to iterate over everything.
+        raise NotImplementedError()
 
     def __contains__(self, obj):
         return self._is_valid_candidate(obj)
 
=== removed file 'lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt'
--- lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt	2011-03-15 02:39:51 +0000
+++ lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt	1970-01-01 00:00:00 +0000
@@ -1,97 +0,0 @@
-SpecificationDepCandidatesVocabulary
-====================================
-
-All blueprints that can be added as a dependency of the context
-blueprint.
-
-    >>> from zope.schema.vocabulary import getVocabularyRegistry
-    >>> vocabulary_registry = getVocabularyRegistry()
-
-First, we set up a product with three blueprints.
-
-    >>> specced_product = factory.makeProduct()
-    >>> spec_a = factory.makeSpecification(
-    ...     name='spec-a', summary='The first spec',
-    ...     product=specced_product)
-    >>> spec_b = factory.makeSpecification(
-    ...     name='spec-b', summary='The second spec',
-    ...     product=specced_product)
-    >>> spec_c = factory.makeSpecification(
-    ...     name='spec-c', summary='The third spec',
-    ...     product=specced_product)
-    >>> sorted([spec.name for spec in specced_product.specifications()])
-    [u'spec-a', u'spec-b', u'spec-c']
-
-
-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']
-
-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
-    True
-
-We mark spec_b as a dependency of spec_a and spec_c as a dependency of
-spec_b.
-
-    >>> spec_a.createDependency(spec_b)
-    <SpecificationDependency at ...>
-    >>> [spec.name for spec in spec_a.dependencies]
-    [u'spec-b']
-
-    >>> spec_b.createDependency(spec_c)
-    <SpecificationDependency at ...>
-    >>> [spec.name for spec in spec_b.dependencies]
-    [u'spec-c']
-
-No circular dependencies - the vocabulary excludes specifications that
-are a dependency of the context spec.
-
-    >>> spec_a in set(spec_b.all_blocked)
-    True
-    >>> spec_b in set(spec_c.all_blocked)
-    True
-    >>> vocab = vocabulary_registry.get(
-    ...     spec_c, "SpecificationDepCandidates")
-    >>> spec_a in [term.value for term in vocab]
-    False
-
-This vocabulary provides the IHugeVocabulary interface.
-
-    >>> from canonical.launchpad.webapp.testing import verifyObject
-    >>> from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
-    >>> verifyObject(IHugeVocabulary, vocab)
-    True
-
-The search() method returns specifications within the vocabulary that
-matches the search string. The string is matched against the name, or
-fallbacks to a full text search.
-
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> naked_vocab = removeSecurityProxy(
-    ...     vocabulary_registry.get(
-    ...         spec_a, "SpecificationDepCandidates"))
-    >>> list(naked_vocab.search('spec-b')) == [spec_b]
-    True
-    >>> list(naked_vocab.search('third')) == [spec_c]
-    True
-
-The search method uses the SQL `LIKE` operator, with the values quoted
-appropriately. Queries conataining regual expression operators, for
-example, will simply look for the respective characters within the
-vocabulary's item (this used to be the cause of an OOPS, see
-https://bugs.launchpad.net/blueprint/+bug/139385 for more
-details).
-
-    >>> list(naked_vocab.search('*'))
-    []
 
=== removed file 'lib/lp/blueprints/vocabularies/tests/test_doc.py'
--- lib/lp/blueprints/vocabularies/tests/test_doc.py	2010-08-25 05:17:20 +0000
+++ lib/lp/blueprints/vocabularies/tests/test_doc.py	1970-01-01 00:00:00 +0000
@@ -1,17 +0,0 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""
-Run the doctests.
-"""
-
-import os
-
-from lp.services.testing import build_doctest_suite
-
-
-here = os.path.dirname(os.path.realpath(__file__))
-
-
-def test_suite():
-    return build_doctest_suite(here, '')
 
=== modified file 'lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py'
--- lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py	2010-10-04 19:50:45 +0000
+++ lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py	2011-03-15 08:46:24 +0000
@@ -176,3 +176,72 @@
         vocab = self.getVocabularyForSpec(spec)
         term = vocab.getTermByToken(canonical_url(candidate))
         self.assertEqual(term.token, canonical_url(candidate))
+
+    def test_searchForTerms_ordering_different_targets_by_name(self):
+        # If the searched for specs are on different targets, the ordering is
+        # by name.
+        spec = self.factory.makeSpecification()
+        foo_b = self.factory.makeSpecification(name='foo-b')
+        foo_a = self.factory.makeSpecification(name='foo-a')
+        vocab = self.getVocabularyForSpec(spec)
+        results = vocab.searchForTerms('foo')
+        self.assertEqual(2, len(results))
+        found = [item.value for item in results]
+        self.assertEqual([foo_a, foo_b], found)
+
+    def test_searchForTerms_ordering_same_product_first(self):
+        # Specs on the same product are returned first.
+        widget = self.factory.makeProduct()
+        spec = self.factory.makeSpecification(product=widget)
+        foo_b = self.factory.makeSpecification(name='foo-b', product=widget)
+        foo_a = self.factory.makeSpecification(name='foo-a')
+        vocab = self.getVocabularyForSpec(spec)
+        results = vocab.searchForTerms('foo')
+        self.assertEqual(2, len(results))
+        found = [item.value for item in results]
+        self.assertEqual([foo_b, foo_a], found)
+
+    def test_searchForTerms_ordering_same_productseries_first(self):
+        # Specs on the same product series are returned before the same
+        # products, and those before others.
+        widget = self.factory.makeProduct()
+        spec = self.factory.makeSpecification(product=widget)
+        spec.proposeGoal(widget.development_focus, widget.owner)
+        foo_c = self.factory.makeSpecification(name='foo-c', product=widget)
+        foo_c.proposeGoal(widget.development_focus, widget.owner)
+        foo_b = self.factory.makeSpecification(name='foo-b', product=widget)
+        foo_a = self.factory.makeSpecification(name='foo-a')
+        vocab = self.getVocabularyForSpec(spec)
+        results = vocab.searchForTerms('foo')
+        self.assertEqual(3, len(results))
+        found = [item.value for item in results]
+        self.assertEqual([foo_c, foo_b, foo_a], found)
+
+    def test_searchForTerms_ordering_same_distribution_first(self):
+        # Specs on the same distribution are returned first.
+        mint = self.factory.makeDistribution()
+        spec = self.factory.makeSpecification(distribution=mint)
+        foo_b = self.factory.makeSpecification(name='foo-b', distribution=mint)
+        foo_a = self.factory.makeSpecification(name='foo-a')
+        vocab = self.getVocabularyForSpec(spec)
+        results = vocab.searchForTerms('foo')
+        self.assertEqual(2, len(results))
+        found = [item.value for item in results]
+        self.assertEqual([foo_b, foo_a], found)
+
+    def test_searchForTerms_ordering_same_distroseries_first(self):
+        # Specs on the same distroseries are returned before the same
+        # distribution, and those before others.
+        mint = self.factory.makeDistribution()
+        next = self.factory.makeDistroSeries(mint)
+        spec = self.factory.makeSpecification(distribution=mint)
+        spec.proposeGoal(next, mint.owner)
+        foo_c = self.factory.makeSpecification(name='foo-c', distribution=mint)
+        foo_c.proposeGoal(next, mint.owner)
+        foo_b = self.factory.makeSpecification(name='foo-b', distribution=mint)
+        foo_a = self.factory.makeSpecification(name='foo-a')
+        vocab = self.getVocabularyForSpec(spec)
+        results = vocab.searchForTerms('foo')
+        self.assertEqual(3, len(results))
+        found = [item.value for item in results]
+        self.assertEqual([foo_c, foo_b, foo_a], found)