Merge lp:~stevenk/launchpad/branch-use-information_type into lp:launchpad

[Steve Kowalik]

Switch Branch to using information_type by default.

Switch Branch to using information_type by default. I have also moved the code from setPrivate to transitionToInformationType and made setPrivate a wrapper around it. Changing the information_type of a branch that is stacked on a non-public branch is now forbidden. IBranch.branchChanged() now forces the branch that is being stacked on top of an existing branch will have its information_type set to the stacked on information_type if it's not public.

I have changed the factory method makeBranch to take an information_type argument, and have also made it call IBranch.branchChanged() if stacked_on is set, which more closely mirrors what happens in production.

This necessitated changing a fair number of tests, which naively just set explicitly_private by hand. I've converted them to setting information_type, but am willing to make use of IBranch.setPrivate() or IBranch.transistionToInformationType() instead.

[William Grant (wgrant)]

32 +def convert_to_information_type(private):
33 + if private:
34 + return InformationType.USERDATA
35 + else:
36 + return InformationType.PUBLIC

I'd put this in model rather than adapters. It's not useful outside model.

101 +class BranchCannotChangeInformationType(Exception):
102 + """The branch cannot change its information type."""

Of course it can't -- a branch isn't an actor. Do you mean that its information type can't be changed?

116 +from lazr.restful.interface import copy_field

Unused?

159 + @property
160 def private(self):
161 - return self.transitively_private
162 + return self.information_type in PRIVATE_INFORMATION_TYPES

Doesn't this need to defer to transitively_private if information_type? Depending on the way information_type is populated later, we may even need to ignore information_type completely until the migration is done.

174 + if (
175 + self.stacked_on and self.stacked_on.information_type !=
176 + InformationType.PUBLIC and information_type !=
177 + self.stacked_on.information_type):

That is the worst line wrapping in the history of the universe.

211 + if (
212 + self.stacked_on and self.stacked_on.information_type !=
213 + InformationType.PUBLIC):
214 + self.information_type = self.stacked_on.information_type

This could do with one fewer line breaks, and possibly a comment.

327 + if information_type:
328 + removeSecurityProxy(branch).information_type = information_type
329 if private:
330 removeSecurityProxy(branch).explicitly_private = True
331 removeSecurityProxy(branch).transitively_private = True
332 + if not information_type:
333 + removeSecurityProxy(branch).information_type = (
334 + InformationType.USERDATA)

Should you perhaps merge the two information_type bits? And how close are we to eliminating makeBranch(private)?

review: Approve (code)

[William Grant (wgrant)]

121 + if (self.stacked_on and self.stacked_on.information_type in
122 + PRIVATE_INFORMATION_TYPES and information_type in
123 + PUBLIC_INFORMATION_TYPES):

This linebreaking is still a capital offence in several jurisdictions. One legal spelling is this:

if (self.stacked on
    and self.stacked_on.information_type in PRIVATE_INFORMATION TYPES
    and information_type in PUBLIC_INFORMATION TYPES):

157 + # If the branch we are stacking on is not public, and we are,
158 + # set our information_type to the stacked on's.

This comment should explain why.

159 + if (self.stacked_on and self.stacked_on.information_type not in
160 + PUBLIC_INFORMATION_TYPES and self.information_type in
161 + PUBLIC_INFORMATION_TYPES):

Another capital offence.

273 + branch = self.factory.makeBranch(
274 + name='branch_%s' % x, private=x > 2)

That linebreak is probably no longer required.

review: Approve (code)

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-05-02 05:25:11 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-05-23 05:26:23 +0000
@@ -758,7 +758,8 @@
         # If the target is private, the landing targets should not include it.
         bmp = self.factory.makeBranchMergeProposal()
         branch = bmp.source_branch
-        removeSecurityProxy(bmp.target_branch).explicitly_private = True
+        removeSecurityProxy(bmp.target_branch).information_type = (
+            InformationType.USERDATA)
         view = BranchView(branch, LaunchpadTestRequest())
         self.assertTrue(view.no_merges)
         self.assertEqual([], view.landing_targets)
@@ -779,7 +780,8 @@
         # it.
         bmp = self.factory.makeBranchMergeProposal()
         branch = bmp.target_branch
-        removeSecurityProxy(bmp.source_branch).explicitly_private = True
+        removeSecurityProxy(bmp.source_branch).information_type = (
+            InformationType.USERDATA)
         view = BranchView(branch, LaunchpadTestRequest())
         self.assertTrue(view.no_merges)
         self.assertEqual([], view.landing_candidates)
