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

[Edwin Grubbs]

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

This is the first branch on the way to adding a picker
to add members to a team with a link on its index page.

Salgado started this branch. I did a little bit of cleanup, but
the following things should be done in a followup branch.

 * Add windmill test.
 * Handle adding teams, which will go into a PROPOSED or INVITED status.

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

The picker widget now clears the search term by default when
the SAVE event is processed (when an item is selected from the results).
The picker widget will also add an onclick handler for you.
    lib/canonical/launchpad/javascript/bugs/bugtask-index.js
    lib/canonical/launchpad/javascript/code/codereview.js

The main part of this feature:
    lib/canonical/launchpad/javascript/registry/team.js
    lib/lp/app/templates/base-layout-macros.pt
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/templates/team-index.pt
    lib/lp/registry/browser/tests/test_person_webservice.py
    lib/lp/registry/templates/team-portlet-membership.pt

Change affecting existing tests:
    lib/lp/registry/model/person.py
    lib/lp/registry/model/teammembership.py
    lib/lp/registry/browser/tests/teammembership-views.txt
    lib/lp/registry/doc/teammembership-email-notification.txt
    lib/lp/registry/doc/teammembership.txt

Tests
-----

./bin/test -vv -t 'test_person_webservice|teammembership-views.txt|teammembership-email-notification.txt|/teammembership.txt'

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

* Open http://launchpad.dev/~guadamen
  * The "Add member" link should be green, click on it to show the picker.
  * When you click on a person in the picker, the progress spinner
    should display where the (+) icon was.
  * Then there should be a green flash where the person is added to the
    "Latest members" list.

[Gavin Panella (allenap)]

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
> + * rec...

review: Approve (code)

=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-12-10 20:59:49 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-12-16 02:39:23 +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');
     }
 }
 
 
=== 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 02:39:23 +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.
+         */
         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 02:39:23 +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 02:39:23 +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); },
+        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');
+    }
+    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.');
+                    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');
+                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);
+                            person_repr = Y.Node.create(
+                                '<li>' + person_html + '</li>');
+                            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();
+                        },
+                        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?
+            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 02:39:23 +0000
@@ -181,6 +181,8 @@
             tal:attributes="src string:${lp_js}/lp/comment.js"></script>
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/lp/errors.js"></script>
+    <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/registry/team.js"></script>
 
   </tal:devmode>
   <tal:production condition="not:devmode">
 
=== 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 02:39:22 +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/')
-
         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 02:39:22 +0000
@@ -751,6 +751,9 @@
             for="lp.registry.interfaces.person.IPerson"
             layer="canonical.launchpad.layers.BugsLayer"
             name="+bugs"/>
+        <adapter
+          factory="lp.registry.browser.person.PersonXHTMLRepresentation"
+          name="lazr.restful.EntryResource" />
         <browser:page
             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 02:39:23 +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:
+            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:
+            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 02:39:23 +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 02:39:22 +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('[e-mail] ')
+        self.person = self.factory.makePerson(
+            name='test-person', displayname='Test Person')
+        self.webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+
+    def test_GET_xhtml_representation(self):
+        response = self.webservice.get(
+            '/~%s' % self.person.name, 'application/xhtml+xml')
+
+        self.assertEqual(response.status, 200)
+
+        rendered_comment = response.body
+        self.assertEquals(
+            rendered_comment,
+            '<a href="/~test-person" class="sprite person">Test Person</a>')
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/registry/doc/teammembership-email-notification.txt'
--- lib/lp/registry/doc/teammembership-email-notification.txt	2009-08-24 03:01:12 +0000
+++ lib/lp/registry/doc/teammembership-email-notification.txt	2009-12-16 02:39:23 +0000
@@ -226,6 +226,7 @@
     >>> cprov = personset.getByName('cprov')
     >>> marilize = personset.getByName('marilize')
     >>> ubuntu_team.addMember(marilize, reviewer=cprov)
+    True
     >>> transaction.commit()
     >>> len(stub.test_emails)
     6
@@ -275,6 +276,7 @@
     >>> mirror_admins.getTeamAdminsEmailAddresses()
     ['[e-mail]']
     >>> ubuntu_team.addMember(mirror_admins, reviewer=cprov)
+    True
     >>> transaction.commit()
     >>> len(stub.test_emails)
     1
