Launchpad itself

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

  1. fix_450262
  2. Merge into devel
Proposed by Jonathan Davies
Status: Merged
Approved by: Brad Crittenden
Approved revision: not available
Merged at revision: 10063
Proposed branch: lp:~jpds/launchpad/fix_450262
Merge into: lp:launchpad
Diff against target: 234 lines (+81/-12)
6 files modified
lib/lp/registry/browser/teammembership.py (+4/-1)
lib/lp/registry/doc/teammembership-email-notification.txt (+20/-3)
lib/lp/registry/interfaces/teammembership.py (+15/-4)
lib/lp/registry/model/teammembership.py (+14/-3)
lib/lp/registry/stories/webservice/xx-person.txt (+16/-0)
lib/lp/registry/templates/teammembership-index.pt (+12/-1)
To merge this branch: bzr merge lp:~jpds/launchpad/fix_450262
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Michael Nelson (community) ui Approve
Review via email: mp+16203@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Jonathan Davies (jpds) wrote :

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

Revision history for this message
Brad Crittenden (bac) wrote :

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.

review: Needs Fixing (code)
Revision history for this message
Jonathan Davies (jpds) wrote :

Screenshot of what this change implements is available at:

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

Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (8.1 KiB)

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

Read more...

review: Needs Fixing (code)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.

review: Approve (ui)
Revision history for this message
Jonathan Davies (jpds) wrote :

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

Revision history for this message
Brad Crittenden (bac) wrote :

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

