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

Proposed by Tim Penhey on 2011-03-14
Status: Merged
Approved by: Michael Hudson-Doyle on 2011-03-14
Approved revision: no longer in the source branch.
Merged at revision: 12598
Proposed branch: lp:~thumper/launchpad/blueprint-dependency-vocabulary
Merge into: lp:launchpad
Prerequisite: lp:~thumper/launchpad/blueprint-dependencies
Diff against target: 370 lines (+134/-156)
4 files modified
lib/lp/blueprints/vocabularies/specificationdependency.py (+65/-42)
lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt (+0/-97)
lib/lp/blueprints/vocabularies/tests/test_doc.py (+0/-17)
lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py (+69/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/blueprint-dependency-vocabulary
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle 2011-03-14 Approve on 2011-03-14
Review via email: mp+53192@code.launchpad.net

Commit message

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

Description of the change

Allow the vocabulary to work with any dependency.

To post a comment you must log in.
Michael Hudson-Doyle (mwhudson) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
2--- lib/lp/blueprints/vocabularies/specificationdependency.py 2011-03-14 21:25:44 +0000
3+++ lib/lp/blueprints/vocabularies/specificationdependency.py 2011-03-15 08:46:24 +0000
4@@ -11,12 +11,12 @@
5
6 from operator import attrgetter
7
8+from storm.locals import SQL, Store
9 from zope.component import getUtility
10 from zope.interface import implements
11 from zope.schema.vocabulary import SimpleTerm
12
13-from canonical.database.sqlbase import quote_like
14-from canonical.launchpad.helpers import shortlist
15+from canonical.database.sqlbase import quote
16 from canonical.launchpad.webapp import (
17 canonical_url,
18 urlparse,
19@@ -27,9 +27,11 @@
20 NamedSQLObjectVocabulary,
21 SQLObjectVocabularyBase,
22 )
23-
24-from lp.blueprints.enums import SpecificationFilter
25-from lp.blueprints.model.specification import Specification
26+from lp.blueprints.interfaces.specification import ISpecification
27+from lp.blueprints.model.specification import (
28+ recursive_blocked_query,
29+ Specification,
30+ )
31 from lp.registry.interfaces.pillar import IPillarNameSet
32
33
34@@ -47,10 +49,10 @@
35 as the context, or
36 - the full URL of the spec, in which case it can be any spec at all.
37
38- For the purposes of enumeration and searching we only consider the first
39- sort of spec for now. The URL form of token only matches precisely,
40- searching only looks for specs on the current target if the search term is
41- not a URL.
42+ For the purposes of enumeration and searching we look at all the possible
43+ specifications, but order those of the same target first. If there is an
44+ associated series as well, then those are shown before other matches not
45+ linked to the same series.
46 """
47
48 implements(IHugeVocabulary)
49@@ -60,30 +62,60 @@
50 displayname = 'Select a blueprint'
51 step_title = 'Search'
52
53- def _is_valid_candidate(self, spec, check_target=False):
54+ def _is_valid_candidate(self, spec):
55 """Is `spec` a valid candidate spec for self.context?
56
57 Invalid candidates are:
58
59- * The spec that we're adding a depdency to,
60- * Specs for a different target, and
61- * Specs that depend on this one.
62+ * None
63+ * The spec that we're adding a depdency to
64+ * Specs that depend on this one
65
66 Preventing the last category prevents loops in the dependency graph.
67 """
68- if check_target and spec.target != self.context.target:
69+ if spec is None or spec == self.context:
70 return False
71- return spec != self.context and spec not in set(self.context.all_blocked)
72-
73- def _filter_specs(self, specs, check_target=False):
74- """Filter `specs` to remove invalid candidates.
75-
76- See `_is_valid_candidate` for what an invalid candidate is.
77+ return spec not in set(self.context.all_blocked)
78+
79+ def _order_by(self):
80+ """Look at the context to provide grouping.
81+
82+ If the blueprint is for a project, then matching results for that
83+ project should be first. If the blueprint is set for a series, then
84+ that series should come before others for the project. Similarly for
85+ the distribution, and the series goal for the distribution.
86+
87+ If all else is equal, the ordering is by name, then database id as a
88+ final uniqueness resolver.
89 """
90- # XXX intellectronica 2007-07-05: is 100 a reasonable count before
91- # starting to warn?
92- return [spec for spec in shortlist(specs, 100)
93- if self._is_valid_candidate(spec, check_target)]
94+ order_statements = []
95+ spec = self.context
96+ if spec.product is not None:
97+ order_statements.append(
98+ "(CASE Specification.product WHEN %s THEN 0 ELSE 1 END)" %
99+ spec.product.id)
100+ if spec.productseries is not None:
101+ order_statements.append(
102+ "(CASE Specification.productseries"
103+ " WHEN %s THEN 0 ELSE 1 END)" %
104+ spec.productseries.id)
105+ elif spec.distribution is not None:
106+ order_statements.append(
107+ "(CASE Specification.distribution WHEN %s THEN 0 ELSE 1 END)"
108+ % spec.distribution.id)
109+ if spec.distroseries is not None:
110+ order_statements.append(
111+ "(CASE Specification.distroseries"
112+ " WHEN %s THEN 0 ELSE 1 END)" %
113+ spec.distroseries.id)
114+ order_statements.append("Specification.name")
115+ order_statements.append("Specification.id")
116+ return SQL(', '.join(order_statements))
117+
118+ def _exclude_blocked_query(self):
119+ """Return the select statement to exclude already blocked specs."""
120+ return SQL("Specification.id not in (WITH %s select id from blocked)"
121+ % recursive_blocked_query(self.context))
122
123 def toTerm(self, obj):
124 if obj.target == self.context.target:
125@@ -127,7 +159,7 @@
126 spec = self._spec_from_url(token)
127 if spec is None:
128 spec = self.context.target.getSpecification(token)
129- if spec and self._is_valid_candidate(spec):
130+ if self._is_valid_candidate(spec):
131 return self.toTerm(spec)
132 raise LookupError(token)
133
134@@ -141,27 +173,18 @@
135 if not query:
136 return CountableIterator(0, [])
137 spec = self._spec_from_url(query)
138- if spec is not None and self._is_valid_candidate(spec):
139+ if self._is_valid_candidate(spec):
140 return CountableIterator(1, [spec])
141- quoted_query = quote_like(query)
142- sql_query = ("""
143- (Specification.name LIKE %s OR
144- Specification.title LIKE %s OR
145- fti @@ ftq(%s))
146- """
147- % (quoted_query, quoted_query, quoted_query))
148- all_specs = Specification.select(sql_query, orderBy=self._orderBy)
149- candidate_specs = self._filter_specs(all_specs, check_target=True)
150- return CountableIterator(len(candidate_specs), candidate_specs)
151
152- @property
153- def _all_specs(self):
154- return self.context.target.specifications(
155- filter=[SpecificationFilter.ALL], prejoin_people=False)
156+ return Store.of(self.context).find(
157+ Specification,
158+ SQL('Specification.fti @@ ftq(%s)' % quote(query)),
159+ self._exclude_blocked_query(),
160+ ).order_by(self._order_by())
161
162 def __iter__(self):
163- return (
164- self.toTerm(spec) for spec in self._filter_specs(self._all_specs))
165+ # We don't ever want to iterate over everything.
166+ raise NotImplementedError()
167
168 def __contains__(self, obj):
169 return self._is_valid_candidate(obj)
170
171=== removed file 'lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt'
172--- lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt 2011-03-15 02:39:51 +0000
173+++ lib/lp/blueprints/vocabularies/tests/specificationdepcandidates.txt 1970-01-01 00:00:00 +0000
174@@ -1,97 +0,0 @@
175-SpecificationDepCandidatesVocabulary
176-====================================
177-
178-All blueprints that can be added as a dependency of the context
179-blueprint.
180-
181- >>> from zope.schema.vocabulary import getVocabularyRegistry
182- >>> vocabulary_registry = getVocabularyRegistry()
183-
184-First, we set up a product with three blueprints.
185-
186- >>> specced_product = factory.makeProduct()
187- >>> spec_a = factory.makeSpecification(
188- ... name='spec-a', summary='The first spec',
189- ... product=specced_product)
190- >>> spec_b = factory.makeSpecification(
191- ... name='spec-b', summary='The second spec',
192- ... product=specced_product)
193- >>> spec_c = factory.makeSpecification(
194- ... name='spec-c', summary='The third spec',
195- ... product=specced_product)
196- >>> sorted([spec.name for spec in specced_product.specifications()])
197- [u'spec-a', u'spec-b', u'spec-c']
198-
199-
200-Iterating over the vocabulary gives all blueprints for specced_product
201-except for spec_a itself.
202-
203- >>> vocab = vocabulary_registry.get(
204- ... spec_a, "SpecificationDepCandidates")
205- >>> sorted([term.value.name for term in vocab])
206- [u'spec-b', u'spec-c']
207-
208-Blueprints for other targets are considered to be 'in' the vocabulary
209-though.
210-
211- >>> unrelated_spec = factory.makeSpecification(
212- ... product=factory.makeProduct())
213- >>> vocab = vocabulary_registry.get(
214- ... spec_a, "SpecificationDepCandidates")
215- >>> unrelated_spec in vocab
216- True
217-
218-We mark spec_b as a dependency of spec_a and spec_c as a dependency of
219-spec_b.
220-
221- >>> spec_a.createDependency(spec_b)
222- <SpecificationDependency at ...>
223- >>> [spec.name for spec in spec_a.dependencies]
224- [u'spec-b']
225-
226- >>> spec_b.createDependency(spec_c)
227- <SpecificationDependency at ...>
228- >>> [spec.name for spec in spec_b.dependencies]
229- [u'spec-c']
230-
231-No circular dependencies - the vocabulary excludes specifications that
232-are a dependency of the context spec.
233-
234- >>> spec_a in set(spec_b.all_blocked)
235- True
236- >>> spec_b in set(spec_c.all_blocked)
237- True
238- >>> vocab = vocabulary_registry.get(
239- ... spec_c, "SpecificationDepCandidates")
240- >>> spec_a in [term.value for term in vocab]
241- False
242-
243-This vocabulary provides the IHugeVocabulary interface.
244-
245- >>> from canonical.launchpad.webapp.testing import verifyObject
246- >>> from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
247- >>> verifyObject(IHugeVocabulary, vocab)
248- True
249-
250-The search() method returns specifications within the vocabulary that
251-matches the search string. The string is matched against the name, or
252-fallbacks to a full text search.
253-
254- >>> from zope.security.proxy import removeSecurityProxy
255- >>> naked_vocab = removeSecurityProxy(
256- ... vocabulary_registry.get(
257- ... spec_a, "SpecificationDepCandidates"))
258- >>> list(naked_vocab.search('spec-b')) == [spec_b]
259- True
260- >>> list(naked_vocab.search('third')) == [spec_c]
261- True
262-
263-The search method uses the SQL `LIKE` operator, with the values quoted
264-appropriately. Queries conataining regual expression operators, for
265-example, will simply look for the respective characters within the
266-vocabulary's item (this used to be the cause of an OOPS, see
267-https://bugs.launchpad.net/blueprint/+bug/139385 for more
268-details).
269-
270- >>> list(naked_vocab.search('*'))
271- []
272
273=== removed file 'lib/lp/blueprints/vocabularies/tests/test_doc.py'
274--- lib/lp/blueprints/vocabularies/tests/test_doc.py 2010-08-25 05:17:20 +0000
275+++ lib/lp/blueprints/vocabularies/tests/test_doc.py 1970-01-01 00:00:00 +0000
276@@ -1,17 +0,0 @@
277-# Copyright 2010 Canonical Ltd. This software is licensed under the
278-# GNU Affero General Public License version 3 (see the file LICENSE).
279-
280-"""
281-Run the doctests.
282-"""
283-
284-import os
285-
286-from lp.services.testing import build_doctest_suite
287-
288-
289-here = os.path.dirname(os.path.realpath(__file__))
290-
291-
292-def test_suite():
293- return build_doctest_suite(here, '')
294
295=== modified file 'lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py'
296--- lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2010-10-04 19:50:45 +0000
297+++ lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2011-03-15 08:46:24 +0000
298@@ -176,3 +176,72 @@
299 vocab = self.getVocabularyForSpec(spec)
300 term = vocab.getTermByToken(canonical_url(candidate))
301 self.assertEqual(term.token, canonical_url(candidate))
302+
303+ def test_searchForTerms_ordering_different_targets_by_name(self):
304+ # If the searched for specs are on different targets, the ordering is
305+ # by name.
306+ spec = self.factory.makeSpecification()
307+ foo_b = self.factory.makeSpecification(name='foo-b')
308+ foo_a = self.factory.makeSpecification(name='foo-a')
309+ vocab = self.getVocabularyForSpec(spec)
310+ results = vocab.searchForTerms('foo')
311+ self.assertEqual(2, len(results))
312+ found = [item.value for item in results]
313+ self.assertEqual([foo_a, foo_b], found)
314+
315+ def test_searchForTerms_ordering_same_product_first(self):
316+ # Specs on the same product are returned first.
317+ widget = self.factory.makeProduct()
318+ spec = self.factory.makeSpecification(product=widget)
319+ foo_b = self.factory.makeSpecification(name='foo-b', product=widget)
320+ foo_a = self.factory.makeSpecification(name='foo-a')
321+ vocab = self.getVocabularyForSpec(spec)
322+ results = vocab.searchForTerms('foo')
323+ self.assertEqual(2, len(results))
324+ found = [item.value for item in results]
325+ self.assertEqual([foo_b, foo_a], found)
326+
327+ def test_searchForTerms_ordering_same_productseries_first(self):
328+ # Specs on the same product series are returned before the same
329+ # products, and those before others.
330+ widget = self.factory.makeProduct()
331+ spec = self.factory.makeSpecification(product=widget)
332+ spec.proposeGoal(widget.development_focus, widget.owner)
333+ foo_c = self.factory.makeSpecification(name='foo-c', product=widget)
334+ foo_c.proposeGoal(widget.development_focus, widget.owner)
335+ foo_b = self.factory.makeSpecification(name='foo-b', product=widget)
336+ foo_a = self.factory.makeSpecification(name='foo-a')
337+ vocab = self.getVocabularyForSpec(spec)
338+ results = vocab.searchForTerms('foo')
339+ self.assertEqual(3, len(results))
340+ found = [item.value for item in results]
341+ self.assertEqual([foo_c, foo_b, foo_a], found)
342+
343+ def test_searchForTerms_ordering_same_distribution_first(self):
344+ # Specs on the same distribution are returned first.
345+ mint = self.factory.makeDistribution()
346+ spec = self.factory.makeSpecification(distribution=mint)
347+ foo_b = self.factory.makeSpecification(name='foo-b', distribution=mint)
348+ foo_a = self.factory.makeSpecification(name='foo-a')
349+ vocab = self.getVocabularyForSpec(spec)
350+ results = vocab.searchForTerms('foo')
351+ self.assertEqual(2, len(results))
352+ found = [item.value for item in results]
353+ self.assertEqual([foo_b, foo_a], found)
354+
355+ def test_searchForTerms_ordering_same_distroseries_first(self):
356+ # Specs on the same distroseries are returned before the same
357+ # distribution, and those before others.
358+ mint = self.factory.makeDistribution()
359+ next = self.factory.makeDistroSeries(mint)
360+ spec = self.factory.makeSpecification(distribution=mint)
361+ spec.proposeGoal(next, mint.owner)
362+ foo_c = self.factory.makeSpecification(name='foo-c', distribution=mint)
363+ foo_c.proposeGoal(next, mint.owner)
364+ foo_b = self.factory.makeSpecification(name='foo-b', distribution=mint)
365+ foo_a = self.factory.makeSpecification(name='foo-a')
366+ vocab = self.getVocabularyForSpec(spec)
367+ results = vocab.searchForTerms('foo')
368+ self.assertEqual(3, len(results))
369+ found = [item.value for item in results]
370+ self.assertEqual([foo_c, foo_b, foo_a], found)