Merge lp:~cjwatson/launchpad/close-account-polish into lp:launchpad

[Colin Watson]

Make close-account safer to run.

I went through the list of FKs to Person.id and identified quite a few more that can safely be either ignored or removed. For the remainder, I borrowed part of personmerge's approach: we now programmatically check all remaining FKs and bail out if there's anything left that we haven't considered. (For example, if the person whose account is being closed still owns a project, we need to decide what to do about that.)

I also arranged to preserve the Account.status_history audit trail (it seemed best to use a new terminal AccountStatus.CLOSED status), and to solve only those owned questions that are in non-final states.

This should make close-account somewhat less eccentric and (I hope) closer to being safe to use on production: it's more careful to preserve audit trails, and bails out rather than doing the wrong thing if it encounters something it doesn't understand.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/registry/personmerge.py'
--- lib/lp/registry/personmerge.py	2018-11-29 13:48:39 +0000
+++ lib/lp/registry/personmerge.py	2018-12-03 11:35:13 +0000
@@ -771,18 +771,7 @@
         ]
 
     references = list(postgresql.listReferences(cur, 'person', 'id'))
-
-    # Sanity check. If we have an indirect reference, it must
-    # be ON DELETE CASCADE. We only have one case of this at the moment,
-    # but this code ensures we catch any new ones added incorrectly.
-    for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
-        # If the ref_tab and ref_col is not Person.id, then we have
-        # an indirect reference. Ensure the update action is 'CASCADE'
-        if ref_tab != 'person' and ref_col != 'id':
-            if updact != 'c':
-                raise RuntimeError(
-                    '%s.%s reference to %s.%s must be ON UPDATE CASCADE'
-                    % (src_tab, src_col, ref_tab, ref_col))
+    postgresql.check_indirect_references(references)
 
     # These rows are in a UNIQUE index, and we can only move them
     # to the new Person if there is not already an entry. eg. if

=== modified file 'lib/lp/registry/scripts/closeaccount.py'
--- lib/lp/registry/scripts/closeaccount.py	2018-12-03 11:35:13 +0000
+++ lib/lp/registry/scripts/closeaccount.py	2018-12-03 11:35:13 +0000
@@ -11,15 +11,28 @@
     Lower,
     Or,
     )
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from lp.answers.enums import QuestionStatus
 from lp.answers.model.question import Question
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.person import PersonCreationRationale
-from lp.registry.model.person import Person
+from lp.registry.model.person import (
+    Person,
+    PersonSettings,
+    )
+from lp.services.database import postgresql
+from lp.services.database.constants import DEFAULT
 from lp.services.database.interfaces import IMasterStore
-from lp.services.identity.model.account import Account
+from lp.services.database.sqlbase import cursor
+from lp.services.identity.interfaces.account import (
+    AccountCreationRationale,
+    AccountStatus,
+    )
 from lp.services.identity.model.emailaddress import EmailAddress
