launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/private-teams-subscribe-to-blueprints-827902 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #827902 in Launchpad itself: "Private teams not able to subscribe to bugs or blueprints for email updates"
  https://bugs.launchpad.net/launchpad/+bug/827902

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-teams-subscribe-to-blueprints-827902/+merge/85609

== Implementation ==

Private teams can subscribe to blueprints. Changes:
1. change vocab/validation on SpecificationSubscription.person field to allow private teams
2. allow private teams in subscribers list to be rendered by precaching launchpad.LimitedView permission as was done for bugs and branches

== Tests ==

Add new test case TestSpecificationViewPrivateArtifacts: 
- test_anonymous_view_specification_with_private_subscriber
- test_view_specification_with_private_subscribe

The above tests subscribe private teams and check the portlet rendering.
There were no existing tests that I could find which tested that subscribing private teams is not allowed.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/browser/specificationsubscription.py
  lib/lp/blueprints/browser/tests/test_specification.py
  lib/lp/blueprints/interfaces/specificationsubscription.py
  lib/lp/blueprints/model/specificationsubscription.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-teams-subscribe-to-blueprints-827902/+merge/85609
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-teams-subscribe-to-blueprints-827902 into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/specificationsubscription.py'
--- lib/lp/blueprints/browser/specificationsubscription.py	2011-06-21 06:40:06 +0000
+++ lib/lp/blueprints/browser/specificationsubscription.py	2011-12-14 08:15:30 +0000
@@ -19,7 +19,9 @@
 from canonical.launchpad.webapp import (
     canonical_url,
     )
