launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28976
[Merge] ~andrey-fedoseev/launchpad:close-account into launchpad:master
Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:close-account into launchpad:master.
Commit message:
close-account script: skip references related to inactive products
announcement.registrant
milestonetag.created_by
productrelease.owner
productreleasefile.uploader
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/428085
--
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 c4e59fd..31e64ad 100644
--- a/lib/lp/registry/scripts/closeaccount.py
+++ b/lib/lp/registry/scripts/closeaccount.py
@@ -8,9 +8,11 @@ __all__ = [
"CloseAccountScript",
]
+from typing import List, Tuple
+
import six
from storm.exceptions import IntegrityError
-from storm.expr import LeftJoin, Lower, Or
+from storm.expr import And, LeftJoin, Lower, Or
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -19,8 +21,12 @@ from lp.answers.model.question import Question
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.bugs.model.bugtask import BugTask
from lp.registry.interfaces.person import PersonCreationRationale
+from lp.registry.model.announcement import Announcement
+from lp.registry.model.milestone import Milestone
+from lp.registry.model.milestonetag import MilestoneTag
from lp.registry.model.person import Person, PersonSettings
from lp.registry.model.product import Product
+from lp.registry.model.productrelease import ProductRelease, ProductReleaseFile
from lp.registry.model.productseries import ProductSeries
from lp.services.database import postgresql
from lp.services.database.constants import DEFAULT
@@ -419,7 +425,7 @@ def close_account(username, log):
"Can't delete non-trivial PPAs for user %s" % person_name
)
- has_references = False
+ reference_counts = [] # type: List[Tuple[str, int]]
# Check for active related projects, and skip inactive ones.
for col in "bug_supervisor", "driver", "owner":
@@ -434,11 +440,7 @@ def close_account(username, log):
)
count = result.get_one()[0]
if count:
- log.error(
- "User %s is still referenced by %d product.%s values"
- % (person_name, count, col)
- )
- has_references = True
+ reference_counts.append(("product.{}".format(col), count))
skip.add(("product", col))
for col in "driver", "owner":
count = store.find(
@@ -448,13 +450,65 @@ def close_account(username, log):
getattr(ProductSeries, col) == person,
).count()
if count:
- log.error(
- "User %s is still referenced by %d productseries.%s values"
- % (person_name, count, col)
- )
- has_references = True
+ reference_counts.append(("productseries.{}".format(col), count))
skip.add(("productseries", col))
+ # 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()
+ 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()
+ if count:
+ reference_counts.append(("milestonetag.created_by", count))
+ skip.add(("milestonetag", "created_by"))
+
+ # Check ProduceReleases, skipping the ones
+ # that are related to inactive products / product series.
+ count = store.find(
+ ProductRelease,
+ ProductRelease.milestone == Milestone.id,
+ Milestone.product == Product.id,
+ Product.active,
+ ProductRelease.owner == person.id,
+ ).count()
+ if count:
+ reference_counts.append(("productrelease.owner", count))
+ skip.add(("productrelease", "owner"))
+
+ # Check ProduceReleases, skipping the ones
+ # that are related to inactive products / product series.
+ count = store.find(
+ ProductReleaseFile,
+ ProductReleaseFile.productrelease == ProductRelease.id,
+ ProductRelease.milestone == Milestone.id,
+ Milestone.product == Product.id,
+ Product.active,
+ ProductReleaseFile.uploader == person.id,
+ ).count()
+ if count:
+ reference_counts.append(("productrelease.owner", count))
+ skip.add(("productreleasefile", "uploader"))
+
# 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
@@ -474,12 +528,14 @@ def close_account(username, log):
)
count = result.get_one()[0]
if count:
+ reference_counts.append(("{}.{}".format(src_tab, src_col), count))
+
+ if reference_counts:
+ for reference, count in reference_counts:
log.error(
- "User %s is still referenced by %d %s.%s values"
- % (person_name, count, src_tab, src_col)
+ "User %s is still referenced by %d %s values"
+ % (person_name, count, reference)
)
- has_references = True
- if has_references:
raise LaunchpadScriptFailure(
"User %s is still referenced" % person_name
)
diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py
index 348b747..b78ff60 100644
--- a/lib/lp/registry/scripts/tests/test_closeaccount.py
+++ b/lib/lp/registry/scripts/tests/test_closeaccount.py
@@ -2,7 +2,9 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test the close-account script."""
+from datetime import datetime
+import pytz
import transaction
from storm.store import Store
from testtools.matchers import (
@@ -25,6 +27,7 @@ from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
from lp.code.tests.helpers import GitHostingFixture
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.teammembership import ITeamMembershipSet
+from lp.registry.model.productrelease import ProductRelease
from lp.registry.scripts.closeaccount import CloseAccountScript
from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache
from lp.services.database.interfaces import IStore
@@ -1005,3 +1008,169 @@ class TestCloseAccount(TestCaseWithFactory):
self.assertRemoved(account_id, person_id)
self.assertIsNone(store.get(Archive, ppa_id))
self.assertEqual(other_ppa, store.get(Archive, other_ppa_id))
+
+ def test_skips_announcements_in_deactivated_products(self):
+ person = self.factory.makePerson()
+ person_id = person.id
+ account_id = person.account.id
+
+ product = self.factory.makeProduct()
+ product.announce(person, "announcement")
+
+ script = self.makeScript([person.name])
+ with dbuser("launchpad"):
+ self.assertRaisesWithContent(
+ LaunchpadScriptFailure,
+ "User %s is still referenced" % person.name,
+ self.runScript,
+ script,
+ )
+ self.assertNotRemoved(account_id, person_id)
+
+ product.active = False
+ with dbuser("launchpad"):
+ self.runScript(script)
+
+ self.assertRemoved(account_id, person_id)
+
+ def test_non_product_announcements_are_not_skipped(self):
+ person = self.factory.makePerson()
+ person_id = person.id
+ account_id = person.account.id
+
+ distro = self.factory.makeDistribution()
+ distro.announce(person, "announcement")
+
+ script = self.makeScript([person.name])
+ with dbuser("launchpad"):
+ self.assertRaisesWithContent(
+ LaunchpadScriptFailure,
+ "User %s is still referenced" % person.name,
+ self.runScript,
+ script,
+ )
+ 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
+
+ active_product_series = self.factory.makeProductSeries(
+ product=active_product
+ )
+ inactive_product_series = self.factory.makeProductSeries(
+ product=inactive_product
+ )
+
+ for milestone_target, expected_to_be_removed in (
+ ({"product": active_product}, False),
+ ({"product": inactive_product}, True),
+ ({"productseries": active_product_series}, False),
+ ({"productseries": inactive_product_series}, True),
+ ({"distribution": self.factory.makeDistribution()}, False),
+ ({"distroseries": self.factory.makeDistroSeries()}, False),
+ ):
+ person = self.factory.makePerson()
+ person_id = person.id
+ account_id = person.account.id
+
+ milestone = self.factory.makeMilestone(**milestone_target)
+ milestone.setTags(["tag"], person)
+ 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_product_releases_from_inactive_products(self):
+
+ active_product = self.factory.makeProduct()
+ inactive_product = self.factory.makeProduct()
+ inactive_product.active = False
+
+ active_product_series = self.factory.makeProductSeries(
+ product=active_product
+ )
+ inactive_product_series = self.factory.makeProductSeries(
+ product=inactive_product
+ )
+
+ for milestone_target, expected_to_be_removed in (
+ ({"product": active_product}, False),
+ ({"product": inactive_product}, True),
+ ({"productseries": active_product_series}, False),
+ ({"productseries": inactive_product_series}, True),
+ ):
+ person = self.factory.makePerson()
+ person_id = person.id
+ account_id = person.account.id
+
+ milestone = self.factory.makeMilestone(**milestone_target)
+ milestone.createProductRelease(person, datetime.now(pytz.UTC))
+ 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_product_releases_files_from_inactive_products(self):
+
+ active_product = self.factory.makeProduct()
+ inactive_product = self.factory.makeProduct()
+ inactive_product.active = False
+
+ active_product_series = self.factory.makeProductSeries(
+ product=active_product
+ )
+ inactive_product_series = self.factory.makeProductSeries(
+ product=inactive_product
+ )
+
+ for milestone_target, expected_to_be_removed in (
+ ({"product": active_product}, False),
+ ({"product": inactive_product}, True),
+ ({"productseries": active_product_series}, False),
+ ({"productseries": inactive_product_series}, True),
+ ):
+ person = self.factory.makePerson()
+ person_id = person.id
+ account_id = person.account.id
+
+ milestone = self.factory.makeMilestone(**milestone_target)
+ product_release = milestone.createProductRelease(
+ milestone.product.owner, datetime.now(pytz.UTC)
+ ) # type: ProductRelease
+ product_release.addReleaseFile(
+ "test.txt", b"test", "text/plain", person
+ )
+ 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)
