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 | 8 | "CloseAccountScript", | 8 | "CloseAccountScript", |
7 | 9 | ] | 9 | ] |
8 | 10 | 10 | ||
9 | 11 | from typing import List, Tuple | ||
10 | 12 | |||
11 | 11 | import six | 13 | import six |
12 | 12 | from storm.exceptions import IntegrityError | 14 | from storm.exceptions import IntegrityError |
14 | 13 | from storm.expr import LeftJoin, Lower, Or | 15 | from storm.expr import And, LeftJoin, Lower, Or |
15 | 14 | from zope.component import getUtility | 16 | from zope.component import getUtility |
16 | 15 | from zope.security.proxy import removeSecurityProxy | 17 | from zope.security.proxy import removeSecurityProxy |
17 | 16 | 18 | ||
18 | @@ -19,8 +21,12 @@ from lp.answers.model.question import Question | |||
19 | 19 | from lp.app.interfaces.launchpad import ILaunchpadCelebrities | 21 | from lp.app.interfaces.launchpad import ILaunchpadCelebrities |
20 | 20 | from lp.bugs.model.bugtask import BugTask | 22 | from lp.bugs.model.bugtask import BugTask |
21 | 21 | from lp.registry.interfaces.person import PersonCreationRationale | 23 | from lp.registry.interfaces.person import PersonCreationRationale |
22 | 24 | from lp.registry.model.announcement import Announcement | ||
23 | 25 | from lp.registry.model.milestone import Milestone | ||
24 | 26 | from lp.registry.model.milestonetag import MilestoneTag | ||
25 | 22 | from lp.registry.model.person import Person, PersonSettings | 27 | from lp.registry.model.person import Person, PersonSettings |
26 | 23 | from lp.registry.model.product import Product | 28 | from lp.registry.model.product import Product |
27 | 29 | from lp.registry.model.productrelease import ProductRelease, ProductReleaseFile | ||
28 | 24 | from lp.registry.model.productseries import ProductSeries | 30 | from lp.registry.model.productseries import ProductSeries |
29 | 25 | from lp.services.database import postgresql | 31 | from lp.services.database import postgresql |
30 | 26 | from lp.services.database.constants import DEFAULT | 32 | from lp.services.database.constants import DEFAULT |
31 | @@ -419,7 +425,7 @@ def close_account(username, log): | |||
32 | 419 | "Can't delete non-trivial PPAs for user %s" % person_name | 425 | "Can't delete non-trivial PPAs for user %s" % person_name |
33 | 420 | ) | 426 | ) |
34 | 421 | 427 | ||
36 | 422 | has_references = False | 428 | reference_counts = [] # type: List[Tuple[str, int]] |
37 | 423 | 429 | ||
38 | 424 | # Check for active related projects, and skip inactive ones. | 430 | # Check for active related projects, and skip inactive ones. |
39 | 425 | for col in "bug_supervisor", "driver", "owner": | 431 | for col in "bug_supervisor", "driver", "owner": |
40 | @@ -434,11 +440,7 @@ def close_account(username, log): | |||
41 | 434 | ) | 440 | ) |
42 | 435 | count = result.get_one()[0] | 441 | count = result.get_one()[0] |
43 | 436 | if count: | 442 | if count: |
49 | 437 | log.error( | 443 | reference_counts.append(("product.{}".format(col), count)) |
45 | 438 | "User %s is still referenced by %d product.%s values" | ||
46 | 439 | % (person_name, count, col) | ||
47 | 440 | ) | ||
48 | 441 | has_references = True | ||
50 | 442 | skip.add(("product", col)) | 444 | skip.add(("product", col)) |
51 | 443 | for col in "driver", "owner": | 445 | for col in "driver", "owner": |
52 | 444 | count = store.find( | 446 | count = store.find( |
53 | @@ -448,13 +450,65 @@ def close_account(username, log): | |||
54 | 448 | getattr(ProductSeries, col) == person, | 450 | getattr(ProductSeries, col) == person, |
55 | 449 | ).count() | 451 | ).count() |
56 | 450 | if count: | 452 | if count: |
62 | 451 | log.error( | 453 | reference_counts.append(("productseries.{}".format(col), count)) |
58 | 452 | "User %s is still referenced by %d productseries.%s values" | ||
59 | 453 | % (person_name, count, col) | ||
60 | 454 | ) | ||
61 | 455 | has_references = True | ||
63 | 456 | skip.add(("productseries", col)) | 454 | skip.add(("productseries", col)) |
64 | 457 | 455 | ||
65 | 456 | # Check announcements, skipping the ones | ||
66 | 457 | # that are related to inactive products. | ||
67 | 458 | count = store.find( | ||
68 | 459 | Announcement, | ||
69 | 460 | Or( | ||
70 | 461 | And(Announcement.product == Product.id, Product.active), | ||
71 | 462 | Announcement.product == None, | ||
72 | 463 | ), | ||
73 | 464 | Announcement.registrant == person, | ||
74 | 465 | ).count() | ||
75 | 466 | if count: | ||
76 | 467 | reference_counts.append(("announcement.registrant", count)) | ||
77 | 468 | skip.add(("announcement", "registrant")) | ||
78 | 469 | |||
79 | 470 | # Check MilestoneTags, skipping the ones | ||
80 | 471 | # that are related to inactive products / product series. | ||
81 | 472 | count = store.find( | ||
82 | 473 | MilestoneTag, | ||
83 | 474 | MilestoneTag.milestone_id == Milestone.id, | ||
84 | 475 | Or( | ||
85 | 476 | And(Milestone.product == Product.id, Product.active), | ||
86 | 477 | Milestone.product == None, | ||
87 | 478 | ), | ||
88 | 479 | MilestoneTag.created_by_id == person.id, | ||
89 | 480 | ).count() | ||
90 | 481 | if count: | ||
91 | 482 | reference_counts.append(("milestonetag.created_by", count)) | ||
92 | 483 | skip.add(("milestonetag", "created_by")) | ||
93 | 484 | |||
94 | 485 | # Check ProductReleases, skipping the ones | ||
95 | 486 | # that are related to inactive products / product series. | ||
96 | 487 | count = store.find( | ||
97 | 488 | ProductRelease, | ||
98 | 489 | ProductRelease.milestone == Milestone.id, | ||
99 | 490 | Milestone.product == Product.id, | ||
100 | 491 | Product.active, | ||
101 | 492 | ProductRelease.owner == person.id, | ||
102 | 493 | ).count() | ||
103 | 494 | if count: | ||
104 | 495 | reference_counts.append(("productrelease.owner", count)) | ||
105 | 496 | skip.add(("productrelease", "owner")) | ||
106 | 497 | |||
107 | 498 | # Check ProductReleases, skipping the ones | ||
108 | 499 | # that are related to inactive products / product series. | ||
109 | 500 | count = store.find( | ||
110 | 501 | ProductReleaseFile, | ||
111 | 502 | ProductReleaseFile.productrelease == ProductRelease.id, | ||
112 | 503 | ProductRelease.milestone == Milestone.id, | ||
113 | 504 | Milestone.product == Product.id, | ||
114 | 505 | Product.active, | ||
115 | 506 | ProductReleaseFile.uploader == person.id, | ||
116 | 507 | ).count() | ||
117 | 508 | if count: | ||
118 | 509 | reference_counts.append(("productreleasefile.uploader", count)) | ||
119 | 510 | skip.add(("productreleasefile", "uploader")) | ||
120 | 511 | |||
121 | 458 | # Closing the account will only work if all references have been handled | 512 | # Closing the account will only work if all references have been handled |
122 | 459 | # by this point. If not, it's safer to bail out. It's OK if this | 513 | # by this point. If not, it's safer to bail out. It's OK if this |
123 | 460 | # doesn't work in all conceivable situations, since some of them may | 514 | # doesn't work in all conceivable situations, since some of them may |
124 | @@ -474,12 +528,14 @@ def close_account(username, log): | |||
125 | 474 | ) | 528 | ) |
126 | 475 | count = result.get_one()[0] | 529 | count = result.get_one()[0] |
127 | 476 | if count: | 530 | if count: |
128 | 531 | reference_counts.append(("{}.{}".format(src_tab, src_col), count)) | ||
129 | 532 | |||
130 | 533 | if reference_counts: | ||
131 | 534 | for reference, count in reference_counts: | ||
132 | 477 | log.error( | 535 | log.error( |
135 | 478 | "User %s is still referenced by %d %s.%s values" | 536 | "User %s is still referenced by %d %s values" |
136 | 479 | % (person_name, count, src_tab, src_col) | 537 | % (person_name, count, reference) |
137 | 480 | ) | 538 | ) |
138 | 481 | has_references = True | ||
139 | 482 | if has_references: | ||
140 | 483 | raise LaunchpadScriptFailure( | 539 | raise LaunchpadScriptFailure( |
141 | 484 | "User %s is still referenced" % person_name | 540 | "User %s is still referenced" % person_name |
142 | 485 | ) | 541 | ) |
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 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). |
149 | 3 | 3 | ||
150 | 4 | """Test the close-account script.""" | 4 | """Test the close-account script.""" |
151 | 5 | from datetime import datetime | ||
152 | 5 | 6 | ||
153 | 7 | import pytz | ||
154 | 6 | import transaction | 8 | import transaction |
155 | 7 | from storm.store import Store | 9 | from storm.store import Store |
156 | 8 | from testtools.matchers import ( | 10 | from testtools.matchers import ( |
157 | @@ -25,6 +27,7 @@ from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow | |||
158 | 25 | from lp.code.tests.helpers import GitHostingFixture | 27 | from lp.code.tests.helpers import GitHostingFixture |
159 | 26 | from lp.registry.interfaces.person import IPersonSet | 28 | from lp.registry.interfaces.person import IPersonSet |
160 | 27 | from lp.registry.interfaces.teammembership import ITeamMembershipSet | 29 | from lp.registry.interfaces.teammembership import ITeamMembershipSet |
161 | 30 | from lp.registry.model.productrelease import ProductRelease | ||
162 | 28 | from lp.registry.scripts.closeaccount import CloseAccountScript | 31 | from lp.registry.scripts.closeaccount import CloseAccountScript |
163 | 29 | from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache | 32 | from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache |
164 | 30 | from lp.services.database.interfaces import IStore | 33 | from lp.services.database.interfaces import IStore |
165 | @@ -1005,3 +1008,169 @@ class TestCloseAccount(TestCaseWithFactory): | |||
166 | 1005 | self.assertRemoved(account_id, person_id) | 1008 | self.assertRemoved(account_id, person_id) |
167 | 1006 | self.assertIsNone(store.get(Archive, ppa_id)) | 1009 | self.assertIsNone(store.get(Archive, ppa_id)) |
168 | 1007 | self.assertEqual(other_ppa, store.get(Archive, other_ppa_id)) | 1010 | self.assertEqual(other_ppa, store.get(Archive, other_ppa_id)) |
169 | 1011 | |||
170 | 1012 | def test_skips_announcements_in_deactivated_products(self): | ||
171 | 1013 | person = self.factory.makePerson() | ||
172 | 1014 | person_id = person.id | ||
173 | 1015 | account_id = person.account.id | ||
174 | 1016 | |||
175 | 1017 | product = self.factory.makeProduct() | ||
176 | 1018 | product.announce(person, "announcement") | ||
177 | 1019 | |||
178 | 1020 | script = self.makeScript([person.name]) | ||
179 | 1021 | with dbuser("launchpad"): | ||
180 | 1022 | self.assertRaisesWithContent( | ||
181 | 1023 | LaunchpadScriptFailure, | ||
182 | 1024 | "User %s is still referenced" % person.name, | ||
183 | 1025 | self.runScript, | ||
184 | 1026 | script, | ||
185 | 1027 | ) | ||
186 | 1028 | self.assertNotRemoved(account_id, person_id) | ||
187 | 1029 | |||
188 | 1030 | product.active = False | ||
189 | 1031 | with dbuser("launchpad"): | ||
190 | 1032 | self.runScript(script) | ||
191 | 1033 | |||
192 | 1034 | self.assertRemoved(account_id, person_id) | ||
193 | 1035 | |||
194 | 1036 | def test_non_product_announcements_are_not_skipped(self): | ||
195 | 1037 | person = self.factory.makePerson() | ||
196 | 1038 | person_id = person.id | ||
197 | 1039 | account_id = person.account.id | ||
198 | 1040 | |||
199 | 1041 | distro = self.factory.makeDistribution() | ||
200 | 1042 | distro.announce(person, "announcement") | ||
201 | 1043 | |||
202 | 1044 | script = self.makeScript([person.name]) | ||
203 | 1045 | with dbuser("launchpad"): | ||
204 | 1046 | self.assertRaisesWithContent( | ||
205 | 1047 | LaunchpadScriptFailure, | ||
206 | 1048 | "User %s is still referenced" % person.name, | ||
207 | 1049 | self.runScript, | ||
208 | 1050 | script, | ||
209 | 1051 | ) | ||
210 | 1052 | self.assertNotRemoved(account_id, person_id) | ||
211 | 1053 | |||
212 | 1054 | def test_skip_milestone_tags_from_inactive_products(self): | ||
213 | 1055 | |||
214 | 1056 | active_product = self.factory.makeProduct() | ||
215 | 1057 | inactive_product = self.factory.makeProduct() | ||
216 | 1058 | inactive_product.active = False | ||
217 | 1059 | |||
218 | 1060 | active_product_series = self.factory.makeProductSeries( | ||
219 | 1061 | product=active_product | ||
220 | 1062 | ) | ||
221 | 1063 | inactive_product_series = self.factory.makeProductSeries( | ||
222 | 1064 | product=inactive_product | ||
223 | 1065 | ) | ||
224 | 1066 | |||
225 | 1067 | for milestone_target, expected_to_be_removed in ( | ||
226 | 1068 | ({"product": active_product}, False), | ||
227 | 1069 | ({"product": inactive_product}, True), | ||
228 | 1070 | ({"productseries": active_product_series}, False), | ||
229 | 1071 | ({"productseries": inactive_product_series}, True), | ||
230 | 1072 | ({"distribution": self.factory.makeDistribution()}, False), | ||
231 | 1073 | ({"distroseries": self.factory.makeDistroSeries()}, False), | ||
232 | 1074 | ): | ||
233 | 1075 | person = self.factory.makePerson() | ||
234 | 1076 | person_id = person.id | ||
235 | 1077 | account_id = person.account.id | ||
236 | 1078 | |||
237 | 1079 | milestone = self.factory.makeMilestone(**milestone_target) | ||
238 | 1080 | milestone.setTags(["tag"], person) | ||
239 | 1081 | script = self.makeScript([person.name]) | ||
240 | 1082 | with dbuser("launchpad"): | ||
241 | 1083 | if not expected_to_be_removed: | ||
242 | 1084 | self.assertRaisesWithContent( | ||
243 | 1085 | LaunchpadScriptFailure, | ||
244 | 1086 | "User %s is still referenced" % person.name, | ||
245 | 1087 | self.runScript, | ||
246 | 1088 | script, | ||
247 | 1089 | ) | ||
248 | 1090 | self.assertNotRemoved(account_id, person_id) | ||
249 | 1091 | else: | ||
250 | 1092 | self.runScript(script) | ||
251 | 1093 | self.assertRemoved(account_id, person_id) | ||
252 | 1094 | |||
253 | 1095 | def test_skip_product_releases_from_inactive_products(self): | ||
254 | 1096 | |||
255 | 1097 | active_product = self.factory.makeProduct() | ||
256 | 1098 | inactive_product = self.factory.makeProduct() | ||
257 | 1099 | inactive_product.active = False | ||
258 | 1100 | |||
259 | 1101 | active_product_series = self.factory.makeProductSeries( | ||
260 | 1102 | product=active_product | ||
261 | 1103 | ) | ||
262 | 1104 | inactive_product_series = self.factory.makeProductSeries( | ||
263 | 1105 | product=inactive_product | ||
264 | 1106 | ) | ||
265 | 1107 | |||
266 | 1108 | for milestone_target, expected_to_be_removed in ( | ||
267 | 1109 | ({"product": active_product}, False), | ||
268 | 1110 | ({"product": inactive_product}, True), | ||
269 | 1111 | ({"productseries": active_product_series}, False), | ||
270 | 1112 | ({"productseries": inactive_product_series}, True), | ||
271 | 1113 | ): | ||
272 | 1114 | person = self.factory.makePerson() | ||
273 | 1115 | person_id = person.id | ||
274 | 1116 | account_id = person.account.id | ||
275 | 1117 | |||
276 | 1118 | milestone = self.factory.makeMilestone(**milestone_target) | ||
277 | 1119 | milestone.createProductRelease(person, datetime.now(pytz.UTC)) | ||
278 | 1120 | script = self.makeScript([person.name]) | ||
279 | 1121 | with dbuser("launchpad"): | ||
280 | 1122 | if not expected_to_be_removed: | ||
281 | 1123 | self.assertRaisesWithContent( | ||
282 | 1124 | LaunchpadScriptFailure, | ||
283 | 1125 | "User %s is still referenced" % person.name, | ||
284 | 1126 | self.runScript, | ||
285 | 1127 | script, | ||
286 | 1128 | ) | ||
287 | 1129 | self.assertNotRemoved(account_id, person_id) | ||
288 | 1130 | else: | ||
289 | 1131 | self.runScript(script) | ||
290 | 1132 | self.assertRemoved(account_id, person_id) | ||
291 | 1133 | |||
292 | 1134 | def test_skip_product_releases_files_from_inactive_products(self): | ||
293 | 1135 | |||
294 | 1136 | active_product = self.factory.makeProduct() | ||
295 | 1137 | inactive_product = self.factory.makeProduct() | ||
296 | 1138 | inactive_product.active = False | ||
297 | 1139 | |||
298 | 1140 | active_product_series = self.factory.makeProductSeries( | ||
299 | 1141 | product=active_product | ||
300 | 1142 | ) | ||
301 | 1143 | inactive_product_series = self.factory.makeProductSeries( | ||
302 | 1144 | product=inactive_product | ||
303 | 1145 | ) | ||
304 | 1146 | |||
305 | 1147 | for milestone_target, expected_to_be_removed in ( | ||
306 | 1148 | ({"product": active_product}, False), | ||
307 | 1149 | ({"product": inactive_product}, True), | ||
308 | 1150 | ({"productseries": active_product_series}, False), | ||
309 | 1151 | ({"productseries": inactive_product_series}, True), | ||
310 | 1152 | ): | ||
311 | 1153 | person = self.factory.makePerson() | ||
312 | 1154 | person_id = person.id | ||
313 | 1155 | account_id = person.account.id | ||
314 | 1156 | |||
315 | 1157 | milestone = self.factory.makeMilestone(**milestone_target) | ||
316 | 1158 | product_release = milestone.createProductRelease( | ||
317 | 1159 | milestone.product.owner, datetime.now(pytz.UTC) | ||
318 | 1160 | ) # type: ProductRelease | ||
319 | 1161 | product_release.addReleaseFile( | ||
320 | 1162 | "test.txt", b"test", "text/plain", person | ||
321 | 1163 | ) | ||
322 | 1164 | script = self.makeScript([person.name]) | ||
323 | 1165 | with dbuser("launchpad"): | ||
324 | 1166 | if not expected_to_be_removed: | ||
325 | 1167 | self.assertRaisesWithContent( | ||
326 | 1168 | LaunchpadScriptFailure, | ||
327 | 1169 | "User %s is still referenced" % person.name, | ||
328 | 1170 | self.runScript, | ||
329 | 1171 | script, | ||
330 | 1172 | ) | ||
331 | 1173 | self.assertNotRemoved(account_id, person_id) | ||
332 | 1174 | else: | ||
333 | 1175 | self.runScript(script) | ||
334 | 1176 | self.assertRemoved(account_id, person_id) |