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.