Merge lp:~stevenk/launchpad/branch-subscribe-aag into lp:launchpad

[Steve Kowalik]

IBranch.subscribe() now forbids open and delegated teams from being subscribed to private branches, and creates an AccessArtifactGrant if the subscriber requires it for access. Secondly, if the reviewer is an open team, they are not subscribed to private branches for merge proposals.

IBranch.subscribe() now forbids open and delegated teams from being subscribed to private branches. This necessitated moving the exception into lp.app from bugs. I have also carried over the test for the webservice, and have written a few tests for the branch case.

I have also added the ability to IBranch.subscribe() to add a AccessArtifactGrant if required when a person is subscribed to a branch and added a test for that behaviour too.

[Ian Booth (wallyworld)]

<wallyworld_> StevenK: TestBranchSubscriptionAddOtherView could iterate over all types of open team and test with each one
<wallyworld_> StevenK: also getVisibleArtifacts - you have the return tuple mixed up
<wallyworld_> StevenK: also, there are no legacy branch triggers so that check isn't needed
<wallyworld_> StevenK: and so the reason the test_subscribe_gives_access test passes is that the branch collection assumes subscribed = visible
<wallyworld_> so that bit of code is sort of pointless atm

review: Needs Fixing

[Ian Booth (wallyworld)]

> <wallyworld_> StevenK: TestBranchSubscriptionAddOtherView could iterate over
> all types of open team and test with each one
> <wallyworld_> StevenK: also getVisibleArtifacts - you have the return tuple
> mixed up
> <wallyworld_> StevenK: also, there are no legacy branch triggers so that check
> isn't needed
> <wallyworld_> StevenK: and so the reason the test_subscribe_gives_access test
> passes is that the branch collection assumes subscribed = visible
> <wallyworld_> so that bit of code is sort of pointless atm

I should clarify - the test will always pass right now, but will be needed for when we uncouple subscription and visibility for branches. So no harm in keeping it.

[Ian Booth (wallyworld)]

320 + # Grant the subscriber access if they can't see the branch (if the
321 + # database triggers aren't going to do it for us).

Fix the above comment and you're good to go.

review: Approve

=== modified file 'lib/lp/app/errors.py'
--- lib/lp/app/errors.py	2011-06-16 20:12:00 +0000
+++ lib/lp/app/errors.py	2012-06-08 06:05:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Cross application type errors for launchpad."""
@@ -9,6 +9,7 @@
     'NameLookupFailed',
     'NotFoundError',
     'POSTToNonCanonicalURL',
+    'SubscriptionPrivacyViolation',
     'TranslationUnavailable',
     'UnexpectedFormData',
     'UserCannotUnsubscribePerson',
@@ -75,5 +76,10 @@
     """User does not have permission to unsubscribe person or team."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class SubscriptionPrivacyViolation(Exception):
+    """The subscription would violate privacy policies."""
+
+
 # Slam a 401 response code onto all ForbiddenAttribute errors.
 error_status(httplib.UNAUTHORIZED)(ForbiddenAttribute)

=== added file 'lib/lp/app/tests/test_errors.py'
--- lib/lp/app/tests/test_errors.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/tests/test_errors.py	2012-06-08 06:05:25 +0000
@@ -0,0 +1,33 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from httplib import (
+    BAD_REQUEST,
+    UNAUTHORIZED,
+    )
+
+from lp.app.errors import (
+    SubscriptionPrivacyViolation,
+    UserCannotUnsubscribePerson,
+    )
+from lp.testing import TestCase
+from lp.testing.layers import FunctionalLayer
+from lp.testing.views import create_webservice_error_view
+
+
+class TestWebServiceErrors(TestCase):
+    """ Test that errors are correctly mapped to HTTP status codes."""
+
+    layer = FunctionalLayer
+
+    def test_UserCannotUnsubscribePerson_unauthorized(self):
+        error_view = create_webservice_error_view(
+            UserCannotUnsubscribePerson())
+        self.assertEqual(UNAUTHORIZED, error_view.status)
+
+    def test_SubscriptionPrivacyViolation_bad_request(self):
+        error_view = create_webservice_error_view(
+            SubscriptionPrivacyViolation())
+        self.assertEqual(BAD_REQUEST, error_view.status)

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2012-03-13 00:45:33 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2012-06-08 06:05:25 +0000
@@ -37,11 +37,11 @@
     LaunchpadFormView,
     ReturnToReferrerMixin,
     )
