Launchpad itself

Merge lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary into lp:launchpad

  1. de-stupid-SpecificationDepCandidatesVocabulary
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11457
Proposed branch: lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary
Merge into: lp:launchpad
Diff against target: 323 lines (+136/-71)
7 files modified
lib/lp/blueprints/browser/specificationdependency.py (+24/-26)
lib/lp/blueprints/interfaces/specification.py (+1/-1)
lib/lp/blueprints/stories/blueprints/xx-dependencies.txt (+1/-1)
lib/lp/blueprints/stories/blueprints/xx-editing.txt (+3/-3)
lib/lp/blueprints/stories/blueprints/xx-superseding.txt (+4/-5)
lib/lp/blueprints/vocabularies/specificationdependency.py (+31/-35)
lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py (+72/-0)
To merge this branch: bzr merge lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+33728@code.launchpad.net

Commit message

Improve test coverage and implementation of SpecificationDepCandidatesVocabulary

Description of the change

Hi,

For linaro, I'm working on bug 3552. This branch consists of basic preparatory drive-by changes to the code that I'm going to be working on that needed to be made before I could look at it without vomiting.

In detail:

 * Use higher level form machinery in the +linkdependency form
 * remove a pointless getattr
 * change blueprint page tests to not be sequential (luckily this was pretty easy)
 * Add some direct tests for SpecificationDepCandidatesVocabulary.getTermByToken
 * Reimplement SpecificationDepCandidatesVocabulary.getTermByToken far more directly
 * Some other simple refactorings of SpecificationDepCandidatesVocabulary

