launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00766
lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary into lp:launchpad/devel
Michael Hudson has proposed merging lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
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
--
https://code.launchpad.net/~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary/+merge/33728
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/de-stupid-SpecificationDepCandidatesVocabulary into lp:launchpad/devel.
=== 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 03:44:44 +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 03:44:44 +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 03:44:44 +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 03:44:44 +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 03:44:44 +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 carlos@xxxxxxxxxxxxx: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 03:44:44 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
# Copyright 2010 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
@@ -106,3 +107,109 @@
filter=[SpecificationFilter.ALL],
prejoin_people=False)
return obj in all_specs and len(self._filter_specs([obj])) > 0
+=======
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""The vocabularies relating to dependencies of specifications."""
+
+__metaclass__ = type
+__all__ = ['SpecificationDepCandidatesVocabulary']
+
+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.launchpad.webapp.vocabulary import (
+ CountableIterator,
+ IHugeVocabulary,
+ SQLObjectVocabularyBase,
+ )
+
+from lp.blueprints.interfaces.specification import SpecificationFilter
+from lp.blueprints.model.specification import Specification
+
+
+class SpecificationDepCandidatesVocabulary(SQLObjectVocabularyBase):
+ """Specifications that could be dependencies of this spec.
+
+ This includes only those specs that are not blocked by this spec
+ (directly or indirectly), unless they are already dependencies.
+
+ The current spec is not included.
+ """
+
+ implements(IHugeVocabulary)
+
+ _table = Specification
+ _orderBy = 'name'
+ displayname = 'Select a blueprint'
+
+ def _filter_specs(self, specs):
+ """Filter `specs` to remove invalid candidates.
+
+ Invalid candidates are:
+
+ * The spec that we're adding a depdency to,
+ * Specs for a different target, and
+ * Specs that depend on this one.
+
+ Preventing the last category prevents loops in the dependency graph.
+ """
+ # XXX intellectronica 2007-07-05: is 100 a reasonable count before
+ # starting to warn?
+ speclist = shortlist(specs, 100)
+ return [spec for spec in speclist
+ if (spec != self.context and
+ spec.target == self.context.target
+ and spec not in self.context.all_blocked)]
+
+ 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 CountableIterator(0, [])
+ 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)
+ return CountableIterator(len(candidate_specs), candidate_specs)
+
+ @property
+ def _all_specs(self):
+ return self.context.target.specifications(
+ filter=[SpecificationFilter.ALL],
+ prejoin_people=False)
+
+ def __iter__(self):
+ return (self.toTerm(spec)
+ for spec in self._filter_specs(self._all_specs))
+
+ def __contains__(self, obj):
+ return obj in self._all_specs and len(self._filter_specs([obj])) > 0
+>>>>>>> MERGE-SOURCE
=== 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 03:44:44 +0000
@@ -0,0 +1,74 @@
+# 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 testtools.matchers import Equals
+
+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.assertThat(
+ vocab.getTermByToken(candidate.name).value, Equals(candidate))
+
+ 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.assertThat(
+ vocab.getTermByToken(candidate.name).value, Equals(candidate))
+
+ 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)
Follow ups
