Code review comment for lp:~wallyworld/launchpad/show-ajax-notifications

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Ian,
I am doing this review although I am not an experienced Javascript reviewer or
coder. I am hoping, hat this will help to change that. ;-) Feel free to
request another review.

A note on the MP itself: You mixed up commit message and the description. You
should swap that back. ;-)

I will mark this needs-information for now just because I have so many
questions. ;-)

 review needs-information

Cheers,
Henning

Am 28.03.2011 04:09, schrieb Ian Booth:
> === modified file 'lib/canonical/launchpad/webapp/configure.zcml'
> === modified file 'lib/canonical/launchpad/webapp/servers.py'
> === modified file 'lib/lp/app/javascript/client.js'
> --- lib/lp/app/javascript/client.js 2011-02-25 01:54:00 +0000
> +++ lib/lp/app/javascript/client.js 2011-03-28 01:50:57 +0000
> @@ -242,6 +242,8 @@
> var media_type = response.getResponseHeader('Content-Type');
> if (media_type.substring(0,16) == 'application/json') {
> representation = Y.JSON.parse(response.responseText);
> + display_notifications(
> + response.getResponseHeader('X-Lazr-Notifications'));
> wrapped = client.wrap_resource(uri, representation);
> result = old_on_success(wrapped);
> if (update_cache) {
> @@ -254,6 +256,73 @@
> }
> };
>
> +/**
> + * Display a list of notifications - error, warning, informational or debug.
> + * @param notifications An json encoded array of (level, message) tuples.
> + */
> +function display_notifications(notifications) {
> + if (notifications === undefined)
> + return;
> +
> + // First remove any existing notifications.
> + function remove_notifications(div_class) {
> + var nodes = Y.all('div.'+div_class);
> + nodes.each(function(node) {
> + var parent = node.get('parentNode');
> + parent.removeChild(node);
> + });
> + }
> + remove_notifications('error');
> + remove_notifications('warning');
> + remove_notifications('informational');
> + remove_notifications('debug');

I am a bit worried that this might accidentally remove more than wanted. Class
names like "error" and "warning" seem so general that somebody might use them
locally. How about "lp-error", "lp-warning" etc. ?

> +
> + // Now display the new ones.
> + notifications = Y.JSON.parse(notifications);
> + var warnings = new Array();
> + var errors = new Array();
> + var infos = new Array();
> + var debugs = new Array();
> +
> + Y.each(notifications, function(notification, key) {
> + var level = notification[0];
> + var message = notification[1];
> + switch (level) {
> + case 10:
> + debugs.push(message);
> + break;
> + case 20:
> + infos.push(message);
> + break;
> + case 30:
> + warnings.push(message);
> + break;
> + case 40:
> + errors.push(message);
> + break;
> + }
> + });
> + // The place where we want to insert the notification divs.
> + var last_message = Y.one('div.context-publication');
> + var display_notification = function(notification, div_class) {
> + var node = Y.Node.create("<div class='"+div_class+"'/>");
> + node.set('innerHTML', notification);
> + last_message.insert(node, 'after');
> + last_message = node;
> + };

Since the cover letter is missing the pre-imp information, I have to ask:
Why append these here? I would have expected that the template defines a place
by naming a special div, e.g. <div class="lp-notifications">. They are placed
very loosely on the page. Or do you expect that warnings might displayed in a
different place than, say debug messages?
Also, putting them in a div would make it easy to implement the removal by
simply removing the content of that div.

> + Y.each(errors, function(notification, key) {
> + display_notification(notification, 'error message');
> + });
> + Y.each(warnings, function(notification, key) {
> + display_notification(notification, 'warning message');
> + });
> + Y.each(infos, function(notification, key) {
> + display_notification(notification, 'informational message');
> + });
> + Y.each(debugs, function(notification, key) {
> + display_notification(notification, 'debug message');
> + });
> +}

I wonder if all these repetitions could not be placed in loops in some way.
That would also make it easier to add more levels, if needed.

>
> // The resources that come together to make Launchpad.
>
>
> === modified file 'lib/lp/app/javascript/tests/test_lp_client.html'
> === modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
> --- lib/lp/app/javascript/tests/test_lp_client.js 2011-02-25 02:51:27 +0000
> +++ lib/lp/app/javascript/tests/test_lp_client.js 2011-03-28 01:50:57 +0000
> @@ -171,6 +171,53 @@
> }
> }));
>
> +function MockHttpResponse () {
> + this.responseText = '[]';
> + this.responseHeaders = {};
> +}
> +
> +MockHttpResponse.prototype = {
> + setResponseHeader: function (header, value) {
> + this.responseHeaders[header] = value;
> + },
> +
> + getResponseHeader: function(header) {
> + return this.responseHeaders[header];
> + }
> +};

I am *really* surprised that a mock request is not part of the testing frame
work. This is my first time looking at JS testing so I don't know if there is
good reason for it.

> +
> +suite.add(new Y.Test.Case({
> + name: "lp.client.notifications",
> +
> + setUp: function() {
> + this.client = new Y.lp.client.Launchpad();
> + this.args=[this.client, null, this._on_success, false];
> + this.response = new MockHttpResponse();
> + this.response.setResponseHeader('Content-Type', 'application/json');
> + },
> +
> + _on_success: function(entry) {
> + },
> +
> + _checkNotificationNode: function(node_class, node_text) {
> + var node = Y.one('div.'+node_class);
> + Assert.areEqual(node_text, node.get("innerHTML"));
> + },
> +
> + test_display_notifications: function() {
> + var notifications = '[ [10, "A debug"], [20, "An info"], ' +
> + '[30, "A warning"], [40, "An error"] ]';
> + this.response.setResponseHeader(
> + 'X-Lazr-Notifications', notifications);
> + Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
> + this._checkNotificationNode('debug', 'A debug');
> + this._checkNotificationNode('informational', 'An info');
> + this._checkNotificationNode('warning', 'A warning');
> + this._checkNotificationNode('error', 'An error');
> + }

If this was a python unit test, I'd ask you to put these into four tests. Any
reason not to do that here? If you want, you could then leave this test in
there (or reduce it to two notifications) to show that different types can be
displayed in one request.

> +
> +}));
> +
>
> // Lock, stock, and two smoking barrels.
> var handle_complete = function(data) {
>
> === modified file 'versions.cfg'
> --- versions.cfg 2011-03-23 16:28:51 +0000
> +++ versions.cfg 2011-03-28 01:50:57 +0000
> @@ -34,7 +34,7 @@
> lazr.delegates = 1.2.0
> lazr.enum = 1.1.2
> lazr.lifecycle = 1.1
> -lazr.restful = 0.18.0
> +lazr.restful = 0.18.1

It is my understanding that your change to lazr.restful is not yet approved or
even landed, right. That would be 0.18.1. Just make sure that is landed before
you land this.

> lazr.restfulclient = 0.11.2
> lazr.smtptest = 1.1
> lazr.testing = 0.1.1

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2Qaw0ACgkQBT3oW1L17ij8kgCg5mIR6/gloDOTHFqMfN+h0xm4
fLgAoNo5NsksXhazpUp5jOyUyh6UgPIc
=HsZl
-----END PGP SIGNATURE-----