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

[Ian Booth]

Ensure team members are unsubscribed from bugs they can no longer see when they are removed from a team.

== 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

[William Grant (wgrant)]

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)

[Ian Booth (wallyworld)]

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.

[Ian Booth (wallyworld)]

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

[Ian Booth (wallyworld)]

I've now added pillar filtering to the search. Another branch will add filtering on grantee for team membership removal.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-06-14 07:55:25 +0000
+++ lib/lp/bugs/model/bug.py	2012-06-14 07:55:25 +0000
@@ -1811,7 +1811,7 @@
             # As a result of the transition, some subscribers may no longer
             # have access to the bug. We need to run a job to remove any such
             # subscriptions.
-            getUtility(IRemoveBugSubscriptionsJobSource).create([self], who)
+            getUtility(IRemoveBugSubscriptionsJobSource).create(who, [self])
 
         return True
 
 
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-06-14 07:55:25 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-06-14 07:55:25 +0000
@@ -1193,7 +1193,7 @@
             # have access to the parent bug. We need to run a job to remove any
             # such subscriptions.
             getUtility(IRemoveBugSubscriptionsJobSource).create(
-                [self.bug], user)
+                user, [self.bug], pillar=target_before_change)
 
     def updateTargetNameCache(self, newtarget=None):
         """See `IBugTask`."""
 
=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-06-14 07:55:25 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-06-14 07:55:25 +0000
@@ -5,6 +5,7 @@
 
 __all__ = [
     'get_bug_privacy_filter',
+    'get_bug_privacy_filter_terms',
     'orderby_expression',
     'search_bugs',
     ]
@@ -68,6 +69,7 @@
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.accesspolicy import AccessPolicyGrant
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.milestone import Milestone
@@ -1378,9 +1380,15 @@
     :return: A SQL filter, a decorator to cache visibility in a resultset that
         returns BugTask objects.
     """
-    bug_filter_terms = _get_bug_privacy_filter_terms(user)
-    if not bug_filter_terms:
+    # Admins can see all bugs, so we can short-circuit the filter.
+    if user is not None and IPersonRoles(user).in_admin:
         return True, _nocache_bug_decorator
+
+    # We want an actual Storm Person.
+    if IPersonRoles.providedBy(user):
+        user = user.person
+
+    bug_filter_terms = get_bug_privacy_filter_terms(user, check_admin=False)
     if len(bug_filter_terms) == 1:
         return bug_filter_terms[0], _nocache_bug_decorator
 
@@ -1388,24 +1396,19 @@
     return expr, _make_cache_user_can_view_bug(user)
 
 
-def _get_bug_privacy_filter_terms(user):
+def get_bug_privacy_filter_terms(user, check_admin=True):
     public_bug_filter = (
         BugTaskFlat.information_type.is_in(PUBLIC_INFORMATION_TYPES))
 
     if user is None:
         return [public_bug_filter]
 
-    admin_team = getUtility(ILaunchpadCelebrities).admin
-    if removeSecurityProxy(user).inTeam(admin_team):
-        return []
-
     artifact_grant_query = Coalesce(
             ArrayIntersects(SQL('BugTaskFlat.access_grants'),
             Select(
                 ArrayAgg(TeamParticipation.teamID),
                 tables=TeamParticipation,
-                where=(TeamParticipation.personID ==
-                       user.id)
+                where=(TeamParticipation.person == user)
             )), False)
 
     policy_grant_query = Coalesce(
@@ -1416,9 +1419,18 @@
                         Join(TeamParticipation,
                             TeamParticipation.teamID ==
                             AccessPolicyGrant.grantee_id)),
-                where=(
-                    TeamParticipation.personID ==
-                    user.id)
+                where=(TeamParticipation.person == user)
             )), False)
 
-    return [public_bug_filter, artifact_grant_query, policy_grant_query]
+    filters = [public_bug_filter, artifact_grant_query, policy_grant_query]
+
+    if check_admin:
+        filters.append(
+            Exists(Select(
+                1, tables=[TeamParticipation],
+                where=And(
+                    TeamParticipation.person == user,
+                    TeamParticipation.team ==
+                        getUtility(ILaunchpadCelebrities).admin))))
+
+    return filters
 
=== modified file 'lib/lp/registry/interfaces/sharingjob.py'
--- lib/lp/registry/interfaces/sharingjob.py	2012-05-22 12:05:51 +0000
+++ lib/lp/registry/interfaces/sharingjob.py	2012-06-14 07:55:25 +0000
@@ -92,7 +92,7 @@
 class IRemoveBugSubscriptionsJobSource(ISharingJobSource):
     """An interface for acquiring IRemoveBugSubscriptionsJobs."""
 
-    def create(pillar, bugs, requestor):
+    def create(pillar, requestor, bugs=None, information_types=None):
         """Create a new job to remove subscriptions for the specified bugs.
 
         Subscriptions for users who no longer have access to the bugs are
 
