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

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

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.

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

There's a generic mocking framework that could be adapted to suit. But
what I needed is very simple - just basic implementations of
set/getResponseHeader methods. So using the prototype capability served
the purpose with minimal lines of code.

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

As you say, I wanted to show different types can be displayed in one
request. Hence 4 individual tests were not suitable. It just seemed
easier to do it all in one given the small line count.

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

Of course! I've asked Benji to review it.