Merge lp:~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358 into lp:launchpad
- unshare-team-members-subscriptions2-1009358
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Ian Booth |
Approved revision: | no longer in the source branch. |
Merged at revision: | 15423 |
Proposed branch: | lp:~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358 |
Merge into: | lp:launchpad |
Prerequisite: | lp:~wallyworld/launchpad/bug-privacy-filter-storm-expressions |
Diff against target: |
649 lines (+259/-84) 10 files modified
lib/lp/bugs/model/bug.py (+1/-1) lib/lp/bugs/model/bugtask.py (+1/-1) lib/lp/bugs/model/bugtasksearch.py (+25/-13) lib/lp/registry/interfaces/sharingjob.py (+1/-1) lib/lp/registry/model/sharingjob.py (+37/-42) lib/lp/registry/model/teammembership.py (+3/-4) lib/lp/registry/services/sharingservice.py (+10/-6) lib/lp/registry/services/tests/test_sharingservice.py (+108/-10) lib/lp/registry/tests/test_sharingjob.py (+72/-4) lib/lp/registry/tests/test_teammembership.py (+1/-2) |
To merge this branch: | bzr merge lp:~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358 |
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email:
|
Commit message
Ensure team members are unsubscribed from bugs they can no longer see when they are removed from a team.
Description of the change
== Implementation ==
This branch stops using RemoveGranteeSu
It was discovered doing this work that a bug existed in the sharingservice deletePillarSharee method. This was fixed and a test enhancement done to test for the issue.
A subsequent branch will delete RemoveGranteeSu
== Tests ==
Add new tests for RemoveBugSubscr
- test_unsubscrib
- test_admins_
Update sharing service deletePillarSharee tests to ensure the identified defect is fixed.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
Thanks for the review!
On Thu 14 Jun 2012 14:45:23 EST, William Grant wrote:
> Review: Approve code
>
> As discussed on IRC, this branch makes the jobs completely unsuitable for product. Most notably, the jobs in a lot of cases end up verifying the subscription consistency of hundreds of thousands or millions of unrelated bugs. However, it's a good intermediate step, since the jobs remain disabled on production.
>
I'll add it pillar filtering prior to landing.
> Since it doesn't make a huge amount of sense to review the functionality, there's just a couple of style comments. In addition to the below, I also propose http://
Looks good I think.
>
> 185 bug_ids = [
> 186 - bug.id for bug in bugs
> 187 + bug.id for bug in bugs or []
> 188 + ]
> 189 + information_types = [
> 190 + info_type.value for info_type in information_types or []
> 191 ]
>
> Can't those list comprehensions be on one line each?
>
The first one can. The second won't fit. To me it looked better if they
were both done the same way. But I've changed it.
> 202 + @property
> 203 + def information_
> 204 + return [
> 205 + enumerated_
> 206 + for value in self.metadata[
>
> Why does this use the registry? You are looking the enum up by the name you got from the enum :)
>
I think you misread it :-) It's pulling serialised values (the names)
from the stored json and looking them up in an items list associated
with the InformationType
> 495 +# def test_revokeAcce
>
> I'm not sure a commented test is useful, particularly when we have the work planned and the test won't ever work as written.
The test worked fine in the previous branch. I had left it in as a
reminder so it could be adapted when the next job was added but will
remove it.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
>
>> 202 + @property
>> 203 + def information_
>> 204 + return [
>> 205 + enumerated_
>> 206 + for value in self.metadata[
>>
>> Why does this use the registry? You are looking the enum up by the name you got from the enum :)
>>
>
> I think you misread it :-) It's pulling serialised values (the names)
> from the stored json and looking them up in an items list associated
> with the InformationType
>
My mistake, I had grabbed some de-serialisation code and the registry
lookuo was indeed surperfulous here.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
I've now added pillar filtering to the search. Another branch will add filtering on grantee for team membership removal.
Preview Diff
1 | === modified file 'lib/lp/bugs/model/bug.py' |
2 | --- lib/lp/bugs/model/bug.py 2012-06-14 07:55:25 +0000 |
3 | +++ lib/lp/bugs/model/bug.py 2012-06-14 07:55:25 +0000 |
4 | @@ -1811,7 +1811,7 @@ |
5 | # As a result of the transition, some subscribers may no longer |
6 | # have access to the bug. We need to run a job to remove any such |
7 | # subscriptions. |
8 | - getUtility(IRemoveBugSubscriptionsJobSource).create([self], who) |
9 | + getUtility(IRemoveBugSubscriptionsJobSource).create(who, [self]) |
10 | |
11 | return True |
12 | |
13 | |
14 | === modified file 'lib/lp/bugs/model/bugtask.py' |
15 | --- lib/lp/bugs/model/bugtask.py 2012-06-14 07:55:25 +0000 |
16 | +++ lib/lp/bugs/model/bugtask.py 2012-06-14 07:55:25 +0000 |
17 | @@ -1193,7 +1193,7 @@ |
18 | # have access to the parent bug. We need to run a job to remove any |
19 | # such subscriptions. |
20 | getUtility(IRemoveBugSubscriptionsJobSource).create( |
21 | - [self.bug], user) |
22 | + user, [self.bug], pillar=target_before_change) |
23 | |
24 | def updateTargetNameCache(self, newtarget=None): |
25 | """See `IBugTask`.""" |
26 | |
27 | === modified file 'lib/lp/bugs/model/bugtasksearch.py' |
28 | --- lib/lp/bugs/model/bugtasksearch.py 2012-06-14 07:55:25 +0000 |
29 | +++ lib/lp/bugs/model/bugtasksearch.py 2012-06-14 07:55:25 +0000 |
30 | @@ -5,6 +5,7 @@ |
31 | |
32 | __all__ = [ |
33 | 'get_bug_privacy_filter', |
34 | + 'get_bug_privacy_filter_terms', |
35 | 'orderby_expression', |
36 | 'search_bugs', |
37 | ] |
38 | @@ -68,6 +69,7 @@ |
39 | from lp.registry.interfaces.milestone import IProjectGroupMilestone |
40 | from lp.registry.interfaces.product import IProduct |
41 | from lp.registry.interfaces.productseries import IProductSeries |
42 | +from lp.registry.interfaces.role import IPersonRoles |
43 | from lp.registry.model.accesspolicy import AccessPolicyGrant |
44 | from lp.registry.model.distribution import Distribution |
45 | from lp.registry.model.milestone import Milestone |
46 | @@ -1378,9 +1380,15 @@ |
47 | :return: A SQL filter, a decorator to cache visibility in a resultset that |
48 | returns BugTask objects. |
49 | """ |
50 | - bug_filter_terms = _get_bug_privacy_filter_terms(user) |
51 | - if not bug_filter_terms: |
52 | + # Admins can see all bugs, so we can short-circuit the filter. |
53 | + if user is not None and IPersonRoles(user).in_admin: |
54 | return True, _nocache_bug_decorator |
55 | + |
56 | + # We want an actual Storm Person. |
57 | + if IPersonRoles.providedBy(user): |
58 | + user = user.person |
59 | + |
60 | + bug_filter_terms = get_bug_privacy_filter_terms(user, check_admin=False) |
61 | if len(bug_filter_terms) == 1: |
62 | return bug_filter_terms[0], _nocache_bug_decorator |
63 | |
64 | @@ -1388,24 +1396,19 @@ |
65 | return expr, _make_cache_user_can_view_bug(user) |
66 | |
67 | |
68 | -def _get_bug_privacy_filter_terms(user): |
69 | +def get_bug_privacy_filter_terms(user, check_admin=True): |
70 | public_bug_filter = ( |
71 | BugTaskFlat.information_type.is_in(PUBLIC_INFORMATION_TYPES)) |
72 | |
73 | if user is None: |
74 | return [public_bug_filter] |
75 | |
76 | - admin_team = getUtility(ILaunchpadCelebrities).admin |
77 | - if removeSecurityProxy(user).inTeam(admin_team): |
78 | - return [] |
79 | - |
80 | artifact_grant_query = Coalesce( |
81 | ArrayIntersects(SQL('BugTaskFlat.access_grants'), |
82 | Select( |
83 | ArrayAgg(TeamParticipation.teamID), |
84 | tables=TeamParticipation, |
85 | - where=(TeamParticipation.personID == |
86 | - user.id) |
87 | + where=(TeamParticipation.person == user) |
88 | )), False) |
89 | |
90 | policy_grant_query = Coalesce( |
91 | @@ -1416,9 +1419,18 @@ |
92 | Join(TeamParticipation, |
93 | TeamParticipation.teamID == |
94 | AccessPolicyGrant.grantee_id)), |
95 | - where=( |
96 | - TeamParticipation.personID == |
97 | - user.id) |
98 | + where=(TeamParticipation.person == user) |
99 | )), False) |
100 | |
101 | - return [public_bug_filter, artifact_grant_query, policy_grant_query] |
102 | + filters = [public_bug_filter, artifact_grant_query, policy_grant_query] |
103 | + |
104 | + if check_admin: |
105 | + filters.append( |
106 | + Exists(Select( |
107 | + 1, tables=[TeamParticipation], |
108 | + where=And( |
109 | + TeamParticipation.person == user, |
110 | + TeamParticipation.team == |
111 | + getUtility(ILaunchpadCelebrities).admin)))) |
112 | + |
113 | + return filters |
114 | |
115 | === modified file 'lib/lp/registry/interfaces/sharingjob.py' |
116 | --- lib/lp/registry/interfaces/sharingjob.py 2012-05-22 12:05:51 +0000 |
117 | +++ lib/lp/registry/interfaces/sharingjob.py 2012-06-14 07:55:25 +0000 |
118 | @@ -92,7 +92,7 @@ |
119 | class IRemoveBugSubscriptionsJobSource(ISharingJobSource): |
120 | """An interface for acquiring IRemoveBugSubscriptionsJobs.""" |
121 | |
122 | - def create(pillar, bugs, requestor): |
123 | + def create(pillar, requestor, bugs=None, information_types=None): |
124 | """Create a new job to remove subscriptions for the specified bugs. |
125 | |
126 | Subscriptions for users who no longer have access to the bugs are |
127 | |
128 | === modified file 'lib/lp/registry/model/sharingjob.py' |
129 | --- lib/lp/registry/model/sharingjob.py 2012-06-14 07:55:25 +0000 |
130 | +++ lib/lp/registry/model/sharingjob.py 2012-06-14 07:55:25 +0000 |
131 | @@ -19,18 +19,15 @@ |
132 | from lazr.enum import ( |
133 | DBEnumeratedType, |
134 | DBItem, |
135 | - enumerated_type_registry, |
136 | ) |
137 | import simplejson |
138 | from sqlobject import SQLObjectNotFound |
139 | from storm.expr import ( |
140 | And, |
141 | - Coalesce, |
142 | In, |
143 | - Join, |
144 | Not, |
145 | + Or, |
146 | Select, |
147 | - SQL, |
148 | ) |
149 | from storm.locals import ( |
150 | Int, |
151 | @@ -48,7 +45,10 @@ |
152 | from lp.bugs.model.bug import Bug |
153 | from lp.bugs.model.bugsubscription import BugSubscription |
154 | from lp.bugs.model.bugtaskflat import BugTaskFlat |
155 | -from lp.bugs.model.bugtasksearch import get_bug_privacy_filter |
156 | +from lp.bugs.model.bugtasksearch import ( |
157 | + get_bug_privacy_filter, |
158 | + get_bug_privacy_filter_terms, |
159 | + ) |
160 | from lp.code.interfaces.branchlookup import IBranchLookup |
161 | from lp.registry.enums import InformationType |
162 | from lp.registry.interfaces.person import IPersonSet |
163 | @@ -61,19 +61,13 @@ |
164 | ISharingJob, |
165 | ISharingJobSource, |
166 | ) |
167 | -from lp.registry.model.accesspolicy import AccessPolicyGrant |
168 | from lp.registry.model.distribution import Distribution |
169 | from lp.registry.model.person import Person |
170 | from lp.registry.model.product import Product |
171 | -from lp.registry.model.teammembership import TeamParticipation |
172 | from lp.services.config import config |
173 | from lp.services.database.enumcol import EnumCol |
174 | from lp.services.database.lpstorm import IStore |
175 | from lp.services.database.stormbase import StormBase |
176 | -from lp.services.database.stormexpr import ( |
177 | - ArrayAgg, |
178 | - ArrayIntersects, |
179 | - ) |
180 | from lp.services.job.model.job import ( |
181 | EnumeratedSubclass, |
182 | Job, |
183 | @@ -307,7 +301,7 @@ |
184 | @property |
185 | def information_types(self): |
186 | return [ |
187 | - enumerated_type_registry[InformationType.name].items[value] |
188 | + InformationType.items[value] |
189 | for value in self.metadata['information_types']] |
190 | |
191 | def getErrorRecipients(self): |
192 | @@ -402,18 +396,21 @@ |
193 | config = config.IRemoveBugSubscriptionsJobSource |
194 | |
195 | @classmethod |
196 | - def create(cls, bugs, requestor): |
197 | + def create(cls, requestor, bugs=None, grantee=None, pillar=None, |
198 | + information_types=None): |
199 | """See `IRemoveBugSubscriptionsJob`.""" |
200 | |
201 | - bug_ids = [ |
202 | - bug.id for bug in bugs |
203 | + bug_ids = [bug.id for bug in bugs or []] |
204 | + information_types = [ |
205 | + info_type.value for info_type in information_types or [] |
206 | ] |
207 | metadata = { |
208 | 'bug_ids': bug_ids, |
209 | + 'information_types': information_types, |
210 | 'requestor.id': requestor.id |
211 | } |
212 | return super(RemoveBugSubscriptionsJob, cls).create( |
213 | - None, None, metadata) |
214 | + pillar, grantee, metadata) |
215 | |
216 | @property |
217 | def requestor_id(self): |
218 | @@ -431,6 +428,12 @@ |
219 | def bugs(self): |
220 | return getUtility(IBugSet).getByNumbers(self.bug_ids) |
221 | |
222 | + @property |
223 | + def information_types(self): |
224 | + return [ |
225 | + InformationType.items[value] |
226 | + for value in self.metadata['information_types']] |
227 | + |
228 | def getErrorRecipients(self): |
229 | # If something goes wrong we want to let the requestor know as well |
230 | # as the pillar maintainer (if there is a pillar). |
231 | @@ -453,35 +456,27 @@ |
232 | |
233 | # Find all bug subscriptions for which the subscriber cannot see the |
234 | # bug. |
235 | - constraints = [ |
236 | - BugTaskFlat.bug_id.is_in(self.bug_ids), |
237 | - Not(Coalesce( |
238 | - ArrayIntersects(SQL('BugTaskFlat.access_grants'), |
239 | - Select( |
240 | - ArrayAgg(TeamParticipation.teamID), |
241 | - tables=TeamParticipation, |
242 | - where=(TeamParticipation.personID == |
243 | - BugSubscription.person_id) |
244 | - )), False)), |
245 | - Not(Coalesce( |
246 | - ArrayIntersects(SQL('BugTaskFlat.access_policies'), |
247 | - Select( |
248 | - ArrayAgg(AccessPolicyGrant.policy_id), |
249 | - tables=(AccessPolicyGrant, |
250 | - Join(TeamParticipation, |
251 | - TeamParticipation.teamID == |
252 | - AccessPolicyGrant.grantee_id)), |
253 | - where=( |
254 | - TeamParticipation.personID == |
255 | - BugSubscription.person_id) |
256 | - )), False)) |
257 | - ] |
258 | + invisible_filter = ( |
259 | + Not(Or(*get_bug_privacy_filter_terms(BugSubscription.person_id)))) |
260 | + filters = [invisible_filter] |
261 | + |
262 | + if self.bug_ids: |
263 | + filters.append(BugTaskFlat.bug_id.is_in(self.bug_ids)) |
264 | + else: |
265 | + if self.information_types: |
266 | + filters.append( |
267 | + BugTaskFlat.information_type.is_in(self.information_types)) |
268 | + if self.product: |
269 | + filters.append( |
270 | + BugTaskFlat.product == self.product) |
271 | + if self.distro: |
272 | + filters.append( |
273 | + BugTaskFlat.distribution == self.distro) |
274 | + |
275 | subscriptions = IStore(BugSubscription).find( |
276 | BugSubscription, |
277 | In(BugSubscription.bug_id, |
278 | - Select( |
279 | - BugTaskFlat.bug_id, |
280 | - where=And(*constraints))) |
281 | + Select(BugTaskFlat.bug_id, where=And(*filters))) |
282 | ) |
283 | for sub in subscriptions: |
284 | sub.bug.unsubscribe( |
285 | |
286 | === modified file 'lib/lp/registry/model/teammembership.py' |
287 | --- lib/lp/registry/model/teammembership.py 2012-06-14 07:55:25 +0000 |
288 | +++ lib/lp/registry/model/teammembership.py 2012-06-14 07:55:25 +0000 |
289 | @@ -43,7 +43,7 @@ |
290 | ) |
291 | from lp.registry.interfaces.role import IPersonRoles |
292 | from lp.registry.interfaces.sharingjob import ( |
293 | - IRemoveGranteeSubscriptionsJobSource, |
294 | + IRemoveBugSubscriptionsJobSource, |
295 | ) |
296 | from lp.registry.interfaces.teammembership import ( |
297 | ACTIVE_STATES, |
298 | @@ -393,9 +393,8 @@ |
299 | # A person has left the team so they may no longer have access |
300 | # to some artifacts shared with the team. We need to run a job |
301 | # to remove any subscriptions to such artifacts. |
302 | - getUtility(IRemoveGranteeSubscriptionsJobSource).create( |
303 | - None, self.person, user) |
304 | - |
305 | + getUtility(IRemoveBugSubscriptionsJobSource).create( |
306 | + user, grantee=self.team) |
307 | else: |
308 | # Changed from an inactive state to another inactive one, so no |
309 | # need to fill/clean the TeamParticipation table. |
310 | |
311 | === modified file 'lib/lp/registry/services/sharingservice.py' |
312 | --- lib/lp/registry/services/sharingservice.py 2012-06-14 07:55:25 +0000 |
313 | +++ lib/lp/registry/services/sharingservice.py 2012-06-14 07:55:25 +0000 |
314 | @@ -40,7 +40,7 @@ |
315 | from lp.registry.interfaces.product import IProduct |
316 | from lp.registry.interfaces.projectgroup import IProjectGroup |
317 | from lp.registry.interfaces.sharingjob import ( |
318 | - IRemoveGranteeSubscriptionsJobSource, |
319 | + IRemoveBugSubscriptionsJobSource, |
320 | ) |
321 | from lp.registry.interfaces.sharingservice import ISharingService |
322 | from lp.registry.model.person import Person |
323 | @@ -328,12 +328,13 @@ |
324 | if len(to_delete) > 0: |
325 | accessartifact_grant_source = getUtility( |
326 | IAccessArtifactGrantSource) |
327 | - accessartifact_grant_source.revokeByArtifact(to_delete) |
328 | + accessartifact_grant_source.revokeByArtifact(to_delete, [sharee]) |
329 | |
330 | # Create a job to remove subscriptions for artifacts the sharee can no |
331 | # longer see. |
332 | - getUtility(IRemoveGranteeSubscriptionsJobSource).create( |
333 | - pillar, sharee, user, information_types=information_types) |
334 | + getUtility(IRemoveBugSubscriptionsJobSource).create( |
335 | + user, bugs=None, pillar=pillar, |
336 | + information_types=information_types) |
337 | |
338 | @available_with_permission('launchpad.Edit', 'pillar') |
339 | def revokeAccessGrants(self, pillar, sharee, user, branches=None, |
340 | @@ -358,8 +359,11 @@ |
341 | |
342 | # Create a job to remove subscriptions for artifacts the sharee can no |
343 | # longer see. |
344 | - getUtility(IRemoveGranteeSubscriptionsJobSource).create( |
345 | - pillar, sharee, user, bugs=bugs, branches=branches) |
346 | + if bugs: |
347 | + getUtility(IRemoveBugSubscriptionsJobSource).create( |
348 | + user, bugs, pillar=pillar) |
349 | + # XXX 2012-06-13 wallyworld bug=1012448 |
350 | + # Remove branch subscriptions when information type fully implemented. |
351 | |
352 | def ensureAccessGrants(self, sharee, user, branches=None, bugs=None, |
353 | ignore_permissions=False): |
354 | |
355 | === modified file 'lib/lp/registry/services/tests/test_sharingservice.py' |
356 | --- lib/lp/registry/services/tests/test_sharingservice.py 2012-06-14 07:55:25 +0000 |
357 | +++ lib/lp/registry/services/tests/test_sharingservice.py 2012-06-14 07:55:25 +0000 |
358 | @@ -26,10 +26,12 @@ |
359 | ) |
360 | from lp.registry.interfaces.accesspolicy import ( |
361 | IAccessArtifactGrantSource, |
362 | + IAccessArtifactSource, |
363 | IAccessPolicyGrantFlatSource, |
364 | IAccessPolicyGrantSource, |
365 | IAccessPolicySource, |
366 | ) |
367 | +from lp.registry.interfaces.person import TeamSubscriptionPolicy |
368 | from lp.registry.services.sharingservice import SharingService |
369 | from lp.services.features.testing import FeatureFixture |
370 | from lp.services.job.tests import block_on_job |
371 | @@ -55,7 +57,7 @@ |
372 | WRITE_FLAG = { |
373 | 'disclosure.enhanced_sharing.writable': 'true', |
374 | 'disclosure.enhanced_sharing_details.enabled': 'true', |
375 | - 'jobs.celery.enabled_classes': 'RemoveGranteeSubscriptionsJob'} |
376 | + 'jobs.celery.enabled_classes': 'RemoveBugSubscriptionsJob'} |
377 | DETAILS_FLAG = {'disclosure.enhanced_sharing_details.enabled': 'true'} |
378 | |
379 | |
380 | @@ -541,12 +543,15 @@ |
381 | # Make some artifact grants for our sharee. |
382 | artifact = self.factory.makeAccessArtifact() |
383 | self.factory.makeAccessArtifactGrant(artifact, grantee) |
384 | + # Make some access policy grants for another sharee. |
385 | + another = self.factory.makePerson() |
386 | + self.factory.makeAccessPolicyGrant(access_policies[0], another) |
387 | + # Make some artifact grants for our yet another sharee. |
388 | + yet_another = self.factory.makePerson() |
389 | + self.factory.makeAccessArtifactGrant(artifact, yet_another) |
390 | for access_policy in access_policies: |
391 | self.factory.makeAccessPolicyArtifact( |
392 | artifact=artifact, policy=access_policy) |
393 | - # Make some access policy grants for another sharee. |
394 | - another = self.factory.makePerson() |
395 | - self.factory.makeAccessPolicyGrant(access_policies[0], another) |
396 | # Delete data for a specific information type. |
397 | with FeatureFixture(WRITE_FLAG): |
398 | self.service.deletePillarSharee( |
399 | @@ -563,10 +568,16 @@ |
400 | expected_data = [ |
401 | (grantee, {policy: SharingPermission.ALL}, []) |
402 | for policy in expected_policies] |
403 | - # Add the expected data for the other sharee. |
404 | + # Add the expected data for the other sharees. |
405 | another_person_data = ( |
406 | another, {access_policies[0]: SharingPermission.ALL}, []) |
407 | expected_data.append(another_person_data) |
408 | + policy_permissions = dict([( |
409 | + policy, SharingPermission.SOME) for policy in access_policies]) |
410 | + yet_another_person_data = ( |
411 | + yet_another, policy_permissions, |
412 | + [InformationType.USERDATA, InformationType.EMBARGOEDSECURITY]) |
413 | + expected_data.append(yet_another_person_data) |
414 | self.assertContentEqual( |
415 | expected_data, self.service.getPillarSharees(pillar)) |
416 | |
417 | @@ -768,15 +779,102 @@ |
418 | information_type=InformationType.USERDATA) |
419 | self._assert_revokeAccessGrants(distro, [bug], None) |
420 | |
421 | - def test_revokeAccessGrantsBranches(self): |
422 | + # XXX 2012-06-13 wallyworld bug=1012448 |
423 | + # Remove branch subscriptions when information type fully implemented. |
424 | +# def test_revokeAccessGrantsBranches(self): |
425 | +# # Users with launchpad.Edit can delete all access for a sharee. |
426 | +# owner = self.factory.makePerson() |
427 | +# product = self.factory.makeProduct(owner=owner) |
428 | +# login_person(owner) |
429 | +# branch = self.factory.makeBranch( |
430 | +# product=product, owner=owner, |
431 | +# information_type=InformationType.USERDATA) |
432 | +# self._assert_revokeAccessGrants(product, None, [branch]) |
433 | + |
434 | + def _assert_revokeTeamAccessGrants(self, pillar, bugs, branches): |
435 | + artifacts = [] |
436 | + if bugs: |
437 | + artifacts.extend(bugs) |
438 | + if branches: |
439 | + artifacts.extend(branches) |
440 | + policy = self.factory.makeAccessPolicy(pillar=pillar) |
441 | + |
442 | + person_grantee = self.factory.makePerson() |
443 | + team_owner = self.factory.makePerson() |
444 | + team_grantee = self.factory.makeTeam( |
445 | + owner=team_owner, |
446 | + subscription_policy=TeamSubscriptionPolicy.RESTRICTED, |
447 | + members=[person_grantee]) |
448 | + |
449 | + # Subscribe the team and person grantees to the artifacts. |
450 | + for person in [team_grantee, person_grantee]: |
451 | + for bug in bugs or []: |
452 | + bug.subscribe(person, pillar.owner) |
453 | + # XXX 2012-06-12 wallyworld bug=1002596 |
454 | + # No need to revoke AAG with triggers removed. |
455 | + if person == person_grantee: |
456 | + accessartifact_source = getUtility(IAccessArtifactSource) |
457 | + getUtility(IAccessArtifactGrantSource).revokeByArtifact( |
458 | + accessartifact_source.find([bug]), [person_grantee]) |
459 | + for branch in branches or []: |
460 | + branch.subscribe(person, |
461 | + BranchSubscriptionNotificationLevel.NOEMAIL, None, |
462 | + CodeReviewNotificationLevel.NOEMAIL, pillar.owner) |
463 | + |
464 | + # Check that grantees have expected access grants and subscriptions. |
465 | + for person in [team_grantee, person_grantee]: |
466 | + visible_bugs, visible_branches = self.service.getVisibleArtifacts( |
467 | + person, branches, bugs) |
468 | + self.assertContentEqual(bugs or [], visible_bugs) |
469 | + self.assertContentEqual(branches or [], visible_branches) |
470 | + for person in [team_grantee, person_grantee]: |
471 | + for bug in bugs or []: |
472 | + self.assertIn(person, bug.getDirectSubscribers()) |
473 | + |
474 | + with FeatureFixture(WRITE_FLAG): |
475 | + self.service.revokeAccessGrants( |
476 | + pillar, team_grantee, pillar.owner, |
477 | + bugs=bugs, branches=branches) |
478 | + with block_on_job(self): |
479 | + transaction.commit() |
480 | + |
481 | + # The grantees now have no access to anything. |
482 | + apgfs = getUtility(IAccessPolicyGrantFlatSource) |
483 | + permission_info = apgfs.findGranteePermissionsByPolicy( |
484 | + [policy], [team_grantee, person_grantee]) |
485 | + self.assertEqual(0, permission_info.count()) |
486 | + |
487 | + # Check that the grantee's subscriptions have been removed. |
488 | + # Branches will be done once they have the information_type attribute. |
489 | + for person in [team_grantee, person_grantee]: |
490 | + for bug in bugs or []: |
491 | + self.assertNotIn(person, bug.getDirectSubscribers()) |
492 | + visible_bugs, visible_branches = self.service.getVisibleArtifacts( |
493 | + person, branches, bugs) |
494 | + self.assertContentEqual([], visible_bugs) |
495 | + self.assertContentEqual([], visible_branches) |
496 | + |
497 | + def test_revokeTeamAccessGrantsBugs(self): |
498 | # Users with launchpad.Edit can delete all access for a sharee. |
499 | owner = self.factory.makePerson() |
500 | - product = self.factory.makeProduct(owner=owner) |
501 | + distro = self.factory.makeDistribution(owner=owner) |
502 | login_person(owner) |
503 | - branch = self.factory.makeBranch( |
504 | - product=product, owner=owner, |
505 | + bug = self.factory.makeBug( |
506 | + distribution=distro, owner=owner, |
507 | information_type=InformationType.USERDATA) |
508 | - self._assert_revokeAccessGrants(product, None, [branch]) |
509 | + self._assert_revokeTeamAccessGrants(distro, [bug], None) |
510 | + |
511 | + # XXX 2012-06-13 wallyworld bug=1012448 |
512 | + # Remove branch subscriptions when information type fully implemented. |
513 | +# def test_revokeAccessGrantsBranches(self): |
514 | +# # Users with launchpad.Edit can delete all access for a sharee. |
515 | +# owner = self.factory.makePerson() |
516 | +# product = self.factory.makeProduct(owner=owner) |
517 | +# login_person(owner) |
518 | +# branch = self.factory.makeBranch( |
519 | +# product=product, owner=owner, |
520 | +# information_type=InformationType.USERDATA) |
521 | +# self._assert_revokeTeamAccessGrants(distro, [bug], None) |
522 | |
523 | def _assert_revokeAccessGrantsUnauthorized(self): |
524 | # revokeAccessGrants raises an Unauthorized exception if the user |
525 | |
526 | === modified file 'lib/lp/registry/tests/test_sharingjob.py' |
527 | --- lib/lp/registry/tests/test_sharingjob.py 2012-06-14 07:55:25 +0000 |
528 | +++ lib/lp/registry/tests/test_sharingjob.py 2012-06-14 07:55:25 +0000 |
529 | @@ -481,7 +481,7 @@ |
530 | |
531 | def create_job(distro, bug, grantee, owner): |
532 | job = getUtility(IRemoveBugSubscriptionsJobSource).create( |
533 | - [bug], owner) |
534 | + owner, [bug]) |
535 | with person_logged_in(owner): |
536 | bug.transitionToInformationType( |
537 | InformationType.EMBARGOEDSECURITY, owner) |
538 | @@ -515,7 +515,7 @@ |
539 | requestor = self.factory.makePerson() |
540 | bug = self.factory.makeBug() |
541 | job = getUtility(IRemoveBugSubscriptionsJobSource).create( |
542 | - [bug], requestor) |
543 | + requestor, [bug]) |
544 | naked_job = removeSecurityProxy(job) |
545 | self.assertIsInstance(job, RemoveBugSubscriptionsJob) |
546 | self.assertEqual(requestor.id, naked_job.requestor_id) |
547 | @@ -527,7 +527,7 @@ |
548 | product = self.factory.makeProduct() |
549 | bug = self.factory.makeBug(product=product) |
550 | job = getUtility(IRemoveBugSubscriptionsJobSource).create( |
551 | - [bug], requestor) |
552 | + requestor, [bug], pillar=product) |
553 | expected_emails = [ |
554 | format_address_for_person(person) |
555 | for person in (product.owner, requestor)] |
556 | @@ -580,7 +580,7 @@ |
557 | reconcile_access_for_artifact( |
558 | bug, bug.information_type, bug.affected_pillars) |
559 | |
560 | - getUtility(IRemoveBugSubscriptionsJobSource).create([bug], owner) |
561 | + getUtility(IRemoveBugSubscriptionsJobSource).create(owner, [bug]) |
562 | with block_on_job(self): |
563 | transaction.commit() |
564 | |
565 | @@ -611,3 +611,71 @@ |
566 | removeSecurityProxy(bug).default_bugtask.product = another_product |
567 | |
568 | self._assert_bug_change_unsubscribes(change_target) |
569 | + |
570 | + def _make_subscribed_bug(self, grantee, product=None, distribution=None, |
571 | + information_type=InformationType.USERDATA): |
572 | + owner = self.factory.makePerson() |
573 | + bug = self.factory.makeBug( |
574 | + owner=owner, product=product, distribution=distribution, |
575 | + information_type=information_type) |
576 | + with person_logged_in(owner): |
577 | + bug.subscribe(grantee, owner) |
578 | + # Subscribing grantee to bug creates an access grant so we need to |
579 | + # revoke that for our test. |
580 | + accessartifact_source = getUtility(IAccessArtifactSource) |
581 | + accessartifact_grant_source = getUtility(IAccessArtifactGrantSource) |
582 | + accessartifact_grant_source.revokeByArtifact( |
583 | + accessartifact_source.find([bug]), [grantee]) |
584 | + |
585 | + return bug, owner |
586 | + |
587 | + def test_unsubscribe_pillar_artifacts_specific_info_types(self): |
588 | + # Only remove subscriptions for bugs of the specified info type. |
589 | + |
590 | + person_grantee = self.factory.makePerson(name='grantee') |
591 | + |
592 | + owner = self.factory.makePerson(name='pillarowner') |
593 | + pillar = self.factory.makeProduct(owner=owner) |
594 | + |
595 | + # Make bugs the person_grantee is subscribed to. |
596 | + bug1, ignored = self._make_subscribed_bug( |
597 | + person_grantee, product=pillar, |
598 | + information_type=InformationType.USERDATA) |
599 | + |
600 | + bug2, ignored = self._make_subscribed_bug( |
601 | + person_grantee, product=pillar, |
602 | + information_type=InformationType.EMBARGOEDSECURITY) |
603 | + |
604 | + # Now run the job, removing access to userdata artifacts. |
605 | + getUtility(IRemoveBugSubscriptionsJobSource).create( |
606 | + pillar.owner, pillar=pillar, |
607 | + information_types=[InformationType.USERDATA]) |
608 | + with block_on_job(self): |
609 | + transaction.commit() |
610 | + |
611 | + self.assertNotIn( |
612 | + person_grantee, removeSecurityProxy(bug1).getDirectSubscribers()) |
613 | + self.assertIn( |
614 | + person_grantee, removeSecurityProxy(bug2).getDirectSubscribers()) |
615 | + |
616 | + def test_admins_retain_subscriptions(self): |
617 | + # Admins subscriptions are retained even if they don't have explicit |
618 | + # access. |
619 | + product = self.factory.makeProduct() |
620 | + owner = self.factory.makePerson() |
621 | + admin = getUtility(ILaunchpadCelebrities).admin.teamowner |
622 | + |
623 | + login_person(owner) |
624 | + bug = self.factory.makeBug( |
625 | + owner=owner, product=product, |
626 | + information_type=InformationType.USERDATA) |
627 | + |
628 | + bug.subscribe(admin, owner) |
629 | + getUtility(IRemoveBugSubscriptionsJobSource).create( |
630 | + owner, [bug], pillar=product) |
631 | + with block_on_job(self): |
632 | + transaction.commit() |
633 | + |
634 | + # Check the result. admin should still be subscribed. |
635 | + subscribers = removeSecurityProxy(bug).getDirectSubscribers() |
636 | + self.assertIn(admin, subscribers) |
637 | |
638 | === modified file 'lib/lp/registry/tests/test_teammembership.py' |
639 | --- lib/lp/registry/tests/test_teammembership.py 2012-06-14 07:55:25 +0000 |
640 | +++ lib/lp/registry/tests/test_teammembership.py 2012-06-14 07:55:25 +0000 |
641 | @@ -999,8 +999,7 @@ |
642 | def setUp(self): |
643 | self.useFixture(FeatureFixture({ |
644 | 'disclosure.unsubscribe_jobs.enabled': 'true', |
645 | - 'jobs.celery.enabled_classes': |
646 | - 'RemoveGranteeSubscriptionsJob', |
647 | + 'jobs.celery.enabled_classes': 'RemoveBugSubscriptionsJob', |
648 | })) |
649 | super(TestTeamMembershipJobs, self).setUp() |
650 |
As discussed on IRC, this branch makes the jobs completely unsuitable for product. Most notably, the jobs in a lot of cases end up verifying the subscription consistency of hundreds of thousands or millions of unrelated bugs. However, it's a good intermediate step, since the jobs remain disabled on production.
Since it doesn't make a huge amount of sense to review the functionality, there's just a couple of style comments. In addition to the below, I also propose http:// pastebin. ubuntu. com/1040252/ as a bit of a cleanup around the admin special case.
185 bug_ids = [
186 - bug.id for bug in bugs
187 + bug.id for bug in bugs or []
188 + ]
189 + information_types = [
190 + info_type.value for info_type in information_types or []
191 ]
Can't those list comprehensions be on one line each?
202 + @property types(self) : type_registry[ InformationType .name]. items[value] 'information_ types'] ]
203 + def information_
204 + return [
205 + enumerated_
206 + for value in self.metadata[
Why does this use the registry? You are looking the enum up by the name you got from the enum :)
495 +# def test_revokeAcce ssGrantsBranche s(self) : makePerson( ) makeProduct( owner=owner) makeBranch( type=Informatio nType.USERDATA) revokeTeamAcces sGrants( distro, [bug], None)
496 +# # Users with launchpad.Edit can delete all access for a sharee.
497 +# owner = self.factory.
498 +# product = self.factory.
499 +# login_person(owner)
500 +# branch = self.factory.
501 +# product=product, owner=owner,
502 +# information_
503 +# self._assert_
I'm not sure a commented test is useful, particularly when we have the work planned and the test won't ever work as written.