@@ -799,7 +801,8 @@
         # the target is private, then the dependent_branches are not shown.
         branch = self.factory.makeProductBranch()
         bmp = self.factory.makeBranchMergeProposal(prerequisite_branch=branch)
-        removeSecurityProxy(bmp.source_branch).explicitly_private = True
+        removeSecurityProxy(bmp.source_branch).information_type = (
+            InformationType.USERDATA)
         view = BranchView(branch, LaunchpadTestRequest())
         self.assertTrue(view.no_merges)
         self.assertEqual([], view.dependent_branches)
 
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-05-02 05:25:11 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-05-23 05:26:23 +0000
@@ -1290,7 +1290,8 @@
         bmp2 = self.factory.makeBranchMergeProposal(
             date_created=(
                 datetime(year=2008, month=10, day=10, tzinfo=pytz.UTC)))
-        removeSecurityProxy(bmp2.source_branch).explicitly_private = True
+        removeSecurityProxy(bmp2.source_branch).information_type = (
+            InformationType.USERDATA)
         self.assertEqual(
             [bmp1], latest_proposals_for_each_branch([bmp1, bmp2]))
 
 
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2012-02-21 22:46:28 +0000
+++ lib/lp/code/errors.py	2012-05-23 05:26:23 +0000
@@ -10,6 +10,7 @@
     'BadStateTransition',
     'BranchCannotBePrivate',
     'BranchCannotBePublic',
+    'BranchCannotChangeInformationType',
     'BranchCreationException',
     'BranchCreationForbidden',
     'BranchCreatorNotMemberOfOwnerTeam',
@@ -155,6 +156,10 @@
     """The branch cannot be made private."""
 
 
+class BranchCannotChangeInformationType(Exception):
+    """The information type of this branch cannot be changed."""
+
+
 class InvalidBranchException(Exception):
     """Base exception for an error resolving a branch for a component.
 
 
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-05-04 04:52:24 +0000
+++ lib/lp/code/model/branch.py	2012-05-23 05:26:23 +0000
@@ -77,6 +77,7 @@
     AlreadyLatestFormat,
     BranchCannotBePrivate,
     BranchCannotBePublic,
+    BranchCannotChangeInformationType,
     BranchMergeProposalExists,
     BranchTargetError,
     BranchTypeError,
@@ -128,7 +129,11 @@
     )
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
 from lp.codehosting.safe_open import safe_open
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    PRIVATE_INFORMATION_TYPES,
+    PUBLIC_INFORMATION_TYPES,
+    )
 from lp.registry.interfaces.person import (
     validate_person,
     validate_public_person,
@@ -187,33 +192,42 @@
 
     @property
     def private(self):
-        return self.transitively_private
+        return self.information_type in PRIVATE_INFORMATION_TYPES
 
     def setPrivate(self, private, user):
         """See `IBranch`."""
-        if private == self.explicitly_private:
+        if private:
+            information_type = InformationType.USERDATA
+        else:
+            information_type = InformationType.PUBLIC
+        return self.transitionToInformationType(information_type, user)
+
+    def transitionToInformationType(self, information_type, who):
+        """See `IBranch`."""
+        if self.information_type == information_type:
             return
+        if (self.stacked_on
+            and self.stacked_on.information_type in PRIVATE_INFORMATION_TYPES
+            and information_type in PUBLIC_INFORMATION_TYPES):
+            raise BranchCannotChangeInformationType()
+        private = information_type in PRIVATE_INFORMATION_TYPES
         # Only check the privacy policy if the user is not special.
-        if (not user_has_special_branch_access(user)):
+        if (not user_has_special_branch_access(who)):
             policy = IBranchNamespacePolicy(self.namespace)
 
             if private and not policy.canBranchesBePrivate():
                 raise BranchCannotBePrivate()
             if not private and not policy.canBranchesBePublic():
                 raise BranchCannotBePublic()
+        self.information_type = information_type
+        # Set the legacy values for now.
         self.explicitly_private = private
         # If this branch is private, then it is also transitively_private
         # otherwise we need to reload the value.
         if private:
             self.transitively_private = True
-            self.information_type = InformationType.USERDATA
         else:
             self.transitively_private = AutoReload
-            self.information_type = InformationType.PUBLIC
-
-    def transitionToInformationType(self, information_type, who):
-        """See `IBranch`."""
-        self.information_type = information_type
 
     registrant = ForeignKey(
         dbName='registrant', foreignKey='Person',
@@ -1052,6 +1066,13 @@
                 self.mirror_status_message = (
                     'Invalid stacked on location: ' + stacked_on_url)
         self.stacked_on = stacked_on_branch
+        # If the branch we are stacking on is not public, and we are,
+        # set our information_type to the stacked on's, since having a
+        # public branch stacked on a private branch does not make sense.
+        if (self.stacked_on
+            and self.stacked_on.information_type in PRIVATE_INFORMATION_TYPES
+            and self.information_type in PUBLIC_INFORMATION_TYPES):
+            self.information_type = self.stacked_on.information_type
         if self.branch_type == BranchType.HOSTED:
             self.last_mirrored = UTC_NOW
         else:
@@ -1207,7 +1228,7 @@
         This method doesn't check the stacked upon branch.  That is handled by
         the `visibleByUser` method.
         """