=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-06-14 07:55:25 +0000
@@ -19,18 +19,15 @@
 from lazr.enum import (
     DBEnumeratedType,
     DBItem,
-    enumerated_type_registry,
     )
 import simplejson
 from sqlobject import SQLObjectNotFound
 from storm.expr import (
     And,
-    Coalesce,
     In,
-    Join,
     Not,
+    Or,
     Select,
-    SQL,
     )
 from storm.locals import (
     Int,
@@ -48,7 +45,10 @@
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugsubscription import BugSubscription
 from lp.bugs.model.bugtaskflat import BugTaskFlat
-from lp.bugs.model.bugtasksearch import get_bug_privacy_filter
+from lp.bugs.model.bugtasksearch import (
+    get_bug_privacy_filter,
+    get_bug_privacy_filter_terms,
+    )
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.person import IPersonSet
@@ -61,19 +61,13 @@
     ISharingJob,
     ISharingJobSource,
     )
-from lp.registry.model.accesspolicy import AccessPolicyGrant
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
-from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.lpstorm import IStore
 from lp.services.database.stormbase import StormBase
-from lp.services.database.stormexpr import (
-    ArrayAgg,
-    ArrayIntersects,
-    )
 from lp.services.job.model.job import (
     EnumeratedSubclass,
     Job,
@@ -307,7 +301,7 @@
     @property
     def information_types(self):
         return [
-            enumerated_type_registry[InformationType.name].items[value]
+            InformationType.items[value]
             for value in self.metadata['information_types']]
 
     def getErrorRecipients(self):
@@ -402,18 +396,21 @@
     config = config.IRemoveBugSubscriptionsJobSource
 
     @classmethod
-    def create(cls, bugs, requestor):
+    def create(cls, requestor, bugs=None, grantee=None, pillar=None,
+               information_types=None):
         """See `IRemoveBugSubscriptionsJob`."""
 
-        bug_ids = [
-            bug.id for bug in bugs
+        bug_ids = [bug.id for bug in bugs or []]
+        information_types = [
+            info_type.value for info_type in information_types or []
         ]
         metadata = {
             'bug_ids': bug_ids,
+            'information_types': information_types,
             'requestor.id': requestor.id
         }
         return super(RemoveBugSubscriptionsJob, cls).create(
-            None, None, metadata)
+            pillar, grantee, metadata)
 
     @property
     def requestor_id(self):
@@ -431,6 +428,12 @@
     def bugs(self):
         return getUtility(IBugSet).getByNumbers(self.bug_ids)
 
+    @property
+    def information_types(self):
+        return [
+            InformationType.items[value]
+            for value in self.metadata['information_types']]
+
     def getErrorRecipients(self):
         # If something goes wrong we want to let the requestor know as well
         # as the pillar maintainer (if there is a pillar).
@@ -453,35 +456,27 @@
 
         # Find all bug subscriptions for which the subscriber cannot see the
         # bug.
-        constraints = [
-            BugTaskFlat.bug_id.is_in(self.bug_ids),
-            Not(Coalesce(
-                ArrayIntersects(SQL('BugTaskFlat.access_grants'),
-                Select(
-                    ArrayAgg(TeamParticipation.teamID),
-                    tables=TeamParticipation,
-                    where=(TeamParticipation.personID ==
-                           BugSubscription.person_id)
-                )), False)),
-            Not(Coalesce(
-                ArrayIntersects(SQL('BugTaskFlat.access_policies'),
-                Select(
-                    ArrayAgg(AccessPolicyGrant.policy_id),
-                    tables=(AccessPolicyGrant,
-                            Join(TeamParticipation,
-                                TeamParticipation.teamID ==
-                                AccessPolicyGrant.grantee_id)),
-                    where=(
-                        TeamParticipation.personID ==
-                        BugSubscription.person_id)
-                )), False))
-        ]
+        invisible_filter = (
+            Not(Or(*get_bug_privacy_filter_terms(BugSubscription.person_id))))
+        filters = [invisible_filter]
+
+        if self.bug_ids:
+            filters.append(BugTaskFlat.bug_id.is_in(self.bug_ids))
+        else:
+            if self.information_types:
+                filters.append(
+                    BugTaskFlat.information_type.is_in(self.information_types))
+            if self.product:
+                filters.append(
+                    BugTaskFlat.product == self.product)
+            if self.distro:
+                filters.append(
+                    BugTaskFlat.distribution == self.distro)
+
         subscriptions = IStore(BugSubscription).find(
             BugSubscription,
             In(BugSubscription.bug_id,
-                Select(
-                    BugTaskFlat.bug_id,
-                    where=And(*constraints)))
+                Select(BugTaskFlat.bug_id, where=And(*filters)))
         )
         for sub in subscriptions:
             sub.bug.unsubscribe(
 
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/model/teammembership.py	2012-06-14 07:55:25 +0000
@@ -43,7 +43,7 @@
     )
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.sharingjob import (
-    IRemoveGranteeSubscriptionsJobSource,
+    IRemoveBugSubscriptionsJobSource,
     )
 from lp.registry.interfaces.teammembership import (
     ACTIVE_STATES,
@@ -393,9 +393,8 @@
                 # A person has left the team so they may no longer have access
                 # to some artifacts shared with the team. We need to run a job
                 # to remove any subscriptions to such artifacts.
-                getUtility(IRemoveGranteeSubscriptionsJobSource).create(
-                    None, self.person, user)
-
+                getUtility(IRemoveBugSubscriptionsJobSource).create(
+                    user, grantee=self.team)
         else:
             # Changed from an inactive state to another inactive one, so no
             # need to fill/clean the TeamParticipation table.
 
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-06-14 07:55:25 +0000
@@ -40,7 +40,7 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.sharingjob import (
-    IRemoveGranteeSubscriptionsJobSource,
+    IRemoveBugSubscriptionsJobSource,
     )
 from lp.registry.interfaces.sharingservice import ISharingService
 from lp.registry.model.person import Person
@@ -328,12 +328,13 @@
         if len(to_delete) > 0:
             accessartifact_grant_source = getUtility(
                 IAccessArtifactGrantSource)
-            accessartifact_grant_source.revokeByArtifact(to_delete)
+            accessartifact_grant_source.revokeByArtifact(to_delete, [sharee])
 
         # Create a job to remove subscriptions for artifacts the sharee can no
         # longer see.
-        getUtility(IRemoveGranteeSubscriptionsJobSource).create(
-            pillar, sharee, user, information_types=information_types)
+        getUtility(IRemoveBugSubscriptionsJobSource).create(
+            user, bugs=None, pillar=pillar,
+            information_types=information_types)
 
     @available_with_permission('launchpad.Edit', 'pillar')
     def revokeAccessGrants(self, pillar, sharee, user, branches=None,
@@ -358,8 +359,11 @@
 
         # Create a job to remove subscriptions for artifacts the sharee can no
         # longer see.
-        getUtility(IRemoveGranteeSubscriptionsJobSource).create(
-            pillar, sharee, user, bugs=bugs, branches=branches)
+        if bugs:
+            getUtility(IRemoveBugSubscriptionsJobSource).create(
+                user, bugs, pillar=pillar)
+        # XXX 2012-06-13 wallyworld bug=1012448
+        # Remove branch subscriptions when information type fully implemented.
 
     def ensureAccessGrants(self, sharee, user, branches=None, bugs=None,
                            ignore_permissions=False):
 
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-06-14 07:55:25 +0000
@@ -26,10 +26,12 @@
     )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
+    IAccessArtifactSource,
     IAccessPolicyGrantFlatSource,
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.registry.services.sharingservice import SharingService
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.tests import block_on_job
@@ -55,7 +57,7 @@
 WRITE_FLAG = {
     'disclosure.enhanced_sharing.writable': 'true',
     'disclosure.enhanced_sharing_details.enabled': 'true',
-    'jobs.celery.enabled_classes': 'RemoveGranteeSubscriptionsJob'}
+    'jobs.celery.enabled_classes': 'RemoveBugSubscriptionsJob'}
 DETAILS_FLAG = {'disclosure.enhanced_sharing_details.enabled': 'true'}
 
 
