Merge lp:~wgrant/launchpad/improve-branch-edit-type into lp:launchpad

[William Grant]

Rework BranchEditView's information type widget to support the new sharing model.

This branch reworks BranchEditView's information type widget to support the new sharing model a little more easily. It used to choose types to show based on Branch.canBe{Public,Private}, which doesn't work well if we want to use a different set (eg. private projects only support Proprietary).

I adjusted Branch.transitionToInformationType to use the information types allowed by the branch's namespace, or all the information types if the user is a Launchpad admin. BranchEditView.getInformationTypesToShow restricts this further, hiding uninteresting types to avoid confusion, and in a sort of attempt to prevent projects from using private branches for nefarious purposes without paying us. These obscure types are only shown in the UI if there's a linked bug of one of the obscure types, similar to the old behaviour in Branch.canBePrivate.

I've dropped the commercial subscription check from Branch.canBePrivate for now, as it's new, not yet enabled, and will be replaced by a branch configuration option shortly.

There's still some awfulness in BranchEditView around restricting type changes for stacked branches, but I plan to disentangle that in a followup.

[j.c.sackett (jcsackett)]

Thanks William. One minor point.

At line #60 you have a comment about using the widget vocabulary in `getInformationTypesToShow`, but you actually do that vocal step on #103, in setUpWidgets. Might be worth moving the comment to the place where the code is actually happening.

review: Approve

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-07-10 10:58:27 +0000
+++ lib/lp/code/browser/branch.py	2012-07-12 21:59:20 +0000
@@ -91,7 +91,11 @@
 from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
 from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.interfaces.bugbranch import IBugBranch
-from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES
+from lp.bugs.interfaces.bugtask import (
+    BugTaskSearchParams,
+    IBugTaskSet,
+    UNRESOLVED_BUGTASK_STATUSES,
+    )
 from lp.code.browser.branchmergeproposal import (
     latest_proposals_for_each_branch,
     )
@@ -121,6 +125,7 @@
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.registry.enums import (
+    InformationType,
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
     )
@@ -1095,36 +1100,56 @@
         if branch.branch_type in (BranchType.HOSTED, BranchType.IMPORTED):
             self.form_fields = self.form_fields.omit('url')
 
+    def getInformationTypesToShow(self):
+        """Get the information types to display on the edit form.
+
+        We display a highly customised set of information types:
+        anything allowed by the namespace, plus the current type,
+        except some of the obscure types unless there's a linked
+        bug with an obscure type.
+        """
+        allowed_types = self.context.getAllowedInformationTypes(self.user)
+        shown_types = (
+            InformationType.PUBLIC,
+            InformationType.USERDATA,
+            InformationType.PROPRIETARY,
+            )
+
+        # We only show Embargoed Security and Unembargoed Security
+        # if the branch is linked to a bug with one of those types,
+        # as they're confusing and not generally useful otherwise.
+        # Once Proprietary is fully deployed, User Data should be
+        # added here.
+        hidden_types = (
+            InformationType.UNEMBARGOEDSECURITY,
+            InformationType.EMBARGOEDSECURITY,
+            )
+        if set(allowed_types).intersection(hidden_types):
+            params = BugTaskSearchParams(
+                user=self.user, linked_branches=self.context.id,
+                information_type=hidden_types)
+            if getUtility(IBugTaskSet).searchBugIds(params).count() > 0:
+                shown_types += hidden_types
+
+        # Now take the intersection of the allowed and shown types.
+        combined_types = set(allowed_types).intersection(shown_types)
+        combined_types.add(self.context.information_type)
+        return combined_types
+
     def setUpWidgets(self, context=None):
         super(BranchEditView, self).setUpWidgets()
-        branch = self.context
-
         if self.form_fields.get('information_type') is not None:
+            # Customise the set of shown types.
+            types_to_show = self.getInformationTypesToShow()
+            # Grab the types from the vocab if they exist.
             # The vocab uses feature flags to control what is displayed so we
             # need to pull info_types from the vocab to use to make the subset
-            # of what we show the user.
+            # of what we show the user. This is mostly to hide Proprietary
+            # while it's disabled.
             info_type_vocab = self.widgets['information_type'].vocabulary
