Hi Salgado, Thanks for the review. Comments below. > Hi Edwin, > > This is a nice branch but I think we're missing some test coverage for > some changes you introduced. > > review needsfixing > > On Fri, 2009-12-18 at 06:36 +0000, Edwin Grubbs wrote: > > Summary > > ------- > > > > This branch is dependent on > > lp:~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part1 > > > > I made the following changes: > > * Added a windmill test > > * Changed the return value from IPerson.addMember() so > > the UI knows what status it decided to use. > > * If the user is allowed to invite a member because they can admin > > the owning team and if the user is allowed to accept the invitation > > because they can admin the member team, addMember() now just fully > > approves the membership. > > > > Over 50 tests broke because I changed the return value for > > addMember() and setStatus(). I only fixed one of them as a sample. > > The rest will have to wait for a followup branch, since I'm interested > > in getting feedback on the changes I made already. > > I think most of these tests are probably not interested in the return > value of addMember(), and in that case I suggest you change these ones > to assign the return value of addMember() to a dummy variable so that > they don't break again when we need to change the return value once > again. I added the dummy variable in the mailinglist-views.txt test, and I will do that for the others. > > === modified file 'lib/canonical/launchpad/javascript/registry/team.js' > > --- lib/canonical/launchpad/javascript/registry/team.js 2009-12-18 06:36:27 > +0000 > > +++ lib/canonical/launchpad/javascript/registry/team.js 2009-12-18 06:36:29 > +0000 > > @@ -15,87 +15,122 @@ > > * @method setup_add_member_handler > > */ > > module.setup_add_member_handler = function() { > > + if (Y.UA.ie) { > > + return; > > + } > > + > > 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); }, > > - config); > > + Y.lp.picker.create('ValidTeamMember', _add_member, config); > > }; > > > > -var _add_member = function(result) { > > - var spinner = Y.one('#add-member-spinner'); > > - var addmember_link = Y.one('.menu-link-add_member'); > > - addmember_link.setStyle('display', 'none'); > > - spinner.setStyle('display', 'inline'); > > - function disable_spinner() { > > - addmember_link.setStyle('display', 'inline'); > > - spinner.setStyle('display', 'none'); > > - } > > +var _add_member = function(selected_person) { > > + var box = Y.one('#membership'); > > + var spinner = box.query('#add-member-spinner'); > > + var addmember_link = box.query('.menu-link-add_member'); > > + addmember_link.addClass('unseen'); > > + spinner.removeClass('unseen'); > > + var disable_spinner = function() { > > + addmember_link.removeClass('unseen'); > > + spinner.addClass('unseen'); > > + }; > > 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); > > + Y.lp.display_error(addmember_link, error_msg); > > }; > > > > - config = { > > + addmember_config = { > > on: { > > - success: function(member_added) { > > - if (!member_added) { > > - disable_spinner(); > > - alert('Already a member.'); > > - return; > > - } > > - if (result.css.match("team")) { > > - disable_spinner(); > > - alert('This is a team'); > > - return; > > - } > > - var members_section = Y.one('#recently-approved'); > > - var members_ul = Y.one('#recently-approved-ul'); > > + success: function(change_and_status) { > > + var did_status_change = change_and_status[0]; > > + var current_status = change_and_status[1]; > > + var members_section, members_ul, count_elem; > > + if (did_status_change === false) { > > + disable_spinner(); > > + Y.lp.display_error( > > + addmember_link, > > + selected_person.title + ' is already ' + > > + current_status + '.'); > > + return; > > + } > > + var is_team = false; > > + if (current_status == 'Invited') { > > I thought current_status would be a JSON representation of an > ITeamMembership, which is what's returned by addMember(), but I guess > I'm wrong? Yes, the REST API represents vocabulary values with strings instead of expanding it into full Entry objects. > Also, since it's capitalized, I think it'd make sense to lower case it > in the error message above. And btw, it should be just an informational > notice as the user hasn't done anything wrong. Done. > > + is_team = true; > > + members_section = box.query('#recently-invited'); > > + members_ul = box.query('#recently-invited-ul'); > > + count_elem = box.query('#invited-member-count'); > > + } else if (current_status == 'Proposed') { > > + is_team = true; > > + members_section = box.query('#recently-proposed'); > > + members_ul = box.query('#recently-proposed-ul'); > > + count_elem = box.query('#proposed-member-count'); > > + } else if (current_status == 'Approved') { > > + members_section = box.query('#recently-approved'); > > + members_ul = box.query('#recently-approved-ul'); > > + count_elem = box.query('#approved-member-count'); > > + } else { > > + Y.lp.display_error( > > + addmember_link, > > + 'Unexpected status: ' + current_status); > > Don't you want an early return here? Yes, done. > > + } > > var first_node = members_ul.get('firstChild'); > > - config = { > > + > > + var xhtml_person_handler = function(person_html) { > > + if (count_elem === null && is_team) { > > + count_elem = Y.Node.create( > > + '' + > > + '1'); > > + var count_box = Y.one( > > + '#membership #membership-counts'); > > + count_box.append(Y.Node.create( > > + ', ')); > > + count_box.append(count_elem); > > + count_box.append(Y.Node.create( > > + ' ' + > > + 'invited members')); > > s/members/member, since there's only one? But then if the user adds > another team which ends up invited, we'll have some trouble to change it > from "2 member" into "2 members". Let's leave it as is, I guess. > > > + } else { > > + var count = count_elem.get('innerHTML'); > > + count = parseInt(count, 10) + 1; > > + count_elem.set('innerHTML', count); > > + } > > + person_repr = Y.Node.create( > > + '
  • ' + person_html + '
    • '); > > + members_section.removeClass('unseen'); > > + members_ul.insertBefore(person_repr, first_node); > > + anim = Y.lazr.anim.green_flash({node: person_repr}); > > + anim.run(); > > + disable_spinner(); > > + }; > > + > > + xhtml_person_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); > > - person_repr = Y.Node.create( > > - '
  • ' + person_html + '
    • '); > > - members_section.setStyle('visibility', > 'visible'); > > - members_ul.insertBefore( > > - person_repr, first_node); > > - anim = Y.lazr.anim.green_flash( > > - {node: person_repr}); > > - anim.run(); > > - disable_spinner(); > > - }, > > + success: xhtml_person_handler, > > failure: error_handler.getFailureHandler() > > }, > > accept: LP.client.XHTML > > }; > > - lp_client.get(result.api_uri, config); > > + lp_client.get(selected_person.api_uri, > xhtml_person_config); > > }, > > failure: error_handler.getFailureHandler() > > }, > > parameters: { > > - // XXX: Why do I always have to get absolute URIs out of the > URIs > > + // XXX: EdwinGrubbs 2009-12-16 bug=497602 > > + // Why do I always have to get absolute URIs out of the URIs > > // in the picker's result/client.links? > > reviewer: LP.client.get_absolute_uri(LP.client.links.me), > > - person: LP.client.get_absolute_uri(result.api_uri) > > + person: LP.client.get_absolute_uri(selected_person.api_uri) > > } > > }; > > > > lp_client.named_post( > > - LP.client.cache.context.self_link, 'addMember', config); > > + LP.client.cache.context.self_link, 'addMember', addmember_config); > > }; > > > > }, '0.1', {requires: [ > > > > === modified file 'lib/lp/registry/browser/person.py' > > --- lib/lp/registry/browser/person.py 2009-12-18 06:36:27 +0000 > > +++ lib/lp/registry/browser/person.py 2009-12-18 06:36:29 +0000 > > @@ -95,6 +95,7 @@ > > from itertools import chain > > from operator import attrgetter, itemgetter > > from textwrap import dedent > > +from storm.zope.interfaces import IResultSet > > > > from zope.error.interfaces import IErrorReportingUtility > > from zope.app.form.browser import TextAreaWidget, TextWidget > > @@ -2678,6 +2679,13 @@ > > orderBy='-TeamMembership.date_proposed') > > return members[:5] > > > > + @cachedproperty > > + def recently_invited_members(self): > > + members = self.context.getMembersByStatus( > > + TeamMembershipStatus.INVITED, > > + orderBy='-TeamMembership.date_proposed') > > + return members[:5] > > I think we ought to have a test showing this new section on a team's > home page. It should be trivial to extend an existing one for that... Done. > > + > > @property > > def recently_approved_hidden(self): > > """Optionally hide the div. > > > === modified file 'lib/lp/registry/interfaces/person.py' > > --- lib/lp/registry/interfaces/person.py 2009-12-09 23:11:31 +0000 > > +++ lib/lp/registry/interfaces/person.py 2009-12-18 06:36:29 +0000 > > @@ -1406,23 +1406,33 @@ > [...] > > + > > + :param may_subscribe_to_list: If the person is not a team, and > > + may_subscribe_to_list is True, then the person may be > subscribed > > + to the team's mailing list, depending on the list status and > the > > + person's auto-subscribe settings. > > + > > + :return: A tuple containing a boolean indicating when the > > + membership status changed and the current > `TeamMembershipStatus`. > > + This depends on the desired status passed as an argument, the > > + subscription policy and the user's priveleges. > > typo, privileges > > > """ > > > > def deactivateAllMembers(comment, reviewer): > > > > > > > > === modified file 'lib/lp/registry/templates/team-members.pt' > > --- lib/lp/registry/templates/team-members.pt 2009-10-26 14:23:12 +0000 > > +++ lib/lp/registry/templates/team-members.pt 2009-12-18 06:36:29 +0000 > > @@ -207,57 +207,59 @@ > [...] > > +
    > > + > > +

    Former members

    > > We need a test for this change too. Added test to lp/registry/stories/team/xx-team-membership.txt.