-        if not self.explicitly_private:
+        if self.information_type not in PRIVATE_INFORMATION_TYPES:
             return True
         if user is None:
             return False
 
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-05-04 04:52:24 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-05-23 05:26:23 +0000
@@ -56,6 +56,7 @@
     AlreadyLatestFormat,
     BranchCannotBePrivate,
     BranchCannotBePublic,
+    BranchCannotChangeInformationType,
     BranchCreatorNotMemberOfOwnerTeam,
     BranchCreatorNotOwner,
     BranchTargetError,
@@ -2334,16 +2335,21 @@
     def test_public_stacked_on_private_is_private(self):
         # A public branch stacked on a private branch is private.
         stacked_on = self.factory.makeBranch(private=True)
-        branch = self.factory.makeBranch(stacked_on=stacked_on, private=False)
+        branch = self.factory.makeBranch(
+            stacked_on=stacked_on, private=False)
         self.assertTrue(branch.private)
+        self.assertEqual(
+            stacked_on.information_type, branch.information_type)
         self.assertTrue(removeSecurityProxy(branch).transitively_private)
         self.assertFalse(branch.explicitly_private)
 
     def test_private_stacked_on_public_is_private(self):
-        # A public branch stacked on a private branch is private.
+        # A private branch stacked on a public branch is private.
         stacked_on = self.factory.makeBranch(private=False)
         branch = self.factory.makeBranch(stacked_on=stacked_on, private=True)
         self.assertTrue(branch.private)
+        self.assertNotEqual(
+            stacked_on.information_type, branch.information_type)
         self.assertTrue(removeSecurityProxy(branch).transitively_private)
         self.assertTrue(branch.explicitly_private)
 
@@ -2436,6 +2442,26 @@
             branch.setPrivate,
             False, branch.owner)
 
+    def test_cannot_transition_with_private_stacked_on(self):
+        # If a public branch is stacked on a private branch, it can not
+        # change its information_type to public.
+        stacked_on = self.factory.makeBranch(private=True)
+        branch = self.factory.makeBranch(stacked_on=stacked_on)
+        self.assertRaises(
+            BranchCannotChangeInformationType,
+            branch.transitionToInformationType, InformationType.PUBLIC,
+            branch.owner)
+
+    def test_can_transition_with_public_stacked_on(self):
+        # If a private branch is stacked on a public branch, it can change
+        # its information_type.
+        stacked_on = self.factory.makeBranch()
+        branch = self.factory.makeBranch(stacked_on=stacked_on, private=True)
+        branch.transitionToInformationType(
+            InformationType.UNEMBARGOEDSECURITY, branch.owner)
+        self.assertEqual(
+            InformationType.UNEMBARGOEDSECURITY, branch.information_type)
+
 
 class TestBranchCommitsForDays(TestCaseWithFactory):
     """Tests for `Branch.commitsForDays`."""
 
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2012-05-02 05:25:11 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2012-05-23 05:26:23 +0000
@@ -929,8 +929,9 @@
             charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.FULL, charlie)
         # Make both branches private.
-        removeSecurityProxy(bmp.source_branch).explicitly_private = True
-        removeSecurityProxy(bmp.target_branch).explicitly_private = True
+        for branch in (bmp.source_branch, bmp.target_branch):
+            removeSecurityProxy(branch).information_type = (
+                InformationType.USERDATA)
         recipients = bmp.getNotificationRecipients(
             CodeReviewNotificationLevel.FULL)
         self.assertFalse(bob in recipients)
 