-            public_types = [
-                    info_type
-                    for info_type in info_type_vocab
-                    if info_type.value in PUBLIC_INFORMATION_TYPES]
-            private_types = [
-                    info_type
-                    for info_type in info_type_vocab
-                    if info_type.value in PRIVATE_INFORMATION_TYPES]
-
-            allowed_information_types = []
-            if branch.private:
-                if branch.canBePublic(self.user):
-                    allowed_information_types.extend(public_types)
-                allowed_information_types.extend(private_types)
-            else:
-                allowed_information_types.extend(public_types)
-                if branch.canBePrivate(self.user):
-                    allowed_information_types.extend(private_types)
-
-            self.widgets['information_type'].vocabulary = (
-                SimpleVocabulary(allowed_information_types))
+            self.widgets['information_type'].vocabulary = SimpleVocabulary(
+                [info_type for info_type in info_type_vocab
+                 if info_type.value in types_to_show])
 
     def validate(self, data):
         # Check that we're not moving a team branch to the +junk

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-07-09 04:14:09 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-07-12 21:59:20 +0000
@@ -47,6 +47,7 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
+    admin_logged_in,
     BrowserTestCase,
     login,
     login_person,
@@ -915,32 +916,11 @@
         admin = admins.teamowner
         browser = self.getUserBrowser(
             canonical_url(branch) + '/+edit', user=admin)
-        browser.getControl("Embargoed Security").click()
+        browser.getControl("User Data").click()
         browser.getControl("Change Branch").click()
         with person_logged_in(person):
             self.assertEqual(
-                InformationType.EMBARGOEDSECURITY, branch.information_type)
-
-    def test_proprietary_in_ui_vocabulary_commercial_projects(self):
-        # Commercial projects can have information type Proprietary.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct()
-        self.factory.makeCommercialSubscription(product)
-        branch = self.factory.makeProductBranch(product=product, owner=owner)
-        with person_logged_in(owner):
-            browser = self.getUserBrowser(
-                canonical_url(branch) + '/+edit', user=owner)
-            self.assertIsNotNone(browser.getControl("Proprietary"))
-
-    def test_proprietary_not_in_ui_vocabulary_normal_projects(self):
-        # Non-commercial projects can not have information type Proprietary.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct()
-        branch = self.factory.makeProductBranch(product=product, owner=owner)
-        with person_logged_in(owner):
-            browser = self.getUserBrowser(
-                canonical_url(branch) + '/+edit', user=owner)
-            self.assertRaises(LookupError, browser.getControl, "Proprietary")
+                InformationType.USERDATA, branch.information_type)
 
     def test_can_not_change_privacy_of_stacked_on_private(self):
         # The privacy field is not shown if the branch is stacked on a
@@ -958,41 +938,83 @@
         self.assertRaises(
             LookupError, browser.getControl, "Information Type")
 
