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

[Steve Kowalik]

Show the information_type in the branch UI if a feature flag is set.

In moving towards the death march of having branch use information_type, show it in the branch UI.

Add a new feature flag, show_information_type_in_branch_ui, because I was short-sighted enough to not put bugs_ in the last feature flag. Refactor the information_type portlet methods out from BugView into a separate module and make use of it in both places. I've added new tests for the UI with the feature flag enabled, and have left existing tests alone.

[William Grant (wgrant)]

39 + @property
40 + def show_information_type_in_ui(self):
41 + feature_flag_base = 'disclosure.show_information_type_in_ui.enabled'
42 + if IBug.providedBy(self.context):
43 + pass
44 + elif IBranch.providedBy(self.context):
45 + feature_flag_base = feature_flag_base.replace('ui', 'branch_ui')
46 + else:
47 + raise NotImplemented()
48 + return bool(getFeatureFlag(feature_flag_base))

Since InformationTypePortlet isn't used as its own view (always as a mixin for an edit form class), this horror can probably die or be replaced with "raise NotImplementedError()". It should also possibly be renamed to InformationTypePortletMixin, and the module name should be informationtype, not information_type.

60 + if (
61 + self.context.information_type == InformationType.USERDATA and
62 + self.show_userdata_as_private):

Why is that extra newline there?

if (self.context.information_type == InformationType.USERDATA
    and self.show_userdata_as_private):

71 + if (
72 + self.context.information_type == InformationType.USERDATA and
73 + self.show_userdata_as_private):

Likewise.

107 def initialize(self):
108 super(BugView, self).initialize()

These can be deleted, as the method looks empty apart from that super() call.

433 +class TestBranchPrivacyPortlet(TestCaseWithFactory):

That's a bit odd. Is there really nowhere else that tests the portlet?

485 + This branch contains <strong id="information-type" tal:content="view/information_type"></strong> information&nbsp;
486 + <div id='information-type-description' style='padding-top: 5px' tal:content="view/information_type_description"></div>

The nbsp before a new div doesn't strike me as particularly useful. Do you know why it's there?

review: Approve (code)

=== added file 'lib/lp/app/browser/informationtype.py'
--- lib/lp/app/browser/informationtype.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/informationtype.py	2012-05-30 05:10:23 +0000
@@ -0,0 +1,61 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'InformationTypePortletMixin',
+    ]
+
+from lazr.restful.interfaces import IJSONRequestCache
+
+from lp.registry.enums import (
+    InformationType,
+    PRIVATE_INFORMATION_TYPES,
+    )
+from lp.registry.vocabularies import InformationTypeVocabulary
+from lp.services.features import getFeatureFlag
+
+
+class InformationTypePortletMixin:
+
+    def initialize(self):
+        cache = IJSONRequestCache(self.request)
+        cache.objects['information_types'] = [
+            {'value': term.value, 'description': term.description,
+            'name': term.title,
+            'description_css_class': 'choice-description'}
+            for term in InformationTypeVocabulary()]
+        cache.objects['private_types'] = [
+            type.title for type in PRIVATE_INFORMATION_TYPES]
+        cache.objects['show_information_type_in_ui'] = (
+            self.show_information_type_in_ui)
+
+    @property
+    def show_information_type_in_ui(self):
+        raise NotImplementedError()
+
+    @property
+    def show_userdata_as_private(self):
+        return bool(getFeatureFlag(
+            'disclosure.display_userdata_as_private.enabled'))
+
+    @property
+    def information_type(self):
+        # This can be replaced with just a return when the feature flag is
+        # dropped.
+        title = self.context.information_type.title
+        if (self.context.information_type == InformationType.USERDATA and
+            self.show_userdata_as_private):
+            return 'Private'
+        return title
+
+    @property
+    def information_type_description(self):
+        # This can be replaced with just a return when the feature flag is
+        # dropped.
+        description = self.context.information_type.description
+        if (self.context.information_type == InformationType.USERDATA and
+            self.show_userdata_as_private):
+                description = (
+                    description.replace('user data', 'private information'))
+        return description

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-05-17 09:45:29 +0000
+++ lib/lp/bugs/browser/bug.py	2012-05-30 05:10:23 +0000
@@ -55,6 +55,7 @@
 from zope.security.interfaces import Unauthorized
 
 from lp import _
