Merge lp:~wallyworld/launchpad/inline-multicheckbox-widget into lp:launchpad

[Ian Booth]

Add a multicheckbox widget which will PATCH a given attribute on a given resource.

Add a multicheckbox widget which will PATCH a given attribute on a given resource.

== Implementation ==

The widget allows the user to select one of more checkboxes derived from a vocabulary. The vocab is either comes from the field definition of the attribute being edited or can be user specified. When the widget is first rendered, it displays as text the currently selected values. When the user chooses to edit the attribute, a popup displays all available choices with the currently selected items ticked. The user can save/cancel using the tick/cross buttons in the top right of the popup, or click outside the popup to cancel. If the user chooses to save, the widget uses the lp client infrastructure to patch the attribute value on the resource with the new data and re-renders the widget value.

This branch provides the base widget implementation. The lp:~wallyworld/launchpad/inline-recipe-distro-series-edit branch uses the widget to edit the distroseries attribute of a source package recipe.

This branch requires a newer version (rev 206) of lazr-js since it relies on an improvement made to the Activator class.

== Demo and QA ==

A screenshot showing the placement of the edit button:
http://people.canonical.com/~ianb/distroseries-checkbox.png
The edit button is placed to the right of the attribute label.

A screenshot of the widget in action:
http://people.canonical.com/~ianb/distroseries-popup.png

== Tests ==

Added new yui tests for the javascript used by the widget:
lp/app/javascript/tests/test_multicheckboxwidget.js
lp/app/javascript/tests/test_multicheckboxwidget.html

Add new tests for the widget class setup:
lp/app/browser/tests/test_inlinemulticheckboxwidget.py
  test_items_for_field_vocabulary
  test_items_for_custom_vocabulary
  test_items_for_custom_vocabulary_name
  test_selected_items_checked

Add new documentation/tests to lazr-js-widgets.txt

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css.in
  lib/lp/app/browser/lazrjs.py
  lib/lp/app/browser/tests/test_inlinemulticheckboxwidget.py
  lib/lp/app/javascript/multicheckbox.js
  lib/lp/app/javascript/tests/test_multicheckboxwidget.html
  lib/lp/app/javascript/tests/test_multicheckboxwidget.js
  lib/lp/app/templates/inline-multicheckbox-widget.pt

./lib/canonical/launchpad/icing/style-3-0.css.in

./lib/lp/app/browser/lazrjs.py
     248: E501 line too long (80 characters)
     495: E501 line too long (80 characters)
     548: E501 line too long (80 characters)
     248: Line exceeds 78 characters.
     495: Line exceeds 78 characters.
     548: Line exceeds 78 characters.
./lib/lp/app/templates/inline-multicheckbox-widget.pt
       1: unbound prefix

[Aaron Bentley (abentley)]

This looks like useful functionality.

Were you aware the lp_save will detect races where the value has changed since being read? If so, why did you choose to forgo that protection by doing a raw patch?

How are logged-out users handled? Usually, we show them the edit icon, and require them to log in when they click it.

Did you consider accepting "linkify" as a boolean parameter rather than "attribute_type"? Seems simpler, and it's always hard to predict future extensions. You might even be able to eliminate "attribute_type" by calling canonical_url unconditionally and handling exceptions.

I think you could shorten your code by sticking your json attributes in a single dict, and accepting such a dict as the input to addMultiCheckboxPatcher would be perfectly consistent with YUI3 parameter style.

Usually, we indent by four spaces, but there are a bunch of places where you've done more, including: 94, 133, 194, 413, 514-517, 527, 535-537, 539-540, 652-653, 781, 792.

I don't know if we have a style recommendation for how many blank lines to spaces to use at the top level, but if we don't, I suggest using the python style of two blank lines.

According to deryck, who just tested on my behalf, function names are scoped to the closure by default, so I think "foo function()" is better than "var foo = function()". Is there a reason why you're using the latter?

You seem a bit inconsistent about spaces when using "+" for string concatenation. I suggest using a space, e.g. "foo" + "bar", not "foo"+"bar". This is more like python style, and I find it more readable.

On a related note, CHECKBOX_TEMPLATE should use the [str1, str2, ...].join("") form per https://dev.launchpad.net/JavaScriptReviewNotes

The expected_item generation in test_items_for_field_vocabulary, test_items_for_custom_vocabulary and test_items_for_custom_vocabulary_name looks duplicated, and should be extracted to a helper.

review: Needs Information

[Ian Booth (wallyworld)]

> This looks like useful functionality.
>
> Were you aware the lp_save will detect races where the value has changed since
> being read? If so, why did you choose to forgo that protection by doing a raw
> patch?
>

I didn't know that. I based that bit of the code on what was already done in
lp/app/javascript/picker.js, which uses client.patch().
I'll look at changing over to lp_save as see how that works out.

> How are logged-out users handled? Usually, we show them the edit icon, and
> require them to log in when they click it.
>

Yes, that's what happens here too.

> Did you consider accepting "linkify" as a boolean parameter rather than
> "attribute_type"? Seems simpler, and it's always hard to predict future
> extensions. You might even be able to eliminate "attribute_type" by calling
> canonical_url unconditionally and handling exceptions.
>

I considered it but thought it was too restrictive. I agree it's hard to predict the future, but passing in a "type" value defers the decision to what to do with it to the implementing code. It allows other possible behaviours based on attribute type to be transparently implemented. Having a linkify boolean exposes a specific implementation detail to the caller and precludes future work without requiring an api change.

