launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/redirect-unsub-private-branch into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #274204 in Launchpad itself: "attempting to unsubscribe yourself from a private branch reports 'not allowed here'"
  https://bugs.launchpad.net/launchpad/+bug/274204

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/redirect-unsub-private-branch/+merge/118859

If the person unsubscribing themselves from a branch no longer has access to it, redirect them to branch's target canonical_url. Add two tests for this case -- one where the subscriber has an APG, so doesn't lose access, and one where the subscriber does not, and so does lose access.
-- 
https://code.launchpad.net/~stevenk/launchpad/redirect-unsub-private-branch/+merge/118859
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/redirect-unsub-private-branch into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2012-06-21 23:00:20 +0000
+++ lib/lp/code/browser/branchsubscription.py	2012-08-09 02:14:18 +0000
@@ -13,6 +13,7 @@
     ]
 
 from lazr.restful.utils import smartquote
+from zope.component import getUtility
 from zope.interface import implements
 
 from lp.app.browser.launchpadform import (
@@ -20,6 +21,7 @@
     LaunchpadEditFormView,
     LaunchpadFormView,
     )
+from lp.app.interfaces.services import IService
 from lp.code.enums import BranchSubscriptionNotificationLevel
 from lp.code.interfaces.branchsubscription import IBranchSubscription
 from lp.services.webapp import (
@@ -289,6 +291,13 @@
 
     @property
     def next_url(self):
-        return canonical_url(self.branch)
+        url = canonical_url(self.branch)
+        # If the subscriber can no longer see the branch, redirect them away.
+        service = getUtility(IService, 'sharing')
+        ignored, branches = service.getVisibleArtifacts(
+            self.person, branches=[self.branch])
+        if not branches:
+            url = canonical_url(self.branch.target)
+        return url
 
     cancel_url = next_url

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-07-27 03:51:46 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-08-09 02:14:18 +0000
@@ -36,6 +36,7 @@
     BranchVisibilityRule,
     )
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import PersonVisibility
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
@@ -653,6 +654,52 @@
         reviews_list = soup.find('dl', attrs={'class': 'reviews'})
         self.assertIsNone(reviews_list.find('a', text='Privateteam'))
 
+    def test_unsubscribe_private_branch(self):
+        # Unsubscribing from a branch with a policy grant still allows the
+        # branch to be seen.
+        product = self.factory.makeProduct()
+        owner = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        [ap] = getUtility(IAccessPolicySource).find(
+            [(product, InformationType.USERDATA)])
+        self.factory.makeAccessPolicyGrant(
+            policy=ap, grantee=subscriber, grantor=product.owner)
+        branch = self.factory.makeBranch(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        with person_logged_in(owner):
+            self.factory.makeBranchSubscription(branch, subscriber, owner)
+            base_url = canonical_url(branch, rootsite='code')
+            expected_title = '%s : Code : %s' % (
+                branch.name, product.displayname)
+        url = '%s/+subscription/%s' % (base_url, subscriber.name)
+        browser = self._getBrowser(user=subscriber)
+        browser.open(url)
+        browser.getControl('Unsubscribe').click()
+        self.assertEqual(base_url, browser.url)
+        self.assertEqual(expected_title, browser.title)
+
+    def test_unsubscribe_private_branch_no_access(self):
+        # Unsubscribing from a branch with no access will redirect to the
+        # context of the branch.
+        product = self.factory.makeProduct()
+        owner = self.factory.makePerson()
+        subscriber = self.factory.makePerson()
+        branch = self.factory.makeBranch(
+            product=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        with person_logged_in(owner):
+            self.factory.makeBranchSubscription(branch, subscriber, owner)
+            base_url = canonical_url(branch, rootsite='code')
+            product_url = canonical_url(product, rootsite='code')
+        url = '%s/+subscription/%s' % (base_url, subscriber.name)
+        browser = self._getBrowser(user=subscriber)
+        browser.open(url)
+        browser.getControl('Unsubscribe').click()
+        self.assertEqual(product_url, browser.url)
+        expected_title = "Code : %s" % product.displayname
+        self.assertEqual(expected_title, browser.title)
+
 
 class TestBranchReviewerEditView(TestCaseWithFactory):
     """Test the BranchReviewerEditView view."""