Merge lp:~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2 into lp:launchpad

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.

Implementation details
----------------------

lib/canonical/launchpad/icing/style-3-0.css
lib/canonical/launchpad/icing/style.css
    Moved the unseen style to style-3-0.css since style.css
    will be going away eventually.

lib/canonical/launchpad/javascript/code/codereview.js
    Added bug number to XXX.

lib/canonical/launchpad/javascript/registry/team.js
    Added handling of teams as new members.

lib/lp/registry/browser/person.py
    Added code for a list of recently invited members in
    the membership portlet.
    Using a css class to hide the lists instead of setting
    the element's style attribute.
    Modified the view for +add-my-teams so that it matches
    the behavior of the ajax "Add member" link.

lib/lp/registry/browser/tests/mailinglist-views.txt
    Fixed test for new return values.

lib/lp/registry/interfaces/person.py
    Documented new return value and organized docs by param.

lib/lp/registry/model/person.py
    Changed return value.

lib/lp/registry/templates/team-index.pt
    Fixed mixing space between left and right portlets.

lib/lp/registry/templates/team-members.pt
    Hide former members list from non-admins, since some users
    complain when it appears that they used to be a member of a team
    that they were added to on accident.

lib/lp/registry/templates/team-portlet-membership.pt
    Added recently invited member list.

lib/lp/registry/windmill/tests/test_team_index.py
    Added windmill test.

Tests
-----

./bin/test -vv -t mailinglist-views.txt
./bin/test -vv --layer=RegistryWindmillTest -t test_team_index

Demo and Q/A
------------

* Open http://launchpad.dev/~testing-spanish-team
  * Log in as <email address hidden>.
  * Click on the green "Add member" link in the portlet.
  * Search for "rosetta" and click on the rosetta admins team.
  * That team should be added to the "Recently approved" list
    since carlos admins both of them.
* Click on the green "Add member" link in the portlet.
  * Search for "simple" and click on the Simple Team.
  * That team should be added to the "Recently invited" list
    since carlos does not admin the Simple Team.
* Log out.
  * The "Add member" link should be gone.

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.displ...

Here are screenshots of this feature that show how adding a person or a team as a member updates the appropriate list and updates the counts at the top of the portlet. If the user adds a team that he does not admin, then the team is invited. If the user does admin the team being added, it skips the invitation status and goes straight to approved.

https://chinstrap.canonical.com/~egrubbs/addmember/

Hi Edwin,

The branch in general looks great, you solved the bulk of the challenges already.
As per our conversation on IRC, the actual fix for the "user is already approved" message is not getting in the situation in the first place, so filing a bug about is a good start.

If we're going to keep the dialog around for a while, I would do one of two things:

1) Add buttons to dismiss the dialog or go back to the results (OK and BACK?)
2) Show the message instead of the results (ie, keep the search fields visible to re-search)

I would also re-phrase the message to something like "$user is already part of the team"

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');
> > + };
> > ...

Here is the incremental diff that addresses both Salgado's and Martin's comments.

{{{
=== modified file 'lib/canonical/launchpad/javascript/lp/errors.js'
--- lib/canonical/launchpad/javascript/lp/errors.js 2009-11-23 19:29:02 +0000
+++ lib/canonical/launchpad/javascript/lp/errors.js 2009-12-21 20:33:38 +0000
@@ -69,7 +69,7 @@
  * @param flash_node {Node} The node to red flash.
  * @param msg {String} The message to display.
 */
-var display_error = function(flash_node, msg) {
+Y.lp.display_error = function(flash_node, msg) {
     create_error_overlay();
     maybe_red_flash(flash_node, function(){
         error_overlay.showError(msg);
@@ -77,5 +77,36 @@
     });
 };

-Y.lp.display_error = display_error;
-}, "0.1", {"requires":["lazr.formoverlay"]});
+
+var info_overlay;
+/*
+ * Display the form overlay for non-error informational messages.
+ *
+ * @method display_info
+ * @param msg {String} The message to display.
+*/
+Y.lp.display_info = function(msg) {
+ if (info_overlay === undefined) {
+ info_overlay = new Y.lazr.PrettyOverlay({
+ //headerContent: '<h2><img src="/@@/info-large"/>Notice</h2>',
+ centered: true,
+ visible: false
+ });
+ info_overlay.render();
+ }
+ var content = Y.Node.create(
+ '<div style="background: url(/@@/info-large) no-repeat; ' +
+ 'min-height: 32px; padding-left: 40px; padding-top: 16px"/></div>');
+ content.appendChild(Y.Node.create(msg));
+ var button_div = Y.Node.create('<div style="text-align: right"></div>');
+ var ok_button = Y.Node.create('<button>OK</button>');
+ ok_button.on('click', function(e) {
+ info_overlay.fire('cancel');
+ });
+ button_div.appendChild(ok_button);
+ content.appendChild(button_div);
+ info_overlay.set('bodyContent', content);
+ info_overlay.show();
+};
+
+}, "0.1", {"requires":["lazr.formoverlay", "lazr.overlay"]});