-    def test_authorised_user_can_change_branch_to_private(self):
-        # An authorised user can make the information type private.
-        team_owner = self.factory.makePerson()
-        user = self.factory.makePerson()
-        team = self.factory.makeTeam(
-            owner=team_owner,
-            visibility=PersonVisibility.PRIVATE,
-            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
-        with person_logged_in(team_owner):
-            team.addMember(user, team_owner)
-        with person_logged_in(user):
-            branch = self.factory.makeBranch(owner=team)
-            browser = self.getUserBrowser(
-                canonical_url(branch) + '/+edit', user=user)
-        self.assertIsNotNone(browser.getControl, "Embargoed Security")
-
-    def test_unauthorised_user_cannot_change_branch_to_private(self):
-        # An unauthorised user cannot make the information type private.
-        user = self.factory.makePerson()
-        with person_logged_in(user):
-            branch = self.factory.makeBranch(owner=user)
-            browser = self.getUserBrowser(
-                canonical_url(branch) + '/+edit', user=user)
-        self.assertRaises(
-            LookupError, browser.getControl, "Embargoed Security")
-
-    def test_branch_for_commercial_project(self):
-        # A branch for a commercial project can be private.
-        product = self.factory.makeProduct()
-        self.factory.makeCommercialSubscription(product)
-        branch = self.factory.makeProductBranch(product=product)
-        with person_logged_in(branch.owner):
-            browser = self.getUserBrowser(
-                canonical_url(branch) + '/+edit', user=branch.owner)
-        self.assertIsNotNone(browser.getControl, "Embargoed Security")
+
+class TestBranchEditViewInformationTypes(TestCaseWithFactory):
+    """Tests for BranchEditView.getInformationTypesToShow."""
+
+    layer = DatabaseFunctionalLayer
+
+    def assertShownTypes(self, types, branch, user=None):
+        if user is None:
+            user = removeSecurityProxy(branch).owner
+        with person_logged_in(user):
+            view = create_initialized_view(branch, '+edit', user=user)
+            self.assertContentEqual(types, view.getInformationTypesToShow())
+
+    def test_public_branch(self):
+        # A normal public branch on a public project can only be public.
+        # We don't show information types like Unembargoed Security
+        # unless there's a linked branch of that type, as they're not
+        # useful or unconfusing otherwise.
+        # The model doesn't enforce this, so it's just a UI thing.
+        branch = self.factory.makeBranch(
+            information_type=InformationType.PUBLIC)
+        self.assertShownTypes([InformationType.PUBLIC], branch)
+
+    def test_public_branch_with_security_bug(self):
+        # A public branch can be set to Unembargoed Security if it has a
+        # linked Unembargoed Security bug. The project policy doesn't
+        # allow private branches, so Embargoed Security and User Data
+        # are unavailable.
+        branch = self.factory.makeBranch(
+            information_type=InformationType.PUBLIC)
+        bug = self.factory.makeBug(
+            information_type=InformationType.UNEMBARGOEDSECURITY)
+        with admin_logged_in():
+            branch.linkBug(bug, branch.owner)
+        self.assertShownTypes(
+            [InformationType.PUBLIC, InformationType.UNEMBARGOEDSECURITY],
+            branch)
+
+    def test_branch_with_disallowed_type(self):
+        # We don't force branches with a disallowed type (eg. Proprietary on a
+        # non-commercial project) to change, so the current type is
+        # shown.
+        branch = self.factory.makeBranch(
+            information_type=InformationType.PROPRIETARY)
+        self.assertShownTypes(
+            [InformationType.PUBLIC, InformationType.PROPRIETARY], branch)
+
+    def test_private_branch(self):
+        # Branches on projects with a private policy can be set to
+        # User Data (aka. Private)
+        branch = self.factory.makeBranch(
+            information_type=InformationType.PUBLIC)
+        with admin_logged_in():
+            branch.product.setBranchVisibilityTeamPolicy(
+                branch.owner, BranchVisibilityRule.PRIVATE)
+        self.assertShownTypes(
+            [InformationType.PUBLIC, InformationType.USERDATA,
+             InformationType.PROPRIETARY], branch)
+
+    def test_private_branch_with_security_bug(self):
+        # Branches on projects that allow private branches can use the
+        # Embargoed Security information type if they have a security
+        # bug linked.
+        branch = self.factory.makeBranch(
+            information_type=InformationType.PUBLIC)
+        with admin_logged_in():
+            branch.product.setBranchVisibilityTeamPolicy(
+                branch.owner, BranchVisibilityRule.PRIVATE)
+        bug = self.factory.makeBug(
+            information_type=InformationType.UNEMBARGOEDSECURITY)
+        with admin_logged_in():
+            branch.linkBug(bug, branch.owner)
+        self.assertShownTypes(
+            [InformationType.PUBLIC, InformationType.UNEMBARGOEDSECURITY,
+             InformationType.EMBARGOEDSECURITY, InformationType.USERDATA,
+             InformationType.PROPRIETARY],
+            branch)
 
 
 class TestBranchUpgradeView(TestCaseWithFactory):

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-07-06 03:33:59 +0000
+++ lib/lp/code/interfaces/branch.py	2012-07-12 21:59:20 +0000
@@ -968,23 +968,11 @@
     def visibleByUser(user):
         """Can the specified user see this branch?"""
 
-    def canBePublic(user):
-        """Can this branch be public?
-
-        A branch can be made public if:
-        - the branch has a visibility policy which allows it
-        - the user is an admin or bzr expert
-        """
-
-    def canBePrivate(user):
-        """Can this branch be private?
-
-        A branch can be made private if:
-        - the branch has a visibility policy which allows it
-        - the user is an admin or bzr expert
-        - the branch is owned by a private team
-          (The branch is already implicitly private)
-        - the branch is linked to a private bug the user can access
+    def getAllowedInformationTypes(user):
+        """Get a list of acceptable `InformationType`s for this branch.
+
+        If the user is a Launchpad admin, any type is acceptable. Otherwise
+        the `IBranchNamespace` is consulted.
         """
 
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-07-11 09:43:04 +0000
+++ lib/lp/code/model/branch.py	2012-07-12 21:59:20 +0000
@@ -237,6 +237,17 @@
             information_type = InformationType.PUBLIC
         return self.transitionToInformationType(information_type, user)
 
+    def getAllowedInformationTypes(self, who):
+        """See `IBranch`."""
+        if user_has_special_branch_access(who):
+            # Until sharing settles down, admins can set any type.
+            types = set(PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES)
+        else:
+            # Otherwise the permitted types are defined by the namespace.
+            policy = IBranchNamespacePolicy(self.namespace)
+            types = set(policy.getAllowedInformationTypes())
+        return types
+
     def transitionToInformationType(self, information_type, who,
                                     verify_policy=True):
         """See `IBranch`."""
@@ -246,11 +257,9 @@
             and self.stacked_on.information_type in PRIVATE_INFORMATION_TYPES
             and information_type in PUBLIC_INFORMATION_TYPES):
             raise BranchCannotChangeInformationType()
-        # Only check the privacy policy if the user is not special.
-        if verify_policy and not user_has_special_branch_access(who):
-            policy = IBranchNamespacePolicy(self.namespace)
-            if information_type not in policy.getAllowedInformationTypes():
-                raise BranchCannotChangeInformationType()
+        if (verify_policy
+            and information_type not in self.getAllowedInformationTypes(who)):
+            raise BranchCannotChangeInformationType()
         self.information_type = information_type
         self._reconcileAccess()
         if information_type in PRIVATE_INFORMATION_TYPES:
@@ -1292,33 +1301,6 @@
                     user, checked_branches)
         return can_access
 
-    def canBePublic(self, user):
-        """See `IBranch`."""
-        policy = IBranchNamespacePolicy(self.namespace)
-        return InformationType.PUBLIC in policy.getAllowedInformationTypes()
-
-    def canBePrivate(self, user):
-        """See `IBranch`."""
-        policy = IBranchNamespacePolicy(self.namespace)
-        # Do the easy checks first.
-        policy_allows = (
-            InformationType.USERDATA in policy.getAllowedInformationTypes())
-        if (policy_allows or
-                user_has_special_branch_access(user) or
-                user.visibility == PersonVisibility.PRIVATE):
-            return True
-        # Branches linked to commercial projects can be private.
-        target = self.target.context
-        if (IProduct.providedBy(target) and
-            target.has_current_commercial_subscription):
-            return True
-        # Branches linked to private bugs can be private.
-        params = BugTaskSearchParams(
-            user=user, linked_branches=self.id,
-            information_type=PRIVATE_INFORMATION_TYPES)
-        bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
-        return bug_ids.count() > 0
-
     @property
     def recipes(self):
         """See `IHasRecipes`."""

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-07-11 09:36:13 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-07-12 21:59:20 +0000
@@ -76,7 +76,10 @@
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
     )
-from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
+from lp.code.interfaces.branchnamespace import (
+    IBranchNamespacePolicy,
+    IBranchNamespaceSet,
+    )
 from lp.code.interfaces.branchrevision import IBranchRevision
 from lp.code.interfaces.codehosting import branch_id_alias
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
@@ -2388,6 +2391,38 @@
         self.assertEqual([], get_policies_for_artifact(branch))
 
 
+class TestBranchGetAllowedInformationTypes(TestCaseWithFactory):
+    """Test Branch.getAllowedInformationTypes."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_normal_user_sees_namespace_types(self):
+        # An unprivileged user sees the types allowed by the namespace.
+        branch = self.factory.makeBranch()
+        policy = IBranchNamespacePolicy(branch.namespace)
+        self.assertContentEqual(
+            policy.getAllowedInformationTypes(),
+            branch.getAllowedInformationTypes(branch.owner))
+        self.assertNotIn(
+            InformationType.PROPRIETARY,
+            branch.getAllowedInformationTypes(branch.owner))
+
+    def test_admin_sees_namespace_types(self):
+        # An admin sees all the types, since they occasionally need to
+        # override the namespace rules. This is hopefully temporary, and
+        # can go away once the new sharing rules (granting
+        # non-commercial projects limited use of private branches) are
+        # deployed.
+        branch = self.factory.makeBranch()
+        admin = self.factory.makeAdministrator()
+        self.assertContentEqual(
+            PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
+            branch.getAllowedInformationTypes(admin))
+        self.assertIn(
+            InformationType.PROPRIETARY,
+            branch.getAllowedInformationTypes(admin))
+
+
 class TestBranchSetPrivate(TestCaseWithFactory):
     """Test IBranch.setPrivate."""
 
@@ -2507,101 +2542,6 @@
             get_policies_for_artifact(branch)[0].type)
 
 
