launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-security-portlet-js-errors-841511 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #841511 in Launchpad itself: "javascript error when changing a bug's security status"
  https://bugs.launchpad.net/launchpad/+bug/841511

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-security-portlet-js-errors-841511/+merge/74045

When the bug task privacy portlet was updated using the popup privacy form, the javascript to update the privacy ribbon caused errors in the console.

== Implementation ==

There appeared to be some redundant code in bugtask_index.js - the calls to display/hide the privacy ribbon were simplified and the code cleaned up. Instead of bugtask_index.js calling setup_privacy_notification(), the display/hide methods were modified to call it the first time either is invoked. So users of the privacy ribbon simple call [display|hide]_privacy_notification() as required and everything is taken care of.

Some of the "redundant" code that was removed was:

                    if (privacy_notification_enabled) {
                        if (notification === null) {
                            Y.lp.app.privacy.setup_privacy_notification();
                        }
                        if (notification.hasClass('hidden')) {
                            Y.one('.portlet.private').setStyles({
                                color: '#333',
                                backgroundColor: '#fbfbfb'
                            });
                        }
                    }

The above code was called if a private bug was made pubic. The "if (notification.hasClass('hidden'))" condition would never be true so the styles would never be set. Before this branch was started, "notification" was null which caused the initial js error. Fixing that removed the null error but the code would still not be executed.

The behaviour as implemented is:

1. Privacy ribbon disabled

Use the grey stripped strip along the top edge of the privacy portlet and down the lhs of the page to indicate a bug is private.

2. Privacy ribbon enabled

Use the new red privacy ribbon across the top of the page.


== Tests ==

The test_privacy.js yui scripts were reworked: the manual set up of the notification node was removed and setUp() modified to call Y.lp.app.privacy._reset_privacy_notification() between tests. The html node used to attach the privacy ribbon was renamed to fit with the default used by the privacy setup code.

bin/test -vvc --layer=YUITest

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/privacy.js
  lib/lp/app/javascript/tests/test_privacy.html
  lib/lp/app/javascript/tests/test_privacy.js
  lib/lp/bugs/javascript/bugtask_index.js

