launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-667687 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #667687 deactivateAccount is incorrect if the person is both owner and driver of a project
  https://bugs.launchpad.net/bugs/667687


= Summary =

While researching another bug I discovered a problem with Person
deactivateAccount.  It shouldn't happen much in real life but would be
difficult to track if it did.

== Proposed fix ==

Change the testing structure to DTRT.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t test_person

== Demo and Q/A ==

Create a project.  Make the owner and the driver the same person.
Deactivate that person.  Ensure the registry experts are now the owner
and driver.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-667687/+merge/39506
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-667687 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-10-26 15:47:24 +0000
+++ lib/lp/registry/model/person.py	2010-10-28 10:31:11 +0000
@@ -2009,8 +2009,9 @@
     # person.
     def deactivateAccount(self, comment):
         """See `IPersonSpecialRestricted`."""
-        assert self.is_valid_person, (
-            "You can only deactivate an account of a valid person.")
+        if not self.is_valid_person:
+            raise AssertionError(
+                "You can only deactivate an account of a valid person.")
 
         for membership in self.team_memberships:
             self.leave(membership.team)
@@ -2045,11 +2046,15 @@
             pillar = pillar_name.pillar
             # XXX flacoste 2007-11-26 bug=164635 The comparison using id below
             # works around a nasty intermittent failure.
+            changed = False
             if pillar.owner.id == self.id:
                 pillar.owner = registry_experts
-            elif pillar.driver.id == self.id:
+                changed = True
+            if pillar.driver is not None and pillar.driver.id == self.id:
                 pillar.driver = registry_experts
-            else:
+                changed = True
+
+            if not changed:
                 # Since we removed the person from all teams, something is
                 # seriously broken here.
                 raise AssertionError(

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-10-26 15:47:24 +0000
+++ lib/lp/registry/tests/test_person.py	2010-10-28 10:31:11 +0000
@@ -30,6 +30,7 @@
     EmailAddressStatus,
     InvalidEmailAddress,
     )
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import (
@@ -248,7 +249,7 @@
         self.guadamen = person_set.getByName('guadamen')
         product_set = getUtility(IProductSet)
         self.bzr = product_set.getByName('bzr')
-        self.now = datetime.now(pytz.timezone('UTC'))
+        self.now = datetime.now(pytz.UTC)
 
     def test_deactivateAccount_copes_with_names_already_in_use(self):
         """When a user deactivates his account, its name is changed.
@@ -273,6 +274,25 @@
         foo_bar.deactivateAccount("blah!")
         self.failUnlessEqual(foo_bar.name, 'name12-deactivatedaccount1')
 
+    def test_deactivateAccountReassignsOwnerAndDriver(self):
+        """Product owner and driver are reassigned.
+
+        If a user is a product owner and/or driver, when the user is
+        deactivated the roles are assigned to the registry experts team.  Note
+        a person can have both roles and the method must handle both at once,
+        that's why this is one test.
+        """
+        user = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=user)
+        with person_logged_in(user):
+            product.driver = user
+            user.deactivateAccount("Going off the grid.")
+        registry_team = getUtility(ILaunchpadCelebrities).registry_experts
+        self.assertEqual(registry_team, product.owner,
+                         "Owner is not registry team.")
+        self.assertEqual(registry_team, product.driver,
+                         "Driver is not registry team.")
+
     def test_getDirectMemberIParticipateIn(self):
         sample_person = Person.byName('name12')
         warty_team = Person.byName('name20')