launchpad-reviewers team mailing list archive

[Leonard Richardson <leonard.richardson@xxxxxxxxxxxxx>]

Leonard Richardson has proposed merging lp:~leonardr/launchpad/520412-from-thekorn into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #520412 in Launchpad itself: "fix review_types argument of the API's Branch.createMergeProposal method"
  https://bugs.launchpad.net/launchpad/+bug/520412

For more details, see:
https://code.launchpad.net/~leonardr/launchpad/520412-from-thekorn/+merge/54868

This branch makes it so you get a 400 error when you give createMergeProposal inconsistent lists for reviewers and review_types, rather than an OOPS. It's a simple port of a branch that Markus Korn wrote a year ago but didn't land. His code was correct, but a bug in lazr.restful prevented it from working. Now the bug has been fixed and I've ported his code, changing a doctest to a unit test as I did.
-- 
https://code.launchpad.net/~leonardr/launchpad/520412-from-thekorn/+merge/54868
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/520412-from-thekorn into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2011-03-24 12:03:02 +0000
+++ lib/lp/code/interfaces/branch.py	2011-03-25 14:19:51 +0000
@@ -21,6 +21,7 @@
     'IBranchNavigationMenu',
     'IBranchSet',
     'user_has_special_branch_access',
+    'WrongNumberOfReviewTypeArguments',
     ]
 
 from cgi import escape
@@ -29,6 +30,7 @@
 from lazr.restful.declarations import (
     call_with,
     collection_default_content,
+    error_status,
     export_as_webservice_collection,
     export_as_webservice_entry,
     export_destructor_operation,
@@ -106,6 +108,13 @@
     BranchLifecycleStatus.MATURE)
 
 
+@error_status(400)
+class WrongNumberOfReviewTypeArguments(ValueError):
+    """Raised in the webservice API if `reviewers` and `review_types`
+    do not have equal length.
+    """
+
+
 def get_blacklisted_hostnames():
     """Return a list of hostnames blacklisted for Branch URLs."""
     hostnames = config.codehosting.blacklisted_hostnames

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-03-23 16:28:51 +0000
+++ lib/lp/code/model/branch.py	2011-03-25 14:19:51 +0000
@@ -109,6 +109,7 @@
     IBranchNavigationMenu,
     IBranchSet,
     user_has_special_branch_access,
+    WrongNumberOfReviewTypeArguments,
     )
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.code.interfaces.branchlookup import IBranchLookup
@@ -472,7 +473,7 @@
         if review_types is None:
             review_types = []
         if len(reviewers) != len(review_types):
-            raise ValueError(
+            raise WrongNumberOfReviewTypeArguments(
                 'reviewers and review_types must be equal length.')
         review_requests = zip(reviewers, review_types)
         return self.addLandingTarget(

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2011-02-13 22:54:48 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2011-03-25 14:19:51 +0000
@@ -105,7 +105,6 @@
     reviewer_link: u'http://api.launchpad.dev/devel/~person-name...'
     self_link: u'http://api.launchpad.dev/devel/~...'
 
-
 == Get an existing merge proposal ==
 
 Branch merge proposals can be fetched through the API.

=== modified file 'lib/lp/code/tests/test_branch_webservice.py'
--- lib/lp/code/tests/test_branch_webservice.py	2011-03-03 03:04:30 +0000
+++ lib/lp/code/tests/test_branch_webservice.py	2011-03-25 14:19:51 +0000
@@ -3,11 +3,9 @@
 
 __metaclass__ = type
 
-import httplib
-
 from zope.component import getUtility
 
-from lazr.restfulclient.errors import HTTPError
+from lazr.restfulclient.errors import BadRequest
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing.layers import DatabaseFunctionalLayer
@@ -15,6 +13,7 @@
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.testing import (
+    api_url,
     launchpadlib_for,
     login_person,
     logout,
@@ -23,6 +22,34 @@
     )
 
 
+class TestBranchOperations(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_createMergeProposal_fails_if_reviewers_and_review_types_are_different_sizes(self):
+
+        source = self.factory.makeBranch(name='rock')
+        source_url = api_url(source)
+
+        target = self.factory.makeBranch(
+            owner=source.owner, product=source.product,
+            name="roll")
+        target_url = api_url(target)
+
+        lp = launchpadlib_for("test", source.owner.name)
+        source = lp.load(source_url)
+        target = lp.load(target_url)
+
+        exception = self.assertRaises(
+            BadRequest, source.createMergeProposal,
+            target_branch=target, initial_comment='Merge\nit!',
+            needs_review=True, commit_message='It was merged!\n',
+            reviewers=[source.owner.self_link], review_types=[])
+        self.assertEquals(
+            exception.content,
+            'reviewers and review_types must be equal length.')
+
+
 class TestBranchDeletes(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -58,11 +85,8 @@
         logout()
         target_branch = self.lp.branches.getByUniqueName(
             unique_name='~jimhenson/fraggle/rock')
-        api_error = self.assertRaises(
-            HTTPError,
-            target_branch.lp_delete)
+        api_error = self.assertRaises(BadRequest, target_branch.lp_delete)
         self.assertIn('Cannot delete', api_error.content)
-        self.assertEqual(httplib.BAD_REQUEST, api_error.response.status)
 
 
 class TestSlashBranches(TestCaseWithFactory):