launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/view-invalid-recipes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #620868 OOPS traversing to a recipe with validation issues
  https://bugs.launchpad.net/bugs/620868

For more details, see:
https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037

Due to changes in the bzr builder validation rules, it is possible,
and has happened, where previously valid build recipes become
invalid.  The code that was displaying the recipe text was first
validating the recipe, which was causing the oops.

The change uses the unvalidated recipe text for displaying on
the recipe page, and for the setting of the initial edit text.

Tests were added to show that both the index and edit views
render for broken recipes.

The browser test case was extended slightly to allow a specific
user to be passed in rather than using the class one.

-- 
https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/view-invalid-recipes into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-12-22 02:01:36 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-01-12 20:38:57 +0000
@@ -496,7 +496,7 @@
     def initial_values(self):
         return {
             'distros': self.context.distroseries,
-            'recipe_text': str(self.context.builder_recipe),
+            'recipe_text': self.context.recipe_text,
             }
 
     @property

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-12-22 02:01:36 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-01-12 20:38:57 +0000
@@ -1343,3 +1343,45 @@
         self.assertRaises(
             Unauthorized,
             self.getUserBrowser, recipe_url + '/+delete', user=nopriv_person)
+
+
+class TestBrokenExistingRecipes(BrowserTestCase):
+    """Existing recipes broken by builder updates need to be editable.
+
+    This happened with a 0.2 -> 0.3 release where the nest command was no
+    longer allowed to refer the '.'.  There were already existing recipes that
+    had this text that were not viewable or editable.  This test case captures
+    that and makes sure the views stay visible.
+    """
+
+    layer = LaunchpadFunctionalLayer
+
+    def makeBrokenRecipe(self):
+        """Make a valid recipe, then break it."""
+        product = self.factory.makeProduct()
+        b1 = self.factory.makeProductBranch(product=product)
+        b2 = self.factory.makeProductBranch(product=product)
+        recipe_text = dedent("""\
+            # bzr-builder format 0.2 deb-version {debupstream}+{revno}
+            %s
+            nest name %s foo
+            """ % (b1.bzr_identity, b2.bzr_identity))
+        recipe = self.factory.makeSourcePackageRecipe(recipe=recipe_text)
+        naked_data = removeSecurityProxy(recipe)._recipe_data
+        nest_instruction = list(naked_data.instructions)[0]
+        nest_instruction.directory = u'.'
+        return recipe
+
+    def test_recipe_is_broken(self):
+        recipe = self.makeBrokenRecipe()
+        self.assertRaises(Exception, str, recipe.builder_recipe)
+
+    def test_recipe_index_renderable(self):
+        recipe = self.makeBrokenRecipe()
+        main_text = self.getMainText(recipe, '+index')
+        self.assertTrue('# bzr-builder format 0.2 deb-version' in main_text)
+
+    def test_recipe_edit_renderable(self):
+        recipe = self.makeBrokenRecipe()
+        main_text = self.getMainText(recipe, '+edit', user=recipe.owner)
+        self.assertTrue('# bzr-builder format 0.2 deb-version' in main_text)

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2010-12-15 14:07:17 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-01-12 20:38:57 +0000
@@ -154,7 +154,7 @@
 
     @property
     def recipe_text(self):
-        return str(self.builder_recipe)
+        return self.builder_recipe.get_recipe_text()
 
     @staticmethod
     def new(registrant, owner, name, recipe, description,

=== modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
--- lib/lp/code/templates/sourcepackagerecipe-index.pt	2010-11-30 12:53:20 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-index.pt	2011-01-12 20:38:57 +0000
@@ -168,7 +168,7 @@
       </div>
       <div class='portlet'>
         <h2>Recipe contents</h2>
-        <pre tal:content="context/builder_recipe" />
+        <pre tal:content="context/recipe_text" />
       </div>
     </div>
   </div>

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-12-30 23:45:34 +0000
+++ lib/lp/testing/__init__.py	2011-01-12 20:38:57 +0000
@@ -723,7 +723,9 @@
         self.user = self.factory.makePerson(password='test')
 
     def getViewBrowser(self, context, view_name=None, no_login=False,
-                       rootsite=None):
+                       rootsite=None, user=None):
+        if user is None:
+            user = self.user
         login(ANONYMOUS)
         url = canonical_url(context, view_name=view_name ,rootsite=rootsite)
         logout()
@@ -733,22 +735,23 @@
             browser.open(url)
             return browser
         else:
-            return self.getUserBrowser(url, self.user)
+            return self.getUserBrowser(url, user)
 
     def getMainContent(self, context, view_name=None, rootsite=None,
-                       no_login=False):
+                       no_login=False, user=None):
         """Beautiful soup of the main content area of context's page."""
         from canonical.launchpad.testing.pages import find_main_content
         browser = self.getViewBrowser(
-            context, view_name, rootsite=rootsite, no_login=no_login)
+            context, view_name, rootsite=rootsite, no_login=no_login,
+            user=user)
         return find_main_content(browser.contents)
 
     def getMainText(self, context, view_name=None, rootsite=None,
-                    no_login=False):
+                    no_login=False, user=None):
         """Return the main text of a context's page."""
         from canonical.launchpad.testing.pages import extract_text
         return extract_text(
-            self.getMainContent(context, view_name, rootsite, no_login))
+            self.getMainContent(context, view_name, rootsite, no_login, user))
 
 
 class WindmillTestCase(TestCaseWithFactory):