-class TestBranchCanBePrivate(TestCaseWithFactory):
-    """Test IBranch.canBePrivate."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        # Use an admin user as we aren't checking edit permissions here.
-        TestCaseWithFactory.setUp(self, '[e-mail]')
-
-    def test_arbitary_branch(self):
-        # By default branches cannot be private.
-        branch = self.factory.makeBranch()
-        self.assertFalse(branch.canBePrivate(branch.owner))
-
-    def test_admin(self):
-        # Admins can make a branch private.
-        branch = self.factory.makeBranch()
-        admin = getUtility(ILaunchpadCelebrities).admin
-        self.assertTrue(branch.canBePrivate(admin))
-
-    def test_visibility_policy_private(self):
-        # Users with a suitable  visibility policy can make a branch private.
-        team = self.factory.makeTeam(
-            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
-        product = self.factory.makeProduct()
-        product.setBranchVisibilityTeamPolicy(
-            team, BranchVisibilityRule.PRIVATE)
-        branch = self.factory.makeBranch(product=product, owner=team)
-        self.assertTrue(branch.canBePrivate(team))
-
-    def test_private_owner(self):
-        # Private team owners can make a branch private.
-        team = self.factory.makeTeam(
-            visibility=PersonVisibility.PRIVATE,
-            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
-        branch = self.factory.makeBranch(owner=team)
-        self.assertTrue(branch.canBePrivate(team))
-
-    def test_commercial_project(self):
-        # Branches linked to commercial projects can be private.
-        product = self.factory.makeProduct()
-        self.factory.makeCommercialSubscription(product)
-        branch = self.factory.makeProductBranch(product=product)
-        user = self.factory.makePerson()
-        self.assertTrue(branch.canBePrivate(user))
-
-    def test_linked_private_bug(self):
-        # Users with access to linked private bugs can make a branch private.
-        for info_type in PRIVATE_INFORMATION_TYPES:
-            user = self.factory.makePerson()
-            bug = self.factory.makeBug(
-                owner=user, information_type=info_type)
-            branch = self.factory.makeBranch()
-            removeSecurityProxy(bug).linkBranch(branch, user)
-            self.assertTrue(branch.canBePrivate(user))
-
-    def test_linked_public_bug(self):
-        # Users with access to linked public bugs cannot make a branch private.
-        for info_type in PUBLIC_INFORMATION_TYPES:
-            user = self.factory.makePerson()
-            bug = self.factory.makeBug(
-                owner=user, information_type=info_type)
-            branch = self.factory.makeBranch()
-            removeSecurityProxy(bug).linkBranch(branch, user)
-            self.assertFalse(branch.canBePrivate(user))
-
-
-class TestBranchCanBePublic(TestCaseWithFactory):
-    """Test IBranch.canBePublic."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        # Use an admin user as we aren't checking edit permissions here.
-        TestCaseWithFactory.setUp(self, '[e-mail]')
-
-    def test_arbitrary_branch(self):
-        # By default branches can be public.
-        branch = self.factory.makeBranch(
-            information_type=InformationType.USERDATA)
-        self.assertTrue(branch.canBePublic(branch.owner))
-
-    def test_visibility_policy_public_not_allowed(self):
-        # Branches cannot be public if the visibility policy forbids it.
-        team = self.factory.makeTeam(
-            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
-        product = self.factory.makeProduct()
-        product.setBranchVisibilityTeamPolicy(
-            team, BranchVisibilityRule.PRIVATE_ONLY)
-        branch = self.factory.makeBranch(
-            product=product, owner=team,
-            information_type=InformationType.USERDATA)
-        self.assertFalse(branch.canBePublic(team))
-
-
 class TestBranchCommitsForDays(TestCaseWithFactory):
     """Tests for `Branch.commitsForDays`."""