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

Hi Raphaël

Thanks for the great review!

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

Yeah, the core work was < 800 lines but then the yui tests pushed it
over. Normally I would go and split it but the core work would still
have produced a large diff.

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

In fact, this mp itself is the 2nd in a pipeline so I know how to use
such things :-P
The first branch adds the non-ajax ui for deleting bugtasks.
This mp would have been broken without the bug fix; if I were to have
introduced a new pipe before this one, it would only have saved a few lines.

> [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?
>

I like the use of an inner method here. The code is not relevant outside
the test method it is in and nicely packages the common checks together
to eliminate duplication.

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

Of course. Typo. Thanks for spotting that.

> [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?
>

Sure. I didn't bother because it was only a few lines but I'll add the
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' ) {
>

Old habits. I prefer the above formatting and used it for the past 15
years so am still adjusting to the requirement to put the braces in the
"wrong" place :-/

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

Yes, and this is not preferred AFAIK.

> 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);
>

I normally use Y.Lang.substitute(). I'll see if I can tweak it to make
it more readable.

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

I used the return to avoid indentation. I'll look at changing.

> [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),
>

Correct. It is not required since the html is replaced. I think it would
be confusing to have code that is never executed?

>
> [11]
>
> 862 + '<table id="affected-software">',
> 863 + '<tbody>',
> 864 + '<tr id="tasksummary49">',
> 865 + '<td>',
> 866 + '<div>',
>
> 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();
>

Ah, I had forgotten about that. Thanks.