Merge lp:~abentley/launchpad/retain-spr-builds into lp:launchpad/db-devel

[Aaron Bentley]

NULL recipes in recipe builds on deletion.

= Summary =
Fix bug #645620: Deleting recipe leaves SourcePackageReleases with no
traceability

== Proposed fix ==
Relax the not-null constraint for recipe on sourcepackagerecipebuild. NULL
sourcepackagerecipebuilds on recipe deletion instead of deleting them.

== Pre-implementation notes ==
Discussed with thumper

== Implementation details ==
None

== Tests ==
bin/test -t test_destroySelf_retains_build -t test_destroySelf_preserves_release

== Demo and Q/A ==
Cannot be demoed because sourcepackagerecipebuilds do not have a valid URL if
their recipe is deleted.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/interfaces/sourcepackagerecipebuild.py
  lib/canonical/launchpad/security.py
  database/schema/patch-2208-90-0.sql
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/code/model/sourcepackagerecipebuild.py
  lib/lp/code/model/sourcepackagerecipe.py

./lib/canonical/launchpad/security.py
     721: E302 expected 2 blank lines, found 1
    1256: E302 expected 2 blank lines, found 1
    1466: E302 expected 2 blank lines, found 1

[Tim Penhey (thumper)]

Looks good. Just one small point: when clearing out the recipe value from the builds, we should use a set based update rather than iterating over each one.

review: Approve

[Stuart Bishop (stub)]

Fine. patch-2208-28-0.sql

review: Approve (db)

=== added file 'database/schema/patch-2208-28-0.sql'
--- database/schema/patch-2208-28-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-28-0.sql	2010-11-17 19:49:40 +0000
@@ -0,0 +1,7 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+ALTER TABLE SourcePackageRecipeBuild ALTER COLUMN recipe DROP NOT NULL;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 28, 0);

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-11-12 23:30:57 +0000
+++ lib/canonical/launchpad/security.py	2010-11-17 19:49:40 +0000
@@ -2284,7 +2284,8 @@
     usedfor = ISourcePackageRecipeBuild
 
     def iter_objects(self):
-        yield self.obj.recipe
+        if self.obj.recipe is not None:
+            yield self.obj.recipe
         yield self.obj.archive
 
 

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipebuild.py'
--- lib/lp/code/interfaces/sourcepackagerecipebuild.py	2010-08-27 02:55:05 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipebuild.py	2010-11-17 19:49:40 +0000
@@ -58,8 +58,7 @@
         title=_("The person who wants this to be done."))
 
     recipe = Object(
-        schema=ISourcePackageRecipe, required=True,
-        title=_("The recipe being built."))
+        schema=ISourcePackageRecipe, title=_("The recipe being built."))
 
     manifest = Object(
         schema=ISourcePackageRecipeData, title=_(

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2010-11-04 06:36:29 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2010-11-17 19:49:40 +0000
@@ -28,7 +28,6 @@
     implements,
     )
 
-from canonical.config import config
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.launchpad.interfaces.lpstorm import (
     IMasterStore,
@@ -198,13 +197,9 @@
         store = Store.of(self)
         self.distroseries.clear()
         self._recipe_data.instructions.find().remove()
-
-        def destroyBuilds(pending):
-            builds = self.getBuilds(pending=pending)
-            for build in builds:
-                build.destroySelf()
-        destroyBuilds(pending=True)
-        destroyBuilds(pending=False)
+        builds = store.find(
+            SourcePackageRecipeBuild, SourcePackageRecipeBuild.recipe==self)
+        builds.set(recipe_id=None)
         store.remove(self._recipe_data)
         store.remove(self)
 

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-10-22 20:50:50 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-11-17 19:49:40 +0000
@@ -109,7 +109,7 @@
 
     is_virtualized = True
 
-    recipe_id = Int(name='recipe', allow_none=False)
+    recipe_id = Int(name='recipe')
     recipe = Reference(recipe_id, 'SourcePackageRecipe.id')
 
     manifest = Reference(

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-11-08 01:08:15 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-11-17 19:49:40 +0000
@@ -51,7 +51,10 @@
     NonPPABuildRequest,
     SourcePackageRecipe,
     )
-from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuildJob
+from lp.code.model.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuild,
+    SourcePackageRecipeBuildJob,
+    )
 from lp.code.tests.helpers import recipe_parser_newest_version
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.interfaces.job import (
@@ -484,7 +487,7 @@
         # Show no database constraints were violated
         Store.of(recipe).flush()
 
-    def test_destroySelf_clears_release(self):
+    def test_destroySelf_preserves_release(self):
         # Destroying a sourcepackagerecipe removes references to its builds
         # from their releases.
         recipe = self.factory.makeSourcePackageRecipe()
@@ -494,7 +497,28 @@
         self.assertEqual(build, release.source_package_recipe_build)
         with person_logged_in(recipe.owner):
             recipe.destroySelf()
-        self.assertIs(None, release.source_package_recipe_build)
+        self.assertIsNot(None, release.source_package_recipe_build)
+
+    def test_destroySelf_retains_build(self):
+        # Destroying a sourcepackagerecipe removes references to its builds
+        # from their releases.
+        recipe = self.factory.makeSourcePackageRecipe()
+        build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
+        store = Store.of(build)
+        store.flush()
+        build_id = build.id
+        build = store.find(
+            SourcePackageRecipeBuild,
+            SourcePackageRecipeBuild.id == build_id).one()
+        self.assertIsNot(None, build)
+        self.assertEqual(recipe, build.recipe)
+        with person_logged_in(recipe.owner):
+            recipe.destroySelf()
+        build = store.find(
+            SourcePackageRecipeBuild,
+            SourcePackageRecipeBuild.id == build_id).one()
+        self.assertIsNot(None, build)
+        self.assertIs(None, build.recipe)
         transaction.commit()
 
     def test_findStaleDailyBuilds(self):