launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/private-branch-unsubscribe-283167 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #283167 in Launchpad itself: "Private branch owner cannot unsubscribe someone"
  https://bugs.launchpad.net/launchpad/+bug/283167

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-branch-unsubscribe-283167/+merge/67651

== Implementation ==

The bug report says that owners of private branches should be able to edit subscriptions to the branch. This mp does that, but for public branches too.

If the branch owner is a team, then the team owner can edit the subscriptions, but an arbitrary member of the owning team cannot.

== Tests ==

Added new tests to TestBranchSubscriptionCanBeUnsubscribedbyUser:
  test_branch_person_owner_can_unsubscribe
  test_branch_team_owner_can_unsubscribe

== Lint ==
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/security.py
  lib/lp/code/model/tests/test_branchsubscription.py
  lib/lp/testing/factory.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-branch-unsubscribe-283167/+merge/67651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-branch-unsubscribe-283167 into lp:launchpad.

=== modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
--- lib/lp/code/model/tests/test_branchsubscription.py	2010-10-26 15:47:24 +0000
+++ lib/lp/code/model/tests/test_branchsubscription.py	2011-07-12 01:51:12 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-import unittest
-
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.errors import UserCannotUnsubscribePerson
 from lp.code.enums import (
@@ -119,6 +117,31 @@
             person=team, subscribed_by=subscribed_by)
         self.assertTrue(subscription.canBeUnsubscribedByUser(subscribed_by))
 
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    def test_branch_person_owner_can_unsubscribe(self):
+        """Branch owner can unsubscribe someone from a branch."""
+        branch_owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=branch_owner)
+        subscribed_by = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        subscription = self.factory.makeBranchSubscription(
+            branch=branch, person=subscriber, subscribed_by=subscribed_by)
+        self.assertTrue(subscription.canBeUnsubscribedByUser(branch_owner))
+
+    def test_branch_team_owner_can_unsubscribe(self):
+        """Branch team owner can unsubscribe someone from a branch.
+
+        If the owner of a branch is a team, then the team owner can
+        unsubscribe someone, but an arbitrary team member cannot.
+        """
+
+        team_owner = self.factory.makePerson()
+        team_member = self.factory.makePerson()
+        branch_owner = self.factory.makeTeam(
+            owner=team_owner, members=[team_member])
+        branch = self.factory.makeBranch(owner=branch_owner)
+        subscribed_by = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        subscription = self.factory.makeBranchSubscription(
+            branch=branch, person=subscriber, subscribed_by=subscribed_by)
+        self.assertTrue(subscription.canBeUnsubscribedByUser(team_owner))
+        self.assertFalse(subscription.canBeUnsubscribedByUser(team_member))

=== modified file 'lib/lp/code/security.py'
--- lib/lp/code/security.py	2011-04-27 16:14:32 +0000
+++ lib/lp/code/security.py	2011-07-12 01:51:12 +0000
@@ -22,8 +22,15 @@
 
         Any team member can edit a branch subscription for their team.
         Launchpad Admins can also edit any branch subscription.
+        The owner of the subscribed branch can edit the subscription. If the
+        branch owner is a team, then the owner of the team (but not arbitrary
+        team members)can edit the subscription.
         """
-        return (user.inTeam(self.obj.person) or
+        branch_owner = self.obj.branch.owner
+        if branch_owner.is_team:
+            branch_owner = branch_owner.teamowner
+        return (user.inTeam(branch_owner) or
+                user.inTeam(self.obj.person) or
                 user.inTeam(self.obj.subscribed_by) or
                 user.in_admin or
                 user.in_bazaar_experts)
@@ -31,5 +38,3 @@
 
 class BranchSubscriptionView(BranchSubscriptionEdit):
     permission = 'launchpad.View'
-
-

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-07-07 15:47:36 +0000
+++ lib/lp/testing/factory.py	2011-07-12 01:51:12 +0000
@@ -1486,11 +1486,7 @@
 
     def makeBranchSubscription(self, branch=None, person=None,
                                subscribed_by=None):
-        """Create a BranchSubscription.
-
-        :param branch_title: The title to use for the created Branch
-        :param person_displayname: The displayname for the created Person
-        """
+        """Create a BranchSubscription."""
         if branch is None:
             branch = self.makeBranch()
         if person is None: