launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/branch-setOwner-oops-628352 into lp:launchpad.

Commit message:
Ensure that renaming a branch via the web service passes without raising a 404.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #628352 in Launchpad itself: "Calling setOwner on a branch via web API OOPses even though operation is successful"
  https://bugs.launchpad.net/launchpad/+bug/628352

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branch-setOwner-oops-628352/+merge/126376

== Implementation ==

This branch relies on an updated lazr.restfulclient as per lp:~wallyworld/lazr.restfulclient/named-post-url-change-1056546

When a branch move (ie new target or new owner) is performed as an API call, poke the url of the newly renamed branch into the request response header. This allows the web service client to update the url of the branch resource object.

== Tests ==

Add a new setOwner test to test_branch_webservice TestBranchOperations.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  versions.cfg
  lib/lp/code/model/branch.py
  lib/lp/code/tests/test_branch_webservice.py

./lib/lp/code/tests/test_branch_webservice.py
      30: E501 line too long (95 characters)
-- 
https://code.launchpad.net/~wallyworld/launchpad/branch-setOwner-oops-628352/+merge/126376
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branch-setOwner-oops-628352 into lp:launchpad.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-09-21 04:04:51 +0000
+++ lib/lp/code/model/branch.py	2012-09-26 04:35:26 +0000
@@ -52,6 +52,8 @@
     removeSecurityProxy,
     )
 
+from lazr.restful.utils import get_current_web_service_request
+
 from lp import _
 from lp.app.enums import (
     InformationType,
@@ -180,7 +182,10 @@
 from lp.services.job.model.job import Job
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
 from lp.services.propertycache import cachedproperty
-from lp.services.webapp import urlappend
+from lp.services.webapp import (
+    canonical_url,
+    urlappend,
+    )
 from lp.services.webapp.authorization import check_permission
 
 
@@ -237,6 +242,17 @@
         reconcile_access_for_artifact(
             self, self.information_type, pillars, wanted_links)
 
+    def _move(self, namespace, mover, rename_if_necessary=False):
+        namespace.moveBranch(
+            self, mover, rename_if_necessary=rename_if_necessary)
+        # If this is from an API request, we need to ensure the response
+        # contains the location of the new URL.
+        api_request = get_current_web_service_request()
+        if api_request:
+            update_trigger_modified_fields(self)
+            api_request.response.setHeader(
+                'Location', canonical_url(self, request=api_request))
+
     def setPrivate(self, private, user):
         """See `IBranch`."""
         if private:
@@ -294,7 +310,7 @@
     def setOwner(self, new_owner, user):
         """See `IBranch`."""
         new_namespace = self.target.getNamespace(new_owner)
-        new_namespace.moveBranch(self, user, rename_if_necessary=True)
+        self._move(new_namespace, user, rename_if_necessary=True)
 
     reviewer = ForeignKey(
         dbName='reviewer', foreignKey='Person',
@@ -375,7 +391,7 @@
             raise BranchTargetError(
                 '%s branches are not allowed for target %s.' % (
                     self.information_type.title, target.displayname))
-        namespace.moveBranch(self, user, rename_if_necessary=True)
+        self._move(namespace, user, rename_if_necessary=True)
         self._reconcileAccess()
 
     @property

=== modified file 'lib/lp/code/tests/test_branch_webservice.py'
--- lib/lp/code/tests/test_branch_webservice.py	2012-09-18 19:41:02 +0000
+++ lib/lp/code/tests/test_branch_webservice.py	2012-09-26 04:35:26 +0000
@@ -84,6 +84,25 @@
             exception.content,
             'Source and target branches must be different.')
 
+    def test_setOwner(self):
+        """Test setOwner via the web API does not raise a 404."""
+        branch_owner = self.factory.makePerson(name='fred')
+        product = self.factory.makeProduct(name='myproduct')
+        self.factory.makeProductBranch(
+            name='mybranch', product=product, owner=branch_owner)
+        self.factory.makeTeam(name='barney', owner=branch_owner)
+        endInteraction()
+
+        lp = launchpadlib_for("test", person=branch_owner)
+        ws_branch = lp.branches.getByUniqueName(
+            unique_name='~fred/myproduct/mybranch')
+        ws_new_owner = lp.people['barney']
+        ws_branch.setOwner(new_owner=ws_new_owner)
+        # Check the result.
+        renamed_branch = lp.branches.getByUniqueName(
+            unique_name='~barney/myproduct/mybranch')
+        self.assertIsNotNone(renamed_branch)
+
 
 class TestBranchDeletes(TestCaseWithFactory):
 

=== modified file 'versions.cfg'
--- versions.cfg	2012-08-30 09:03:47 +0000
+++ versions.cfg	2012-09-26 04:35:26 +0000
@@ -50,7 +50,7 @@
 lazr.jobrunner = 0.10
 lazr.lifecycle = 1.1
 lazr.restful = 0.19.6
-lazr.restfulclient = 0.13.0
+lazr.restfulclient = 0.13.1
 lazr.smtptest = 1.3
 lazr.testing = 0.1.1
 lazr.uri = 1.0.2