launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-660141 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #660141 in Launchpad itself: "OOPS when trying to reassign branch non-private team when branches are private for another team"
  https://bugs.launchpad.net/launchpad/+bug/660141

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-660141/+merge/61059

moveBranch can raise BranchCreationForbidden if the target's visibility policy forbids the new owner from having branches. BranchEditView wasn't catching that, so it OOPSed when such a change was attempted.

This branch makes it display a nice error message instead.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-660141/+merge/61059
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-660141 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-05-05 05:29:34 +0000
+++ lib/lp/code/browser/branch.py	2011-05-16 04:01:07 +0000
@@ -1078,6 +1078,10 @@
                 try:
                     namespace.validateMove(
                         self.context, self.user, name=data['name'])
+                except BranchCreationForbidden:
+                    self.addError(
+                        "%s is not allowed to own branches in %s." % (
+                        owner.displayname, self.context.target.displayname))
                 except BranchExists, e:
                     self._setBranchExists(e.existing_branch)
 

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2011-03-04 11:02:47 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-05-16 04:01:07 +0000
@@ -46,6 +46,7 @@
 from lp.code.enums import (
     BranchLifecycleStatus,
     BranchType,
+    BranchVisibilityRule,
     )
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.testing import (
@@ -56,7 +57,10 @@
     person_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.matchers import BrowsesWithQueryLimit
+from lp.testing.matchers import (
+    BrowsesWithQueryLimit,
+    Contains,
+    )
 from lp.testing.views import create_initialized_view
 
 
@@ -844,3 +848,50 @@
         branch = self.factory.makeProductBranch()
         root_context = IRootContext(branch)
         self.assertEqual(branch.product, root_context)
+
+
+class TestBranchEditView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_allowed_owner_is_ok(self):
+        # A branch's owner can be changed to a team permitted by the
+        # visibility policy.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+        team = self.factory.makeTeam(
+            owner=person, displayname="Permitted team")
+        branch.product.setBranchVisibilityTeamPolicy(
+            None, BranchVisibilityRule.FORBIDDEN)
+        branch.product.setBranchVisibilityTeamPolicy(
+            team, BranchVisibilityRule.PRIVATE)
+        browser = self.getUserBrowser(
+            canonical_url(branch) + '/+edit', user=person)
+        browser.getControl("Owner").displayValue = ["Permitted team"]
+        browser.getControl("Change Branch").click()
+        with person_logged_in(person):
+            self.assertEquals(team, branch.owner)
+
+    def test_forbidden_owner_is_error(self):
+        # An error is displayed if a branch's owner is changed to
+        # a value forbidden by the visibility policy.
+        product = self.factory.makeProduct(displayname='Some Product')
+        person = self.factory.makePerson()
+        branch = self.factory.makeBranch(product=product, owner=person)
+        self.factory.makeTeam(
+            owner=person, displayname="Forbidden team")
+        branch.product.setBranchVisibilityTeamPolicy(
+            None, BranchVisibilityRule.FORBIDDEN)
+        branch.product.setBranchVisibilityTeamPolicy(
+            person, BranchVisibilityRule.PRIVATE)
+        browser = self.getUserBrowser(
+            canonical_url(branch) + '/+edit', user=person)
+        browser.getControl("Owner").displayValue = ["Forbidden team"]
+        browser.getControl("Change Branch").click()
+        self.assertThat(
+            browser.contents,
+            Contains(
+                'Forbidden team is not allowed to own branches in '
+                'Some Product.'))
+        with person_logged_in(person):
+            self.assertEquals(person, branch.owner)