+from lp.app.errors import SubscriptionPrivacyViolation
 from lp.bugs.browser.structuralsubscription import (
     expose_structural_subscription_data_to_js,
     )
 from lp.bugs.enums import BugNotificationLevel
-from lp.bugs.errors import SubscriptionPrivacyViolation
 from lp.bugs.interfaces.bug import IBug
 from lp.bugs.interfaces.bugsubscription import IBugSubscription
 from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions

=== modified file 'lib/lp/bugs/errors.py'
--- lib/lp/bugs/errors.py	2011-11-25 04:25:00 +0000
+++ lib/lp/bugs/errors.py	2012-06-08 06:05:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Errors used in the lp/bugs modules."""
@@ -8,7 +8,6 @@
     'BugCannotBePrivate',
     'InvalidBugTargetType',
     'InvalidDuplicateValue',
-    'SubscriptionPrivacyViolation',
 ]
 
 import httplib
@@ -29,10 +28,5 @@
 
 
 @error_status(httplib.BAD_REQUEST)
-class SubscriptionPrivacyViolation(Exception):
-    """The subscription would violate privacy policies."""
-
-
-@error_status(httplib.BAD_REQUEST)
 class BugCannotBePrivate(Exception):
     """The bug is not allowed to be private."""

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-06-07 06:04:46 +0000
+++ lib/lp/bugs/model/bug.py	2012-06-08 06:05:25 +0000
@@ -85,6 +85,7 @@
 from lp.app.enums import ServiceUsage
 from lp.app.errors import (
     NotFoundError,
+    SubscriptionPrivacyViolation,
     UserCannotUnsubscribePerson,
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -105,7 +106,6 @@
 from lp.bugs.errors import (
     BugCannotBePrivate,
     InvalidDuplicateValue,
-    SubscriptionPrivacyViolation,
     )
 from lp.bugs.interfaces.bug import (
     IBug,
@@ -177,8 +177,8 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.role import IPersonRoles
+from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sharingjob import IRemoveBugSubscriptionsJobSource
-from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.model.accesspolicy import reconcile_access_for_artifact
 from lp.registry.model.person import (

=== modified file 'lib/lp/bugs/tests/test_errors.py'
--- lib/lp/bugs/tests/test_errors.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_errors.py	2012-06-08 06:05:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for bugs errors."""
@@ -15,7 +15,6 @@
 from lp.bugs.errors import (
     InvalidBugTargetType,
     InvalidDuplicateValue,
-    SubscriptionPrivacyViolation,
     )
 from lp.testing import TestCase
 from lp.testing.layers import FunctionalLayer
@@ -35,8 +34,3 @@
         error_view = create_webservice_error_view(
             InvalidDuplicateValue("Dup"))
         self.assertEqual(EXPECTATION_FAILED, error_view.status)