-from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.authorization import (
+    precache_permission_for_objects,
+    )
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.publisher import LaunchpadView
 from lp.app.browser.launchpadform import (
@@ -149,15 +151,18 @@
         """
         can_unsubscribe = []
         cannot_unsubscribe = []
+        subscribers = []
         for subscription in self.context.subscriptions:
-            if not check_permission('launchpad.View', subscription.person):
-                continue
+            subscribers.append(subscription.person)
             if subscription.person == self.user:
                 can_unsubscribe = [subscription] + can_unsubscribe
             elif subscription.canBeUnsubscribedByUser(self.user):
                 can_unsubscribe.append(subscription)
             else:
                 cannot_unsubscribe.append(subscription)
+        # Cache permission so private subscribers can be viewed.
+        precache_permission_for_objects(
+                    self.request, 'launchpad.LimitedView', subscribers)
 
         sorted_subscriptions = can_unsubscribe + cannot_unsubscribe
         return sorted_subscriptions

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2011-11-18 16:25:47 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2011-12-14 08:15:30 +0000
@@ -4,18 +4,21 @@
 __metaclass__ = type
 
 from datetime import datetime
-import doctest
 import unittest
 
+from BeautifulSoup import BeautifulSoup
 import pytz
 from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.testing.pages import (
     extract_text,
     find_tag_by_id,
+    setupBrowser,
+    setupBrowserForUser,
     )
 from canonical.launchpad.webapp.interfaces import BrowserNotificationLevel
 from canonical.testing.layers import DatabaseFunctionalLayer
@@ -26,9 +29,12 @@
     ISpecification,
     ISpecificationSet,
     )
+from lp.registry.interfaces.person import PersonVisibility
 from lp.testing import (
+    BrowserTestCase,
     FakeLaunchpadRequest,
     login_person,
+    logout,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -159,6 +165,64 @@
                 "... Registered by Some Person ... ago ..."))
 
 
+class TestSpecificationViewPrivateArtifacts(BrowserTestCase):
+    """ Tests that specifications with private team artifacts can be viewed.
+
+    A Specification may be associated with a private team as follows:
+    - a subscriber is a private team
+
+    A logged in user who is not authorised to see the private team(s) still
+    needs to be able to view the specification. The private team will be
+    rendered in the normal way, displaying the team name and Launchpad URL.
+    """
+
+    layer = DatabaseFunctionalLayer
+
+    def _getBrowser(self, user=None):
+        if user is None:
+            browser = setupBrowser()
+            logout()
+            return browser
+        else:
+            login_person(user)
+        return setupBrowserForUser(user=user)
+
+    def test_view_specification_with_private_subscriber(self):
+        # A specification with a private subscriber is rendered.
+        private_subscriber = self.factory.makeTeam(
+            name="privateteam",
+            visibility=PersonVisibility.PRIVATE)
+        spec = self.factory.makeSpecification()
+        with person_logged_in(spec.owner):
+            spec.subscribe(private_subscriber, spec.owner)
+            # Ensure the specification subscriber is rendered.
+            url = canonical_url(spec, rootsite='blueprints')
+            user = self.factory.makePerson()
+            browser = self._getBrowser(user)
+            browser.open(url)
+            soup = BeautifulSoup(browser.contents)
+            subscriber_portlet = soup.find(
+                'div', attrs={'id': 'subscribers'})
+            self.assertIsNotNone(
+                subscriber_portlet.find('a', text='Privateteam'))
+
+    def test_anonymous_view_specification_with_private_subscriber(self):
+        # A specification with a private subscriber is not rendered for anon.
+        private_subscriber = self.factory.makeTeam(
+            name="privateteam",
+            visibility=PersonVisibility.PRIVATE)
+        spec = self.factory.makeSpecification()
+        with person_logged_in(spec.owner):
+            spec.subscribe(private_subscriber, spec.owner)
+            # Viewing the specification doesn't display private subscriber.
+            url = canonical_url(spec, rootsite='blueprints')
+            browser = self._getBrowser()
+            browser.open(url)
+            soup = BeautifulSoup(browser.contents)
+            self.assertIsNone(
+                soup.find('div', attrs={'id': 'subscriber-privateteam'}))
+
+
 class TestSpecificationEditStatusView(TestCaseWithFactory):
     """Test the SpecificationEditStatusView."""
 

=== modified file 'lib/lp/blueprints/interfaces/specificationsubscription.py'
--- lib/lp/blueprints/interfaces/specificationsubscription.py	2011-06-08 13:25:43 +0000
+++ lib/lp/blueprints/interfaces/specificationsubscription.py	2011-12-14 08:15:30 +0000
@@ -28,7 +28,7 @@
     )
 
 from canonical.launchpad import _
-from lp.services.fields import PublicPersonChoice
+from lp.services.fields import PersonChoice
 
 
 class ISpecificationSubscription(Interface):
@@ -38,9 +38,9 @@
 
     id = Int(
         title=_('ID'), required=True, readonly=True)
-    person = PublicPersonChoice(
+    person = PersonChoice(
             title=_('Subscriber'), required=True,
-            vocabulary='ValidPersonOrTeam', readonly=True,
+            vocabulary='ValidPerson', readonly=True,
             description=_(
             'The person you would like to subscribe to this blueprint. '
             'They will be notified of the subscription by e-mail.')

=== modified file 'lib/lp/blueprints/model/specificationsubscription.py'
--- lib/lp/blueprints/model/specificationsubscription.py	2011-06-08 13:25:43 +0000
+++ lib/lp/blueprints/model/specificationsubscription.py	2011-12-14 08:15:30 +0000
@@ -17,7 +17,7 @@
 from lp.blueprints.interfaces.specificationsubscription import (
     ISpecificationSubscription,
     )
-from lp.registry.interfaces.person import validate_public_person
+from lp.registry.interfaces.person import validate_person
 
 
 class SpecificationSubscription(SQLBase):
@@ -30,7 +30,7 @@
         foreignKey='Specification', notNull=True)
     person = ForeignKey(
         dbName='person', foreignKey='Person',
-        storm_validator=validate_public_person, notNull=True)
+        storm_validator=validate_person, notNull=True)
     essential = BoolCol(notNull=True, default=False)
 
     def canBeUnsubscribedByUser(self, user):