launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23116
[Merge] lp:~cjwatson/launchpad/close-account-polish into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/close-account-polish into lp:launchpad with lp:~cjwatson/launchpad/close-account-perms as a prerequisite.
Commit message:
Make close-account safer to run.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/close-account-polish/+merge/359865
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/close-account-polish into lp:launchpad.
=== modified file 'lib/lp/registry/personmerge.py'
--- lib/lp/registry/personmerge.py 2018-10-05 15:35:34 +0000
+++ lib/lp/registry/personmerge.py 2018-11-29 19:04:47 +0000
@@ -775,18 +775,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-11-29 19:04:47 +0000
+++ lib/lp/registry/scripts/closeaccount.py 2018-11-29 19:04:47 +0000
@@ -11,15 +11,32 @@
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.interfaces.person import (
+ IPersonSet,
+ PersonCreationRationale,
+ )
+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,
+ IAccountSet,
+ )
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,6 +50,10 @@
"""
store = IMasterStore(Person)
+ cur = cursor()
+ references = list(postgresql.listReferences(cur, 'person', 'id'))
+ postgresql.check_indirect_references(references)
+
row = store.using(
Person,
LeftJoin(EmailAddress, Person.id == EmailAddress.personID)
@@ -57,6 +78,78 @@
# succeed.
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.
@@ -65,41 +158,64 @@
# 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')
+ person = removeSecurityProxy(getUtility(IPersonSet).get(person_id))
+ 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 account_id is not None:
- store.find(Account, Account.id == account_id).remove()
+ table_notification('Account')
+ account = removeSecurityProxy(getUtility(IAccountSet).get(account_id))
+ 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)
# 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(
+ 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 +232,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 +266,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)
@@ -167,6 +291,33 @@
AND attendee = ?
AND Sprint.time_starts > CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
""", (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" %
+ (username, count, src_tab, src_col))
+ has_references = True
+ if has_references:
+ raise LaunchpadScriptFailure("User %s is still referenced" % username)
return True
=== modified file 'lib/lp/registry/scripts/tests/test_closeaccount.py'
--- lib/lp/registry/scripts/tests/test_closeaccount.py 2018-11-29 19:04:47 +0000
+++ lib/lp/registry/scripts/tests/test_closeaccount.py 2018-11-29 19:04:47 +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', 'mark@xxxxxxxxxxx')
- 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', 'mark@xxxxxxxxxxx')
- script = self.makeScript(['mark@xxxxxxxxxxx'])
+ 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-11-29 19:04:47 +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-11-29 19:04:47 +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-11-29 19:04:47 +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])
Follow ups
