launchpad-reviewers team mailing list archive

[Colin Watson <mp+433735@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-close-account-joins into launchpad:master.

Commit message:
close-account: Fix reported reference counts from several columns

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/433735

In cases involving a nullable foreign key, we need to use left joins to get correct counts.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-close-account-joins into launchpad:master.

diff --git a/lib/lp/registry/scripts/closeaccount.py b/lib/lp/registry/scripts/closeaccount.py
index 505f4e8..df4c8ad 100644
--- a/lib/lp/registry/scripts/closeaccount.py
+++ b/lib/lp/registry/scripts/closeaccount.py
@@ -11,7 +11,7 @@ __all__ = [
 from typing import List, Tuple
 
 import six
-from storm.expr import And, LeftJoin, Lower, Or
+from storm.expr import Join, LeftJoin, Lower, Or
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -491,29 +491,37 @@ def close_account(username, log):
 
     # Check announcements, skipping the ones
     # that are related to inactive products.
-    count = store.find(
-        Announcement,
-        Or(
-            And(Announcement.product == Product.id, Product.active),
-            Announcement.product == None,
-        ),
-        Announcement.registrant == person,
-    ).count()
+    count = (
+        store.using(
+            Announcement,
+            LeftJoin(Product, Announcement.product == Product.id),
+        )
+        .find(
+            Announcement,
+            Or(Product.active, Announcement.product == None),
+            Announcement.registrant == person,
+        )
+        .count()
+    )
     if count:
         reference_counts.append(("announcement.registrant", count))
     skip.add(("announcement", "registrant"))
 
     # Check MilestoneTags, skipping the ones
     # that are related to inactive products / product series.
-    count = store.find(
-        MilestoneTag,
-        MilestoneTag.milestone_id == Milestone.id,
-        Or(
-            And(Milestone.product == Product.id, Product.active),
-            Milestone.product == None,
-        ),
-        MilestoneTag.created_by_id == person.id,
-    ).count()
+    count = (
+        store.using(
+            MilestoneTag,
+            Join(Milestone, MilestoneTag.milestone_id == Milestone.id),
+            LeftJoin(Product, Milestone.product == Product.id),
+        )
+        .find(
+            MilestoneTag,
+            Or(Product.active, Milestone.product == None),
+            MilestoneTag.created_by_id == person.id,
+        )
+        .count()
+    )
     if count:
         reference_counts.append(("milestonetag.created_by", count))
     skip.add(("milestonetag", "created_by"))
@@ -531,7 +539,7 @@ def close_account(username, log):
         reference_counts.append(("productrelease.owner", count))
     skip.add(("productrelease", "owner"))
 
-    # Check ProductReleases, skipping the ones
+    # Check ProductReleaseFiles, skipping the ones
     # that are related to inactive products / product series.
     count = store.find(
         ProductReleaseFile,
@@ -547,34 +555,36 @@ def close_account(username, log):
 
     # Check Branches, skipping the ones that are related to inactive products.
     for col_name in "owner", "reviewer":
-        count = store.find(
-            Branch,
-            Or(
-                And(
-                    Branch.product == Product.id,
-                    Product.active,
-                ),
-                Branch.product == None,
-            ),
-            getattr(Branch, col_name) == person.id,
-        ).count()
+        count = (
+            store.using(
+                Branch,
+                LeftJoin(Product, Branch.product == Product.id),
+            )
+            .find(
+                Branch,
+                Or(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()
+    count = (
+        store.using(
+            Specification,
+            LeftJoin(Product, Specification.product == Product.id),
+        )
+        .find(
+            Specification,
+            Or(Product.active, Specification.product == None),
+            Specification._assignee == person.id,
+        )
+        .count()
+    )
     if count:
         reference_counts.append(("specification.assignee", count))
     skip.add(("specification", "assignee"))
diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py
index 7a13f9d..f63aebf 100644
--- a/lib/lp/registry/scripts/tests/test_closeaccount.py
+++ b/lib/lp/registry/scripts/tests/test_closeaccount.py
@@ -1044,10 +1044,14 @@ class TestCloseAccount(TestCaseWithFactory):
                 self.runScript,
                 script,
             )
+        self.assertIn(
+            "ERROR User %s is still referenced by 1 announcement.registrant "
+            "values" % person.name,
+            script.logger.getLogBuffer(),
+        )
         self.assertNotRemoved(account_id, person_id)
 
     def test_skip_milestone_tags_from_inactive_products(self):
-
         active_product = self.factory.makeProduct()
         inactive_product = self.factory.makeProduct()
         inactive_product.active = False
@@ -1082,6 +1086,11 @@ class TestCloseAccount(TestCaseWithFactory):
                         self.runScript,
                         script,
                     )
+                    self.assertIn(
+                        "ERROR User %s is still referenced by 1 "
+                        "milestonetag.created_by values" % person.name,
+                        script.logger.getLogBuffer(),
+                    )
                     self.assertNotRemoved(account_id, person_id)
                 else:
                     self.runScript(script)
@@ -1196,6 +1205,11 @@ class TestCloseAccount(TestCaseWithFactory):
                             self.runScript,
                             script,
                         )
+                        self.assertIn(
+                            "ERROR User %s is still referenced by 1 "
+                            "branch.%s values" % (person.name, col_name),
+                            script.logger.getLogBuffer(),
+                        )
                         self.assertNotRemoved(account_id, person_id)
                     else:
                         self.runScript(script)
@@ -1227,6 +1241,11 @@ class TestCloseAccount(TestCaseWithFactory):
                         self.runScript,
                         script,
                     )
+                    self.assertIn(
+                        "ERROR User %s is still referenced by 1 "
+                        "specification.assignee values" % person.name,
+                        script.logger.getLogBuffer(),
+                    )
                     self.assertNotRemoved(account_id, person_id)
                 else:
                     self.runScript(script)