Code review comment for lp:~wallyworld/launchpad/delete-bugtask-ui-ajax-878909

Hi Ian,

Excellent work. Thanks for the detailed MP and all the comments you put in the code, I really appreciate it.

A few remarks/suggestions. I suppose you're quite eager to land this so please take the nitpicks (especially the ones about indentation) with a grain of salt ;).

[0]

1249 lines. Please have mercy on us poor reviewers ;). I know Javascript is verbose and you've done a lot of refactoring but still, I'm sure you would have had a volunteer reviewer for this branch sooner if you had split it into 2 or 3. Also, the problem with enormous branches is that the odds of getting a conflict goes way up.

> 3. Bug found
> There were conflicting implementations of userCanEditImportance() found. One was essentially:
> bugtask.userCanEditImportance(self.user) and not bugtask.bugwatch

For instance you could have fixed this in another pipe (I don't know if you already use the bzr pipeline plugin but in case you don't, please check it out: http://wiki.bazaar.canonical.com/BzrPipeline).

[1]

294 + """ Test that the client cache contains the expected data.
295 +
296 + The cache data is used by the Javascript to enable the delete
297 + links to work as expected.
298 + """

Weird indentation for the second """.

[2]

312 + def check_bugtask_data(bugtask, can_delete):
313 + self.assertIn(bugtask.id, all_bugtask_data)

Don't you think it would be easier to read if you moved this method outside of the test method?

[3]

361 + self.assertEqual(
362 + view.request.response.getHeader('content-type'),
363 + 'application/json')

'application/json' should be the first argument, otherwise a failure will be difficult to interpret.

[4]

334 + bug = self.factory.makeBug()
335 + bugtask = self.factory.makeBugTask(bug=bug)
336 + target_name = bugtask.bugtargetdisplayname
337 + bugtask_url = canonical_url(bugtask, rootsite='bugs')

And

370 + bug = self.factory.makeBug()
371 + bugtask = self.factory.makeBugTask(bug=bug)
372 + target_name = bugtask.bugtargetdisplayname
373 + default_bugtask_url = canonical_url(
374 + bug.default_bugtask, rootsite='bugs')

Would you mind refactoring that code into a common setup method?

[5]

517 + if( bugtask_data.hasOwnProperty(id) ) {

small typos: if( bugtask_data.hasOwnProperty(id) ) { → if (bugtask_data.hasOwnProperty(id)) {

Same here:

532 + if( link.hasClass('js-action') ) {
537 + if( Y.Lang.isFunction(connect_func)) {
629 + if( content_type === 'application/json' ) {

[6]

591 + var delete_text = [
592 + '<p class="large-warning" style="padding:2px 2px 0 36px;">',
593 + 'You are about to mark bug "',
594 + conf.bug_title,
595 + '"<br>as no longer affecting ',
596 + conf.targetname,
597 + '.<br><br><strong>Please confirm you really want to do this.',
598 + '</strong></p>'
599 + ].join('');

Well, I do that all the times so I won't blame you but you will admit this is not very readable and someone editing this code might easily introduce a bug…

a) multi-line js string
I have no solution for the multi-line string… I hear an alternative solution is to use:
var template='\
line1 \
line2';
… but that is rather ugly too.

b) string interpolation
For the string interpolation part maybe you could start using Mustache.js which was integrated recently in lp by abentley?
e.g:
var view = {
  title: "Joe",
  calc: function() {
    return 2 + 4;
  }
}
var template = "{{title}} spends {{calc}}";
var html = Mustache.to_html(template, view);

[7]

638 + return;
639 + }
640 + // We have received HTML, so we replace the current bugtask table with a

I think the possible code paths would be better expressed with an if then else instead of the 'return;' statement in the middle of the method.

[8]

668 + var qs = Y.lp.client.append_qs(
669 + '', 'field.actions.delete_bugtask', 'Delete');

Weird indentation here.

[9]

663 + namespace._hideDeleteSpinner(delete_link);
[...]
674 + start:
675 + function() {
676 + namespace._showDeleteSpinner(delete_link);
677 + },

I suppose you did not use 'end' here because the spinner will get removed anyway if the XHR is a success (?)… but maybe you could consider using it anyway for readability/security:
start: Y.bind(namespace._showDeleteSpinner, namespace, delete_link),
end: Y.bind(namespace._hideDeleteSpinner, namespace, delete_link),

[10]

725 + var expander = new Y.lp.app.widgets.expander.Expander(
726 + icon_node, row_node, { animate_node: content_node });

Weird indentation here.

[11]

862 + '<table id="affected-software">',
863 + ' <tbody>',
864 + ' <tr id="tasksummary49">',
865 + ' <td>',
866 + ' <div>',
867 + ' <a class="sprite product" href="#">Product</a>',
868 + ' <button class="lazr-btn yui3-activator-act">',
869 + ' Edit',
870 + ' </button>',
871 + ' <a id="bugtask-delete-task49" href="http://foo" ',
872 + ' class="sprite remove bugtask-delete"></a>',
873 + ' </div>',
874 + ' </td>',
875 + ' <td><table><tr id="task49"><td>',
876 + ' <a id="show-widget-product" class="sprite product" ',
877 + ' href="#">Product</a></td>',
878 + ' </td></tr></table></td>',
879 + ' </tr></tbody></table>'].join(''));

To avoid having this in js tests files, Henning suggested a little trick that I find really nice: you can simply put that string in the test html file with a bogus type="text/x-template". You can see an example of this in the two following files:
lib/lp/app/javascript/confirmationoverlay/tests/test_confirmationoverlay.html → <script type="text/x-template" id="form-template">
lib/lp/app/javascript/confirmationoverlay/tests/test_confirmationoverlay.js → Y.one('#form-template').getContent();

[12]

A few weird indentations:

978 + var test_table =
979 + '<table id="affected-software">'+
980 + '<tr><td>foo</td></tr></table>';

983 + Y.Assert.areEqual(
984 + '<tbody><tr><td>foo</td></tr></tbody>',
985 + this.fixture.one('table#affected-software').getContent());

1001 + module._process_bugtask_delete_response(
1002 + response, this.link_conf.row_id);

1025 + module._process_bugtask_delete_response(
1026 + response, this.link_conf.row_id);

1040 + var orig_delete_repsonse =
1041 + module._process_bugtask_delete_response;

1058 + Y.Assert.areEqual(
1059 + this.delete_link.get('href'),
1060 + mockio.last_request.url);
1061 + Y.Assert.areEqual(
1062 + 'POST', mockio.last_request.config.method);
1063 + Y.Assert.areEqual(
1064 + 'application/json; application/xhtml',
1065 + mockio.last_request.config.headers.Accept);
1066 + Y.Assert.areEqual(
1067 + 'field.actions.delete_bugtask=Delete',
1068 + mockio.last_request.config.data);

[13]

If you fix the lint error in ./lib/lp/bugs/javascript/bugtask_index.js that prevents further processing, make lint has 2-3 things to say about this file that you might want to fix ;)