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

[Michael Hudson-Doyle]

Improve test coverage and implementation of SpecificationDepCandidatesVocabulary

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

[Tim Penhey (thumper)]

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

review: Approve

=== 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 @@
     'SpecificationDependencyTreeView',
     ]
 
-from zope.formlib import form
-from zope.schema import Choice
+from zope.interface import Interface
+
+from lazr.restful.interface import copy_field
 
 from canonical.launchpad import _
 from canonical.launchpad.webapp import (
@@ -27,34 +28,31 @@
     )
 
 
+class AddSpecificationDependencySchema(Interface):
+
+    dependency = copy_field(
+        ISpecificationDependency['dependency'],
+        readonly=False,
+        description=_(
+            "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."))
+
+
 class SpecificationDependencyAddView(LaunchpadFormView):
-    schema = ISpecificationDependency
-    field_names = ['dependency']
+    schema = AddSpecificationDependencySchema
     label = _('Depends On')
 
-    def setUpFields(self):
-        """Override the setup to define own fields."""
-        self.form_fields = form.Fields(
-            Choice(
-                __name__='dependency',
-                title=_(u'Depends On'),
-                vocabulary='SpecificationDepCandidates',
-                required=True,
-                description=_(
-                    "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.")),
-            render_context=self.render_context)
-
     def validate(self, data):
-        is_valid = True
-        token = self.request.form.get(self.widgets['dependency'].name)
-        try:
-            self.widgets['dependency'].vocabulary.getTermByToken(token)
-        except LookupError:
-            is_valid = False
-        if not is_valid:
+        """See `LaunchpadFormView.validate`.
+
+        Because it's too hard to set a good error message from inside the
+        widget -- it will be the infamously inscrutable 'Invalid Value' -- we
+        replace it here.
+        """
+        if self.getFieldError('dependency'):
+            token = self.request.form.get(self.widgets['dependency'].name)
             self.setFieldError(
                 'dependency',
                 'There is no blueprint named "%s" in %s, or '

=== 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 @@
     def _validate(self, specurl):
         TextLine._validate(self, specurl)
         if (ISpecification.providedBy(self.context) and
-            specurl == getattr(self.context, 'specurl')):
+            specurl == self.context.specurl):
             # The specurl wasn't changed
             return
 

=== 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 @@
 dependencies we could remove. The extension manager one, and "e4x".
 
   >>> owner_browser.getControl('Dependency').displayOptions
-  ['Extension Manager System Upgrades', 'Support E4X in EcmaScript']
+  ['Extension Manager Upgrades', 'Support E4X in EcmaScript']
 
 Again, the page contains a link back to the blueprint in case we change
 our mind.

=== 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 @@
 
 Launchpad won't let us use an URL already used in another blueprint.
 
-  >>> url = 'http://wiki.ubuntu.com/NetworkMagic'
+  >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
   >>> browser.getControl('Specification URL').value = url
   >>> browser.getControl('Change').click()
 
-  >>> message = ('http://wiki.ubuntu.com/NetworkMagic is already registered '
-  ...            'by another blueprint')
+  >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
+  ...            'registered by another blueprint')
   >>> message in browser.contents
   True
 

=== 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 @@
 
 First, we need to be able to see this page, we will go in as Carlos, the
 approver of the specification. Not only will we test the existence of the
-page, we will also test that the spec is currently in the Drafting status.
-NB: the sampledata has this spec as New, but other tests in this story
-modify that to Drafting, so this test will not run standalone.
+page, we will also test that the spec is currently in the New status (i.e.
+not already superseded).
 
 >>> browser.addHeader('Authorization', 'Basic [e-mail]:test')
 >>> browser.open(
 ...     'http://blueprints.launchpad.dev/firefox/+spec/'
 ...     + 'extension-manager-upgrades')
->>> 'Drafting' in browser.contents
+>>> 'New' in browser.contents
 True
 
 Make sure Bug 4116 stays fixed
@@ -31,7 +30,7 @@
 The page contains a link back to the blueprint, in case we change our
 mind.
 
-    >>> back_link = browser.getLink('Extension Manager System Upgrades')
+    >>> back_link = browser.getLink('Extension Manager Upgrades')
     >>> back_link.url
     'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
     >>> browser.getLink('Cancel').url

=== 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 @@
                     spec.target == self.context.target
                     and spec not in self.context.all_blocked)]
 