-
-    def test_SubscriptionPrivacyViolation_bad_request(self):
-        error_view = create_webservice_error_view(
-            SubscriptionPrivacyViolation())
-        self.assertEqual(BAD_REQUEST, error_view.status)

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/branchsubscription.py	2012-06-08 06:05:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -20,6 +20,7 @@
     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 (
@@ -226,14 +227,17 @@
         person = data['person']
         subscription = self.context.getSubscription(person)
         if subscription is None:
-            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)
+            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)
         else:
             self.add_notification_message(
                 '%s was already subscribed to this branch with: '

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-06-02 13:55:16 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-06-08 06:05:25 +0000
@@ -547,7 +547,8 @@
         # A branch with a private owner is rendered.
         private_owner = self.factory.makeTeam(
             displayname="PrivateTeam", visibility=PersonVisibility.PRIVATE)
-        branch = self.factory.makeAnyBranch(owner=private_owner)
+        with person_logged_in(private_owner):
+            branch = self.factory.makeAnyBranch(owner=private_owner)
         # Ensure the branch owner is rendered.
         url = canonical_url(branch, rootsite='code')
         user = self.factory.makePerson()
@@ -560,7 +561,8 @@
         # A private branch with a private owner is rendered.
         private_owner = self.factory.makeTeam(
             displayname="PrivateTeam", visibility=PersonVisibility.PRIVATE)
-        branch = self.factory.makeAnyBranch(owner=private_owner)
+        with person_logged_in(private_owner):
+            branch = self.factory.makeAnyBranch(owner=private_owner)
         # Ensure the branch owner is rendered.
         url = canonical_url(branch, rootsite='code')
         user = self.factory.makePerson()
@@ -576,7 +578,8 @@
         # A branch with a private owner is not rendered for anon users.
         private_owner = self.factory.makeTeam(
             visibility=PersonVisibility.PRIVATE)
-        branch = self.factory.makeAnyBranch(owner=private_owner)
+        with person_logged_in(private_owner):
+            branch = self.factory.makeAnyBranch(owner=private_owner)
         # Viewing the branch results in an error.
         url = canonical_url(branch, rootsite='code')
         browser = self._getBrowser()
@@ -604,7 +607,7 @@
         private_subscriber = self.factory.makeTeam(
             name="privateteam", visibility=PersonVisibility.PRIVATE)
         branch = self.factory.makeAnyBranch()
-        with person_logged_in(branch.owner):
+        with person_logged_in(private_subscriber):
             self.factory.makeBranchSubscription(
                 branch, private_subscriber, branch.owner)
         # Viewing the branch doesn't show the private subscriber.

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2012-02-21 12:18:30 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2012-06-08 06:05:25 +0000
@@ -657,7 +657,7 @@
         member = self.factory.makePerson(email='[e-mail]')
         with person_logged_in(owner):
             private_team.addMember(member, owner)
-        branch = self.factory.makeProductBranch(owner=private_team)
+            branch = self.factory.makeProductBranch(owner=private_team)
         return private_team, member, branch
 
     def test_private_team_membership_for_team_member(self):

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-06-08 06:05:25 +0000
@@ -54,7 +54,10 @@
     make_merge_proposal_without_reviewers,
     )
 from lp.registry.enums import InformationType
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    )
 from lp.services.messages.model.message import MessageSet
 from lp.services.webapp import canonical_url
 from lp.services.webapp.interfaces import (
@@ -217,7 +220,8 @@
         # Set up some review requests.
         public_person1 = self.factory.makePerson()
         private_team1 = self.factory.makeTeam(
-            visibility=PersonVisibility.PRIVATE)
+            visibility=PersonVisibility.PRIVATE,
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
         self._nominateReviewer(public_person1, owner)
         self._nominateReviewer(private_team1, owner)
 

=== modified file 'lib/lp/code/browser/tests/test_branchsubscription.py'
--- lib/lp/code/browser/tests/test_branchsubscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/tests/test_branchsubscription.py	2012-06-08 06:05:25 +0000
@@ -1,13 +1,18 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for BranchSubscriptions."""
 
 __metaclass__ = type
 
+from lp.registry.enums import InformationType
 from lp.services.webapp.interfaces import IPrimaryContext
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
 
 
 class TestBranchSubscriptionPrimaryContext(TestCaseWithFactory):
@@ -22,3 +27,26 @@
         self.assertEqual(
             IPrimaryContext(subscription).context,
             IPrimaryContext(subscription.branch).context)
+
+
+class TestBranchSubscriptionAddOtherView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_cannot_subscribe_open_team_to_private_branch(self):
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(
+            information_type=InformationType.USERDATA, 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(
+                ['Open and delegated teams cannot be subscribed to private '
+                'branches.'], view.errors)

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-06-05 02:03:44 +0000
+++ lib/lp/code/model/branch.py	2012-06-08 06:05:25 +0000
@@ -2,8 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212,W0141,F0401
-from lp.bugs.interfaces.bugtarget import IBugTarget
-from lp.registry.interfaces.accesspolicy import IAccessPolicyArtifactSource, IAccessPolicySource, IAccessArtifactSource
 
 __metaclass__ = type
 __all__ = [
@@ -52,11 +50,15 @@
     )
 
 from lp import _
-from lp.app.errors import UserCannotUnsubscribePerson
+from lp.app.errors import (
+    SubscriptionPrivacyViolation,
+    UserCannotUnsubscribePerson,
+    )
 from lp.app.interfaces.launchpad import (
     ILaunchpadCelebrities,
     IPrivacy,
     )
+from lp.app.interfaces.services import IService
 from lp.bugs.interfaces.bugtask import (
     BugTaskSearchParams,
     IBugTaskSet,
@@ -155,6 +157,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
@@ -817,6 +820,11 @@
     def subscribe(self, person, notification_level, max_diff_lines,
                   code_review_level, subscribed_by):
         """See `IBranch`."""
+        if (person.is_team and self.information_type in
+            PRIVATE_INFORMATION_TYPES and person.anyone_can_join()):
+            raise SubscriptionPrivacyViolation(
+                "Open and delegated teams cannot be subscribed to private "
+                "branches.")
         # If the person is already subscribed, update the subscription with
         # the specified notification details.
         subscription = self.getSubscription(person)
@@ -831,6 +839,14 @@
             subscription.notification_level = notification_level
             subscription.max_diff_lines = max_diff_lines
             subscription.review_level = code_review_level
+        # Grant the subscriber access if they can't see the branch.
+        service = getUtility(IService, 'sharing')
+        ignored, branches = service.getVisibleArtifacts(
+            person, branches=[self])
+        if not branches:
+            service.ensureAccessGrants(
+                subscribed_by, person, branches=[self],
+                ignore_permissions=True)
         return subscription
 
     def getSubscription(self, person):
@@ -858,13 +874,12 @@
         """See `IBranch`."""
         return self.getSubscription(person) is not None
 
-    def unsubscribe(self, person, unsubscribed_by, **kwargs):
+    def unsubscribe(self, person, unsubscribed_by, ignore_permissions=False):
         """See `IBranch`."""
         subscription = self.getSubscription(person)
         if subscription is None:
             # Silent success seems order of the day (like bugs).
             return
-        ignore_permissions = kwargs.get('ignore_permissions', False)
         if (not ignore_permissions
             and not subscription.canBeUnsubscribedByUser(unsubscribed_by)):
             raise UserCannotUnsubscribePerson(

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2012-04-12 15:16:12 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2012-06-08 06:05:25 +0000
@@ -76,6 +76,7 @@
     IncrementalDiff,
     PreviewDiff,
     )
+from lp.registry.enums import PRIVATE_INFORMATION_TYPES
 from lp.registry.interfaces.person import (
     IPerson,
     IPersonSet,
@@ -634,17 +635,29 @@
                 self._subscribeUserToStackedBranch(
                     branch.stacked_on, user, checked_branches)
 
+    def _acceptable_to_give_visibility(self, branch, reviewer):
+        # If the branch is private, only closed teams can be subscribed to
+        # prevent leaks.
+        if (branch.information_type in PRIVATE_INFORMATION_TYPES and
+            reviewer.is_team and reviewer.anyone_can_join()):
+            return False
+        return True
+
     def _ensureAssociatedBranchesVisibleToReviewer(self, reviewer):
         """ A reviewer must be able to see the source and target branches.
 
         Currently, we ensure the required visibility by subscribing the user
-        to the branch and those on which it is stacked.
+        to the branch and those on which it is stacked. We do not subscribe
+        the reviewer if the branch is private and the reviewer is an open
+        team.
         """
         source = self.source_branch
-        if not source.visibleByUser(reviewer):
+        if (not source.visibleByUser(reviewer) and
+            self._acceptable_to_give_visibility(source, reviewer)):
             self._subscribeUserToStackedBranch(source, reviewer)
         target = self.target_branch
-        if not target.visibleByUser(reviewer):
+        if (not target.visibleByUser(reviewer) and
+            self._acceptable_to_give_visibility(source, reviewer)):
             self._subscribeUserToStackedBranch(target, reviewer)
 
     def nominateReviewer(self, reviewer, registrant, review_type=None,

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-06-04 09:41:48 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-06-08 06:05:25 +0000
@@ -116,7 +116,10 @@
     IAccessPolicyArtifactSource,
     IAccessPolicySource,
     )
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.sourcepackage import SourcePackage
 from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
@@ -2365,7 +2368,9 @@
         self.assertTrue(branch.explicitly_private)
 
     def test_personal_branches_for_private_teams_are_private(self):
-        team = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            visibility=PersonVisibility.PRIVATE)
         branch = self.factory.makePersonalBranch(owner=team)
         self.assertTrue(branch.private)
         self.assertEqual(InformationType.USERDATA, branch.information_type)

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2012-06-08 06:05:25 +0000
@@ -31,6 +31,7 @@
 from lp.code.model.branchcollection import GenericBranchCollection
 from lp.code.tests.helpers import remove_all_sample_data_branches
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.webapp.interfaces import (
     DEFAULT_FLAVOR,
@@ -684,7 +685,9 @@
         # A person in a team that is subscribed to a branch can see that
         # branch, even if it's private.
         team_owner = self.factory.makePerson()
-        team = self.factory.makeTeam(team_owner)
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            owner=team_owner)
         private_branch = self.factory.makeAnyBranch(
             information_type=InformationType.USERDATA)
         # Subscribe the team.

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2012-06-08 06:05:25 +0000
@@ -63,7 +63,10 @@
     make_merge_proposal_without_reviewers,
     )
 from lp.registry.enums import InformationType
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    TeamSubscriptionPolicy,
+    )
 from lp.registry.interfaces.product import IProductSet
 from lp.services.database.constants import UTC_NOW
 from lp.services.webapp import canonical_url
@@ -151,6 +154,40 @@
         self.setPrivate(bmp.prerequisite_branch)
         self.assertTrue(bmp.private)
 
+    def test_open_reviewer_with_private_branch(self):
+        """If the reviewer is an open team, and either of the branches are
+        private, they are not subscribed."""
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        trunk = self.factory.makeBranch(product=product, owner=owner)
+        team = self.factory.makeTeam()
+        branch = self.factory.makeBranch(
+            information_type=InformationType.USERDATA, owner=owner,
+            product=product)
+        with person_logged_in(owner):
+            trunk.reviewer = team
+            bmp = self.factory.makeBranchMergeProposal(
+                source_branch=branch, target_branch=trunk)
+            subscriptions = [bsub.person for bsub in branch.subscriptions]
+            self.assertEqual([owner], subscriptions)
+
+    def test_closed_reviewer_with_private_branch(self):
+        """If the reviewer is a closed team, they are subscribed."""
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        trunk = self.factory.makeBranch(product=product, owner=owner)
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        branch = self.factory.makeBranch(
+            information_type=InformationType.USERDATA, owner=owner,
+            product=product)
+        with person_logged_in(owner):
+            trunk.reviewer = team
+            bmp = self.factory.makeBranchMergeProposal(
+                source_branch=branch, target_branch=trunk)
+            subscriptions = [bsub.person for bsub in branch.subscriptions]
+            self.assertContentEqual([owner, team], subscriptions)
+
 
 class TestBranchMergeProposalTransitions(TestCaseWithFactory):
     """Test the state transitions of branch merge proposals."""
@@ -1530,7 +1567,8 @@
         self._test_nominate_grants_visibility(reviewer)
 
     def test_nominate_team_grants_visibility(self):
-        reviewer = self.factory.makeTeam()
+        reviewer = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
         self._test_nominate_grants_visibility(reviewer)
 
     def test_comment_with_vote_creates_reference(self):

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2012-05-04 04:52:24 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2012-06-08 06:05:25 +0000
@@ -44,6 +44,7 @@
 from lp.registry.interfaces.person import (
     NoSuchPerson,
     PersonVisibility,
+    TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.product import NoSuchProduct
 from lp.registry.model.sourcepackage import SourcePackage
@@ -379,7 +380,9 @@
         # Given a privacy policy for a namespace, branches are created with
         # the correct information type.
         person = self.factory.makePerson()
-        team = self.factory.makeTeam(owner=person)
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            owner=person)
         product = self.factory.makeProduct()
         namespace = ProductNamespace(team, product)
         product.setBranchVisibilityTeamPolicy(
@@ -1337,38 +1340,33 @@
           * "charlie", who is a member of zulu.
           * "doug", who is a member of no teams.
         """
-        TestCaseWithFactory.setUp(self, '[e-mail]')
+        super(BranchVisibilityPolicyTestCase, self).setUp(
+            '[e-mail]')
         # Our test product.
         self.product = self.factory.makeProduct()
         # Create some test people.
         self.albert = self.factory.makePerson(
-            email='[e-mail]',
             name='albert', displayname='Albert Tester')
         self.bob = self.factory.makePerson(
-            email='[e-mail]',
             name='bob', displayname='Bob Tester')
         self.charlie = self.factory.makePerson(
-            email='[e-mail]',
             name='charlie', displayname='Charlie Tester')
         self.doug = self.factory.makePerson(
-            email='[e-mail]',
             name='doug', displayname='Doug Tester')
-
         self.people = (self.albert, self.bob, self.charlie, self.doug)
 
         # And create some test teams.
-        self.xray = self.factory.makeTeam(name='xray')
-        self.yankee = self.factory.makeTeam(name='yankee')
-        self.zulu = self.factory.makeTeam(name='zulu')
+        self.xray = self.factory.makeTeam(
+            name='xray', members=[self.albert],
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        self.yankee = self.factory.makeTeam(
+            name='yankee', members=[self.albert, self.bob],
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        self.zulu = self.factory.makeTeam(
+            name='zulu', members=[self.albert, self.charlie],
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
         self.teams = (self.xray, self.yankee, self.zulu)
 
-        # Set the memberships of our test people to the test teams.
-        self.albert.join(self.xray)
-        self.albert.join(self.yankee)
-        self.albert.join(self.zulu)
-        self.bob.join(self.yankee)
-        self.charlie.join(self.zulu)
-
     def defineTeamPolicies(self, team_policies):
         """Shortcut to help define team policies."""
         for team, rule in team_policies:

=== modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
--- lib/lp/code/model/tests/test_branchsubscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_branchsubscription.py	2012-06-08 06:05:25 +0000
@@ -1,15 +1,19 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the BranchSubscrptions model object.."""
 
 __metaclass__ = type
 
-from lp.app.errors import UserCannotUnsubscribePerson
+from lp.app.errors import (
+    SubscriptionPrivacyViolation,
+    UserCannotUnsubscribePerson,
+    )
 from lp.code.enums import (
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
     )
+from lp.registry.enums import InformationType
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -67,6 +71,30 @@
             subscription.person,
             self.factory.makePerson())
 
+    def test_cannot_subscribe_open_team_to_private_branch(self):
+        """It is forbidden to subscribe a open team to a private branch."""
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(
+            information_type=InformationType.USERDATA, owner=owner)
+        team = self.factory.makeTeam()
+        with person_logged_in(owner):
+            self.assertRaises(
+                SubscriptionPrivacyViolation, branch.subscribe, team, None,
+                None, None, owner)
+
+    def test_subscribe_gives_access(self):
+        """Subscribing a user to a branch gives them access."""
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(
+            information_type=InformationType.USERDATA, owner=owner)
+        subscribee = self.factory.makePerson()
+        with person_logged_in(owner):
+            self.assertFalse(branch.visibleByUser(subscribee))
+            branch.subscribe(
+                subscribee, BranchSubscriptionNotificationLevel.NOEMAIL,
+                None, CodeReviewNotificationLevel.NOEMAIL, owner)
+            self.assertTrue(branch.visibleByUser(subscribee))
+
 
 class TestBranchSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
     """Tests for BranchSubscription.canBeUnsubscribedByUser."""

=== modified file 'lib/lp/code/model/tests/test_branchvisibility.py'
--- lib/lp/code/model/tests/test_branchvisibility.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_branchvisibility.py	2012-06-08 06:05:25 +0000
@@ -27,6 +27,7 @@
     )
 from lp.code.interfaces.branch import IBranchSet
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.registry.interfaces.role import IPersonRoles
 from lp.security import AccessBranch
 from lp.services.webapp.authorization import (
@@ -106,7 +107,9 @@
         naked_branch = removeSecurityProxy(branch)
         person = self.factory.makePerson()
         teamowner = self.factory.makePerson()
-        team = self.factory.makeTeam(owner=teamowner, members=[person])
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            owner=teamowner, members=[person])
 
         # Not visible to an unsubscribed person.
         access = AccessBranch(naked_branch)

=== modified file 'lib/lp/registry/tests/test_distro_webservice.py'
--- lib/lp/registry/tests/test_distro_webservice.py	2012-05-31 03:54:13 +0000
+++ lib/lp/registry/tests/test_distro_webservice.py	2012-06-08 06:05:25 +0000
@@ -21,6 +21,7 @@
     SeriesSourcePackageBranchSet,
     )
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.testing import (
     api_url,
@@ -198,7 +199,9 @@
         # it is visible.
         branch, distro = self.makeBranch()
         person = self.factory.makePerson()
-        team = self.factory.makeTeam(members=[person])
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            members=[person])
         removeSecurityProxy(branch).subscribe(
             team, BranchSubscriptionNotificationLevel.NOEMAIL,
             BranchSubscriptionDiffSize.NODIFF,

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-06-03 23:11:40 +0000
+++ lib/lp/testing/factory.py	2012-06-08 06:05:25 +0000
@@ -1514,7 +1514,7 @@
             person = self.makePerson()
         if subscribed_by is None:
             subscribed_by = person
-        return branch.subscribe(person,
+        return branch.subscribe(removeSecurityProxy(person),
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.NOEMAIL, subscribed_by)