The second idea - to call canonical_url unconditionally and handle exceptions - I also considered and it was 50/50 which approach I went with. I tend to steer away from using exceptions in this way though. To me they are to communicate errors not probe for functionality etc. I settled for the attribute_type parameter to allow for future flexibility but if you feel its a case of YAGNI, I'll happily use the exception approach. I do think the attribute_type approach is a bit ugly but it was the lesser of 2 evils for me.

> I think you could shorten your code by sticking your json attributes in a
> single dict, and accepting such a dict as the input to addMultiCheckboxPatcher
> would be perfectly consistent with YUI3 parameter style.
>

I used the same approach as already used for InlineEditPickerWidget in going with the individual parameters. Note that one of those parameters passed to addMultiCheckboxPatcher() and also addPickerPatcher() in the InlineEditPickerWidget case is actually a dict of config values. I'll find out the rational there and consider applying any improvements to both widgets so they are implemented consistently.

> Usually, we indent by four spaces, but there are a bunch of places where
> you've done more, including: 94, 133, 194, 413, 514-517, 527, 535-537,
> 539-540, 652-653, 781, 792.
>

Yeah, it's a personal preference - I prefer to try and keep the method parameters close to the method name instead of all the way over to the left. I'll fix them.

> I don't know if we have a style recommendation for how many blank lines to
> spaces to use at the top level, but if we don't, I suggest using the python
> style of two blank lines.
>

Ok.

> According to deryck, who just tested on my behalf, function names are scoped
> to the closure by default, so I think "foo function()" is better than "var foo
> = function()". Is there a reason why you're using th...

[Ian Booth (wallyworld)]

I had another look at the javascript infrastructure....

> > Were you aware the lp_save will detect races where the value has changed
> since
> > being read? If so, why did you choose to forgo that protection by doing a
> raw
> > patch?
> >
>
> I didn't know that. I based that bit of the code on what was already done in
> lp/app/javascript/picker.js, which uses client.patch().
> I'll look at changing over to lp_save as see how that works out.
>

AFAICT, the lp_save is designed to be used in cases where the entire object being edited is modified. It relies on onchange events being used to mark attributes as dirty and calls patch() with the entire set of dirty attributes. Here. we are editing a single attribute - "distroseries" - but there is no distroseries DOM node as such that is called "distroseries", hence the dirty detection doesn't work. The current approach using patch() seems to be the correct one given this use case. And I think the web services infrastructure used to do the patch handles the etag stuff too.

> > How are logged-out users handled? Usually, we show them the edit icon, and
> > require them to log in when they click it.
> >
>
> Yes, that's what happens here too.
>

Actually, what happens for this new widget is the same as for the existing pickers - the edit button is not rendered if the user is not logged in.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-03-16 09:15 PM, Ian Booth wrote:
>> Did you consider accepting "linkify" as a boolean parameter rather than
>> "attribute_type"? Seems simpler, and it's always hard to predict future
>> extensions. You might even be able to eliminate "attribute_type" by calling
>> canonical_url unconditionally and handling exceptions.
>>
>
> I considered it but thought it was too restrictive. I agree it's hard to predict the future, but passing in a "type" value defers the decision to what to do with it to the implementing code. It allows other possible behaviours based on attribute type to be transparently implemented. Having a linkify boolean exposes a specific implementation detail to the caller and precludes future work without requiring an api change.
>
> The second idea - to call canonical_url unconditionally and handle exceptions - I also considered and it was 50/50 which approach I went with. I tend to steer away from using exceptions in this way though. To me they are to communicate errors not probe for functionality etc. I settled for the attribute_type parameter to allow for future flexibility but if you feel its a case of YAGNI, I'll happily use the exception approach. I do think the attribute_type approach is a bit ugly but it was the lesser of 2 evils for me.

I think it's a YAGNI, but I don't insist that you change it.

>> I think you could shorten your code by sticking your json attributes in a
>> single dict, and accepting such a dict as the input to addMultiCheckboxPatcher
>> would be perfectly consistent with YUI3 parameter style.
>>
>
> I used the same approach as already used for InlineEditPickerWidget in going with the individual parameters. Note that one of those parameters passed to addMultiCheckboxPatcher() and also addPickerPatcher() in the InlineEditPickerWidget case is actually a dict of config values. I'll find out the rational there and consider applying any improvements to both widgets so they are implemented consistently.

"config" in YUI3 seems to be a pretty loose term. They tend to use it
the same way python uses keyword arguments.

>> According to deryck, who just tested on my behalf, function names are scoped
>> to the closure by default, so I think "foo function()" is better than "var foo
>> = function()". Is there a reason why you're using the latter?
>>
>
> Depends on what existing code I was grepping to provide guidance on what to do. I tended to follow what had gone before.

I've been following what went before too, but the code I looked at used
named functions, so our existing code is probably inconsistent.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2BfHUACgkQ0F+nu1YWqI0HvwCeMZVaCxkYCp7HeHhh7NArKIR5
m74AnjQv1VmpkonwN+rwMYkNO+a3Kx3L
=k9pc
-----END PGP SIGNATURE-----

[Ian Booth (wallyworld)]

