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	2019-01-15 17:04:31 +0000
> +++ lib/lp/registry/scripts/closeaccount.py	2019-02-21 13:03:43 +0000
> @@ -282,6 +287,9 @@
>  
>          # "Affects me too" information
>          ('BugAffectsPerson', 'person'),
> +
> +        # Hardware submissions
> +        ('HWSubmission', 'owner'),

It's slightly surprising that this works without crashing on e.g. HWSubmissionDevices.

>          ]
>      for table, person_id_column in removals:
>          table_notification(table)
> @@ -306,6 +314,22 @@
>      # the placeholder person row.
>      skip.add(('sprintattendance', 'attendee'))
>  
> +    # generate_ppa_htaccess currently relies on seeing active
> +    # ArchiveAuthToken rows so that it knows which ones to remove from
> +    # .htpasswd files on disk in response to the cancellation of the
> +    # corresponding ArchiveSubscriber rows; but even once PPA authorisation
> +    # is handled dynamically, we probably still want to have the per-person
> +    # audit trail here.
> +    store.find(
> +        ArchiveSubscriber,
> +        ArchiveSubscriber.subscriber_id == person.id,
> +        ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT).set(
> +            date_cancelled=UTC_NOW,
> +            cancelled_by_id=janitor.id,
> +            status=ArchiveSubscriberStatus.CANCELLED)

I believe this is only the second place we set CANCELLED. Possibly worth considering factoring it out now, but not critical.

> +    skip.add(('archivesubscriber', 'subscriber'))
> +    skip.add(('archiveauthtoken', 'person'))
> +
>      # 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


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