+from lp.services.openid.model.openididentifier import OpenIdIdentifier
 from lp.services.scripts.base import (
     LaunchpadScript,
     LaunchpadScriptFailure,
@@ -33,73 +46,171 @@
     """
     store = IMasterStore(Person)
 
-    row = store.using(
+    cur = cursor()
+    references = list(postgresql.listReferences(cur, 'person', 'id'))
+    postgresql.check_indirect_references(references)
+
+    person = store.using(
         Person,
         LeftJoin(EmailAddress, Person.id == EmailAddress.personID)
     ).find(
-        (Person.id, Person.accountID, Person.name, Person.teamownerID),
+        Person,
         Or(Person.name == username,
            Lower(EmailAddress.email) == Lower(username))).one()
-    if row is None:
+    if person is None:
         raise LaunchpadScriptFailure("User %s does not exist" % username)
-    person_id, account_id, username, teamowner_id = row
+    person_name = person.name
 
     # We don't do teams
-    if teamowner_id is not None:
-        raise LaunchpadScriptFailure("%s is a team" % username)
+    if person.is_team:
+        raise LaunchpadScriptFailure("%s is a team" % person_name)
 
-    log.info("Closing %s's account" % username)
+    log.info("Closing %s's account" % person_name)
 
     def table_notification(table):
         log.debug("Handling the %s table" % table)
 
     # All names starting with 'removed' are blacklisted, so this will always
     # succeed.
-    new_name = 'removed%d' % person_id
+    new_name = 'removed%d' % person.id
+
+    # Some references can safely remain in place and link to the cleaned-out
+    # Person row.
+    skip = {
+        # These references express some kind of audit trail.  The actions in
+        # question still happened, and in some cases the rows may still have
+        # functional significance (e.g. subscriptions or access grants), but
+        # we no longer identify the actor.
+        ('accessartifactgrant', 'grantor'),
+        ('accesspolicygrant', 'grantor'),
+        ('binarypackagepublishinghistory', 'removed_by'),
+        ('branchmergeproposal', 'merge_reporter'),
+        ('branchmergeproposal', 'merger'),
+        ('branchmergeproposal', 'queuer'),
+        ('branchmergeproposal', 'reviewer'),
+        ('branchsubscription', 'subscribed_by'),
+        ('bug', 'who_made_private'),
+        ('bugactivity', 'person'),
+        ('bugnomination', 'decider'),
+        ('bugsubscription', 'subscribed_by'),
+        ('faq', 'last_updated_by'),
+        ('featureflagchangelogentry', 'person'),
+        ('gitactivity', 'changee'),
+        ('gitactivity', 'changer'),
+        ('gitrule', 'creator'),
+        ('gitrulegrant', 'grantor'),
+        ('gitsubscription', 'subscribed_by'),
+        ('messageapproval', 'disposed_by'),
+        ('messageapproval', 'posted_by'),
+        ('packagecopyrequest', 'requester'),
+        ('packagediff', 'requester'),
+        ('personlocation', 'last_modified_by'),
+        ('persontransferjob', 'major_person'),
+        ('persontransferjob', 'minor_person'),
+        ('poexportrequest', 'person'),
+        ('question', 'answerer'),
+        ('questionreopening', 'answerer'),
+        ('questionreopening', 'reopener'),
+        ('snapbuild', 'requester'),
+        ('sourcepackagepublishinghistory', 'creator'),
+        ('sourcepackagepublishinghistory', 'removed_by'),
+        ('sourcepackagepublishinghistory', 'sponsor'),
+        ('sourcepackagerecipebuild', 'requester'),
+        ('specification', 'approver'),
+        ('specification', 'completer'),
+        ('specification', 'drafter'),
+        ('specification', 'goal_decider'),
+        ('specification', 'goal_proposer'),
+        ('specification', 'last_changed_by'),
+        ('specification', 'starter'),
+        ('structuralsubscription', 'subscribed_by'),
+        ('teammembership', 'acknowledged_by'),
+        ('teammembership', 'proposed_by'),
+        ('teammembership', 'reviewed_by'),
+        ('translationimportqueueentry', 'importer'),
+        ('translationmessage', 'reviewer'),
+        ('translationmessage', 'submitter'),
+        ('usertouseremail', 'recipient'),
+        ('usertouseremail', 'sender'),
+        ('xref', 'creator'),
+        }
+    reference_names = {
+        (src_tab, src_col) for src_tab, src_col, _, _, _, _ in references}
+    for src_tab, src_col in skip:
+        if (src_tab, src_col) not in reference_names:
+            raise AssertionError(
+                "%s.%s is not a Person reference; possible typo?" %
+                (src_tab, src_col))
+
+    # XXX cjwatson 2018-11-29: Registrants could possibly be left as-is, but
+    # perhaps we should pretend that the registrant was ~registry in that
+    # case instead?
 
     # Remove the EmailAddress. This is the most important step, as
     # people requesting account removal seem to primarily be interested
     # in ensuring we no longer store this information.
     table_notification('EmailAddress')
-    store.find(EmailAddress, EmailAddress.personID == person_id).remove()
+    store.find(EmailAddress, EmailAddress.personID == person.id).remove()
 
     # Clean out personal details from the Person table
     table_notification('Person')
-    store.find(Person, Person.id == person_id).set(
-        display_name='Removed by request',
-        name=new_name,
-        accountID=None,
-        homepage_content=None,
-        iconID=None,
-        mugshotID=None,
-        hide_email_addresses=True,
-        registrantID=None,
-        logoID=None,
-        creation_rationale=PersonCreationRationale.UNKNOWN,
-        creation_comment=None)
-
-    # Remove the Account. We don't set the status to deactivated,
-    # as this script is used to satisfy people who insist on us removing
-    # all their personal details from our systems. This includes any
-    # identification tokens like email addresses or openid identifiers.
-    # So the Account record would be unusable, and contain no useful
-    # information.
-    table_notification('Account')
-    if account_id is not None:
-        store.find(Account, Account.id == account_id).remove()
+    person.display_name = 'Removed by request'
+    person.name = new_name
+    person.homepage_content = None
+    person.icon = None
+    person.mugshot = None
+    person.hide_email_addresses = False
+    person.registrant = None
+    person.logo = None
+    person.creation_rationale = PersonCreationRationale.UNKNOWN
+    person.creation_comment = None
+
+    # Keep the corresponding PersonSettings row, but reset everything to the
+    # defaults.
+    table_notification('PersonSettings')
+    store.find(PersonSettings, PersonSettings.personID == person.id).set(
+        selfgenerated_bugnotifications=DEFAULT,
+        # XXX cjwatson 2018-11-29: These two columns have NULL defaults, but
+        # perhaps shouldn't?
+        expanded_notification_footers=False,
+        require_strong_email_authentication=False)
+    skip.add(('personsettings', 'person'))
+
+    # Remove almost everything from the Account row and the corresponding
+    # OpenIdIdentifier rows, preserving only a minimal audit trail.
+    if person.account is not None:
+        table_notification('Account')
+        account = removeSecurityProxy(person.account)
+        account.displayname = 'Removed by request'
+        account.creation_rationale = AccountCreationRationale.UNKNOWN
+        janitor = getUtility(ILaunchpadCelebrities).janitor
+        person.setAccountStatus(
+            AccountStatus.CLOSED, janitor, "Closed using close-account.")
+
+        table_notification('OpenIdIdentifier')
+        store.find(
+            OpenIdIdentifier,
+            OpenIdIdentifier.account_id == account.id).remove()
 
     # Reassign their bugs
     table_notification('BugTask')
-    store.find(BugTask, BugTask.assigneeID == person_id).set(assigneeID=None)
+    store.find(BugTask, BugTask.assigneeID == person.id).set(assigneeID=None)
 
     # Reassign questions assigned to the user, and close all their questions
-    # since nobody else can
+    # in non-final states since nobody else can.
     table_notification('Question')
-    store.find(Question, Question.assigneeID == person_id).set(assigneeID=None)
-    store.find(Question, Question.ownerID == person_id).set(
+    store.find(Question, Question.assigneeID == person.id).set(assigneeID=None)
+    owned_non_final_questions = store.find(
+        Question, Question.ownerID == person.id,
+        Question.status.is_in([
+            QuestionStatus.OPEN, QuestionStatus.NEEDSINFO,
+            QuestionStatus.ANSWERED,
+            ]))
+    owned_non_final_questions.set(
         status=QuestionStatus.SOLVED,
         whiteboard=(
             'Closed by Launchpad due to owner requesting account removal'))
+    skip.add(('question', 'owner'))
 
     # Remove rows from tables in simple cases in the given order
     removals = [
@@ -116,10 +227,14 @@
 
         # Subscriptions
         ('BranchSubscription', 'person'),
+        ('BugMute', 'person'),
+        ('BugSubscription', 'person'),
+        ('BugSubscriptionFilterMute', 'person'),
         ('GitSubscription', 'person'),
-        ('BugSubscription', 'person'),
+        ('MailingListSubscription', 'person'),
         ('QuestionSubscription', 'person'),
         ('SpecificationSubscription', 'person'),
+        ('StructuralSubscription', 'subscriber'),
 
         # Personal stuff, freeing up the namespace for others who want to play
         # or just to remove any fingerprints identifying the user.
@@ -146,7 +261,11 @@
         ('POExportRequest', 'person'),
 
         # Access grants
+        ('AccessArtifactGrant', 'grantee'),
+        ('AccessPolicyGrant', 'grantee'),
+        ('ArchivePermission', 'person'),
         ('GitRuleGrant', 'grantee'),
+        ('SharingJob', 'grantee'),
         ]
     for table, person_id_column in removals:
         table_notification(table)
@@ -156,7 +275,7 @@
                 'table': table,
                 'person_id_column': person_id_column,
                 },
-            (person_id,))
+            (person.id,))
 
     # Trash Sprint Attendance records in the future.
     table_notification('SprintAttendance')
@@ -166,7 +285,35 @@
         WHERE Sprint.id = SprintAttendance.sprint
             AND attendee = ?
             AND Sprint.time_starts > CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
-        """, (person_id,))
+        """, (person.id,))
+    # Any remaining past sprint attendance records can harmlessly refer to
+    # the placeholder person row.
+    skip.add(('sprintattendance', 'attendee'))
+
+    # Closing the account will only work if all references have been handled
+    # by this point.  If not, it's safer to bail out.  It's OK if this
+    # doesn't work in all conceivable situations, since some of them may
+    # require careful thought and decisions by a human administrator.
+    has_references = False
+    for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
+        if (src_tab, src_col) in skip:
+            continue
+        result = store.execute("""
+            SELECT COUNT(*) FROM %(src_tab)s WHERE %(src_col)s = ?
+            """ % {
+                'src_tab': src_tab,
+                'src_col': src_col,
+                },
+            (person.id,))
+        count = result.get_one()[0]
+        if count:
+            log.error(
+                "User %s is still referenced by %d %s.%s values" %
+                (person_name, count, src_tab, src_col))
+            has_references = True
+    if has_references:
+        raise LaunchpadScriptFailure(
+            "User %s is still referenced" % person_name)
 
     return True
 

=== modified file 'lib/lp/registry/scripts/tests/test_closeaccount.py'
--- lib/lp/registry/scripts/tests/test_closeaccount.py	2018-12-03 11:35:13 +0000
+++ lib/lp/registry/scripts/tests/test_closeaccount.py	2018-12-03 11:35:13 +0000
@@ -7,20 +7,28 @@
 
 __metaclass__ = type
 
+from testtools.matchers import (
+    Not,
+    StartsWith,
+    )
+import transaction
+from twisted.python.compat import nativeString
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.answers.enums import QuestionStatus
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.scripts.closeaccount import CloseAccountScript
+from lp.services.database.sqlbase import flush_database_caches
 from lp.services.identity.interfaces.account import (
     AccountStatus,
     IAccountSet,
     )
-from lp.services.log.logger import DevNullLogger
+from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
+from lp.services.log.logger import BufferLogger
 from lp.services.scripts.base import LaunchpadScriptFailure
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import dbuser
-from lp.testing.faketransaction import FakeTransaction
 from lp.testing.layers import ZopelessDatabaseLayer
 
 
@@ -37,38 +45,81 @@
 
     def makeScript(self, test_args):
         script = CloseAccountScript(test_args=test_args)
-        script.logger = DevNullLogger()
-        script.txn = FakeTransaction()
+        script.logger = BufferLogger()
+        script.txn = transaction
         return script
 
-    def getSampleUser(self, name, email):
-        """Return a sampledata account with some personal information."""
-        person = getUtility(IPersonSet).getByEmail(email)
-        account = removeSecurityProxy(person.account)
-        self.assertEqual(AccountStatus.ACTIVE, account.status)
-        self.assertEqual(name, person.name)
-        return person.id, account.id
+    def runScript(self, script):
+        try:
+            script.main()
+        finally:
+            self.addDetail("log", script.logger.content)
+            flush_database_caches()
+
+    def makePopulatedUser(self):
+        """Return a person and account linked to some personal information."""
+        person = self.factory.makePerson(karma=10)
+        self.assertEqual(AccountStatus.ACTIVE, person.account.status)
+        self.assertNotEqual([], list(person.account.openid_identifiers))
+        self.factory.makeBugTask().transitionToAssignee(person, validate=False)
+        self.factory.makeQuestion().assignee = person
+        self.factory.makeQuestion(owner=person)
+        self.factory.makeGPGKey(owner=person)
+        self.factory.makeBranchSubscription(person=person)
+        self.factory.makeBug().subscribe(person, person)
+        self.factory.makeGitSubscription(person=person)
+        team = self.factory.makeTeam()
+        self.factory.makeMailingList(team, team.teamowner).subscribe(person)
+        self.factory.makeQuestionSubscription(person=person)
+        self.factory.makeSpecification().subscribe(person)
+        person.addLanguage(self.factory.makeLanguage())
+        self.factory.makeSSHKey(person=person)
+        self.factory.makeAccessArtifactGrant(grantee=person)
+        self.factory.makeAccessPolicyGrant(grantee=person)
+        self.factory.makeGitRuleGrant(grantee=person)
+        person.selfgenerated_bugnotifications = True
+        person.expanded_notification_footers = True
+        person.require_strong_email_authentication = True
+        return person, person.id, person.account.id
 
     def assertRemoved(self, account_id, person_id):
-        # We can't just set the account to DEACTIVATED, as the
-        # close-account.py script is used to satisfy people who insist on us
-        # removing all their personal details from our system.  The Account
-        # has been removed entirely.
-        self.assertRaises(
-            LookupError, getUtility(IAccountSet).get, account_id)
+        # The Account row still exists, but has been anonymised, leaving
+        # only a minimal audit trail.
+        account = getUtility(IAccountSet).get(account_id)
+        self.assertEqual('Removed by request', account.displayname)
+        self.assertEqual(AccountStatus.CLOSED, account.status)
+        self.assertIn('Closed using close-account.', account.status_history)
 
-        # The Person record still exists to maintain links with information
+        # The Person row still exists to maintain links with information
         # that won't be removed, such as bug comments, but has been
-        # anonymized.
+        # anonymised.
         person = getUtility(IPersonSet).get(person_id)
-        self.assertStartsWith(person.name, 'removed')
+        self.assertThat(person.name, StartsWith('removed'))
         self.assertEqual('Removed by request', person.display_name)
+        self.assertEqual(account, person.account)
+
+        # The corresponding PersonSettings row has been reset to the
+        # defaults.
+        self.assertFalse(person.selfgenerated_bugnotifications)
+        self.assertFalse(person.expanded_notification_footers)
+        self.assertFalse(person.require_strong_email_authentication)
+
+        # EmailAddress and OpenIdIdentifier rows have been removed.
+        self.assertEqual(
+            [], list(getUtility(IEmailAddressSet).getByPerson(person)))
+        self.assertEqual([], list(account.openid_identifiers))
 
     def assertNotRemoved(self, account_id, person_id):
         account = getUtility(IAccountSet).get(account_id)
+        self.assertNotEqual('Removed by request', account.displayname)
         self.assertEqual(AccountStatus.ACTIVE, account.status)
         person = getUtility(IPersonSet).get(person_id)
-        self.assertIsNotNone(person.account)
+        self.assertEqual(account, person.account)
+        self.assertNotEqual('Removed by request', person.display_name)
+        self.assertThat(person.name, Not(StartsWith('removed')))
+        self.assertNotEqual(
+            [], list(getUtility(IEmailAddressSet).getByPerson(person)))
+        self.assertNotEqual([], list(account.openid_identifiers))
 
     def test_nonexistent(self):
         script = self.makeScript(['nonexistent-person'])
@@ -76,7 +127,7 @@
             self.assertRaisesWithContent(
                 LaunchpadScriptFailure,
                 'User nonexistent-person does not exist',
-                script.main)
+                self.runScript, script)
 
     def test_team(self):
         team = self.factory.makeTeam()
@@ -85,20 +136,37 @@
             self.assertRaisesWithContent(
                 LaunchpadScriptFailure,
                 '%s is a team' % team.name,
-                script.main)
+                self.runScript, script)
+
+    def test_unhandled_reference(self):
+        person = self.factory.makePerson()
+        person_id = person.id
+        account_id = person.account.id
+        self.factory.makeProduct(owner=person)
+        script = self.makeScript([nativeString(person.name)])
+        with dbuser('launchpad'):
+            self.assertRaisesWithContent(
+                LaunchpadScriptFailure,
+                'User %s is still referenced' % person.name,
+                self.runScript, script)
+        self.assertIn(
+            'ERROR User %s is still referenced by 1 product.owner values' % (
+                person.name),
+            script.logger.getLogBuffer())
+        self.assertNotRemoved(account_id, person_id)
 
     def test_single_by_name(self):
-        person_id, account_id = self.getSampleUser('mark', '[e-mail]')
-        script = self.makeScript(['mark'])
+        person, person_id, account_id = self.makePopulatedUser()
+        script = self.makeScript([nativeString(person.name)])
         with dbuser('launchpad'):
-            script.main()
+            self.runScript(script)
         self.assertRemoved(account_id, person_id)
 
     def test_single_by_email(self):
-        person_id, account_id = self.getSampleUser('mark', '[e-mail]')
-        script = self.makeScript(['[e-mail]'])
+        person, person_id, account_id = self.makePopulatedUser()
+        script = self.makeScript([nativeString(person.preferredemail.email)])
         with dbuser('launchpad'):
-            script.main()
+            self.runScript(script)
         self.assertRemoved(account_id, person_id)
 
     def test_multiple(self):
@@ -107,7 +175,64 @@
         account_ids = [person.account.id for person in persons]
         script = self.makeScript([persons[0].name, persons[1].name])
         with dbuser('launchpad'):
-            script.main()
+            self.runScript(script)
         self.assertRemoved(account_ids[0], person_ids[0])
         self.assertRemoved(account_ids[1], person_ids[1])
         self.assertNotRemoved(account_ids[2], person_ids[2])
+
+    def test_retains_audit_trail(self):
+        person = self.factory.makePerson()
+        person_id = person.id
+        account_id = person.account.id
+        branch_subscription = self.factory.makeBranchSubscription(
+            subscribed_by=person)
+        snap = self.factory.makeSnap()
+        snap_build = self.factory.makeSnapBuild(requester=person, snap=snap)
+        specification = self.factory.makeSpecification(drafter=person)
+        script = self.makeScript([nativeString(person.name)])
+        with dbuser('launchpad'):
+            self.runScript(script)
+        self.assertRemoved(account_id, person_id)
+        self.assertEqual(person, branch_subscription.subscribed_by)
+        self.assertEqual(person, snap_build.requester)
+        self.assertEqual(person, specification.drafter)
+
+    def test_solves_questions_in_non_final_states(self):
+        person = self.factory.makePerson()
+        person_id = person.id
+        account_id = person.account.id
+        questions = []
+        for status in (
+                QuestionStatus.OPEN, QuestionStatus.NEEDSINFO,
+                QuestionStatus.ANSWERED):
+            question = self.factory.makeQuestion(owner=person)
+            removeSecurityProxy(question).status = status
+            questions.append(question)
+        script = self.makeScript([nativeString(person.name)])
+        with dbuser('launchpad'):
+            self.runScript(script)
+        self.assertRemoved(account_id, person_id)
+        for question in questions:
+            self.assertEqual(QuestionStatus.SOLVED, question.status)
+            self.assertEqual(
+                'Closed by Launchpad due to owner requesting account removal',
+                question.whiteboard)
+
+    def test_skips_questions_in_final_states(self):
+        person = self.factory.makePerson()
+        person_id = person.id
+        account_id = person.account.id
+        questions = {}
+        for status in (
+                QuestionStatus.SOLVED, QuestionStatus.EXPIRED,
+                QuestionStatus.INVALID):
+            question = self.factory.makeQuestion(owner=person)
+            removeSecurityProxy(question).status = status
+            questions[status] = question
+        script = self.makeScript([nativeString(person.name)])
+        with dbuser('launchpad'):
+            self.runScript(script)
+        self.assertRemoved(account_id, person_id)
+        for question_status, question in questions.items():
+            self.assertEqual(question_status, question.status)
+            self.assertIsNone(question.whiteboard)

=== modified file 'lib/lp/services/database/postgresql.py'
--- lib/lp/services/database/postgresql.py	2018-03-21 14:54:49 +0000
+++ lib/lp/services/database/postgresql.py	2018-12-03 11:35:13 +0000
@@ -240,6 +240,20 @@
     return rv
 
 
+def check_indirect_references(references):
+    # Sanity check. If we have an indirect reference, it must
+    # be ON DELETE CASCADE. We only have one case of this at the moment,
+    # but this code ensures we catch any new ones added incorrectly.
+    for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
+        # If the ref_tab and ref_col is not Person.id, then we have
+        # an indirect reference. Ensure the update action is 'CASCADE'
+        if ref_tab != 'person' and ref_col != 'id':
+            if updact != 'c':
+                raise RuntimeError(
+                    '%s.%s reference to %s.%s must be ON UPDATE CASCADE'
+                    % (src_tab, src_col, ref_tab, ref_col))
+
+
 def generateResetSequencesSQL(cur):
     """Return SQL that will reset table sequences to match the data in them.
     """

=== modified file 'lib/lp/services/identity/interfaces/account.py'
--- lib/lp/services/identity/interfaces/account.py	2016-04-28 02:25:46 +0000
+++ lib/lp/services/identity/interfaces/account.py	2018-12-03 11:35:13 +0000
@@ -84,10 +84,17 @@
         The account has been suspended by a Launchpad admin.
         """)
 