@@ -326,6 +328,7 @@
 
     >>> landscape = personset.getByName('landscape-developers')
     >>> ubuntu_team.addMember(landscape, reviewer=cprov)
+    True
 
     # Reset stub.test_emails as we don't care about the notification triggered
     # by the addMember() call.
@@ -359,6 +362,7 @@
 
     >>> launchpad = personset.getByName('launchpad')
     >>> ubuntu_team.addMember(launchpad, reviewer=cprov, force_team_add=True)
+    True
     >>> flush_database_updates()
     >>> transaction.commit()
     >>> len(stub.test_emails)
@@ -812,6 +816,7 @@
     >>> member = factory.makePerson(
     ...     name='team-member', email='[e-mail]')
     >>> team_one.addMember(member, owner)
+    True
     >>> print_distinct_emails()
     From: Team One ...
     ----------------------------------------
@@ -838,6 +843,7 @@
     >>> team_two = factory.makeTeam(
     ...     name='team-two', email='[e-mail]', owner=owner)
     >>> team_one.addMember(team_two, owner, force_team_add=True)
+    True
     >>> print_distinct_emails()
     From: Team One ...
     ----------------------------------------
 
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt	2009-11-30 20:18:42 +0000
+++ lib/lp/registry/doc/teammembership.txt	2009-12-16 02:39:22 +0000
@@ -132,7 +132,6 @@
 Other users must use the join method if they are going to add themselves
 to a team.
 
-    >>> from zope.security.interfaces import Unauthorized
     >>> mark = personset.getByName('mark')
     >>> t3.addMember(salgado, reviewer=mark, status=TeamMembershipStatus.ADMIN)
     Traceback (most recent call last):
@@ -142,8 +141,12 @@
     # Log in as the team owner.
     >>> login_person(t3.teamowner)
 
+addMember returns True if the member got added (i.e. he wasn't already a
+member of the team).
+
     >>> t3.addMember(
     ...     salgado, reviewer=mark, status=TeamMembershipStatus.ADMIN)
+    True
     >>> from canonical.launchpad.interfaces import ITeamMembershipSet
     >>> membershipset = getUtility(ITeamMembershipSet)
     >>> flush_database_updates()
@@ -155,13 +158,27 @@
     >>> salgado in t3.activemembers
     True
 
+addMember returns True also when the member is added as a proposed
+member.
+
     >>> marilize = personset.getByName('marilize')
     >>> t3.addMember(
     ...     marilize, reviewer=mark, status=TeamMembershipStatus.PROPOSED)
+    True
     >>> flush_database_updates()
     >>> marilize in t3.activemembers
     False
 
+If addMember is called with a person that is already a member, it
+returns False.
+
+    >>> t3.addMember(
+    ...     salgado, reviewer=mark, status=TeamMembershipStatus.ADMIN)
+    False
+    >>> t3.addMember(
+    ...     marilize, reviewer=mark, status=TeamMembershipStatus.PROPOSED)
+    False
+
 As expected, the membership object implements ITeamMembership.
 
     >>> from canonical.launchpad.webapp.testing import verifyObject
@@ -175,6 +192,7 @@
 invitation before the team is made a member.
 
     >>> t1.addMember(t2, reviewer)
+    True
     >>> membership = membershipset.getByPersonAndTeam(t2, t1)
     >>> membership.status == TeamMembershipStatus.INVITED
     True
@@ -192,6 +210,7 @@
 A team admin can also decline an invitation made to his team.
 
     >>> t2.addMember(t3, reviewer=mark)
+    True
     >>> login_person(t3.teamowner)
     >>> t3.declineInvitationToBeMemberOf(t2, comment='something')
     >>> membership = membershipset.getByPersonAndTeam(t3, t2)
@@ -205,6 +224,7 @@
 
     >>> login_person(t3.teamowner)
     >>> t2.addMember(t3, reviewer=mark, force_team_add=True)
+    True
     >>> [m.displayname for m in t2.allmembers]
     [u'Foo Bar', u'Guilherme Salgado', u't3']
 
@@ -226,6 +246,7 @@
 Adding t2 as a member of t5 will add all t2 members as t5 members too.
 
     >>> t5.addMember(t2, reviewer, force_team_add=True)
+    True
     >>> [m.displayname for m in t5.allmembers]
     [u'Foo Bar', u'Guilherme Salgado', u't2', u't3']
 
@@ -233,7 +254,9 @@
 members too.
 
     >>> t4.addMember(t5, reviewer, force_team_add=True)
