Merge lp:~rockstar/launchpad/bug-589454 into lp:launchpad

Proposed by Paul Hummer
Status: Merged
Merged at revision: 10972
Proposed branch: lp:~rockstar/launchpad/bug-589454
Merge into: lp:launchpad
Diff against target: 56 lines (+29/-6)
2 files modified
lib/lp/code/browser/ (+8/-6)
lib/lp/code/browser/tests/ (+21/-0)
To merge this branch: bzr merge lp:~rockstar/launchpad/bug-589454
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email:

Description of the change

This branch fixes bug #589454. The problem is that LaunchpadFormView._validate gathers errors from widget validation and then continues on to gather errors in LaunchpadFormView.validate. The problem is that if the validate method relies on the widgets being valid, you'll get an oops from the validate method.

Curtis suggested I do something like:

if len(data.errors):

I didn't like that solution, since it's not allowing for other validation that can and should also occur. The way I wrote it, if name isn't set, it shows validation errors, and if name is a duplicate, then it shows that error. This all happens without effecting other validation code.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

We discussed a few style improvements on IRC:
 * Keeping the ISourcePackageRecipeSource utility in a variable to avoid breaking the "if" line in an awkward way.
 * Double-quoting rather than single-quoting stirngs that may contain apostrophes.
 * A single data.get('name') instead of separate existence check and retrieval.

Unfortunately there seems to be no easy way to ask the test browser for the error message associated with the "name" field, so we'll continue to find them by class and then use list indexing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/'
--- lib/lp/code/browser/ 2010-05-28 20:54:57 +0000
+++ lib/lp/code/browser/ 2010-06-08 05:56:28 +0000
@@ -318,12 +318,14 @@
318 'recipe_text',318 'recipe_text',
319 'The recipe text is not a valid bzr-builder recipe.')319 'The recipe text is not a valid bzr-builder recipe.')
321 if getUtility(ISourcePackageRecipeSource).exists(321 name = data.get('name', None)
322 self.user, data['name']):322 if name:
323 self.setFieldError(323 SourcePackageRecipeSource = getUtility(ISourcePackageRecipeSource)
324 'name',324 if SourcePackageRecipeSource.exists(self.user, name):
325 'There is already a recipe owned by %s with this name.' %325 self.setFieldError(
326 self.user.displayname)326 'name',
327 'There is already a recipe owned by %s with this name.' %
328 self.user.displayname)
329class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):331class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):
=== modified file 'lib/lp/code/browser/tests/'
--- lib/lp/code/browser/tests/ 2010-06-03 21:13:05 +0000
+++ lib/lp/code/browser/tests/ 2010-06-08 05:56:28 +0000
@@ -119,6 +119,27 @@
119 self.assertTextMatchesExpressionIgnoreWhitespace(119 self.assertTextMatchesExpressionIgnoreWhitespace(
120 pattern, main_text)120 pattern, main_text)
122 def test_create_new_recipe_empty_name(self):
123 # Leave off the name and make sure that the widgets validate before
124 # the content validates.
125 product = self.factory.makeProduct(
126 name='ratatouille', displayname='Ratatouille')
127 branch = self.factory.makeBranch(
128 owner=self.chef, product=product, name='veggies')
129 self.factory.makeSourcePackage()
131 # A new recipe can be created from the branch page.
132 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
133 browser.getLink('Create packaging recipe').click()
135 browser.getControl('Description').value = 'Make some food!'
136 browser.getControl('Secret Squirrel').click()
137 browser.getControl('Create Recipe').click()
139 self.assertEqual(
140 extract_text(find_tags_by_class(browser.contents, 'message')[1]),
141 'Required input is missing.')
122 def test_create_recipe_bad_text(self):143 def test_create_recipe_bad_text(self):
123 # If a user tries to create source package recipe with bad text, they144 # If a user tries to create source package recipe with bad text, they
124 # should get an error.145 # should get an error.