Merge lp:~nigelbabu/launchpad/specification-validation-59301 into lp:launchpad

[Nigel Babu]

[r=allenap][bug=59301] When attempting to use a specification URL that has already been used in another specification, link to that specification in the error message.

= 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

[Gavin Panella (allenap)]

Thanks for doing this, it's a nice improvement.

There are some minor issues with the branch as it stands, but I doubt
they'll hold you up for long.

[1]

- errormessage = _("%s is already registered by another blueprint.")
+ errormessage = _("%s is already registered by <a href=\"%s\">%s</a>.")

I suggest using single quotes here to make it a little more readable:

    errormessage = _('%s is already registered by <a href="%s">%s</a>.')

[2]

         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)))

canonical_url(None) blows up, so move the initialization of
specification_url to within the following conditional.

There should probably be a test for this too, something like:

    def test_specurl_validation_okay(self):
        spec = self.factory.makeSpecification()
        field = ISpecification['specurl'].bind(spec)
        field.validate(u'http://example.com/nigelb')

[3]

+ raise LaunchpadValidationError(structured(self.errormessage %
+ (specurl, specification_url, specification.title)))

This is unsafe; structured() should be used slightly differently:

            raise LaunchpadValidationError(
                structured(self.errormessage, specurl, specification_url,
                           specification.title))

In the test (or in a new test) try setting the spec title to something
that must be escaped - e.g. <script>...</script> - so that escaping is
demonstrated.

[4]

+ self.assertEqual(str(e),
+ '%s is already registered by <a href="%s">%s</a>.'
+ % (u'http://ubuntu.com', url, existing.title))

The order of arguments to assertEqual() is (expected, observed). When
the test fails it refers to them as "reference" and "actual"
respectively. Just switch them around.

review: Needs Fixing

[Gavin Panella (allenap)]

Looks good :)

review: Approve

=== 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-02 04:07:32 +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)
@@ -133,7 +135,10 @@
 
         specification = getUtility(ISpecificationSet).getByURL(specurl)
         if specification is not None:
-            raise LaunchpadValidationError(self.errormessage % specurl)
+            specification_url = canonical_url(specification)
+            raise LaunchpadValidationError(
+                    structured(self.errormessage, specurl, specification_url,
+                        specification.title))
 
 
 class ISpecificationPublic(IHasOwner, IHasLinkedBranches):
@@ -373,7 +378,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-02 04:07:32 +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,39 @@
             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_duplicate(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(
+            '%s is already registered by <a href="%s">%s</a>.'
+            % (u'http://ubuntu.com', url, existing.title), str(e))
+
+    def test_specurl_validation_valid(self):
+        spec = self.factory.makeSpecification()
+        field = ISpecification['specurl'].bind(spec)
+        field.validate(u'http://example.com/nigelb')
+
+    def test_specurl_validation_escape(self):
+        existing = self.factory.makeSpecification(
+                specurl=u'http://ubuntu.com/foo',
+                title='<script>alert("foo");</script>')
+        cleaned_title = '&lt;script&gt;alert("foo");&lt;/script&gt;'
+        spec = self.factory.makeSpecification()
+        url = canonical_url(existing)
+        field = ISpecification['specurl'].bind(spec)
+        e = self.assertRaises(LaunchpadValidationError, field.validate,
+            u'http://ubuntu.com/foo')
+        self.assertEqual(
+            '%s is already registered by <a href="%s">%s</a>.'
+            % (u'http://ubuntu.com/foo', url, cleaned_title), str(e))

=== modified file 'lib/lp/blueprints/stories/blueprints/xx-creation.txt'
--- lib/lp/blueprints/stories/blueprints/xx-creation.txt	2011-05-29 21:23:37 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-creation.txt	2011-09-02 04:07:32 +0000
@@ -633,8 +633,8 @@
     ...      print message.renderContents()
     There is 1 error...already in use by another blueprint...
 
-Attempting to register a duplicate blueprint from a non-target context produces
-the same error:
+Attempting to register a duplicate blueprint from a non-target context
+produces the same error:
 
     >>> url = 'http://blueprints.launchpad.dev/sprints/rome/+addspec'
     >>> user_browser.open(url)
@@ -702,7 +702,8 @@
     'http://blueprints.launchpad.dev/ubuntu/+addspec'
     >>> for message in find_tags_by_class(user_browser.contents, 'message'):
     ...      print message.renderContents()
-    There is 1 error...already registered by another blueprint...
+    There is 1 error...is already registered by...
+    ...Network Magic: Auto Network Detection...
 
 
 Registering blueprints from other locations

=== modified file 'lib/lp/blueprints/stories/blueprints/xx-editing.txt'
--- lib/lp/blueprints/stories/blueprints/xx-editing.txt	2011-03-10 01:25:13 +0000
+++ lib/lp/blueprints/stories/blueprints/xx-editing.txt	2011-09-02 04:07:32 +0000
@@ -6,13 +6,13 @@
 
 First, we need to load the +edit page.
 
-  >>> browser.addHeader('Authorization', 'Basic [e-mail]:test')
-  >>> features_domain = 'http://blueprints.launchpad.dev'
-  >>> spec_path = '/firefox/+spec/extension-manager-upgrades'
-  >>> browser.open(features_domain + spec_path)
-  >>> browser.getLink('Change details').click()
-  >>> browser.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+edit'
+    >>> browser.addHeader('Authorization', 'Basic [e-mail]:test')
+    >>> features_domain = 'http://blueprints.launchpad.dev'
+    >>> spec_path = '/firefox/+spec/extension-manager-upgrades'
+    >>> browser.open(features_domain + spec_path)
+    >>> browser.getLink('Change details').click()
+    >>> browser.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+edit'
 
 The page links back to the blueprint page, in case we change our minds.
 
@@ -22,109 +22,110 @@
 
 Launchpad won't let us use an URL already used in another blueprint.
 
-  >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
-  >>> browser.getControl('Specification URL').value = url
-  >>> browser.getControl('Change').click()
+    >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
+    >>> browser.getControl('Specification URL').value = url
+    >>> browser.getControl('Change').click()
 
-  >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
-  ...            'registered by another blueprint')
-  >>> message in browser.contents
-  True
+    >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
+    ...            'registered by')
+    >>> message in browser.contents
+    True
 
 Test the name validator filling a name of a existing specification for that
 product.
 
