launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/cleanup-merged-people-1 into lp:launchpad.

Commit message:
clean up merged people data.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #204002 in Launchpad itself: "can't approve/decline a member with a merged user"
  https://bugs.launchpad.net/launchpad/+bug/204002
  Bug #393914 in Launchpad itself: "deleted or merged persons/teams can have memberships left over which cannot be revoked"
  https://bugs.launchpad.net/launchpad/+bug/393914

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/cleanup-merged-people-1/+merge/137700

Merged teams and users continue to have relationships because some data
could not be transferred or deleted during the merge process, because
there are race conditions. TeamMembership records continue to exist for
example and they cause 404's because they are listed in pending,
invited, and former members. In the case or merged private-teams, the
+members page will raise a 403 error for team admins.


RULES

    Pre-implementation: lifeless and stub
    * Create a daily garbo job that deletes TeamMembership rows for
      merged people.
    * This could also do the same for deactivated users or user who
      are suspended for more than 30 days.
    * A successful implementation could also be extended to remove
      archive, bug, blueprint, branch, and question subscriptions.


QA

    * Ask a webops to run ./cronscripts/garbo-hourly.py for qastaging
      (wampee).
    * Visit https://qastaging.launchpad.net/~launchpad-users/+members
      and step to the second batch of Former members
    * Verify that easter_egg (a.j-merged) is not listed among the former
      members.


LINT

    database/schema/security.cfg
    lib/lp/scripts/garbo.py
    lib/lp/scripts/tests/test_garbo.py


LoC

    I have more than 10,000 lines of credit this week.


TEST

    ./bin/test -vvc -t TeamMembershipPruner lp.scripts.tests.test_garbo


IMPLEMENTATION

I added a garbo job that deletes all the membership records for merged
users and teams. There are about 176 TeamMemberships that were left
behind by merge. The TeamParticipation rules already discounted the
merged users, so it is just the TM rows that are causing 403 and 404
issues for team admins.
    database/schema/security.cfg
    lib/lp/scripts/garbo.py
    lib/lp/scripts/tests/test_garbo.py
-- 
https://code.launchpad.net/~sinzui/launchpad/cleanup-merged-people-1/+merge/137700
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/cleanup-merged-people-1 into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-11-30 10:15:26 +0000
+++ database/schema/security.cfg	2012-12-03 22:09:24 +0000
@@ -2264,6 +2264,7 @@
 public.sourcepackagerelease             = SELECT
 public.sourcepackagepublishinghistory   = SELECT, UPDATE
 public.suggestivepotemplate             = INSERT, DELETE
+public.teammembership                   = SELECT, DELETE
 public.teamparticipation                = SELECT, DELETE
 public.translationmessage               = SELECT, DELETE
 public.translationtemplateitem          = SELECT, DELETE

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-11-26 08:33:03 +0000
+++ lib/lp/scripts/garbo.py	2012-12-03 22:09:24 +0000
@@ -70,6 +70,7 @@
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
+from lp.registry.model.teammembership import TeamMembership
 from lp.services.config import config
 from lp.services.database import postgresql
 from lp.services.database.bulk import (
@@ -1005,6 +1006,24 @@
                 % chunk_size)
 
 
+class TeamMembershipPruner(BulkPruner):
+    """Remove team memberships for merged people.
+
+    People merge can leave team membership records behind because:
+        * The membership duplicates another membership.
+        * The membership would have created a cyclic relationshop.
+        * The operation avoid a race condition.
+    """
+    target_table_class = TeamMembership
+    ids_to_prune_query = """
+        SELECT TeamMembership.id
+        FROM TeamMembership, Person
+        WHERE
+            TeamMembership.person = person.id
+            AND person.merged IS NOT NULL
+        """
+
+
 class BugNotificationPruner(BulkPruner):
     """Prune `BugNotificationRecipient` records no longer of interest.
 
@@ -1632,6 +1651,7 @@
         ScrubPOFileTranslator,
         ProductInformationTypeDefault,
         SuggestiveTemplatesCacheUpdater,
+        TeamMembershipPruner,
         POTranslationPruner,
         UnlinkedAccountPruner,
         UnusedAccessPolicyPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-11-26 08:33:03 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-12-03 22:09:24 +0000
@@ -61,8 +61,10 @@
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.teammembership import TeamMembershipStatus
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.product import Product
+from lp.registry.model.teammembership import TeamMembership
 from lp.scripts.garbo import (
     AntiqueSessionPruner,
     BulkPruner,
@@ -739,6 +741,25 @@
             personset.getByName('test-unlinked-person-new'), None)
         self.assertIs(personset.getByName('test-unlinked-person-old'), None)
 
+    def test_TeamMembershipPruner(self):
+        # Garbo should remove team memberships for meregd users and teams.
+        switch_dbuser('testadmin')
+        merged_user = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[merged_user])
+        merged_team = self.factory.makeTeam()
+        team.addMember(
+            merged_team, team.teamowner, status=TeamMembershipStatus.PROPOSED)
+        # This is fast and dirty way to place the user and team in a
+        # merged state to verify what the TeamMembershipPruner sees.
+        removeSecurityProxy(merged_user).merged = self.factory.makePerson()
+        removeSecurityProxy(merged_team).merged = self.factory.makeTeam()
+        store = Store.of(team)
+        store.flush()
+        result = store.find(TeamMembership, TeamMembership.team == team.id)
+        self.assertEqual(3, result.count())
+        self.runDaily()
+        self.assertContentEqual([team.teamowner], [tm.person for tm in result])
+
     def test_BugNotificationPruner(self):
         # Create some sample data
         switch_dbuser('testadmin')