Merge lp://staging/~jpds/launchpad/fix_450262 into lp://staging/launchpad

= Summary =

In certain circumstances, we need to be able to remove people from teams silently without the change being widely announced.

This branch adds a "Silently change" checkbox to a person's team membership page which, if checked, disables email notifications for that change.

To avoid abuse, this feature is limited to Launchpad Administrators.

As we discussed on IRC this change needs a test to ensure email is sent when silent is False and is not sent when silent is True.

I'll do a more thorough review after the test is added.

Screenshot of what this change implements is available at:

http://people.canonical.com/~jpds/fix_450262_screen.png

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

Hi Jonathan! Wow, you're really getting stuck into some great bugs :)

Your change here looks great... I've only got a few small thoughts.

I was about to mention that the field needs a description after looking at your screenshot, but on merging I see you've already done that! :)

I'm not an English expert, but "Do not send notification to anyone regarding this membership change." reads more clearly to me than "Do not send notification to anyone of this membership change."? (I was confused for half a second by of until I realised the context). But up to you, I think they're both fine.

The label 'Silent' on it's own could still be a bit more descriptive - I know the description is there, but the label itself should make sense on it's own too. Maybe 'Change silently'? That would still be suitably complemented by the description?

It is a shame that editing membership doesn't add a normal notification on the screen after submitting, something like "Foo Bar's membership has been updated successfully. No one was notified via email.", but that's not part of this branch. It was because of this that I was initially uncertain whether deactivation was also silent - I saw checking your diff that it is, but that then left me wondering why reactivation cannot be silent?

To reproduce:
1. Visit https://launchpad.dev/~ubuntu-team/+members
2. Silently deactivate a member.
3. Edit their membership again under Former members.
I can then reactivate their membership but without any option to do it silently?

Again, not a big issue at this point, but it's worth an XXX if you're not keen to tackle it now - unless it's an intentional decision?

So I'd be happy for you to land this as long as you've considering the above points.

Changes since Brad's review: http://pastebin.ubuntu.com/343668/

Thanks for the code changes Jonathan. I'll land it for you now.

Hi Jonathan,

Due to the fact that you cannot submit directly to PQM and must rely on your reviewer to do it for you we find ourselves in a bit of a bind. After I approved your code and sent it off using 'ec2 land', which runs the full (slow) test suite before sending to PQM, you continued working on this branch and pushed new revisions. So the version tested by ec2 and the version landed by PQM are different. Were the developer the same person who submits it to PQM then this problem would not occur as s/he would be more cognizant of the steps that had been taken.

So, here we are.

Clearly moving forward we need the understanding that once a MP is approved no additional changes should be made to the branch on Launchpad. I see the MP has an 'Approved revision' but it is not set on this MP. It would be nice if ec2 and PQM could co-operate to only land the approved revision but I don't know if that is feasible ATM.

The unreviewed changes are at http://paste.ubuntu.com/343901/

Two small things:
1) top-level module elements are separated by two blank lines according to our coding standards. So the new class you added needs an additional line before and after it.

2) We /try/ to use en_us so please change to 'authorized'.

3) There needs to be a test for the condition where the exception is raised. Consider for a moment that you had a typo in the name of the exception but it was never tested. In production if the situation were to arise then we'd get a run-time error. So, for this reason, you really need a test to ensure the error handling mechanism works properly.

To clean up this situation I'd recommend you file a bug for trivial clean up, apply the changes I've suggested, and put the new, tiny branch up for review. It would be too ugly to try to re-use this merge proposal any further than I've done here.

Finally, don't worry about the mix up. Let's be more careful in the future, though. Thanks again for the great branches you've done lately.