review: Approve (code)
Revision history for this message
Brad Crittenden (bac) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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-18 23:48:21 +0000
@@ -301,9 +301,12 @@
301 else:301 else:
302 expires = self.context.dateexpires302 expires = self.context.dateexpires
303303
304 silent = self.request.form.get('silent', False)
305
304 self.context.setExpirationDate(expires, self.user)306 self.context.setExpirationDate(expires, self.user)
305 self.context.setStatus(307 self.context.setStatus(
306 status, self.user, self.request.form_ng.getOne('comment'))308 status, self.user, self.request.form_ng.getOne('comment'),
309 silent)
307 return True310 return True
308311
309 def _getExpirationDate(self):312 def _getExpirationDate(self):
310313
=== 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-18 23:48:21 +0000
@@ -9,14 +9,16 @@
9 >>> def by_to_addrs(a, b):9 >>> def by_to_addrs(a, b):
10 ... return cmp(a[1], b[1])10 ... return cmp(a[1], b[1])
1111
12 >>> def setStatus(membership, status, reviewer=None, comment=None):12 >>> def setStatus(membership, status, reviewer=None, comment=None,
13 ... silent=False):
13 ... """Set the status of the given membership.14 ... """Set the status of the given membership.
14 ...15 ...
15 ... Also sets the reviewer and comment, calling flush_database_updates16 ... Also sets the reviewer and comment, calling flush_database_updates
16 ... and transaction.commit after, to ensure the changes are flushed to17 ... and transaction.commit after, to ensure the changes are flushed to
17 ... the database.18 ... the database.
18 ... """19 ... """
19 ... membership.setStatus(status, reviewer, comment=comment)20 ... membership.setStatus(status, reviewer, comment=comment,
21 ... silent=silent)
20 ... flush_database_updates()22 ... flush_database_updates()
21 ... transaction.commit()23 ... transaction.commit()
2224
@@ -780,7 +782,7 @@
780 >>> mirror_admins_membership = membershipset.getByPersonAndTeam(782 >>> mirror_admins_membership = membershipset.getByPersonAndTeam(
781 ... mirror_admins, ubuntu_team)783 ... mirror_admins, ubuntu_team)
782 >>> setStatus(mirror_admins_membership, TeamMembershipStatus.DEACTIVATED,784 >>> setStatus(mirror_admins_membership, TeamMembershipStatus.DEACTIVATED,
783 ... reviewer=mark)785 ... reviewer=mark, silent=False)
784 >>> len(stub.test_emails)786 >>> len(stub.test_emails)
785 6787 6
786788
@@ -797,6 +799,21 @@
797 <http://launchpad.dev/~ubuntu-team>799 <http://launchpad.dev/~ubuntu-team>
798 ----------------------------------------800 ----------------------------------------
799801
802Deactivating memberships can also be done silently (no email notifications
803sent) by Launchpad Administrators.
804
805 >>> thumper = getUtility(IPersonSet).getByName('thumper')
806 >>> hwdb_admins = personset.getByName('hwdb-team')
807 >>> thumper_hwdb_membership = membershipset.getByPersonAndTeam(thumper,
808 ... hwdb_admins)
809 >>> print thumper_hwdb_membership.status.title
810 Approved
811 >>> setStatus(thumper_hwdb_membership,
812 ... TeamMembershipStatus.DEACTIVATED, reviewer=mark, silent=True)
813 >>> len(stub.test_emails)
814 0
815 >>> print thumper_hwdb_membership.status.title
816 Deactivated
800817
801Joining a team with a mailing list818Joining a team with a mailing list
802----------------------------------819----------------------------------
803820
=== 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-18 23:48:21 +0000
@@ -16,15 +16,16 @@
16 'TeamMembershipStatus',16 'TeamMembershipStatus',
17 ]17 ]
1818
19from zope.schema import Choice, Datetime, Int, Text19from zope.schema import Bool, Choice, Datetime, Int, Text
20from zope.interface import Attribute, Interface20from zope.interface import Attribute, Interface
21from zope.security.interfaces import Unauthorized
21from lazr.enum import DBEnumeratedType, DBItem22from lazr.enum import DBEnumeratedType, DBItem
2223
23from lazr.restful.interface import copy_field24from lazr.restful.interface import copy_field
24from lazr.restful.fields import Reference25from lazr.restful.fields import Reference
25from lazr.restful.declarations import (26from lazr.restful.declarations import (
26 call_with, export_as_webservice_entry, export_write_operation, exported,27 call_with, export_as_webservice_entry, export_write_operation, exported,
27 operation_parameters, REQUEST_USER)28 operation_parameters, REQUEST_USER, webservice_error)
2829
29from canonical.launchpad import _30from canonical.launchpad import _
3031
@@ -33,6 +34,13 @@
33# admin to do so, depending on the team's renewal policy.34# admin to do so, depending on the team's renewal policy.
34DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT = 735DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT = 7
3536
37class UserCannotChangeMembershipSilently(Unauthorized):
38 """User not permitted to change membership status silently.
39
40 Raised when a user tries to change someone's membership silently, and is not
41 a Launchpad Administrator.
42 """
43 webservice_error(401) # HTTP Error: 'Unauthorised'
3644
37class TeamMembershipStatus(DBEnumeratedType):45class TeamMembershipStatus(DBEnumeratedType):
38 """TeamMembership Status46 """TeamMembership Status
@@ -212,9 +220,12 @@
212 @call_with(user=REQUEST_USER)220 @call_with(user=REQUEST_USER)
213 @operation_parameters(221 @operation_parameters(
214 status=copy_field(status),222 status=copy_field(status),
215 comment=copy_field(reviewer_comment))223 comment=copy_field(reviewer_comment),
224 silent=Bool(title=_("Do not send notifications of status change. For "
225 "use by Launchpad administrators only."),
226 required=False, default=False))
216 @export_write_operation()227 @export_write_operation()
217 def setStatus(status, user, comment=None):228 def setStatus(status, user, comment=None, silent=False):
218 """Set the status of this membership.229 """Set the status of this membership.
219230
220 The user and comment are stored in last_changed_by and231 The user and comment are stored in last_changed_by and
221232
=== 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-18 23:48:21 +0000
@@ -40,7 +40,7 @@
40from lp.registry.interfaces.teammembership import (40from lp.registry.interfaces.teammembership import (
41 CyclicalTeamMembershipError, DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,41 CyclicalTeamMembershipError, DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
42 ITeamMembership, ITeamMembershipSet, ITeamParticipation,42 ITeamMembership, ITeamMembershipSet, ITeamParticipation,
43 TeamMembershipStatus)43 TeamMembershipStatus, UserCannotChangeMembershipSilently)
44from canonical.launchpad.webapp import canonical_url44from canonical.launchpad.webapp import canonical_url
45from canonical.launchpad.webapp.tales import DurationFormatterAPI45from canonical.launchpad.webapp.tales import DurationFormatterAPI
4646
@@ -165,6 +165,11 @@
165 template % replacements, force_wrap=True)165 template % replacements, force_wrap=True)
166 simple_sendmail(from_addr, address, subject, msg)166 simple_sendmail(from_addr, address, subject, msg)
167167
168 def canChangeStatusSilently(self, user):
169 """Ensure that the user is in the Launchpad Administrators group before
170 silently making changes to their membership status."""
171 return user.inTeam(getUtility(ILaunchpadCelebrities).admin)
172
168 def canChangeExpirationDate(self, person):173 def canChangeExpirationDate(self, person):
169 """See `ITeamMembership`."""174 """See `ITeamMembership`."""
170 person_is_admin = self.team in person.getAdministratedTeams()175 person_is_admin = self.team in person.getAdministratedTeams()
@@ -269,11 +274,16 @@
269 team.displayname, config.canonical.noreply_from_address)274 team.displayname, config.canonical.noreply_from_address)
270 simple_sendmail(from_addr, to_addrs, subject, msg)275 simple_sendmail(from_addr, to_addrs, subject, msg)
271276
272 def setStatus(self, status, user, comment=None):277 def setStatus(self, status, user, comment=None, silent=False):
273 """See `ITeamMembership`."""278 """See `ITeamMembership`."""
274 if status == self.status:279 if status == self.status:
275 return280 return
276281
282 if silent and not self.canChangeStatusSilently(user):
283 raise UserCannotChangeMembershipSilently(
284 "Only Launchpad administrators may change membership status "
285 "silently.")
286
277 approved = TeamMembershipStatus.APPROVED287 approved = TeamMembershipStatus.APPROVED
278 admin = TeamMembershipStatus.ADMIN288 admin = TeamMembershipStatus.ADMIN
279 expired = TeamMembershipStatus.EXPIRED289 expired = TeamMembershipStatus.EXPIRED
@@ -360,7 +370,8 @@
360 if self.person == self.last_changed_by and self.status == proposed:370 if self.person == self.last_changed_by and self.status == proposed:
361 return371 return
362372
363 self._sendStatusChangeNotification(old_status)373 if not silent:
374 self._sendStatusChangeNotification(old_status)
364375
365 def _sendStatusChangeNotification(self, old_status):376 def _sendStatusChangeNotification(self, old_status):
366 """Send a status change notification to all team admins and the377 """Send a status change notification to all team admins and the
367378
=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt 2009-12-05 18:37:28 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt 2009-12-18 23:48:21 +0000
@@ -243,6 +243,22 @@
243 >>> print webservice.get(243 >>> print webservice.get(
244 ... salgado_landscape['self_link']).jsonBody()['status']244 ... salgado_landscape['self_link']).jsonBody()['status']
245 Deactivated245 Deactivated
246
247 >>> print webservice.named_post(
248 ... salgado_landscape['self_link'], 'setStatus', {},
249 ... status='Approved', silent=True)
250 HTTP/1.1 200 Ok
251 ...
252
253 >>> print webservice.get(
254 ... salgado_landscape['self_link']).jsonBody()['status']
255 Approved
256
257 >>> print webservice.named_post(
258 ... salgado_landscape['self_link'], 'setStatus', {},
259 ... status='Deactivated', silent=True)
260 HTTP/1.1 200 Ok
261 ...
246262
247 # Now revert the change to salgado's membership to not break other tests263 # Now revert the change to salgado's membership to not break other tests
248 # further down.264 # further down.
249265
=== 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-18 23:48:21 +0000
@@ -58,7 +58,9 @@
58 </textarea>58 </textarea>
59 <div class="formHelp">This comment will be sent together with the59 <div class="formHelp">This comment will be sent together with the
60 notification of this change to all team administrators and this60 notification of this change to all team administrators and this
61 member.</div>61 member<span tal:condition="view/isActive"><span
62 tal:condition="context/required:launchpad.Admin">, unless the 'Silent'
63 option is selected</span></span>.</div>
62 </td>64 </td>
63 </tr>65 </tr>
64 </metal:macro>66 </metal:macro>
@@ -140,6 +142,15 @@
140142
141 <metal:comment use-macro="template/macros/comment" />143 <metal:comment use-macro="template/macros/comment" />
142144
145 <tr tal:condition="context/required:launchpad.Admin">
146 <th>Change silently:</th>
147 <td>
148 <input type="checkbox" value="no" name="silent" id="silent" />
149 <div class="formHelp">Do not send notifications to anyone
150 regarding this membership change.</div>
151 </td>
152 </tr>
153
143 <tr>154 <tr>
144 <th />155 <th />
145 <td>156 <td>