=== modified file 'lib/lp/code/model/tests/test_branchvisibility.py'
--- lib/lp/code/model/tests/test_branchvisibility.py	2012-01-15 17:43:05 +0000
+++ lib/lp/code/model/tests/test_branchvisibility.py	2012-05-23 05:26:23 +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 visibility of branches.
@@ -134,14 +134,11 @@
         private_owner = self.factory.makePerson()
         test_branches = []
         for x in range(5):
-            # We want the first 3 public and the last 3 private
-            branch = self.factory.makeBranch(name='branch_%s' % x)
-            # The 3rd, 4th and 5th will be explicitly private.
-            branch.explicitly_private = x > 2
+            # We want the first 3 public and the last 3 private.
+            branch = self.factory.makeBranch(private=x > 2)
             test_branches.append(branch)
         test_branches.append(
-            self.factory.makeBranch(
-                name='branch_5', private=True, owner=private_owner))
+            self.factory.makeBranch(private=True, owner=private_owner))
 
         # Anonymous users see just the public branches.
         branch_info = [(branch, branch.private)
 
=== modified file 'lib/lp/code/xmlrpc/tests/test_branch.py'
--- lib/lp/code/xmlrpc/tests/test_branch.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/xmlrpc/tests/test_branch.py	2012-05-23 05:26:23 +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).
 
 """Unit tests for the public codehosting API."""
@@ -16,6 +16,7 @@
 from lp.code.interfaces.codehosting import BRANCH_ALIAS_PREFIX
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.xmlrpc.branch import PublicCodehostingAPI
+from lp.registry.enums import InformationType
 from lp.services.xmlrpc import LaunchpadFault
 from lp.testing import (
     person_logged_in,
@@ -323,7 +324,8 @@
     def test_private_branch_as_development_focus(self):
         # We resolve private linked branches using the writable alias.
         product, trunk = self.makeProdutWithTrunk()
-        removeSecurityProxy(trunk).explicitly_private = True
+        removeSecurityProxy(trunk).information_type = (
+            InformationType.USERDATA)
         self.assertOnlyWritableResolves(product.name)
 
     def test_private_branch_as_user(self):
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-05-04 14:52:44 +0000
+++ lib/lp/testing/factory.py	2012-05-23 05:26:23 +0000
@@ -140,6 +140,7 @@
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
     InformationType,
+    PRIVATE_INFORMATION_TYPES,
     )
 from lp.registry.interfaces.accesspolicy import (
     IAccessArtifactGrantSource,
@@ -1072,8 +1073,8 @@
 
     def makeBranch(self, branch_type=None, owner=None,
                    name=None, product=_DEFAULT, url=_DEFAULT, registrant=None,
-                   private=False, stacked_on=None, sourcepackage=None,
-                   reviewer=None, **optional_branch_args):
+                   private=None, information_type=None, stacked_on=None,
+                   sourcepackage=None, reviewer=None, **optional_branch_args):
         """Create and return a new, arbitrary Branch of the given type.
 
         Any parameters for `IBranchNamespace.createBranch` can be specified to
@@ -1119,11 +1120,22 @@
         branch = namespace.createBranch(
             branch_type=branch_type, name=name, registrant=registrant,
             url=url, **optional_branch_args)
-        if private:
-            removeSecurityProxy(branch).explicitly_private = True
-            removeSecurityProxy(branch).transitively_private = True
+        assert information_type is None or private is None, (
+            "Can not specify both information_type and private")
+        if information_type is not None or private is not None:
+            if information_type:
+                private = information_type in PRIVATE_INFORMATION_TYPES
+            else:
+                information_type = (
+                    InformationType.USERDATA if private else
+                    InformationType.PUBLIC)
+            if private:
+                removeSecurityProxy(branch).explicitly_private = True
+                removeSecurityProxy(branch).transitively_private = True
+            removeSecurityProxy(branch).information_type = information_type
         if stacked_on is not None:
-            removeSecurityProxy(branch).stacked_on = stacked_on
+            removeSecurityProxy(branch).branchChanged(
+                stacked_on.unique_name, 'rev1', None, None, None)
         if reviewer is not None:
             removeSecurityProxy(branch).reviewer = reviewer
         return branch