Merge lp:~wallyworld/launchpad/assign-non-contributor into lp:launchpad

[Ian Booth]

[r=lifeless,wgrant][ui=huwshimi][bug=698138] When a bug task assignee is changed inline and the new assignee is a non-contributer, prompt the user to confirm their choice of assignee.

When a bug task assignee is changed inline and the new assignee is a non-contributer, prompt the user to confirm their choice of assignee. Currently the warning is only display if Javascript is disabled and the assignee changed using a form submission.

== Implementation ==

This new functionality is delivered on top of new validation infrastructure built into Launchpad's picker widget (which itself extends Y.lazr.Picker).

When a picker is created, one can optionally pass in a validation callback. This will be invoked when the user selects a picker item. Without the validation callback, the picker save function is invoked immediately. However, with the validation callback, the callback is invoked and returns either true/false. If true, save is invoked, if false, the picker stays open at the current selection.

For this bug fix, a yesno validation callback is implemented in the picker. This is the first example of using the validation callback infrastructure. The picker widget provides the boiler plate to wire it all together. The bug task Javascript provides the actual callback business logic. When the user makes a selection, an xhr call is made to see if that person is a contributor. If they aren't, a confirmation prompt appears allowing the user to answer "no" and choose another assignee. The conformation prompt replaces the picker content and animation is used to make it look good.

== Demo ==

Here's a screencast: http://people.canonical.com/~ianb/assignee_validation.ogv

== Tests ==

Tests were added for the new infrastructure and a Windmill test. The picker implementation was modified slightly to allow a list of vocabulary terms to be passed in as well as a vocabulary name. This was required to allow a test case to create a picker.

There were no existing Javascript tests for Launchpad's picker so a new test suite was added: lp/app/javascript/tests/picker.js
This test suite specifically tests the new yes/no validation callback added to the picker:
  test_picker_can_be_instantiated()
  test_no_confirmation_required()
  test_confirmation_yes()
  test_confirmation_no()

A unit test was added for the BugContributorView:
  TestBugContributorView
    test_non_contributor()
    test_contributor()

A windmill test was added for the specific use case of in-line bug assignment:
  lp/bugs/windmill/tests/test_bug_inline_assignment.py

= Launchpad lint =

I fixed some existing Javascript lint issues but was not comfortable about changing the suggested !== and === items.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/picker.js
  lib/lp/app/javascript/tests/picker.html
  lib/lp/app/javascript/tests/picker.js
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/configure.zcml
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/windmill/tests/test_bug_inline_assignment.py

./lib/lp/app/javascript/picker.js
      92: Expected '!==' and instead saw '!='.
     222: Expected '!==' and instead saw '!='.
     286: Expected '!==' and instead saw '!='.
     302: Expected '!==' and instead saw '!='.
     307: Expected '!==' and instead saw '!='.
     355: Expected '!==' and instead saw '!='.
     355: Expected '!==' and instead saw '!='.
     422: Move 'var' declarations to the top of the function.
     422: Stopping. (86% scanned).
       0: JSLINT had a fatal error.
     193: Line exceeds 78 characters.
./lib/lp/bugs/javascript/bugtask_index.js
     269: Expected '===' and instead saw '=='.
     726: Expected '===' and instead saw '=='.
     759: Expected '===' and instead saw '=='.
     815: Expected '===' and instead saw '=='.
     817: Expected '===' and instead saw '=='.
     886: Expected '===' and instead saw '=='.
    1008: Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead.
    1022: Expected '===' and instead saw '=='.
    1035: Expected '===' and instead saw '=='.
     387: Line exceeds 78 characters.
     388: Line exceeds 78 characters.
     389: Line exceeds 78 characters.
     443: Line exceeds 78 characters.

[William Grant (wgrant)]

As discussed on IRC, I have a couple of concerns.

Firstly, BugContributorView should probably be an API named operation. While lazr.restful doesn't in general like dicts very much, this is just a dict of JSON primitive types. Declare the method without a return type, and just return the dict.

Secondly, synchronous XHR is never a good idea -- it hangs the page while it's running. In older browsers (eg. Firefox) it actually hangs the whole browser! The picker is already thoroughly asynchronous; it seems like it shouldn't be all that difficult to make this bit async too.

review: Needs Fixing (code*)

[Robert Collins (lifeless)]

I concur

[Ian Booth (wallyworld)]

> Firstly, BugContributorView should probably be an API named operation. While
> lazr.restful doesn't in general like dicts very much, this is just a dict of
> JSON primitive types. Declare the method without a return type, and just
> return the dict.
>

I originally did it as a named_get but went with the view because there seemed to be issues with returning a dict. But reverting to the original api call and making a few tweaks and it all works.

> Secondly, synchronous XHR is never a good idea -- it hangs the page while it's
> running. In older browsers (eg. Firefox) it actually hangs the whole browser!
> The picker is already thoroughly asynchronous; it seems like it shouldn't be
> all that difficult to make this bit async too.

Luckily, it turned out that the main issue with the original async implementation was subsequently fixed to solve another issue along the way and it was easy to revert to the async approach.

[William Grant (wgrant)]

yesno_save_confirmation is a bit messy and unobvious. It may be clearer to create the whole structure from a single string, then fill the text in using deeper selectors. This would also encourage you to fix the yes_label/no_label string concatenation. The function could also probably just be called save_confirmation or confirm_save. Perhaps confirm/cancel is better terminology than yes/no, too. And I'm not a fan of the inline styles on .yesyno_buttons, nor of the typo in that class name. Is content_node.insert(validation_content_container, 'before') going to treat unsafe content properly? We probably want the behaviour where it's treated as text unless it's a Node, given the XSS that's here already (see below).

