launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/fix-branch-subscription-for-public into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1100061 in Launchpad itself: "BranchSubscriptionAddOtherView rejects open teams even for public branches"
  https://bugs.launchpad.net/launchpad/+bug/1100061

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/fix-branch-subscription-for-public/+merge/143441

As a fix for an OOPS, the BranchSubscriptionAddOtherView view was changed so it would check if an open team was subscribing to a private branch, so it could show a proper error message -- however, it did this in a buggy way, and didn't actually check if the branch was private, thereby denying subscriptions using that view for open teams.

I have refactored the check out into a model method so both places can make use of it.
-- 
https://code.launchpad.net/~stevenk/launchpad/fix-branch-subscription-for-public/+merge/143441
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/fix-branch-subscription-for-public into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2012-11-26 08:40:20 +0000
+++ lib/lp/code/browser/branchsubscription.py	2013-01-16 06:49:22 +0000
@@ -217,8 +217,8 @@
         if data.has_key('person'):
             person = data['person']
             subscription = self.context.getSubscription(person)
-            if (subscription is None and person.is_team and 
-                person.anyone_can_join()):
+            if subscription is None and not self.context.userCanBeSubscribed(
+                person):
                 self.setFieldError('person', "Open and delegated teams "
                 "cannot be subscribed to private branches.")
 

=== modified file 'lib/lp/code/browser/tests/test_branchsubscription.py'
--- lib/lp/code/browser/tests/test_branchsubscription.py	2012-09-18 18:36:09 +0000
+++ lib/lp/code/browser/tests/test_branchsubscription.py	2013-01-16 06:49:22 +0000
@@ -50,3 +50,18 @@
             self.assertContentEqual(
                 ['Open and delegated teams cannot be subscribed to private '
                 'branches.'], view.errors)
+
+    def test_can_subscribe_open_team_to_public_branch(self):
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=owner)
+        team = self.factory.makeTeam()
+        form = {
+            'field.person': team.name,
+            'field.notification_level': 'NOEMAIL',
+            'field.max_diff_lines': 'NODIFF',
+            'field.review_level': 'NOEMAIL',
+            'field.actions.subscribe_action': 'Subscribe'}
+        with person_logged_in(owner):
+            view = create_initialized_view(
+                branch, '+addsubscriber', pricipal=owner, form=form)
+            self.assertContentEqual([], view.errors)

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2013-01-07 02:40:55 +0000
+++ lib/lp/code/interfaces/branch.py	2013-01-16 06:49:22 +0000
@@ -786,6 +786,9 @@
         """
 
     # subscription-related methods
+    def userCanBeSubscribed(person):
+        """Return if the `IPerson` can be subscribed to the branch."""
+
     @operation_parameters(
         person=Reference(
             title=_("The person to subscribe."),

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-10-08 15:53:56 +0000
+++ lib/lp/code/model/branch.py	2013-01-16 06:49:22 +0000
@@ -870,6 +870,10 @@
         """See `IBranch`."""
         return self._associatedSuiteSourcePackages
 
+    def userCanBeSubscribed(self, person):
+        return not (person.is_team and self.information_type in
+            PRIVATE_INFORMATION_TYPES and person.anyone_can_join())
+
     # subscriptions
     def subscribe(self, person, notification_level, max_diff_lines,
                   code_review_level, subscribed_by,
@@ -879,8 +883,7 @@
         Subscribe person to this branch and also to any editable stacked on
         branches they cannot see.
         """
-        if (person.is_team and self.information_type in
-            PRIVATE_INFORMATION_TYPES and person.anyone_can_join()):
+        if not self.userCanBeSubscribed(person):
             raise SubscriptionPrivacyViolation(
                 "Open and delegated teams cannot be subscribed to private "
                 "branches.")