-    def _doSearch(self, query):
-        """Return terms where query is in the text of name
-        or title, or matches the full text index.
-        """
-
+    def toTerm(self, obj):
+        return SimpleTerm(obj, obj.name, obj.title)
+
+    def getTermByToken(self, token):
+        """See `zope.schema.interfaces.IVocabularyTokenized`.
+
+        The tokens for specifications are just the name of the 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])
+        raise LookupError(token)
+
+    def search(self, query):
+        """See `SQLObjectVocabularyBase.search`.
+
+        We find specs where query is in the text of name or title, or matches
+        the full text index and then filter out ineligible specs using
+        `_filter_specs`.
+        """
         if not query:
-            return []
-
+            return CountableIterator(0, [])
         quoted_query = quote_like(query)
         sql_query = ("""
             (Specification.name LIKE %s OR
@@ -71,38 +87,18 @@
             """
             % (quoted_query, quoted_query, quoted_query))
         all_specs = Specification.select(sql_query, orderBy=self._orderBy)
-
-        return self._filter_specs(all_specs)
-
-    def toTerm(self, obj):
-        return SimpleTerm(obj, obj.name, obj.title)
-
-    def getTermByToken(self, token):
-        search_results = self._doSearch(token)
-        for search_result in search_results:
-            if search_result.name == token:
-                return self.toTerm(search_result)
-        raise LookupError(token)
-
-    def search(self, query):
-        candidate_specs = self._doSearch(query)
-        return CountableIterator(len(candidate_specs),
-                                 candidate_specs)
-
+        candidate_specs = self._filter_specs(all_specs)
+        return CountableIterator(len(candidate_specs), candidate_specs)
+
+    @property
     def _all_specs(self):
-        all_specs = self.context.target.specifications(
+        return self.context.target.specifications(
             filter=[SpecificationFilter.ALL],
             prejoin_people=False)
-        return self._filter_specs(all_specs)
 
     def __iter__(self):
-        return (self.toTerm(spec) for spec in self._all_specs())
+        return (self.toTerm(spec)
+                for spec in self._filter_specs(self._all_specs))
 
     def __contains__(self, obj):
-        # We don't use self._all_specs here, since it will call
-        # self._filter_specs(all_specs) which will cause all the specs
-        # to be loaded, whereas obj in all_specs will query a single object.
-        all_specs = self.context.target.specifications(
-            filter=[SpecificationFilter.ALL],
-            prejoin_people=False)
-        return obj in all_specs and len(self._filter_specs([obj])) > 0
+        return obj in self._all_specs and len(self._filter_specs([obj])) > 0

=== 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 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `SpecificationDepCandidatesVocabulary`.
+
+There is also a doctest in specificationdepcandidates.txt.
+"""
+
+__metaclass__ = type
+
+from zope.schema.vocabulary import getVocabularyRegistry
+
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.testing import TestCaseWithFactory
+
+
+class TestSpecificationDepCandidatesVocabulary(TestCaseWithFactory):
+    """Tests for `SpecificationDepCandidatesVocabulary`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def getVocabularyForSpec(self, spec):
+        return getVocabularyRegistry().get(
+            spec, name='SpecificationDepCandidates')
+
+    def test_getTermByToken_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
+        product = self.factory.makeProduct()
+        spec = self.factory.makeSpecification(product=product)
+        candidate = self.factory.makeSpecification(product=product)
+        vocab = self.getVocabularyForSpec(spec)
+        self.assertEqual(
+            candidate, vocab.getTermByToken(candidate.name).value)
+
+    def test_getTermByToken_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
+        distro = self.factory.makeDistribution()
+        spec = self.factory.makeSpecification(distribution=distro)
+        candidate = self.factory.makeSpecification(distribution=distro)
+        vocab = self.getVocabularyForSpec(spec)
+        self.assertEqual(
+            candidate, vocab.getTermByToken(candidate.name).value)
+
+    def test_getTermByToken_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()
+        spec = self.factory.makeSpecification(product=product)
+        candidate = self.factory.makeSpecification(product=product)
+        candidate.createDependency(spec)
+        vocab = self.getVocabularyForSpec(spec)
+        self.assertRaises(LookupError, vocab.getTermByToken, candidate.name)
+
+    def test_getTermByToken_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):
+        # 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)