Merge lp:~stevenk/launchpad/bugs-information_type-ui-portlet into lp:launchpad

[Steve Kowalik]

Show information_type in the privacy portlet and allow it to be edited via JS if a feature flag is set.

Building on the changes introduced in https://code.launchpad.net/~stevenk/launchpad/bugs-information_type-ui-filebug/+merge/102635, and the march forward, show the information_type in the privacy portlet on Bug:+index.

This branch sort of closes bug 249112, but I'd rather not do so until the feature flag is enabled on production.

As before, I have left the existing tests and added new ones. I have done a drive-by that corrects a capitalization failure introduced by a previous branch.

[Ian Booth (wallyworld)]

Looks good but I would prefer the code in the callbacks (info type click and xhr success) to be broken out as separate functions. This achieves the correct separation of concerns, allows for much better, cleaner unit tests (eg code to make xhr call tested standalone without having to invoke ui code, code to test ui update after success tested without having to make xhr call etc), and allows for subsequent refactoring to a more mvc style approach. One of the reasons bugtask_index is untested is because it didn't take this more modular approach and it would be a shame to continue old habits from the past. The core logic is all there, it just needs some minor changes and 2 extra tests. Strictly speaking there should also be a test for the setup function. Also, the new test_information_type_choice.js file doesn't follow the test template eg it's missing the test_library_exists test.

eg

namespace.setup_information_type_choice = function() {
  information_type_edit.on("save", function(e) {
      namespace.perform_info_type_save(
        information_type_edit.get('value'));
  });
}

namespace.perform_info_type_save = function(value) {
  var config = {
    on: {
     success: function(id, response) {
         namespace.info_type_save_success();
     }
    }
    ......
  };
  lp_client.named_post(xxxxx);
};

namespace.info_type_save_success = function() {
    subscription_ns.update_subscription_status();
    if (private_type) {
      body.replaceClass('public', 'private');
      privacy_ns.display_privacy_notification();
    } else {
      body.replaceClass('private', 'public');
      privacy_ns.hide_privacy_notification();
    }
}

Unit tests would monkey patch the relevant functions to ensure they are called as expected. This is a common pattern in our current yui tests.

review: Needs Fixing

[Ian Booth (wallyworld)]

Thanks very much for making the suggested fixes.

review: Approve

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-04-19 05:08:15 +0000
+++ lib/lp/bugs/browser/bug.py	2012-04-24 07:48:19 +0000
@@ -553,6 +553,11 @@
     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)
@@ -560,7 +565,9 @@
             {'value': term.value, 'description': term.description,
             'name': term.title} for term in InformationTypeVocabulary()]
         cache.objects['private_types'] = [
-            type.name for type in PRIVATE_INFORMATION_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):
@@ -865,7 +872,7 @@
         label = 'Bug #%i - Set ' % self.context.bug.id
         if bool(getFeatureFlag(
             'disclosure.show_information_type_in_ui.enabled')):
-            label += 'Information type'
+            label += 'information type'
         else:
             label += 'visibility and security'
         return label

=== modified file 'lib/lp/bugs/browser/tests/test_bugview.py'
--- lib/lp/bugs/browser/tests/test_bugview.py	2012-04-11 06:01:04 +0000
+++ lib/lp/bugs/browser/tests/test_bugview.py	2012-04-24 07:48:19 +0000
@@ -3,6 +3,7 @@
 
 __metaclass__ = type
 
+from lazr.restful.interfaces import IJSONRequestCache
 from zope.security.proxy import removeSecurityProxy
 
 from lp.bugs.browser.bug import BugView
@@ -73,3 +74,19 @@
         with FeatureFixture(feature_flag):
             view = BugView(self.bug, LaunchpadTestRequest())
             self.assertEqual('Private', view.information_type)
