launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/sharingservice-rasj-miss into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1026837 in Launchpad itself: "job not created properly when branch access revoked using sharing ui"
  https://bugs.launchpad.net/launchpad/+bug/1026837

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/sharingservice-rasj-miss/+merge/115874

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.
-- 
https://code.launchpad.net/~stevenk/launchpad/sharingservice-rasj-miss/+merge/115874
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/sharingservice-rasj-miss into lp:launchpad.

=== 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:18:21 +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:18:21 +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,7 +1119,7 @@
         self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
         self.assertContentEqual(branches[:9], shared_branches)
 
-    def test_getPeopleWithoutAccess_bugs(self):
+    def test_getPeopleWithoutAccess(self):
         # Test the getPeopleWithoutAccess method.
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
@@ -1137,7 +1144,11 @@
         bug = self.factory.makeBug(
             product=product, owner=owner,
             information_type=InformationType.USERDATA)
-        access_artifact = self.factory.makeAccessArtifact(concrete=bug)
+        branch = self.factory.makeBranch(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        bug_artifact = self.factory.makeAccessArtifact(concrete=bug)
+        branch_artifact = self.factory.makeAccessArtifact(concrete=branch)
 
         # Grant access to some of the people.
         # Some access policy grants.
@@ -1149,11 +1160,15 @@
         # Some access artifact grants.
         for person in people[7:]:
             self.factory.makeAccessArtifactGrant(
-                artifact=access_artifact, grantee=person, grantor=owner)
+                artifact=bug_artifact, grantee=person, grantor=owner)
+            self.factory.makeAccessArtifactGrant(
+                artifact=branch_artifact, grantee=person, grantor=owner)
 
         # Check the results.
         without_access = self.service.getPeopleWithoutAccess(bug, people)
         self.assertContentEqual(people[:5], without_access)
+        without_access = self.service.getPeopleWithoutAccess(branch, people)
+        self.assertContentEqual(people[:5], without_access)
 
     def _make_Artifacts(self):
         # Make artifacts for test (in)visible artifact methods.