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

[Paul Hummer]

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):
    return

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.

[Jeroen T. Vermeulen (jtv)]

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

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-05-28 20:54:57 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-06-08 05:56:28 +0000
@@ -318,12 +318,14 @@
                 'recipe_text',
                 'The recipe text is not a valid bzr-builder recipe.')
 
-        if getUtility(ISourcePackageRecipeSource).exists(
-                                                    self.user, data['name']):
-            self.setFieldError(
-                'name',
-                'There is already a recipe owned by %s with this name.' %
-                    self.user.displayname)
+        name = data.get('name', None)
+        if name:
+            SourcePackageRecipeSource = getUtility(ISourcePackageRecipeSource)
+            if SourcePackageRecipeSource.exists(self.user, name):
+                self.setFieldError(
+                    'name',
+                    'There is already a recipe owned by %s with this name.' %
+                        self.user.displayname)
 
 
 class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):
 
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-03 21:13:05 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-08 05:56:28 +0000
@@ -119,6 +119,27 @@
         self.assertTextMatchesExpressionIgnoreWhitespace(
             pattern, main_text)
 
+    def test_create_new_recipe_empty_name(self):
+        # Leave off the name and make sure that the widgets validate before
+        # the content validates.
+        product = self.factory.makeProduct(
+            name='ratatouille', displayname='Ratatouille')
+        branch = self.factory.makeBranch(
+            owner=self.chef, product=product, name='veggies')
+        self.factory.makeSourcePackage()
+
+        # A new recipe can be created from the branch page.
+        browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
+        browser.getLink('Create packaging recipe').click()
+
+        browser.getControl('Description').value = 'Make some food!'
+        browser.getControl('Secret Squirrel').click()
+        browser.getControl('Create Recipe').click()
+
+        self.assertEqual(
+            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
+            'Required input is missing.')
+
     def test_create_recipe_bad_text(self):
         # If a user tries to create source package recipe with bad text, they
         # should get an error.