launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/infotype-widget-1007984 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1007984 in Launchpad itself: "BugTask:+index information type widget doesn't handle failure"
  https://bugs.launchpad.net/launchpad/+bug/1007984

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/infotype-widget-1007984/+merge/108665

== Implementation ==

1. Implement the showError and handleError methods on the error handler object.
2. Wire up the spinner.
3. Render both the new value and description test when thepopup widget is clicked and reset back if there is an error.

== Tests ==

Update the relevant yui tests. In particular, add a new test to check that the behaviour when an error occurs is as expected.
Also, the tests as written had an issue in that they were failing to disable the animation on the subscribers portlet. This lead to null exceptions and browser issues when running the tests. So I added code to do the correct thing.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/choice.js
  lib/lp/bugs/javascript/bug_subscription_portlet.js
  lib/lp/bugs/javascript/information_type_choice.js
  lib/lp/bugs/javascript/tests/test_information_type_choice.html
  lib/lp/bugs/javascript/tests/test_information_type_choice.js
-- 
https://code.launchpad.net/~wallyworld/launchpad/infotype-widget-1007984/+merge/108665
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/infotype-widget-1007984 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/choice.js'
--- lib/lp/app/javascript/choice.js	2012-06-04 11:41:47 +0000
+++ lib/lp/app/javascript/choice.js	2012-06-05 08:13:22 +0000
@@ -2,7 +2,7 @@
 
 var namespace = Y.namespace('lp.app.choice');
 
-function hook_up_spinner(widget) {
+namespace.hook_up_choicesource_spinner = function(widget) {
   // ChoiceSource makes assumptions about HTML in lazr-js
   // that don't hold true here, so we need to do our own
   // spinner icon and clear it when finished.
@@ -19,7 +19,7 @@
     icon.addClass('edit');
     icon.setStyle('bottom', '0px');
   }, widget, '_uiClearWaiting');
-}
+};
 
 namespace.addBinaryChoice = function(config, resource_uri, attribute) {
   var widget = new Y.ChoiceSource(config);
@@ -28,7 +28,7 @@
     cfg: {
       patch: attribute,
       resource: resource_uri}});
-  hook_up_spinner(widget);
+  namespace.hook_up_choicesource_spinner(widget);
   widget.render();
 };
 
@@ -41,7 +41,7 @@
     cfg: {
       patch: attribute,
       resource: resource_uri}});
-  hook_up_spinner(widget);
+  namespace.hook_up_choicesource_spinner(widget);
   widget.on('save', function(e) {
       var cb = widget.get('contentBox');
       var value = widget.get('value');

=== modified file 'lib/lp/bugs/javascript/bug_subscription_portlet.js'
--- lib/lp/bugs/javascript/bug_subscription_portlet.js	2012-02-21 22:46:28 +0000
+++ lib/lp/bugs/javascript/bug_subscription_portlet.js	2012-06-05 08:13:22 +0000
@@ -58,7 +58,7 @@
 /*
  * Update the text and link at the top of the subscription portlet.
  */
-function update_subscription_status() {
+function update_subscription_status(skip_animation) {
     var status = Y.one('#current_user_subscription');
     var span = status.one('span');
     var whitespace = status.one('span+span');
@@ -113,7 +113,9 @@
             }
         }
     }
-    Y.lp.anim.green_flash({ node: status }).run();
+    if(!Y.Lang.isBoolean(skip_animation) || !skip_animation) {
+        Y.lp.anim.green_flash({ node: status }).run();
+    }
 }
 namespace.update_subscription_status = update_subscription_status;
 

=== modified file 'lib/lp/bugs/javascript/information_type_choice.js'
--- lib/lp/bugs/javascript/information_type_choice.js	2012-05-30 20:31:37 +0000
+++ lib/lp/bugs/javascript/information_type_choice.js	2012-06-05 08:13:22 +0000
@@ -9,10 +9,25 @@
 var namespace = Y.namespace('lp.bugs.information_type_choice');
 var information_type_descriptions = {};
 
