Merge lp:~wallyworld/launchpad/show-ajax-notifications into lp:launchpad

-----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_messag...

Hi Henning

Thanks for the review.

> 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.
>

No problem. I'm happy to discuss this mp with you. I'm also relatively
inexperienced with javascript.

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

Oh dear. What was I smoking.

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

It may help to know that the javascript in this mp is designed to
replicate the existing functionality provided by the "notifications"
macro defined in base-layout-macros.pt. The tales macros provide the
functionality for when page loads occur and this mp provides the same
functionality for when inline edits occur.

>> + 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. ?
>
>

These css class names are already defined in the tales macro. However,
they are actually "error message", "warning message" etc. The code works
as is since the matching infrastructure looks for "error.*" etc. There's
issues with constructing a jquery selector for class names with spaces
that I haven't worked out yet. I meant to do this before the mp was
submitted but forgot. Thanks for pointing this out. I'll fix it.

>> + // 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
>

The implementation in this mp is as per the existing tales macro and
where it places the various divs containing the messages.

 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.
>

The debug, warning, info, error messages are displayed one under the
other inside the "context-publication" 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, 'i...

I've now implemented the requested changes.

> >> + 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. ?
> >
> >
>
> These css class names are already defined in the tales macro. However,
> they are actually "error message", "warning message" etc. The code works
> as is since the matching infrastructure looks for "error.*" etc. There's
> issues with constructing a jquery selector for class names with spaces
> that I haven't worked out yet. I meant to do this before the mp was
> submitted but forgot. Thanks for pointing this out. I'll fix it.

Thanks for the fix. I still think that the names are pretty generic but I guess that is a general problem and we should define a global "lp-" prefix. I just read about YUI's support for automatic class prefixes.

>
>
> >> + // 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;
> >> + };
> The debug, warning, info, error messages are displayed one under the
> other inside the "context-publication" div.

Sorry, I had not looked at what the "publication" diff is. But to correct you, they are displayed *under* the publication. That is obviously the right place for them, so all is fine here.

> > 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.
> >
>
> Perhaps. It's fairly easy to understand as is and setting stuff up so it
> can be done in a loop will maybe add more boilerplate around it all.
> I'll see what can be done.

Ah, I see you introduced notification_info. That is a good pattern and you could have followed that from the start. ;-) Have a look at this branch to see what I have in mind, although I have not run the code yet.
lp:~henninge/launchpad/wallyworld-show-ajax-notifications-refactor

I don't insist on you using this but it would be more consistent.

That's all. Thank you for your patience. I really learned from this review.

Henning