>
> >> I think you could shorten your code by sticking your json attributes in a
> >> single dict, and accepting such a dict as the input to
> addMultiCheckboxPatcher
> >> would be perfectly consistent with YUI3 parameter style.
> >>
> >
> > I used the same approach as already used for InlineEditPickerWidget in going
> with the individual parameters. Note that one of those parameters passed to
> addMultiCheckboxPatcher() and also addPickerPatcher() in the
> InlineEditPickerWidget case is actually a dict of config values. I'll find out
> the rational there and consider applying any improvements to both widgets so
> they are implemented consistently.
>
> "config" in YUI3 seems to be a pretty loose term. They tend to use it
> the same way python uses keyword arguments.
>

I did some more digging and I think what's there now is appropriate. The addMultiCheckboxPatcher() function takes a combination of explicit parameters and a config dict(). The parameters are used by the addMultiCheckboxPatcher function itself to set things up. addMultiCheckboxPatcher is not a widget per se, but it does construct a widget to do the actual work, in this case PrettyOverlay. The config parameter passed to addMultiCheckboxPatcher is in turn augmented with additional attribute values and then passed to the constructor of PrettyOverlay in the manner expected for yui widgets.

So I think we should stick with the current implementation given the nature of the code. The other requested changes have been made.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-03-16 10:55 PM, Ian Booth wrote:
> AFAICT, the lp_save is designed to be used in cases where the entire object being edited is modified.

My belief, too.

> It relies on onchange events being used to mark attributes as dirty and calls patch() with the entire set of dirty attributes.

Okay.

> Here. we are editing a single attribute - "distroseries" - but there is no distroseries DOM node as such that is called "distroseries", hence the dirty detection doesn't work.

The web service doesn't deal in DOM nodes AFAIK. It deals in json
structures and Entities, and such.

But I guess you mean that there's no attribute called "distroseries" in
the API, rather a distroseries_collection_link?

Anyhow, are you saying that lp_save doesn't work with collections at all?

> The current approach using patch() seems to be the correct one given this use case.
> And I think the web services infrastructure used to do the patch
handles the etag stuff too.

The etag stuff was what I was talking about. I can't see anything
handling them, and I don't know how it would acquire information about
the previous etag of the distroseries_collection_link.

That said, if this is the only way that works, then clearly it's the
right way.

 status approved

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2CYDAACgkQ0F+nu1YWqI3LSACeI6IlWwYHeFrUXJrTuV9smh97
BdYAmQGbcmCbxeCJSHDi6vwHMugVIiEf
=gWkA
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2011-03-08 00:40:50 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2011-03-18 07:43:37 +0000
@@ -791,6 +791,13 @@
 button {
     padding:0;
     }
+button.overlay-close-button {
+    float: right;
+    width: 15px;
+    height: 15px;
+    display: block;
+    margin-top: 4px;
+    }
 .fieldRequired, .fieldOptional {
     color: #999;
     }

=== modified file 'lib/lp/app/browser/lazrjs.py'
--- lib/lp/app/browser/lazrjs.py	2011-03-10 01:25:13 +0000
+++ lib/lp/app/browser/lazrjs.py	2011-03-18 07:43:37 +0000
@@ -8,6 +8,7 @@
     'BooleanChoiceWidget',
     'EnumChoiceWidget',
     'InlineEditPickerWidget',
+    'InlineMultiCheckboxWidget',
     'standard_text_html_representation',
     'TextAreaEditorWidget',
     'TextLineEditorWidget',
@@ -19,7 +20,10 @@
 from zope.app.pagetemplate.viewpagetemplatefile import ViewPageTemplateFile
 from zope.component import getUtility
 from zope.security.checker import canAccess, canWrite
-from zope.schema.interfaces import IVocabulary
+from zope.schema.interfaces import (
+    ICollection,
+    IVocabulary,
+    )
 from zope.schema.vocabulary import getVocabularyRegistry
 
 from lazr.enum import IEnumeratedType
@@ -301,6 +305,114 @@
         return user and user in vocabulary
 
 