animate_validation_content could be a little shorter if you dropped content_node and called .hide() directly. The four lines of steps_node could also be replaced with a single .all('.steps').hide() line.

"Restore a picker to it's" s/it's/its/

reset_form has another instance of steps_node which could be replaced with a single line. It also has some duplicated content_node references which could be replaced with chained calls.

getContributorInfo is explicitly exposed in beta, but we're not exposing things outside devel except as necessary. Please restrict it to devel.

XSS here:
702 + "<p>{person_name} did not previously have any " +
703 + "assigned bugs in {pillar}.</p>" +
704 + "<p>Do you really want to assign them to this bug?</p>";
705 + var yesno_content = Y.Lang.substitute(
706 + yesno_content_template,
707 + {person_name: person, pillar: pillar});

test_inline_assignment_non_contributer should probably take an s/contributer/contributor/. The XPath with td[5] seems a bit fragile, is there a class you could use instead? I understand there might not be.

review: Needs Fixing (code*)

[Robert Collins (lifeless)]

I thought the plan was 'assign' / 'select again' rather than Y/N ?

[Ian Booth (wallyworld)]

> yesno_save_confirmation is a bit messy and unobvious. It may be clearer to

I don't think it's that bad. It's basically some node construction and assembly.

> create the whole structure from a single string, then fill the text in using
> deeper selectors. This would also encourage you to fix the yes_label/no_label

This isn't viable since the buttons need to be constructed separately so that onclick handlers can be added. The concatenation is in line with the coding guidelines which say that up to 3 lines of concatenation for a given string is permissible.
I've restructured the javascript to hopefully make it a little clearer.

> string concatenation. The function could also probably just be called
> save_confirmation or confirm_save. Perhaps confirm/cancel is better

I've kept the name as is because there may be other validator types in the future and the validator type name needs to be included in the function name for disambiguation.

> terminology than yes/no, too. And I'm not a fan of the inline styles on
> .yesyno_buttons, nor of the typo in that class name. Is

Typo fixed. The inline styles were copied from elsewhere. I've created a new css class called extra_form_buttons and used that instead.

> content_node.insert(validation_content_container, 'before') going to treat
> unsafe content properly? We probably want the behaviour where it's treated as
> text unless it's a Node, given the XSS that's here already (see below).
>

The code as written is ok - it's just assembling the nodes created in the callback.

> animate_validation_content could be a little shorter if you dropped
> content_node and called .hide() directly. The four lines of steps_node could
> also be replaced with a single .all('.steps').hide() line.
>
> "Restore a picker to it's" s/it's/its/
>
> reset_form has another instance of steps_node which could be replaced with a
> single line. It also has some duplicated content_node references which could
> be replaced with chained calls.
>

Done

> getContributorInfo is explicitly exposed in beta, but we're not exposing
> things outside devel except as necessary. Please restrict it to devel.
>

Done

> XSS here:
> 702 + "<p>{person_name} did not previously have any
> " +
> 703 + "assigned bugs in {pillar}.</p>" +
> 704 + "<p>Do you really want to assign them to this
> bug?</p>";
> 705 + var yesno_content = Y.Lang.substitute(
> 706 + yesno_content_template,
> 707 + {person_name: person, pillar: pillar});
>

Fixed - variable values to be substituted are escaped.

> test_inline_assignment_non_contributer should probably take an
> s/contributer/contributor/. The XPath with td[5] seems a bit fragile, is there
> a class you could use instead? I understand there might not be.

No class sadly. jquery is preferable but doesn't seem to "do the right thing" when used with Windmill. xpath works fine every time.

[William Grant (wgrant)]

> > yesno_save_confirmation is a bit messy and unobvious. It may be clearer to
>
> I don't think it's that bad. It's basically some node construction and
> assembly.
>
> > create the whole structure from a single string, then fill the text in using
> > deeper selectors. This would also encourage you to fix the
> yes_label/no_label
>
> This isn't viable since the buttons need to be constructed separately so that
> onclick handlers can be added. The concatenation is in line with the coding
> guidelines which say that up to 3 lines of concatenation for a given string is
> permissible.
> I've restructured the javascript to hopefully make it a little clearer.

I still think it could be improved. See lp:~wgrant/launchpad/assign-non-contributor for some suggestions.

> > string concatenation. The function could also probably just be called
> > save_confirmation or confirm_save. Perhaps confirm/cancel is better
>
> I've kept the name as is because there may be other validator types in the
> future and the validator type name needs to be included in the function name
> for disambiguation.

OK.

> > terminology than yes/no, too. And I'm not a fan of the inline styles on
> > .yesyno_buttons, nor of the typo in that class name. Is
>
> Typo fixed. The inline styles were copied from elsewhere. I've created a new
> css class called extra_form_buttons and used that instead.

Thanks!

> > content_node.insert(validation_content_container, 'before') going to treat
> > unsafe content properly? We probably want the behaviour where it's treated
> as
> > text unless it's a Node, given the XSS that's here already (see below).
> >
>
> The code as written is ok - it's just assembling the nodes created in the
> callback.

In my experimentation I've found that this is fine. insert handles nodes/text properly.

> [snip remaining fixed points; thanks]

[Ian Booth (wallyworld)]

>> This isn't viable since the buttons need to be constructed separately so that
>> onclick handlers can be added. The concatenation is in line with the coding
>> guidelines which say that up to 3 lines of concatenation for a given string is
>> permissible.
>> I've restructured the javascript to hopefully make it a little clearer.
>
> I still think it could be improved. See lp:~wgrant/launchpad/assign-non-contributor for some suggestions.
>

Thanks for the suggestion. I guess it's a matter of personal taste what
you're used to. I prefer the programmatic approach of constructing nodes
and wiring together :-)

