launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #933766 in Launchpad itself: "Update bug to use information_visibility_policy"
  https://bugs.launchpad.net/launchpad/+bug/933766

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/bugs-information_type-ui-secrecy/+merge/102624

Add a new feature flag, disclosure.show_information_type_in_ui.enabled, and make use of it inside Bug:+secrecy. I've been quite careful with the changes I've made and I have left the existing tests alone, and added new ones that enable the feature flag and make sure we can change the information_type and that the vocab is used correctly.

I've also cleaned up a little bit of lint, and made sure that InformationTypeVocabulary works with the widget we're using.
-- 
https://code.launchpad.net/~stevenk/launchpad/bugs-information_type-ui-secrecy/+merge/102624
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/bugs-information_type-ui-secrecy into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-04-11 07:10:12 +0000
+++ lib/lp/bugs/browser/bug.py	2012-04-19 05:13:28 +0000
@@ -623,6 +623,7 @@
             return 'Private'
         return title
 
+
 class BugActivity(BugView):
 
     page_title = 'Activity log'
@@ -861,23 +862,53 @@
 
     @property
     def label(self):
-        return 'Bug #%i - Set visibility and security' % self.context.bug.id
+        label = 'Bug #%i - Set ' % self.context.bug.id
+        if bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            label += 'Information type'
+        else:
+            label += 'visibility and security'
+        return label
 
     page_title = label
 
-    class schema(Interface):
-        """Schema for editing secrecy info."""
-        private_field = copy_field(IBug['private'], readonly=False)
-        security_related_field = copy_field(
-            IBug['security_related'], readonly=False)
+    @property
+    def field_names(self):
+        if bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            return ['information_type']
+        else:
+            return ['private', 'security_related']
+
+    custom_widget('information_type', LaunchpadRadioWidgetWithDescription)
+
+    @property
+    def schema(self):
+        """Schema for editing the information type of a `IBug`."""
+        class privacy_schema(Interface):
+            private_field = copy_field(IBug['private'], readonly=False)
+            security_related_field = copy_field(
+                IBug['security_related'], readonly=False)
+
+        class information_type_schema(Interface):
+            information_type_field = copy_field(
+                IBug['information_type'], readonly=False,
+                vocabulary=InformationTypeVocabulary())
+        if bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            return information_type_schema
+        else:
+            return privacy_schema
 
     def setUpFields(self):
         """See `LaunchpadFormView`."""
         super(BugSecrecyEditView, self).setUpFields()
-        bug = self.context.bug
-        if (bug.information_type == InformationType.PROPRIETARY
-            and len(bug.affected_pillars) > 1):
-            self.form_fields = self.form_fields.omit('private')
+        if not bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            bug = self.context.bug
+            if (bug.information_type == InformationType.PROPRIETARY
+                and len(bug.affected_pillars) > 1):
+                self.form_fields = self.form_fields.omit('private')
 
     @property
     def next_url(self):
@@ -891,29 +922,32 @@
     @property
     def initial_values(self):
         """See `LaunchpadFormView.`"""
-        return {'private': self.context.bug.private,
+        if bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            return {'information_type': self.context.bug.information_type}
+        else:
+            return {
+                'private': self.context.bug.private,
                 'security_related': self.context.bug.security_related}
 
     @action('Change', name='change')
     def change_action(self, action, data):
         """Update the bug."""
-        # We will modify data later, so take a copy now.
         data = dict(data)
-
-        # We handle privacy changes by hand instead of leaving it to
-        # the usual machinery because we must use
-        # bug.transitionToInformationType() to ensure auditing information is
-        # recorded.
         bug = self.context.bug
-        private = data.pop('private', bug.private)
+        if bool(getFeatureFlag(
+            'disclosure.show_information_type_in_ui.enabled')):
+            information_type = data.pop('information_type')
+        else:
+            private = data.pop('private', bug.private)
+            security_related = data.pop('security_related')
+            information_type = convert_to_information_type(
+                private, security_related)
         user_will_be_subscribed = (
-            private and bug.getSubscribersForPerson(self.user).is_empty())
-        security_related = data.pop('security_related')
-        user = getUtility(ILaunchBag).user
-        # This will change when the UI does.
-        information_type = convert_to_information_type(
-            private, security_related)
-        changed = bug.transitionToInformationType(information_type, user)
+            information_type in PRIVATE_INFORMATION_TYPES and
+            bug.getSubscribersForPerson(self.user).is_empty())
+        changed = bug.transitionToInformationType(
+            information_type, self.user)
         if changed:
             self._handlePrivacyChanged(user_will_be_subscribed)
         if self.request.is_ajax:

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2012-04-04 05:46:26 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2012-04-19 05:13:28 +0000
@@ -406,6 +406,38 @@
         with person_logged_in(owner):
             self.assertTrue(bug.security_related)
 