@@ -541,12 +543,15 @@
         # Make some artifact grants for our sharee.
         artifact = self.factory.makeAccessArtifact()
         self.factory.makeAccessArtifactGrant(artifact, grantee)
+        # Make some access policy grants for another sharee.
+        another = self.factory.makePerson()
+        self.factory.makeAccessPolicyGrant(access_policies[0], another)
+        # Make some artifact grants for our yet another sharee.
+        yet_another = self.factory.makePerson()
+        self.factory.makeAccessArtifactGrant(artifact, yet_another)
         for access_policy in access_policies:
             self.factory.makeAccessPolicyArtifact(
                 artifact=artifact, policy=access_policy)
-        # Make some access policy grants for another sharee.
-        another = self.factory.makePerson()
-        self.factory.makeAccessPolicyGrant(access_policies[0], another)
         # Delete data for a specific information type.
         with FeatureFixture(WRITE_FLAG):
             self.service.deletePillarSharee(
@@ -563,10 +568,16 @@
             expected_data = [
                 (grantee, {policy: SharingPermission.ALL}, [])
                 for policy in expected_policies]
-        # Add the expected data for the other sharee.
+        # Add the expected data for the other sharees.
         another_person_data = (
             another, {access_policies[0]: SharingPermission.ALL}, [])
         expected_data.append(another_person_data)
+        policy_permissions = dict([(
+            policy, SharingPermission.SOME) for policy in access_policies])
+        yet_another_person_data = (
+            yet_another, policy_permissions,
+            [InformationType.USERDATA, InformationType.EMBARGOEDSECURITY])
+        expected_data.append(yet_another_person_data)
         self.assertContentEqual(
             expected_data, self.service.getPillarSharees(pillar))
 
@@ -768,15 +779,102 @@
             information_type=InformationType.USERDATA)
         self._assert_revokeAccessGrants(distro, [bug], None)
 
