launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/revokeAccessGrants-robust-1040898 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1040898 in Launchpad itself: "ProgrammingError call revokeAccessGrants() with nonsensical args"
  https://bugs.launchpad.net/launchpad/+bug/1040898

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/revokeAccessGrants-robust-1040898/+merge/121352

== Implementation ==

Quick fix to raise a ValueError if ISharingService.revokeAccessGrants() is invoked without specifying at least one of bugs or branches.

== Tests ==

Add new sharing service test: test_revokeAccessGrants_without_bugs_or_branches

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/revokeAccessGrants-robust-1040898/+merge/121352
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/revokeAccessGrants-robust-1040898 into lp:launchpad.

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-08-16 06:06:36 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-08-27 00:11:19 +0000
@@ -515,6 +515,9 @@
         if not self.write_enabled:
             raise Unauthorized("This feature is not yet enabled.")
 
+        if not branches and not bugs:
+            raise ValueError("Either bugs or branches must be specified")
+
         artifacts = []
         if branches:
             artifacts.extend(branches)

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-08-16 06:06:36 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-08-27 00:11:19 +0000
@@ -1071,6 +1071,17 @@
             Unauthorized, self.service.revokeAccessGrants,
             product, grantee, product.owner, bugs=[bug])
 
+    def test_revokeAccessGrants_without_bugs_or_branches(self):
+        # The revokeAccessGrants method raises a ValueError if called without
+        # specifying either bugs or branches.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        grantee = self.factory.makePerson()
+        login_person(owner)
+        self.assertRaises(
+            ValueError, self.service.revokeAccessGrants,
+            product, grantee, product.owner)
+
     def _assert_ensureAccessGrants(self, user, bugs, branches,
                                    grantee=None):
         # Creating access grants works as expected.