launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/improper-notification-removal into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #754058 in Launchpad itself: "post bug filing notification is cleared by an ajax request a few seconds after page load"
  https://bugs.launchpad.net/launchpad/+bug/754058

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/improper-notification-removal/+merge/56855

After an xhr patch request, existing page notifications were being removed so that any new ones could be displayed in a clean context. However, this broke pages which issued several xhr calls during initial loading.

== Implementation ==

Make removal of notifications via javascript explicit. There is a lp_client remove_notifications() method which can be called when required.

== Tests ==

Modify existing lp_client javascript test plus add one new test:
  test_display_notifications()
  test_remove_notifications()

-- 
https://code.launchpad.net/~wallyworld/launchpad/improper-notification-removal/+merge/56855
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/improper-notification-removal into lp:launchpad.

=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js	2011-04-06 16:59:07 +0000
+++ lib/lp/app/javascript/client.js	2011-04-07 22:34:31 +0000
@@ -260,6 +260,25 @@
     }
 };
 
+var NOTIFICATION_INFO = {
+    'level10': {
+        'selector': '.debug.message',
+        'css_class': 'debug message'
+    },
+    'level20': {
+        'selector': '.informational.message',
+        'css_class': 'informational message'
+    },
+    'level30': {
+        'selector': '.warning.message',
+        'css_class': 'warning message'
+    },
+    'level40': {
+        'selector': '.error.message',
+        'css_class': 'error message'
+    }
+};
+
 /**
  * Display a list of notifications - error, warning, informational or debug.
  * @param notifications An json encoded array of (level, message) tuples.
@@ -268,53 +287,36 @@
     if (notifications === undefined)
         return;
 
-    var notification_info = {
+    var notifications_by_level = {
         'level10': {
-            'notifications': new Array(),
-            'selector': '.debug.message',
-            'css_class': 'debug message'
+            'notifications': new Array()
         },
         'level20': {
-            'notifications': new Array(),
-            'selector': '.informational.message',
-            'css_class': 'informational message'
+            'notifications': new Array()
         },
         'level30': {
-            'notifications': new Array(),
-            'selector': '.warning.message',
-            'css_class': 'warning message'
+            'notifications': new Array()
         },
         'level40': {
-            'notifications': new Array(),
-            'selector': '.error.message',
-            'css_class': 'error message'
+            'notifications': new Array()
         }
     };
 
-    // First remove any existing notifications.
-    Y.each(notification_info, function (info) {
-        var nodes = Y.all('div#request-notifications div'+info.selector);
-        nodes.each(function(node) {
-            var parent = node.get('parentNode');
-            parent.removeChild(node);
-        });
-    });
-
-    // Now display the new ones.
+    // Extract the notifications from the json.
     notifications = Y.JSON.parse(notifications);
     Y.each(notifications, function(notification, key) {
         var level = notification[0];
         var message = notification[1];
-        var info = notification_info['level'+level];
-        info.notifications.push(message);
+        notifications_by_level['level'+level].notifications.push(message);
     });
 
     // The place where we want to insert the notification divs.
     var last_message = null;
     // A mapping from the div class to notification messages.
-    Y.each(notification_info, function(info) {
+    Y.each(notifications_by_level, function(info, key) {
         Y.each(info.notifications, function(notification) {
-            var node = Y.Node.create("<div class='"+info.css_class+"'/>");
+            var css_class = NOTIFICATION_INFO[key].css_class;
+            var node = Y.Node.create("<div class='"+css_class+"'/>");
             node.set('innerHTML', notification);
             if (last_message==null) {
                 var div = Y.one('div#request-notifications');
@@ -327,6 +329,19 @@
     });
 }
 
+/**
+ * Remove any notifications that are currently displayed.
+ */
+module.remove_notifications = function() {
+    Y.each(NOTIFICATION_INFO, function (info) {
+        var nodes = Y.all('div#request-notifications div'+info.selector);
+        nodes.each(function(node) {
+            var parent = node.get('parentNode');
+            parent.removeChild(node);
+        });
+    });
+};
+
 // The resources that come together to make Launchpad.
 
 // A hosted file resource.

=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js	2011-04-05 12:10:01 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js	2011-04-07 22:34:31 +0000
@@ -233,16 +233,25 @@
         this._checkNotificationNode('.debug.message', 'A debug');
         this._checkNotificationNode('.informational.message', 'An info');
 
-        // Check that existing notifications are removed when a new request is
-        // received.
+        // Any subsequent request should preserve existing notifications.
         var new_notifications = '[ [30, "A warning"], [40, "An error"] ]';
         this.response.setResponseHeader(
                 'X-Lazr-Notifications', new_notifications);
         Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
+        this._checkNotificationNode('.debug.message', 'A debug');
+        this._checkNotificationNode('.informational.message', 'An info');
+        this._checkNotificationNode('.warning.message', 'A warning');
+        this._checkNotificationNode('.error.message', 'An error');
+    },
+
+    test_remove_notifications: function() {
+        var notifications = '[ [10, "A debug"], [20, "An info"] ]';
+        this.response.setResponseHeader(
+                'X-Lazr-Notifications', notifications);
+        Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
+        Y.lp.client.remove_notifications();
         this._checkNoNotificationNode('.debug.message');
         this._checkNoNotificationNode('.informational.message');
-        this._checkNotificationNode('.warning.message', 'A warning');
-        this._checkNotificationNode('.error.message', 'An error');
     }
 
 }));