Code review comment for lp://staging/~jpds/launchpad/fix_450262

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 @@
> <http://launchpad.dev/~ubuntu-team>
> ----------------------------------------
>
> +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 @@
>
> <metal:comment use-macro="template/macros/comment" />
>
> + <tr tal:condition="context/required:launchpad.Admin">
> + <th>Silent:</th>
> + <td>
> + <input type="checkbox" value="no" name="silent" id="silent" />
> + </td>
> + </tr>

The <th> and <td> 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.

> <tr>
> <th />
> <td>

Please see this paste for suggested changes:

http://pastebin.ubuntu.com/342934/