launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/revokeAccessGrants_specs into lp:launchpad.

Requested reviews:
  Richard Harding (rharding)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/revokeAccessGrants_specs/+merge/126261

This branch extends the method SharingService.revokeAccessGrants() to
handle specifications too.

The changes mostly copy the pattern already used for bugs and branches.

test: ./bin/test -vvt    lp.registry.services.tests.test_sharingservice

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/revokeAccessGrants_specs/+merge/126261
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-09-19 13:22:42 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-09-25 14:46:23 +0000
@@ -273,7 +273,8 @@
         branches=List(
             Reference(schema=IBranch), title=_('Branches'), required=False))
     @operation_for_version('devel')
-    def revokeAccessGrants(pillar, grantee, user, branches=None, bugs=None):
+    def revokeAccessGrants(pillar, grantee, user, branches=None, bugs=None,
+                           specifications=None):
         """Remove a grantee's access to the specified artifacts.
 
         :param pillar: the pillar from which to remove access
@@ -281,6 +282,7 @@
         :param user: the user making the request
         :param bugs: the bugs for which to revoke access
         :param branches: the branches for which to revoke access
+        :param specifications: the specifications for which to revoke access
         """
 
     @export_write_operation()

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-09-24 03:43:34 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-09-25 14:46:23 +0000
@@ -604,17 +604,20 @@
 
     @available_with_permission('launchpad.Edit', 'pillar')
     def revokeAccessGrants(self, pillar, grantee, user, branches=None,
-                           bugs=None):
+                           bugs=None, specifications=None):
         """See `ISharingService`."""
 
-        if not branches and not bugs:
-            raise ValueError("Either bugs or branches must be specified")
+        if not branches and not bugs and not specifications:
+            raise ValueError(
+                "Either bugs, branches or specifications must be specified")
 
         artifacts = []
         if branches:
             artifacts.extend(branches)
         if bugs:
             artifacts.extend(bugs)
+        if specifications:
+            artifacts.extend(specifications)
         # Find the access artifacts associated with the bugs and branches.
         accessartifact_source = getUtility(IAccessArtifactSource)
         artifacts_to_delete = accessartifact_source.find(artifacts)

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-09-24 03:58:15 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-09-25 14:46:23 +0000
@@ -886,12 +886,15 @@
         self._assert_deleteGranteeRemoveSubscriptions(
             [InformationType.USERDATA])
 
-    def _assert_revokeAccessGrants(self, pillar, bugs, branches):
+    def _assert_revokeAccessGrants(self, pillar, bugs, branches,
+                                   specifications):
         artifacts = []
         if bugs:
             artifacts.extend(bugs)
         if branches:
             artifacts.extend(branches)
+        if specifications:
+            artifacts.extend(specifications)
         policy = self.factory.makeAccessPolicy(pillar=pillar)
         # Grant access to a grantee and another person.
         grantee = self.factory.makePerson()
@@ -916,6 +919,8 @@
                 branch.subscribe(person,
                     BranchSubscriptionNotificationLevel.NOEMAIL, None,
                     CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+            for spec in specifications or []:
+                spec.subscribe(person)
 
         # Check that grantee has expected access grants.
         accessartifact_grant_source = getUtility(IAccessArtifactGrantSource)
@@ -925,7 +930,8 @@
         self.assertEqual(1, grants.count())
 
         self.service.revokeAccessGrants(
-            pillar, grantee, pillar.owner, bugs=bugs, branches=branches)
+            pillar, grantee, pillar.owner, bugs=bugs, branches=branches,
+            specifications=specifications)
         with block_on_job(self):
             transaction.commit()
 
@@ -939,6 +945,8 @@
             self.assertNotIn(grantee, bug.getDirectSubscribers())
         for branch in branches or []:
             self.assertNotIn(grantee, branch.subscribers)
+        for spec in specifications or []:
+            self.assertNotIn(grantee, spec.subscribers)
 
         # Someone else still has access to the bugs and branches.
         grants = accessartifact_grant_source.findByArtifact(
@@ -949,6 +957,8 @@
             self.assertIn(someone, bug.getDirectSubscribers())
         for branch in branches or []:
             self.assertIn(someone, branch.subscribers)
+        for spec in specifications or []:
+            self.assertIn(someone, spec.subscribers)
 
     def test_revokeAccessGrantsBugs(self):
         # Users with launchpad.Edit can delete all access for a grantee.
@@ -958,7 +968,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeAccessGrants(distro, [bug], None)
+        self._assert_revokeAccessGrants(distro, [bug], None, None)
 
     def test_revokeAccessGrantsBranches(self):
         owner = self.factory.makePerson()
@@ -967,14 +977,30 @@
         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):
