Merge ~andrey-fedoseev/launchpad:close-account into launchpad:master
- Git
- lp:~andrey-fedoseev/launchpad
- close-account
- Merge into master
Proposed by
Andrey Fedoseev
Status: | Merged |
---|---|
Approved by: | Andrey Fedoseev |
Approved revision: | 7f4c454f3aec41e3ee0690c081ea7bd0291e2978 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~andrey-fedoseev/launchpad:close-account |
Merge into: | launchpad:master |
Diff against target: |
334 lines (+241/-16) 2 files modified
lib/lp/registry/scripts/closeaccount.py (+72/-16) lib/lp/registry/scripts/tests/test_closeaccount.py (+169/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+428085@code.launchpad.net |
Commit message
close-account script: skip references related to inactive products
announcement.
milestonetag.
productrelease.
productreleasef
Description of the change
To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/registry/scripts/closeaccount.py b/lib/lp/registry/scripts/closeaccount.py |
2 | index c4e59fd..708bd5b 100644 |
3 | --- a/lib/lp/registry/scripts/closeaccount.py |
4 | +++ b/lib/lp/registry/scripts/closeaccount.py |
5 | @@ -8,9 +8,11 @@ __all__ = [ |
6 | "CloseAccountScript", |
7 | ] |
8 | |
9 | +from typing import List, Tuple |
10 | + |
11 | import six |
12 | from storm.exceptions import IntegrityError |
13 | -from storm.expr import LeftJoin, Lower, Or |
14 | +from storm.expr import And, LeftJoin, Lower, Or |
15 | from zope.component import getUtility |
16 | from zope.security.proxy import removeSecurityProxy |
17 | |
18 | @@ -19,8 +21,12 @@ from lp.answers.model.question import Question |
19 | from lp.app.interfaces.launchpad import ILaunchpadCelebrities |
20 | from lp.bugs.model.bugtask import BugTask |
21 | from lp.registry.interfaces.person import PersonCreationRationale |
22 | +from lp.registry.model.announcement import Announcement |
23 | +from lp.registry.model.milestone import Milestone |
24 | +from lp.registry.model.milestonetag import MilestoneTag |
25 | from lp.registry.model.person import Person, PersonSettings |
26 | from lp.registry.model.product import Product |
27 | +from lp.registry.model.productrelease import ProductRelease, ProductReleaseFile |
28 | from lp.registry.model.productseries import ProductSeries |
29 | from lp.services.database import postgresql |
30 | from lp.services.database.constants import DEFAULT |
31 | @@ -419,7 +425,7 @@ def close_account(username, log): |
32 | "Can't delete non-trivial PPAs for user %s" % person_name |
33 | ) |
34 | |
35 | - has_references = False |
36 | + reference_counts = [] # type: List[Tuple[str, int]] |
37 | |
38 | # Check for active related projects, and skip inactive ones. |
39 | for col in "bug_supervisor", "driver", "owner": |
40 | @@ -434,11 +440,7 @@ def close_account(username, log): |
41 | ) |
42 | count = result.get_one()[0] |
43 | if count: |
44 | - log.error( |
45 | - "User %s is still referenced by %d product.%s values" |
46 | - % (person_name, count, col) |
47 | - ) |
48 | - has_references = True |
49 | + reference_counts.append(("product.{}".format(col), count)) |
50 | skip.add(("product", col)) |
51 | for col in "driver", "owner": |
52 | count = store.find( |
53 | @@ -448,13 +450,65 @@ def close_account(username, log): |
54 | getattr(ProductSeries, col) == person, |
55 | ).count() |
56 | if count: |
57 | - log.error( |
58 | - "User %s is still referenced by %d productseries.%s values" |
59 | - % (person_name, count, col) |
60 | - ) |
61 | - has_references = True |
62 | + reference_counts.append(("productseries.{}".format(col), count)) |
63 | skip.add(("productseries", col)) |
64 | |
65 | + # Check announcements, skipping the ones |
66 | + # that are related to inactive products. |
67 | + count = store.find( |
68 | + Announcement, |
69 | + Or( |
70 | + And(Announcement.product == Product.id, Product.active), |
71 | + Announcement.product == None, |
72 | + ), |
73 | + Announcement.registrant == person, |
74 | + ).count() |
75 | + if count: |
76 | + reference_counts.append(("announcement.registrant", count)) |
77 | + skip.add(("announcement", "registrant")) |
78 | + |
79 | + # Check MilestoneTags, skipping the ones |
80 | + # that are related to inactive products / product series. |
81 | + count = store.find( |
82 | + MilestoneTag, |
83 | + MilestoneTag.milestone_id == Milestone.id, |
84 | + Or( |
85 | + And(Milestone.product == Product.id, Product.active), |
86 | + Milestone.product == None, |
87 | + ), |
88 | + MilestoneTag.created_by_id == person.id, |
89 | + ).count() |
90 | + if count: |
91 | + reference_counts.append(("milestonetag.created_by", count)) |
92 | + skip.add(("milestonetag", "created_by")) |
93 | + |
94 | + # Check ProductReleases, skipping the ones |
95 | + # that are related to inactive products / product series. |
96 | + count = store.find( |
97 | + ProductRelease, |
98 | + ProductRelease.milestone == Milestone.id, |
99 | + Milestone.product == Product.id, |
100 | + Product.active, |
101 | + ProductRelease.owner == person.id, |
102 | + ).count() |
103 | + if count: |
104 | + reference_counts.append(("productrelease.owner", count)) |
105 | + skip.add(("productrelease", "owner")) |
106 | + |
107 | + # Check ProductReleases, skipping the ones |
108 | + # that are related to inactive products / product series. |
109 | + count = store.find( |
110 | + ProductReleaseFile, |
111 | + ProductReleaseFile.productrelease == ProductRelease.id, |
112 | + ProductRelease.milestone == Milestone.id, |
113 | + Milestone.product == Product.id, |
114 | + Product.active, |
115 | + ProductReleaseFile.uploader == person.id, |
116 | + ).count() |
117 | + if count: |
118 | + reference_counts.append(("productreleasefile.uploader", count)) |
119 | + skip.add(("productreleasefile", "uploader")) |
120 | + |
121 | # Closing the account will only work if all references have been handled |
122 | # by this point. If not, it's safer to bail out. It's OK if this |
123 | # doesn't work in all conceivable situations, since some of them may |
124 | @@ -474,12 +528,14 @@ def close_account(username, log): |
125 | ) |
126 | count = result.get_one()[0] |
127 | if count: |
128 | + reference_counts.append(("{}.{}".format(src_tab, src_col), count)) |
129 | + |
130 | + if reference_counts: |
131 | + for reference, count in reference_counts: |
132 | log.error( |
133 | - "User %s is still referenced by %d %s.%s values" |
134 | - % (person_name, count, src_tab, src_col) |
135 | + "User %s is still referenced by %d %s values" |
136 | + % (person_name, count, reference) |
137 | ) |
138 | - has_references = True |
139 | - if has_references: |
140 | raise LaunchpadScriptFailure( |
141 | "User %s is still referenced" % person_name |
142 | ) |
143 | diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py |
144 | index 348b747..b78ff60 100644 |
145 | --- a/lib/lp/registry/scripts/tests/test_closeaccount.py |
146 | +++ b/lib/lp/registry/scripts/tests/test_closeaccount.py |
147 | @@ -2,7 +2,9 @@ |
148 | # GNU Affero General Public License version 3 (see the file LICENSE). |
149 | |
150 | """Test the close-account script.""" |
151 | +from datetime import datetime |
152 | |
153 | +import pytz |
154 | import transaction |
155 | from storm.store import Store |
156 | from testtools.matchers import ( |
157 | @@ -25,6 +27,7 @@ from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow |
158 | from lp.code.tests.helpers import GitHostingFixture |
159 | from lp.registry.interfaces.person import IPersonSet |
160 | from lp.registry.interfaces.teammembership import ITeamMembershipSet |
161 | +from lp.registry.model.productrelease import ProductRelease |
162 | from lp.registry.scripts.closeaccount import CloseAccountScript |
163 | from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache |
164 | from lp.services.database.interfaces import IStore |
165 | @@ -1005,3 +1008,169 @@ class TestCloseAccount(TestCaseWithFactory): |
166 | self.assertRemoved(account_id, person_id) |
167 | self.assertIsNone(store.get(Archive, ppa_id)) |
168 | self.assertEqual(other_ppa, store.get(Archive, other_ppa_id)) |
169 | + |
170 | + def test_skips_announcements_in_deactivated_products(self): |
171 | + person = self.factory.makePerson() |
172 | + person_id = person.id |
173 | + account_id = person.account.id |
174 | + |
175 | + product = self.factory.makeProduct() |
176 | + product.announce(person, "announcement") |
177 | + |
178 | + script = self.makeScript([person.name]) |
179 | + with dbuser("launchpad"): |
180 | + self.assertRaisesWithContent( |
181 | + LaunchpadScriptFailure, |
182 | + "User %s is still referenced" % person.name, |
183 | + self.runScript, |
184 | + script, |
185 | + ) |
186 | + self.assertNotRemoved(account_id, person_id) |
187 | + |
188 | + product.active = False |
189 | + with dbuser("launchpad"): |
190 | + self.runScript(script) |
191 | + |
192 | + self.assertRemoved(account_id, person_id) |
193 | + |
194 | + def test_non_product_announcements_are_not_skipped(self): |
195 | + person = self.factory.makePerson() |
196 | + person_id = person.id |
197 | + account_id = person.account.id |
198 | + |
199 | + distro = self.factory.makeDistribution() |
200 | + distro.announce(person, "announcement") |
201 | + |
202 | + script = self.makeScript([person.name]) |
203 | + with dbuser("launchpad"): |
204 | + self.assertRaisesWithContent( |
205 | + LaunchpadScriptFailure, |
206 | + "User %s is still referenced" % person.name, |
207 | + self.runScript, |
208 | + script, |
209 | + ) |
210 | + self.assertNotRemoved(account_id, person_id) |
211 | + |
212 | + def test_skip_milestone_tags_from_inactive_products(self): |
213 | + |
214 | + active_product = self.factory.makeProduct() |
215 | + inactive_product = self.factory.makeProduct() |
216 | + inactive_product.active = False |
217 | + |
218 | + active_product_series = self.factory.makeProductSeries( |
219 | + product=active_product |
220 | + ) |
221 | + inactive_product_series = self.factory.makeProductSeries( |
222 | + product=inactive_product |
223 | + ) |
224 | + |
225 | + for milestone_target, expected_to_be_removed in ( |
226 | + ({"product": active_product}, False), |
227 | + ({"product": inactive_product}, True), |
228 | + ({"productseries": active_product_series}, False), |
229 | + ({"productseries": inactive_product_series}, True), |
230 | + ({"distribution": self.factory.makeDistribution()}, False), |
231 | + ({"distroseries": self.factory.makeDistroSeries()}, False), |
232 | + ): |
233 | + person = self.factory.makePerson() |
234 | + person_id = person.id |
235 | + account_id = person.account.id |
236 | + |
237 | + milestone = self.factory.makeMilestone(**milestone_target) |
238 | + milestone.setTags(["tag"], person) |
239 | + script = self.makeScript([person.name]) |
240 | + with dbuser("launchpad"): |
241 | + if not expected_to_be_removed: |
242 | + self.assertRaisesWithContent( |
243 | + LaunchpadScriptFailure, |
244 | + "User %s is still referenced" % person.name, |
245 | + self.runScript, |
246 | + script, |
247 | + ) |
248 | + self.assertNotRemoved(account_id, person_id) |
249 | + else: |
250 | + self.runScript(script) |
251 | + self.assertRemoved(account_id, person_id) |
252 | + |
253 | + def test_skip_product_releases_from_inactive_products(self): |
254 | + |
255 | + active_product = self.factory.makeProduct() |
256 | + inactive_product = self.factory.makeProduct() |
257 | + inactive_product.active = False |
258 | + |
259 | + active_product_series = self.factory.makeProductSeries( |
260 | + product=active_product |
261 | + ) |
262 | + inactive_product_series = self.factory.makeProductSeries( |
263 | + product=inactive_product |
264 | + ) |
265 | + |
266 | + for milestone_target, expected_to_be_removed in ( |
267 | + ({"product": active_product}, False), |
268 | + ({"product": inactive_product}, True), |
269 | + ({"productseries": active_product_series}, False), |
270 | + ({"productseries": inactive_product_series}, True), |
271 | + ): |
272 | + person = self.factory.makePerson() |
273 | + person_id = person.id |
274 | + account_id = person.account.id |
275 | + |
276 | + milestone = self.factory.makeMilestone(**milestone_target) |
277 | + milestone.createProductRelease(person, datetime.now(pytz.UTC)) |
278 | + script = self.makeScript([person.name]) |
279 | + with dbuser("launchpad"): |
280 | + if not expected_to_be_removed: |
281 | + self.assertRaisesWithContent( |
282 | + LaunchpadScriptFailure, |
283 | + "User %s is still referenced" % person.name, |
284 | + self.runScript, |
285 | + script, |
286 | + ) |
287 | + self.assertNotRemoved(account_id, person_id) |
288 | + else: |
289 | + self.runScript(script) |
290 | + self.assertRemoved(account_id, person_id) |
291 | + |
292 | + def test_skip_product_releases_files_from_inactive_products(self): |
293 | + |
294 | + active_product = self.factory.makeProduct() |
295 | + inactive_product = self.factory.makeProduct() |
296 | + inactive_product.active = False |
297 | + |
298 | + active_product_series = self.factory.makeProductSeries( |
299 | + product=active_product |
300 | + ) |
301 | + inactive_product_series = self.factory.makeProductSeries( |
302 | + product=inactive_product |
303 | + ) |
304 | + |
305 | + for milestone_target, expected_to_be_removed in ( |
306 | + ({"product": active_product}, False), |
307 | + ({"product": inactive_product}, True), |
308 | + ({"productseries": active_product_series}, False), |
309 | + ({"productseries": inactive_product_series}, True), |
310 | + ): |
311 | + person = self.factory.makePerson() |
312 | + person_id = person.id |
313 | + account_id = person.account.id |
314 | + |
315 | + milestone = self.factory.makeMilestone(**milestone_target) |
316 | + product_release = milestone.createProductRelease( |
317 | + milestone.product.owner, datetime.now(pytz.UTC) |
318 | + ) # type: ProductRelease |
319 | + product_release.addReleaseFile( |
320 | + "test.txt", b"test", "text/plain", person |
321 | + ) |
322 | + script = self.makeScript([person.name]) |
323 | + with dbuser("launchpad"): |
324 | + if not expected_to_be_removed: |
325 | + self.assertRaisesWithContent( |
326 | + LaunchpadScriptFailure, |
327 | + "User %s is still referenced" % person.name, |
328 | + self.runScript, |
329 | + script, |
330 | + ) |
331 | + self.assertNotRemoved(account_id, person_id) |
332 | + else: |
333 | + self.runScript(script) |
334 | + self.assertRemoved(account_id, person_id) |