Merge lp:~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358 into lp:launchpad

Proposed by Ian Booth
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
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+110215@code.launchpad.net

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 RemoveGranteeSubscriptionsJob and replaces it with an enhanced version of RemoveBugSubscriptionsJob. This ensures the correct functionality for all cases where subscriptions need to be removed for invisible bugs. A side effect is that for now, branch subscriptions aren't removed in some cases, but this functionality was only partially implemented anyway. The next step is to introduce a RemoveBranchSubscriptionsJob to handle branches.

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 RemoveGranteeSubscriptionsJob.

== Tests ==

Add new tests for RemoveBugSubscriptionsJob:
- test_unsubscribe_pillar_artifacts_specific_info_types
- test_admins_retain_subscriptions

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/bugs/model/bug.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/bugs/model/bugtasksearch.py
  lib/lp/registry/interfaces/sharingjob.py
  lib/lp/registry/model/sharingjob.py
  lib/lp/registry/model/teammembership.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/tests/test_sharingjob.py
  lib/lp/registry/tests/test_teammembership.py

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

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
203 + def information_types(self):
204 + return [
205 + enumerated_type_registry[InformationType.name].items[value]
206 + for value in self.metadata['information_types']]

Why does this use the registry? You are looking the enum up by the name you got from the enum :)

495 +# def test_revokeAccessGrantsBranches(self):
496 +# # Users with launchpad.Edit can delete all access for a sharee.
497 +# owner = self.factory.makePerson()
498 +# product = self.factory.makeProduct(owner=owner)
499 +# login_person(owner)
500 +# branch = self.factory.makeBranch(
501 +# product=product, owner=owner,
502 +# information_type=InformationType.USERDATA)
503 +# self._assert_revokeTeamAccessGrants(distro, [bug], None)

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.

review: Approve (code)
Revision history for this message
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://pastebin.ubuntu.com/1040252/ as a bit of a cleanup around the admin special case.

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_types(self):
> 204 + return [
> 205 + enumerated_type_registry[InformationType.name].items[value]
> 206 + for value in self.metadata['information_types']]
>
> 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.name attribute.

> 495 +# def test_revokeAccessGrantsBranches(self):
>
> 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.

Revision history for this message
Ian Booth (wallyworld) wrote :

>
>> 202 + @property
>> 203 + def information_types(self):
>> 204 + return [
>> 205 + enumerated_type_registry[InformationType.name].items[value]
>> 206 + for value in self.metadata['information_types']]
>>
>> 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.name attribute.
>

My mistake, I had grabbed some de-serialisation code and the registry
lookuo was indeed surperfulous here.

Revision history for this message
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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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