-namespace.save_information_type = function(value, lp_client) {
+// For testing.
+var skip_animation = false;
+
+namespace.save_information_type = function(widget, value, lp_client) {
     var error_handler = new Y.lp.client.ErrorHandler();
+    error_handler.showError = function(error_msg) {
+        Y.lp.app.errors.display_error(
+            Y.one('#information-type'), error_msg);
+    };
+    error_handler.handleError = function(ioId, response) {
+        var orig_value = LP.cache.bug.information_type;
+        widget.set('value', orig_value);
+        update_information_type_description(orig_value);
+        return false;
+    };
     var config = {
         on: {
+            start: Y.bind(widget._uiSetWaiting),
+            end: Y.bind(widget._uiClearWaiting),
             success: function(id, response) {
                 namespace.information_type_save_success(value);
             },
@@ -35,7 +50,6 @@
 };
 
 namespace.get_information_type_banner_text = function(value) {
-    console.dir(LP.cache);
     var fallback_text = "The information on this page is private.";
     var text_template = "This page contains {info_type} information.";
 
@@ -45,7 +59,7 @@
     if (LP.cache.show_information_type_in_ui) {
         return Y.Lang.substitute(text_template, {'info_type': value});
     } else {
-        return fallback_text; 
+        return fallback_text;
     }
 };
 
@@ -54,8 +68,7 @@
     var private_type = (Y.Array.indexOf(LP.cache.private_types, value) >= 0);
     var subscription_ns = Y.lp.bugs.bugtask_index.portlets.subscription;
     var privacy_banner = Y.lp.app.banner.privacy.getPrivacyBanner();
-    subscription_ns.update_subscription_status();
-    update_information_type_description(value);
+    subscription_ns.update_subscription_status(skip_animation);
     if (private_type) {
         var banner_text = namespace.get_information_type_banner_text(value);
         privacy_banner.updateText(banner_text);
@@ -67,7 +80,9 @@
     }
 };
 
-namespace.setup_information_type_choice = function(privacy_link, lp_client) {
+namespace.setup_information_type_choice = function(privacy_link, lp_client,
+                                                   skip_anim) {
+    skip_animation = skip_anim;
     Y.Array.each(LP.cache.information_types, function(info_type) {
         information_type_descriptions[info_type.value] = info_type.description;
     });
@@ -81,6 +96,7 @@
         items: LP.cache.information_types,
         backgroundColor: '#FFFF99'
     });
+    Y.lp.app.choice.hook_up_choicesource_spinner(information_type_edit);
     information_type_edit.render();
     information_type_edit.on("save", function(e) {
         var value = information_type_edit.get('value');
@@ -89,10 +105,13 @@
         if (value === 'Private') {
             value = 'User Data';
         }
-        namespace.save_information_type(value, lp_client);
+        update_information_type_description(value);
+        namespace.save_information_type(
+            information_type_edit, value, lp_client);
     });
     privacy_link.addClass('js-action');
+    return information_type_edit;
 };
 }, "0.1", {"requires": ["base", "oop", "node", "event", "io-base",
                         "lazr.choiceedit", "lp.bugs.bugtask_index",
-                        "lp.app.banner.privacy"]});
+                        "lp.app.banner.privacy", "lp.app.choice"]});

=== modified file 'lib/lp/bugs/javascript/tests/test_information_type_choice.html'
--- lib/lp/bugs/javascript/tests/test_information_type_choice.html	2012-05-16 02:09:30 +0000
+++ lib/lp/bugs/javascript/tests/test_information_type_choice.html	2012-06-05 08:13:22 +0000
@@ -28,6 +28,8 @@
       <script type="text/javascript"
           src="../../../../../build/js/lp/app/lp.js"></script>
       <script type="text/javascript"
+          src="../../../../../build/js/lp/app/choice.js"></script>
+      <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>

