launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/sensible-superseded-by into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #394051 in Launchpad itself: "Difficult to supersede a blueprint"
  https://bugs.launchpad.net/launchpad/+bug/394051
  Bug #1071704 in Launchpad itself: ""Mark superseded" (+supersede) always times out for Ubuntu"
  https://bugs.launchpad.net/launchpad/+bug/1071704

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/sensible-superseded-by/+merge/132009

Rip out the Blueprint superseded_by dropdown and burn it. Replace it with a text box that allows the user to type the name of the blueprint. Sprinkle in some infrastructure into ISpecificationSet to help.

I have performed some clean up to squeak into LoC net negative, as well as reformatting a doctest to silence lint.
-- 
https://code.launchpad.net/~stevenk/launchpad/sensible-superseded-by/+merge/132009
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/sensible-superseded-by into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2012-10-12 17:16:08 +0000
+++ lib/lp/blueprints/browser/specification.py	2012-10-30 04:32:19 +0000
@@ -61,7 +61,6 @@
     TextAreaWidget,
     TextWidget,
     )
-from zope.app.form.browser.itemswidgets import DropdownWidget
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.event import notify
@@ -75,10 +74,7 @@
 from zope.schema import (
     Bool,
     Choice,
-    )
-from zope.schema.vocabulary import (
-    SimpleTerm,
-    SimpleVocabulary,
+    TextLine,
     )
 
 from lp import _
@@ -1039,58 +1035,52 @@
         return canonical_url(self.context)
 
 
-class SupersededByWidget(DropdownWidget):
-    """Custom select widget for specification superseding.
-
-    This is just a standard DropdownWidget with the (no value) text
-    rendered as something meaningful to the user, as per Bug #4116.
-
-    TODO: This should be replaced with something more scalable as there
-    is no upper limit to the number of specifications.
-    -- StuartBishop 20060704
-    """
-    _messageNoValue = _("(Not Superseded)")
-
-
 class SpecificationSupersedingView(LaunchpadFormView):
     schema = ISpecification
     field_names = ['superseded_by']
     label = _('Mark blueprint superseded')
-    custom_widget('superseded_by', SupersededByWidget)
 
     @property
     def initial_values(self):
-        return {
-            'superseded_by': self.context.superseded_by,
-            }
+        return {'superseded_by': self.context.superseded_by}
 
     def setUpFields(self):
         """Override the setup to define own fields."""
