launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/launchpad/garbo-bulk-pruner into lp:launchpad with lp:~stub/launchpad/garbo as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stub/launchpad/garbo-bulk-pruner/+merge/87461

Trash EmailAddress and Account records not linked to a Person. With this, we can drop the redundant EmailAddress.account field bringing a little sanity to our data model and poisoning some lurking bugs at the source.
-- 
https://code.launchpad.net/~stub/launchpad/garbo-bulk-pruner/+merge/87461
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/garbo-bulk-pruner into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-01-03 01:40:09 +0000
+++ database/schema/security.cfg	2012-01-04 11:29:06 +0000
@@ -2114,6 +2114,7 @@
 
 [garbo]
 groups=script,read
+public.account                          = SELECT, DELETE
 public.answercontact                    = SELECT, DELETE
 public.branch                           = SELECT, UPDATE
 public.branchjob                        = SELECT, DELETE
@@ -2141,7 +2142,7 @@
 public.codeimportevent                  = SELECT, DELETE
 public.codeimporteventdata              = SELECT, DELETE
 public.codeimportresult                 = SELECT, DELETE
-public.emailaddress                     = SELECT, UPDATE
+public.emailaddress                     = SELECT, UPDATE, DELETE
 public.hwsubmission                     = SELECT, UPDATE
 public.job                              = SELECT, INSERT, DELETE
 public.logintoken                       = SELECT, DELETE

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-12-30 06:47:17 +0000
+++ lib/lp/scripts/garbo.py	2012-01-04 11:29:06 +0000
@@ -65,6 +65,7 @@
     )
 from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.services.identity.model.account import Account
 from lp.services.identity.model.emailaddress import EmailAddress
 from lp.services.job.model.job import Job
 from lp.services.librarian.model import TimeLimitedToken
@@ -310,6 +311,31 @@
         """
 
 
+class AccountOnlyEmailAddressPruner(BulkPruner):
+    """Remove EmailAddress records not linked to a Person."""
+    target_table_class = EmailAddress
+    ids_to_prune_query = "SELECT id FROM EmailAddress WHERE person IS NULL"
+
+
+class UnlinkedAccountPruner(BulkPruner):
+    """Remove Account records not linked to a Person."""
+    target_table_class = Account
+    # We join with EmailAddress to ensure we only attempt removal after
+    # the EmailAddress rows have been removed by
+    # AccountOnlyEmailAddressPruner. We join with Person to work around
+    # records with bad crosslinks. These bad crosslinks will be fixed by
+    # dropping the EmailAddress.account column.
+    ids_to_prune_query = """
+        SELECT Account.id
+        FROM Account
+        LEFT OUTER JOIN EmailAddress ON Account.id = EmailAddress.account
+        LEFT OUTER JOIN Person ON Account.id = Person.account
+        WHERE
+            EmailAddress.id IS NULL
+            AND Person.id IS NULL
+        """
+
+
 class BugSummaryJournalRollup(TunableLoop):
     """Rollup BugSummaryJournal rows into BugSummary."""
     maximum_chunk_size = 5000
@@ -1242,6 +1268,8 @@
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
         UnusedPOTMsgSetPruner,
+        AccountOnlyEmailAddressPruner,
+        UnlinkedAccountPruner,
         ]
     experimental_tunable_loops = [
         PersonPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-12-30 06:47:17 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-01-04 11:29:06 +0000
@@ -51,10 +51,7 @@
     )
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportresult import CodeImportResult
-from lp.registry.interfaces.person import (
-    IPersonSet,
-    PersonCreationRationale,
-    )
+from lp.registry.interfaces.person import IPersonSet
 from lp.scripts.garbo import (
     AntiqueSessionPruner,
     BulkPruner,
@@ -626,18 +623,14 @@
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         rev1 = self.factory.makeRevision('Author 1 <author-1@xxxxxxxxxxx>')
         rev2 = self.factory.makeRevision('Author 2 <author-2@xxxxxxxxxxx>')
-        rev3 = self.factory.makeRevision('Author 3 <author-3@xxxxxxxxxxx>')
 
         person1 = self.factory.makePerson(email='Author-1@xxxxxxxxxxx')
         person2 = self.factory.makePerson(
             email='Author-2@xxxxxxxxxxx',
             email_address_status=EmailAddressStatus.NEW)
-        account3 = self.factory.makeAccount(
-            'Author 3', 'Author-3@xxxxxxxxxxx')
 
         self.assertEqual(rev1.revision_author.person, None)
         self.assertEqual(rev2.revision_author.person, None)
-        self.assertEqual(rev3.revision_author.person, None)
 
         self.runDaily()
 
@@ -646,7 +639,6 @@
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         self.assertEqual(rev1.revision_author.person, person1)
         self.assertEqual(rev2.revision_author.person, None)
-        self.assertEqual(rev3.revision_author.person, None)
 
         # Validating an email address creates a linkage.
         person2.validateAndEnsurePreferredEmail(person2.guessedemails[0])
@@ -656,33 +648,20 @@
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         self.assertEqual(rev2.revision_author.person, person2)
 
-        # Creating a person for an existing account creates a linkage.
-        person3 = account3.createPerson(PersonCreationRationale.UNKNOWN)
-        self.assertEqual(rev3.revision_author.person, None)
-
-        self.runDaily()
-        LaunchpadZopelessLayer.switchDbUser('testadmin')
-        self.assertEqual(rev3.revision_author.person, person3)
-
     def test_HWSubmissionEmailLinker(self):
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         sub1 = self.factory.makeHWSubmission(
             emailaddress='author-1@xxxxxxxxxxx')
         sub2 = self.factory.makeHWSubmission(
             emailaddress='author-2@xxxxxxxxxxx')
-        sub3 = self.factory.makeHWSubmission(
-            emailaddress='author-3@xxxxxxxxxxx')
 
         person1 = self.factory.makePerson(email='Author-1@xxxxxxxxxxx')
         person2 = self.factory.makePerson(
             email='Author-2@xxxxxxxxxxx',
             email_address_status=EmailAddressStatus.NEW)
-        account3 = self.factory.makeAccount(
-            'Author 3', 'Author-3@xxxxxxxxxxx')
 
         self.assertEqual(sub1.owner, None)
         self.assertEqual(sub2.owner, None)
-        self.assertEqual(sub3.owner, None)
 
         self.runDaily()
 
@@ -691,7 +670,6 @@
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         self.assertEqual(sub1.owner, person1)
         self.assertEqual(sub2.owner, None)
-        self.assertEqual(sub3.owner, None)
 
         # Validating an email address creates a linkage.
         person2.validateAndEnsurePreferredEmail(person2.guessedemails[0])
@@ -701,14 +679,6 @@
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         self.assertEqual(sub2.owner, person2)
 
-        # Creating a person for an existing account creates a linkage.
-        person3 = account3.createPerson(PersonCreationRationale.UNKNOWN)
-        self.assertEqual(sub3.owner, None)
-
-        self.runDaily()
-        LaunchpadZopelessLayer.switchDbUser('testadmin')
-        self.assertEqual(sub3.owner, person3)
-
     def test_PersonPruner(self):
         personset = getUtility(IPersonSet)
         # Switch the DB user because the garbo_daily user isn't allowed to