Launchpad itself

Merge lp:~bac/launchpad/bug-559200 into lp:launchpad

  1. bug-559200
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/bug-559200
Merge into: lp:launchpad
Diff against target: 192 lines (+130/-15)
3 files modified
lib/lp/code/windmill/tests/test_productseries_setbranch.py (+58/-0)
lib/lp/registry/browser/productseries.py (+34/-15)
lib/lp/registry/browser/tests/productseries-setbranch-view.txt (+38/-0)
To merge this branch: bzr merge lp:~bac/launchpad/bug-559200
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+23256@code.launchpad.net

Commit message

Fix form validation errors on productseries/+setbranch. Provide windmill test to verify JS is loaded and working.

Description of the change

= Summary =

Field validation was not properly implemented which allowed OOPS to
happen or for errors to go unnoticed. Also a test was missing to prove
the JS controls were properly loaded.

== Proposed fix ==

Fixed the validation by not return a next_url when errors occur. Wrote
new windmill test that simply demonstrates the selective enabling and
disabling of one field, which proves the JS controls are loaded and
working. Full tests for those controls are already done elsewhere.

== Pre-implementation notes ==

Brief chats with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.code -t test_productseries_setbranch.py
bin/test -vvm lp.registry productseries-setbranch-view.txt

== Demo and Q/A ==

Go to http://launchpad.dev/firefox/trunk/+setbranch. Create a new
branch. Return and try to create another new branch or link to a an
external repository using the same branch name. Verify you get a
validation error.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/tests/productseries-setbranch-view.txt
  lib/lp/registry/browser/productseries.py
  lib/lp/code/windmill/tests/test_productseries_setbranch.py

== Pylint notices ==

All of these look to be bogus except the line too long, which I will fix.

lib/lp/registry/browser/productseries.py
    34: [F0401] Unable to import 'zope.component'
    35: [F0401] Unable to import 'zope.app.form.browser'
    36: [F0401] Unable to import 'zope.formlib'
    38: [F0401] Unable to import 'zope.schema'
    39: [F0401] Unable to import 'zope.schema.vocabulary'
    41: [F0401] Unable to import 'z3c.ptcompat'
    43: [F0401] Unable to import 'canonical.cachedproperty'
    44: [F0401] Unable to import 'canonical.launchpad'
    45: [F0401] Unable to import 'canonical.launchpad.fields'
    46: [F0401] Unable to import 'lp.blueprints.browser.specificationtarget'
    48: [F0401] Unable to import 'lp.blueprints.interfaces.specification'
    50: [F0401] Unable to import 'lp.bugs.interfaces.bugtask'
    51: [F0401] Unable to import 'lp.bugs.browser.bugtask'
    52: [F0401] Unable to import 'canonical.launchpad.helpers'
    53: [F0401] Unable to import 'lp.code.browser.branch'
    54: [F0401] Unable to import 'lp.code.browser.branchref'
    55: [F0401] Unable to import 'lp.code.enums'
    56: [F0401] Unable to import 'lp.code.interfaces.branch'
    58: [F0401] Unable to import 'lp.code.interfaces.branchjob'
    59: [F0401] Unable to import 'lp.code.interfaces.branchtarget'
    60: [F0401] Unable to import 'lp.code.interfaces.codeimport'
    62: [F0401] Unable to import 'lp.services.worlddata.interfaces.country'
    63: [F0401] Unable to import 'lp.bugs.interfaces.bugtask'
    64: [F0401] Unable to import 'canonical.launchpad.interfaces.launchpad'
    65: [F0401] Unable to import 'lp.registry.browser'
    66: [F0401] Unable to import
'lp.registry.browser.structuralsubscription'
    69: [F0401] Unable to import 'lp.registry.interfaces.packaging'
    71: [F0401] Unable to import 'lp.translations.interfaces.potemplate'
    72: [F0401] Unable to import
'lp.translations.interfaces.productserieslanguage'
    74: [F0401] Unable to import 'lp.services.worlddata.interfaces.language'
    75: [F0401] Unable to import 'canonical.launchpad.webapp'
    79: [F0401] Unable to import 'canonical.launchpad.webapp.authorization'
    80: [F0401] Unable to import 'canonical.launchpad.webapp.breadcrumb'
    81: [F0401] Unable to import 'canonical.launchpad.webapp.interfaces'
    83: [F0401] Unable to import 'canonical.launchpad.webapp.launchpadform'
    85: [F0401] Unable to import 'canonical.launchpad.webapp.menu'
    86: [F0401] Unable to import 'canonical.widgets.itemswidgets'
    87: [F0401] Unable to import 'canonical.widgets.textwidgets'
    89: [F0401] Unable to import 'lp.registry.browser'
    91: [F0401] Unable to import 'lp.registry.interfaces.series'
    92: [F0401] Unable to import 'lp.registry.interfaces.productseries'
    94: [F0401] Unable to import 'lazr.enum'
    95: [F0401] Unable to import 'lazr.restful.interface'
    383: [E1002, ProductSeriesUbuntuPackagingView.__init__] Use super on
