Merge lp:~stevenk/launchpad/set-recipe-text-bad-data into lp:launchpad

[Steve Kowalik]

[r=lifeless][bug=592513,683321] Don't OOPS if an invalid branch is used while editing a recipe.

Fix bug 683321, 'Editing recipe oopses if invalid branch used'. The real error was that the request_action for creating recipes dealt with four exceptions, but request_action for editing recipes only dealt with three. This means if the branch passed in was invalid, the exception wouldn't be caught and would leak up the stack, causing the OOPS.

I have fixed this by refactoring the exception handler to a parent class of both AddView and EditView, RecipeTextValidatorMixin. While performing this refactoring, I fixed bug 592513 by accident and removed the XXX.

I added a test that checks the error is raised correctly when a recipe is edited and is submitted with a branch that doesn't exist.

[Robert Collins (lifeless)]

Broadly good; I have some tweaks that may help you.

Firstly, a docstring on the method might be nice, its not clear why you only accept *args, for instance.

Secondly to handle the fact that an error occured, you might like to raise a specific exception instead of returning None.

thusly:
class ErrorHandled(Exception):
    """A field error occured and was handled."""

try:
    self.error_handler(...)
except ErrorHandled:
    return

that, to me, would make it clearer than checking for 'is None'. What do you think?

review: Approve

[Robert Collins (lifeless)]

To clarify what I mean, the error_handler guts would then be
+ try:
+ return callable(*args)
+ except TooNewRecipeFormat:
....
raise ErrorHandled()

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-02-14 00:58:45 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-15 11:35:12 +0000
@@ -399,6 +399,10 @@
                           "the owner of the recipe ."))
 
 
+class ErrorHandled(Exception):
+    """A field error occured and was handled."""
+
+
 class RecipeTextValidatorMixin:
     """Class to validate that the Source Package Recipe text is valid."""
 
@@ -414,6 +418,25 @@
         except RecipeParseError, error:
             self.setFieldError('recipe_text', str(error))
 
+    def error_handler(self, callable, *args, **kwargs):
+        try:
+            return callable(*args)
+        except TooNewRecipeFormat:
+            self.setFieldError(
+                'recipe_text',
+                'The recipe format version specified is not available.')
+        except ForbiddenInstructionError, e:
+            self.setFieldError(
+                'recipe_text',
+                'The bzr-builder instruction "%s" is not permitted '
+                'here.' % e.instruction_name)
+        except NoSuchBranch, e:
+            self.setFieldError(
+                'recipe_text', '%s is not a branch on Launchpad.' % e.name)
+        except PrivateBranchRecipe, e:
+            self.setFieldError('recipe_text', str(e))
+        raise ErrorHandled()
+
 
 class RelatedBranchesWidget(Widget):
     """A widget to render the related branches for a recipe."""
@@ -524,36 +547,20 @@
 
     @action('Create Recipe', name='create')
     def request_action(self, action, data):
+        owner = data['owner']
+        if data['use_ppa'] == CREATE_NEW:
+            ppa_name = data.get('ppa_name', None)
+            ppa = owner.createPPA(ppa_name)
+        else:
+            ppa = data['daily_build_archive']
         try:
-            owner = data['owner']
-            if data['use_ppa'] == CREATE_NEW:
-                ppa_name = data.get('ppa_name', None)
-                ppa = owner.createPPA(ppa_name)
-            else:
-                ppa = data['daily_build_archive']
-            source_package_recipe = getUtility(
-                ISourcePackageRecipeSource).new(
-                    self.user, owner, data['name'],
-                    data['recipe_text'], data['description'], data['distros'],
-                    ppa, data['build_daily'])
+            source_package_recipe = self.error_handler(
+                getUtility(ISourcePackageRecipeSource).new,
+                self.user, owner, data['name'],
+                data['recipe_text'], data['description'], data['distros'],
+                ppa, data['build_daily'])
             Store.of(source_package_recipe).flush()
-        except TooNewRecipeFormat:
-            self.setFieldError(
-                'recipe_text',
-                'The recipe format version specified is not available.')
-            return
-        except ForbiddenInstructionError:
-            # XXX: bug=592513 We shouldn't be hardcoding "run" here.
-            self.setFieldError(
-                '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
-        except PrivateBranchRecipe, e:
-            self.setFieldError('recipe_text', str(e))
+        except ErrorHandled:
             return
 
         self.next_url = canonical_url(source_package_recipe)
@@ -638,24 +645,10 @@
         recipe = parser.parse()
         if self.context.builder_recipe != recipe:
             try:
-                self.context.setRecipeText(recipe_text)
+                self.error_handler(self.context.setRecipeText, recipe_text)
                 changed = True
-            except TooNewRecipeFormat:
-                self.setFieldError(
-                    'recipe_text',
-                    'The recipe format version specified is not available.')
-                return
-            except ForbiddenInstructionError:
-                # XXX: bug=592513 We shouldn't be hardcoding "run" here.
-                self.setFieldError(
-                    'recipe_text',
-                    'The bzr-builder instruction "run" is not permitted'
-                    ' here.')
-                return
-            except PrivateBranchRecipe, e:
-                self.setFieldError('recipe_text', str(e))
-                return
-
+            except ErrorHandled:
+                return
 
         distros = data.pop('distros')
         if distros != self.context.distroseries:

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-14 07:47:07 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-15 11:35:12 +0000
@@ -986,6 +986,19 @@
             get_message_text(browser, 1),
             'Recipe may not refer to private branch: %s' % bzr_identity)
 
+    def test_edit_recipe_no_branch(self):
+        # If a user tries to set a source package recipe to use a branch
+        # that isn't registred, they will get an error.
+        recipe = self.factory.makeSourcePackageRecipe(owner=self.user)
+        no_branch_recipe_text = recipe.recipe_text[:-4]
+        expected_name = recipe.base_branch.unique_name[:-3]
+        browser = self.getViewBrowser(recipe, '+edit')
+        browser.getControl('Recipe text').value = no_branch_recipe_text
+        browser.getControl('Update Recipe').click()
+        self.assertEqual(
+            get_message_text(browser, 1),
+            'lp://dev/%s is not a branch on Launchpad.' % expected_name)
+
     def _test_edit_recipe_with_no_related_branches(self, recipe):
         # The Related Branches section should not appear if there are no
         # related branches.