-  >>> browser.getControl('Name').value = 'e4x'
-  >>> url = 'http://wiki.mozilla.org/Firefox:1.1_Product_Team'
-  >>> browser.getControl('Specification URL').value = url
-  >>> browser.getControl('Change').click()
+    >>> browser.getControl('Name').value = 'e4x'
+    >>> url = 'http://wiki.mozilla.org/Firefox:1.1_Product_Team'
+    >>> browser.getControl('Specification URL').value = url
+    >>> browser.getControl('Change').click()
 
-  >>> message = 'e4x is already in use by another blueprint'
-  >>> message in browser.contents
-  True
+    >>> message = 'e4x is already in use by another blueprint'
+    >>> message in browser.contents
+    True
 
 Now, let's POST the resulting changes. We should be redirected to the
 specification home page.
 
-  >>> browser.getControl('Name').value = 'extension-manager-upgrades'
-  >>> browser.getControl('Title').value = 'Extension Manager System Upgrades'
-  >>> browser.getControl('Specification URL').value = url
-  >>> summary = ('Simplify the way extensions are installed and registered '
-  ...            'so that: 1. third party applications can easily register '
-  ...            'and deregister extensions that live with their code. 2. '
-  ...            'developers can easily register extensions that they are '
-  ...            'developing out of a location apart from their build (e.g.'
-  ...            ' their home directory), and  3. developers can easily '
-  ...            'install extensions for testing.')
-  >>> browser.getControl('Summary').value = summary
-  >>> browser.getControl('Status Whiteboard').value = 'XXX'
-  >>> browser.getControl('Change').click()
-  >>> browser.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
+    >>> browser.getControl('Name').value = 'extension-manager-upgrades'
+    >>> browser.getControl('Title').value = (
+    ...     'Extension Manager System Upgrades')
+    >>> browser.getControl('Specification URL').value = url
+    >>> summary = ('Simplify the way extensions are installed and registered '
+    ...            'so that: 1. third party applications can easily register '
+    ...            'and deregister extensions that live with their code. 2. '
+    ...            'developers can easily register extensions that they are '
+    ...            'developing out of a location apart from their build (e.g.'
+    ...            ' their home directory), and  3. developers can easily '
+    ...            'install extensions for testing.')
+    >>> browser.getControl('Summary').value = summary
+    >>> browser.getControl('Status Whiteboard').value = 'XXX'
+    >>> browser.getControl('Change').click()
+    >>> browser.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
 
 Also, we would like to assign these to someone other than Carlos, and we
 would also like to have a drafter associated with it.
 