+
+    def test_proprietary_hidden(self):
+        # When the proprietary_information_type.disabled feature flag is
+        # enabled, it isn't in the JSON request cache.
+        feature_flag = {
+            'disclosure.proprietary_information_type.disabled': 'on'}
+        with FeatureFixture(feature_flag):
+            view = BugView(self.bug, LaunchpadTestRequest())
+            view.initialize()
+            cache = IJSONRequestCache(view.request)
+            expected = [
+                InformationType.PUBLIC, InformationType.UNEMBARGOEDSECURITY,
+                InformationType.EMBARGOEDSECURITY, InformationType.USERDATA]
+            self.assertContentEqual(expected, [
+                type['value']
+                for type in cache.objects['information_types']])

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2012-04-06 17:28:25 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2012-04-24 07:48:19 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+/* Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Form overlay widgets and subscriber handling for bug pages.
@@ -117,7 +117,10 @@
 
         privacy_link = Y.one('#privacy-link');
 
-        if (privacy_link) {
+        if (privacy_link && LP.cache.show_information_type_in_ui) {
+            Y.lp.bugs.information_type_choice.setup_information_type_choice(
+                privacy_link, lp_client);
+        } else if (privacy_link) {
             var privacy_link_url = privacy_link.getAttribute('href') +
               '/++form++';
             var privacy_div = Y.one('#privacy-text');
@@ -1521,6 +1524,7 @@
                         "lazr.formoverlay", "lp.anim", "lazr.base",
                         "lazr.overlay", "lazr.choiceedit", "lp.app.picker",
                         "lp.bugs.bugtask_index.portlets.subscription",
+                        "lp.bugs.information_type_choice",
                         "lp.app.widgets.expander", "lp.client", "escape",
                         "lp.client.plugins", "lp.app.errors",
                         "lp.app.privacy", "lp.app.confirmationoverlay"]});

=== added file 'lib/lp/bugs/javascript/information_type_choice.js'
--- lib/lp/bugs/javascript/information_type_choice.js	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/information_type_choice.js	2012-04-24 07:48:19 +0000
@@ -0,0 +1,69 @@
+/* Copyright 2012 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Information Type choice widget for bug pages.
+ */
+
+YUI.add('lp.bugs.information_type_choice', function(Y) {
+
+var namespace = Y.namespace('lp.bugs.information_type_choice');
+
+namespace.save_information_type = function(value, lp_client) {
+    var error_handler = new Y.lp.client.ErrorHandler();
+    var config = {
+        on: {
+            success: function(id, response) {
+                namespace.information_type_save_success(value);
+            },
+            failure: error_handler.getFailureHandler()
+        },
+        parameters: {
+            information_type: value
+        }
+    };
+    lp_client.named_post(
+        LP.cache.bug.self_link, 'transitionToInformationType', config);
+};
+
+namespace.information_type_save_success = function(value) {
+    var body = Y.one('body');
+    var private_type = (Y.Array.indexOf(LP.cache.private_types, value) >= 0);
+    var subscription_ns = Y.lp.bugs.bugtask_index.portlets.subscription;
+    var privacy_ns = Y.lp.app.privacy;
+    subscription_ns.update_subscription_status();
+    if (private_type) {
+        body.replaceClass('public', 'private');
+        privacy_ns.display_privacy_notification();
+    } else {
+        body.replaceClass('private', 'public');
+        privacy_ns.hide_privacy_notification();
+    }
+};
+
+namespace.setup_information_type_choice = function(privacy_link, lp_client) {
+    var information_type = Y.one('#information-type');
+    var information_type_edit = new Y.ChoiceSource({
+        editicon: privacy_link,
+        contentBox: Y.one('#privacy'),
+        value_location: information_type,
+        value: LP.cache.bug.information_type,
+        title: "Change information type",
+        items: LP.cache.information_types,
+        backgroundColor: '#FFFF99'
+    });
+    information_type_edit.render();
+    information_type_edit.on("save", function(e) {
+        var value = information_type_edit.get('value');
+        // This is required due to the display_userdata_as_private feature
+        // flag.
+        if (value === 'Private') {
+            value = 'User Data';
+        }
+        namespace.save_information_type(value, lp_client);
+    });
+    privacy_link.addClass('js-action');
+};
+
+}, "0.1", {"requires": ["base", "oop", "node", "event", "io-base",
+                        "lazr.choiceedit", "lp.app.privacy",
+                        "lp.bugs.bugtask_index"]});