+from lp.app.browser.informationtype import InformationTypePortletMixin
 from lp.app.browser.launchpadform import (
     action,
     custom_widget,
@@ -518,7 +519,7 @@
         return getUtility(ILaunchBag).bugtask
 
 
-class BugView(LaunchpadView, BugViewMixin):
+class BugView(InformationTypePortletMixin, LaunchpadView, BugViewMixin):
     """View class for presenting information about an `IBug`.
 
     Since all bug pages are registered on IBugTask, the context will be
@@ -535,19 +536,6 @@
         return bool(getFeatureFlag(
             'disclosure.show_information_type_in_ui.enabled'))
 
-    def initialize(self):
-        super(BugView, self).initialize()
-        cache = IJSONRequestCache(self.request)
-        cache.objects['information_types'] = [
-            {'value': term.value, 'description': term.description,
-            'name': term.title,
-            'description_css_class': 'choice-description'}
-            for term in InformationTypeVocabulary()]
-        cache.objects['private_types'] = [
-            type.title for type in PRIVATE_INFORMATION_TYPES]
-        cache.objects['show_information_type_in_ui'] = (
-            self.show_information_type_in_ui)
-
     @cachedproperty
     def page_description(self):
         return IBug(self.context).description
@@ -596,34 +584,6 @@
         return ProxiedLibraryFileAlias(
             attachment.libraryfile, attachment).http_url
 
-    @property
-    def information_type(self):
-        # This can be replaced with just a return when the feature flag is
-        # dropped.
-        title = self.context.information_type.title
-        show_userdata_as_private = bool(getFeatureFlag(
-            'disclosure.display_userdata_as_private.enabled'))
-        if (
-            self.context.information_type == InformationType.USERDATA and
-            show_userdata_as_private):
-            return 'Private'
-        return title
-
-    @property
-    def information_type_description(self):
-        # This can be replaced with just a return when the feature flag is
-        # dropped.
-        description = self.context.information_type.description
-        show_userdata_as_private = bool(getFeatureFlag(
-            'disclosure.display_userdata_as_private.enabled'))
-        if (
-            self.context.information_type == InformationType.USERDATA and
-            show_userdata_as_private):
-                description = (
-                    description.replace('user data', 'private information'))
-
-        return description
-
 
 class BugActivity(BugView):
 

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-03-27 04:36:24 +0000
+++ lib/lp/code/browser/branch.py	2012-05-30 05:10:23 +0000
@@ -75,6 +75,7 @@
 from zope.traversing.interfaces import IPathAdapter
 
 from lp import _
+from lp.app.browser.informationtype import InformationTypePortletMixin
 from lp.app.browser.launchpad import Hierarchy
 from lp.app.browser.launchpadform import (
     action,
@@ -123,10 +124,14 @@
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.productseries import IProductSeries
-from lp.registry.vocabularies import UserTeamsParticipationPlusSelfVocabulary
+from lp.registry.vocabularies import (
+    InformationTypeVocabulary,
+    UserTeamsParticipationPlusSelfVocabulary,
+    )
 from lp.services import searchbuilder
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
+from lp.services.features import getFeatureFlag
 from lp.services.feeds.browser import (
     BranchFeedLink,
     FeedsMixin,
@@ -425,7 +430,8 @@
         return branch.url
 
 
-class BranchView(LaunchpadView, FeedsMixin, BranchMirrorMixin):
+class BranchView(LaunchpadView, FeedsMixin, BranchMirrorMixin,
+                 InformationTypePortletMixin):
 
     feed_types = (
         BranchFeedLink,
@@ -437,7 +443,13 @@
 
     label = page_title
 
+    @property
+    def show_information_type_in_ui(self):
+        return bool(getFeatureFlag(
+            'disclosure.show_information_type_in_branch_ui.enabled'))
+
     def initialize(self):
+        super(BranchView, self).initialize()
         self.branch = self.context
         self.notices = []
         # Cache permission so private team owner can be rendered.
@@ -706,33 +718,38 @@
         self.setFieldError(field_name, structured(message))
 
 
-class BranchEditSchema(Interface):
-    """Defines the fields for the edit form.
-
-    This is necessary so as to make an editable field for the branch privacy.
-    Normally the field is not editable through the interface in order to stop
-    direct setting of the private attribute, but in this case we actually want
-    the user to be able to edit it.
-    """
-    use_template(IBranch, include=[
-        'name',
-        'url',
-        'description',
-        'lifecycle_status',
-        'whiteboard',
-        ])
-    explicitly_private = copy_field(
-        IBranch['explicitly_private'], readonly=False)
-    reviewer = copy_field(IBranch['reviewer'], required=True)
-    owner = copy_field(IBranch['owner'], readonly=False)
-
-
 class BranchEditFormView(LaunchpadEditFormView):
     """Base class for forms that edit a branch."""
 
-    schema = BranchEditSchema
     field_names = None
 
+    @cachedproperty
+    def schema(self):
+        class BranchEditSchema(Interface):
+            """Defines the fields for the edit form.
+
+            This is necessary so as to make an editable field for the
+            branch privacy.  Normally the field is not editable through
+            the interface in order to stop direct setting of the private
+            attribute, but in this case we actually want the user to be
+            able to edit it.
+            """
+            use_template(IBranch, include=[
+                'name',
+                'url',
+                'description',
+                'lifecycle_status',
+                'whiteboard',
+                ])
+            information_type = copy_field(
+                IBranch['information_type'], readonly=False,
+                vocabulary=InformationTypeVocabulary())
+            explicitly_private = copy_field(
+                IBranch['explicitly_private'], readonly=False)
+            reviewer = copy_field(IBranch['reviewer'], required=True)
+            owner = copy_field(IBranch['owner'], readonly=False)
+        return BranchEditSchema
+
     @property
     def page_title(self):
         return 'Edit %s' % self.context.displayname
@@ -744,7 +761,7 @@
     @property
     def adapters(self):
         """See `LaunchpadFormView`"""
-        return {BranchEditSchema: self.context}
+        return {self.schema: self.context}
 
     @action('Change Branch', name='change')
     def change_action(self, action, data):
@@ -767,6 +784,10 @@
         if 'private' in data:
             # Read only for display.
             data.pop('private')
+        if 'information_type' in data:
+            information_type = data.pop('information_type')
+            self.context.transitionToInformationType(
+                information_type, self.user)
         if 'explicitly_private' in data:
             private = data.pop('explicitly_private')
             if (private != self.context.private
@@ -1014,16 +1035,27 @@
 
 
 class BranchEditView(BranchEditFormView, BranchNameValidationMixin):
-    """The main branch view for editing the branch attributes."""
-
-    field_names = [
-        'owner', 'name', 'explicitly_private', 'url', 'description',
-        'lifecycle_status']
+    """The main branch for editing the branch attributes."""
+
+    @property
+    def show_information_type_in_ui(self):
+        return bool(getFeatureFlag(
+            'disclosure.show_information_type_in_branch_ui.enabled'))
+
+    @property
+    def field_names(self):
+        fields = [
+            'owner', 'name', 'explicitly_private', 'url', 'description',
+            'lifecycle_status']
+        if self.show_information_type_in_ui:
+            fields[2] = 'information_type'
+        return fields
 
     custom_widget('lifecycle_status', LaunchpadRadioWidgetWithDescription)
+    custom_widget('information_type', LaunchpadRadioWidgetWithDescription)
 
     def setUpFields(self):
-        LaunchpadFormView.setUpFields(self)
+        super(BranchEditView, self).setUpFields()
         # This is to prevent users from converting push/import
         # branches to pull branches.
         branch = self.context
@@ -1061,7 +1093,8 @@
                 user_has_special_branch_access(self.user))
 
         if not show_private_field:
-            self.form_fields = self.form_fields.omit('explicitly_private')
+            self.form_fields = self.form_fields.omit(
+                'explicitly_private', 'information_type')
 
         # If the user can administer branches, then they should be able to
         # assign the ownership of the branch to any valid person or team.

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2012-03-07 00:58:37 +0000
+++ lib/lp/code/browser/configure.zcml	2012-05-30 05:10:23 +0000
@@ -390,9 +390,11 @@
             name="+index"
             template="../templates/branch-index.pt"/>
         <browser:page
+            name="+portlet-privacy"
+            template="../templates/branch-portlet-privacy.pt"/>
+        <browser:page
             name="++bug-links"
             template="../templates/branch-bug-links.pt"/>
-
         <browser:page
             name="++branch-information"
             template="../templates/branch-information.pt" />
@@ -417,16 +419,12 @@
         <browser:page
             name="++branch-related-bugs-specs"
             template="../templates/branch-related-bugs-specs.pt" />
-
     </browser:pages>
     <browser:pages
         for="lp.code.interfaces.branch.IBranch"
         layer="lp.code.publisher.CodeLayer"
         permission="zope.Public">
         <browser:page
-            name="+portlet-privacy"
-            template="../templates/branch-portlet-privacy.pt"/>
-        <browser:page
             name="+heading"
             template="../templates/branch-heading.pt"/>
     </browser:pages>

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-05-24 02:16:59 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-05-30 05:10:23 +0000
@@ -10,10 +10,12 @@
 
 from BeautifulSoup import BeautifulSoup
 import pytz
+from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.headings import IRootContext
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bugtask import (
     BugTaskStatus,
     UNRESOLVED_BUGTASK_STATUSES,
@@ -37,6 +39,7 @@
 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
 from lp.services.helpers import truncate_text
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
@@ -897,6 +900,39 @@
         with person_logged_in(person):
             self.assertEquals(team, branch.owner)
 
+    def test_information_type_in_ui(self):
+        # The information_type of a branch can be changed via the UI when
+        # the feature flag is enabled.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+        feature_flag = {
+            'disclosure.show_information_type_in_branch_ui.enabled': 'on'}
+        admins = getUtility(ILaunchpadCelebrities).admin
+        admin = admins.teamowner
+        with FeatureFixture(feature_flag):
+            browser = self.getUserBrowser(
+                canonical_url(branch) + '/+edit', user=admin)
+            browser.getControl("Embargoed Security").click()
+            browser.getControl("Change Branch").click()
+        with person_logged_in(person):
+            self.assertEqual(
+                InformationType.EMBARGOEDSECURITY, branch.information_type)
+
+    def test_information_type_in_ui_vocabulary(self):
+        # The vocabulary that Branch:+edit uses for the information_type
+        # has been correctly created.
+        person = self.factory.makePerson()
+        branch = self.factory.makeProductBranch(owner=person)
+        feature_flags = {
+            'disclosure.show_information_type_in_branch_ui.enabled': 'on',
+            'disclosure.proprietary_information_type.disabled': 'on'}
+        admins = getUtility(ILaunchpadCelebrities).admin
+        admin = admins.teamowner
+        with FeatureFixture(feature_flags):
+            browser = self.getUserBrowser(
+                canonical_url(branch) + '/+edit', user=admin)
+            self.assertRaises(LookupError, browser.getControl, "Proprietary")
+
 
 class TestBranchUpgradeView(TestCaseWithFactory):
 
@@ -916,3 +952,48 @@
         self.assertEqual(
             'An upgrade is already in progress for branch %s.' %
             branch.bzr_identity, view.request.notifications[0].message)
+
+
+class TestBranchPrivacyPortlet(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_information_type_in_ui(self):
+        # With the show_information_type_in_branch_ui feature flag on, the
+        # privacy portlet shows the information_type.
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(private=True, owner=owner)
+        feature_flag = {
+            'disclosure.show_information_type_in_branch_ui.enabled': 'on'}
+        with FeatureFixture(feature_flag):
+            with person_logged_in(owner):
+                view = create_initialized_view(branch, '+portlet-privacy')
+                soup = BeautifulSoup(view.render())
+        information_type = soup.find('strong')
+        description = soup.find('div', id='information-type-description')
+        self.assertEqual('User Data', information_type.renderContents())
+        self.assertEqual(
+            'Visible only to users with whom the project has shared '
+            'information\ncontaining user data.\n',
+            description.renderContents())
+
+    def test_information_type_in_ui_with_display_as_private(self):
+        # With both show_information_type_in_branch_ui and
+        # display_userdata_as_private, the information_type is shown with
+        # User Data masked as Private.
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(private=True, owner=owner)
+        feature_flags = {
+            'disclosure.show_information_type_in_branch_ui.enabled': 'on',
+            'disclosure.display_userdata_as_private.enabled': 'on'}
+        with FeatureFixture(feature_flags):
+            with person_logged_in(owner):
+                view = create_initialized_view(branch, '+portlet-privacy')
+                soup = BeautifulSoup(view.render())
+        information_type = soup.find('strong')
+        description = soup.find('div', id='information-type-description')
+        self.assertEqual('Private', information_type.renderContents())
+        self.assertEqual(
+            'Visible only to users with whom the project has shared '
+            'information\ncontaining private information.\n',
+            description.renderContents())

=== modified file 'lib/lp/code/templates/branch-portlet-privacy.pt'
--- lib/lp/code/templates/branch-portlet-privacy.pt	2011-08-24 08:57:25 +0000
+++ lib/lp/code/templates/branch-portlet-privacy.pt	2012-05-30 05:10:23 +0000
@@ -7,6 +7,11 @@
     class python: path('context/private') and 'portlet private' or 'portlet public'
   "
 >
+  <tal:information_type tal:condition="view/show_information_type_in_ui">
+    This branch contains <strong id="information-type" tal:content="view/information_type"></strong> information
+    <div id='information-type-description' style='padding-top: 5px' tal:content="view/information_type_description"></div>
+  </tal:information_type>
+  <tal:privacy tal:condition="not:view/show_information_type_in_ui">
   <div tal:condition="not:context/private"
   >This branch is public</div>
   <div tal:condition="context/private">
@@ -15,4 +20,5 @@
           because it is stacked on a private branch.
       </tal:msg>
   </div>
+  </tal:privacy>
 </div>

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-05-18 05:31:54 +0000
+++ lib/lp/services/features/flags.py	2012-05-30 05:10:23 +0000
@@ -320,6 +320,12 @@
      '',
      '',
      ''),
+    ('disclosure.show_information_type_in_branch_ui.enabled',
+     'boolean',
+     'If true, displays the information_type directly on branch pages.',
+     '',
+     '',
+     ''),
     ])
 
 # The set of all flag names that are documented.