-  >>> browser.getLink(url='+people').click()
-  >>> browser.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+people'
-  >>> back_link = browser.getLink('Extension Manager System Upgrades')
-  >>> back_link.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
-  >>> browser.getControl('Assignee').value = '[e-mail]'
-  >>> browser.getControl('Drafter').value = '[e-mail]'
-  >>> browser.getControl('Approver').value = '[e-mail]'
-  >>> browser.getControl('Status Whiteboard').value = 'YYY'
-  >>> browser.getControl('Change').click()
-  >>> browser.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
+    >>> browser.getLink(url='+people').click()
+    >>> browser.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+people'
+    >>> back_link = browser.getLink('Extension Manager System Upgrades')
+    >>> back_link.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
+    >>> browser.getControl('Assignee').value = '[e-mail]'
+    >>> browser.getControl('Drafter').value = '[e-mail]'
+    >>> browser.getControl('Approver').value = '[e-mail]'
+    >>> browser.getControl('Status Whiteboard').value = 'YYY'
+    >>> browser.getControl('Change').click()
+    >>> browser.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
 
 Finally, we should be able to change the status metadata (definition status,
 implementation status, estimated man days etc) of the specification.
 
-  >>> browser.getLink(url='+status').click()
-  >>> back_link = browser.getLink('Extension Manager System Upgrades')
-  >>> back_link.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
-  >>> browser.getControl('Definition Status').value = ['DRAFT']
-  >>> # browser.getControl('Estimated Developer Days').value = '5'
-  >>> browser.getControl('Implementation Status').value = ['SLOW']
-  >>> browser.getControl('Status Whiteboard').value = 'XXX'
-  >>> browser.getControl('Change').click()
-  >>> browser.url
-  'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
+    >>> browser.getLink(url='+status').click()
+    >>> back_link = browser.getLink('Extension Manager System Upgrades')
+    >>> back_link.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
+    >>> browser.getControl('Definition Status').value = ['DRAFT']
+    >>> # browser.getControl('Estimated Developer Days').value = '5'
+    >>> browser.getControl('Implementation Status').value = ['SLOW']
+    >>> browser.getControl('Status Whiteboard').value = 'XXX'
+    >>> browser.getControl('Change').click()
+    >>> browser.url
+    'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
 
 Any logged in user can edit a specification whiteboard.
 
-  >>> user_browser.open(
-  ...     'http://blueprints.launchpad.dev/kubuntu/'
-  ...     '+spec/krunch-desktop-plan')
-  >>> user_browser.getLink(url='+whiteboard').click()
-  >>> back_link = user_browser.getLink('The Krunch Desktop Plan')
-  >>> back_link.url
-  'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
-
-  >>> user_browser.getControl('Whiteboard').value = 'XXX by Sample Person'
-  >>> user_browser.getControl('Change').click()
-  >>> user_browser.url
-  'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
-
-  >>> 'XXX by Sample Person' in user_browser.contents
-  True
+    >>> user_browser.open(
+    ...     'http://blueprints.launchpad.dev/kubuntu/'
+    ...     '+spec/krunch-desktop-plan')
+    >>> user_browser.getLink(url='+whiteboard').click()
+    >>> back_link = user_browser.getLink('The Krunch Desktop Plan')
+    >>> back_link.url
+    'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
+
+    >>> user_browser.getControl('Whiteboard').value = 'XXX by Sample Person'
+    >>> user_browser.getControl('Change').click()
+    >>> user_browser.url
+    'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
+
+    >>> 'XXX by Sample Person' in user_browser.contents
+    True
 
 Regular users can't access the change status page.
 
-  >>> user_browser.getLink(url='+status')
-  Traceback (most recent call last):
-  ...
-  LinkNotFoundError
+    >>> user_browser.getLink(url='+status')
+    Traceback (most recent call last):
+    ...
+    LinkNotFoundError
 
-  >>> user_browser.open(
-  ...     'http://blueprints.launchpad.dev/kubuntu/'
-  ...     '+spec/krunch-desktop-plan/+status')
-  Traceback (most recent call last):
-  ...
-  Unauthorized:...
+    >>> user_browser.open(
+    ...     'http://blueprints.launchpad.dev/kubuntu/'
+    ...     '+spec/krunch-desktop-plan/+status')
+    Traceback (most recent call last):
+    ...
+    Unauthorized:...
 
 Nor can they change a blueprint's priority.