+        self._assert_revokeAccessGrants(product, None, [branch], None)
+
+    def test_revokeAccessGrantsSpecifications(self):
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=(
+                SpecificationSharingPolicy.EMBARGOED_OR_PROPRIETARY))
+        self.factory.makeAccessPolicy(
+            pillar=product, type=InformationType.EMBARGOED)
+        login_person(owner)
+        specification = self.factory.makeSpecification(
+            product=product, owner=owner,
+            information_type=InformationType.EMBARGOED)
+        self._assert_revokeAccessGrants(product, None, None, [specification])
+
+    def _assert_revokeTeamAccessGrants(self, pillar, bugs, branches,
+                                       specifications):
         artifacts = []
         if bugs:
             artifacts.extend(bugs)
         if branches:
             artifacts.extend(branches)
+        if specifications:
+            artifacts.extend(specifications)
         policy = self.factory.makeAccessPolicy(pillar=pillar)
 
         person_grantee = self.factory.makePerson()
@@ -982,7 +1008,7 @@
         team_grantee = self.factory.makeTeam(
             owner=team_owner,
             membership_policy=TeamMembershipPolicy.RESTRICTED,
-            members=[person_grantee])
+            members=[person_grantee], email='team@xxxxxxxxxxx')
 
         # Subscribe the team and person grantees to the artifacts.
         for person in [team_grantee, person_grantee]:
@@ -998,19 +1024,28 @@
                 branch.subscribe(
                     person, BranchSubscriptionNotificationLevel.NOEMAIL,
                     None, CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+            # Snbscribing somebody to a specification does not yet imply
+            # granting access to this person.
+            self.service.ensureAccessGrants(
+                [person], pillar.owner, specifications=specifications)
+            for spec in specifications or []:
+                spec.subscribe(person)
 
         # Check that grantees have expected access grants and subscriptions.
         for person in [team_grantee, person_grantee]:
             visible_bugs, visible_branches, visible_specs = (
-                self.service.getVisibleArtifacts(person, branches, bugs))
+                self.service.getVisibleArtifacts(
+                    person, branches, bugs, specifications))
             self.assertContentEqual(bugs or [], visible_bugs)
             self.assertContentEqual(branches or [], visible_branches)
+            self.assertContentEqual(specifications or [], visible_specs)
         for person in [team_grantee, person_grantee]:
             for bug in bugs or []:
                 self.assertIn(person, bug.getDirectSubscribers())
 
         self.service.revokeAccessGrants(
-            pillar, team_grantee, pillar.owner, bugs=bugs, branches=branches)
+            pillar, team_grantee, pillar.owner, bugs=bugs, branches=branches,
+            specifications=specifications)
         with block_on_job(self):
             transaction.commit()
 
@@ -1029,6 +1064,7 @@
                 self.service.getVisibleArtifacts(person, branches, bugs))
             self.assertContentEqual([], visible_bugs)
             self.assertContentEqual([], visible_branches)
+            self.assertContentEqual([], visible_specs)
 
     def test_revokeTeamAccessGrantsBugs(self):
         # Users with launchpad.Edit can delete all access for a grantee.
@@ -1038,7 +1074,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeTeamAccessGrants(distro, [bug], None)
+        self._assert_revokeTeamAccessGrants(distro, [bug], None, None)
 
     def test_revokeTeamAccessGrantsBranches(self):
         # Users with launchpad.Edit can delete all access for a grantee.
@@ -1047,7 +1083,21 @@
         login_person(owner)
         branch = self.factory.makeBranch(
             owner=owner, information_type=InformationType.USERDATA)
-        self._assert_revokeTeamAccessGrants(product, None, [branch])
+        self._assert_revokeTeamAccessGrants(product, None, [branch], None)
+
+    def test_revokeTeamAccessGrantsSpecifications(self):
+        # Users with launchpad.Edit can delete all access for a grantee.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=(
+                SpecificationSharingPolicy.EMBARGOED_OR_PROPRIETARY))
+        self.factory.makeAccessPolicy(product, InformationType.EMBARGOED)
+        login_person(owner)
+        specification = self.factory.makeSpecification(
+            product=product, owner=owner,
+            information_type=InformationType.EMBARGOED)
+        self._assert_revokeTeamAccessGrants(
+            product, None, None, [specification])
 
     def _assert_revokeAccessGrantsUnauthorized(self):
         # revokeAccessGrants raises an Unauthorized exception if the user