In any case, I've made the suggested changes. It does mean less lines of
code. The coding guidelines say not to concatenate strings over more
than 3 lines so I've used the approach of ['foo', 'bar'].join('') as per
the guidelines.

[William Grant (wgrant)]

> >> This isn't viable since the buttons need to be constructed separately so
> that
> >> onclick handlers can be added. The concatenation is in line with the coding
> >> guidelines which say that up to 3 lines of concatenation for a given string
> is
> >> permissible.
> >> I've restructured the javascript to hopefully make it a little clearer.
> >
> > I still think it could be improved. See lp:~wgrant/launchpad/assign-non-
> contributor for some suggestions.
> >
>
> Thanks for the suggestion. I guess it's a matter of personal taste what
> you're used to. I prefer the programmatic approach of constructing nodes
> and wiring together :-)

Indeed. While your method seems nicer in some ways, I think this is far easier to understand. You can clearly see the structure and exactly what we're hooking up to it, and the two parts are clearly separated.

> In any case, I've made the suggested changes. It does mean less lines of
> code. The coding guidelines say not to concatenate strings over more
> than 3 lines so I've used the approach of ['foo', 'bar'].join('') as per
> the guidelines.

It was a single string literal, not a series of concatenations, but OK. Neither is pretty.

One remaining stylistic niggle: extra_form_buttons and important-notice-popup have styles added, but one uses hyphens and the other underscores. I guess we probably have a standard for this.

review: Approve (code*)

[Ian Booth (wallyworld)]

>
>> In any case, I've made the suggested changes. It does mean less lines of
>> code. The coding guidelines say not to concatenate strings over more
>> than 3 lines so I've used the approach of ['foo', 'bar'].join('') as per
>> the guidelines.
>
> It was a single string literal, not a series of concatenations, but OK. Neither is pretty.
>

Ah, it was changed to a single literal from a series of concatenations,
my mistake in the wording above sorry. But I also thought that using "\"
for line continuation was forbidden too :-)

> One remaining stylistic niggle: extra_form_buttons and important-notice-popup have styles added, but one uses hyphens and the other underscores. I guess we probably have a standard for this.

Yes, you are right. I missed that inconsistency. I will change it, thanks.

[Robert Collins (lifeless)]

Ok this looks fine. Its a shame that the vocabs we have are so coupled to our batchnav implementation - it means we'll need to change them as we rollout the constraint based batching.

review: Approve

[Huw Wilkins (huwshimi)]

This looks good to me. Thanks for implementing the things that we talked about previously.

I know you've had a lot of back and forward with this but one thing that has been niggling me that I didn't pick up when I looked at previously is that it doesn't look like there is any space between the Assign/Choose Again buttons.

It would be really nice to fix that, but I don't want to hold you back from landing this for one space so I'm approving and you can fix it if you haven't been defeated by this review already.

review: Approve (ui)

[Ian Booth (wallyworld)]

> This looks good to me. Thanks for implementing the things that we talked about
> previously.
>
> I know you've had a lot of back and forward with this but one thing that has
> been niggling me that I didn't pick up when I looked at previously is that it
> doesn't look like there is any space between the Assign/Choose Again buttons.
>
> It would be really nice to fix that, but I don't want to hold you back from
> landing this for one space so I'm approving and you can fix it if you haven't
> been defeated by this review already.

I used a css style:

.extra-form-buttons button {
    margin-right: 0.7em;
    }

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2011-03-30 10:47:41 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2011-04-13 07:06:21 +0000
@@ -828,6 +828,14 @@
     border-width: 0 0 1px 0;
     display: block;
     }
