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

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
1=== modified file 'lib/lp/registry/browser/teammembership.py'
2--- lib/lp/registry/browser/teammembership.py 2009-10-07 21:54:20 +0000
3+++ lib/lp/registry/browser/teammembership.py 2009-12-18 23:48:21 +0000
4@@ -301,9 +301,12 @@
5 else:
6 expires = self.context.dateexpires
7
8+ silent = self.request.form.get('silent', False)
9+
10 self.context.setExpirationDate(expires, self.user)
11 self.context.setStatus(
12- status, self.user, self.request.form_ng.getOne('comment'))
13+ status, self.user, self.request.form_ng.getOne('comment'),
14+ silent)
15 return True
16
17 def _getExpirationDate(self):
18
19=== modified file 'lib/lp/registry/doc/teammembership-email-notification.txt'
20--- lib/lp/registry/doc/teammembership-email-notification.txt 2009-08-24 03:01:12 +0000
21+++ lib/lp/registry/doc/teammembership-email-notification.txt 2009-12-18 23:48:21 +0000
22@@ -9,14 +9,16 @@
23 >>> def by_to_addrs(a, b):
24 ... return cmp(a[1], b[1])
25
26- >>> def setStatus(membership, status, reviewer=None, comment=None):
27+ >>> def setStatus(membership, status, reviewer=None, comment=None,
28+ ... silent=False):
29 ... """Set the status of the given membership.
30 ...
31 ... Also sets the reviewer and comment, calling flush_database_updates
32 ... and transaction.commit after, to ensure the changes are flushed to
33 ... the database.
34 ... """
35- ... membership.setStatus(status, reviewer, comment=comment)
36+ ... membership.setStatus(status, reviewer, comment=comment,
37+ ... silent=silent)
38 ... flush_database_updates()
39 ... transaction.commit()
40
41@@ -780,7 +782,7 @@
42 >>> mirror_admins_membership = membershipset.getByPersonAndTeam(
43 ... mirror_admins, ubuntu_team)
44 >>> setStatus(mirror_admins_membership, TeamMembershipStatus.DEACTIVATED,
45- ... reviewer=mark)
46+ ... reviewer=mark, silent=False)
47 >>> len(stub.test_emails)
48 6
49
50@@ -797,6 +799,21 @@
51 <http://launchpad.dev/~ubuntu-team>
52 ----------------------------------------
53
54+Deactivating memberships can also be done silently (no email notifications
55+sent) by Launchpad Administrators.
56+
57+ >>> thumper = getUtility(IPersonSet).getByName('thumper')
58+ >>> hwdb_admins = personset.getByName('hwdb-team')
59+ >>> thumper_hwdb_membership = membershipset.getByPersonAndTeam(thumper,
60+ ... hwdb_admins)
61+ >>> print thumper_hwdb_membership.status.title
62+ Approved
63+ >>> setStatus(thumper_hwdb_membership,
64+ ... TeamMembershipStatus.DEACTIVATED, reviewer=mark, silent=True)
65+ >>> len(stub.test_emails)
66+ 0
67+ >>> print thumper_hwdb_membership.status.title
68+ Deactivated
69
70 Joining a team with a mailing list
71 ----------------------------------
72
73=== modified file 'lib/lp/registry/interfaces/teammembership.py'
74--- lib/lp/registry/interfaces/teammembership.py 2009-06-25 04:06:00 +0000
75+++ lib/lp/registry/interfaces/teammembership.py 2009-12-18 23:48:21 +0000
76@@ -16,15 +16,16 @@
77 'TeamMembershipStatus',
78 ]
79
80-from zope.schema import Choice, Datetime, Int, Text
81+from zope.schema import Bool, Choice, Datetime, Int, Text
82 from zope.interface import Attribute, Interface
83+from zope.security.interfaces import Unauthorized
84 from lazr.enum import DBEnumeratedType, DBItem
85
86 from lazr.restful.interface import copy_field
87 from lazr.restful.fields import Reference
88 from lazr.restful.declarations import (
89 call_with, export_as_webservice_entry, export_write_operation, exported,
90- operation_parameters, REQUEST_USER)
91+ operation_parameters, REQUEST_USER, webservice_error)
92
93 from canonical.launchpad import _
94
95@@ -33,6 +34,13 @@
96 # admin to do so, depending on the team's renewal policy.
97 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT = 7
98
99+class UserCannotChangeMembershipSilently(Unauthorized):
100+ """User not permitted to change membership status silently.
101+
102+ Raised when a user tries to change someone's membership silently, and is not
103+ a Launchpad Administrator.
104+ """
105+ webservice_error(401) # HTTP Error: 'Unauthorised'
106
107 class TeamMembershipStatus(DBEnumeratedType):
108 """TeamMembership Status
109@@ -212,9 +220,12 @@
110 @call_with(user=REQUEST_USER)
111 @operation_parameters(
112 status=copy_field(status),
113- comment=copy_field(reviewer_comment))
114+ comment=copy_field(reviewer_comment),
115+ silent=Bool(title=_("Do not send notifications of status change. For "
116+ "use by Launchpad administrators only."),
117+ required=False, default=False))
118 @export_write_operation()
119- def setStatus(status, user, comment=None):
120+ def setStatus(status, user, comment=None, silent=False):
121 """Set the status of this membership.
122
123 The user and comment are stored in last_changed_by and
124
125=== modified file 'lib/lp/registry/model/teammembership.py'
126--- lib/lp/registry/model/teammembership.py 2009-06-25 04:06:00 +0000
127+++ lib/lp/registry/model/teammembership.py 2009-12-18 23:48:21 +0000
128@@ -40,7 +40,7 @@
129 from lp.registry.interfaces.teammembership import (
130 CyclicalTeamMembershipError, DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
131 ITeamMembership, ITeamMembershipSet, ITeamParticipation,
132- TeamMembershipStatus)
133+ TeamMembershipStatus, UserCannotChangeMembershipSilently)
134 from canonical.launchpad.webapp import canonical_url
135 from canonical.launchpad.webapp.tales import DurationFormatterAPI
136
137@@ -165,6 +165,11 @@
138 template % replacements, force_wrap=True)
139 simple_sendmail(from_addr, address, subject, msg)
140
141+ def canChangeStatusSilently(self, user):
142+ """Ensure that the user is in the Launchpad Administrators group before
143+ silently making changes to their membership status."""
144+ return user.inTeam(getUtility(ILaunchpadCelebrities).admin)
145+
146 def canChangeExpirationDate(self, person):
147 """See `ITeamMembership`."""
148 person_is_admin = self.team in person.getAdministratedTeams()
149@@ -269,11 +274,16 @@
150 team.displayname, config.canonical.noreply_from_address)
151 simple_sendmail(from_addr, to_addrs, subject, msg)
152
153- def setStatus(self, status, user, comment=None):
154+ def setStatus(self, status, user, comment=None, silent=False):
155 """See `ITeamMembership`."""
156 if status == self.status:
157 return
158
159+ if silent and not self.canChangeStatusSilently(user):
160+ raise UserCannotChangeMembershipSilently(
161+ "Only Launchpad administrators may change membership status "
162+ "silently.")
163+
164 approved = TeamMembershipStatus.APPROVED
165 admin = TeamMembershipStatus.ADMIN
166 expired = TeamMembershipStatus.EXPIRED
167@@ -360,7 +370,8 @@
168 if self.person == self.last_changed_by and self.status == proposed:
169 return
170
171- self._sendStatusChangeNotification(old_status)
172+ if not silent:
173+ self._sendStatusChangeNotification(old_status)
174
175 def _sendStatusChangeNotification(self, old_status):
176 """Send a status change notification to all team admins and the
177
178=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
179--- lib/lp/registry/stories/webservice/xx-person.txt 2009-12-05 18:37:28 +0000
180+++ lib/lp/registry/stories/webservice/xx-person.txt 2009-12-18 23:48:21 +0000
181@@ -243,6 +243,22 @@
182 >>> print webservice.get(
183 ... salgado_landscape['self_link']).jsonBody()['status']
184 Deactivated
185+
186+ >>> print webservice.named_post(
187+ ... salgado_landscape['self_link'], 'setStatus', {},
188+ ... status='Approved', silent=True)
189+ HTTP/1.1 200 Ok
190+ ...
191+
192+ >>> print webservice.get(
193+ ... salgado_landscape['self_link']).jsonBody()['status']
194+ Approved
195+
196+ >>> print webservice.named_post(
197+ ... salgado_landscape['self_link'], 'setStatus', {},
198+ ... status='Deactivated', silent=True)
199+ HTTP/1.1 200 Ok
200+ ...
201
202 # Now revert the change to salgado's membership to not break other tests
203 # further down.
204
205=== modified file 'lib/lp/registry/templates/teammembership-index.pt'
206--- lib/lp/registry/templates/teammembership-index.pt 2009-12-03 18:33:22 +0000
207+++ lib/lp/registry/templates/teammembership-index.pt 2009-12-18 23:48:21 +0000
208@@ -58,7 +58,9 @@
209 </textarea>
210 <div class="formHelp">This comment will be sent together with the
211 notification of this change to all team administrators and this
212- member.</div>
213+ member<span tal:condition="view/isActive"><span
214+ tal:condition="context/required:launchpad.Admin">, unless the 'Silent'
215+ option is selected</span></span>.</div>
216 </td>
217 </tr>
218 </metal:macro>
219@@ -140,6 +142,15 @@
220
221 <metal:comment use-macro="template/macros/comment" />
222
223+ <tr tal:condition="context/required:launchpad.Admin">
224+ <th>Change silently:</th>
225+ <td>
226+ <input type="checkbox" value="no" name="silent" id="silent" />
227+ <div class="formHelp">Do not send notifications to anyone
228+ regarding this membership change.</div>
229+ </td>
230+ </tr>
231+
232 <tr>
233 <th />
234 <td>