launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/createAccessGrants-robust into lp:launchpad with lp:~wallyworld/launchpad/sharing-jobs-config as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/createAccessGrants-robust/+merge/108518

== Implemenation ==

Rename ISharingService.createAccessGrants to ensureAccessGrants and make it check to see if any grants already exists and only create any missing ones.

== Tests ==

Update the sharing service tests which call _assert_ensureAccessGrants, and add a new test for the new method behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingser
-- 
https://code.launchpad.net/~wallyworld/launchpad/createAccessGrants-robust/+merge/108518
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/createAccessGrants-robust into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-06-04 04:25:25 +0000
+++ lib/lp/bugs/model/bug.py	2012-06-04 04:25:25 +0000
@@ -851,7 +851,7 @@
             service = getUtility(IService, 'sharing')
             bugs, ignored = service.getVisibleArtifacts(person, bugs=[self])
             if not bugs:
-                service.createAccessGrants(
+                service.ensureAccessGrants(
                     subscribed_by, person, bugs=[self],
                     ignore_permissions=True)
 

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-06-04 04:25:25 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-06-04 04:25:25 +0000
@@ -171,8 +171,8 @@
         branches=List(
             Reference(schema=IBranch), title=_('Branches'), required=False))
     @operation_for_version('devel')
-    def createAccessGrants(user, sharee, branches=None, bugs=None):
-        """Grant a sharee access to the specified artifacts.
+    def ensureAccessGrants(user, sharee, branches=None, bugs=None):
+        """Ensure a sharee has an access grant to the specified artifacts.
 
         :param user: the user making the request
         :param sharee: the person or team for whom to grant access

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-06-04 04:25:25 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-06-04 04:25:25 +0000
@@ -361,7 +361,7 @@
         getUtility(IRemoveGranteeSubscriptionsJobSource).create(
             pillar, sharee, user, bugs=bugs, branches=branches)
 
-    def createAccessGrants(self, user, sharee, branches=None, bugs=None,
+    def ensureAccessGrants(self, user, sharee, branches=None, bugs=None,
                            **kwargs):
         """See `ISharingService`."""
 
@@ -384,6 +384,13 @@
         # Ensure there are access artifacts associated with the bugs and
         # branches.
         artifacts = getUtility(IAccessArtifactSource).ensure(artifacts)
-        # Create access to bugs/branches for the specified sharee.
+        aagsource = getUtility(IAccessArtifactGrantSource)
+        artifacts_with_grants = [
+            artifact_grant.abstract_artifact
+            for artifact_grant in
+            aagsource.find([(artifact, sharee) for artifact in artifacts])]
+        # Create access to bugs/branches for the specified sharee for which a
+        # grant does not already exist.
+        missing_artifacts = set(artifacts) - set(artifacts_with_grants)
         getUtility(IAccessArtifactGrantSource).grant(
-            list(product(artifacts, [sharee], [user])))
+            list(product(missing_artifacts, [sharee], [user])))

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-06-04 04:25:25 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-06-04 04:25:25 +0000
@@ -814,11 +814,13 @@
             Unauthorized, self.service.revokeAccessGrants,
             product, product.owner, sharee, bugs=[bug])
 
-    def _assert_createAccessGrants(self, user, bugs, branches):
+    def _assert_ensureAccessGrants(self, user, bugs, branches,
+                                   grantee=None):
         # Creating access grants works as expected.
-        grantee = self.factory.makePerson()
+        if not grantee:
+            grantee = self.factory.makePerson()
         with FeatureFixture(WRITE_FLAG):
