launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === 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
> @@ -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))

This can probably use Person.account (and we can drop the account_id SELECT earlier) now that we're leaving it linked to the Person.

> +        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 = [
> @@ -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)

Worth explicitly stating that all known references and personal data have already been wiped?

>  
>      return True
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/close-account-polish/+merge/359865
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.