+class InlineMultiCheckboxWidget(WidgetBase):
+    """Wrapper for the lazr-js multicheckbox widget."""
+
+    __call__ = ViewPageTemplateFile(
+                        '../templates/inline-multicheckbox-widget.pt')
+
+    def __init__(self, context, exported_field,
+                 label, label_tag="span", attribute_type="default",
+                 vocabulary=None, header=None,
+                 empty_display_value="None", selected_items=list(),
+                 items_tag="span", items_style='',
+                 content_box_id=None, edit_view="+edit", edit_url=None,
+                 edit_title=''):
+        """Create a widget wrapper.
+
+        :param context: The object that is being edited.
+        :param exported_field: The attribute being edited. This should be
+            a field from an interface of the form ISomeInterface['fieldname']
+        :param label: The label text to display above the checkboxes
+        :param label_tag: The tag in which to wrap the label text.
+        :param attribute_type: The attribute type. Currently only "reference"
+            is supported. Used to determine whether to linkify the selected
+            checkbox item values. So ubuntu/hoary becomes
+            http://launchpad.net/devel/api/ubuntu/hoary
+        :param vocabulary: The name of the vocabulary which provides the
+            items or a vocabulary instance.
+        :param header: The text to display as the title of the popup form.
+        :param empty_display_value: The text to display if no items are
+            selected.
+        :param selected_items: The currently selected items.
+        :param items_tag: The tag in which to wrap the items checkboxes.
+        :param items_style: The css style to use for each item checkbox.
+        :param content_box_id: The HTML id to use for this widget.
+            Automatically generated if this is not provided.
+        :param edit_view: The view name to use to generate the edit_url if
+            one is not specified.
+        :param edit_url: The URL to use for editing when the user isn't logged
+            in and when JS is off.  Defaults to the edit_view on the context.
+        :param edit_title: Used to set the title attribute of the anchor.
+
+        """
+        super(InlineMultiCheckboxWidget, self).__init__(
+            context, exported_field, content_box_id,
+            edit_view, edit_url, edit_title)
+
+        linkify_items = attribute_type == "reference"
+
+        if header is None:
+            header = self.exported_field.title+":"
+        self.header = header,
+        self.empty_display_value = empty_display_value
+        self.label = label
+        self.label_open_tag = "<%s>" % label_tag
+        self.label_close_tag = "</%s>" % label_tag
+        self.items = selected_items
+        self.items_open_tag = ("<%s id='%s'>" %
+            (items_tag, self.content_box_id+"-items"))
+        self.items_close_tag = "</%s>" % items_tag
+        self.linkify_items = linkify_items
+
+        if vocabulary is None:
+            if ICollection.providedBy(exported_field):
+                vocabulary = exported_field.value_type.vocabularyName
+            else:
+                vocabulary = exported_field.vocabularyName
+
+
+        if isinstance(vocabulary, basestring):
+            vocabulary = getVocabularyRegistry().get(context, vocabulary)
+
+        # Construct checkbox data dict for each item in the vocabulary.
+        items = []
+        style = ';'.join(['font-weight: normal', items_style])
+        for item in vocabulary:
+            item_value = item.value if safe_hasattr(item, 'value') else item
+            checked = item_value in selected_items
+            if linkify_items:
+                save_value = canonical_url(item_value, force_local_path=True)
+            else:
+                save_value = item_value.name
+            new_item = {
+                'name': item.title,
+                'token': item.token,
+                'style': style,
+                'checked': checked,
+                'value': save_value}
+            items.append(new_item)
+        self.has_choices = len(items)
+
+        # JSON encoded attributes.
+        self.json_content_box_id = simplejson.dumps(self.content_box_id)
+        self.json_attribute = simplejson.dumps(self.api_attribute)
+        self.json_attribute_type = simplejson.dumps(attribute_type)
+        self.json_items = simplejson.dumps(items)
+        self.json_description = simplejson.dumps(
+            self.exported_field.description)
+
+    @property
+    def config(self):
+        return dict(
+            header=self.header,
+            )
+
+    @property
+    def json_config(self):
+        return simplejson.dumps(self.config)
+
+
 def vocabulary_to_choice_edit_items(
     vocab, css_class_prefix=None, disabled_items=None, as_json=False,
     name_fn=None, value_fn=None):