-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-security-portlet-js-errors-841511/+merge/74045
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-security-portlet-js-errors-841511 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/privacy.js'
--- lib/lp/app/javascript/privacy.js	2011-08-10 20:57:56 +0000
+++ lib/lp/app/javascript/privacy.js	2011-09-05 07:15:25 +0000
@@ -1,6 +1,8 @@
 YUI.add('lp.app.privacy', function(Y) {
 
 var namespace = Y.namespace('lp.app.privacy');
+
+var notification_node = null;
 /*
  * Display privacy notifications
  *
@@ -8,6 +10,8 @@
  */
 
 function setup_privacy_notification(config) {
+    if (notification_node !== null)
+        return;
     var notification_text = 'The information on this page is private';
     var hidden = true;
     var target_id = "maincontent";
@@ -24,21 +28,28 @@
     }
     var id_selector = "#" + target_id;
     var main = Y.one(id_selector);
-    var notification = Y.Node.create('<div></div>')
+    notification_node = Y.Node.create('<div></div>')
         .addClass('global-notification');
     if (hidden) {
-        notification.addClass('hidden');
+        notification_node.addClass('hidden');
     }
     var notification_span = Y.Node.create('<span></span>')
         .addClass('sprite')
         .addClass('notification-private');
-    notification.set('text', notification_text);
+    notification_node.set('text', notification_text);
 
-    main.appendChild(notification);
-    notification.appendChild(notification_span);
+    main.appendChild(notification_node);
+    notification_node.appendChild(notification_span);
 }
 namespace.setup_privacy_notification = setup_privacy_notification;
 
+/**
+ * For unit tests - we need to reset the notification setup.
+ */
+namespace._reset_privacy_notification = function () {
+    notification_node = null;
+};
+
 function display_privacy_notification() {
     /* Set a temporary class on the body for the feature flag,
      this is because we have no way to use feature flags in
@@ -49,6 +60,7 @@
     /* Set the visible flag so that the content moves down. */
     body.addClass('global-notification-visible');
 
+    setup_privacy_notification();
     var global_notification = Y.one('.global-notification');
     if (global_notification.hasClass('hidden')) {
         global_notification.addClass('transparent');
@@ -85,6 +97,7 @@
  * This should be called after the page has loaded e.g. on 'domready'.
  */
 function hide_privacy_notification() {
+    setup_privacy_notification();
     if (!Y.one('.global-notification').hasClass('hidden')) {
         var fade_out = new Y.Anim({
             node: '.global-notification',

=== modified file 'lib/lp/app/javascript/tests/test_privacy.html'
--- lib/lp/app/javascript/tests/test_privacy.html	2011-07-29 14:32:29 +0000
+++ lib/lp/app/javascript/tests/test_privacy.html	2011-09-05 07:15:25 +0000
@@ -21,6 +21,6 @@
   </head>
   <body class="yui3-skin-sam">
     <!-- The example markup required by the script to run -->
-  <div id="notification-area"> </div>
+  <div id="maincontent"> </div>
   </body>
 </html>

=== modified file 'lib/lp/app/javascript/tests/test_privacy.js'
--- lib/lp/app/javascript/tests/test_privacy.js	2011-08-10 20:57:56 +0000
+++ lib/lp/app/javascript/tests/test_privacy.js	2011-09-05 07:15:25 +0000
@@ -15,13 +15,14 @@
         name: 'privacy',
 
         _reset_container: function () {
+            Y.lp.app.privacy._reset_privacy_notification();
             var body = Y.one(document.body);
 
             // Replace the container.
-            var container = Y.one('#notification-area');
+            var container = Y.one('#maincontent');
             container.remove(true);
             container = Y.Node.create('<div></div>')
-                .set('id', 'notification-area');
+                .set('id', 'maincontent');
             body.appendChild(container);
             return container;
         },
@@ -31,18 +32,7 @@
             var container = this._reset_container();
             var login_logout = Y.Node.create('<div></div>')
                 .addClass('login-logout');
-            var notification = Y.Node.create('<div></div>')
-                .addClass('global-notification')
-                .addClass('hidden');
-            var notification_span = Y.Node.create('<span></span>')
-                .addClass('sprite')
-                .addClass('notification-private');
-
-            notification.set('text', "This bug is private");
-
             container.appendChild(login_logout);
-            container.appendChild(notification);
-            notification.appendChild(notification_span);
         },
 
         test_setup: function() {
@@ -61,15 +51,14 @@
         },
 
         test_display_shows_ribbon: function () {
+            Y.lp.app.privacy.display_privacy_notification();
             var ribbon = Y.one('.global-notification');
-            Y.Assert.isTrue(ribbon.hasClass('hidden'));
-            Y.lp.app.privacy.display_privacy_notification();
             Y.Assert.isFalse(ribbon.hasClass('hidden.'));
         },
 
         test_hide_removes_ribbon: function () {
+            Y.lp.app.privacy.display_privacy_notification();
             var ribbon = Y.one('.global-notification');
-            Y.lp.app.privacy.display_privacy_notification();
             Y.Assert.isFalse(ribbon.hasClass('hidden'));
             Y.lp.app.privacy.hide_privacy_notification(false);
 

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-09-02 01:01:49 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-09-05 07:15:25 +0000
@@ -386,7 +386,6 @@
                 privacy_spinner.setStyle('display', 'none');
                 privacy_link.setStyle('display', 'inline');
 
-                var notification = Y.one('.global-notification');
                 if (private_flag) {
                     Y.one('body').replaceClass('public', 'private');
                     privacy_div.replaceClass('public', 'private');
@@ -394,23 +393,9 @@
                         'innerHTML',
                         'This report is <strong>private</strong> ');
                     if (privacy_notification_enabled) {
-                        if (notification === null) {
-                            Y.lp.app.privacy.setup_privacy_notification();
-                        }
                         Y.lp.app.privacy.display_privacy_notification();
                     }
                 } else {
-                    if (privacy_notification_enabled) {
-                        if (notification === null) {
-                            Y.lp.app.privacy.setup_privacy_notification();
-                        }
-                        if (notification.hasClass('hidden')) {
-                            Y.one('.portlet.private').setStyles({
-                                color: '#333',
-                                backgroundColor: '#fbfbfb'
-                            });
-                        }
-                    }
                     Y.one('body').replaceClass('private', 'public');
                     privacy_div.replaceClass('private', 'public');
                     privacy_text.set(
@@ -1113,7 +1098,7 @@
 namespace.load_more_comments = function(batched_comments_url,
                                         comments_container) {
     var spinner = Y.Node.create(
-        '<img src="/@@/spinner" style="text_align: center; display: none" />');
+        '<img src="/@@/spinner" style="text_align: center; display: none"/>');
     var spinner_span = Y.one('#more-comments-spinner');
     spinner_span.setStyle('display', 'inline');
     var handlers = {
@@ -1142,7 +1127,7 @@
         }
     };
     var request = Y.io(batched_comments_url, {on: handlers});
-}
+};
 
 
 /**