launchpad-reviewers team mailing list archive

[Leonard Richardson <leonard.richardson@xxxxxxxxxxxxx>]

Leonard Richardson has proposed merging lp:~leonardr/launchpad/bug-106338 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~leonardr/launchpad/bug-106338/+merge/52085

This branch takes advantage of a lazr.restful bugfix to fix bug 106338, an OOPS when you try to modify a conjoined bugtask over the web service.

The actual fix is only one line:

 class ConjoinedBugTaskEditError(Exception):
     """An error raised when trying to modify a conjoined bugtask."""
+    webservice_error(httplib.BAD_REQUEST)

But without lazr.restful 0.17.2, that webservice_error() call does absolutely nothing. To make a ConjoinedBugTaskEditError turn into a 400 request, you had to call lazr.restful.errors.expose() on *each instance*. Now you can call it on the class.

I also did some drive-by test cleanup to make it easier to write unit tests using launchpadlib instead of WebServiceCaller. I added a default version to launchpadlib_for, since all unit tests that aren't version-specific should be testing the 'devel' version. I also added a helper method for turning a data model object into a URL you can pass into Launchpad.load() to get the correpsonding object through the web service. Then I rewrote a couple of tests to use launchpadlib instead of WebServiceCaller.

-- 
https://code.launchpad.net/~leonardr/launchpad/bug-106338/+merge/52085
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/bug-106338 into lp:launchpad.

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2011-02-24 15:30:54 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2011-03-03 15:25:28 +0000
@@ -47,6 +47,7 @@
     'valid_remote_bug_url',
     ]
 
+import httplib
 from lazr.enum import (
     DBEnumeratedType,
     DBItem,
@@ -402,6 +403,7 @@
 
 class ConjoinedBugTaskEditError(Exception):
     """An error raised when trying to modify a conjoined bugtask."""
+    webservice_error(httplib.BAD_REQUEST)
 
 
 class UserCannotEditBugTaskStatus(Unauthorized):
@@ -410,7 +412,7 @@
     Raised when a user tries to transition to a new status who doesn't
     have the necessary permissions.
     """
-    webservice_error(401) # HTTP Error: 'Unauthorised'
+    webservice_error(httplib.UNAUTHORIZED)
 
 
 class UserCannotEditBugTaskImportance(Unauthorized):
@@ -419,7 +421,7 @@
     Raised when a user tries to transition to a new importance who
     doesn't have the necessary permissions.
     """
-    webservice_error(401) # HTTP Error: 'Unauthorised'
+    webservice_error(httplib.UNAUTHORIZED)
 
 
 class UserCannotEditBugTaskMilestone(Unauthorized):
@@ -428,7 +430,7 @@
     Raised when a user tries to transition to a milestone who doesn't have
     the necessary permissions.
     """
-    webservice_error(401) # HTTP Error: 'Unauthorised'
+    webservice_error(httplib.UNAUTHORIZED)
 
 
 class UserCannotEditBugTaskAssignee(Unauthorized):
@@ -437,18 +439,18 @@
     Raised when a user with insufficient prilieges tries to set
     the assignee of a bug task.
     """
-    webservice_error(401) # HTTP Error: 'Unauthorised'
+    webservice_error(httplib.UNAUTHORIZED)
 
 
 class IllegalTarget(Exception):
     """Exception raised when trying to set an illegal bug task target."""
-    webservice_error(400) #Bad request.
+    webservice_error(httplib.BAD_REQUEST)
 
 
 class IllegalRelatedBugTasksParams(Exception):
     """Exception raised when trying to overwrite all relevant parameters
     in a search for related bug tasks"""
-    webservice_error(400) #Bad request.
+    webservice_error(httplib.BAD_REQUEST)
 
 
 class IBugTask(IHasDateCreated, IHasBug):

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2011-02-23 22:46:39 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2011-03-03 15:25:28 +0000
@@ -18,6 +18,8 @@
     )
 from zope.component import getMultiAdapter
 
