Merge lp:~abentley/launchpad/bad-branch-name into lp:launchpad

[Aaron Bentley]

Treat bad branch location in recipe as field error

= Summary =
Fix bug #592792: bad branch names in recipes produce oopses

== Proposed fix ==
Catch NoSuchBranch when creating the SourcePackageRecipe and set a suitable
field error.

== Pre-implementation notes ==
None

== Implementation details ==
Extracted createRecipe so it could be reused.

Implemented bad branch handling for SourcePackageRecipeDataInstruction.

== Tests ==
bin/test -t test_create_recipe_bad_base_branch -t test_create_recipe_bad_instruction_branch

== Demo and Q/A ==
Attempt to create a recipe for a base branch that doesn't exist. This should
produce a validation error.

Attempt to create a recipe that merges a branchh that doesn't exist. This
should produce 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/code/model/sourcepackagerecipedata.py
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

== Pyflakes notices ==

lib/lp/code/browser/tests/test_sourcepackagerecipe.py
    283: local variable 'recipe_fings' is assigned to but never used

[Māris Fogels (mars)]

Hi Aaron,

This change looks good. Some questions:

  * Would the text "No such branch: foo" be clearer for our users than "Unknown branch: foo"? I personally find the meaning of "No such X" clearer than "Unknown X".

  * I see two convoluted calls to "extract_text(find_tags_by_class(browser.contents, 'message')[1])" in this diff. Should it be extracted into a nice helper function like get_page_message()?

Otherwise, this looks good to me. r=mars

Maris

review: Approve

[Aaron Bentley (abentley)]

Updated per our discussion.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-06-12 13:34:11 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-06-15 15:10:56 +0000
@@ -40,6 +40,7 @@
 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.code.errors import ForbiddenInstruction
+from lp.code.interfaces.branch import NoSuchBranch
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
 from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -364,6 +365,10 @@
                 'recipe_text',
                 'The bzr-builder instruction "run" is not permitted here.')
             return
+        except NoSuchBranch, e:
+            self.setFieldError(
+                'recipe_text', '%s is not a branch on Launchpad.' % e.name)
+            return
 
         self.next_url = canonical_url(source_package_recipe)
 
 
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-12 13:52:31 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-15 15:10:56 +0000
@@ -65,6 +65,12 @@
         return extract_text(find_main_content(browser.contents))
 
 
+def get_message_text(browser, index):
+    """Return the text of a message, specified by index."""
+    tags = find_tags_by_class(browser.contents, 'message')[index]
+    return extract_text(tags)
+
+
 class TestSourcePackageRecipeAddView(TestCaseForRecipe):
 
     layer = DatabaseFunctionalLayer
@@ -142,7 +148,7 @@
         browser.getControl('Create Recipe').click()
 
         self.assertEqual(
-            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
+            get_message_text(browser, 1),
             'The bzr-builder instruction "run" is not permitted here.')
 
     def test_create_new_recipe_empty_name(self):
@@ -162,30 +168,53 @@
         browser.getControl('Create Recipe').click()
 
         self.assertEqual(
-            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
-            'Required input is missing.')
+            get_message_text(browser, 1), 'Required input is missing.')
+
+    def createRecipe(self, recipe_text, branch=None):
+        if branch is None:
+            product = self.factory.makeProduct(
+                name='ratatouille', displayname='Ratatouille')
+            branch = self.factory.makeBranch(
+                owner=self.chef, product=product, name='veggies')
+
+        # 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(name='field.name').value = 'daily'
+        browser.getControl('Description').value = 'Make some food!'
+        browser.getControl('Recipe text').value = recipe_text
+        browser.getControl('Create Recipe').click()
+        return browser
 
     def test_create_recipe_bad_text(self):
         # If a user tries to create source package recipe with bad text, they
         # should get an error.
-        product = self.factory.makeProduct(
-            name='ratatouille', displayname='Ratatouille')
-        branch = self.factory.makeBranch(
-            owner=self.chef, product=product, name='veggies')
-
-        # 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(name='field.name').value = 'daily'
-        browser.getControl('Description').value = 'Make some food!'
-        browser.getControl('Recipe text').value = 'Foo bar baz'
-        browser.getControl('Create Recipe').click()
-
+        browser = self.createRecipe('Foo bar baz')
         self.assertEqual(
-            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
+            get_message_text(browser, 1),
             'The recipe text is not a valid bzr-builder recipe.')
 
+    def test_create_recipe_bad_base_branch(self):
+        # If a user tries to create source package recipe with a bad base
+        # branch location, they should get an error.
+        browser = self.createRecipe(MINIMAL_RECIPE_TEXT % 'foo')
+        self.assertEqual(
+            get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
+
+    def test_create_recipe_bad_instruction_branch(self):
+        # If a user tries to create source package recipe with a bad
+        # instruction branch location, they should get an error.
+        product = self.factory.makeProduct(
+            name='ratatouille', displayname='Ratatouille')
+        branch = self.factory.makeBranch(
+            owner=self.chef, product=product, name='veggies')
+        recipe = MINIMAL_RECIPE_TEXT % branch.bzr_identity
+        recipe += 'nest packaging foo debian'
+        browser = self.createRecipe(recipe, branch)
+        self.assertEqual(
+            get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
+
     def test_create_dupe_recipe(self):
         # You shouldn't be able to create a duplicate recipe owned by the same
         # person with the same name.
@@ -206,7 +235,7 @@
         browser.getControl('Create Recipe').click()
 
         self.assertEqual(
-            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
+            get_message_text(browser, 1),
             'There is already a recipe owned by Master Chef with this name.')
 
 
 
=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2010-06-11 04:28:38 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2010-06-15 15:10:56 +0000
@@ -211,6 +211,8 @@
             line_number += 1
             comment = None
             db_branch = branch_map[instruction.recipe_branch.url]
+            if db_branch is None:
+                raise NoSuchBranch(instruction.recipe_branch.url)
             insn = _SourcePackageRecipeDataInstruction(
                 instruction.recipe_branch.name, type, comment,
                 line_number, db_branch, instruction.recipe_branch.revspec,