+.extra-form-buttons {
+    text-align: center;
+    height: 3em;
+    white-space: nowrap;
+    }
+.extra-form-buttons button {
+    margin-right: 0.7em;
+    }
 .actions * {
     /*
       Action links are those that begin the process of doing something.
@@ -1054,6 +1062,11 @@
 .sml-informational::before {
     content: url('/@@/info');
     }
+.important-notice-popup {
+    padding: 1em 1em 0 1em;
+    width: auto;
+    overflow: hidden;
+    }
 .important-notice-container {
     text-align: center;
     width: 100%;

=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js	2011-04-11 14:20:53 +0000
+++ lib/lp/app/javascript/client.js	2011-04-13 07:06:21 +0000
@@ -589,7 +589,7 @@
             'headers': headers,
             data: data
         };
-        Y.io(uri, y_config);
+        return Y.io(uri, y_config);
     },
 
     'named_get' : function(uri, operation_name, config) {

=== modified file 'lib/lp/app/javascript/multicheckbox.js'
--- lib/lp/app/javascript/multicheckbox.js	2011-03-31 04:52:03 +0000
+++ lib/lp/app/javascript/multicheckbox.js	2011-04-13 07:06:21 +0000
@@ -73,9 +73,7 @@
     config.content_box_id = content_box_id;
     var editform = namespace.create(attribute_name, items, help_text, config);
     editform._resource_uri = resource_uri;
-    var extra_buttons = Y.Node.create(
-        '<div style="text-align: center; height: 3em; ' +
-        'white-space: nowrap"/>');
+    var extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
     activator.subscribe('act', function (e) {
         editform.show();
     });

=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js	2011-04-06 19:13:23 +0000
+++ lib/lp/app/javascript/picker.js	2011-04-13 07:06:21 +0000
@@ -39,7 +39,7 @@
     var remove_button_text = 'Remove';
     var null_display_value = 'None';
     var show_search_box = true;
-    resource_uri = Y.lp.client.normalize_uri(resource_uri)
+    resource_uri = Y.lp.client.normalize_uri(resource_uri);
 
     if (config !== undefined) {
         if (config.remove_button_text !== undefined) {
@@ -101,7 +101,6 @@
         var success_handler = function (entry) {
           activator.renderSuccess(entry.getHTML(attribute_name));
           show_hide_buttons();
-          return;
         };
 
         var patch_payload = {};
@@ -151,11 +150,9 @@
     };
 
     config.save = save;
-    var picker = namespace.create(vocabulary, config);
+    var picker = namespace.create(vocabulary, config, activator);
     picker._resource_uri = resource_uri;
-    var extra_buttons = Y.Node.create(
-        '<div style="text-align: center; height: 3em; ' +
-        'white-space: nowrap"/>');
+    var extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
     var remove_button, assign_me_button;
     if (show_remove_button) {
         remove_button = Y.Node.create(
@@ -219,8 +216,96 @@
  * Remove the Loading.... spinner (if it exists).
  */
 function hide_temporary_spinner(temp_spinner) {
-    if( temp_spinner != null )
+    if( temp_spinner != null ) {
         temp_spinner.addClass('unseen');
+    }
+}
+
+/*
+ * After the user selects an item using the picker, this function can be used
+ * to present the user with a yes/no prompt to ensure they really want to use
+ * the selection. If they answer yes, the selection is processed as normal. If
+ * they answer no, they can make another selection.
+ *
+ * @param {Picker} picker The picker displaying the yes/no content.
+ * @param {String} content The html content to display.
+ * @param {String} yes_label The label for the "yes" button.
+ * @param {String} no_label The label for the "no" button.
+ * @param {Object} yes_fn The function to call if the user answers yes.
+ * @param {Object} no_fn The function to call if the user answers no.
+ */
+namespace.yesno_save_confirmation = function(
+        picker, content, yes_label, no_label, yes_fn, no_fn) {
+
+    var node = Y.Node.create(
+        ['<div class="validation-node">',
+          '<div class="step-on" style="width: 100%;"></div>',
+          '<div class="transparent important-notice-popup">',
+            '<div class="validation-content-placeholder"></div>',
+            '<div class="extra-form-buttons">',
+              '<button class="yes_button" type="button"/>',
+              '<button class="no_button" type="button"/>',
+            '</div>',
+          '</div>',
+        '</div>'].join(''));
+
+    var button_callback = function(e, callback_fn) {
+        e.halt();
+        if (Y.Lang.isFunction(callback_fn) ) {
+            callback_fn();
+        }
+        reset_form(picker);
+    };
+    node.one(".yes_button")
+        .set('text', yes_label)
+        .on('click', function(e) { button_callback(e, yes_fn); });
+
+    node.one(".no_button")
+        .set('text', no_label)
+        .on('click', function(e) { button_callback(e, no_fn); });
+
+    node.one(".validation-content-placeholder").replace(content);
+    picker.get('contentBox').one('.yui3-widget-bd').insert(node, 'before');
+    animate_validation_content(picker, node.one(".important-notice-popup"));
+};
+
+/*
+ * Insert the validation content into the form and animate its appearance.
+ */
+function animate_validation_content(picker, validation_content) {
+    picker.get('contentBox').one('.yui3-widget-bd').hide();
+    picker.get('contentBox').all('.steps').hide();
+    var validation_fade_in = new Y.Anim({
+        node: validation_content,
+        to: {opacity: 1},
+        duration: 0.9
+    });
+    validation_fade_in.run();
+}
+
+/*
+ * Restore a picker to its functional state after a validation operation.
+ */
+function reset_form(picker) {
+    picker.get('contentBox').all('.steps').show();
+    var validation_node = picker.get('contentBox').one('.validation-node');
+    var content_node = picker.get('contentBox').one('.yui3-widget-bd');
+    if (validation_node != null) {
+        validation_node.get('parentNode').removeChild(validation_node);
+        content_node.addClass('transparent');
+        content_node.setStyle('opacity', 0);
+        content_node.show();
+        var content_fade_in = new Y.Anim({
+            node: content_node,
+            to: {opacity: 1},
+            duration: 0.6
+        });
+        content_fade_in.run();
+    } else {
+        content_node.removeClass('transparent');
+        content_node.setStyle('opacity', 1);
+        content_node.show();
+    }
 }
 
 /**
@@ -236,27 +321,27 @@
   *                        config.save is a Function (optional) which takes
   *                        a single string argument.
   */
-namespace.create = function (vocabulary, config) {
+namespace.create = function (vocabulary, config, activator) {
     if (Y.UA.ie) {
         return;
     }
 
+    var header = 'Choose an item.';
+    var step_title = "Enter search terms";
     if (config !== undefined) {
-        var header = 'Choose an item.';
         if (config.header !== undefined) {
             header = config.header;
         }
 
-        var step_title = "Enter search terms";
         if (config.step_title !== undefined) {
             step_title = config.step_title;
         }
     }
 
-    if (typeof vocabulary != 'string') {
+    if (typeof vocabulary != 'string' && typeof vocabulary != 'object') {
         throw new TypeError(
             "vocabulary argument for Y.lp.picker.create() must be a " +
-            "string: " + vocabulary);
+            "string or associative array: " + vocabulary);
     }
 
     var new_config = Y.merge(config, {
@@ -275,13 +360,26 @@
 
     picker.subscribe('save', function (e) {
         Y.log('Got save event.');
-        if (Y.Lang.isFunction(config.save)) {
-            config.save(e.details[Y.lazr.Picker.SAVE_RESULT]);
+        e.preventDefault();
+        var picker_result = e.details[Y.lazr.Picker.SAVE_RESULT];
+        var do_save = function() {
+            if (Y.Lang.isFunction(config.save)) {
+                config.save(picker_result);
+            }
+            picker._defaultSave(e);
+        };
+        var validate_callback = config.validate_callback;
+        if (Y.Lang.isFunction(validate_callback)) {
+            validate_callback(
+                    picker, picker_result, do_save, undefined);
+        } else {
+            do_save();
         }
     });
 
     picker.subscribe('cancel', function (e) {
         Y.log('Got cancel event.');
+        reset_form(picker);
     });
 
     // Search for results, create batches and update the display.
@@ -291,15 +389,7 @@
         var search_text = e.details[0];
         var selected_batch = e.details[1] || 0;
         var start = BATCH_SIZE * selected_batch;
-        var client = new Y.lp.client.Launchpad();
-
-        var success_handler = function (ignore, response, args) {
-            var entry = Y.JSON.parse(response.responseText);
-            var total_size = entry.total_size;
-            var start = entry.start;
-            var results = entry.entries;
-
-            hide_temporary_spinner(config.temp_spinner);
+        var display_vocabulary = function(results, total_size, start) {
             if (total_size > (MAX_BATCHES * BATCH_SIZE))  {
                 picker.set('error',
                     'Too many matches. Please try to narrow your search.');
@@ -322,41 +412,55 @@
                                 });
                         }
                     }
-
                     picker.set('batches', batches);
                 }
             }
         };
 
-        var qs = '';
-        qs = Y.lp.client.append_qs(qs, 'name', vocabulary);
-        qs = Y.lp.client.append_qs(qs, 'search_text', search_text);
-        qs = Y.lp.client.append_qs(qs, 'batch', BATCH_SIZE);
-        qs = Y.lp.client.append_qs(qs, 'start', start);
-
-        // The uri needs to be relative, so that the vocabulary
-        // has the same context as the form. Some vocabularies
-        // use the context to limit the results to the same project.
-
-        var uri = '';
-        if (Y.Lang.isValue(config['context'])){
-            uri += Y.lp.get_url_path(config['context'].get('web_link')) + '/';
+        // We can pass in a vocabulary name
+        if (typeof vocabulary == 'string') {
+            var success_handler = function (ignore, response, args) {
+                var entry = Y.JSON.parse(response.responseText);
+                var total_size = entry.total_size;
+                var start = entry.start;
+                var results = entry.entries;
+                hide_temporary_spinner(config.temp_spinner);
+                display_vocabulary(results, total_size, start);
+            };
+
+            var qs = '';
+            qs = Y.lp.client.append_qs(qs, 'name', vocabulary);
+            qs = Y.lp.client.append_qs(qs, 'search_text', search_text);
+            qs = Y.lp.client.append_qs(qs, 'batch', BATCH_SIZE);
+            qs = Y.lp.client.append_qs(qs, 'start', start);
+
+            // The uri needs to be relative, so that the vocabulary
+            // has the same context as the form. Some vocabularies
+            // use the context to limit the results to the same project.
+
+            var uri = '';
+            if (Y.Lang.isValue(config['context'])){
+                uri += Y.lp.get_url_path(config['context'].get('web_link')) + '/';
+            }            
+            uri += '@@+huge-vocabulary?' + qs;
+
+            Y.io(uri, {
+                headers: {'Accept': 'application/json'},
+                timeout: 20000,
+                on: {
+                    success: success_handler,
+                    failure: function (arg) {
+                        hide_temporary_spinner(config.temp_spinner);
+                        picker.set('error', 'Loading results failed.');
+                        picker.set('search_mode', false);
+                        Y.log("Loading " + uri + " failed.");
+                    }
+                }
+            });
+        // Or we can pass in a vocabulary directly.
+        } else {
+            display_vocabulary(vocabulary, Y.Object.size(vocabulary), 1);
         }
-        uri += '@@+huge-vocabulary?' + qs;
-
-        Y.io(uri, {
-            headers: {'Accept': 'application/json'},
-            timeout: 20000,
-            on: {
-                success: success_handler,
-                failure: function (arg) {
-                    hide_temporary_spinner(config.temp_spinner);
-                    picker.set('error', 'Loading results failed.');
-                    picker.set('search_mode', false);
-                    Y.log("Loading " + uri + " failed.");
-                }
-            }
-        });
     };
 
     picker.after('search', search_handler);

=== added file 'lib/lp/app/javascript/tests/picker.html'
--- lib/lp/app/javascript/tests/picker.html	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/picker.html	2011-04-13 07:06:21 +0000
@@ -0,0 +1,48 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
+  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
+<html>
+  <head>
+  <title>Launchpad Picker</title>
+
+  <!-- YUI 3.0 Setup -->
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/javascript/test.css" />
+
+  <!-- Some required dependencies -->
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/testing/config.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/activator/activator.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/overlay/overlay.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/picker/picker.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/anim/anim.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr/lazr.js"></script>
+  <script type="text/javascript" src="../client.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../picker.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="picker.js"></script>
+</head>
+<body class="yui3-skin-sam">
+  <div class="yui3-widget yui3-activator yui3-activator-focused">
+      <span id="picker_id" class="yui3-activator-content yui3-activator-success">
+        <span id="pickertest">
+          <span>
+            A picker widget test
+            <button id="edit-pickertest-btn"
+                    class="lazr-btn yui3-activator-act yui3-activator-hidden">
+              Edit
+            </button>
+          </span>
+           <span class="yui3-activator-data-box">
+           </span>
+          <div class="yui3-activator-message-box yui3-activator-hidden"/>
+        </span>
+      </span>
+  </div>
+  <div id="log"></div>
+</body>
+</html>

=== added file 'lib/lp/app/javascript/tests/picker.js'
--- lib/lp/app/javascript/tests/picker.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/picker.js	2011-04-13 07:06:21 +0000
@@ -0,0 +1,180 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw',
+    combine: false,
+    fetchCSS: false
+    }).use('test', 'console', 'node', 'lp', 'lp.client', 'escape', 'event',
+        'event-focus', 'event-simulate', 'lazr.picker','lp.app.picker',
+        function(Y) {
+
+var Assert = Y.Assert;
+
+/*
+ * A wrapper for the Y.Event.simulate() function.  The wrapper accepts
+ * CSS selectors and Node instances instead of raw nodes.
+ */
+function simulate(widget, selector, evtype, options) {
+    var rawnode = Y.Node.getDOMNode(widget.one(selector));
+    Y.Event.simulate(rawnode, evtype, options);
+}
+
+/* Helper function to clean up a dynamically added widget instance. */
+function cleanup_widget(widget) {
+    // Nuke the boundingBox, but only if we've touched the DOM.
+    if (widget.get('rendered')) {
+        var bb = widget.get('boundingBox');
+        bb.get('parentNode').removeChild(bb);
+    }
+    // Kill the widget itself.
+    widget.destroy();
+}
+
+var suite = new Y.Test.Suite("Launchpad Picker Tests");
+
+/*
+ * Test cases for a picker with a yesno validation callback.
+ */
+suite.add(new Y.Test.Case({
+
+    name: 'picker_yesyno_validation',
+
+    setUp: function() {
+        this.vocabulary = [
+            {"value": "fred", "title": "Fred", "css": "sprite-person",
+                "description": "[e-mail]", "api_uri": "~/fred"},
+            {"value": "frieda", "title": "Frieda", "css": "sprite-person",
+                "description": "[e-mail]", "api_uri": "~/frieda"}
+        ];
+    },
+
+    tearDown: function() {
+        cleanup_widget(this.picker);
+    },
+
+    create_picker: function(validate_callback) {
+        this.picker = Y.lp.app.picker.addPickerPatcher(
+            this.vocabulary,
+            "foo/bar",
+            "test_link",
+            "picker_id",
+            {"step_title": "Choose someone",
+             "header": "Pick Someone",
+             "remove_button_text": "Remove someone",
+             "null_display_value": "Noone",
+             "show_remove_button": true,
+             "show_assign_me_button": true,
+             "validate_callback": validate_callback});
+    },
+
+    test_picker_can_be_instantiated: function() {
+        // Ensure the picker can be instantiated.
+        this.create_picker();
+        Assert.isInstanceOf(
+            Y.lazr.Picker, this.picker, "Picker failed to be instantiated");
+    },
+
+    // Called when the picker saves it's data. Sets a flag for checking.
+    picker_save_callback: function(save_flag) {
+        return function(e) {
+            save_flag.event_has_fired = true;
+        };
+    },
+
+    // A validation callback stub. Instead of going to the server to see if
+    // a picker value requires confirmation, we compare it to a known value.
+    yesno_validate_callback: function(save_flag, expected_value) {
+        var save_fn = this.picker_save_callback(save_flag);
+        return function(picker, value, ignore, cancel_fn) {
+            Assert.areEqual(
+                    expected_value, value.api_uri, "unexpected picker value");
+            if (value === null) {
+                return true;
+            }
+            var requires_confirmation = value.api_uri !== "~/fred";
+            if (requires_confirmation) {
+                var yesno_content = "<p>Confirm selection</p>";
+                Y.lp.app.picker.yesno_save_confirmation(
+                        picker, yesno_content, "Yes", "No",
+                        save_fn, cancel_fn);
+            } else {
+                save_fn();
+            }
+        };
+    },
+
+    test_no_confirmation_required: function() {
+        // Test that the picker saves the selected value if no confirmation
+        // is required.
+        var save_flag = {};
+        this.create_picker(this.yesno_validate_callback(save_flag, "~/fred"));
+        this.picker.set('results', this.vocabulary);
+        this.picker.render();
+
+        simulate(
+            this.picker.get('boundingBox').one('.yui3-picker-results'),
+                'li:nth-child(1)', 'click');
+        Assert.isTrue(save_flag.event_has_fired, "save event wasn't fired.");
+    },
+
+    test_confirmation_yes: function() {
+        // Test that the picker saves the selected value if the user answers
+        // "Yes" to a confirmation request.
+        var save_flag = {};
+        this.create_picker(
+                this.yesno_validate_callback(save_flag, "~/frieda"));
+        this.picker.set('results', this.vocabulary);
+        this.picker.render();
+
+        simulate(
+            this.picker.get('boundingBox').one('.yui3-picker-results'),
+                'li:nth-child(2)', 'click');
+        var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+
+        simulate(
+                yesno, 'button:nth-child(1)', 'click');
+        Assert.isTrue(
+                save_flag.event_has_fired, "save event wasn't fired.");
+    },
+
+    test_confirmation_no: function() {
+        // Test that the picker doesn't save the selected value if the answers
+        // "No" to a confirmation request.
+        var save_flag = {};
+        this.create_picker(
+                this.yesno_validate_callback(save_flag, "~/frieda"));
+        this.picker.set('results', this.vocabulary);
+        this.picker.render();
+
+        simulate(
+            this.picker.get('boundingBox').one('.yui3-picker-results'),
+                'li:nth-child(2)', 'click');
+        var yesno = this.picker.get('contentBox').one('.extra-form-buttons');
+        simulate(
+                yesno, 'button:nth-child(2)', 'click');
+        Assert.areEqual(
+                undefined, save_flag.event_has_fired,
+                "save event wasn't fired.");
+    }
+}));
+
+// Lock, stock, and two smoking barrels.
+var handle_complete = function(data) {
+    status_node = Y.Node.create(
+        '<p id="complete">Test status: complete</p>');
+    Y.one('body').appendChild(status_node);
+    };
+Y.Test.Runner.on('complete', handle_complete);
+Y.Test.Runner.add(suite);
+
+var yui_console = new Y.Console({
+    newestOnTop: false
+});
+yui_console.render('#log');
+
+Y.on('domready', function() {
+    Y.Test.Runner.run();
+});
+
+});

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-04-07 18:55:36 +0000
+++ lib/lp/bugs/configure.zcml	2011-04-13 07:06:21 +0000
@@ -241,7 +241,8 @@
                     conjoined_slave
                     subscribe
                     getConjoinedMaster