+    CLOSED = DBItem(50, """
+        Closed
+
+        The account has been permanently closed, removing as much personal
+        information as possible.
+        """)
+
 
 INACTIVE_ACCOUNT_STATUSES = [
     AccountStatus.PLACEHOLDER, AccountStatus.DEACTIVATED,
-    AccountStatus.SUSPENDED]
+    AccountStatus.SUSPENDED, AccountStatus.CLOSED]
 
 
 class AccountCreationRationale(DBEnumeratedType):
@@ -238,9 +245,13 @@
             AccountStatus.NOACCOUNT, AccountStatus.ACTIVE],
         AccountStatus.NOACCOUNT: [AccountStatus.ACTIVE],
         AccountStatus.ACTIVE: [
-            AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED],
-        AccountStatus.DEACTIVATED: [AccountStatus.ACTIVE],
-        AccountStatus.SUSPENDED: [AccountStatus.DEACTIVATED],
+            AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED,
+            AccountStatus.CLOSED],
+        AccountStatus.DEACTIVATED: [
+            AccountStatus.ACTIVE, AccountStatus.CLOSED],
+        AccountStatus.SUSPENDED: [
+            AccountStatus.DEACTIVATED, AccountStatus.CLOSED],
+        AccountStatus.CLOSED: [],
         }
 
     def constraint(self, value):