=== modified file 'lib/canonical/launchpad/javascript/registry/team.js'
--- lib/canonical/launchpad/javascript/registry/team.js 2009-12-18 06:06:30 +0000
+++ lib/canonical/launchpad/javascript/registry/team.js 2009-12-21 20:35:57 +0000
@@ -54,13 +54,14 @@
                 var members_section, members_ul, count_elem;
                 if (did_status_change === false) {
                     disable_spinner();
- Y.lp.display_error(
- addmember_link,
+ Y.lp.display_info(
                         selected_person.title + ' is already ' +
- current_status + '.');
+ current_status.toLowerCase() +
+ ' as a member of the team.');
                     return;
                 }
                 var is_team = false;
+ current_status = 'foo';
                 if (current_status == 'Invited') {
                     is_team = true;
                     members_section = box.query('#recently-invited');
@@ -79,6 +80,7 @@
                     Y.lp.display_error(
                         addmember_link,
                         'Unexpected status: ' + current_status);
+ return;
                 }
                 var firs...

Hi Martin,

Thanks for the review. For some reason, your comment didn't get added to the merge proposal, so I pasted it here again.

On Mon, Dec 21, 2009 at 1:35 PM, Martin Albisetti <email address hidden> wrote:
> Review: Needs Fixing ui
> Hi Edwin,
>
> The branch in general looks great, you solved the bulk of the challenges already.
> As per our conversation on IRC, the actual fix for the "user is already approved" message is not getting in the situation in the first place, so filing a bug about is a good start.

Opened bug 499210 for this.

> If we're going to keep the dialog around for a while, I would do one of two things:
>
> 1) Add buttons to dismiss the dialog or go back to the results (OK and BACK?)
> 2) Show the message instead of the results (ie, keep the search fields visible to re-search)

I added an "OK" button to dismiss the dialog.

> I would also re-phrase the message to something like "$user is already part of the team"

There actually are two potential errors, since a team could already be in an invited status, so it would be unclear to say that they are already part of the team. Here are screenshots of both error messages.

https://chinstrap.canonical.com/~egrubbs/addmember/new_approved_error_message.png
https://chinstrap.canonical.com/~egrubbs/addmember/new_invited_error_message.png

Hi Edwin.

This is a nice addition. I have no remarks about new test. I have a suggestion about the team story

> === modified file 'lib/lp/registry/stories/team/xx-team-home.txt'
> --- lib/lp/registry/stories/team/xx-team-home.txt 2009-11-22 15:43:16 +0000
> +++ lib/lp/registry/stories/team/xx-team-home.txt 2009-12-21 19:53:58 +0000

...

> @@ -88,14 +93,14 @@
> Show received invitations
>
> If the team does not have any recently approved or proposed members,
> -the recent members sections are not displayed:
> +the recent members sections are hidden using the "unseen" css class:
>
> >>> browser.open('http://launchpad.dev/~launchpad')
> >>> print find_tag_by_id(browser.contents, 'recently-approved')
> - None
> + <td id="recently-approved" ... class="unseen">...
>
> - >>> print find_tag_by_id(browser.contents, 'recently-applied')
> - None
> + >>> print find_tag_by_id(browser.contents, 'recently-proposed')
> + <td id="recently-proposed" class="unseen">...

These tests will break if we reimplement this as a list. We should not print
markup in user stories because they do no read the source. The test is parsing
the tree many times. I think this can be expressed like this so that it runs
faster and is robust.

    >>> browser.open('http://launchpad.dev/~launchpad')
    >>> content = find_main_content(browser.contents)
    >>> tag = find_tag_by_id(content, 'recently-approved')
    >>> print tag['class']
    unseen

    >>> tag = find_tag_by_id(content, 'recently-proposed')
    >>> print tag['class']
    unseen

.