=== added file 'lib/lp/app/browser/tests/test_inlinemulticheckboxwidget.py'
--- lib/lp/app/browser/tests/test_inlinemulticheckboxwidget.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_inlinemulticheckboxwidget.py	2011-03-18 07:43:37 +0000
@@ -0,0 +1,82 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the InlineMultiCheckboxWidget."""
+
+__metaclass__ = type
+
+import simplejson
+
+from zope.interface import Interface
+from zope.schema import List
+from zope.schema._field import Choice
+from zope.schema.vocabulary import getVocabularyRegistry
+
+from lazr.enum import EnumeratedType, Item
+
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.lazrjs import InlineMultiCheckboxWidget
+from lp.testing import (
+    TestCaseWithFactory,
+    )
+
+
+class Alphabet(EnumeratedType):
+    """A vocabulary for testing."""
+    A = Item("A", "Letter A")
+    B = Item("B", "Letter B")
+
+
+class TestInlineMultiCheckboxWidget(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def _getWidget(self, **kwargs):
+
+        class ITest(Interface):
+            test_field = List(
+                Choice(vocabulary='BuildableDistroSeries'))
+        return InlineMultiCheckboxWidget(
+            None, ITest['test_field'], "Label", edit_url='fake', **kwargs)
+
+    def _makeExpectedItems(self, vocab, selected=list(), value_fn=None):
+        if value_fn is None:
+            value_fn = lambda item: item.value.name
+        expected_items = []
+        style = 'font-weight: normal;'
+        for item in vocab:
+            new_item = {
+                'name': item.title,
+                'token': item.token,
+                'style': style,
+                'checked': (item.value in selected),
+                'value': value_fn(item)}
+            expected_items.append(new_item)
+        return expected_items
+
+    def test_items_for_field_vocabulary(self):
+        widget = self._getWidget(attribute_type="reference")
+        vocab = getVocabularyRegistry().get(None, 'BuildableDistroSeries')
+        value_fn = lambda item: canonical_url(
+            item.value, force_local_path=True)
+        expected_items = self._makeExpectedItems(vocab, value_fn=value_fn)
+        self.assertEqual(simplejson.dumps(expected_items), widget.json_items)
+
+    def test_items_for_custom_vocabulary(self):
+        widget = self._getWidget(vocabulary=Alphabet)
+        expected_items = self._makeExpectedItems(Alphabet)
+        self.assertEqual(simplejson.dumps(expected_items), widget.json_items)
+
+    def test_items_for_custom_vocabulary_name(self):
+        widget = self._getWidget(vocabulary="CountryName")
+        vocab = getVocabularyRegistry().get(None, "CountryName")
+        expected_items = self._makeExpectedItems(vocab)
+        self.assertEqual(simplejson.dumps(expected_items), widget.json_items)
+
+    def test_selected_items_checked(self):
+        widget = self._getWidget(
+            vocabulary=Alphabet, selected_items=[Alphabet.A])
+        expected_items = self._makeExpectedItems(
+            Alphabet, selected=[Alphabet.A])
+        self.assertEqual(simplejson.dumps(expected_items), widget.json_items)

=== modified file 'lib/lp/app/doc/lazr-js-widgets.txt'
--- lib/lp/app/doc/lazr-js-widgets.txt	2011-03-10 00:40:15 +0000
+++ lib/lp/app/doc/lazr-js-widgets.txt	2011-03-18 07:43:37 +0000
@@ -399,3 +399,95 @@
 
 The edit link can be changed in exactly the same way as for the
 TextLineEditorWidget above.
+
+
+InlineMultiCheckboxWidget
+-------------------
+
+This widget is used to edit fields which are Lists or Sets. It displays the
+current items in the collection when the page is rendered and provides the
+ability to edit the selected items via a popup overlay. The popup has a set of
+checkboxes for selecting one or more items from a vocabulary. The vocabulary
+defaults to that associated with the field being edited but can be user
+defined.
+
+    >>> from lp.app.browser.lazrjs import InlineMultiCheckboxWidget
+
+The bare minimum that you need to provide the widget is the object that you
+are editing, and the exported field that is being edited, and the label to
+display for the set of checkboxes.
+
+The surrounding tag for the label and set of checkboxes are both customisable,
+and a prefix may be given.  The prefix is rendered as part of the widget, but
+isn't updated when the value changes.
+
+Other customisable parameters include the popup header text (defaults to the
+field title suffixed by ":"), the string to render when the field contains no
+selected items (defaults to "None"), and a CSS style to add to each checkbox
+node (defaults to '').
+
+If the user does not have edit rights, the widget just renders the text based
+on the current value of the field on the object:
+
+    >>> login(ANONYMOUS)
+    >>> from lp.code.interfaces.sourcepackagerecipe import (
+    ...     ISourcePackageRecipe,
+    ...     )
+    >>> distroseries = ISourcePackageRecipe['distroseries']
+    >>> recipe = factory.makeSourcePackageRecipe(
+    ...     owner=eric, name=u'cake_recipe', description=u'Yummy.')
+    >>> widget = InlineMultiCheckboxWidget(
+    ...     recipe, distroseries, 'Recipe distro series',
+    ...     header='Select distroseries:', vocabulary='BuildableDistroSeries',
+    ...     label_tag='dt', items_tag='dl',
+    ...     selected_items=recipe.distroseries)
+    >>> print widget()
+    <span id="edit-distroseries">
+      <dt>
+        Recipe distro series
+      </dt>
+      <span class="yui3-activator-data-box">
+        <dl id='edit-distroseries-items'>
+    ...
+      </span>
+      <div class="yui3-activator-message-box yui3-activator-hidden" />
+    </span>
+
+If the user has edit rights, an edit icon is rendered and some javascript is
+rendered to hook up the widget.
+
+    >>> login_person(eric)
+    >>> print widget()
+    <span id="edit-distroseries">
+    <BLANKLINE>
+      <dt>
+        Recipe distro series
+    <BLANKLINE>
+          <button class="lazr-btn yui3-activator-act yui3-activator-hidden"
+                  id="edit-distroseries-btn">
+            Edit
+          </button>
+    <BLANKLINE>
+        <noscript>
+          <a class="sprite edit"
+             href="http://code.launchpad.dev/~eric/+recipe/cake_recipe/+edit"
+             title=""></a>
+        </noscript>
+      </dt>
+    <BLANKLINE>
+      <span class="yui3-activator-data-box">
+        <dl id='edit-distroseries-items'>
+    ...
+      <div class="yui3-activator-message-box yui3-activator-hidden" />
+      </span>
+      <script>
+      LPS.use('lp.app.multicheckbox', function(Y) {
+      ...
+      </script>
+
+
+Changing the edit link
+**********************
+
+The edit link can be changed in exactly the same way as for the
+TextLineEditorWidget above.

=== added file 'lib/lp/app/javascript/multicheckbox.js'
--- lib/lp/app/javascript/multicheckbox.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/multicheckbox.js	2011-03-18 07:43:37 +0000
@@ -0,0 +1,232 @@
+YUI.add('lp.app.multicheckbox', function(Y) {
+
+var namespace = Y.namespace('lp.app.multicheckbox');
+
+/* Add a multicheckbox widget which will PATCH a given attribute on
+ * a given resource.
+ *
+ * @method addMultiCheckboxPatcher
+ * @param {Array} items The items which to display as checkboxes.
+ * @param {String} help_text The text to display beneath the checkboxes.
+ * @param {String} resource_uri The object being modified.
+ * @param {String} attribute_name The attribute on the resource being
+ *                                modified.
+ * @param {String} attribute_type The attribute type.
+ *     "reference": the items are object references
+ *     Other values are currently ignored.
+ * @param {String} content_box_id
+ * @param {Object} config Object literal of config name/value pairs.
+ *     config.header: a line of text at the top of the widget.
+ *     config.step_title: overrides the subtitle.
+ */
+namespace.addMultiCheckboxPatcher = function (
+    items, help_text, resource_uri, attribute_name, attribute_type,
+    content_box_id, config, client) {
+
+
+    if (Y.UA.ie) {
+        return;
+    }
+
+    // We may have been passed a mock client for testing but if not, create
+    // a proper one.
+    if (client === undefined)
+        client = new Y.lp.client.Launchpad();
+
+    var content_box = Y.one('#'+content_box_id);
+    var result_node = Y.one('#'+content_box_id+'-items');
+    var widget_node = Y.one('#'+attribute_name);
+    var activator = new Y.lazr.activator.Activator(
+        {contentBox: content_box, animationNode: widget_node});
+
+    var failure_handler = function (id, response, args) {
+        activator.renderFailure(
+            Y.Node.create(
+                '<div>' + response.statusText +
+                    '<pre>' + response.responseText + '</pre>' +
+                '</div>'));
+    };
+
+    // The function called to save the selected items.
+    function save(editform, item_value_mapping) {
+        var choice_nodes = Y.one('[id="'+attribute_name+'.items"]');
+        var result = namespace.getSelectedItems(
+            choice_nodes, item_value_mapping, attribute_type);
+        activator.renderProcessing();
+        var success_handler = function (entry) {
+            var xhtml = entry.getHTML(attribute_name);
+            result_node.set('innerHTML', xhtml);
+            activator.renderSuccess(result_node);
+        };
+
+        var patch_payload = {};
+        patch_payload[attribute_name] = result;
+        client.patch(editform._resource_uri, patch_payload, {
+            accept: 'application/json;include=lp_html',
+            on: {
+                success: success_handler,
+                failure: failure_handler
+            }
+        });
+    }
+
+    config.save = save;
+    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"/>');
+    activator.subscribe('act', function (e) {
+        editform.show();
+    });
+    activator.render();
+    return editform;
+};
+
+
+/**
+  * Creates a multicheckbox widget that has already been rendered and hidden.
+  *
+  * @requires dom, lazr.activator, lazr.overlay
+  * @method create
+  * @param {String} attribute_name The attribute on the resource being
+  *                                modified.
+  * @param {Array} items Items for which to create checkbox elements.
+  * @param {String} help_text text display below the checkboxes.
+  * @param {Object} config Optional Object literal of config name/value pairs.
+  *                        config.header is a line of text at the top of
+  *                        the widget.
+  *                        config.save is a Function (optional) which takes
+  *                        a single string argument.
+  */
+namespace.create = function (attribute_name, items, help_text, config) {
+    if (Y.UA.ie) {
+        return;
+    }
+
+    if (config !== undefined) {
+        var header = 'Choose an item.';
+        if (config.header !== undefined) {
+            header = config.header;
+        }
+    }
+
+    var new_config = Y.merge(config, {
+        align: {
+            points: [Y.WidgetPositionAlign.CC,
+                     Y.WidgetPositionAlign.CC]
+        },
+        progressbar: true,
+        progress: 100,
+        headerContent: "<h2>" + header + "</h2>",
+        centered: true,
+        zIndex: 1000,
+        visible: false
+        });
+
+    // We use a pretty overlay to display the checkboxes.
+    var editform = new Y.lazr.PrettyOverlay(new_config);
+
+    // The html for each checkbox.
+    var CHECKBOX_TEMPLATE =
+        ['<label style="{item_style}" for="{field_name}.{field_index}">',
+        '<input id="{field_name}.{field_index}" ',
+        'name="{field_name}.{field_index}" ',
+        'class="checkboxType" type="checkbox" value="{field_value}" ',
+        '{item_checked}>&nbsp;{field_text}</label>'].join("");
+
+    var content = Y.Node.create("<div/>");
+    var header_node = Y.Node.create(
+        "<div class='yui3-lazr-formoverlay-form-header'/>");
+    content.appendChild(header_node);
+    var body = Y.Node.create("<div class='yui3-widget-bd'/>");
+
+    // Set up the nodes for each checkbox.
+    var choices_nodes = Y.Node.create('<ul id="'+attribute_name+'.items"/>');
+    // A mapping from checkbox value attributes (data token) -> data values
+    var item_value_mapping = {};
+    Y.Array.each(items, function(data, i) {
+        var checked_html = '';
+        if (data.checked)
+            checked_html = 'checked="checked"';
+        var checkbox_html = Y.Lang.substitute(
+            CHECKBOX_TEMPLATE,
+            {field_name: "field."+attribute_name, field_index:i,
+            field_value: data.token, field_text: data.name,
+            item_style: data.style, item_checked: checked_html});
+
+        var choice_item = Y.Node.create("<li/>");
+        choice_item.set("innerHTML", checkbox_html);
+        choices_nodes.appendChild(choice_item);
+        item_value_mapping[data.token] = data.value;
+    }, this);
+    body.appendChild(choices_nodes);
+    content.appendChild(body);
+    var help_node = Y.Node.create("<p class='formHelp'>"+help_text+"</p>");
+    content.appendChild(help_node);
+    editform.set('bodyContent', content);
+
+    // We replace the default Close button (x) with our own save/cancel ones.
+    var close_node = editform.get('boundingBox').one('div.close');
+    var orig_close_button = close_node.one('a');
+    orig_close_button.setAttribute('style', 'display: none');
+    var save_button = Y.Node.create(
+        '<button id="'+config.content_box_id+'-save" ' +
+        'class="overlay-close-button lazr-pos lazr-btn">Ok</button>');
+    var close_button = Y.Node.create(
+        '<button class="overlay-close-button lazr-neg lazr-btn">Cancel' +
+        '</button>');
+    save_button.on('click', function(e) {
+        e.halt();
+        editform.hide();
+        config.save(editform, item_value_mapping);
+    });
+    close_button.on('click', function(e) {
+        e.halt();
+        editform.fire('cancel');
+    });
+    close_node.appendChild(close_button);
+    close_node.appendChild(save_button);
+    editform.render();
+    editform.hide();
+    return editform;
+};
+
+
+/*
+ * Return a list of the selected checkbox values.
+ * Exposed via the namespace so it is accessible to tests.
+ */
+namespace.getSelectedItems = function(choice_nodes, item_value_mapping,
+                                      attribute_type) {
+    var result = [];
+    choice_nodes.all('.checkboxType').each(function(item) {
+        if (item.get("checked")) {
+            var item_token = item.getAttribute("value");
+            var item_value = item_value_mapping[item_token];
+            var marshalled_value = marshall(item_value, attribute_type);
+            result.push(marshalled_value);
+        }
+    });
+    return result;
+};
+
+
+/*
+ * Transform the selected value according to the attribute type we are editing
+ */
+function marshall(value, attribute_type) {
+    switch (attribute_type) {
+        case "reference":
+            var item_value = Y.lp.client.normalize_uri(value);
+            return Y.lp.client.get_absolute_uri(item_value);
+        break;
+        default:
+            return value;
+    }
+}
+
+}, "0.1", {"requires": [
+    "dom", "lazr.overlay", "lazr.activator", "lp.client"
+    ]});

=== added file 'lib/lp/app/javascript/tests/test_multicheckboxwidget.html'
--- lib/lp/app/javascript/tests/test_multicheckboxwidget.html	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_multicheckboxwidget.html	2011-03-18 07:43:37 +0000
@@ -0,0 +1,42 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
+  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
+<html>
+  <head>
+  <title>Multicheckbox Widget</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/lazr.js"></script>
+  <script type="text/javascript" src="../client.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../multicheckbox.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_multicheckboxwidget.js"></script>
+</head>
+<body class="yui3-skin-sam">
+  <span id="edit-test">
+    <span id="multicheckboxtest">
+      <span>
+        A multicheckbox widget test
+        <button id="edit-multicheckboxtest-btn"
+                class="lazr-btn yui3-activator-act yui3-activator-hidden">
+          Edit
+        </button>
+      </span>
+       <span class="yui3-activator-data-box">
+           <span id="edit-test-items"/>
+       </span>
+      <div class="yui3-activator-message-box yui3-activator-hidden"/>
+    </span>
+  </span>
+  <div id="log"></div>
+</body>
+</html>
\ No newline at end of file

=== added file 'lib/lp/app/javascript/tests/test_multicheckboxwidget.js'
--- lib/lp/app/javascript/tests/test_multicheckboxwidget.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_multicheckboxwidget.js	2011-03-18 07:43:37 +0000
@@ -0,0 +1,184 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw',
+    combine: false,
+    fetchCSS: false
+    }).use('test', 'console', 'dom', 'event', 'event-simulate',
+        'lazr.overlay', 'lazr.activator',
+        'lp.client', 'lp.app.multicheckbox',
+        function(Y) {
+
+var suite = new Y.Test.Suite("lp.app.multicheckbox Tests");
+
+/*
+ * 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 MockClient = function() {
+    /* A mock to provide the result of a patch operation. */
+};
+
+MockClient.prototype = {
+    'patch': function(uri, representation, config, headers) {
+        var patch_content = new Y.lp.client.Entry();
+        var html = representation['multicheckboxtest'];
+        patch_content.set('lp_html', {'multicheckboxtest': html});
+        config.on.success(patch_content);
+    }
+};
+
+suite.add(new Y.Test.Case({
+    name: "lp.app.multicheckbox",
+
+    createWidget: function(header) {
+      // Create a widget with some default data
+      var config = {"empty_display_value": "None"};
+      if( header != 'None' ) {
+        if( header == null )
+            header = 'Test multicheckbox widget:';
+        config['header'] = header;
+      }
+
+      var mock_client = new MockClient();
+      this.widget = Y.lp.app.multicheckbox.addMultiCheckboxPatcher(
+        [{"token": "0", "style": "font-weight: normal;", "checked": true,
+            "name": "Item 0", "value": "item1"},
+        {"token": "1", "style": "font-weight: normal;", "checked": false,
+            "name": "Item 1", "value": "item2"}],
+        'A test',
+        '/~fred/+recipe/a-recipe',
+        'multicheckboxtest',
+        'reference',
+        'edit-test',
+        config, mock_client);
+    },
+
+    tearDown: function() {
+        cleanup_widget(this.widget);
+    },
+
+    test_widget_can_be_instantiated: function() {
+        this.createWidget();
+        Y.Assert.isInstanceOf(
+            Y.lazr.PrettyOverlay, this.widget,
+            "Widget failed to be instantiated");
+    },
+
+    test_header_value: function() {
+        // Check the header text value.
+        this.createWidget();
+        var header = Y.one('.yui3-widget-hd');
+        Y.Assert.areEqual(
+            header.get('innerHTML'), '<h2>Test multicheckbox widget:</h2>');
+    },
+
+    test_default_header_value: function() {
+        // Check the default header text value.
+        this.createWidget(header='None');
+        var header = Y.one('.yui3-widget-hd');
+        Y.Assert.areEqual(
+            header.get('innerHTML'), '<h2>Choose an item.</h2>');
+    },
+
+    test_help_text_value: function() {
+        // Check the help text value.
+        this.createWidget();
+        var header = Y.one('.yui3-widget-bd p.formHelp');
+        Y.Assert.areEqual(
+            header.get('innerHTML'), 'A test');
+    },
+
+    test_widget_has_correct_choice_data: function() {
+        // Make sure the checkboxes are rendered with expected data values..
+        this.createWidget();
+        for( var x=0; x<2; x++ ) {
+            var item = Y.one(
+                  'input[id="field.multicheckboxtest.'+x+'][type=checkbox]');
+            Y.Assert.areEqual(item.getAttribute('value'), x);
+            Y.Assert.areEqual(item.get('checked'), x==0);
+        }
+    },
+
+    test_widget_has_correct_choice_text: function() {
+        // Make sure the checkboxes are rendered with expected label text.
+        this.createWidget();
+        for( var x=0; x<2; x++ ) {
+            var item = Y.one('label[for="field.multicheckboxtest.'+x+']');
+            var txt = item.get('textContent');
+            //remove any &nbsp in the text
+            txt = txt.replace(/^[\s\xA0]+/g,'').replace(/[\s(&nbsp;)]+$/g,'');
+            Y.Assert.areEqual(txt, 'Item '+ x);
+        }
+    },
+
+    test_getSelectedItems: function() {
+        // Test that getSelectedItems returns the correct values.
+        this.createWidget();
+        var items = Y.one('[id="multicheckboxtest.items"]');
+        var mapping = {0: 'Item0', 1: 'Item1'};
+        var selected = Y.lp.app.multicheckbox.getSelectedItems(
+            items, mapping, '');
+        Y.ArrayAssert.itemsAreEqual(['Item0'], selected);
+    },
+
+    test_marshall_references: function() {
+        // Test that getSelectedItems returns the correct values for reference
+        // values which are links to domain objects.
+        this.createWidget();
+        var items = Y.one('[id="multicheckboxtest.items"]');
+        var mapping = {0: '/ubuntu/Item0', 1: '/ubuntu/Item1'};
+        var selected = Y.lp.app.multicheckbox.getSelectedItems(
+            items, mapping, 'reference');
+        var item_value = Y.lp.client.normalize_uri(selected[0]);
+        var link = Y.lp.client.get_absolute_uri(item_value);
+        Y.ArrayAssert.itemsAreEqual([link], selected);
+    },
+
+    test_widget_content_from_patch_success: function() {
+        // Test that when the user clicks "save" and the result comes back,
+        // the DOM is correctly updated.
+        this.createWidget();
+        simulate(this.widget.get('boundingBox'), '#edit-test-save', 'click');
+        var test_content = 'file:///api/devel/item1';
+        var items = Y.one('[id="edit-test-items"]');
+        Y.Assert.areEqual(test_content, items.get('innerHTML'));
+    }
+
+}));
+
+// 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();
+});
+});

=== added file 'lib/lp/app/templates/inline-multicheckbox-widget.pt'
--- lib/lp/app/templates/inline-multicheckbox-widget.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/app/templates/inline-multicheckbox-widget.pt	2011-03-18 07:43:37 +0000
@@ -0,0 +1,50 @@
+<span tal:define="items view/items|nothing"
+     tal:attributes="id view/content_box_id">
+  <tal:label-open-tag replace="structure view/label_open_tag"/>
+    <span tal:replace="structure view/label"/>
+    <tal:has_choices condition="python:view.has_choices and view.can_write">
+      <button tal:attributes="id string:${view/content_box_id}-btn"
+              class="lazr-btn yui3-activator-act yui3-activator-hidden">
+        Edit
+      </button>
+    </tal:has_choices>
+    <noscript tal:condition="view/can_write">
+      <a tal:attributes="href view/edit_url;
+                         title view/edit_title"
+         class="sprite edit"></a>
+    </noscript>
+  <tal:label-close-tag replace="structure view/label_close_tag"/>
+  <span class="yui3-activator-data-box">
+    <tal:items-open-tag replace="structure view/items_open_tag"/>
+      <span tal:condition="not:items" tal:content="structure view/empty_display_value"/>
+        <ul tal:condition="items">
+              <li tal:condition="view/linkify_items"
+                  tal:repeat="item items"
+                  tal:content="structure item/fmt:link"/>
+              <li tal:condition="not:view/linkify_items"
+                  tal:repeat="item items"
+                  tal:content="structure item"/>
+        </ul>
+    <tal:items-close-tag replace="structure view/items_close_tag"/>
+  </span>
+  <div class="yui3-activator-message-box yui3-activator-hidden"/>
+</span>
+<script tal:condition="view/can_write"
+        tal:content="structure string:
+LPS.use('lp.app.multicheckbox', function(Y) {
+    if (Y.UA.ie) {
+        return;
+    }
+
+    Y.on('load', function(e) {
+      Y.lp.app.multicheckbox.addMultiCheckboxPatcher(
+        ${view/json_items},
+        ${view/json_description},
+        ${view/json_resource_uri},
+        ${view/json_attribute},
+        ${view/json_attribute_type},
+        ${view/json_content_box_id},
+        ${view/json_config});
+      }, window);
+});
+"/>

=== modified file 'versions.cfg'
--- versions.cfg	2011-03-15 21:09:44 +0000
+++ versions.cfg	2011-03-18 07:43:37 +0000
@@ -39,7 +39,7 @@
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1
 lazr.uri = 1.0.2
-lazr-js = 1.6DEV-r202
+lazr-js = 1.6DEV-r206
 manuel = 1.1.1
 martian = 0.11
 mechanize = 0.1.11