launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/recipe-errors into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #683321 Editing recipe oopses if invalid branch used
  https://bugs.launchpad.net/bugs/683321


= Summary =
Fix bug #683321: Editing recipe oopses if invalid branch used

== Proposed fix ==
Unify error handling into a context manager

== Pre-implementation notes ==
None

== Implementation details ==
With this context manager, two operations (setRecipeText and
ISourcePackageRecipeSource).new) that produce the same exceptions have them
handled the same way.  This ensures that adding and editing recipes has uniform
exception handling.

== Tests ==
bin/test -t edit_recipe_bad_base_branch sourcepackagerecipe

== Demo and Q/A ==
Create a recipe.  Edit it to set the branch to an invalid value.  You should
get an error message, not an OOPS.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

./lib/lp/code/browser/sourcepackagerecipe.py
     338: Line exceeds 78 characters.
./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
     370: E501 line too long (80 characters)
     752: E501 line too long (85 characters)
     766: E501 line too long (89 characters)
     778: E501 line too long (85 characters)
     793: E501 line too long (89 characters)
     809: E501 line too long (85 characters)
     370: Line exceeds 78 characters.
     752: Line exceeds 78 characters.
     766: Line exceeds 78 characters.
     778: Line exceeds 78 characters.
     793: Line exceeds 78 characters.
     796: Line exceeds 78 characters.
     809: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~abentley/launchpad/recipe-errors/+merge/42305
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/recipe-errors into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-11-28 23:32:25 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-11-30 20:43:10 +0000
@@ -315,6 +315,38 @@
         except RecipeParseError, error:
             self.setFieldError('recipe_text', str(error))
 
+    class HandleRecipeErrors():
+        """Handle errors that may be encountered setting a new recipe text."""
+
+        def __init__(self, view):
+            self.view = view
+            self.return_now = False
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, exc_type, exc_val, exc_tb):
+            if exc_type is None:
+                return True
+            elif isinstance(exc_val, TooNewRecipeFormat):
+                self.view.setFieldError(
+                    'recipe_text',
+                    'The recipe format version specified is not available.')
+            elif isinstance(exc_val, ForbiddenInstructionError):
+                self.view.setFieldError(
+                    'recipe_text',
+                    'The bzr-builder instruction "run" is not permitted here.')
+            elif isinstance(exc_val, NoSuchBranch):
+                self.view.setFieldError(
+                    'recipe_text', '%s is not a branch on Launchpad.' %
+                    exc_val.name)
+            elif isinstance(exc_val, PrivateBranchRecipe):
+                self.view.setFieldError('recipe_text', str(exc_val))
+            else:
+                return False
+            self.return_now = True
+            return True
+
 
 class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):
     """View for creating Source Package Recipes."""
@@ -344,30 +376,14 @@
 
     @action('Create Recipe', name='create')
     def request_action(self, action, data):
-        try:
+        with self.HandleRecipeErrors(self) as handler:
             source_package_recipe = getUtility(
                 ISourcePackageRecipeSource).new(
                     self.user, data['owner'], data['name'],
                     data['recipe_text'], data['description'], data['distros'],
                     data['daily_build_archive'], 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))
+        if handler.return_now:
             return
 
         self.next_url = canonical_url(source_package_recipe)
@@ -437,25 +453,11 @@
         parser = RecipeParser(recipe_text)
         recipe = parser.parse()
         if self.context.builder_recipe != recipe:
-            try:
+            with self.HandleRecipeErrors(self) as 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
-
+            if handler.return_now:
+                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	2010-11-28 22:38:48 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-11-30 20:43:10 +0000
@@ -557,6 +557,17 @@
             extract_text(find_tags_by_class(browser.contents, 'message')[1]),
             'The bzr-builder instruction "run" is not permitted here.')
 
+    def test_edit_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.
+        recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)
+        browser = self.getViewBrowser(recipe, '+edit')
+        browser.getControl('Recipe text').value = (
+            MINIMAL_RECIPE_TEXT % 'foo')
+        browser.getControl('Update Recipe').click()
+        self.assertEqual(
+            get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
+
     def test_edit_recipe_format_too_new(self):
         # If the recipe's format version is too new, we should notify the
         # user.