Thanks Edwin. The dialog now looks better, minus an alignment issue between the text and the icon. If you could top-align the text with the icon, it would be super great.

The only remaining issue for me is the fact that the persons' name doesn't appear on the page grayed out while it's being saved, like everywhere else.

Could you factor in that behavior?

> Thanks Edwin. The dialog now looks better, minus an alignment issue between
> the text and the icon. If you could top-align the text with the icon, it would
> be super great.
>
> The only remaining issue for me is the fact that the persons' name doesn't
> appear on the page grayed out while it's being saved, like everywhere else.
>
> Could you factor in that behavior?

Martin, I've run into a snag adding the grayed out team/person with a spinner. A team may be added to the "Latest members" or the "Latest invited" list depending on whether the user is also an admin on the team being added as a member. Also, the "Latest members" and "Latest invited" labels may need to be unhidden and presumably re-hidden if they are empty at the end of the process. It seems like a bad idea to make another REST request to gather the information just to put the spinner in the right spot.

On Mon, 2009-12-21 at 20:54 +0000, Edwin Grubbs wrote:
> Here is the incremental diff that addresses both Salgado's and
> Martin's comments.

Looks good to me, but there's some code that was apparently written to
test corner cases and left behind accidentally. See below

>
> {{{
> === modified file 'lib/canonical/launchpad/javascript/lp/errors.js'
> --- lib/canonical/launchpad/javascript/lp/errors.js 2009-11-23 19:29:02 +0000
> +++ lib/canonical/launchpad/javascript/lp/errors.js 2009-12-21 20:33:38 +0000
> @@ -69,7 +69,7 @@
> * @param flash_node {Node} The node to red flash.
> * @param msg {String} The message to display.
> */
> -var display_error = function(flash_node, msg) {
> +Y.lp.display_error = function(flash_node, msg) {
> create_error_overlay();
> maybe_red_flash(flash_node, function(){
> error_overlay.showError(msg);
> @@ -77,5 +77,36 @@
> });
> };
>
> -Y.lp.display_error = display_error;
> -}, "0.1", {"requires":["lazr.formoverlay"]});
> +
> +var info_overlay;
> +/*
> + * Display the form overlay for non-error informational messages.
> + *
> + * @method display_info
> + * @param msg {String} The message to display.
> +*/
> +Y.lp.display_info = function(msg) {
> + if (info_overlay === undefined) {
> + info_overlay = new Y.lazr.PrettyOverlay({
> + //headerContent: '<h2><img src="/@@/info-large"/>Notice</h2>',

Why is this commented out?

> + centered: true,
> + visible: false
> + });
> + info_overlay.render();
> + }
> + var content = Y.Node.create(
> + '<div style="background: url(/@@/info-large) no-repeat; ' +
> + 'min-height: 32px; padding-left: 40px; padding-top: 16px"/></div>');
> + content.appendChild(Y.Node.create(msg));
> + var button_div = Y.Node.create('<div style="text-align: right"></div>');
> + var ok_button = Y.Node.create('<button>OK</button>');
> + ok_button.on('click', function(e) {
> + info_overlay.fire('cancel');
> + });
> + button_div.appendChild(ok_button);
> + content.appendChild(button_div);
> + info_overlay.set('bodyContent', content);
> + info_overlay.show();
> +};
> +
> +}, "0.1", {"requires":["lazr.formoverlay", "lazr.overlay"]});
>
> === modified file 'lib/canonical/launchpad/javascript/registry/team.js'
> --- lib/canonical/launchpad/javascript/registry/team.js 2009-12-18 06:06:30 +0000
> +++ lib/canonical/launchpad/javascript/registry/team.js 2009-12-21 20:35:57 +0000
> @@ -54,13 +54,14 @@
> var members_section, members_ul, count_elem;
> if (did_status_change === false) {
> disable_spinner();
> - Y.lp.display_error(
> - addmember_link,
> + Y.lp.display_info(
> selected_person.title + ' is already ' +
> - current_status + '.');
> + current_status.toLowerCase() +
> + ' as a member of the team.');
> return;
> }
> var is_team = false;
> + current_status = 'foo';

!?

I really hope your windmill te...

This is a pattern we will need to solve many times, so lets land this now, and start a discussion on how to fix it in a more general manner.

Thanks Edwin, this seems good to land for me.

I removed the commented out headerContent, which was unnecessary. I also removed the debugging code that set current_status to 'foo'.