launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/branch-information_type-ui into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/branch-information_type-ui/+merge/107909

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.
-- 
https://code.launchpad.net/~stevenk/launchpad/branch-information_type-ui/+merge/107909
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/branch-information_type-ui into lp:launchpad.

=== added file 'lib/lp/app/browser/information_type.py'
--- lib/lp/app/browser/information_type.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/information_type.py	2012-05-30 03:28:28 +0000
@@ -0,0 +1,72 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'InformationTypePortlet',
+    ]
+
+from lazr.restful.interfaces import IJSONRequestCache
+
+from lp.bugs.interfaces.bug import IBug
+from lp.code.interfaces.branch import IBranch
+from lp.registry.enums import (
+    InformationType,
+    PRIVATE_INFORMATION_TYPES,
+    )
+from lp.registry.vocabularies import InformationTypeVocabulary
+from lp.services.features import getFeatureFlag
+
+
+class InformationTypePortlet:
+
+    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):
+        feature_flag_base = 'disclosure.show_information_type_in_ui.enabled'
+        if IBug.providedBy(self.context):
+            pass
+        elif IBranch.providedBy(self.context):
+            feature_flag_base = feature_flag_base.replace('ui', 'branch_ui')
+        else:
+            raise NotImplemented()
+        return bool(getFeatureFlag(feature_flag_base))
+
+    @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 03:28:28 +0000
@@ -55,6 +55,7 @@
 from zope.security.interfaces import Unauthorized
 
 from lp import _
+from lp.app.browser.information_type import InformationTypePortlet
 from lp.app.browser.launchpadform import (
     action,
     custom_widget,
@@ -518,7 +519,7 @@
         return getUtility(ILaunchBag).bugtask
 
 
-class BugView(LaunchpadView, BugViewMixin):
+class BugView(InformationTypePortlet, LaunchpadView, BugViewMixin):
     """View class for presenting information about an `IBug`.
 
     Since all bug pages are registered on IBugTask, the context will be
@@ -530,23 +531,8 @@
     all the pages off IBugTask instead of IBug.
     """
 
-    @property
-    def show_information_type_in_ui(self):
-        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):
@@ -596,34 +582,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 03:28:28 +0000
@@ -75,6 +75,7 @@
 from zope.traversing.interfaces import IPathAdapter
 
 from lp import _
+from lp.app.browser.information_type import InformationTypePortlet
 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,
+                 InformationTypePortlet):
 
     feed_types = (
         BranchFeedLink,
@@ -438,6 +444,7 @@
     label = page_title
 
     def initialize(self):
+        super(BranchView, self).initialize()
         self.branch = self.context
         self.notices = []
         # Cache permission so private team owner can be rendered.
@@ -723,6 +730,8 @@
         ])
     explicitly_private = copy_field(
         IBranch['explicitly_private'], readonly=False)
+    information_type = copy_field(
+        IBranch['information_type'], readonly=False)
     reviewer = copy_field(IBranch['reviewer'], required=True)
     owner = copy_field(IBranch['owner'], readonly=False)
 
@@ -767,6 +776,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,13 +1027,24 @@
 
 
 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)
@@ -1030,6 +1054,13 @@
         if branch.branch_type in (BranchType.HOSTED, BranchType.IMPORTED):
             self.form_fields = self.form_fields.omit('url')
 
+        if self.show_information_type_in_ui:
+            self.form_fields = self.form_fields.omit('information_type')
+            information_type_field = copy_field(
+                IBranch['information_type'], readonly=False,
+                vocabulary=InformationTypeVocabulary())
+            self.form_fields += form.Fields(information_type_field)
+
         policy = IBranchNamespacePolicy(branch.namespace)
         if branch.private:
             # If the branch is private, and can be public, show the field.
@@ -1061,7 +1092,11 @@
                 user_has_special_branch_access(self.user))
 
         if not show_private_field:
-            self.form_fields = self.form_fields.omit('explicitly_private')
+            if self.show_information_type_in_ui:
+                omit = 'information_type'
+            else:
+                omit = 'explicitly_private'
+            self.form_fields = self.form_fields.omit(omit)
 
         # 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 03:28:28 +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 03:28:28 +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 03:28:28 +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&nbsp;
+    <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 03:28:28 +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.