launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-846163 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #846163 in Launchpad itself: "bug filing notice is removed before the user can read it"
  https://bugs.launchpad.net/launchpad/+bug/846163

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-846163/+merge/79013

Bug 846163 describes the "Thank you for your bug report." notification
disappearing just as the page loads.  This behavior is caused by the
REST client removing all notifications at the end of a successful
request regardless of whether or not there are new notifications to
display.

This branch changes the behavior such that notifications are only
removed if an explicitly specified empty list of notifications is
returned in a response.  This leaves us the ability to clear
notifications if so desired but keeps them from being cleared
accidentally.

Tests: The notification display tests were modified to demonstrate the
new behavior.  They can be run by loading this file in a web browser:
lib/lp/app/javascript/tests/test_lp_client.html.

Demo/QA: file a new bug.  The thank you message should stay visible.

Lint: the "make lint" report is clear.

-- 
https://code.launchpad.net/~benji/launchpad/bug-846163/+merge/79013
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-846163 into lp:launchpad.

=== modified file 'lib/lp/app/browser/launchpadform.py'
--- lib/lp/app/browser/launchpadform.py	2011-09-26 13:28:23 +0000
+++ lib/lp/app/browser/launchpadform.py	2011-10-11 17:37:25 +0000
@@ -147,11 +147,8 @@
         notifications = ([(notification.level, notification.message)
              for notification in request.response.notifications])
         if notifications:
-            json_notifications = simplejson.dumps(notifications)
-        else:
-            json_notifications = simplejson.dumps(None)
-        request.response.setHeader(
-            'X-Lazr-Notifications', json_notifications)
+            request.response.setHeader(
+                'X-Lazr-Notifications', simplejson.dumps(notifications))
 
     def render(self):
         """Return the body of the response.

=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js	2011-08-30 11:07:14 +0000
+++ lib/lp/app/javascript/client.js	2011-10-11 17:37:25 +0000
@@ -327,8 +327,12 @@
         var media_type = response.getResponseHeader('Content-Type');
         if (media_type.substring(0,16) === 'application/json') {
             representation = Y.JSON.parse(response.responseText);
-            module.display_notifications(
-                    response.getResponseHeader('X-Lazr-Notifications'));
+            // If the response contains a notification header, display the
+            // notifications.
+            notificaxns = response.getResponseHeader('X-Lazr-Notifications');
+            if (notificaxns !== null) {
+                module.display_notifications(notificaxns);
+            }
             wrapped = client.wrap_resource(uri, representation);
             result = old_on_success(wrapped);
             if (update_cache) {

=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js	2011-09-15 09:53:59 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js	2011-10-11 17:37:25 +0000
@@ -374,22 +374,33 @@
     },
 
     test_remove_notifications: function() {
+        // Make some notifications that will be removed.
         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);
+
+        // If the notifications header is just the string "null", then the
+        // current notifications are removed.
         this.response.setResponseHeader('X-Lazr-Notifications', "null");
         Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
         this._checkNoNotificationNode('.debug.message');
         this._checkNoNotificationNode('.informational.message');
-        notifications = '[ [10, "A debug"], [20, "An info"] ]';
+    },
+
+    test_notifications_not_removed: function() {
+        // Make some notifications that will be removed.
+        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);
+
+        // If the response does not include a notifications header, then any
+        // pre-existing notifiactions are not removed.
         this.response.setResponseHeader('X-Lazr-Notifications', null);
         Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
-        this._checkNoNotificationNode('.debug.message');
-        this._checkNoNotificationNode('.informational.message');
+        this._checkNotificationNode('.debug.message', 'A debug');
+        this._checkNotificationNode('.informational.message', 'An info');
     }
 
 }));