launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/better-error-open-owner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/better-error-open-owner/+merge/83550

Change Product:+edit-people to no longer blindly return Zope validation errors.

Zope performs form validation before we do, and if a constraint is not valid, it will already be set in the .errors list when validate() is called. I have reformatted the validate method, and now remove WidgetInputError from .errors adding a better error message than 'Constraint not satisfied'

I have also cleaned up lint in test_product, since I got frustrated with LostObjectErrors due to the view aborting the transaction on error and started converting the doctest to a unit test. I got the doctest working, so I removed my in-progress unit tests but left the lint fixes.
-- 
https://code.launchpad.net/~stevenk/launchpad/better-error-open-owner/+merge/83550
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/better-error-open-owner into lp:launchpad.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2011-10-29 18:23:06 +0000
+++ lib/lp/registry/browser/product.py	2011-11-28 05:21:47 +0000
@@ -59,6 +59,7 @@
     TextAreaWidget,
     TextWidget,
     )
+from zope.app.form.interfaces import WidgetInputError
 from zope.component import getUtility
 from zope.event import notify
 from zope.formlib import form
@@ -2305,23 +2306,31 @@
 
         At most one may be specified.
         """
-        # If errors have already been found we can skip validation.
-        if len(self.errors) > 0:
-            return
         xfer = data.get('transfer_to_registry', False)
         owner = data.get('owner')
-        if owner is not None and xfer:
-            self.setFieldError(
-                'owner',
-                'You may not specify a new owner if you '
-                'select the checkbox.')
-        elif xfer:
-            data['owner'] = getUtility(ILaunchpadCelebrities).registry_experts
-        elif owner is None:
-            self.setFieldError(
-                'owner',
-                'You must specify a maintainer or select '
-                'the checkbox.')
+        error = None
+        if xfer:
+            if owner:
+                error = (
+                    'You may not specify a new owner if you select the '
+                    'checkbox.')
+            else:
+                celebrities = getUtility(ILaunchpadCelebrities)
+                data['owner'] = celebrities.registry_experts
+        else:
+            if not owner:
+                if self.errors and isinstance(
+                    self.errors[0], WidgetInputError):
+                    del self.errors[0]
+                    error = (
+                        'You must choose a valid person or team to be the '
+                        'owner for %s.' % self.context.displayname)
+                else:
+                    error = (
+                        'You must specify a maintainer or select the '
+                        'checkbox.')
+        if error:
+            self.setFieldError('owner', error)
 
     @action(_('Save changes'), name='save')
     def save_action(self, action, data):

=== modified file 'lib/lp/registry/browser/tests/product-edit-people-view.txt'
--- lib/lp/registry/browser/tests/product-edit-people-view.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/registry/browser/tests/product-edit-people-view.txt	2011-11-28 05:21:47 +0000
@@ -42,6 +42,24 @@
     >>> firefox.owner.name
     u'no-priv'
 
+Ownership can not be transferred to an open team.
+
+    >>> owner = factory.makePerson()
+    >>> login_person(owner)
+    >>> product = factory.makeProduct(owner=owner)
+    >>> from lp.registry.interfaces.person import TeamSubscriptionPolicy
+    >>> team = factory.makeTeam(
+    ...     name='open', subscription_policy=TeamSubscriptionPolicy.OPEN)
+    >>> form = {
+    ...     'field.owner': 'open',
+    ...     'field.actions.save': 'Save changes',
+    ...     }
+
+    >>> transaction.commit()
+    >>> view = create_initialized_view(product, '+edit-people', form=form)
+    >>> view.errors
+    [u'You must choose a valid person or team to be the owner for ...']
+
 
 Assigning to Registry Administrators
 ------------------------------------
@@ -51,6 +69,7 @@
 
     >>> login_person(sample_person)
     >>> product = factory.makeProduct(owner=sample_person)
+    >>> transaction.commit()
 
     >>> form = {
     ...     'field.transfer_to_registry': 'on',
@@ -77,6 +96,7 @@
 Selecting both the owner/maintainer and the checkbox is also an error.
 
     >>> product = factory.makeProduct(owner=sample_person)
+    >>> transaction.commit()
     >>> form = {
     ...     'field.owner': 'no-priv',
     ...     'field.transfer_to_registry': 'on',

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2011-10-29 18:23:06 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2011-11-28 05:21:47 +0000
@@ -8,20 +8,15 @@
 import datetime
 
 import pytz
-
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
-from canonical.launchpad.testing.pages import (
-    find_tag_by_id,
-    )
+from canonical.launchpad.testing.pages import find_tag_by_id
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.enums import ServiceUsage
-from lp.registry.browser.product import (
-    ProductLicenseMixin,
-    )
+from lp.registry.browser.product import ProductLicenseMixin
 from lp.registry.interfaces.product import (
     License,
     IProductSet,