-            self.service.createAccessGrants(
+            self.service.ensureAccessGrants(
                 user, grantee, bugs=bugs, branches=branches)
 
         # Check that grantee has expected access grants.
@@ -841,7 +843,7 @@
         self.assertContentEqual(bugs or [], shared_bugs)
         self.assertContentEqual(branches or [], shared_branches)
 
-    def test_createAccessGrantsBugs(self):
+    def test_ensureAccessGrantsBugs(self):
         # Access grants can be created for bugs.
         owner = self.factory.makePerson()
         distro = self.factory.makeDistribution(owner=owner)
@@ -849,19 +851,39 @@
         bug = self.factory.makeBug(
             distribution=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_createAccessGrants(owner, [bug], None)
+        self._assert_ensureAccessGrants(owner, [bug], None)
 
-    def test_createAccessGrantsBranches(self):
+    def test_ensureAccessGrantsBranches(self):
         # Access grants can be created for branches.
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         login_person(owner)
         branch = self.factory.makeBranch(
-            product=product, owner=owner, private=True)
-        self._assert_createAccessGrants(owner, None, [branch])
-
-    def _assert_createAccessGrantsUnauthorized(self, user):
-        # createAccessGrants raises an Unauthorized exception if the user
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        self._assert_ensureAccessGrants(owner, None, [branch])
+
+    def test_ensureAccessGrantsExisting(self):
+        # Any existing access grants are retained and new ones created.
+        owner = self.factory.makePerson()
+        distro = self.factory.makeDistribution(owner=owner)
+        login_person(owner)
+        bug = self.factory.makeBug(
+            distribution=distro, owner=owner,
+            information_type=InformationType.USERDATA)
+        bug2 = self.factory.makeBug(
+            distribution=distro, owner=owner,
+            information_type=InformationType.USERDATA)
+        # Create an existing access grant.
+        grantee = self.factory.makePerson()
+        with FeatureFixture(WRITE_FLAG):
+            self.service.ensureAccessGrants(owner, grantee, bugs=[bug])
+        # Test with a new bug as well as the one for which access is already
+        # granted.
+        self._assert_ensureAccessGrants(owner, [bug, bug2], None, grantee)
+
+    def _assert_ensureAccessGrantsUnauthorized(self, user):
+        # ensureAccessGrants raises an Unauthorized exception if the user
         # is not permitted to do so.
         product = self.factory.makeProduct()
         bug = self.factory.makeBug(
@@ -869,23 +891,23 @@
         sharee = self.factory.makePerson()
         with FeatureFixture(WRITE_FLAG):
             self.assertRaises(
-                Unauthorized, self.service.createAccessGrants,
+                Unauthorized, self.service.ensureAccessGrants,
                 user, sharee, bugs=[bug])
 
-    def test_createAccessGrantsAnonymous(self):
+    def test_ensureAccessGrantsAnonymous(self):
         # Anonymous users are not allowed.
         with FeatureFixture(WRITE_FLAG):
             login(ANONYMOUS)
-            self._assert_createAccessGrantsUnauthorized(ANONYMOUS)
+            self._assert_ensureAccessGrantsUnauthorized(ANONYMOUS)
 
-    def test_createAccessGrantsAnyone(self):
+    def test_ensureAccessGrantsAnyone(self):
         # Unauthorized users are not allowed.
         with FeatureFixture(WRITE_FLAG):
             anyone = self.factory.makePerson()
             login_person(anyone)
-            self._assert_createAccessGrantsUnauthorized(anyone)
+            self._assert_ensureAccessGrantsUnauthorized(anyone)
 
-    def test_createAccessGrants_without_flag(self):
+    def test_ensureAccessGrants_without_flag(self):
         # The feature flag needs to be enabled.
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
@@ -894,7 +916,7 @@
         sharee = self.factory.makePerson()
         login_person(owner)
         self.assertRaises(
-            Unauthorized, self.service.createAccessGrants,
+            Unauthorized, self.service.ensureAccessGrants,
             product.owner, sharee, bugs=[bug])
 
     def test_getSharedArtifacts(self):
@@ -972,7 +994,8 @@
         branches = []
         for x in range(0, 10):
             branch = self.factory.makeBranch(
-                product=product, owner=owner, private=True)
+                product=product, owner=owner,
+                information_type=InformationType.USERDATA)
             branches.append(branch)
 
         def grant_access(artifact):