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 (launchpad-reviewers)
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/109960

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 changed how next_url is set up for the parent class of the view in question, and will only set it if the exception wasn't raised.
-- 
https://code.launchpad.net/~stevenk/launchpad/fix-branch-subscribe-open-team/+merge/109960
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/fix-branch-subscribe-open-team into 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-13 00:04:23 +0000
@@ -89,11 +89,9 @@
         # handle the case nicely.
         return self.context.getSubscription(self.user) is not None
 
-    @property
-    def next_url(self):
-        return canonical_url(self.context)
-
-    cancel_url = next_url
+    def set_next_url(self):
+        self.next_url = canonical_url(self.context)
+        self.cancel_url = self.next_url
 
     def add_notification_message(self, initial,
                                  notification_level, max_diff_lines,
@@ -142,6 +140,7 @@
             self.add_notification_message(
                 'You have subscribed to this branch with: ',
                 notification_level, max_diff_lines, review_level)
+        self.set_next_url()
 
 
 class BranchSubscriptionEditOwnView(_BranchSubscriptionView):
@@ -185,6 +184,7 @@
         else:
             self.request.response.addNotification(
                 'You are not subscribed to this branch.')
+        self.set_next_url()
 
     @action("Unsubscribe")
     def unsubscribe(self, action, data):
@@ -196,6 +196,7 @@
         else:
             self.request.response.addNotification(
                 'You are not subscribed to this branch.')
+        self.set_next_url()
 
 
 class BranchSubscriptionAddOtherView(_BranchSubscriptionView):
@@ -226,6 +227,7 @@
         review_level = data['review_level']
         person = data['person']
         subscription = self.context.getSubscription(person)
+        errors = False
         if subscription is None:
             try:
                 self.context.subscribe(
@@ -233,6 +235,7 @@
                     self.user)
             except SubscriptionPrivacyViolation as error:
                 self.setFieldError('person', unicode(error))
+                errors = True
             else:
                 self.add_notification_message(
                     '%s has been subscribed to this branch with: '
@@ -244,6 +247,8 @@
                 % person.displayname,
                 subscription.notification_level, subscription.max_diff_lines,
                 review_level)
+        if not errors:
+            self.set_next_url()
 
 
 class BranchSubscriptionEditView(LaunchpadEditFormView):

=== modified file 'lib/lp/code/browser/tests/test_branchsubscription.py'
--- lib/lp/code/browser/tests/test_branchsubscription.py	2012-06-06 05:58:44 +0000
+++ lib/lp/code/browser/tests/test_branchsubscription.py	2012-06-13 00:04:23 +0000
@@ -50,3 +50,4 @@
             self.assertContentEqual(
                 ['Open and delegated teams cannot be subscribed to private '
                 'branches.'], view.errors)
+            self.assertIs(None, view.next_url)