launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/fix-branch-subscribe-open-team into lp:launchpad.

Requested reviews:
  Launchpad code reviewers from Canonical (canonical-launchpad-reviewers): code
Related bugs:
  Bug #933768 in Launchpad itself: "Update branch to use information_visibility_policy"
  https://bugs.launchpad.net/launchpad/+bug/933768

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/fix-branch-subscribe-open-team/+merge/111326

This branch is a follow-up to some of the behaviour introduced in https://code.launchpad.net/~stevenk/launchpad/branch-subscribe-aag/+merge/108881. That branch changed Branch.subscribe() to raise an exception if an open or delegated team was trying to be subscribed to a private branch. I had changed the browser code to catch the exception and call setFieldError(), but since next_url was unilaterally set, Zope happily redirected back to Branch:+index, and it looked like nothing happened.

I have removed most of that code and made use of a validate() method, which is the proper pattern.
-- 
https://code.launchpad.net/~stevenk/launchpad/fix-branch-subscribe-open-team/+merge/111326
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2012-06-06 05:58:44 +0000
+++ lib/lp/code/browser/branchsubscription.py	2012-06-21 01:44:12 +0000
@@ -20,7 +20,6 @@
     LaunchpadEditFormView,
     LaunchpadFormView,
     )
-from lp.app.errors import SubscriptionPrivacyViolation
 from lp.code.enums import BranchSubscriptionNotificationLevel
 from lp.code.interfaces.branchsubscription import IBranchSubscription
 from lp.services.webapp import (
@@ -212,6 +211,14 @@
 
     page_title = label = "Subscribe to branch"
 
+    def validate(self, data):
+        person = data['person']
+        subscription = self.context.getSubscription(person)
+        if (subscription is None and person.is_team and 
+            person.anyone_can_join()):
+            self.setFieldError('person', "Open and delegated teams cannot "
+                "be subscribed to private branches.")
+
     @action("Subscribe", name="subscribe_action")
     def subscribe_action(self, action, data):
         """Subscribe the specified user to the branch.
@@ -227,17 +234,13 @@
         person = data['person']
         subscription = self.context.getSubscription(person)
         if subscription is None:
-            try:
-                self.context.subscribe(
-                    person, notification_level, max_diff_lines, review_level,
-                    self.user)
-            except SubscriptionPrivacyViolation as error:
-                self.setFieldError('person', unicode(error))
-            else:
-                self.add_notification_message(
-                    '%s has been subscribed to this branch with: '
-                    % person.displayname, notification_level, max_diff_lines,
-                    review_level)
+            self.context.subscribe(
+                person, notification_level, max_diff_lines, review_level,
+                self.user)
+            self.add_notification_message(
+                '%s has been subscribed to this branch with: '
+                % person.displayname, notification_level, max_diff_lines,
+                review_level)
         else:
             self.add_notification_message(
                 '%s was already subscribed to this branch with: '