launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/close-account-code-import into lp:launchpad.

Commit message:
Skip code import references in close-account.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/close-account-code-import/+merge/366916

Code imports don't inherently contain any personal information about the person who created them.

I also skipped RevisionAuthor.person for now.  This isn't great, and there's a bug reference, but we can otherwise be basically stuck if a person ever pushed a branch to Launchpad even though the amount of personal information retained is quite scant.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/close-account-code-import into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/closeaccount.py'
--- lib/lp/registry/scripts/closeaccount.py	2019-05-02 17:36:15 +0000
+++ lib/lp/registry/scripts/closeaccount.py	2019-05-03 12:20:18 +0000
@@ -91,9 +91,11 @@
         ('accessartifactgrant', 'grantor'),
         ('accesspolicygrant', 'grantor'),
         ('binarypackagepublishinghistory', 'removed_by'),
+        ('branch', 'registrant'),
         ('branchmergeproposal', 'merge_reporter'),
         ('branchmergeproposal', 'merger'),
         ('branchmergeproposal', 'queuer'),
+        ('branchmergeproposal', 'registrant'),
         ('branchmergeproposal', 'reviewer'),
         ('branchsubscription', 'subscribed_by'),
         ('bug', 'owner'),
@@ -103,10 +105,14 @@
         ('bugnomination', 'owner'),
         ('bugtask', 'owner'),
         ('bugsubscription', 'subscribed_by'),
+        ('codeimport', 'owner'),
+        ('codeimport', 'registrant'),
+        ('codeimportevent', 'person'),
         ('faq', 'last_updated_by'),
         ('featureflagchangelogentry', 'person'),
         ('gitactivity', 'changee'),
         ('gitactivity', 'changer'),
+        ('gitrepository', 'registrant'),
         ('gitrule', 'creator'),
         ('gitrulegrant', 'grantor'),
         ('gitsubscription', 'subscribed_by'),
@@ -156,6 +162,12 @@
         # This is maintained by trigger functions and a garbo job.  It
         # doesn't need to be updated immediately.
         ('bugsummary', 'viewed_by'),
+
+        # XXX cjwatson 2019-05-02 bug=1827399: This is suboptimal because it
+        # does retain some personal information, but it's currently hard to
+        # deal with due to the size and complexity of references to it.  We
+        # can hopefully provide a garbo job for this eventually.
+        ('revisionauthor', 'person'),
         }
     reference_names = {
         (src_tab, src_col) for src_tab, src_col, _, _, _, _ in references}

=== modified file 'lib/lp/registry/scripts/tests/test_closeaccount.py'
--- lib/lp/registry/scripts/tests/test_closeaccount.py	2019-05-02 17:36:15 +0000
+++ lib/lp/registry/scripts/tests/test_closeaccount.py	2019-05-03 12:20:18 +0000
@@ -23,6 +23,8 @@
 from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.model.bugsummary import BugSummary
+from lp.code.enums import TargetRevisionControlSystems
+from lp.code.tests.helpers import GitHostingFixture
 from lp.hardwaredb.interfaces.hwdb import (
     HWBus,
     IHWDeviceSet,
@@ -491,3 +493,22 @@
             self.runScript(script)
         self.assertRemoved(account_id, person_id)
         self.assertEqual(person, product.owner)
+
+    def test_skips_code_import(self):
+        self.useFixture(GitHostingFixture())
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        code_imports = [
+            self.factory.makeCodeImport(
+                registrant=person, target_rcs_type=target_rcs_type, owner=team)
+            for target_rcs_type in (
+                TargetRevisionControlSystems.BZR,
+                TargetRevisionControlSystems.GIT)]
+        person_id = person.id
+        account_id = person.account.id
+        script = self.makeScript([six.ensure_str(person.name)])
+        with dbuser('launchpad'):
+            self.runScript(script)
+        self.assertRemoved(account_id, person_id)
+        self.assertEqual(person, code_imports[0].registrant)
+        self.assertEqual(person, code_imports[1].registrant)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2019-02-07 15:04:13 +0000
+++ lib/lp/testing/factory.py	2019-05-03 12:20:18 +0000
@@ -2408,7 +2408,7 @@
                        git_repo_url=None,
                        bzr_branch_url=None, registrant=None,
                        rcs_type=None, target_rcs_type=None,
-                       review_status=None):
+                       review_status=None, owner=None):
         """Create and return a new, arbitrary code import.
 
         The type of code import will be inferred from the source details
@@ -2439,20 +2439,20 @@
                 registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.BZR_SVN,
                 target_rcs_type=target_rcs_type,
-                url=svn_branch_url, review_status=review_status)
+                url=svn_branch_url, review_status=review_status, owner=owner)
         elif git_repo_url is not None:
             assert rcs_type in (None, RevisionControlSystems.GIT)
             return code_import_set.new(
                 registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.GIT,
                 target_rcs_type=target_rcs_type,
-                url=git_repo_url, review_status=review_status)
+                url=git_repo_url, review_status=review_status, owner=owner)
         elif bzr_branch_url is not None:
             return code_import_set.new(
                 registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.BZR,
                 target_rcs_type=target_rcs_type,
-                url=bzr_branch_url, review_status=review_status)
+                url=bzr_branch_url, review_status=review_status, owner=owner)
         else:
             assert rcs_type in (None, RevisionControlSystems.CVS)
             return code_import_set.new(
@@ -2460,7 +2460,7 @@
                 rcs_type=RevisionControlSystems.CVS,
                 target_rcs_type=target_rcs_type,
                 cvs_root=cvs_root, cvs_module=cvs_module,
-                review_status=review_status)
+                review_status=review_status, owner=owner)
 
     def makeChangelog(self, spn=None, versions=[]):
         """Create and return a LFA of a valid Debian-style changelog.