Hi Jonathan, Thanks for taking on another interesting bug to fix. Thanks also for the additional test. I did you a bit of a disservice by not doing a full review earlier. Now that I have I've discovered a few more things that need fixing and yet another test that needs to be added. But I'm confident with these additions the branch will be ready to land, at least from a code perspective. Recall you need to get beuno or noodles to do a UI review. > === modified file 'lib/lp/registry/browser/teammembership.py' > --- lib/lp/registry/browser/teammembership.py 2009-10-07 21:54:20 +0000 +++ lib/lp/registry/browser/teammembership.py 2009-12-16 10:19:15 +0000 > @@ -303,7 +303,8 @@ > > self.context.setExpirationDate(expires, self.user) The silent param is a boolean, so rather than relying on the form to return True, False, or None do the following: silent = self.request.form.get('silent', False) and then use that value in the call to setStatus. > self.context.setStatus( > - status, self.user, self.request.form_ng.getOne('comment')) > + status, self.user, self.request.form_ng.getOne('comment'), > + self.request.form.get('silent')) > return True > > def _getExpirationDate(self): > === 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 10:19:15 +0000 > @@ -9,14 +9,16 @@ > >>> def by_to_addrs(a, b): > ... return cmp(a[1], b[1]) > > - >>> def setStatus(membership, status, reviewer=None, comment=None): > + >>> def setStatus(membership, status, reviewer=None, comment=None, > + ... silent=False): I think we have the choice to indent by 4 or line up after the opening paren here. I prefer the latter. > ... """Set the status of the given membership. > ... > ... Also sets the reviewer and comment, calling flush_database_updates > ... and transaction.commit after, to ensure the changes are flushed to > ... the database. > ... """ > - ... membership.setStatus(status, reviewer, comment=comment) > + ... membership.setStatus(status, reviewer, comment=comment, > + ... silent=silent) > ... flush_database_updates() > ... transaction.commit() > > @@ -780,7 +782,7 @@ > >>> mirror_admins_membership = membershipset.getByPersonAndTeam( > ... mirror_admins, ubuntu_team) > >>> setStatus(mirror_admins_membership, TeamMembershipStatus.DEACTIVATED, > - ... reviewer=mark) > + ... reviewer=mark, silent=False) > >>> len(stub.test_emails) > 6 > > @@ -797,6 +799,21 @@ > > ---------------------------------------- > > +Deactivating memberships can also be done silently (no email notifications > +sent) by Launchpad Administrators. > + > + >>> thumper = getUtility(IPersonSet).getByName('thumper') > + >>> hwdb_admins = personset.getByName('hwdb-team') > + >>> thumper_hwdb_membership = membershipset.getByPersonAndTeam(thumper, > + ... hwdb_admins) > + >>> print thumper_hwdb_membership.status.title > + Approved > + >>> setStatus(thumper_hwdb_membership, > + ... TeamMembershipStatus.DEACTIVATED, reviewer=mark, silent=True) > + >>> len(stub.test_emails) > + 0 > + >>> print thumper_hwdb_membership.status.title > + Deactivated Nice new test! > Joining a team with a mailing list > ---------------------------------- > === 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 10:19:15 +0000 > @@ -214,7 +214,7 @@ > status=copy_field(status), > comment=copy_field(reviewer_comment)) > @export_write_operation() > - def setStatus(status, user, comment=None): > + def setStatus(status, user, comment=None, silent=False): > """Set the status of this membership. The way you've made this change does not allow the new parameter to be used via the webservice/launchpadlib. Look at the paste I've included at the bottom for changes which will export the new parameter. You'll need to do a 'make clean build' to ensure the wadl gets rebuilt. A test in registry/webservices/xx-person.txt will be needed to show that the silent param works from the web service and that it only works for admins. You can also exercise it interactively using launchpadlib. Here's a transcript to get you started: % bin/ipy In [1]: from launchpadlib.launchpad import Launchpad In [2]: lp = Launchpad.login_with('interactive', 'dev') In [3]: salgado = lp.people['salgado'] In [4]: salgado_ls = list(salgado.memberships_details)[1] In [5]: salgado_ls.setStatus(status='Deactivated', silent=False) In [6]: salgado_ls.setStatus(status='Deactivated', silent=True) If you have your mailer set up properly you'll even get the email when generated. > The user and comment are stored in last_changed_by and > === 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 10:19:15 +0000 > @@ -165,6 +165,14 @@ > template % replacements, force_wrap=True) > simple_sendmail(from_addr, address, subject, msg) > > + def canChangeStatusSilently(self, user): > + """Ensure that the user is in the Launchpad Administrators group before > + silently making changes to their membership status.""" > + if user.inTeam(getUtility(ILaunchpadCelebrities).admin): > + return True > + else: > + return False Simplify to: > + return user.inTeam(getUtility(ILaunchpadCelebrities).admin > def canChangeExpirationDate(self, person): > """See `ITeamMembership`.""" > person_is_admin = self.team in person.getAdministratedTeams() > @@ -269,11 +277,15 @@ > team.displayname, config.canonical.noreply_from_address) > simple_sendmail(from_addr, to_addrs, subject, msg) > > - def setStatus(self, status, user, comment=None): > + def setStatus(self, status, user, comment=None, silent=False): > """See `ITeamMembership`.""" > if status == self.status: > return > > + if silent: > + assert self.canChangeStatusSilently(user), ( > + "You may not change user's membership statuses silently.") > + > approved = TeamMembershipStatus.APPROVED > admin = TeamMembershipStatus.ADMIN > expired = TeamMembershipStatus.EXPIRED > @@ -360,7 +372,8 @@ > if self.person == self.last_changed_by and self.status == proposed: > return > > - self._sendStatusChangeNotification(old_status) > + if not silent: > + self._sendStatusChangeNotification(old_status) > > def _sendStatusChangeNotification(self, old_status): > """Send a status change notification to all team admins and the > === modified file 'lib/lp/registry/templates/teammembership-index.pt' > --- lib/lp/registry/templates/teammembership-index.pt 2009-12-03 18:33:22 +0000 > +++ lib/lp/registry/templates/teammembership-index.pt 2009-12-16 10:19:15 +0000 > @@ -140,6 +140,13 @@ > > > > + > + Silent: > + > + > + > + The and should be indented only 2 spaces. I don't like the use of "silent" in the UI. I suspect the UI reviewer might comment on that. In the attached paste I have some changes to the template to add some explanatory text to the checkbox and to augment the explanation for the comment. > > > Please see this paste for suggested changes: http://pastebin.ubuntu.com/342934/