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 + '
',
593 + 'You are about to mark bug "',
594 + conf.bug_title,
595 + '"
as no longer affecting ',
596 + conf.targetname,
597 + '.
Please confirm you really want to do this.',
598 + '
',
866 + ' ',
867 + ' Product',
868 + ' ',
871 + ' ',
873 + ' ',
874 + ' | ',
875 + '
| ',
879 + '