Merge lp:~wgrant/launchpad/edit-stacked-information-type into lp:launchpad

[William Grant]

Stop special-casing private stacked-on branches so much on Branch:+edit.

This branch simplifies BranchEditView's handling of the information type of branches stacked on private branches. Previously it replaced the usual radio buttons with a disabled checkbox, and some text describing why it's locked. I've integrated this into the normal information type widget, so if the stacked-on branch is private it'll display only (branch.stacked_on.information_type, branch.information_type), allowing the user to make it consistent by switching to the matching type.

There's no replacement for the warning text describing why the choices are restricted, but the case is uncommon enough that we probably don't care. It's already restricted by branch visibility policies without a corresponding warning.

[Ian Booth (wallyworld)]

Looks great.

review: Approve

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-07-12 21:56:46 +0000
+++ lib/lp/code/browser/branch.py	2012-07-13 01:31:19 +0000
@@ -48,9 +48,7 @@
 from lazr.uri import URI
 import pytz
 import simplejson
-from zope.app.form import CustomWidgetFactory
 from zope.app.form.browser import TextAreaWidget
-from zope.app.form.browser.boolwidgets import CheckBoxWidget
 from zope.component import (
     getUtility,
     queryAdapter,
@@ -124,11 +122,7 @@
 from lp.code.interfaces.branchcollection import IAllBranches
 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,
-    )
+from lp.registry.enums import InformationType
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.vocabularies import (
@@ -1033,34 +1027,7 @@
 
     def setUpFields(self):
         super(BranchEditView, self).setUpFields()
-        # This is to prevent users from converting push/import
-        # branches to pull branches.
         branch = self.context
-        if branch.private:
-            # If this branch is stacked on a private branch, render some text
-            # to inform the user the information type cannot be changed.
-            if (branch.stacked_on and branch.stacked_on.information_type in
-                PRIVATE_INFORMATION_TYPES):
-                stacked_info_type = branch.stacked_on.information_type.title
-                private_info = Bool(
-                    __name__="private",
-                    title=_("Branch is %s" % stacked_info_type),
-                    description=_(
-                        "This branch is %(info_type)s because it is "
-                        "stacked on a %(info_type)s branch." % {
-                            'info_type': stacked_info_type}))
-                private_info_field = form.Fields(
-                    private_info, render_context=self.render_context)
-                self.form_fields = (private_info_field
-                    + self.form_fields.omit('information_type'))
-                new_field_names = self.field_names
-                index = new_field_names.index('information_type')
-                new_field_names[index] = 'private'
-                self.form_fields = self.form_fields.select(*new_field_names)
-                self.form_fields['private'].custom_widget = (
-                    CustomWidgetFactory(
-                        CheckBoxWidget, extra='disabled="disabled"'))
-
         # If the user can administer branches, then they should be able to
         # assign the ownership of the branch to any valid person or team.
         if check_permission('launchpad.Admin', branch):
@@ -1109,27 +1076,33 @@
         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
+
+        # If we're stacked on a private branch, only show that
+        # information type.
+        if self.context.stacked_on and self.context.stacked_on.private:
+            shown_types = set([self.context.stacked_on.information_type])
+        else:
+            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)

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-07-12 09:13:16 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-07-13 01:31:19 +0000
@@ -36,10 +36,7 @@
     BranchVisibilityRule,
     )
 from lp.registry.enums import InformationType
-from lp.registry.interfaces.person import (
-    PersonVisibility,
-    TeamSubscriptionPolicy,
-    )
+from lp.registry.interfaces.person import PersonVisibility
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
@@ -985,6 +982,23 @@
         self.assertShownTypes(
             [InformationType.PUBLIC, InformationType.PROPRIETARY], branch)
 
+    def test_stacked_on_private(self):
+        # A branch stacked on a private branch has its choices limited
+        # to the current type and the stacked-on type.
+        product = self.factory.makeProduct()
+        stacked_on_branch = self.factory.makeBranch(
+            product=product, information_type=InformationType.USERDATA)
+        branch = self.factory.makeBranch(
+            product=product, stacked_on=stacked_on_branch,
+            owner=product.owner,
+            information_type=InformationType.EMBARGOEDSECURITY)
+        with admin_logged_in():
+            branch.product.setBranchVisibilityTeamPolicy(
+                branch.owner, BranchVisibilityRule.PRIVATE)
+        self.assertShownTypes(
+            [InformationType.EMBARGOEDSECURITY, InformationType.USERDATA],
+            branch)
+
     def test_private_branch(self):
         # Branches on projects with a private policy can be set to
         # User Data (aka. Private)