=== modified file 'lib/lp/services/identity/tests/test_account.py'
--- lib/lp/services/identity/tests/test_account.py	2015-01-07 00:35:41 +0000
+++ lib/lp/services/identity/tests/test_account.py	2018-12-03 11:35:13 +0000
@@ -54,31 +54,38 @@
         account = self.factory.makeAccount(status=AccountStatus.NOACCOUNT)
         login_celebrity('admin')
         self.assertCannotTransition(
-            account, [AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED])
+            account,
+            [AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED,
+             AccountStatus.CLOSED])
         self.assertCanTransition(account, [AccountStatus.ACTIVE])
 
     def test_status_from_active(self):
-        # The status may change from ACTIVE to DEACTIVATED or SUSPENDED.
+        # The status may change from ACTIVE to DEACTIVATED, SUSPENDED, or
+        # CLOSED.
         account = self.factory.makeAccount(status=AccountStatus.ACTIVE)
         login_celebrity('admin')
         self.assertCannotTransition(account, [AccountStatus.NOACCOUNT])
         self.assertCanTransition(
-            account, [AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED])
+            account,
+            [AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED,
+             AccountStatus.CLOSED])
 
     def test_status_from_deactivated(self):
-        # The status may change from DEACTIVATED to ACTIVATED.
+        # The status may change from DEACTIVATED to ACTIVATED or CLOSED.
         account = self.factory.makeAccount()
         login_celebrity('admin')
         account.setStatus(AccountStatus.DEACTIVATED, None, 'gbcw')
         self.assertCannotTransition(
             account, [AccountStatus.NOACCOUNT, AccountStatus.SUSPENDED])
-        self.assertCanTransition(account, [AccountStatus.ACTIVE])
+        self.assertCanTransition(
+            account, [AccountStatus.ACTIVE, AccountStatus.CLOSED])
 
     def test_status_from_suspended(self):
-        # The status may change from SUSPENDED to DEACTIVATED.
+        # The status may change from SUSPENDED to DEACTIVATED or CLOSED.
         account = self.factory.makeAccount()
         login_celebrity('admin')
         account.setStatus(AccountStatus.SUSPENDED, None, 'spammer!')
         self.assertCannotTransition(
             account, [AccountStatus.NOACCOUNT, AccountStatus.ACTIVE])
-        self.assertCanTransition(account, [AccountStatus.DEACTIVATED])
+        self.assertCanTransition(
+            account, [AccountStatus.DEACTIVATED, AccountStatus.CLOSED])