Cheers,
mwh

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Looks good. I think that assertEquals is simpler and more easily understood than assertThat(..., Equals(...)).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/blueprints/browser/specificationdependency.py'
2--- lib/lp/blueprints/browser/specificationdependency.py 2010-08-20 20:31:18 +0000
3+++ lib/lp/blueprints/browser/specificationdependency.py 2010-08-26 22:39:59 +0000
4@@ -11,8 +11,9 @@
5 'SpecificationDependencyTreeView',
6 ]
7
8-from zope.formlib import form
9-from zope.schema import Choice
10+from zope.interface import Interface
11+
12+from lazr.restful.interface import copy_field
13
14 from canonical.launchpad import _
15 from canonical.launchpad.webapp import (
16@@ -27,34 +28,31 @@
17 )
18
19
20+class AddSpecificationDependencySchema(Interface):
21+
22+ dependency = copy_field(
23+ ISpecificationDependency['dependency'],
24+ readonly=False,
25+ description=_(
26+ "If another blueprint needs to be fully implemented "
27+ "before this feature can be started, then specify that "
28+ "dependency here so Launchpad knows about it and can "
29+ "give you an accurate project plan."))
30+
31+
32 class SpecificationDependencyAddView(LaunchpadFormView):
33- schema = ISpecificationDependency
34- field_names = ['dependency']
35+ schema = AddSpecificationDependencySchema
36 label = _('Depends On')
37
38- def setUpFields(self):
39- """Override the setup to define own fields."""
40- self.form_fields = form.Fields(
41- Choice(
42- __name__='dependency',
43- title=_(u'Depends On'),
44- vocabulary='SpecificationDepCandidates',
45- required=True,
46- description=_(
47- "If another blueprint needs to be fully implemented "
48- "before this feature can be started, then specify that "
49- "dependency here so Launchpad knows about it and can "
50- "give you an accurate project plan.")),
51- render_context=self.render_context)
52-
53 def validate(self, data):
54- is_valid = True
55- token = self.request.form.get(self.widgets['dependency'].name)
56- try:
57- self.widgets['dependency'].vocabulary.getTermByToken(token)
58- except LookupError:
59- is_valid = False
60- if not is_valid:
61+ """See `LaunchpadFormView.validate`.
62+
63+ Because it's too hard to set a good error message from inside the
64+ widget -- it will be the infamously inscrutable 'Invalid Value' -- we
65+ replace it here.
66+ """
67+ if self.getFieldError('dependency'):
68+ token = self.request.form.get(self.widgets['dependency'].name)
69 self.setFieldError(
70 'dependency',
71 'There is no blueprint named "%s" in %s, or '
72
73=== modified file 'lib/lp/blueprints/interfaces/specification.py'
74--- lib/lp/blueprints/interfaces/specification.py 2010-08-20 20:31:18 +0000
75+++ lib/lp/blueprints/interfaces/specification.py 2010-08-26 22:39:59 +0000
76@@ -554,7 +554,7 @@
77 def _validate(self, specurl):
78 TextLine._validate(self, specurl)
79 if (ISpecification.providedBy(self.context) and
80- specurl == getattr(self.context, 'specurl')):
81+ specurl == self.context.specurl):
82 # The specurl wasn't changed
83 return
84
85
86=== renamed file 'lib/lp/blueprints/stories/blueprints/02-buglinks.txt' => 'lib/lp/blueprints/stories/blueprints/xx-buglinks.txt'
87=== renamed file 'lib/lp/blueprints/stories/blueprints/01-creation.txt' => 'lib/lp/blueprints/stories/blueprints/xx-creation.txt'
88=== renamed file 'lib/lp/blueprints/stories/blueprints/06-dependencies.txt' => 'lib/lp/blueprints/stories/blueprints/xx-dependencies.txt'
89--- lib/lp/blueprints/stories/blueprints/06-dependencies.txt 2009-09-16 17:30:05 +0000
90+++ lib/lp/blueprints/stories/blueprints/xx-dependencies.txt 2010-08-26 22:39:59 +0000
91@@ -84,7 +84,7 @@
92 dependencies we could remove. The extension manager one, and "e4x".
93
94 >>> owner_browser.getControl('Dependency').displayOptions
95- ['Extension Manager System Upgrades', 'Support E4X in EcmaScript']
96+ ['Extension Manager Upgrades', 'Support E4X in EcmaScript']
97
98 Again, the page contains a link back to the blueprint in case we change
99 our mind.
100
101=== renamed file 'lib/lp/blueprints/stories/blueprints/10-distrorelease.txt' => 'lib/lp/blueprints/stories/blueprints/xx-distrorelease.txt'
102=== renamed file 'lib/lp/blueprints/stories/blueprints/04-editing.txt' => 'lib/lp/blueprints/stories/blueprints/xx-editing.txt'
103--- lib/lp/blueprints/stories/blueprints/04-editing.txt 2009-09-18 21:17:07 +0000
104+++ lib/lp/blueprints/stories/blueprints/xx-editing.txt 2010-08-26 22:39:59 +0000
105@@ -22,12 +22,12 @@
106
107 Launchpad won't let us use an URL already used in another blueprint.
108
109- >>> url = 'http://wiki.ubuntu.com/NetworkMagic'
110+ >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
111 >>> browser.getControl('Specification URL').value = url
112 >>> browser.getControl('Change').click()
113
114- >>> message = ('http://wiki.ubuntu.com/NetworkMagic is already registered '
115- ... 'by another blueprint')
116+ >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
117+ ... 'registered by another blueprint')
118 >>> message in browser.contents
119 True
120
121
122=== renamed file 'lib/lp/blueprints/stories/blueprints/07-milestones.txt' => 'lib/lp/blueprints/stories/blueprints/xx-milestones.txt'
123=== renamed file 'lib/lp/blueprints/stories/blueprints/14-non-ascii-imagemap.txt' => 'lib/lp/blueprints/stories/blueprints/xx-non-ascii-imagemap.txt'
124=== renamed file 'lib/lp/blueprints/stories/blueprints/08-productseries.txt' => 'lib/lp/blueprints/stories/blueprints/xx-productseries.txt'
125=== renamed file 'lib/lp/blueprints/stories/blueprints/05-reviews.txt' => 'lib/lp/blueprints/stories/blueprints/xx-reviews.txt'
126=== renamed file 'lib/lp/blueprints/stories/blueprints/15-superseding-within-projects.txt' => 'lib/lp/blueprints/stories/blueprints/xx-superseding-within-projects.txt'
127=== renamed file 'lib/lp/blueprints/stories/blueprints/13-superseding.txt' => 'lib/lp/blueprints/stories/blueprints/xx-superseding.txt'
128--- lib/lp/blueprints/stories/blueprints/13-superseding.txt 2009-09-16 17:30:05 +0000
129+++ lib/lp/blueprints/stories/blueprints/xx-superseding.txt 2010-08-26 22:39:59 +0000
130@@ -5,15 +5,14 @@
131
132 First, we need to be able to see this page, we will go in as Carlos, the
133 approver of the specification. Not only will we test the existence of the
134-page, we will also test that the spec is currently in the Drafting status.
135-NB: the sampledata has this spec as New, but other tests in this story
136-modify that to Drafting, so this test will not run standalone.
137+page, we will also test that the spec is currently in the New status (i.e.
138+not already superseded).
139
140 >>> browser.addHeader('Authorization', 'Basic carlos@canonical.com:test')
141 >>> browser.open(
142 ... 'http://blueprints.launchpad.dev/firefox/+spec/'
143 ... + 'extension-manager-upgrades')
144->>> 'Drafting' in browser.contents
145+>>> 'New' in browser.contents
146 True
147
148 Make sure Bug 4116 stays fixed
149@@ -31,7 +30,7 @@
150 The page contains a link back to the blueprint, in case we change our
151 mind.
152
153- >>> back_link = browser.getLink('Extension Manager System Upgrades')
154+ >>> back_link = browser.getLink('Extension Manager Upgrades')
155 >>> back_link.url
156 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
157 >>> browser.getLink('Cancel').url
158
159=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
160--- lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-25 21:34:32 +0000
161+++ lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-26 22:39:59 +0000
162@@ -55,14 +55,30 @@
163 spec.target == self.context.target
164 and spec not in self.context.all_blocked)]
165
166- def _doSearch(self, query):
167- """Return terms where query is in the text of name
168- or title, or matches the full text index.
169- """
170-
171+ def toTerm(self, obj):
172+ return SimpleTerm(obj, obj.name, obj.title)
173+
174+ def getTermByToken(self, token):
175+ """See `zope.schema.interfaces.IVocabularyTokenized`.
176+
177+ The tokens for specifications are just the name of the spec.
178+ """
179+ spec = self.context.target.getSpecification(token)
180+ if spec is not None:
181+ filtered = self._filter_specs([spec])
182+ if len(filtered) > 0:
183+ return self.toTerm(filtered[0])
184+ raise LookupError(token)
185+
186+ def search(self, query):
187+ """See `SQLObjectVocabularyBase.search`.
188+
189+ We find specs where query is in the text of name or title, or matches
190+ the full text index and then filter out ineligible specs using
191+ `_filter_specs`.
192+ """
193 if not query:
194- return []
195-
196+ return CountableIterator(0, [])
197 quoted_query = quote_like(query)
198 sql_query = ("""
199 (Specification.name LIKE %s OR
200@@ -71,38 +87,18 @@
201 """
202 % (quoted_query, quoted_query, quoted_query))
203 all_specs = Specification.select(sql_query, orderBy=self._orderBy)
204-
205- return self._filter_specs(all_specs)
206-
207- def toTerm(self, obj):
208- return SimpleTerm(obj, obj.name, obj.title)
209-
210- def getTermByToken(self, token):
211- search_results = self._doSearch(token)
212- for search_result in search_results:
213- if search_result.name == token:
214- return self.toTerm(search_result)
215- raise LookupError(token)
216-
217- def search(self, query):
218- candidate_specs = self._doSearch(query)
219- return CountableIterator(len(candidate_specs),
220- candidate_specs)
221-
222+ candidate_specs = self._filter_specs(all_specs)
223+ return CountableIterator(len(candidate_specs), candidate_specs)
224+
225+ @property
226 def _all_specs(self):
227- all_specs = self.context.target.specifications(
228+ return self.context.target.specifications(
229 filter=[SpecificationFilter.ALL],
230 prejoin_people=False)
231- return self._filter_specs(all_specs)
232
233 def __iter__(self):
234- return (self.toTerm(spec) for spec in self._all_specs())
235+ return (self.toTerm(spec)
236+ for spec in self._filter_specs(self._all_specs))
237
238 def __contains__(self, obj):
239- # We don't use self._all_specs here, since it will call
240- # self._filter_specs(all_specs) which will cause all the specs
241- # to be loaded, whereas obj in all_specs will query a single object.
242- all_specs = self.context.target.specifications(
243- filter=[SpecificationFilter.ALL],
244- prejoin_people=False)
245- return obj in all_specs and len(self._filter_specs([obj])) > 0
246+ return obj in self._all_specs and len(self._filter_specs([obj])) > 0
247
248=== added file 'lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py'
249--- lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 1970-01-01 00:00:00 +0000
250+++ lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2010-08-26 22:39:59 +0000
251@@ -0,0 +1,72 @@
252+# Copyright 2010 Canonical Ltd. This software is licensed under the
253+# GNU Affero General Public License version 3 (see the file LICENSE).
254+
255+"""Tests for `SpecificationDepCandidatesVocabulary`.
256+
257+There is also a doctest in specificationdepcandidates.txt.
258+"""
259+
260+__metaclass__ = type
261+
262+from zope.schema.vocabulary import getVocabularyRegistry
263+
264+from canonical.testing import DatabaseFunctionalLayer
265+
266+from lp.testing import TestCaseWithFactory
267+
268+
269+class TestSpecificationDepCandidatesVocabulary(TestCaseWithFactory):
270+ """Tests for `SpecificationDepCandidatesVocabulary`."""
271+
272+ layer = DatabaseFunctionalLayer
273+
274+ def getVocabularyForSpec(self, spec):
275+ return getVocabularyRegistry().get(
276+ spec, name='SpecificationDepCandidates')
277+
278+ def test_getTermByToken_product(self):
279+ # Calling getTermByToken for a dependency vocab for a spec from a
280+ # product with the name of an acceptable candidate spec returns the
281+ # term for the candidate
282+ product = self.factory.makeProduct()
283+ spec = self.factory.makeSpecification(product=product)
284+ candidate = self.factory.makeSpecification(product=product)
285+ vocab = self.getVocabularyForSpec(spec)
286+ self.assertEqual(
287+ candidate, vocab.getTermByToken(candidate.name).value)
288+
289+ def test_getTermByToken_distro(self):
290+ # Calling getTermByToken for a dependency vocab for a spec from a
291+ # distribution with the name of an acceptable candidate spec returns
292+ # the term for the candidate
293+ distro = self.factory.makeDistribution()
294+ spec = self.factory.makeSpecification(distribution=distro)
295+ candidate = self.factory.makeSpecification(distribution=distro)
296+ vocab = self.getVocabularyForSpec(spec)
297+ self.assertEqual(
298+ candidate, vocab.getTermByToken(candidate.name).value)
299+
300+ def test_getTermByToken_disallows_blocked(self):
301+ # getTermByToken with the name of an candidate spec that is blocked by
302+ # the vocab's context raises LookupError.
303+ product = self.factory.makeProduct()
304+ spec = self.factory.makeSpecification(product=product)
305+ candidate = self.factory.makeSpecification(product=product)
306+ candidate.createDependency(spec)
307+ vocab = self.getVocabularyForSpec(spec)
308+ self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)
309+
310+ def test_getTermByToken_disallows_context(self):
311+ # getTermByToken with the name of the vocab's context raises
312+ # LookupError.
313+ spec = self.factory.makeSpecification()
314+ vocab = self.getVocabularyForSpec(spec)
315+ self.assertRaises(LookupError, vocab.getTermByToken, spec.name)
316+
317+ def test_getTermByToken_disallows_spec_for_other_target(self):
318+ # getTermByToken with the name of a spec with a different target
319+ # raises LookupError.
320+ spec = self.factory.makeSpecification()
321+ candidate = self.factory.makeSpecification()
322+ vocab = self.getVocabularyForSpec(spec)
323+ self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)