Hi Edwin, I've put a lot of comments in the diff below, but none of them are critical, and a lot of them are suggestions. This is a nice new feature that works well :) review approve code merge approve Gavin. > === modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js' > --- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-12-14 15:49:13 +0000 > +++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-12-16 15:51:58 +0000 > @@ -239,21 +239,6 @@ > > > /* > - * Clear the subscribe someone else picker. > - * > - * @method clear_picker > - * @param e {Object} The event object. > - */ > -function clear_picker(e) { > - var input = Y.one('.yui-picker-search-box input'); > - input.set('value', ''); > - this.set('error', ''); > - this.set('results', [{}]); > - this._results_box.set('innerHTML', ''); > - this.set('batches', []); > -} > - > -/* > * Initialize click handler for the subscribe someone else link. > * > * @method setup_subscribe_someone_else_handler > @@ -262,23 +247,14 @@ > function setup_subscribe_someone_else_handler(subscription) { > var config = { > header: 'Subscribe someone else', > - step_title: 'Search' > + step_title: 'Search', > + picker_activator: '.menu-link-addsubscriber' > }; > > var picker = Y.lp.picker.create( > 'ValidPersonOrTeam', > function(result) { subscribe_someone_else(result, subscription); }, > config); > - // Clear results and search terms on cancel or save. > - picker.on('save', clear_picker, picker); > - picker.on('cancel', clear_picker, picker); > - > - var subscription_link_someone_else = Y.one('.menu-link-addsubscriber'); > - subscription_link_someone_else.on('click', function(e) { > - e.halt(); > - picker.show(); > - }); > - subscription_link_someone_else.addClass('js-action'); > } > > /* > @@ -626,21 +602,12 @@ > if (Y.Lang.isValue(link_branch_link)) { > var config = { > header: 'Link a related branch', > - step_title: 'Search' > + step_title: 'Search', > + picker_activator: '.menu-link-addbranch' > }; > > var picker = Y.lp.picker.create( > 'Branch', get_branch_and_link_to_bug, config); > - > - // Clear results and search terms on cancel or save. > - picker.on('save', clear_picker, picker); > - picker.on('cancel', clear_picker, picker); > - > - link_branch_link.on('click', function(e) { > - e.halt(); > - picker.show(); > - }); > - link_branch_link.addClass('js-action'); > } > } Thanks for fixing this up. > > > === modified file 'lib/canonical/launchpad/javascript/code/codereview.js' > --- lib/canonical/launchpad/javascript/code/codereview.js 2009-11-30 23:51:34 +0000 > +++ lib/canonical/launchpad/javascript/code/codereview.js 2009-12-16 15:51:58 +0000 > @@ -22,6 +22,14 @@ > var link = Y.one('#request-review'); > if (link !== null) { > link.addClass('js-action'); > + /* XXX: salgado, 2009-11-11: This will cause the picker to be > + * recreated every time the user clicks on the link. Although that > + * makes it unnecessary to have the widget cleared, it makes it > + * impossible to persist the state of the picker between clicks on the > + * link. We should probably have a policy to enforce that we > + * just hide/show widgets when a link is clicked more than once, > + * instead of recreating the widgets every time. > + */ Can you consider filing a bug for this and updating the XXX? > link.on('click', show_request_review_form); > } > link = Y.one('.menu-link-set_commit_message'); > @@ -51,7 +59,7 @@ > */ > function commit_message_listener(message, saved) > { > - if (message == '') { > + if (message === '') { > // Hide the multiline editor > Y.one('#edit-commit-message').addClass('unseen'); > // Show the link again > > === modified file 'lib/canonical/launchpad/javascript/lp/picker.js' > --- lib/canonical/launchpad/javascript/lp/picker.js 2009-12-01 15:11:51 +0000 > +++ lib/canonical/launchpad/javascript/lp/picker.js 2009-12-16 15:51:58 +0000 > @@ -15,6 +15,8 @@ > * @param {String} resource_uri The object being modified. > * @param {String} attribute_name The attribute on the resource being > * modified. > + * @param {Bool} show_remove_button Should the remove button be shown? > + * @param {Bool} show_assign_me_button Should the 'assign me' button be shown? > * @param {String} content_box_id > * @param {Object} config Object literal of config name/value pairs. > * config.header is a line of text at the top of > @@ -219,10 +221,10 @@ > "string: " + vocabulary); > } > > - var picker = new Y.Picker({ > + var new_config = Y.merge(config, { > align: { > points: [Y.WidgetPositionExt.CC, > - Y.WidgetPositionExt.CC] > + Y.WidgetPositionExt.CC] > }, > progressbar: true, > progress: 100, > @@ -231,6 +233,7 @@ > zIndex: 1000, > visible: false > }); > + var picker = new Y.Picker(new_config); > > picker.subscribe('save', function (e) { > Y.log('Got save event.'); > > === added file 'lib/canonical/launchpad/javascript/registry/team.js' > --- lib/canonical/launchpad/javascript/registry/team.js 1970-01-01 00:00:00 +0000 > +++ lib/canonical/launchpad/javascript/registry/team.js 2009-12-16 15:51:58 +0000 > @@ -0,0 +1,102 @@ > +/** Copyright (c) 2009, Canonical Ltd. All rights reserved. > + * > + * Objects for subscription handling. > + * > + * @module lp.subscriber > + */ > + > +YUI.add('registry.team', function(Y) { > + > +var module = Y.namespace('registry.team'); > + > +/* > + * Initialize click handler for the add member link > + * > + * @method setup_add_member_handler > + */ > +module.setup_add_member_handler = function() { > + var config = { > + header: 'Add a member', > + step_title: 'Search', > + picker_activator: '.menu-link-add_member' > + }; > + > + var picker = Y.lp.picker.create( > + 'ValidTeamMember', > + function(result) { _add_member(result); }, Could this be simplified to just `_add_member`? > + config); > +}; > + > +var _add_member = function(result) { > + var spinner = Y.one('#add-member-spinner'); > + var addmember_link = Y.one('.menu-link-add_member'); These both query the page globally, which isn't out of the norm in the LP JavaScript code, but it is like using global variables. Can you do these queries from within an area of the DOM that is specific to this widget? I'm not going to cry if you can't. > + addmember_link.setStyle('display', 'none'); > + spinner.setStyle('display', 'inline'); Can you use the unseen CSS class instead of setting display? > + function disable_spinner() { > + addmember_link.setStyle('display', 'inline'); > + spinner.setStyle('display', 'none'); > + } > + lp_client = new LP.client.Launchpad(); > + > + var error_handler = new LP.client.ErrorHandler(); > + error_handler.clearProgressUI = disable_spinner; > + error_handler.showError = function(error_msg) { > + Y.lp.display_error(Y.one('.menu-link-add_member'), error_msg); > + }; > + > + config = { > + on: { > + success: function(member_added) { > + if (!member_added) { > + disable_spinner(); > + alert('Already a member.'); Is this alert() a left-over from development? Same below. > + return; > + } > + if (result.css.match("team")) { This is nasty :) If there's not prettier way to do it, please can you add a comment explaining why? Also, consider renaming the result argument to chosen_member or something like that. With lots of callbacks it gets confusing. > + disable_spinner(); > + alert('This is a team'); > + return; > + } > + var members_section = Y.one('#recently-approved'); > + var members_ul = Y.one('#recently-approved-ul'); > + var first_node = members_ul.get('firstChild'); > + config = { > + on: { > + success: function(person_html) { > + var total_members = Y.one( > + '#member-count').get('innerHTML'); > + total_members = parseInt(total_members, 10) + 1; > + Y.one('#member-count').set( > + 'innerHTML', total_members); The code above is a bit hackish, but not worth spending too long trying to make cleaner. Is it possible to stuff the updated member count into the member_added result? Mmm, that probably needs something that was being proposed on the list a while back: ask the API to do something and return several results. But, once something like that is available, perhaps there's an argument for adding a guideline against parsing information back out of the DOM. > + person_repr = Y.Node.create( > + '
  • ' + person_html + '
    • '); > + members_section.setStyle('visibility', 'visible'); Again, not a big issue, but could you use a CSS class for this? I just vaguely remember that it's The Preferred Way; I can't justify why it's better off the top of my head. > + members_ul.insertBefore( > + person_repr, first_node); > + anim = Y.lazr.anim.green_flash( > + {node: person_repr}); > + anim.run(); > + disable_spinner(); > + }, > + failure: error_handler.getFailureHandler() > + }, > + accept: LP.client.XHTML > + }; > + lp_client.get(result.api_uri, config); > + }, > + failure: error_handler.getFailureHandler() > + }, > + parameters: { > + // XXX: Why do I always have to get absolute URIs out of the URIs > + // in the picker's result/client.links? Can you add a bug number here, or perhaps demote it to just a regular comment? > + reviewer: LP.client.get_absolute_uri(LP.client.links.me), > + person: LP.client.get_absolute_uri(result.api_uri) > + } > + }; > + > + lp_client.named_post( > + LP.client.cache.context.self_link, 'addMember', config); > +}; > + > +}, '0.1', {requires: [ > + 'node', 'lazr.anim', 'lp.picker', 'lp.errors', 'lp.client.plugins']}); > > === modified file 'lib/lp/app/templates/base-layout-macros.pt' > --- lib/lp/app/templates/base-layout-macros.pt 2009-12-08 16:43:44 +0000 > +++ lib/lp/app/templates/base-layout-macros.pt 2009-12-16 15:51:58 +0000 > @@ -181,6 +181,8 @@ > tal:attributes="src string:${lp_js}/lp/comment.js"> > > + > > > > > === modified file 'lib/lp/bugs/tests/test_bugs_webservice.py' > --- lib/lp/bugs/tests/test_bugs_webservice.py 2009-12-01 12:21:56 +0000 > +++ lib/lp/bugs/tests/test_bugs_webservice.py 2009-12-16 15:51:58 +0000 > @@ -120,16 +120,6 @@ > self.assertEqual(response.status, 200) > > rendered_comment = response.body > - # XXX Bjorn Tillenius 2009-05-15 bug=377003 > - # The current request is a web service request when rendering > - # the HTML, causing canonical_url to produce links pointing to the > - # web service. Adjust the test to compensate for this, and accept > - # that the links will be incorrect for now. We should fix this > - # before using it for anything useful. > - rendered_comment = rendered_comment.replace( > - 'http://api.launchpad.dev/beta/', > - 'http://launchpad.dev/') > - Hurrah! > self.assertRenderedCommentsEqual( > rendered_comment, self.expected_comment_html) > > > === modified file 'lib/lp/registry/browser/configure.zcml' > --- lib/lp/registry/browser/configure.zcml 2009-12-05 18:37:28 +0000 > +++ lib/lp/registry/browser/configure.zcml 2009-12-16 15:51:58 +0000 > @@ -751,6 +751,9 @@ > for="lp.registry.interfaces.person.IPerson" > layer="canonical.launchpad.layers.BugsLayer" > name="+bugs"/> > + + factory="lp.registry.browser.person.PersonXHTMLRepresentation" > + name="lazr.restful.EntryResource" /> > name="+review" > for="lp.registry.interfaces.person.IPerson" > > === modified file 'lib/lp/registry/browser/person.py' > --- lib/lp/registry/browser/person.py 2009-12-08 10:20:37 +0000 > +++ lib/lp/registry/browser/person.py 2009-12-16 15:51:58 +0000 > @@ -102,7 +102,7 @@ > from zope.interface import classImplements, implements, Interface > from zope.interface.exceptions import Invalid > from zope.interface.interface import invariant > -from zope.component import getUtility > +from zope.component import adapts, getUtility > from zope.publisher.interfaces import NotFound > from zope.publisher.interfaces.browser import IBrowserPublisher > from zope.schema import Bool, Choice, List, Text, TextLine > @@ -117,6 +117,7 @@ > from lazr.delegates import delegates > from lazr.config import as_timedelta > from lazr.restful.interface import copy_field, use_template > +from lazr.restful.interfaces import IWebServiceClientRequest > from canonical.lazr.utils import safe_hasattr > from canonical.database.sqlbase import flush_database_updates > > @@ -226,7 +227,8 @@ > logoutPerson, allowUnauthenticatedSession) > from canonical.launchpad.webapp.menu import get_current_view > from canonical.launchpad.webapp.publisher import LaunchpadView > -from canonical.launchpad.webapp.tales import DateTimeFormatterAPI > +from canonical.launchpad.webapp.tales import ( > + DateTimeFormatterAPI, PersonFormatterAPI) > from lazr.uri import URI, InvalidURIError > > from canonical.launchpad import _ > @@ -2460,7 +2462,7 @@ > > @property > def next_url(self): > - """Redirect back to the +languages page if request originated there.""" > + """Redirect back to the originating page.""" > redirection_url = self.request.get('redirection_url') > if redirection_url: > return redirection_url > @@ -2468,7 +2470,7 @@ > > @property > def cancel_url(self): > - """Redirect back to the +languages page if request originated there.""" > + """Redirect back to the originating page.""" > redirection_url = self.getRedirectionURL() > if redirection_url: > return redirection_url > @@ -2676,12 +2678,29 @@ > orderBy='-TeamMembership.date_proposed') > return members[:5] > > - @cachedproperty > - def has_recent_approved_or_proposed_members(self): > - """Does the team have recently approved or proposed members?""" > - approved = self.recently_approved_members.count() > 0 > - proposed = self.recently_proposed_members.count() > 0 > - return approved or proposed > + @property > + def recently_approved_hidden(self): > + """Optionally hide the div. > + > + The AJAX on the page needs the elements to be present > + but hidden in case it adds a member to the list. > + """ > + if self.recently_approved_members.count() == 0: I think .is_empty() is more efficient than .count() here. > + return 'visibility: collapse' > + else: > + return '' > + > + @property > + def recently_proposed_hidden(self): > + """Optionally hide the div. > + > + The AJAX on the page needs the elements to be present > + but hidden in case it adds a member to the list. > + """ > + if self.recently_proposed_members.count() == 0: Same with .is_empty(). If recently_proposed_members is a SQLObjectResultSet, you can adapt it to a storm.zope.IResultSet first. > + return 'visibility: collapse' > + else: > + return '' > > @cachedproperty > def openpolls(self): > @@ -5826,8 +5845,7 @@ > usedfor = ITeamIndexMenu > facet = 'overview' > title = 'Change team' > - links = ('edit', 'join', 'add_member', 'add_my_teams', > - 'leave') > + links = ('edit', 'join', 'add_my_teams', 'leave') > > > class TeamEditMenu(TeamNavigationMenuBase): > @@ -5843,3 +5861,16 @@ > classImplements(TeamIndexView, ITeamIndexMenu) > classImplements(TeamEditView, ITeamEditMenu) > classImplements(PersonIndexView, IPersonIndexMenu) > + > + > +class PersonXHTMLRepresentation: > + adapts(IPerson, IWebServiceClientRequest) > + implements(Interface) > + > + def __init__(self, person, request): > + self.person = person > + self.request = request > + > + def __call__(self): > + """Render `Person` as XHTML using the webservice.""" > + return PersonFormatterAPI(self.person).link(None) > > === modified file 'lib/lp/registry/browser/tests/teammembership-views.txt' > --- lib/lp/registry/browser/tests/teammembership-views.txt 2009-11-13 13:06:50 +0000 > +++ lib/lp/registry/browser/tests/teammembership-views.txt 2009-12-16 15:51:58 +0000 > @@ -63,6 +63,7 @@ > > >>> login_person(team_owner) > >>> super_team.addMember(team, team_owner) > + True > >>> membership = membership_set.getByPersonAndTeam(team, super_team) > >>> login_person(team.teamowner) > >>> view = TeamInvitationView(membership, request) > > === added file 'lib/lp/registry/browser/tests/test_person_webservice.py' > --- lib/lp/registry/browser/tests/test_person_webservice.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/registry/browser/tests/test_person_webservice.py 2009-12-16 15:51:58 +0000 > @@ -0,0 +1,38 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +__metaclass__ = type > + > +import unittest > + > +from canonical.launchpad.ftests import login > +from lp.testing import TestCaseWithFactory > +from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller > +from canonical.testing import DatabaseFunctionalLayer > + > + > +class TestPersonRepresentation(TestCaseWithFactory): > + layer = DatabaseFunctionalLayer > + > + def setUp(self): > + TestCaseWithFactory.setUp(self) > + login('