=== added file 'lib/lp/bugs/javascript/tests/test_information_type_choice.html'
--- lib/lp/bugs/javascript/tests/test_information_type_choice.html	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_information_type_choice.html	2012-04-24 07:48:19 +0000
@@ -0,0 +1,80 @@
+<!DOCTYPE html>
+<!--
+Copyright 2012 Canonical Ltd.  This software is licensed under the
+GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<html>
+  <head>
+      <title>lp.bugs.information_type_choice Tests</title>
+
+      <!-- YUI and test setup -->
+      <script type="text/javascript"
+              src="../../../../../build/js/yui/yui/yui.js">
+      </script>
+      <link rel="stylesheet"
+      href="../../../../../build/js/yui/console/assets/console-core.css" />
+      <link rel="stylesheet"
+      href="../../../../../build/js/yui/console/assets/skins/sam/console.css" />
+      <link rel="stylesheet"
+      href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
+
+      <script type="text/javascript"
+              src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
+
+      <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+
+      <!-- Dependencies -->
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/testing/mockio.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/client.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/extras/extras.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/anim/anim.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/effects/effects.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/lazr/lazr.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/choiceedit/choiceedit.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/privacy.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/overlay/overlay.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/expander.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/errors.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/bugs/bug_subscription_portlet.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/bugs/bugtask_index.js"></script>
+
+      <!-- The module under test. -->
+      <script type="text/javascript" src="../information_type_choice.js"></script>
+
+      <!-- Placeholder for any css asset for this module. -->
+      <!-- <link rel="stylesheet" href="../assets/bugs.information_type_choice-core.css" /> -->
+
+      <!-- The test suite -->
+      <script type="text/javascript" src="test_information_type_choice.js"></script>
+
+    </head>
+    <body class="yui3-skin-sam">
+        <ul id="suites">
+            <li>lp.bugs.information_type_choice.test</li>
+        </ul>
+        <div id="privacy">
+            <div id="privacy-text">
+                This report contains information that is <strong id="information-type">Public</strong><a class="sprite edit" id="privacy-link" href="#">edit</a>
+            </div>
+        </div>
+        <div id="current_user_subscription">
+            <span></span>
+            <a href="#" text="" class="menu-link-mute_subscription unmute"></a>
+            <a href="#" text="" class="menu-link-mute_subscription mute"></a>
+        </div>
+    </body>
+</html>

=== added file 'lib/lp/bugs/javascript/tests/test_information_type_choice.js'
--- lib/lp/bugs/javascript/tests/test_information_type_choice.js	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_information_type_choice.js	2012-04-24 07:48:19 +0000
@@ -0,0 +1,106 @@
+/* Copyright (c) 2012, Canonical Ltd. All rights reserved. */
+
+YUI.add('lp.bugs.information_type_choice.test', function (Y) {
+
+    var tests = Y.namespace('lp.bugs.information_type_choice.test');
+    var ns = Y.lp.bugs.information_type_choice;
+    tests.suite = new Y.Test.Suite('lp.bugs.information_type_choice Tests');
+
+    tests.suite.add(new Y.Test.Case({
+        name: 'lp.bugs.information_type_choice_tests',
+
+        setUp: function() {
+            window.LP = {
+                cache: {
+                    context: {
+                        web_link: ''
+                    },
+                    notifications_text: {
+                        muted: ''
+                    },
+                    bug: {
+                        information_type: 'Public',
+                        self_link: '/bug/1'
+                    },
+                    show_information_type_in_ui: true,
+                    private_types: ['Private'],
+                    information_types: [
+                        {'value': 'Public', 'description': 'Public',
+                            'name': 'Public'},
+                        {'value': 'Private', 'description': 'Private',
+                            'name': 'Private'}
+                    ]
+                }
+            };
+        },
+        tearDown: function () {},
+
+        test_library_exists: function () {
+            Y.Assert.isObject(Y.lp.bugs.information_type_choice,
+                "Cannot locate the lp.bugs.information_type_choice module");
+        },
+
+        test_save_information_type: function() {
+            var mockio = new Y.lp.testing.mockio.MockIo();
+            var lp_client = new Y.lp.client.Launchpad({
+                io_provider: mockio
+            });
+            ns.save_information_type('User Data', lp_client);
+            Y.Assert.areEqual('/api/devel/bug/1', mockio.last_request.url);
+            Y.Assert.areEqual(
+                'ws.op=transitionToInformationType&' +
+                'information_type=User%20Data',
+                mockio.last_request.config.data);
+        },
+
+        test_information_type_save_success_private: function() {
+            var privacy_ns = Y.lp.app.privacy;
+            var orig_function = privacy_ns.display_privacy_notification;
+            var function_called = false;
+            privacy_ns.display_privacy_notification = function() {
+                function_called = true;
+            };
+            ns.information_type_save_success('Private');
+            var body = Y.one('body');
+            Y.Assert.isTrue(body.hasClass('private'));
+            Y.Assert.isTrue(function_called);
+            privacy_ns.display_privacy_notification = orig_function;
+        },
+
+        test_information_type_save_success_public: function() {
+            var privacy_ns = Y.lp.app.privacy;
+            var orig_function = privacy_ns.hide_privacy_notification;
+            var function_called = false;
+            privacy_ns.hide_privacy_notification = function() {
+                function_called = true;
+            };
+            ns.information_type_save_success('Public');
+            var body = Y.one('body');
+            Y.Assert.isTrue(body.hasClass('public'));
+            Y.Assert.isTrue(function_called);
+            privacy_ns.hide_privacy_notification = orig_function;
+        },
+
+        test_perform_update_information_type: function() {
+            var lp_client = new Y.lp.client.Launchpad();
+            var privacy_link = Y.one('#privacy-link');
+            var information_type = Y.one('#information-type');
+            ns.setup_information_type_choice(privacy_link, lp_client);
+            privacy_link.simulate('click');
+            var private_choice = Y.one(
+                '.yui3-ichoicelist-content a[href=#Private]');
+            var orig_save_information_type = ns.save_information_type;
+            var function_called = false;
+            ns.save_information_type = function(value, lp_client) {
+                Y.Assert.areEqual('User Data', value);
+                function_called = true;
+            };
+            private_choice.simulate('click');
+            Y.Assert.isTrue(function_called);
+            ns.save_information_type = orig_save_information_type;
+        }
+    }));
+
+}, '0.1', {'requires': ['test', 'console', 'event', 'node-event-simulate',
+        'lp.testing.mockio', 'lp.client', 'lp.bugs.information_type_choice',
+        'lp.app.privacy']});

