Merge lp:~thumper/launchpad/fix-deletion-syncUpdate into lp:launchpad

[Tim Penhey]

Fix deletion of branches linked as translations_branch.

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

[Jonathan Lange (jml)]

On Thu, Aug 12, 2010 at 3:06 AM, Tim Penhey <email address hidden> wrote:
> 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.
>

Thanks Tim.

Can you please define a constant in lp.testing.sampledata for
'<email address hidden>' and use that in the tests? Alternatively, use
self.factory.loginAsAnyone() or one of the login helpers.

Other than that, look good.

cheers,
jml

[Jonathan Lange (jml)]

See comment.

review: Approve

[Tim Penhey (thumper)]

On Fri, 13 Aug 2010 01:41:16 you wrote:
> On Thu, Aug 12, 2010 at 3:06 AM, Tim Penhey <email address hidden>
wrote:
> > 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.
>
> Thanks Tim.
>
> Can you please define a constant in lp.testing.sampledata for
> '<email address hidden>' and use that in the tests? Alternatively, use
> self.factory.loginAsAnyone() or one of the login helpers.

I removed the use of '<email address hidden>'

Tim

=== 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, '[e-mail]')
-        self.product = ProductSet().getByName('firefox')
-        self.user = getUtility(IPersonSet).getByEmail('[e-mail]')
+        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):