+    def test_set_information_type(self):
+        # Test that the bug's information_type can be updated using the
+        # view with the feature flag on.
+        bug = self.factory.makeBug()
+        feature_flag = {
+            'disclosure.show_information_type_in_ui.enabled': 'on'}
+        with FeatureFixture(feature_flag):
+            with person_logged_in(bug.owner):
+                view = create_initialized_view(
+                    bug.default_bugtask, name='+secrecy', form={
+                        'field.information_type': 'USERDATA',
+                        'field.actions.change': 'Change'})
+        self.assertEqual([], view.errors)
+        self.assertEqual(InformationType.USERDATA, bug.information_type)
+
+    def test_information_type_vocabulary(self):
+        # Test that the view creates the vocabulary correctly.
+        bug = self.factory.makeBug()
+        feature_flags = {
+            'disclosure.show_information_type_in_ui.enabled': 'on',
+            'disclosure.proprietary_information_type.disabled': 'on',
+            'disclosure.display_userdata_as_private.enabled': 'on'}
+        with FeatureFixture(feature_flags):
+            with person_logged_in(bug.owner):
+                view = create_initialized_view(
+                    bug.default_bugtask, name='+secrecy',
+                    principal=bug.owner)
+                html = view.render()
+        self.assertIn('Private', html)
+        self.assertNotIn('User Data', html)
+        self.assertNotIn('Proprietary', html)
+
 
 class TestBugTextViewPrivateTeams(TestCaseWithFactory):
     """ Test for rendering BugTextView with private team artifacts.

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2012-04-16 23:55:51 +0000
+++ lib/lp/bugs/interfaces/bug.py	2012-04-19 05:13:28 +0000
@@ -214,7 +214,7 @@
     information_type = exported(
         Choice(
             title=_('Information Type'), vocabulary=InformationType,
-            required=False, readonly=True,
+            required=True, readonly=True, default=InformationType.PUBLIC,
             description=_(
                 'The type of information contained in this bug report.')))
 

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-04-12 07:28:33 +0000
+++ lib/lp/registry/vocabularies.py	2012-04-19 05:13:28 +0000
@@ -65,6 +65,7 @@
 
 from operator import attrgetter
 
+from lazr.enum import IEnumeratedType
 from lazr.restful.interfaces import IReference
 from lazr.restful.utils import safe_hasattr
 from sqlobject import (
@@ -2228,6 +2229,8 @@
 
 class InformationTypeVocabulary(SimpleVocabulary):
 
+    implements(IEnumeratedType)
+
     def __init__(self):
         types = [
             InformationType.PUBLIC,

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-04-18 22:54:23 +0000
+++ lib/lp/services/features/flags.py	2012-04-19 05:13:28 +0000
@@ -339,6 +339,13 @@
      '',
      '',
      ''),
+    ('disclosure.show_information_type_in_ui.enabled',
+     'boolean',
+     ('If true, displays the information_type directly in the UI when '
+      'filing a bug and changing the information_type.'),
+     '',
+     '',
+     ''),
     ])
 
 # The set of all flag names that are documented.