-                    findSimilarBugs"/>
+                    findSimilarBugs
+                    getContributorInfo"/>
             <require
                 permission="launchpad.Edit"
                 attributes="

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2011-03-30 19:04:42 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2011-04-13 07:06:21 +0000
@@ -62,6 +62,7 @@
     export_write_operation,
     exported,
     mutator_for,
+    operation_for_version,
     operation_parameters,
     operation_returns_collection_of,
     rename_parameters_as,
@@ -654,6 +655,28 @@
     def findSimilarBugs(user, limit=10):
         """Return the list of possible duplicates for this BugTask."""
 
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(person=copy_field(assignee))
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getContributorInfo(user, person):
+        """Is the person a contributor to bugs in this task's pillar?
+
+        :param user: The user doing the search. Private bugs that this
+            user doesn't have access to won't be included in the search.
+        :param person: The person to check to see if they are a contributor.
+
+        Return a dict with the following values:
+        is_contributor: True if the user has any bugs assigned to him in the
+        context of this bug task's pillar, either directly or by team
+        participation.
+        person_name: the displayname of the person
+        pillar_name: the displayname of the bug task's pillar
+
+        This API call is provided for use by the client Javascript where the
+        calling context does not have access to the person or pillar names.
+        """
+
     def getConjoinedMaster(bugtasks, bugtasks_by_package=None):
         """Return the conjoined master in the given bugtasks, if any.
 

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-03-22 05:56:34 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-04-13 07:06:21 +0000
@@ -50,7 +50,7 @@
             display_privacy_notification();
         });
     }
-    
+
     /*
      * Check the page for links related to overlay forms and request the HTML
      * for these forms.
@@ -443,7 +443,7 @@
         Y.one('body').addClass('feature-flag-bugs-private-notification-enabled');
         /* Set the visible flag so that the content moves down. */
         Y.one('body').addClass('global-notification-visible');
-        
+
         if (Y.one('.global-notification').hasClass('hidden')) {
             Y.one('.global-notification').addClass('transparent');
             Y.one('.global-notification').removeClass('hidden');
@@ -464,12 +464,12 @@
                 duration: 0.2,
                 easing: Y.Easing.easeOut
             });
-            
+
             fade_in.run();
             body_space.run();
             login_space.run();
         }
-        
+
         Y.on('click', function(e) {
             hide_privacy_notification(true);
             e.halt();
@@ -509,11 +509,11 @@
             body_space.on('end', function() {
                 Y.one('body').removeClass('global-notification-visible');
             });
-            
+
             fade_out.run();
             body_space.run();
             login_space.run();
-            
+
             if (highlight_portlet) {
                 var portlet_colour = new Y.Anim({
                     node: '.portlet.private',
@@ -521,7 +521,7 @@
                         color: '#fff',
                         backgroundColor:'#8d1f1f'
                     },
-                    duration: 0.4,
+                    duration: 0.4
                 });
                 portlet_colour.run();
             }
@@ -830,6 +830,56 @@
         milestone_choice_edit.render();
     }
     if (Y.Lang.isValue(assignee_content)) {
+        // A validation callback called by the picker when the user selects
+        // an assignee. We check to see if an assignee is a contributor and if
+        // they are not, the user is asked to confirm their selection.
+        var validate_assignee = function(picker, value, save_fn, cancel_fn) {
+            if (value === null) {
+                return true;
+            }
+            var assignee_uri = Y.lp.client.normalize_uri(value.api_uri);
+            assignee_uri = Y.lp.client.get_absolute_uri(assignee_uri);
+            var error_handler = new Y.lp.client.ErrorHandler();
+            error_handler.showError = function(error_msg) {
+                Y.lp.app.errors.display_error(null, error_msg);
+            };
+
+            var process_contributor_result = function(contributor_info) {
+                var is_contributor = contributor_info.is_contributor;
+                if (!is_contributor) {
+                    // Handle assignment to non contributor
+                    var person = Y.Escape.html(contributor_info.person_name);
+                    var pillar = Y.Escape.html(contributor_info.pillar_name);
+                    var yesno_content_template =
+                        "<p>{person_name} did not previously have any " +
+                        "assigned bugs in {pillar}.</p>" +
+                        "<p>Do you really want to assign them to this bug?</p>";
+                    var yesno_content = Y.Lang.substitute(
+                            yesno_content_template,
+                            {person_name: person, pillar: pillar});
+                    Y.lp.app.picker.yesno_save_confirmation(
+                            picker, yesno_content, "Assign", "Choose Again",
+                            save_fn, cancel_fn);
+                } else {
+                    if (Y.Lang.isFunction(save_fn)) {
+                        save_fn();
+                    }
+                }
+            };
+
+            var y_config =  {
+                on: {
+                    success: process_contributor_result,
+                    failure: error_handler.getFailureHandler()
+                },
+                parameters: {
+                    person: assignee_uri
+                }
+            };
+            lp_client.named_get(
+                    conf.bugtask_path, "getContributorInfo", y_config);
+        };
+
         var step_title =
             (conf.assignee_vocabulary == 'ValidAssignee') ?
             "Search for people or teams" :
@@ -844,7 +894,8 @@
              "remove_button_text": "Remove assignee",
              "null_display_value": "Unassigned",
              "show_remove_button": conf.user_can_unassign,
-             "show_assign_me_button": true});
+             "show_assign_me_button": true,
+             "validate_callback": validate_assignee});
         // Ordinary users can select only themselves and their teams.
         // Do not show the team selection, if a user is not a member
         // of any team,
@@ -1058,6 +1109,6 @@
                         "json-parse", "substitute", "widget-position-ext",
                         "lazr.formoverlay", "lazr.anim", "lazr.base",
                         "lazr.overlay", "lazr.choiceedit", "lp.app.picker",
-                        "lp.client",
+                        "lp.client", "escape",
                         "lp.client.plugins", "lp.bugs.bugtask_index.portlets",
                         "lp.bugs.subscriber", "lp.app.errors"]});

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-04-01 14:46:21 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-04-13 07:06:21 +0000
@@ -658,6 +658,15 @@
                     bugtask.sourcepackagenameID == self.sourcepackagenameID):
                     bugtask.sourcepackagenameID = PassthroughValue(new_spnid)
 
+    def getContributorInfo(self, user, person):
+        """See `IBugTask`."""
+        result = {}
+        result['is_contributor'] = person.isBugContributorInTarget(
+            user, self.pillar)
+        result['person_name'] = person.displayname
+        result['pillar_name'] = self.pillar.displayname
+        return result
+
     def getConjoinedMaster(self, bugtasks, bugtasks_by_package=None):
         """See `IBugTask`."""
         conjoined_master = None

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-03-30 13:33:35 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-04-13 07:06:21 +0000
@@ -1355,8 +1355,32 @@
         self.assertNotIn(BugTaskStatus.UNKNOWN, UNRESOLVED_BUGTASK_STATUSES)
 
 
-def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
-    suite.addTest(DocTestSuite('lp.bugs.model.bugtask'))
-    return suite
+class TestBugTaskContributor(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_non_contributor(self):
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(owner=owner)
+        # Create a person who has not contributed
+        person = self.factory.makePerson()
+        result = bug.default_bugtask.getContributorInfo(owner, person)
+        self.assertFalse(result['is_contributor'])
+        self.assertEqual(person.displayname, result['person_name'])
+        self.assertEqual(
+            bug.default_bugtask.pillar.displayname, result['pillar_name'])
+
+    def test_contributor(self):
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product, owner=owner)
+        bug1 = self.factory.makeBug(product=product, owner=owner)
+        # Create a person who has contributed
+        person = self.factory.makePerson()
+        login('[e-mail]')
+        bug1.default_bugtask.transitionToAssignee(person)
+        result = bug.default_bugtask.getContributorInfo(owner, person)
+        self.assertTrue(result['is_contributor'])
+        self.assertEqual(person.displayname, result['person_name'])
+        self.assertEqual(
+            bug.default_bugtask.pillar.displayname, result['pillar_name'])

=== added file 'lib/lp/bugs/windmill/tests/test_bug_inline_assignment.py'
--- lib/lp/bugs/windmill/tests/test_bug_inline_assignment.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/windmill/tests/test_bug_inline_assignment.py	2011-04-13 07:06:21 +0000
@@ -0,0 +1,57 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from lp.bugs.windmill.testing import BugsWindmillLayer
+from lp.testing import WindmillTestCase
+from lp.testing.windmill import lpuser
+from lp.testing.windmill.constants import (
+    FOR_ELEMENT,
+    )
+
+
+class TestInlineAssignment(WindmillTestCase):
+
+    layer = BugsWindmillLayer
+
+    def test_inline_assignment_non_contributor(self):
+        """Test assigning bug to a non contributor displays a confirmation."""
+
+        import transaction
+        # Create a person who has not contributed
+        self.factory.makePerson(name="fred")
+        transaction.commit()
+
+        client, start_url = self.getClientFor(
+            "/firefox/+bug/1", lpuser.SAMPLE_PERSON)
+
+        ASSIGN_BUTTON = (u'//*[@id="affected-software"]//tr//td[5]' +
+            '//button[contains(@class, "yui3-activator-act")]')
+        client.waits.forElement(xpath=ASSIGN_BUTTON, timeout=FOR_ELEMENT)
+        client.click(xpath=ASSIGN_BUTTON)
+
+        VISIBLE_PICKER_OVERLAY = (
+            u'//div[contains(@class, "yui3-picker ") and '
+             'not(contains(@class, "yui3-picker-hidden"))]')
+
+        client.waits.forElement(
+            xpath=VISIBLE_PICKER_OVERLAY, timeout=FOR_ELEMENT)
+
+        def full_picker_element_xpath(element_path):
+            return VISIBLE_PICKER_OVERLAY + element_path
+
+        client.type(xpath=full_picker_element_xpath(
+            "//input[@class='yui3-picker-search']"), text='fred')
+        client.click(xpath=full_picker_element_xpath(
+            "//div[@class='yui3-picker-search-box']/button"))
+
+        PICKER_RESULT = full_picker_element_xpath(
+            "//ul[@class='yui3-picker-results']/li[1]/span")
+
+        client.waits.forElement(xpath=PICKER_RESULT, timeout=FOR_ELEMENT)
+        client.click(xpath=PICKER_RESULT)
+
+        CONFIRMATION = ("//div[@class='important-notice-balloon']")
+        client.waits.forElement(xpath=CONFIRMATION, timeout=FOR_ELEMENT)
+        self.client.asserts.assertTextIn(
+            xpath=CONFIRMATION,
+            validator="Fred did not previously have any assigned bugs")