+    True
     >>> t4.addMember(t1, reviewer, force_team_add=True)
+    True
     >>> [m.displayname for m in t4.allmembers]
     [u'Foo Bar', u'Guilherme Salgado', u't1', u't2', u't3', u't5']
 
@@ -327,6 +350,7 @@
 
     >>> cprov = getUtility(IPersonSet).getByName('cprov')
     >>> t3.addMember(cprov, reviewer)
+    True
     >>> [m.displayname for m in t3.allmembers]
     [u'Celso Providelo', u'Foo Bar']
 
@@ -391,15 +415,22 @@
     None
 
 When we approve his membership, the datejoined will contain the date that it
-was approved.
+was approved. It returns True to indicate that the status was changed.
 
     >>> membership.setStatus(TeamMembershipStatus.APPROVED, foobar)
+    True
     >>> print membership.status.title
     Approved
     >>> utc_now = datetime.now(pytz.timezone('UTC'))
     >>> membership.datejoined.date() == utc_now.date()
     True
 
+If setStatus is called again with the same status, it returns False,
+to indicate that the status didn't change.
+
+    >>> membership.setStatus(TeamMembershipStatus.APPROVED, foobar)
+    False
+
 Other status updates won't change datejoined, regardless of the status.
 That's because datejoined stores the date in which the membership was first
 made active.
@@ -415,6 +446,7 @@
 
     >>> foobar_on_buildd.setStatus(
     ...     TeamMembershipStatus.DEACTIVATED, foobar)
+    True
     >>> print foobar_on_buildd.status.title
     Deactivated
     >>> foobar_on_buildd.datejoined <= utc_now
@@ -422,6 +454,7 @@
 
     >>> foobar_on_buildd.setStatus(
     ...     TeamMembershipStatus.APPROVED, foobar)
+    True
     >>> print foobar_on_buildd.status.title
     Approved
     >>> foobar_on_buildd.datejoined <= utc_now
@@ -790,6 +823,7 @@
     >>> admins = getUtility(IPersonSet).getByName('admins')
     >>> login_person(t1.teamowner)
     >>> t1.addMember(admins, reviewer=t1.teamowner, force_team_add=True)
+    True
     >>> flush_database_updates()
     >>> print '\n'.join(sorted(
     ...     team.name for team in salgado.teams_participated_in))
@@ -804,6 +838,7 @@
 for Salgado.
 
     >>> admins.addMember(t2, reviewer=admins.teamowner, force_team_add=True)
+    True
     >>> flush_database_updates()
     >>> print '\n'.join(sorted(
     ...     team.name for team in salgado.teams_participated_in))
@@ -845,6 +880,7 @@
 Or changed:
 
     >>> membership.setStatus(TeamMembershipStatus.DEACTIVATED, mark)
+    True
     >>> no_priv._inTeam_cache
     {}
     >>> no_priv.inTeam(admins)
@@ -902,3 +938,4 @@
     >>> bad_membership.sendAutoRenewalNotification()
 
     >>> bad_membership.setStatus(TeamMembershipStatus.EXPIRED, bad_user)
+    True
 
=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py	2009-06-25 04:06:00 +0000
+++ lib/lp/registry/interfaces/teammembership.py	2009-12-16 02:39:23 +0000
@@ -224,6 +224,8 @@
         transition.
 
         The given status must be different than the current status.
