Merge lp:~wallyworld/launchpad/bugpicker-search-button-1031544 into lp:launchpad

[Ian Booth]

Refactor bug duplicate form so that there is a common bug picker base class, providing standard picker functionality for selecting the dupe bug.

== Implementation ==

The line count for this branch makes it look bigger than it is. It boils down to pulling out base bug picker code from the dupe picker widget and introducing a base bug picker extending the standard picker class. The benefits are twofold:

1. Standard picker infrastructure used, providing search button, progress bar etc, plus the ability to display a number of matching bugs and select one when/if we support searching on more than just a bug number.

2. Separate out the search and selection of a bug from what to do with it once selected. Thus we can now very easily apply this new bug picker everywhere eg linking a bug to a branch.

The dupe bug picker widget is now quite small - it just provides the dupe specific messages and and hooks into the save/remove events to perform the xhr calls to complete the use cases.

The diff contains a lot of red since code was moved from one module to another, and the copied code in the new module shows up green. But it's essentially the same code.

== Demo/QA ==

http://people.canonical.com/~ianb/dupe-bug-picker.ogv

== Tests ==

Separate yui tests modules are provided for the base bug picker widget and the derived dupe bug picker widget. A common set of tests is factored out and used for both test modules so that the dupe picker widget runs the same tests as the base picker widget plus additional tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/icing/css/components/bug_picker.css
  lib/lp/app/javascript/picker/picker.js
  lib/lp/bugs/javascript/bug_picker.js
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/duplicates.js
  lib/lp/bugs/javascript/tests/test_bug_picker.html
  lib/lp/bugs/javascript/tests/test_bug_picker.js
  lib/lp/bugs/javascript/tests/test_duplicates.html
  lib/lp/bugs/javascript/tests/test_duplicates.js

[Curtis Hovey (sinzui)]

Thank you for this glorious branch Ian.

I don't think "please" is required on line 68, the application will not be polite it you make a mistake.

The edit action must follow the value that is being edited. When a bug is marked as a dupe, the video shows the edit icon leads the line, which implies the change the whole bug, but the user wants to change the value (the duplicate bug number) Line 884 of the diff might be the issue. I think this moves the edit link after the bug number:
            dupe_span.setContent([
                '<span id="mark-duplicate-text">',
                'Duplicate of <a>bug #</a></span>',
                '<a id="change_duplicate_bug" ',
                'title="Edit or remove linked duplicate bug" ',
                'class="sprite edit action-icon"',
                '>Edit</a>'].join(""));

I see an opportunity on Line 1024 to stop implying the users' previous action caused the page to show a warning message. We could use this instead.
    <p class="block-sprite large-warning">

review: Approve (code)

[Curtis Hovey (sinzui)]

Ian, disregard my suggestion to move the action link. While the rule I cite is correct, the bug page has issues using it as a menu link too.

IDEAS for follow branches.

Please read comment #9 of bug 227310. We are close to fixing it. I think a small text revision to the instructions can remove the repeated text in the header. I wonder if we should show a paragraph before the affects table stating that the bug is a duplicate, show the bug number, then follow it with the edit link as I suggested in my previous comment. This would be link the bug-converted-to-question paragraph. I think this addresses bug 227310 and my concern that we need an edit link after the master bug number.

If the work to fix bug 346531 is less than 4 hours, I would like you to do it.

[Curtis Hovey (sinzui)]

I mean *like* bug-converted-to-question paragraph. I think the paragraph would also fix bug 524294.

I think we need to fix bug 317258 because when the bug is private, you are disclosing it to a different set of people.

[Ian Booth (wallyworld)]

Thanks for getting through the many lines in this mp.

>
> I don't think "please" is required on line 68, the application will not be
> polite it you make a mistake.
>

This is copied directly from the existing base lazr picker code. I made it into an attribute so it could be overridden. So I'm not creating new text in this mp. I'll update the message and also the existing lazr picker yui tests if you do want to go ahead with the wording change.

[Ian Booth (wallyworld)]

>
> I see an opportunity on Line 1024 to stop implying the users' previous action
> caused the page to show a warning message. We could use this instead.
> <p class="block-sprite large-warning">

Yes, good pick up. I've fixed the icon and rendering.

I also noticed in doing do that the javascript rendered a simplified version of what the TAL provides - the javascript just said "This is a duplicate" rather than "This is a duplicate of <a title='foo'>bug #1</a>". Previously the javascript didn't have the required info to do the proper rendering now it does. So it's been fixed.