Launchpad itself

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

  1. specification-validation-59301
  2. Merge into devel
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
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/blueprints/interfaces/specification.py
  lib/lp/blueprints/model/tests/test_specification.py

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

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
Revision history for this message
Gavin Panella (allenap) wrote :

Looks good :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
46 )46 )
4747
48from canonical.launchpad import _48from canonical.launchpad import _
49from canonical.launchpad.webapp import canonical_url
50from canonical.launchpad.webapp.menu import structured
49from lp.app.validators import LaunchpadValidationError51from lp.app.validators import LaunchpadValidationError
50from lp.app.validators.url import valid_webref52from lp.app.validators.url import valid_webref
51from lp.blueprints.enums import (53from lp.blueprints.enums import (
@@ -122,7 +124,7 @@
122124
123class SpecURLField(TextLine):125class SpecURLField(TextLine):
124126
125 errormessage = _("%s is already registered by another blueprint.")127 errormessage = _('%s is already registered by <a href=\"%s\">%s</a>.')
126128
127 def _validate(self, specurl):129 def _validate(self, specurl):
128 TextLine._validate(self, specurl)130 TextLine._validate(self, specurl)
@@ -133,7 +135,10 @@
133135
134 specification = getUtility(ISpecificationSet).getByURL(specurl)136 specification = getUtility(ISpecificationSet).getByURL(specurl)
135 if specification is not None:137 if specification is not None:
136 raise LaunchpadValidationError(self.errormessage % specurl)138 specification_url = canonical_url(specification)
139 raise LaunchpadValidationError(
140 structured(self.errormessage, specurl, specification_url,
141 specification.title))
137142
138143
139class ISpecificationPublic(IHasOwner, IHasLinkedBranches):144class ISpecificationPublic(IHasOwner, IHasLinkedBranches):
@@ -373,7 +378,7 @@
373 CollectionField(378 CollectionField(
374 title=_("Branches associated with this spec, usually "379 title=_("Branches associated with this spec, usually "
375 "branches on which this spec is being implemented."),380 "branches on which this spec is being implemented."),
376 value_type=Reference(schema=Interface), # ISpecificationBranch381 value_type=Reference(schema=Interface), # ISpecificationBranch
377 readonly=True),382 readonly=True),
378 as_of="devel")383 as_of="devel")
379384
380385
=== 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 @@
77
8from testtools.matchers import Equals8from testtools.matchers import Equals
99
10from canonical.launchpad.webapp import canonical_url
10from canonical.testing.layers import DatabaseFunctionalLayer11from canonical.testing.layers import DatabaseFunctionalLayer
12from lp.app.validators import LaunchpadValidationError
13from lp.blueprints.interfaces.specification import ISpecification
11from lp.testing import TestCaseWithFactory14from lp.testing import TestCaseWithFactory
1215
1316
@@ -93,3 +96,39 @@
93 dave.displayname]96 dave.displayname]
94 people = [sub.person.displayname for sub in spec.subscriptions]97 people = [sub.person.displayname for sub in spec.subscriptions]
95 self.assertEqual(sorted_subscriptions, people)98 self.assertEqual(sorted_subscriptions, people)
99
100
101class TestSpecificationValidation(TestCaseWithFactory):
102
103 layer = DatabaseFunctionalLayer
104
105 def test_specurl_validation_duplicate(self):
106 existing = self.factory.makeSpecification(
107 specurl=u'http://ubuntu.com')
108 spec = self.factory.makeSpecification()
109 url = canonical_url(existing)
110 field = ISpecification['specurl'].bind(spec)
111 e = self.assertRaises(LaunchpadValidationError, field.validate,
112 u'http://ubuntu.com')
113 self.assertEqual(
114 '%s is already registered by <a href="%s">%s</a>.'
115 % (u'http://ubuntu.com', url, existing.title), str(e))
116
117 def test_specurl_validation_valid(self):
118 spec = self.factory.makeSpecification()
119 field = ISpecification['specurl'].bind(spec)
120 field.validate(u'http://example.com/nigelb')
121
122 def test_specurl_validation_escape(self):
123 existing = self.factory.makeSpecification(
124 specurl=u'http://ubuntu.com/foo',
125 title='<script>alert("foo");</script>')
126 cleaned_title = '&lt;script&gt;alert("foo");&lt;/script&gt;'
127 spec = self.factory.makeSpecification()
128 url = canonical_url(existing)
129 field = ISpecification['specurl'].bind(spec)
130 e = self.assertRaises(LaunchpadValidationError, field.validate,
131 u'http://ubuntu.com/foo')
132 self.assertEqual(
133 '%s is already registered by <a href="%s">%s</a>.'
134 % (u'http://ubuntu.com/foo', url, cleaned_title), str(e))
96135
=== 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 @@
633 ... print message.renderContents()633 ... print message.renderContents()
634 There is 1 error...already in use by another blueprint...634 There is 1 error...already in use by another blueprint...
635635
636Attempting to register a duplicate blueprint from a non-target context produces636Attempting to register a duplicate blueprint from a non-target context
637the same error:637produces the same error:
638638
639 >>> url = 'http://blueprints.launchpad.dev/sprints/rome/+addspec'639 >>> url = 'http://blueprints.launchpad.dev/sprints/rome/+addspec'
640 >>> user_browser.open(url)640 >>> user_browser.open(url)
@@ -702,7 +702,8 @@
702 'http://blueprints.launchpad.dev/ubuntu/+addspec'702 'http://blueprints.launchpad.dev/ubuntu/+addspec'
703 >>> for message in find_tags_by_class(user_browser.contents, 'message'):703 >>> for message in find_tags_by_class(user_browser.contents, 'message'):
704 ... print message.renderContents()704 ... print message.renderContents()
705 There is 1 error...already registered by another blueprint...705 There is 1 error...is already registered by...
706 ...Network Magic: Auto Network Detection...
706707
707708
708Registering blueprints from other locations709Registering blueprints from other locations
709710
=== 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 @@
66
7First, we need to load the +edit page.7First, we need to load the +edit page.
88
9 >>> browser.addHeader('Authorization', 'Basic carlos@canonical.com:test')9 >>> browser.addHeader('Authorization', 'Basic carlos@canonical.com:test')
10 >>> features_domain = 'http://blueprints.launchpad.dev'10 >>> features_domain = 'http://blueprints.launchpad.dev'
11 >>> spec_path = '/firefox/+spec/extension-manager-upgrades'11 >>> spec_path = '/firefox/+spec/extension-manager-upgrades'
12 >>> browser.open(features_domain + spec_path)12 >>> browser.open(features_domain + spec_path)
13 >>> browser.getLink('Change details').click()13 >>> browser.getLink('Change details').click()
14 >>> browser.url14 >>> browser.url
15 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+edit'15 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+edit'
1616
17The page links back to the blueprint page, in case we change our minds.17The page links back to the blueprint page, in case we change our minds.
1818
@@ -22,109 +22,110 @@
2222
23Launchpad won't let us use an URL already used in another blueprint.23Launchpad won't let us use an URL already used in another blueprint.
2424
25 >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'25 >>> url = 'https://wiki.ubuntu.com/MediaIntegrityCheck'
26 >>> browser.getControl('Specification URL').value = url26 >>> browser.getControl('Specification URL').value = url
27 >>> browser.getControl('Change').click()27 >>> browser.getControl('Change').click()
2828
29 >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '29 >>> message = ('https://wiki.ubuntu.com/MediaIntegrityCheck is already '
30 ... 'registered by another blueprint')30 ... 'registered by')
31 >>> message in browser.contents31 >>> message in browser.contents
32 True32 True
3333
34Test the name validator filling a name of a existing specification for that34Test the name validator filling a name of a existing specification for that
35product.35product.
3636
37 >>> browser.getControl('Name').value = 'e4x'37 >>> browser.getControl('Name').value = 'e4x'
38 >>> url = 'http://wiki.mozilla.org/Firefox:1.1_Product_Team'38 >>> url = 'http://wiki.mozilla.org/Firefox:1.1_Product_Team'
39 >>> browser.getControl('Specification URL').value = url39 >>> browser.getControl('Specification URL').value = url
40 >>> browser.getControl('Change').click()40 >>> browser.getControl('Change').click()
4141
42 >>> message = 'e4x is already in use by another blueprint'42 >>> message = 'e4x is already in use by another blueprint'
43 >>> message in browser.contents43 >>> message in browser.contents
44 True44 True
4545
46Now, let's POST the resulting changes. We should be redirected to the46Now, let's POST the resulting changes. We should be redirected to the
47specification home page.47specification home page.
4848
49 >>> browser.getControl('Name').value = 'extension-manager-upgrades'49 >>> browser.getControl('Name').value = 'extension-manager-upgrades'
50 >>> browser.getControl('Title').value = 'Extension Manager System Upgrades'50 >>> browser.getControl('Title').value = (
51 >>> browser.getControl('Specification URL').value = url51 ... 'Extension Manager System Upgrades')
52 >>> summary = ('Simplify the way extensions are installed and registered '52 >>> browser.getControl('Specification URL').value = url
53 ... 'so that: 1. third party applications can easily register '53 >>> summary = ('Simplify the way extensions are installed and registered '
54 ... 'and deregister extensions that live with their code. 2. '54 ... 'so that: 1. third party applications can easily register '
55 ... 'developers can easily register extensions that they are '55 ... 'and deregister extensions that live with their code. 2. '
56 ... 'developing out of a location apart from their build (e.g.'56 ... 'developers can easily register extensions that they are '
57 ... ' their home directory), and 3. developers can easily '57 ... 'developing out of a location apart from their build (e.g.'
58 ... 'install extensions for testing.')58 ... ' their home directory), and 3. developers can easily '
59 >>> browser.getControl('Summary').value = summary59 ... 'install extensions for testing.')
60 >>> browser.getControl('Status Whiteboard').value = 'XXX'60 >>> browser.getControl('Summary').value = summary
61 >>> browser.getControl('Change').click()61 >>> browser.getControl('Status Whiteboard').value = 'XXX'
62 >>> browser.url62 >>> browser.getControl('Change').click()
63 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'63 >>> browser.url
64 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
6465
65Also, we would like to assign these to someone other than Carlos, and we66Also, we would like to assign these to someone other than Carlos, and we
66would also like to have a drafter associated with it.67would also like to have a drafter associated with it.
6768
68 >>> browser.getLink(url='+people').click()69 >>> browser.getLink(url='+people').click()
69 >>> browser.url70 >>> browser.url
70 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+people'71 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades/+people'
71 >>> back_link = browser.getLink('Extension Manager System Upgrades')72 >>> back_link = browser.getLink('Extension Manager System Upgrades')
72 >>> back_link.url73 >>> back_link.url
73 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'74 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
74 >>> browser.getControl('Assignee').value = 'tsukimi@quaqua.net'75 >>> browser.getControl('Assignee').value = 'tsukimi@quaqua.net'
75 >>> browser.getControl('Drafter').value = 'daf@canonical.com'76 >>> browser.getControl('Drafter').value = 'daf@canonical.com'
76 >>> browser.getControl('Approver').value = 'stuart.bishop@canonical.com'77 >>> browser.getControl('Approver').value = 'stuart.bishop@canonical.com'
77 >>> browser.getControl('Status Whiteboard').value = 'YYY'78 >>> browser.getControl('Status Whiteboard').value = 'YYY'
78 >>> browser.getControl('Change').click()79 >>> browser.getControl('Change').click()
79 >>> browser.url80 >>> browser.url
80 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'81 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
8182
82Finally, we should be able to change the status metadata (definition status,83Finally, we should be able to change the status metadata (definition status,
83implementation status, estimated man days etc) of the specification.84implementation status, estimated man days etc) of the specification.
8485
85 >>> browser.getLink(url='+status').click()86 >>> browser.getLink(url='+status').click()
86 >>> back_link = browser.getLink('Extension Manager System Upgrades')87 >>> back_link = browser.getLink('Extension Manager System Upgrades')
87 >>> back_link.url88 >>> back_link.url
88 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'89 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
89 >>> browser.getControl('Definition Status').value = ['DRAFT']90 >>> browser.getControl('Definition Status').value = ['DRAFT']
90 >>> # browser.getControl('Estimated Developer Days').value = '5'91 >>> # browser.getControl('Estimated Developer Days').value = '5'
91 >>> browser.getControl('Implementation Status').value = ['SLOW']92 >>> browser.getControl('Implementation Status').value = ['SLOW']
92 >>> browser.getControl('Status Whiteboard').value = 'XXX'93 >>> browser.getControl('Status Whiteboard').value = 'XXX'
93 >>> browser.getControl('Change').click()94 >>> browser.getControl('Change').click()
94 >>> browser.url95 >>> browser.url
95 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'96 'http://blueprints.launchpad.dev/firefox/+spec/extension-manager-upgrades'
9697
97Any logged in user can edit a specification whiteboard.98Any logged in user can edit a specification whiteboard.
9899
99 >>> user_browser.open(100 >>> user_browser.open(
100 ... 'http://blueprints.launchpad.dev/kubuntu/'101 ... 'http://blueprints.launchpad.dev/kubuntu/'
101 ... '+spec/krunch-desktop-plan')102 ... '+spec/krunch-desktop-plan')
102 >>> user_browser.getLink(url='+whiteboard').click()103 >>> user_browser.getLink(url='+whiteboard').click()
103 >>> back_link = user_browser.getLink('The Krunch Desktop Plan')104 >>> back_link = user_browser.getLink('The Krunch Desktop Plan')
104 >>> back_link.url105 >>> back_link.url
105 'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'106 'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
106107
107 >>> user_browser.getControl('Whiteboard').value = 'XXX by Sample Person'108 >>> user_browser.getControl('Whiteboard').value = 'XXX by Sample Person'
108 >>> user_browser.getControl('Change').click()109 >>> user_browser.getControl('Change').click()
109 >>> user_browser.url110 >>> user_browser.url
110 'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'111 'http://blueprints.launchpad.dev/kubuntu/+spec/krunch-desktop-plan'
111112
112 >>> 'XXX by Sample Person' in user_browser.contents113 >>> 'XXX by Sample Person' in user_browser.contents
113 True114 True
114115
115Regular users can't access the change status page.116Regular users can't access the change status page.
116117
117 >>> user_browser.getLink(url='+status')118 >>> user_browser.getLink(url='+status')
118 Traceback (most recent call last):119 Traceback (most recent call last):
119 ...120 ...
120 LinkNotFoundError121 LinkNotFoundError
121122
122 >>> user_browser.open(123 >>> user_browser.open(
123 ... 'http://blueprints.launchpad.dev/kubuntu/'124 ... 'http://blueprints.launchpad.dev/kubuntu/'
124 ... '+spec/krunch-desktop-plan/+status')125 ... '+spec/krunch-desktop-plan/+status')
125 Traceback (most recent call last):126 Traceback (most recent call last):
126 ...127 ...
127 Unauthorized:...128 Unauthorized:...
128129
129Nor can they change a blueprint's priority.130Nor can they change a blueprint's priority.
130131