+
+        Return True if the status got changed, otherwise False.
         """
 
 
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2009-12-05 18:37:28 +0000
+++ lib/lp/registry/model/person.py	2009-12-16 02:39:23 +0000
@@ -1236,6 +1236,7 @@
                 status = TeamMembershipStatus.INVITED
                 event = TeamInvitationEvent
 
+        member_added = True
         expires = self.defaultexpirationdate
         tm = TeamMembership.selectOneBy(person=person, team=self)
         if tm is None:
@@ -1250,11 +1251,12 @@
             # We can't use tm.setExpirationDate() here because the reviewer
             # here will be the member themselves when they join an OPEN team.
             tm.dateexpires = expires
-            tm.setStatus(status, reviewer, comment)
+            member_added = tm.setStatus(status, reviewer, comment)
 
         if not person.is_team and may_subscribe_to_list:
             person.autoSubscribeToMailingList(self.mailing_list,
                                               requester=reviewer)
+        return member_added
 
     # The three methods below are not in the IPerson interface because we want
     # to protect them with a launchpad.Edit permission. We could do that by
 
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2009-06-25 04:06:00 +0000
+++ lib/lp/registry/model/teammembership.py	2009-12-16 02:39:23 +0000
@@ -272,7 +272,7 @@
     def setStatus(self, status, user, comment=None):
         """See `ITeamMembership`."""
         if status == self.status:
-            return
+            return False
 
         approved = TeamMembershipStatus.APPROVED
         admin = TeamMembershipStatus.ADMIN
@@ -357,10 +357,9 @@
         # When a member proposes himself, a more detailed notification is
         # sent to the team admins by a subscriber of JoinTeamEvent; that's
         # why we don't send anything here.
-        if self.person == self.last_changed_by and self.status == proposed:
-            return
-
-        self._sendStatusChangeNotification(old_status)
+        if self.person != self.last_changed_by or self.status != proposed:
+            self._sendStatusChangeNotification(old_status)
+        return True
 
     def _sendStatusChangeNotification(self, old_status):
         """Send a status change notification to all team admins and the
 
=== modified file 'lib/lp/registry/templates/team-index.pt'
--- lib/lp/registry/templates/team-index.pt	2009-10-26 21:12:49 +0000
+++ lib/lp/registry/templates/team-index.pt	2009-12-16 02:39:22 +0000
@@ -17,6 +17,16 @@
         rel="meta" type="application/rdf+xml"
         title="FOAF" href="+rdf"
         />
+  <script type="text/javascript"
+          tal:content="string:
+    YUI().use('registry.team', function(Y) {
+        Y.on('load',
+             function(e) {
+                 Y.registry.team.setup_add_member_handler();
+             },
+             window);
+    });
+  "/>
   </tal:block>
 </head>
 
 
=== modified file 'lib/lp/registry/templates/team-portlet-membership.pt'
--- lib/lp/registry/templates/team-portlet-membership.pt	2009-10-23 21:11:12 +0000
+++ lib/lp/registry/templates/team-portlet-membership.pt	2009-12-16 02:39:23 +0000
@@ -18,7 +18,9 @@
     <div id="membership-summary">
       <div>
         <img src="/@@/team" alt="team" />
-        <strong><tal:active content="context/all_member_count" /></strong>
+        <strong id="member-count">
+          <tal:active content="context/all_member_count" />
+        </strong>
         <a tal:attributes="href string:${context/fmt:url/+members}#active"
           >active members</a><tal:invited
             define="invited_member_count context/invited_member_count"
@@ -90,24 +92,33 @@
   <tal:can-view
     condition="context/@@+restricted-membership/userCanViewMembership"
     define="overview_menu context/menu:overview">
-  <table style="margin: 0px 0px .5em 0px;"
-    tal:condition="view/has_recent_approved_or_proposed_members">
+  <table style="margin: 0px 0px .5em 0px;">
     <tr>
+      <td style="padding: 0px 1em 1em 0px;"
+          tal:define="link context/menu:overview/add_member">
+        <span id="add-member-spinner" class="update-in-progress-message"
+              style="display: none">
+          Saving...
+        </span>
+        <tal:add-member replace="structure link/fmt:link-icon" />
+      </td>
       <td style="padding: 0px 0px 1em 0px;"
           tal:define="link context/menu:overview/mugshots">
         <tal:mugshots replace="structure link/fmt:link-icon" />
       </td>
     </tr>
     <tr>
-      <td style="padding: 3px 3em 0px 0px;" id="recently-approved"
-        tal:condition="view/recently_approved_members">
-        <h3 style="color:black; font-weight:bold; margin: 0px">
-          Recently approved
-        </h3>
-        <ul tal:condition="view/recently_approved_members">
-          <li tal:repeat="person view/recently_approved_members"
-              tal:content="structure person/fmt:link" />
-        </ul>
+      <td style="padding: 3px 3em 0px 0px;">
+        <div id="recently-approved"
+             tal:attributes="style view/recently_approved_hidden">
+          <h3 style="color:black; font-weight:bold; margin: 0px">
+            Latest members
+          </h3>
+          <ul id="recently-approved-ul">
+            <li tal:repeat="person view/recently_approved_members"
+                tal:content="structure person/fmt:link" />
+          </ul>
+        </div>
       </td>
       <td style="padding: 0px;" id="recently-applied"
         tal:condition="view/recently_proposed_members">