-        if self.context.target is None:
-            raise AssertionError("No target found for this spec.")
-        specs = sorted(self.context.target.specifications(self.user),
-                       key=attrgetter('name'))
-        terms = [SimpleTerm(spec, spec.name, spec.title)
-                 for spec in specs if spec != self.context]
-
         self.form_fields = form.Fields(
-            Choice(
+            TextLine(
                 __name__='superseded_by',
                 title=_("Superseded by"),
-                vocabulary=SimpleVocabulary(terms),
                 required=False,
                 description=_(
                     "The blueprint which supersedes this one. Note "
-                    "that selecting a blueprint here and pressing "
+                    "that entering a blueprint here and pressing "
                     "Continue will change the blueprint status "
                     "to Superseded.")),
             render_context=self.render_context)
 
+    def _fetchSpecification(self, name):
+        pillar = self.context.target
+        if '/' in name:
+            pillar, name = name.split('/')
+        return getUtility(ISpecificationSet).getByName(pillar, name)
+
+    def validate(self, data):
+        """See `LaunchpadFormView`.`"""
+        super(SpecificationSupersedingView, self).validate(data)
+        if data['superseded_by']:
+            if not self._fetchSpecification(data['superseded_by']):
+                self.setFieldError(
+                    'superseded_by',
+                    "No blueprint named '%s'." % data['superseded_by'])
+
     @action(_('Continue'), name='supersede')
     def supersede_action(self, action, data):
         # Store some shorter names to avoid line-wrapping.
         SUPERSEDED = SpecificationDefinitionStatus.SUPERSEDED
         NEW = SpecificationDefinitionStatus.NEW
+        if data['superseded_by'] is not None:
+            data['superseded_by'] = self._fetchSpecification(
+                data['superseded_by'])
         self.context.superseded_by = data['superseded_by']
         # XXX: salgado, 2010-11-24, bug=680880: This logic should be in model
         # code.

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2012-09-27 14:30:40 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2012-10-30 04:32:19 +0000
@@ -1,8 +1,6 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0211,E0213
-
 """Specification interfaces."""
 
 __metaclass__ = type
@@ -693,6 +691,10 @@
     def getByURL(url):
         """Return the specification with the given url."""
 
+    def getByName(pillar, name):
+        """Return the specification with the given name for the given pillar.
+        """
+
     def new(name, title, specurl, summary, definition_status,
         owner, approver=None, product=None, distribution=None, assignee=None,
         drafter=None, whiteboard=None,

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-10-22 20:04:30 +0000
+++ lib/lp/blueprints/model/specification.py	2012-10-30 04:32:19 +0000
@@ -1,8 +1,6 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212
-
 __metaclass__ = type
 __all__ = [
     'get_specification_filters',
@@ -108,6 +106,7 @@
     sqlvalues,
     )
 from lp.services.database.stormexpr import fti_search
+from lp.services.database.lpstorm import IStore
 from lp.services.mail.helpers import get_contact_email_addresses
 from lp.services.propertycache import (
     cachedproperty,
@@ -1001,10 +1000,7 @@
                     index += 1
                     decorator(person, column)
 
-        results = Store.of(self).find(
-            Specification,
-            *clauses
-            )
+        results = Store.of(self).find(Specification, *clauses)
         return DecoratedResultSet(results, pre_iter_hook=cache_people)
 
     @property
@@ -1021,8 +1017,8 @@
 
     def specificationCount(self, user):
         """See IHasSpecifications."""
-        return self.specifications(user,
-                                   filter=[SpecificationFilter.ALL]).count()
+        return self.specifications(
+            user, filter=[SpecificationFilter.ALL]).count()
 
 
 class SpecificationSet(HasSpecificationsMixin):
@@ -1160,10 +1156,19 @@
 
     def getByURL(self, url):
         """See ISpecificationSet."""
-        specification = Specification.selectOneBy(specurl=url)
-        if specification is None:
-            return None
-        return specification
+        return Specification.selectOneBy(specurl=url)
+
+    def getByName(self, pillar, name):
+        """See ISpecificationSet."""
+        clauses = [Specification.name == name]
+        if IDistribution.providedBy(pillar):
+            clauses.append(Specification.distributionID == pillar.id)
+        elif IProduct.providedBy(pillar):
+            clauses.append(Specification.productID == pillar.id)
+        result = IStore(Specification).find(Specification, *clauses)
+        if result is None:
+            return result
+        return result.one()
 
     @property
     def coming_sprints(self):

=== modified file 'lib/lp/blueprints/stories/blueprints/xx-superseding-within-projects.txt'
--- lib/lp/blueprints/stories/blueprints/xx-superseding-within-projects.txt	2010-08-26 02:30:06 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-superseding-within-projects.txt	2012-10-30 04:32:19 +0000
@@ -1,5 +1,5 @@
-
-== Superseeding blueprints ==
+Superseeding blueprints
+=======================
 
 Bueprints can only be superseded by other blueprints in the same project.
 The user interface code for the superseding form passes blueprints by their
@@ -37,7 +37,7 @@
 
     >>> browser.open('http://blueprints.launchpad.dev/redfish/' +
     ...     '+spec/another-unique-blueprint/+supersede')
-    >>> browser.getControl('Superseded by').displayValue = ['A unique Blueprint']
+    >>> browser.getControl('Superseded by').value = 'a-unique-blueprint'
     >>> browser.getControl('Continue').click()
 
     >>> 'This blueprint has been superseded' in browser.contents

=== modified file 'lib/lp/blueprints/stories/blueprints/xx-superseding.txt'
--- lib/lp/blueprints/stories/blueprints/xx-superseding.txt	2010-08-26 02:30:06 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-superseding.txt	2012-10-30 04:32:19 +0000
@@ -8,24 +8,19 @@
 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')
->>> 'New' in browser.contents
-True
+    >>> browser.addHeader('Authorization', 'Basic carlos@xxxxxxxxxxxxx:test')
+    >>> browser.open(
+    ...     'http://blueprints.launchpad.dev/firefox/+spec/'
+    ...     + 'extension-manager-upgrades')
+    >>> 'New' in browser.contents
+    True
 
 Make sure Bug 4116 stays fixed
 
->>> browser.open(
-...     'http://blueprints.launchpad.dev/firefox/+spec/'
-...     + 'extension-manager-upgrades/+supersede'
-...     )
->>> superseded_control = browser.getControl('Superseded by')
->>> superseded_control.options[0]
-''
->>> superseded_control.displayOptions[0]
-'(Not Superseded)'
+    >>> browser.open(
+    ...     'http://blueprints.launchpad.dev/firefox/+spec/'
+    ...     + 'extension-manager-upgrades/+supersede'
+    ...     )
 
 The page contains a link back to the blueprint, in case we change our
 mind.
@@ -38,27 +33,26 @@
 
 Next, we will POST to that form, setting the spec which supersedes this one:
 
->>> browser.getControl('Superseded by').value = ['svg-support']
->>> browser.getControl('Continue').click()
+    >>> browser.getControl('Superseded by').value = 'svg-support'
+    >>> browser.getControl('Continue').click()
 
 Now, on the spec page we should see an alert that the spec has been
 superseded. The spec status should also have changed to superseded.
 
->>> 'This blueprint has been superseded.' in browser.contents
-True
->>> 'Superseded' in browser.contents
-True
+    >>> 'This blueprint has been superseded.' in browser.contents
+    True
+    >>> 'Superseded' in browser.contents
+    True
 
 And finally, we want to clear the superseding spec data and reset the
-status to New. If we POST back to the form, with no spec selected,
+status to New. If we POST back to the form, with Superseded by empty,
 then it should automatically do this:
 
->>> browser.getLink('Mark superseded').click()
->>> browser.getControl('Superseded by').displayValue = ['(Not Superseded)']
->>> browser.getControl('Continue').click()
+    >>> browser.getLink('Mark superseded').click()
+    >>> browser.getControl('Superseded by').value = ''
+    >>> browser.getControl('Continue').click()
 
 Let's confirm the status change:
 
->>> 'New' in browser.contents
-True
-
+    >>> 'New' in browser.contents
+    True