+from lazr.restfulclient.errors import BadRequest
+
 from canonical.launchpad.ftests import (
     login,
     logout,
@@ -31,10 +33,18 @@
     )
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
-from lp.testing import (
-    launchpadlib_for,
-    TestCaseWithFactory,
-    )
+<<<<<<< TREE
+from lp.testing import (
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
+=======
+from lp.testing import (
+    api_url,
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
+>>>>>>> MERGE-SOURCE
 from lp.testing.matchers import HasQueryCount
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
@@ -263,21 +273,21 @@
         self.message2 = self.factory.makeMessage(parent=self.message1)
         # Only link message2 to the bug.
         self.bug.linkMessage(self.message2)
-        self.webservice = LaunchpadWebServiceCaller(
-            'launchpad-library', 'salgado-change-anything')
+        self.webservice = launchpadlib_for('launchpad-library', 'salgado')
 
     def test_messages(self):
         # When one of the messages on a bug is linked to a parent that
-        # isn't linked to the bug, the webservice should still return
-        # the correct collection link for the bug's messages.
-        response = self.webservice.get('/bugs/%d/messages' % self.bug.id)
-        self.failUnlessEqual(response.status, 200)
+        # isn't linked to the bug, the webservice should still include
+        # that message in the bug's associated messages.
+        bug = self.webservice.load(api_url(self.bug))
+        messages = bug.messages
+        latest_message = [message for message in messages][-1]
+        self.failUnlessEqual(self.message2.subject, latest_message.subject)
+
         # The parent_link for the latest message should be None
         # because the parent is not a member of this bug's messages
         # collection itself.
-        latest_message = response.jsonBody()['entries'][-1]
-        self.failUnlessEqual(self.message2.subject, latest_message['subject'])
-        self.failUnlessEqual(None, latest_message['parent_link'])
+        self.failUnlessEqual(None, latest_message.parent)
 
 
 class TestPostBugWithLargeCollections(TestCaseWithFactory):
@@ -310,19 +320,46 @@
     def test_many_subscribers(self):
         # Many subscriptions do not cause an OOPS for IBug POSTs.
         bug = self.factory.makeBug()
-        webservice = LaunchpadWebServiceCaller(
-            'launchpad-library', 'salgado-change-anything')
+
         real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
         snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
+
+        webservice = launchpadlib_for('test', 'salgado')
         try:
             login(ADMIN_EMAIL)
             for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
                 person = self.factory.makePerson()
                 bug.subscribe(person, person)
             logout()
-            response = webservice.named_post(
-                '/bugs/%d' % bug.id, 'subscribe',
-                person='http://api.launchpad.dev/beta/~name12')
-            self.failUnlessEqual(200, response.status)
+            lp_bug = webservice.load(api_url(bug))
+
+            # Adding one more subscriber through the web service
+            # doesn't cause an OOPS.
+            person_to_subscribe = webservice.load('/~name12')
+            lp_bug.subscribe(person=person_to_subscribe)
         finally:
             snapshot.HARD_LIMIT_FOR_SNAPSHOT = real_hard_limit_for_snapshot
+
+
+class TestEditConjoinedBugtask(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_edit_conjoined_bugtask_gives_bad_request_error(self):
+        # Create the conjoined bugtask.
+        bug = self.factory.makeBug()
+        product = self.factory.makeProduct()
+        conjoined_bugtask = self.factory.makeBugTask(
+            bug=bug, target=product.development_focus)
+
+        # Try to edit it through the web service.
+        launchpad = launchpadlib_for('test', bug.owner)
+        lp_task = launchpad.load(api_url(conjoined_bugtask))
+        lp_task.status = 'Invalid'
+        try:
+            lp_task.lp_save()
+        except BadRequest, e:
+            self.assertEquals(
+                e.contents,
+                "This task cannot be edited directly, it should be edited "
+                "through its conjoined_master.")

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-02-25 02:08:52 +0000
+++ lib/lp/testing/__init__.py	2011-03-03 15:25:28 +0000
@@ -9,6 +9,7 @@
 __all__ = [
     'ANONYMOUS',
     'anonymous_logged_in',
+    'api_url',
     'build_yui_unittest_suite',
     'BrowserTestCase',
     'capture_events',
@@ -152,6 +153,7 @@
 # XXX: JonathanLange 2010-01-01: Why?!
 from lp.testing._tales import test_tales
 from lp.testing._webservice import (
+    api_url,
     launchpadlib_credentials_for,
     launchpadlib_for,
     oauth_access_token_for,

=== modified file 'lib/lp/testing/_webservice.py'
--- lib/lp/testing/_webservice.py	2011-01-28 10:03:12 +0000
+++ lib/lp/testing/_webservice.py	2011-03-03 15:25:28 +0000
@@ -15,6 +15,8 @@
 import shutil
 import tempfile
 
+from lazr.restful.utils import get_current_web_service_request
+
 from launchpadlib.credentials import (
     AccessToken,
     AnonymousAccessToken,
@@ -31,6 +33,7 @@
 from canonical.launchpad.webapp.adapter import get_request_statements
 from canonical.launchpad.webapp.interaction import ANONYMOUS
 from canonical.launchpad.webapp.interfaces import OAuthPermission
+from canonical.launchpad.webapp.publisher import canonical_url
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing._login import (
     login,
@@ -38,6 +41,19 @@
     )
 
 
+def api_url(obj):
+    """Find the web service URL of a data model object.
+
+    This makes it easy to load up the factory object you just created
+    in launchpadlib.
+
+    :param: Which web service version to use.
+
+    :return: A relative URL suitable for passing into Launchpad.load().
+    """
+    return canonical_url(obj, force_local_path=True)
+
+
 def oauth_access_token_for(consumer_name, person, permission, context=None):
     """Find or create an OAuth access token for the given person.
     :param consumer_name: An OAuth consumer name.
@@ -121,7 +137,7 @@
 
 def launchpadlib_for(
     consumer_name, person=None, permission=OAuthPermission.WRITE_PRIVATE,
-    context=None, version=None, service_root="http://api.launchpad.dev/";):
+    context=None, version="devel", service_root="http://api.launchpad.dev/";):
     """Create a Launchpad object for the given person.
 
     :param consumer_name: An OAuth consumer name.
@@ -142,7 +158,6 @@
         credentials = launchpadlib_credentials_for(
             consumer_name, person, permission, context)
     transaction.commit()
-    version = version or Launchpad.DEFAULT_VERSION
     cache = tempfile.mkdtemp(prefix='launchpadlib-cache-')
     zope.testing.cleanup.addCleanUp(_clean_up_cache, (cache,))
     return Launchpad(credentials, None, None, service_root=service_root,

=== modified file 'versions.cfg'
--- versions.cfg	2011-03-01 10:33:03 +0000
+++ versions.cfg	2011-03-03 15:25:28 +0000
@@ -34,7 +34,7 @@
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2
 lazr.lifecycle = 1.1
-lazr.restful = 0.17.1
+lazr.restful = 0.17.2
 lazr.restfulclient = 0.11.2
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1