-    def test_revokeAccessGrantsBranches(self):
+    # XXX 2012-06-13 wallyworld bug=1012448
+    # Remove branch subscriptions when information type fully implemented.
+#    def test_revokeAccessGrantsBranches(self):
+#        # Users with launchpad.Edit can delete all access for a sharee.
+#        owner = self.factory.makePerson()
+#        product = self.factory.makeProduct(owner=owner)
+#        login_person(owner)
+#        branch = self.factory.makeBranch(
+#            product=product, owner=owner,
+#            information_type=InformationType.USERDATA)
+#        self._assert_revokeAccessGrants(product, None, [branch])
+
+    def _assert_revokeTeamAccessGrants(self, pillar, bugs, branches):
+        artifacts = []
+        if bugs:
+            artifacts.extend(bugs)
+        if branches:
+            artifacts.extend(branches)
+        policy = self.factory.makeAccessPolicy(pillar=pillar)
+
+        person_grantee = self.factory.makePerson()
+        team_owner = self.factory.makePerson()
+        team_grantee = self.factory.makeTeam(
+            owner=team_owner,
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
+            members=[person_grantee])
+
+        # Subscribe the team and person grantees to the artifacts.
+        for person in [team_grantee, person_grantee]:
+            for bug in bugs or []:
+                bug.subscribe(person, pillar.owner)
+                # XXX 2012-06-12 wallyworld bug=1002596
+                # No need to revoke AAG with triggers removed.
+                if person == person_grantee:
+                    accessartifact_source = getUtility(IAccessArtifactSource)
+                    getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+                        accessartifact_source.find([bug]), [person_grantee])
+            for branch in branches or []:
+                branch.subscribe(person,
+                    BranchSubscriptionNotificationLevel.NOEMAIL, None,
+                    CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+
+        # Check that grantees have expected access grants and subscriptions.
+        for person in [team_grantee, person_grantee]:
+            visible_bugs, visible_branches = self.service.getVisibleArtifacts(
+                person, branches, bugs)
+            self.assertContentEqual(bugs or [], visible_bugs)
+            self.assertContentEqual(branches or [], visible_branches)
+        for person in [team_grantee, person_grantee]:
+            for bug in bugs or []:
+                self.assertIn(person, bug.getDirectSubscribers())
+
+        with FeatureFixture(WRITE_FLAG):
+            self.service.revokeAccessGrants(
+                pillar, team_grantee, pillar.owner,
+                bugs=bugs, branches=branches)
+        with block_on_job(self):
+            transaction.commit()
+
+        # The grantees now have no access to anything.
+        apgfs = getUtility(IAccessPolicyGrantFlatSource)
+        permission_info = apgfs.findGranteePermissionsByPolicy(
+            [policy], [team_grantee, person_grantee])
+        self.assertEqual(0, permission_info.count())
+
+        # Check that the grantee's subscriptions have been removed.
+        # Branches will be done once they have the information_type attribute.
+        for person in [team_grantee, person_grantee]:
+            for bug in bugs or []:
+                self.assertNotIn(person, bug.getDirectSubscribers())
+            visible_bugs, visible_branches = self.service.getVisibleArtifacts(
+                person, branches, bugs)
+            self.assertContentEqual([], visible_bugs)
+            self.assertContentEqual([], visible_branches)
+
+    def test_revokeTeamAccessGrantsBugs(self):
         # Users with launchpad.Edit can delete all access for a sharee.
         owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
+        distro = self.factory.makeDistribution(owner=owner)
         login_person(owner)
-        branch = self.factory.makeBranch(
-            product=product, owner=owner,
+        bug = self.factory.makeBug(
+            distribution=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeAccessGrants(product, None, [branch])
+        self._assert_revokeTeamAccessGrants(distro, [bug], None)
+
+    # XXX 2012-06-13 wallyworld bug=1012448
+    # Remove branch subscriptions when information type fully implemented.
+#    def test_revokeAccessGrantsBranches(self):
+#        # Users with launchpad.Edit can delete all access for a sharee.
+#        owner = self.factory.makePerson()
+#        product = self.factory.makeProduct(owner=owner)
+#        login_person(owner)
+#        branch = self.factory.makeBranch(
+#            product=product, owner=owner,
+#            information_type=InformationType.USERDATA)
+#        self._assert_revokeTeamAccessGrants(distro, [bug], None)
 
     def _assert_revokeAccessGrantsUnauthorized(self):
         # revokeAccessGrants raises an Unauthorized exception if the user
 
=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-06-14 07:55:25 +0000
@@ -481,7 +481,7 @@
 
         def create_job(distro, bug, grantee, owner):
             job = getUtility(IRemoveBugSubscriptionsJobSource).create(
-                [bug], owner)
+                owner, [bug])
             with person_logged_in(owner):
                 bug.transitionToInformationType(
                             InformationType.EMBARGOEDSECURITY, owner)
@@ -515,7 +515,7 @@
         requestor = self.factory.makePerson()
         bug = self.factory.makeBug()
         job = getUtility(IRemoveBugSubscriptionsJobSource).create(
-            [bug], requestor)
+            requestor, [bug])
         naked_job = removeSecurityProxy(job)
         self.assertIsInstance(job, RemoveBugSubscriptionsJob)
         self.assertEqual(requestor.id, naked_job.requestor_id)
@@ -527,7 +527,7 @@
         product = self.factory.makeProduct()
         bug = self.factory.makeBug(product=product)
         job = getUtility(IRemoveBugSubscriptionsJobSource).create(
-            [bug], requestor)
+            requestor, [bug], pillar=product)
         expected_emails = [
             format_address_for_person(person)
             for person in (product.owner, requestor)]
