launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/fix-deletion-syncUpdate into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #599259 ForbiddenAttribute: syncUpdate
  https://bugs.launchpad.net/bugs/599259


This branch fixes a bug stopping branches that have been linked to product series as translations branches from being deleted.

The guts of the issue was the code that cleared the translations_branch attribute of the product series didn't work - because it was clearing the "branch" attribute, not the "translations_branch".  A case here of missing tests.

I added the test, and also fixed up the model class to only flush to the DB once at the end
of the _breakReference() call.

As part of the cleanup, I also moved the Store.flush() call into a test cleanup method that gets run for every test, and removed the use of sample data.

tests:
  TestBranchDeletion
-- 
https://code.launchpad.net/~thumper/launchpad/fix-deletion-syncUpdate/+merge/32410
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/fix-deletion-syncUpdate into lp:launchpad/devel.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-08-02 02:36:32 +0000
+++ lib/lp/code/model/branch.py	2010-08-12 02:06:49 +0000
@@ -646,6 +646,7 @@
         # actually a very interesting thing to tell the user about.
         if self.code_import is not None:
             DeleteCodeImport(self.code_import)()
+        Store.of(self).flush()
 
     def associatedProductSeries(self):
         """See `IBranch`."""
@@ -1129,7 +1130,6 @@
 
     def __call__(self):
         self.affected_object.prerequisite_branch = None
-        Store.of(self.affected_object).flush()
 
 
 class ClearSeriesBranch(DeletionOperation):
@@ -1143,7 +1143,6 @@
     def __call__(self):
         if self.affected_object.branch == self.branch:
             self.affected_object.branch = None
-        Store.of(self.affected_object).flush()
 
 
 class ClearSeriesTranslationsBranch(DeletionOperation):
@@ -1156,9 +1155,8 @@
         self.branch = branch
 
     def __call__(self):
-        if self.affected_object.branch == self.branch:
-            self.affected_object.branch = None
-        self.affected_object.syncUpdate()
+        if self.affected_object.translations_branch == self.branch:
+            self.affected_object.translations_branch = None
 
 
 class ClearOfficialPackageBranch(DeletionOperation):

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-08-02 02:36:32 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-08-12 02:06:49 +0000
@@ -931,15 +931,18 @@
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        TestCaseWithFactory.setUp(self, 'test@xxxxxxxxxxxxx')
-        self.product = ProductSet().getByName('firefox')
-        self.user = getUtility(IPersonSet).getByEmail('test@xxxxxxxxxxxxx')
+        TestCaseWithFactory.setUp(self)
+        self.user = self.factory.makePerson()
+        self.product = self.factory.makeProduct(owner=self.user)
         self.branch = self.factory.makeProductBranch(
             name='to-delete', owner=self.user, product=self.product)
         # The owner of the branch is subscribed to the branch when it is
         # created.  The tests here assume no initial connections, so
         # unsubscribe the branch owner here.
         self.branch.unsubscribe(self.branch.owner, self.branch.owner)
+        # Make sure that the tests all flush the database changes.
+        self.addCleanup(Store.of(self.branch).flush)
+        login_person(self.user)
 
     def test_deletable(self):
         """A newly created branch can be deleted without any problems."""
@@ -1077,10 +1080,14 @@
         # remove the Job and BranchJob.
         branch = self.factory.makeAnyBranch()
         getUtility(ITranslationTemplatesBuildJobSource).create(branch)
-
         branch.destroySelf(break_references=True)
 
-        Store.of(branch).flush()
+    def test_linked_translations_branch_cleared(self):
+        # The translations_branch of a series that is linked to the branch
+        # should be cleared.
+        dev_focus = self.branch.product.development_focus
+        dev_focus.translations_branch = self.branch
+        self.branch.destroySelf(break_references=True)
 
     def test_unrelated_TranslationTemplatesBuildJob_intact(self):
         # No innocent BuildQueue entries are harmed in deleting a
@@ -1120,21 +1127,15 @@
     def test_destroySelf_with_SourcePackageRecipe(self):
         """If branch is a base_branch in a recipe, it is deleted."""
         recipe = self.factory.makeSourcePackageRecipe()
-        store = Store.of(recipe)
         recipe.base_branch.destroySelf(break_references=True)
-        # show no DB constraints have been violated
-        store.flush()
 
     def test_destroySelf_with_SourcePackageRecipe_as_non_base(self):
         """If branch is referred to by a recipe, it is deleted."""
         branch1 = self.factory.makeAnyBranch()
         branch2 = self.factory.makeAnyBranch()
-        recipe = self.factory.makeSourcePackageRecipe(
+        self.factory.makeSourcePackageRecipe(
             branches=[branch1, branch2])
-        store = Store.of(recipe)
         branch2.destroySelf(break_references=True)
-        # show no DB constraints have been violated
-        store.flush()
 
 
 class TestBranchDeletionConsequences(TestCase):