launchpad-reviewers team mailing list archive

[Nigel Babu <nigelbabu@xxxxxxxxx>]

Nigel Babu has proposed merging lp:~nigelbabu/launchpad/specification-validation-59301 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #59301 in Launchpad itself: "Don't give me a vague "URL is already registered by another blueprint" error"
  https://bugs.launchpad.net/launchpad/+bug/59301

For more details, see:
https://code.launchpad.net/~nigelbabu/launchpad/specification-validation-59301/+merge/73631

= Description =
The validation error for specurl is frustrating because it doesn't tell which other blueprint has the same specurl. This merge will mention the blueprint name and link to the blueprint.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/interfaces/specification.py
  lib/lp/blueprints/model/tests/test_specification.py
-- 
https://code.launchpad.net/~nigelbabu/launchpad/specification-validation-59301/+merge/73631
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~nigelbabu/launchpad/specification-validation-59301 into lp:launchpad.

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2011-06-07 06:49:18 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2011-09-01 07:20:36 +0000
@@ -46,6 +46,8 @@
     )
 
 from canonical.launchpad import _
+from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.menu import structured
 from lp.app.validators import LaunchpadValidationError
 from lp.app.validators.url import valid_webref
 from lp.blueprints.enums import (
@@ -122,7 +124,7 @@
 
 class SpecURLField(TextLine):
 
-    errormessage = _("%s is already registered by another blueprint.")
+    errormessage = _("%s is already registered by <a href=\"%s\">%s</a>.")
 
     def _validate(self, specurl):
         TextLine._validate(self, specurl)
@@ -132,8 +134,10 @@
             return
 
         specification = getUtility(ISpecificationSet).getByURL(specurl)
+        specification_url = canonical_url(specification)
         if specification is not None:
-            raise LaunchpadValidationError(self.errormessage % specurl)
+            raise LaunchpadValidationError(structured(self.errormessage %
+                (specurl, specification_url, specification.title)))
 
 
 class ISpecificationPublic(IHasOwner, IHasLinkedBranches):
@@ -373,7 +377,7 @@
         CollectionField(
             title=_("Branches associated with this spec, usually "
             "branches on which this spec is being implemented."),
-            value_type=Reference(schema=Interface), # ISpecificationBranch
+            value_type=Reference(schema=Interface),  # ISpecificationBranch
             readonly=True),
         as_of="devel")
 

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2011-07-21 19:26:51 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2011-09-01 07:20:36 +0000
@@ -7,7 +7,10 @@
 
 from testtools.matchers import Equals
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.validators import LaunchpadValidationError
+from lp.blueprints.interfaces.specification import ISpecification
 from lp.testing import TestCaseWithFactory
 
 
@@ -93,3 +96,20 @@
             dave.displayname]
         people = [sub.person.displayname for sub in spec.subscriptions]
         self.assertEqual(sorted_subscriptions, people)
+
+
+class TestSpecificationValidation(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_specurl_validation(self):
+        existing = self.factory.makeSpecification(
+            specurl=u'http://ubuntu.com')
+        spec = self.factory.makeSpecification()
+        url = canonical_url(existing)
+        field = ISpecification['specurl'].bind(spec)
+        e = self.assertRaises(LaunchpadValidationError, field.validate,
+            u'http://ubuntu.com')
+        self.assertEqual(str(e),
+            '%s is already registered by <a href="%s">%s</a>.'
+            % (u'http://ubuntu.com', url, existing.title))