=== modified file 'lib/lp/bugs/javascript/tests/test_information_type_choice.js'
--- lib/lp/bugs/javascript/tests/test_information_type_choice.js	2012-05-30 13:59:24 +0000
+++ lib/lp/bugs/javascript/tests/test_information_type_choice.js	2012-06-05 08:13:22 +0000
@@ -25,10 +25,12 @@
                     show_information_type_in_ui: true,
                     private_types: ['Private'],
                     information_types: [
-                        {'value': 'Public', 'description': 'Public',
-                            'name': 'Public'},
-                        {'value': 'Private', 'description': 'Private',
-                            'name': 'Private'}
+                        {'value': 'Public', 'name': 'Public',
+                            'description': 'Public Description'},
+                        {'value': 'Private', 'name': 'Private',
+                            'description': 'Private Description'},
+                        {'value': 'User Data', 'name': 'User Data',
+                            'description': 'User Data Description'}
                     ]
                 }
             };
@@ -39,7 +41,8 @@
 
             var lp_client = new Y.lp.client.Launchpad();
             var privacy_link = Y.one('#privacy-link');
-            ns.setup_information_type_choice(privacy_link, lp_client);
+            this.widget = ns.setup_information_type_choice(
+                privacy_link, lp_client, true);
         },
         tearDown: function () {
             if (this.fixture !== null) {
@@ -75,7 +78,7 @@
             var lp_client = new Y.lp.client.Launchpad({
                 io_provider: mockio
             });
-            ns.save_information_type('User Data', lp_client);
+            ns.save_information_type(this.widget, 'User Data', lp_client);
             Y.Assert.areEqual('/api/devel/bug/1', mockio.last_request.url);
             Y.Assert.areEqual(
                 'ws.op=transitionToInformationType&' +
@@ -84,10 +87,6 @@
         },
 
         test_information_type_save_success_private: function() {
-            var description_node = Y.one('#information-type-description');
-            Y.Assert.areEqual(
-                'Everyone can see this information.',
-                description_node.get('text'));
             var old_func = this._shim_privacy_banner();
             var hide_flag = false;
             var update_flag = false;
@@ -103,7 +102,6 @@
             Y.Assert.isTrue(body.hasClass('private'));
             Y.Assert.isTrue(hide_flag);
             Y.Assert.isTrue(update_flag);
-            Y.Assert.areEqual('Private', description_node.get('text'));
             this._unshim_privacy_banner(old_func);
         },
 
@@ -118,8 +116,6 @@
             var body = Y.one('body');
             Y.Assert.isTrue(body.hasClass('public'));
             Y.Assert.isTrue(flag);
-            var description_node = Y.one('#information-type-description');
-            Y.Assert.areEqual('Public', description_node.get('text'));
             this._unshim_privacy_banner(old_func);
         },
 
@@ -130,12 +126,36 @@
                 '.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) {
+            ns.save_information_type = function(widget, value, lp_client) {
                 Y.Assert.areEqual(
                     'User Data', value); function_called = true; };
             private_choice.simulate('click');
+            var description_node = Y.one('#information-type-description');
+            Y.Assert.areEqual(
+                'User Data Description', description_node.get('text'));
             Y.Assert.isTrue(function_called);
             ns.save_information_type = orig_save_information_type;
+        },
+
+        test_information_type_save_error: function() {
+            var mockio = new Y.lp.testing.mockio.MockIo();
+            var lp_client = new Y.lp.client.Launchpad({
+                io_provider: mockio
+            });
+            this.widget.set('value', 'User Data');
+            ns.save_information_type(this.widget, 'User Data', lp_client);
+            mockio.last_request.respond({
+                status: 500,
+                statusText: 'An error occurred'
+            });
+            // The original info type value from the cache should have been
+            // set back into the widget.
+            Y.Assert.areEqual('Public', this.widget.get('value'));
+            var description_node = Y.one('#information-type-description');
+            Y.Assert.areEqual(
+                'Public Description', description_node.get('text'));
+            // The error was displayed.
+            Y.Assert.isNotNull(Y.one('.yui3-lazr-formoverlay-errors'));
         }
     }));