Hi Tom,

I have a lot of comments about this branch, but I think it's a nice
feature and it works well.

Gavin.


> === modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
> --- lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-08-25 10:55:16 +0000
> +++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-08-26 11:02:06 +0000
> @@ -1208,6 +1208,7 @@
>      }
>
>      var tr = Y.get('#' + conf.row_id);
> +    var bugtarget_content = Y.get('#bugtarget-picker-' + conf.row_id);
>      var status_content = tr.query('.status-content');
>      var importance_content = tr.query('.importance-content');
>      var milestone_content = tr.query('.milestone-content');
> @@ -1223,6 +1224,20 @@
>      }
>
>      if ((LP.client.links.me !== undefined) && (LP.client.links.me !== null))  {
> +        if (Y.Lang.isValue(bugtarget_content)) {
> +            if (conf.target_is_product) {
> +                var bugtarget_picker = Y.lp.picker.addPickerPatcher(
> +                    'Product',
> +                    conf.bugtask_path,
> +                    "target_link",
> +                    bugtarget_content.get('id'),
> +                    true,
> +                    false,
> +                    {"step_title": "Search products",
> +                     "header": "Change product"});
> +            }
> +        }
> +
>          if (conf.user_can_edit_status) {
>              var status_choice_edit = new Y.ChoiceSource({
>                  contentBox: status_content,
>
> === modified file 'lib/canonical/launchpad/javascript/lp/picker.js'
> --- lib/canonical/launchpad/javascript/lp/picker.js	2009-08-05 14:02:16 +0000
> +++ lib/canonical/launchpad/javascript/lp/picker.js	2009-08-26 11:02:06 +0000
> @@ -78,8 +78,23 @@
>      var save = function (picker_result) {
>          activator.renderProcessing();
>          var success_handler = function (entry) {
> -            var node = Y.Node.create(
> -                '<span>' + entry.get('assignee_link') + '</span>');
> +            var xhtml = Y.DOM.create(entry)[1];

This is fragile; the reason is that the first element returned from
Y.DOM.create() is a text node containing "\n\n", which is the text
after the XML PI and the start of the dl. If the rendering of this
ever changes it'll break.

The following strips the XML PI to demonstrate the problem.

    entry = entry.replace(/<[?].+[?]>/, "");
    var xhtml = Y.DOM.create(entry); // No [1]

It seems that Y.DOM.create considers whitespace after the PI to be
significant, while unenclosed whitespace is not. I don't know if this
is correct behaviour, but it's annoying.

Perhaps it's possible to change the XML renderer to remove this
whitespace? Or add a test to make sure it's always there? Mmm, but
even then, I just get a gut feeling that this is not behaviour we
should rely upon in YUI; it might change. Gah! Software is sh*t :)

> +            var current_field = null;
> +            for (var i = 0; i < xhtml.childNodes.length; i++) {
> +                var element = xhtml.childNodes[i];
> +                if (element.tagName == 'DT') {
> +                    current_field = element.innerHTML;

We control this field, so I guess it's not so bad, but using innerHTML
is not really the right thing to do, because it contains the raw HTML,
not the decoded data we're interested in. In this case it's the same,
so perhaps I'm being over cautious.

> +                    continue;
> +                }
> +                if (element.tagName == 'DD') {
> +                    if (current_field == attribute_name) {
> +                        // The field value is found
> +                        node = Y.get(element).query('span');
> +                    } else if (current_field == 'self_link') {
> +                        picker._resource_uri = element.innerHTML;

Same here.

> +                    }
> +                }
> +            }

This loop is a pain, but I guess it's necessary. You could use YUI to
help a bit here I think:

    var xhtml = Y.get(Y.DOM.create(entry));
    var dts = xhtml.queryAll('dt');
    var dds = xhtml.queryAll('dd');
    for (var i = 0; i < dts.size(); i++) {
        var name = dts.item(i).get('text'); // text
        var value = dds.item(i); // node
        if (name == attribute_name) {
            // The field value is found
            node = value.query('span');
        } else if (name == 'self_link') {
            picker._resource_uri = value.get('text');
        }
    }

(That also fixes the innerHTML problem.)

A function that takes a dl (or the XML for a dl) and returns an object
mapping would be something useful and reusable to factor out of
this... perhaps something like:

    var mapping = {},
        dts = xhtml.queryAll('dt'),
        dds = xhtml.queryAll('dd');
    for (var i = 0; i < dts.size(); i++) {
        mapping[dts.item(i).get('text')] = dds.item(i);
    }
    return mapping;

>              activator.renderSuccess(node);
>              show_hide_buttons();
>          };
> @@ -89,7 +104,7 @@
>              picker_result.api_uri);
>
>          var client = new LP.client.Launchpad();
> -        client.patch(resource_uri, patch_payload, {
> +        client.patch(picker._resource_uri, patch_payload, {

Because this is odd looking, please can you add a comment as to why
this is necessary.

I noticed a cool side-effect here which you might want to exploit. If
someone else changes the target while you've got the page up, then you
go to change the target, a NotImplementedError comes back from
this_is_a_null_bugtask_method. This error could be transformed into a
"the target has been changed elsewhere" message. If that's interesting
then consider filing a bug about it, it's not something for this
branch.

>              accept: 'application/xhtml+xml',
>              on: {
>                  success: success_handler,
> @@ -119,7 +134,7 @@
>          patch_payload[attribute_name] = null;
>
>          var client = new LP.client.Launchpad();
> -        client.patch(resource_uri, patch_payload, {
> +        client.patch(picker._resource_uri, patch_payload, {
>              on: {
>                  success: success_handler,
>                  failure: failure_handler
> @@ -128,6 +143,7 @@
>      };
>
>      var picker = Y.lp.picker.create(vocabulary, save, config);
> +    picker._resource_uri = resource_uri;
>      var extra_buttons = Y.Node.create(
>          '<div style="text-align: center; height: 3em; ' +
>          'white-space: nowrap"/>');
> @@ -158,6 +174,8 @@
>      activator.render();
>
>      show_hide_buttons();
> +
> +    return picker;
>  };
>
>  /**
> @@ -297,5 +315,5 @@
>  }, '0.1', {
>  requires: [
>      'io', 'dom', 'dump', 'lazr.picker', 'lazr.activator', 'json-parse',
> -    'lp.client.helpers'
> +    'datatype-xml', 'lp.client.helpers'

Is this in newer versions of YUI? I don't think we have this module,
do we?

>      ]});
>
> === modified file 'lib/lp/bugs/browser/bugtask.py'
> --- lib/lp/bugs/browser/bugtask.py	2009-08-25 12:35:23 +0000
> +++ lib/lp/bugs/browser/bugtask.py	2009-08-26 11:02:06 +0000
> @@ -10,6 +10,7 @@
>      'BugListingBatchNavigator',
>      'BugListingPortletView',
>      'BugNominationsView',
> +    'bugtarget_renderer',
>      'BugTargetTraversalMixin',
>      'BugTargetView',
>      'BugTaskContextMenu',
> @@ -76,7 +77,7 @@
>  from lazr.lifecycle.snapshot import Snapshot
>  from lazr.restful.interface import copy_field
>  from lazr.restful.interfaces import (
> -    IFieldHTMLRenderer, IReferenceChoice, IWebServiceClientRequest)
> +    IFieldHTMLRenderer, IReference, IReferenceChoice, IWebServiceClientRequest)

Line too long.

There's also some lint in bugtask-tasks-and-nominations-table-row.pt,
and another long line in this file.

>
>  from canonical.config import config
>  from canonical.database.sqlbase import cursor
> @@ -137,7 +138,8 @@
>  from canonical.launchpad.webapp.authorization import check_permission
>  from canonical.launchpad.webapp.batching import TableBatchNavigator
>  from canonical.launchpad.webapp.menu import structured
> -from canonical.launchpad.webapp.tales import PersonFormatterAPI, FormattersAPI
> +from canonical.launchpad.webapp.tales import (
> +    FormattersAPI, ObjectImageDisplayAPI, PersonFormatterAPI)
>
>  from canonical.lazr.interfaces import IObjectPrivacy
>  from lazr.restful.interfaces import IJSONRequestCache
> @@ -161,7 +163,24 @@
>  def assignee_renderer(context, field, request):
>      """Render a bugtask assignee as a link."""
>      def render(value):
> -        return PersonFormatterAPI(context.assignee).link('+assignedbugs')
> +        if context.assignee is None:
> +            return ''
> +        else:
> +            return PersonFormatterAPI(context.assignee).link(None)
> +    return render
> +
> +@component.adapter(IBugTask, IReference, IWebServiceClientRequest)
> +@implementer(IFieldHTMLRenderer)

Nice, I think this will make it easier for future developers to figure
out what's going on.

> +def bugtarget_renderer(context, field, request):
> +    """Render a bugtarget as a link."""
> +    def render(value):
> +        html = """<span>
> +          <a href="%(href)s" class="%(class)s">%(displayname)s</a>
> +        </span>""" % {
> +            'href': canonical_url(context.target),
> +            'class': ObjectImageDisplayAPI(context.target).sprite_css(),
> +            'displayname': context.bugtargetdisplayname}

This is vulnerable to script injection. All of the replacement vars
should be escaped - with cgi.escape(...), and cgi.escape(..., True)
for attributes - but especially the displayname.

We really need a way of producing quick/small HTML snippets safely in
Python and in Javascript.

> +        return html
>      return render
>
>  def unique_title(title):
> @@ -3151,6 +3170,7 @@
>              'bugtask_path': '/'.join(
>                  [''] + canonical_url(self.context).split('/')[3:]),
>              'prefix': get_prefix(self.context),
> +            'target_is_product': IProduct.providedBy(self.context.target),
>              'status_widget_items': self.status_widget_items,
>              'status_value': self.context.status.title,
>              'importance_widget_items': self.importance_widget_items,
>
> === modified file 'lib/lp/bugs/configure.zcml'
> --- lib/lp/bugs/configure.zcml	2009-08-24 20:28:33 +0000
> +++ lib/lp/bugs/configure.zcml	2009-08-26 11:02:06 +0000
> @@ -270,11 +270,13 @@
>              factory="lp.bugs.browser.bugtask.assignee_renderer"
>              name="assignee"/>
>          <adapter
> +            factory="lp.bugs.browser.bugtask.bugtarget_renderer"
> +            name="target"/>
> +        <adapter
>              provides="canonical.launchpad.webapp.interfaces.IBreadcrumb"
>              for="lp.bugs.interfaces.bugtask.IBugTask"
>              factory="lp.bugs.browser.bugtask.BugTaskBreadcrumb"
>              permission="zope.Public"/>
> -
>
>          <!-- NullBugTask -->
>
>
> === modified file 'lib/lp/bugs/interfaces/bugtask.py'
> --- lib/lp/bugs/interfaces/bugtask.py	2009-08-20 01:11:52 +0000
> +++ lib/lp/bugs/interfaces/bugtask.py	2009-08-26 11:02:06 +0000
> @@ -634,6 +634,7 @@
>          value is set to None, date_assigned is also set to None.
>          """
>
> +    @mutator_for(target)
>      @operation_parameters(
>          target=copy_field(target))
>      @export_write_operation()

Does there need to be a webapi test for this?

>
> === modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
> --- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2009-08-21 16:23:58 +0000
> +++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2009-08-26 11:02:06 +0000
> @@ -36,12 +36,20 @@
>        </tal:not-conjoined-task>
>      </td>
>      <td tal:condition="not:indent_task">
> -      <a tal:attributes="
> -            href context/target/fmt:url;
> -            title view/target_link_title;
> -            class context/target/image:sprite_css"
> -        tal:content="context/bugtargetdisplayname"
> -      />
> +      <span tal:attributes="id string:bugtarget-picker-${row_id}">
> +        <span class="yui-activator-data-box">
> +          <span title="This project&rsquo;s license has not been specified.">
> +            <a tal:attributes="href context/target/fmt:url;
> +                                    title view/target_link_title;
> +                                    class context/target/image:sprite_css"

Indentation.

> +               tal:content="context/bugtargetdisplayname" />
> +          </span>
> +        </span>
> +        <button class="lazr-btn yui-activator-act yui-activator-hidden">
> +          Edit
> +        </button>
> +        <div class="yui-activator-message-box yui-activator-hidden" />
> +      </span>
>      </td>
>
>      <tal:conjoined-task condition="is_conjoined_slave">
>