an old style class
    403: [E1002, ProductSeriesUbuntuPackagingView.setUpFields] Use super
on an old style class
    424: [E1002, ProductSeriesUbuntuPackagingView.setUpWidgets] Use
super on an old style class
    753: [E1002, ProductSeriesSetBranchView.setUpWidgets] Use super on
an old style class
    870: [E1002, ProductSeriesSetBranchView.validate_widgets] Use super
on an old style class
    870: [E1002, ProductSeriesSetBranchView.validate_widgets] Use super
on an old style class

lib/lp/code/windmill/tests/test_productseries_setbranch.py
    35: [C0301] Line too long (79/78)
    11: [F0401] Unable to import 'canonical.launchpad.windmill.testing'
    13: [F0401] Unable to import 'lp.code.windmill.testing'
    14: [F0401] Unable to import 'lp.testing'

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'lib/lp/code/windmill/tests/test_productseries_setbranch.py'
2--- lib/lp/code/windmill/tests/test_productseries_setbranch.py 1970-01-01 00:00:00 +0000
3+++ lib/lp/code/windmill/tests/test_productseries_setbranch.py 2010-04-13 20:37:28 +0000
4@@ -0,0 +1,58 @@
5+# Copyright 2010 Canonical Ltd. This software is licensed under the
6+# GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+"""Test for productseries setbranch Javascript."""
9+
10+__metaclass__ = type
11+__all__ = []
12+
13+import unittest
14+
15+from canonical.launchpad.windmill.testing import lpuser
16+
17+from lp.code.windmill.testing import CodeWindmillLayer
18+from lp.testing import WindmillTestCase
19+
20+
21+class TestProductSeriesSetbranch(WindmillTestCase):
22+ """Test productseries +setbranch Javascript controls."""
23+
24+ layer = CodeWindmillLayer
25+ suite_name = 'ProductSeriesSetBranch'
26+
27+ def test_productseries_setbranch(self):
28+ """Test productseries JS on /$projectseries/+setbranch page."""
29+
30+ # Ensure we're logged in as 'foo bar'
31+ user = lpuser.FOO_BAR
32+ user.ensure_login(self.client)
33+ self.client.open(
34+ url=u'http://launchpad.dev:8085/firefox/trunk/+setbranch')
35+
36+ # To demonstrate the Javascript is loaded we simply need to see that
37+ # one of the controls is deactivated when the radio button selections
38+ # change. When "Link to a Bazaar branch" is selected the
39+ # branch_location field should be enabled. When any other radio
40+ # button is selected the branch_location field is disabled.
41+ self.client.waits.forElement(id=u'field.branch_type.link-lp-bzr',
42+ timeout=u'20000')
43+
44+ # Select Bazaar as the RCS type...
45+ self.client.click(id=u'field.branch_type.link-lp-bzr')
46+ self.client.waits.forElement(id=u'field.branch_location',
47+ timeout=u'20000')
48+ # And the branch location is enabled.
49+ self.client.asserts.assertElemJS(id=u'field.branch_location',
50+ js='!element.disabled')
51+
52+ # Select 'create new'...
53+ self.client.click(id=u'field.branch_type.create-new')
54+ self.client.waits.forElement(id=u'field.branch_location',
55+ timeout=u'20000')
56+ # And the branch location is now disabled, proving that the javascript
57+ # controls have loaded and are functioning.
58+ self.client.asserts.assertElemJS(id=u'field.branch_location',
59+ js='element.disabled')
60+
61+def test_suite():
62+ return unittest.TestLoader().loadTestsFromName(__name__)
63
64=== modified file 'lib/lp/registry/browser/productseries.py'
65--- lib/lp/registry/browser/productseries.py 2010-04-12 23:10:39 +0000
66+++ lib/lp/registry/browser/productseries.py 2010-04-13 20:37:28 +0000
67@@ -805,6 +805,19 @@
68 'branch_type': LINK_LP_BZR,
69 }
70
71+ errors_in_action = False
72+
73+ @property
74+ def next_url(self):
75+ """Return the next_url.
76+
77+ Use the value from `ReturnToReferrerMixin` or None if there
78+ are errors.
79+ """
80+ if self.errors_in_action:
81+ return None
82+ return super(ProductSeriesSetBranchView, self).next_url
83+
84 def setUpWidgets(self):
85 """See `LaunchpadFormView`."""
86 super(ProductSeriesSetBranchView, self).setUpWidgets()
87@@ -1008,16 +1021,16 @@
88 branch = self._createBzrBranch(
89 BranchType.MIRRORED, branch_name, branch_owner,
90 data['repo_url'])
91+ if branch is None:
92+ self.errors_in_action = True
93+ return
94
95- if branch is not None:
96- self.context.branch = branch
97- self.request.response.addInfoNotification(
98- 'Mirrored branch created and linked to '
99- 'the series.')
100+ self.context.branch = branch
101+ self.request.response.addInfoNotification(
102+ 'Mirrored branch created and linked to '
103+ 'the series.')
104 else:
105 # We need to create an import request.
106-
107- # Ensure the URL has not already been imported.
108 if rcs_type == RevisionControlSystemsExtended.CVS:
109 cvs_root = data.get('repo_url')
110 cvs_module = data.get('cvs_module')
111@@ -1027,14 +1040,20 @@
112 cvs_module = None
113 url = data.get('repo_url')
114 rcs_item = RevisionControlSystems.items[rcs_type.name]
115- code_import = getUtility(ICodeImportSet).new(
116- registrant=branch_owner,
117- target=IBranchTarget(self.context.product),
118- branch_name=branch_name,
119- rcs_type=rcs_item,
120- url=url,
121- cvs_root=cvs_root,
122- cvs_module=cvs_module)
123+ try:
124+ code_import = getUtility(ICodeImportSet).new(
125+ registrant=branch_owner,
126+ target=IBranchTarget(self.context.product),
127+ branch_name=branch_name,
128+ rcs_type=rcs_item,
129+ url=url,
130+ cvs_root=cvs_root,
131+ cvs_module=cvs_module)
132+ except BranchExists, e:
133+ self._setBranchExists(e.existing_branch,
134+ 'branch_name')
135+ self.errors_in_action = True
136+ return
137 self.context.branch = code_import.branch
138 self.request.response.addInfoNotification(
139 'Code import created and branch linked to the '
140
141=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
142--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt 2010-04-05 18:02:00 +0000
143+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt 2010-04-13 20:37:28 +0000
144@@ -168,6 +168,7 @@
145 ... 'field.actions.update': 'Update',
146 ... }
147 >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
148+ >>> transaction.commit()
149 >>> for error in view.errors:
150 ... print error
151 >>> for notification in view.request.response.notifications:
152@@ -337,3 +338,40 @@
153
154 >>> for notification in view.request.response.notifications:
155 ... print notification.message
156+
157+Using a branch name that already exists results in an error.
158+
159+ >>> form = {
160+ ... 'field.branch_type': 'import-external',
161+ ... 'field.rcs_type': 'GIT',
162+ ... 'field.branch_name': 'chevette-branch',
163+ ... 'field.branch_owner': product.owner.name,
164+ ... 'field.repo_url': 'git://github.com/different/chevette',
165+ ... 'field.actions.update': 'Update',
166+ ... }
167+ >>> view = create_initialized_view(series, name='+setbranch',
168+ ... principal=product.owner, form=form)
169+ >>> for error in view.errors:
170+ ... print error
171+ You already have a branch for <em>Chevy</em> called <em>chevette-branch</em>.
172+
173+ >>> for notification in view.request.response.notifications:
174+ ... print notification.message
175+
176+Bazaar external branches are handled differently but they also give an
177+error if a duplicate name is used.
178+
179+ >>> form = {
180+ ... 'field.branch_type': 'import-external',
181+ ... 'field.rcs_type': 'BZR',
182+ ... 'field.branch_name': 'blazer-branch',
183+ ... 'field.branch_owner': product.owner.name,
184+ ... 'field.repo_url': 'http://bzr.com/foo',
185+ ... 'field.actions.update': 'Update',
186+ ... }
187+ >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
188+ >>> for error in view.errors:
189+ ... print error
190+ You already have a branch for <em>Chevy</em> called <em>blazer-branch</em>.
191+ >>> for notification in view.request.response.notifications:
192+ ... print notification.message