Merge lp:~stevenk/launchpad/sharingservice-rasj-miss into lp:launchpad

[Steve Kowalik]

Fix the sharing service to create a RemoveArtifactSubscriptionsJob for branches as well as bugs, as well as writing some extra branch tests.

Fix the sharing services to create a job for branches as well as bugs. Also fix a little bit of lint, remove a few XXXes, and write some branch tests.

[Ian Booth (wallyworld)]

Looks good, thanks.

review: Approve

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-07-17 21:58:06 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-07-20 02:48:18 +0000
@@ -463,11 +463,8 @@
 
         # Create a job to remove subscriptions for artifacts the sharee can no
         # longer see.
-        if bugs:
-            getUtility(IRemoveArtifactSubscriptionsJobSource).create(
-                user, bugs, grantee=sharee, pillar=pillar)
-        # XXX 2012-06-13 wallyworld bug=1012448
-        # Remove branch subscriptions when information type fully implemented.
+        getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+            user, artifacts, grantee=sharee, pillar=pillar)
 
     def ensureAccessGrants(self, sharees, 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-07-17 21:58:06 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-07-20 02:48:18 +0000
@@ -15,7 +15,6 @@
 from lp.app.interfaces.services import IService
 from lp.bugs.interfaces.bug import IBug
 from lp.code.enums import (
-    BranchSubscriptionDiffSize,
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
     )
@@ -816,9 +815,10 @@
         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 bug in bugs or []:
             self.assertNotIn(grantee, bug.getDirectSubscribers())
+        for branch in branches or []:
+            self.assertNotIn(grantee, branch.subscribers)
 
         # Someone else still has access to the bugs and branches.
         grants = accessartifact_grant_source.findByArtifact(
@@ -840,6 +840,15 @@
             information_type=InformationType.USERDATA)
         self._assert_revokeAccessGrants(distro, [bug], None)
 
+    def test_revokeAccessGrantsBranches(self):
+        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:
@@ -866,9 +875,9 @@
                     getUtility(IAccessArtifactGrantSource).revokeByArtifact(
                         accessartifact_source.find([bug]), [person_grantee])
             for branch in branches or []:
-                branch.subscribe(person,
-                    BranchSubscriptionNotificationLevel.NOEMAIL, None,
-                    CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+                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]:
@@ -913,6 +922,15 @@
             information_type=InformationType.USERDATA)
         self._assert_revokeTeamAccessGrants(distro, [bug], None)
 
+    def test_revokeTeamAccessGrantsBranches(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(
+            owner=owner, information_type=InformationType.USERDATA)
+        self._assert_revokeTeamAccessGrants(product, None, [branch])
+
     def _assert_revokeAccessGrantsUnauthorized(self):
         # revokeAccessGrants raises an Unauthorized exception if the user
         # is not permitted to do so.
@@ -1094,17 +1112,6 @@
             grant_access(bug, i == 9)
         for i, branch in enumerate(branches):
             grant_access(branch, i == 9)
-            # XXX bug=1001042 wallyworld 2012-05-18
-            # for now we need to subscribe users to the branch in order
-            # for the underlying BranchCollection to allow access. This will
-            # no longer be the case when BranchCollection supports the new
-            # access policy framework.
-            if i < 9:
-                branch.subscribe(
-                    user, BranchSubscriptionNotificationLevel.NOEMAIL,
-                    BranchSubscriptionDiffSize.NODIFF,
-                    CodeReviewNotificationLevel.NOEMAIL,
-                    owner)
 
         # Check the results.
         shared_bugtasks, shared_branches = self.service.getSharedArtifacts(
@@ -1112,12 +1119,28 @@
         self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
         self.assertContentEqual(branches[:9], shared_branches)
 
-    def test_getPeopleWithoutAccess_bugs(self):
-        # Test the getPeopleWithoutAccess method.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        login_person(owner)
-
+    def test_getPeopleWithAccessBugs(self):
+        # Test the getPeopleWithoutAccess method with bugs.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        bug = self.factory.makeBug(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        login_person(owner)
+        self._assert_getPeopleWithoutAccess(product, bug)
+
+    def test_getPeopleWithAccessBranches(self):
+        # Test the getPeopleWithoutAccess method with branches.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        branch = self.factory.makeBranch(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        login_person(owner)
+        self._assert_getPeopleWithoutAccess(product, branch)
+
+    def _assert_getPeopleWithoutAccess(self, product, artifact):
+        access_artifact = self.factory.makeAccessArtifact(concrete=artifact)
         # Make some people to check. people[:5] will not have access.
         people = []
         # Make a team with access.
@@ -1133,26 +1156,20 @@
         people.append(team_with_access)
         people.append(member_with_access)
 
-        # Make the bug to use.
-        bug = self.factory.makeBug(
-            product=product, owner=owner,
-            information_type=InformationType.USERDATA)
-        access_artifact = self.factory.makeAccessArtifact(concrete=bug)
-
-        # Grant access to some of the people.
-        # Some access policy grants.
+        # Create some access policy grants.
         [policy] = getUtility(IAccessPolicySource).find(
             [(product, InformationType.USERDATA)])
         for person in people[5:7]:
             self.factory.makeAccessPolicyGrant(
-                policy=policy, grantee=person, grantor=owner)
-        # Some access artifact grants.
+                policy=policy, grantee=person, grantor=product.owner)
+        # And some access artifact grants.
         for person in people[7:]:
             self.factory.makeAccessArtifactGrant(
-                artifact=access_artifact, grantee=person, grantor=owner)
+                artifact=access_artifact, grantee=person,
+                grantor=product.owner)
 
         # Check the results.
-        without_access = self.service.getPeopleWithoutAccess(bug, people)
+        without_access = self.service.getPeopleWithoutAccess(artifact, people)
         self.assertContentEqual(people[:5], without_access)
 
     def _make_Artifacts(self):