Code review comment for lp:~nigelbabu/launchpad/specification-validation-59301

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.