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.
> === 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?
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.
> + 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?
> + }
> 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(
> + '