Merge lp:~nigelbabu/launchpad/specification-validation-59301 into lp:launchpad
Proposed by
Nigel Babu
Status: | Merged |
---|---|
Approved by: | Gavin Panella |
Approved revision: | no longer in the source branch. |
Merged at revision: | 13852 |
Proposed branch: | lp:~nigelbabu/launchpad/specification-validation-59301 |
Merge into: | lp:launchpad |
Diff against target: |
335 lines (+137/-91) 4 files modified
lib/lp/blueprints/interfaces/specification.py (+8/-3) lib/lp/blueprints/model/tests/test_specification.py (+39/-0) lib/lp/blueprints/stories/blueprints/xx-creation.txt (+4/-3) lib/lp/blueprints/stories/blueprints/xx-editing.txt (+86/-85) |
To merge this branch: | bzr merge lp:~nigelbabu/launchpad/specification-validation-59301 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+73631@code.launchpad.net |
Commit message
[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 of the change
= 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/
lib/lp/
To post a comment you must log in.
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.") %s\">%s< /a>.")
+ errormessage = _("%s is already registered by <a href=\"
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_url = canonical_
if specification is not None:
- raise LaunchpadValida
+ raise LaunchpadValida
+ (specurl, specification_url, specification.
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) : makeSpecificati on() 'specurl' ].bind( spec)
field. validate( u'http:// example. com/nigelb')
spec = self.factory.
field = ISpecification[
[3]
+ raise LaunchpadValida tionError( structured( self.errormessa ge % title)) )
+ (specurl, specification_url, specification.
This is unsafe; structured() should be used slightly differently:
raise LaunchpadValida tionError(
structured( self.errormessa ge, specurl, specification_url,
specifica tion.title) )
In the test (or in a new test) try setting the spec title to something ...</script> - so that escaping is
that must be escaped - e.g. <script>
demonstrated.
[4]
+ self.assertEqua l(str(e) , ubuntu. com', url, existing.title))
+ '%s is already registered by <a href="%s">%s</a>.'
+ % (u'http://
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.