@@ -580,7 +580,7 @@
         reconcile_access_for_artifact(
             bug, bug.information_type, bug.affected_pillars)
 
-        getUtility(IRemoveBugSubscriptionsJobSource).create([bug], owner)
+        getUtility(IRemoveBugSubscriptionsJobSource).create(owner, [bug])
         with block_on_job(self):
             transaction.commit()
 
@@ -611,3 +611,71 @@
             removeSecurityProxy(bug).default_bugtask.product = another_product
 
         self._assert_bug_change_unsubscribes(change_target)
+
+    def _make_subscribed_bug(self, grantee, product=None, distribution=None,
+                             information_type=InformationType.USERDATA):
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(
+            owner=owner, product=product, distribution=distribution,
+            information_type=information_type)
+        with person_logged_in(owner):
+            bug.subscribe(grantee, owner)
+        # Subscribing grantee to bug creates an access grant so we need to
+        # revoke that for our test.
+        accessartifact_source = getUtility(IAccessArtifactSource)
+        accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
+        accessartifact_grant_source.revokeByArtifact(
+            accessartifact_source.find([bug]), [grantee])
+
+        return bug, owner
+
+    def test_unsubscribe_pillar_artifacts_specific_info_types(self):
+        # Only remove subscriptions for bugs of the specified info type.
+
+        person_grantee = self.factory.makePerson(name='grantee')
+
+        owner = self.factory.makePerson(name='pillarowner')
+        pillar = self.factory.makeProduct(owner=owner)
+
+        # Make bugs the person_grantee is subscribed to.
+        bug1, ignored = self._make_subscribed_bug(
+            person_grantee, product=pillar,
+            information_type=InformationType.USERDATA)
+
+        bug2, ignored = self._make_subscribed_bug(
+            person_grantee, product=pillar,
+            information_type=InformationType.EMBARGOEDSECURITY)
+
+        # Now run the job, removing access to userdata artifacts.
+        getUtility(IRemoveBugSubscriptionsJobSource).create(
+            pillar.owner, pillar=pillar,
+            information_types=[InformationType.USERDATA])
+        with block_on_job(self):
+            transaction.commit()
+
+        self.assertNotIn(
+            person_grantee, removeSecurityProxy(bug1).getDirectSubscribers())
+        self.assertIn(
+            person_grantee, removeSecurityProxy(bug2).getDirectSubscribers())
+
+    def test_admins_retain_subscriptions(self):
+        # Admins subscriptions are retained even if they don't have explicit
+        # access.
+        product = self.factory.makeProduct()
+        owner = self.factory.makePerson()
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+
+        login_person(owner)
+        bug = self.factory.makeBug(
+            owner=owner, product=product,
+            information_type=InformationType.USERDATA)
+
+        bug.subscribe(admin, owner)
+        getUtility(IRemoveBugSubscriptionsJobSource).create(
+            owner, [bug], pillar=product)
+        with block_on_job(self):
+            transaction.commit()
+
+        # Check the result. admin should still be subscribed.
+        subscribers = removeSecurityProxy(bug).getDirectSubscribers()
+        self.assertIn(admin, subscribers)
 
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2012-06-14 07:55:25 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2012-06-14 07:55:25 +0000
@@ -999,8 +999,7 @@
     def setUp(self):
         self.useFixture(FeatureFixture({
             'disclosure.unsubscribe_jobs.enabled': 'true',
-            'jobs.celery.enabled_classes':
-                'RemoveGranteeSubscriptionsJob',
+            'jobs.celery.enabled_classes': 'RemoveBugSubscriptionsJob',
         }))
         super(TestTeamMembershipJobs, self).setUp()