=== modified file 'lib/lp/bugs/templates/bug-portlet-privacy.pt'
--- lib/lp/bugs/templates/bug-portlet-privacy.pt	2009-09-18 09:11:17 +0000
+++ lib/lp/bugs/templates/bug-portlet-privacy.pt	2012-04-24 07:48:19 +0000
@@ -8,22 +8,29 @@
   "
   tal:define="link context/menu:context/visibility"
 >
-  <div tal:condition="not:context/private" id="privacy-text">
-    <a class="sprite edit" id="privacy-link"
-       tal:attributes="href link/path; title link/text"
-       tal:condition="link/enabled">This report is public</a>
-    <tal:unchangeable condition="not:link/enabled">
-      This report is public
-    </tal:unchangeable>
-  </div>
-  <div tal:condition="context/private" id="privacy-text">
-    <a class="sprite edit" id="privacy-link"
-      tal:attributes="href link/path; title link/text"
-      tal:condition="link/enabled">This report is <strong>private</strong></a>
-    <tal:unchangeable condition="not:link/enabled">
-      This report is private
-    </tal:unchangeable>
-  </div>
-  <div class="sprite security" id="security-message"
-       tal:condition="context/security_related">Security vulnerability</div>
+  <div id="privacy-text">
+    <tal:information_type tal:condition="view/show_information_type_in_ui">
+      This report contains information that is <strong id="information-type" tal:content="view/information_type"></strong>&nbsp;<a class="sprite edit" id="privacy-link" tal:attributes="href link/path" tal:condition="link/enabled"></a>
+    </tal:information_type>
+    <tal:privacy tal:condition="not:view/show_information_type_in_ui">
+      <div tal:condition="not:context/private" id="privacy-text">
+        <a class="sprite edit" id="privacy-link"
+          tal:attributes="href link/path; title link/text"
+          tal:condition="link/enabled">This report is public</a>
+        <tal:unchangeable condition="not:link/enabled">
+          This report is public
+        </tal:unchangeable>
+      </div>
+      <div tal:condition="context/private" id="privacy-text">
+        <a class="sprite edit" id="privacy-link"
+          tal:attributes="href link/path; title link/text"
+          tal:condition="link/enabled">This report is <strong>private</strong></a>
+        <tal:unchangeable condition="not:link/enabled">
+          This report is private
+        </tal:unchangeable>
+      </div>
+      <div class="sprite security" id="security-message"
+        tal:condition="context/security_related">Security vulnerability</div>
+    </tal:privacy>
+  </div>
 </div>