launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+428163@xxxxxxxxxxxxxxxxxx>]

Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:close-account into launchpad:master.

Commit message:
Skip certain fields reference checking in `close-account`:

 - branch.{owner,reviewer,registrant} from inactive products
 - specification.assignee from inactive products

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/428163
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:close-account into launchpad:master.

diff --git a/lib/lp/registry/scripts/closeaccount.py b/lib/lp/registry/scripts/closeaccount.py
index 708bd5b..063e0a5 100644
--- a/lib/lp/registry/scripts/closeaccount.py
+++ b/lib/lp/registry/scripts/closeaccount.py
@@ -19,7 +19,9 @@ 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.blueprints.model.specification import Specification
 from lp.bugs.model.bugtask import BugTask
+from lp.code.model.branch import Branch
 from lp.registry.interfaces.person import PersonCreationRationale
 from lp.registry.model.announcement import Announcement
 from lp.registry.model.milestone import Milestone
@@ -509,6 +511,40 @@ def close_account(username, log):
         reference_counts.append(("productreleasefile.uploader", count))
     skip.add(("productreleasefile", "uploader"))
 
+    # Check Branches, skipping the ones that are related to inactive products.
+    for col_name in "owner", "reviewer", "registrant":
+        count = store.find(
+            Branch,
+            Or(
+                And(
+                    Branch.product == Product.id,
+                    Product.active,
+                ),
+                Branch.product == None,
+            ),
+            getattr(Branch, col_name) == person.id,
+        ).count()
+        if count:
+            reference_counts.append(("branch.{}".format(col_name), count))
+        skip.add(("branch", col_name))
+
+    # Check Specification, skipping the ones
+    # that are related to inactive products / product series.
+    count = store.find(
+        Specification,
+        Or(
+            And(
+                Specification.product == Product.id,
+                Product.active,
+            ),
+            Specification.product == None,
+        ),
+        Specification._assignee == person.id,
+    ).count()
+    if count:
+        reference_counts.append(("specification.assignee", count))
+    skip.add(("specification", "assignee"))
+
     # 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
diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py
index b78ff60..9cac8fc 100644
--- a/lib/lp/registry/scripts/tests/test_closeaccount.py
+++ b/lib/lp/registry/scripts/tests/test_closeaccount.py
@@ -1174,3 +1174,71 @@ class TestCloseAccount(TestCaseWithFactory):
                 else:
                     self.runScript(script)
                     self.assertRemoved(account_id, person_id)
+
+    def test_skip_branches_from_inactive_products(self):
+        active_product = self.factory.makeProduct()
+        inactive_product = self.factory.makeProduct()
+        inactive_product.active = False
+
+        for branch_target, expected_to_be_removed in (
+            (active_product, False),
+            (inactive_product, True),
+            (self.factory.makeSourcePackage(), False),
+        ):
+            for col_name in "owner", "reviewer", "registrant":
+                person = self.factory.makePerson()
+                make_branch_kwargs = {col_name: person}
+                if col_name == "registrant":
+                    make_branch_kwargs["owner"] = self.factory.makeTeam(
+                        members=[person]
+                    )
+                self.factory.makeBranch(
+                    target=branch_target, **make_branch_kwargs
+                )
+
+                person_id = person.id
+                account_id = person.account.id
+                script = self.makeScript([person.name])
+                with dbuser("launchpad"):
+                    if not expected_to_be_removed:
+                        self.assertRaisesWithContent(
+                            LaunchpadScriptFailure,
+                            "User %s is still referenced" % person.name,
+                            self.runScript,
+                            script,
+                        )
+                        self.assertNotRemoved(account_id, person_id)
+                    else:
+                        self.runScript(script)
+                        self.assertRemoved(account_id, person_id)
+
+    def test_skip_specifications_from_inactive_products(self):
+        active_product = self.factory.makeProduct()
+        inactive_product = self.factory.makeProduct()
+        inactive_product.active = False
+
+        for specification_target, expected_to_be_removed in (
+            ({"product": active_product}, False),
+            ({"product": inactive_product}, True),
+            ({"distribution": self.factory.makeDistribution()}, False),
+        ):
+            person = self.factory.makePerson()
+            person_id = person.id
+            account_id = person.account.id
+
+            self.factory.makeSpecification(
+                assignee=person, **specification_target
+            )
+            script = self.makeScript([person.name])
+            with dbuser("launchpad"):
+                if not expected_to_be_removed:
+                    self.assertRaisesWithContent(
+                        LaunchpadScriptFailure,
+                        "User %s is still referenced" % person.name,
+                        self.runScript,
+                        script,
+                    )
+                    self.assertNotRemoved(account_id, person_id)
+                else:
+                    self.runScript(script)
+                    self.assertRemoved(account_id, person_id)