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
=== modified file 'lib/lp/blueprints/browser/specificationdependency.py'
--- lib/lp/blueprints/browser/specificationdependency.py 2010-08-20 20:31:18 +0000
+++ lib/lp/blueprints/browser/specificationdependency.py 2010-08-26 22:39:59 +0000
@@ -11,8 +11,9 @@
11 'SpecificationDependencyTreeView',11 'SpecificationDependencyTreeView',
12 ]12 ]
1313
14from zope.formlib import form14from zope.interface import Interface
15from zope.schema import Choice15
16from lazr.restful.interface import copy_field
1617
17from canonical.launchpad import _18from canonical.launchpad import _
18from canonical.launchpad.webapp import (19from canonical.launchpad.webapp import (
@@ -27,34 +28,31 @@
27 )28 )
2829
2930
31class AddSpecificationDependencySchema(Interface):
32
33 dependency = copy_field(
34 ISpecificationDependency['dependency'],
35 readonly=False,
36 description=_(
37 "If another blueprint needs to be fully implemented "
38 "before this feature can be started, then specify that "
39 "dependency here so Launchpad knows about it and can "
40 "give you an accurate project plan."))
41
42
30class SpecificationDependencyAddView(LaunchpadFormView):43class SpecificationDependencyAddView(LaunchpadFormView):
31 schema = ISpecificationDependency44 schema = AddSpecificationDependencySchema
32 field_names = ['dependency']
33 label = _('Depends On')45 label = _('Depends On')
3446
35 def setUpFields(self):
36 """Override the setup to define own fields."""
37 self.form_fields = form.Fields(
38 Choice(
39 __name__='dependency',
40 title=_(u'Depends On'),
41 vocabulary='SpecificationDepCandidates',
42 required=True,
43 description=_(
44 "If another blueprint needs to be fully implemented "
45 "before this feature can be started, then specify that "
46 "dependency here so Launchpad knows about it and can "
47 "give you an accurate project plan.")),
48 render_context=self.render_context)
49
50 def validate(self, data):47 def validate(self, data):
51 is_valid = True48 """See `LaunchpadFormView.validate`.
52 token = self.request.form.get(self.widgets['dependency'].name)49
53 try:50 Because it's too hard to set a good error message from inside the
54 self.widgets['dependency'].vocabulary.getTermByToken(token)51 widget -- it will be the infamously inscrutable 'Invalid Value' -- we
55 except LookupError:52 replace it here.
56 is_valid = False53 """
57 if not is_valid:54 if self.getFieldError('dependency'):
55 token = self.request.form.get(self.widgets['dependency'].name)
58 self.setFieldError(56 self.setFieldError(
59 'dependency',57 'dependency',
60 'There is no blueprint named "%s" in %s, or '58 'There is no blueprint named "%s" in %s, or '
6159
=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py 2010-08-20 20:31:18 +0000
+++ lib/lp/blueprints/interfaces/specification.py 2010-08-26 22:39:59 +0000
@@ -554,7 +554,7 @@
554 def _validate(self, specurl):554 def _validate(self, specurl):
555 TextLine._validate(self, specurl)555 TextLine._validate(self, specurl)
556 if (ISpecification.providedBy(self.context) and556 if (ISpecification.providedBy(self.context) and
557 specurl == getattr(self.context, 'specurl')):557 specurl == self.context.specurl):
558 # The specurl wasn't changed558 # The specurl wasn't changed
559 return559 return
560560
561561
=== renamed file 'lib/lp/blueprints/stories/blueprints/02-buglinks.txt' => 'lib/lp/blueprints/stories/blueprints/xx-buglinks.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/01-creation.txt' => 'lib/lp/blueprints/stories/blueprints/xx-creation.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/06-dependencies.txt' => 'lib/lp/blueprints/stories/blueprints/xx-dependencies.txt'
--- lib/lp/blueprints/stories/blueprints/06-dependencies.txt 2009-09-16 17:30:05 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-dependencies.txt 2010-08-26 22:39:59 +0000
@@ -84,7 +84,7 @@
84dependencies we could remove. The extension manager one, and "e4x".84dependencies we could remove. The extension manager one, and "e4x".
8585
86 >>> owner_browser.getControl('Dependency').displayOptions86 >>> owner_browser.getControl('Dependency').displayOptions
87 ['Extension Manager System Upgrades', 'Support E4X in EcmaScript']87 ['Extension Manager Upgrades', 'Support E4X in EcmaScript']
8888
89Again, the page contains a link back to the blueprint in case we change89Again, the page contains a link back to the blueprint in case we change
90our mind.90our mind.
9191
=== renamed file 'lib/lp/blueprints/stories/blueprints/10-distrorelease.txt' => 'lib/lp/blueprints/stories/blueprints/xx-distrorelease.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/04-editing.txt' => 'lib/lp/blueprints/stories/blueprints/xx-editing.txt'
--- lib/lp/blueprints/stories/blueprints/04-editing.txt 2009-09-18 21:17:07 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-editing.txt 2010-08-26 22:39:59 +0000
@@ -22,12 +22,12 @@
2222
23Launchpad won't let us use an URL already used in another blueprint.23Launchpad won't let us use an URL already used in another blueprint.
2424
25 >>> url = 'http://wiki.ubuntu.com/NetworkMagic'25 >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
26 >>> browser.getControl('Specification URL').value = url26 >>> browser.getControl('Specification URL').value = url
27 >>> browser.getControl('Change').click()27 >>> browser.getControl('Change').click()
2828
29 >>> message = ('http://wiki.ubuntu.com/NetworkMagic is already registered '29 >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
30 ... 'by another blueprint')30 ... 'registered by another blueprint')
31 >>> message in browser.contents31 >>> message in browser.contents
32 True32 True
3333
3434
=== renamed file 'lib/lp/blueprints/stories/blueprints/07-milestones.txt' => 'lib/lp/blueprints/stories/blueprints/xx-milestones.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/14-non-ascii-imagemap.txt' => 'lib/lp/blueprints/stories/blueprints/xx-non-ascii-imagemap.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/08-productseries.txt' => 'lib/lp/blueprints/stories/blueprints/xx-productseries.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/05-reviews.txt' => 'lib/lp/blueprints/stories/blueprints/xx-reviews.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/15-superseding-within-projects.txt' => 'lib/lp/blueprints/stories/blueprints/xx-superseding-within-projects.txt'
=== renamed file 'lib/lp/blueprints/stories/blueprints/13-superseding.txt' => 'lib/lp/blueprints/stories/blueprints/xx-superseding.txt'
--- lib/lp/blueprints/stories/blueprints/13-superseding.txt 2009-09-16 17:30:05 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-superseding.txt 2010-08-26 22:39:59 +0000
@@ -5,15 +5,14 @@
55
6First, we need to be able to see this page, we will go in as Carlos, the6First, we need to be able to see this page, we will go in as Carlos, the
7approver of the specification. Not only will we test the existence of the7approver of the specification. Not only will we test the existence of the
8page, we will also test that the spec is currently in the Drafting status.8page, we will also test that the spec is currently in the New status (i.e.
9NB: the sampledata has this spec as New, but other tests in this story9not already superseded).
10modify that to Drafting, so this test will not run standalone.
1110
12>>> browser.addHeader('Authorization', 'Basic carlos@canonical.com:test')11>>> browser.addHeader('Authorization', 'Basic carlos@canonical.com:test')
13>>> browser.open(12>>> browser.open(
14... 'http://blueprints.launchpad.dev/firefox/+spec/'13... 'http://blueprints.launchpad.dev/firefox/+spec/'
15... + 'extension-manager-upgrades')14... + 'extension-manager-upgrades')
16>>> 'Drafting' in browser.contents15>>> 'New' in browser.contents
17True16True
1817
19Make sure Bug 4116 stays fixed18Make sure Bug 4116 stays fixed
@@ -31,7 +30,7 @@
31The page contains a link back to the blueprint, in case we change our30The page contains a link back to the blueprint, in case we change our
32mind.31mind.
3332
34 >>> back_link = browser.getLink('Extension Manager System Upgrades')33 >>> back_link = browser.getLink('Extension Manager Upgrades')
35 >>> back_link.url34 >>> back_link.url
36 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'35 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
37 >>> browser.getLink('Cancel').url36 >>> browser.getLink('Cancel').url
3837
=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
--- lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-25 21:34:32 +0000
+++ lib/lp/blueprints/vocabularies/specificationdependency.py 2010-08-26 22:39:59 +0000
@@ -55,14 +55,30 @@
55 spec.target == self.context.target55 spec.target == self.context.target
56 and spec not in self.context.all_blocked)]56 and spec not in self.context.all_blocked)]
5757
58 def _doSearch(self, query):58 def toTerm(self, obj):
59 """Return terms where query is in the text of name59 return SimpleTerm(obj, obj.name, obj.title)
60 or title, or matches the full text index.60
61 """61 def getTermByToken(self, token):
6262 """See `zope.schema.interfaces.IVocabularyTokenized`.
63
64 The tokens for specifications are just the name of the spec.
65 """
66 spec = self.context.target.getSpecification(token)
67 if spec is not None:
68 filtered = self._filter_specs([spec])
69 if len(filtered) > 0:
70 return self.toTerm(filtered[0])
71 raise LookupError(token)
72
73 def search(self, query):
74 """See `SQLObjectVocabularyBase.search`.
75
76 We find specs where query is in the text of name or title, or matches
77 the full text index and then filter out ineligible specs using
78 `_filter_specs`.
79 """
63 if not query:80 if not query:
64 return []81 return CountableIterator(0, [])
65
66 quoted_query = quote_like(query)82 quoted_query = quote_like(query)
67 sql_query = ("""83 sql_query = ("""
68 (Specification.name LIKE %s OR84 (Specification.name LIKE %s OR
@@ -71,38 +87,18 @@
71 """87 """
72 % (quoted_query, quoted_query, quoted_query))88 % (quoted_query, quoted_query, quoted_query))
73 all_specs = Specification.select(sql_query, orderBy=self._orderBy)89 all_specs = Specification.select(sql_query, orderBy=self._orderBy)
7490 candidate_specs = self._filter_specs(all_specs)
75 return self._filter_specs(all_specs)91 return CountableIterator(len(candidate_specs), candidate_specs)
7692
77 def toTerm(self, obj):93 @property
78 return SimpleTerm(obj, obj.name, obj.title)
79
80 def getTermByToken(self, token):
81 search_results = self._doSearch(token)
82 for search_result in search_results:
83 if search_result.name == token:
84 return self.toTerm(search_result)
85 raise LookupError(token)
86
87 def search(self, query):
88 candidate_specs = self._doSearch(query)
89 return CountableIterator(len(candidate_specs),
90 candidate_specs)
91
92 def _all_specs(self):94 def _all_specs(self):
93 all_specs = self.context.target.specifications(95 return self.context.target.specifications(
94 filter=[SpecificationFilter.ALL],96 filter=[SpecificationFilter.ALL],
95 prejoin_people=False)97 prejoin_people=False)
96 return self._filter_specs(all_specs)
9798
98 def __iter__(self):99 def __iter__(self):
99 return (self.toTerm(spec) for spec in self._all_specs())100 return (self.toTerm(spec)
101 for spec in self._filter_specs(self._all_specs))
100102
101 def __contains__(self, obj):103 def __contains__(self, obj):
102 # We don't use self._all_specs here, since it will call104 return obj in self._all_specs and len(self._filter_specs([obj])) > 0
103 # self._filter_specs(all_specs) which will cause all the specs
104 # to be loaded, whereas obj in all_specs will query a single object.
105 all_specs = self.context.target.specifications(
106 filter=[SpecificationFilter.ALL],
107 prejoin_people=False)
108 return obj in all_specs and len(self._filter_specs([obj])) > 0
109105
=== added file 'lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py'
--- lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/vocabularies/tests/test_specificationdependency.py 2010-08-26 22:39:59 +0000
@@ -0,0 +1,72 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for `SpecificationDepCandidatesVocabulary`.
5
6There is also a doctest in specificationdepcandidates.txt.
7"""
8
9__metaclass__ = type
10
11from zope.schema.vocabulary import getVocabularyRegistry
12
13from canonical.testing import DatabaseFunctionalLayer
14
15from lp.testing import TestCaseWithFactory
16
17
18class TestSpecificationDepCandidatesVocabulary(TestCaseWithFactory):
19 """Tests for `SpecificationDepCandidatesVocabulary`."""
20
21 layer = DatabaseFunctionalLayer
22
23 def getVocabularyForSpec(self, spec):
24 return getVocabularyRegistry().get(
25 spec, name='SpecificationDepCandidates')
26
27 def test_getTermByToken_product(self):
28 # Calling getTermByToken for a dependency vocab for a spec from a
29 # product with the name of an acceptable candidate spec returns the
30 # term for the candidate
31 product = self.factory.makeProduct()
32 spec = self.factory.makeSpecification(product=product)
33 candidate = self.factory.makeSpecification(product=product)
34 vocab = self.getVocabularyForSpec(spec)
35 self.assertEqual(
36 candidate, vocab.getTermByToken(candidate.name).value)
37
38 def test_getTermByToken_distro(self):
39 # Calling getTermByToken for a dependency vocab for a spec from a
40 # distribution with the name of an acceptable candidate spec returns
41 # the term for the candidate
42 distro = self.factory.makeDistribution()
43 spec = self.factory.makeSpecification(distribution=distro)
44 candidate = self.factory.makeSpecification(distribution=distro)
45 vocab = self.getVocabularyForSpec(spec)
46 self.assertEqual(
47 candidate, vocab.getTermByToken(candidate.name).value)
48
49 def test_getTermByToken_disallows_blocked(self):
50 # getTermByToken with the name of an candidate spec that is blocked by
51 # the vocab's context raises LookupError.
52 product = self.factory.makeProduct()
53 spec = self.factory.makeSpecification(product=product)
54 candidate = self.factory.makeSpecification(product=product)
55 candidate.createDependency(spec)
56 vocab = self.getVocabularyForSpec(spec)
57 self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)
58
59 def test_getTermByToken_disallows_context(self):
60 # getTermByToken with the name of the vocab's context raises
61 # LookupError.
62 spec = self.factory.makeSpecification()
63 vocab = self.getVocabularyForSpec(spec)
64 self.assertRaises(LookupError, vocab.getTermByToken, spec.name)
65
66 def test_getTermByToken_disallows_spec_for_other_target(self):
67 # getTermByToken with the name of a spec with a different target
68 # raises LookupError.
69 spec = self.factory.makeSpecification()
70 candidate = self.factory.makeSpecification()
71 vocab = self.getVocabularyForSpec(spec)
72 self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)