Launchpad itself

Merge lp:~brian-murray/launchpad/bug-97633 into lp:launchpad

  1. bug-97633
  2. Merge into devel
Proposed by Brian Murray
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 11046
Proposed branch: lp:~brian-murray/launchpad/bug-97633
Merge into: lp:launchpad
Diff against target: 435 lines (+205/-24)
12 files modified
lib/canonical/launchpad/doc/notification-recipient-set.txt (+14/-1)
lib/canonical/launchpad/interfaces/launchpad.py (+8/-1)
lib/canonical/launchpad/mailnotification.py (+34/-11)
lib/lp/bugs/doc/bugnotification-email.txt (+1/-1)
lib/lp/bugs/doc/bugnotification-sending.txt (+2/-2)
lib/lp/bugs/doc/bugnotifications.txt (+1/-1)
lib/lp/bugs/doc/bugtask.txt (+2/-2)
lib/lp/bugs/mail/tests/test_bug_task_assignment.py (+120/-0)
lib/lp/bugs/model/bugtask.py (+1/-1)
lib/lp/bugs/tests/test_bugchanges.py (+2/-2)
lib/lp/bugs/tests/test_bugtask.py (+1/-1)
lib/lp/services/mail/notificationrecipientset.py (+19/-1)
To merge this branch: bzr merge lp:~brian-murray/launchpad/bug-97633
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+28109@code.launchpad.net

Commit message

Modify new bug subscription message to identify assignment and assigner instead of treating it like a subscription.

Description of the change

= Summary =

Bug 97633 describes three issues two of which are addressed in this bug report
while the third was put into a separate bug report (bug 596974).

1) Email notification says you have been subscribed when you were assigned
2) Receive 2 emails one about the bug subscription and one about bug
transition
3) Email notification does identify self-assignment (bug 596974)

== Proposed fix ==

1) This is caused by generate_bug_add_email in mailnotification.py not
checking if the new_recipient was an assignee. Additionally, there is no
tracking of whom the assigner was.

2) This is caused by notify_bugtask_edited calling both
add_bug_change_notifications and _send_bug_details_to_new_subscribers for
task assignment changes.

== Implementation details ==

While determining how to prevent the assignee from receiving a notification
about the bug task assignee changing I discovered that
INotificationRecipientSet did not have a method for removing a recipient from
the recipient list and had to create one. Additionally, there were no tests
for testing the email notifications sent to bug assignees so I wrote them in
test_bug_task_assignment.py.

When I was reading all the code to understand how the notification system
worked I also fixed some typos.

lib/canonical/launchpad/doc/notification-recipient-set.txt
 * Created a test for removing recipients

lib/canonical/launchpad/interfaces/launchpad.py
 * documented remove method

lib/canonical/launchpad/mailnotification.py
 * added in event_creator tracking for who assigned the task
 * added in "You have been assigned" to email contents
 * added new_subscribers parament to add_bug_change_notifications

lib/lp/bugs/mail/tests/test_bug_task_assignment.py
 * new file with tests for changes

lib/lp/services/mail/notificationrecipientset.py
 * added in remove method

lib/lp/bugs/doc/bugnotification-email.txt
lib/lp/bugs/doc/bugnotification-sending.txt
lib/lp/bugs/doc/bugnotifications.txt
lib/lp/bugs/doc/bugtask.txt
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/tests/test_bugtask.py
 * Fixed some typos

== Tests ==

bin/test -vv -t test_bug_task_assignment -t notification-recipient-set

Test bug subscription email messages passes too:

bin/test -vv -t test_bug_subscribe

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/notification-recipient-set.txt'
2--- lib/canonical/launchpad/doc/notification-recipient-set.txt 2010-03-18 18:55:02 +0000
3+++ lib/canonical/launchpad/doc/notification-recipient-set.txt 2010-06-23 13:33:39 +0000
4@@ -202,7 +202,20 @@
5 >>> recipients.getReason(sample_person)
6 ('Notified for fun.', 'Fun')
7
8-== A person first impression stick ==
9+== Removing recipients ==
10+
11+It is also possible to remove a person from the NotificationRecipientSet():
12+
13+ >>> recipients = NotificationRecipientSet()
14+ >>> recipients.add(
15+ ... [sample_person, no_priv, cprov], 'Notified for fun.', 'Fun')
16+ >>> [person.displayname for person in recipients.getRecipients()]
17+ [u'Celso Providelo', u'No Privileges Person', u'Sample Person']
18+ >>> recipients.remove([sample_person, cprov])
19+ >>> [person.displayname for person in recipients.getRecipients()]
20+ [u'No Privileges Person']
21+
22+== A person's first impression sticks ==
23
24 In general, the most specific rationale is used for a given email.
25 A rationale given for a person is considered more
26
27=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
28--- lib/canonical/launchpad/interfaces/launchpad.py 2010-06-14 20:12:43 +0000
29+++ lib/canonical/launchpad/interfaces/launchpad.py 2010-06-23 13:33:39 +0000
30@@ -594,7 +594,7 @@
31 """
32
33 def add(person, reason, header):
34- """Add a person or sequence of person to the recipients list.
35+ """Add a person or a sequence of persons to the recipients list.
36
37 When the added person is a team without an email address, all its
38 members emails will be added. If the person is already in the
39@@ -608,6 +608,13 @@
40 X-Launchpad-Message-Rationale header.
41 """
42
43+ def remove(person):
44+ """Remove a person or a list of persons from the recipients list.
45+
46+ :param person: The `IPerson` or a sequence of `IPerson`
47+ that will removed from the recipients list.
48+ """
49+
50 def update(recipient_set):
51 """Updates this instance's reasons with reasons from another set.
52
53
54=== modified file 'lib/canonical/launchpad/mailnotification.py'
55--- lib/canonical/launchpad/mailnotification.py 2010-06-16 08:36:27 +0000
56+++ lib/canonical/launchpad/mailnotification.py 2010-06-23 13:33:39 +0000
57@@ -27,7 +27,8 @@
58 from canonical.config import config
59 from canonical.database.sqlbase import block_implicit_flushes
60 from lp.bugs.adapters.bugdelta import BugDelta
61-from lp.bugs.adapters.bugchange import BugDuplicateChange, get_bug_changes
62+from lp.bugs.adapters.bugchange import (
63+ BugDuplicateChange, get_bug_changes, BugTaskAssigneeChange)
64 from canonical.launchpad.helpers import (
65 get_contact_email_addresses, get_email_template, shortlist)
66 from canonical.launchpad.interfaces import (
67@@ -161,7 +162,8 @@
68
69
70 def _send_bug_details_to_new_bug_subscribers(
71- bug, previous_subscribers, current_subscribers, subscribed_by=None):
72+ bug, previous_subscribers, current_subscribers, subscribed_by=None,
73+ event_creator=None):
74 """Send an email containing full bug details to new bug subscribers.
75
76 This function is designed to handle situations where bugtasks get
77@@ -198,7 +200,7 @@
78 reason, rationale = recipients.getReason(to_addr)
79 subject, contents = generate_bug_add_email(
80 bug, new_recipients=True, subscribed_by=subscribed_by,
81- reason=reason)
82+ reason=reason, event_creator=event_creator)
83 msg = bug_notification_builder.build(
84 from_addr, to_addr, contents, subject, email_date,
85 rationale=rationale, references=references)
86@@ -339,9 +341,8 @@
87 template % {'url': file_alias_url, 'error_msg': message},
88 headers={'X-Launchpad-Unhandled-Email': message})
89
90-
91 def generate_bug_add_email(bug, new_recipients=False, reason=None,
92- subscribed_by=None):
93+ subscribed_by=None, event_creator=None):
94 """Generate a new bug notification from the given IBug.
95
96 If new_recipients is supplied we generate a notification explaining
97@@ -389,7 +390,14 @@
98 }
99
100 if new_recipients:
101- contents += "You have been subscribed to a %(visibility)s bug"
102+ if "assignee" in reason:
103+ contents += "You have been assigned a bug task for a %(visibility)s bug"
104+ if event_creator is not None:
105+ contents += " by %(assigner)s"
106+ content_substitutions['assigner'] = (
107+ event_creator.unique_displayname)
108+ else:
109+ contents += "You have been subscribed to a %(visibility)s bug"
110 if subscribed_by is not None:
111 contents += " by %(subscribed_by)s"
112 content_substitutions['subscribed_by'] = (
113@@ -578,7 +586,8 @@
114 key=operator.attrgetter('displayname'))
115
116
117-def add_bug_change_notifications(bug_delta, old_bugtask=None):
118+def add_bug_change_notifications(bug_delta, old_bugtask=None,
119+ new_subscribers=None):
120 """Generate bug notifications and add them to the bug."""
121 changes = get_bug_changes(bug_delta)
122 recipients = bug_delta.bug.getBugNotificationRecipients(
123@@ -603,6 +612,13 @@
124 include_master_dupe_subscribers=False))
125 bug_delta.bug.addChange(
126 change, recipients=no_dupe_master_recipients)
127+ elif (isinstance(change, BugTaskAssigneeChange) and
128+ new_subscribers is not None):
129+ for person in new_subscribers:
130+ reason, rationale = recipients.getReason(person)
131+ if 'Assignee' in rationale:
132+ recipients.remove(person)
133+ bug_delta.bug.addChange(change, recipients=recipients)
134 else:
135 bug_delta.bug.addChange(change, recipients=recipients)
136 else:
137@@ -625,13 +641,20 @@
138 bugtask_deltas=bugtask_delta,
139 user=IPerson(event.user))
140
141- add_bug_change_notifications(
142- bug_delta, old_bugtask=event.object_before_modification)
143-
144+ event_creator = IPerson(event.user)
145 previous_subscribers = event.object_before_modification.bug_subscribers
146 current_subscribers = event.object.bug_subscribers
147+ prev_subs_set = set(previous_subscribers)
148+ cur_subs_set = set(current_subscribers)
149+ new_subs = cur_subs_set.difference(prev_subs_set)
150+
151+ add_bug_change_notifications(
152+ bug_delta, old_bugtask=event.object_before_modification,
153+ new_subscribers=new_subs)
154+
155 _send_bug_details_to_new_bug_subscribers(
156- event.object.bug, previous_subscribers, current_subscribers)
157+ event.object.bug, previous_subscribers, current_subscribers,
158+ event_creator=event_creator)
159 update_security_contact_subscriptions(modified_bugtask, event)
160
161
162
163=== modified file 'lib/lp/bugs/doc/bugnotification-email.txt'
164--- lib/lp/bugs/doc/bugnotification-email.txt 2010-06-10 10:54:51 +0000
165+++ lib/lp/bugs/doc/bugnotification-email.txt 2010-06-23 13:33:39 +0000
166@@ -507,7 +507,7 @@
167 X-Launchpad-Bug-Commenters: name12
168
169 The build() method of a builder accepts a number of parameters and
170-returns an instance of email.MIMEText. The most basic invokation of
171+returns an instance of email.MIMEText. The most basic invocation of
172 this method requires a from address, a to address, a body, a subject
173 and a sending date for the mail.
174
175
176=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
177--- lib/lp/bugs/doc/bugnotification-sending.txt 2010-06-11 13:26:07 +0000
178+++ lib/lp/bugs/doc/bugnotification-sending.txt 2010-06-23 13:33:39 +0000
179@@ -153,7 +153,7 @@
180 ... flush_database_updates()
181
182
183-To every message that get sent out, [Bug $bugid] is prefixed to the
184+To every message that gets sent out, [Bug $bugid] is prefixed to the
185 subject. It gets prefixed only if it's not already present in the
186 subject, though, which is often the case when someone replies via email.
187
188@@ -435,7 +435,7 @@
189
190 >>> flush_notifications()
191
192-If Sample Person does a few changes, and Foo Bar steps in a does a
193+If Sample Person does a few changes, and Foo Bar steps in and does a
194 change in between, three notifications will be sent:
195
196 >>> from canonical.launchpad.interfaces import IPersonSet
197
198=== modified file 'lib/lp/bugs/doc/bugnotifications.txt'
199--- lib/lp/bugs/doc/bugnotifications.txt 2009-06-12 16:36:02 +0000
200+++ lib/lp/bugs/doc/bugnotifications.txt 2010-06-23 13:33:39 +0000
201@@ -17,7 +17,7 @@
202
203 >>> login("test@canonical.com")
204
205-Here are the things that cause bug notifications to be add, and what
206+Here are the things that cause bug notifications to be added and what
207 those notifications look like.
208
209
210
211=== modified file 'lib/lp/bugs/doc/bugtask.txt'
212--- lib/lp/bugs/doc/bugtask.txt 2010-05-25 16:45:26 +0000
213+++ lib/lp/bugs/doc/bugtask.txt 2010-06-23 13:33:39 +0000
214@@ -571,7 +571,7 @@
215 Traceback (most recent call last):
216 ...
217 UserCannotEditBugTaskAssignee: Regular users can assign and unassign
218- only themselves and their teams. Only project onwers, bug supervisors,
219+ only themselves and their teams. Only project owners, bug supervisors,
220 drivers and release managers can assign others.
221
222 >>> devel_focus_alsa_utils_task.transitionToAssignee(sample_person)
223@@ -592,7 +592,7 @@
224 Traceback (most recent call last):
225 ...
226 UserCannotEditBugTaskAssignee: Regular users can assign and unassign
227- only themselves and their teams. Only project onwers, bug supervisors,
228+ only themselves and their teams. Only project owners, bug supervisors,
229 drivers and release managers can assign others.
230
231 >>> login('test@canonical.com')
232
233=== added file 'lib/lp/bugs/mail/tests/test_bug_task_assignment.py'
234--- lib/lp/bugs/mail/tests/test_bug_task_assignment.py 1970-01-01 00:00:00 +0000
235+++ lib/lp/bugs/mail/tests/test_bug_task_assignment.py 2010-06-23 13:33:39 +0000
236@@ -0,0 +1,120 @@
237+# Copyright 2010 Canonical Ltd. This software is licensed under the
238+# GNU Affero General Public License version 3 (see the file LICENSE).
239+
240+"""Tests for Bug task assignment-related email tests."""
241+
242+from unittest import TestLoader
243+
244+import transaction
245+
246+from zope.component import getUtility
247+from zope.interface import providedBy
248+from zope.event import notify
249+
250+from canonical.testing import DatabaseFunctionalLayer
251+from canonical.launchpad.database import BugNotification
252+from canonical.launchpad.webapp.interfaces import ILaunchBag
253+
254+from lp.services.mail import stub
255+from lp.bugs.scripts.bugnotification import construct_email_notifications
256+from lp.testing import TestCaseWithFactory
257+
258+from lazr.lifecycle.event import (ObjectModifiedEvent)
259+from lazr.lifecycle.snapshot import Snapshot
260+
261+
262+class TestAssignmentNotification(TestCaseWithFactory):
263+ """Test emails sent when assigned a bug report."""
264+
265+ layer = DatabaseFunctionalLayer
266+
267+ def setUp(self):
268+ # Run the tests as a logged-in user.
269+ super(TestAssignmentNotification, self).setUp(
270+ user='test@canonical.com')
271+ self.user = getUtility(ILaunchBag).user
272+ self.product = self.factory.makeProduct(owner=self.user,
273+ name='rebirth')
274+ self.bug = self.factory.makeBug(product=self.product)
275+ self.bug_task = self.bug.getBugTask(self.product)
276+ self.bug_task_before_modification = Snapshot(self.bug_task,
277+ providing=providedBy(self.bug_task))
278+ self.person_assigned_email = 'stever@example.com'
279+ self.person_assigned = self.factory.makePerson(
280+ name='assigned', displayname='Steve Rogers',
281+ email=self.person_assigned_email)
282+ self.team_member_email = 'hankp@example.com'
283+ self.team_member = self.factory.makePerson(
284+ name='giantman', displayname='Hank Pym',
285+ email=self.team_member_email)
286+ self.team_assigned = self.factory.makeTeam(
287+ name='avengers', owner=self.user)
288+ self.team_assigned.addMember(self.team_member, self.user)
289+ # adding people to a team generates email
290+ transaction.commit()
291+ del stub.test_emails[:]
292+
293+ def test_assignee_notification_message(self):
294+ """Test notification string when a person is assigned a task by
295+ someone else."""
296+ self.assertEqual(len(stub.test_emails), 0, 'emails in queue')
297+ self.bug_task.transitionToAssignee(self.person_assigned)
298+ notify(ObjectModifiedEvent(
299+ self.bug_task, self.bug_task_before_modification,
300+ ['assignee'], user=self.user))
301+ transaction.commit()
302+ self.assertEqual(len(stub.test_emails), 1, 'email not sent')
303+ rationale = 'You have been assigned a bug task for a public bug by Sample Person'
304+ msg = stub.test_emails[-1][2]
305+ self.assertTrue(rationale in msg,
306+ '%s not in\n%s\n' % (rationale, msg))
307+
308+ def test_assignee_not_a_subscriber(self):
309+ """Test that a new recipient being assigned a bug task does send
310+ a NEW message."""
311+ self.assertEqual(len(stub.test_emails), 0, 'emails in queue')
312+ self.bug_task.transitionToAssignee(self.person_assigned)
313+ notify(ObjectModifiedEvent(
314+ self.bug_task, self.bug_task_before_modification,
315+ ['assignee'], user=self.user))
316+ transaction.commit()
317+ self.assertEqual(len(stub.test_emails), 1, 'email not sent')
318+ new_message = '[NEW]'
319+ msg = stub.test_emails[-1][2]
320+ self.assertTrue(new_message in msg,
321+ '%s not in \n%s\n' % (new_message, msg))
322+
323+ def test_assignee_new_subscriber(self):
324+ """Build a list of people who will receive e-mails about the bug
325+ task changes and ensure the assignee is not one."""
326+ self.bug_task.transitionToAssignee(self.person_assigned)
327+ notify(ObjectModifiedEvent(
328+ self.bug_task, self.bug_task_before_modification,
329+ ['assignee'], user=self.user))
330+ latest_notification = BugNotification.selectFirst(orderBy='-id')
331+ notifications, messages = construct_email_notifications([latest_notification])
332+ self.assertEqual(len(notifications), 1,
333+ 'email notication not created')
334+ receivers = [message['To'] for message in messages]
335+ self.assertFalse(self.person_assigned_email in receivers,
336+ 'Assignee was emailed about the bug task change')
337+
338+ def test_team_assigned_new_subscriber(self):
339+ """Assign a team, who is not subscribed to a bug, a bug task and
340+ ensure that team members do not receive an e-mail about the bug
341+ task changes."""
342+ self.bug_task.transitionToAssignee(self.team_assigned)
343+ notify(ObjectModifiedEvent(
344+ self.bug_task, self.bug_task_before_modification,
345+ ['assignee'], user=self.user))
346+ latest_notification = BugNotification.selectFirst(orderBy='-id')
347+ notifications, messages = construct_email_notifications([latest_notification])
348+ self.assertEqual(len(notifications), 1,
349+ 'email notification not created')
350+ receivers = [message['To'] for message in messages]
351+ self.assertFalse(self.team_member_email in receivers,
352+ 'Team member was emailed about the bug task change')
353+
354+
355+def test_suite():
356+ return TestLoader().loadTestsFromName(__name__)
357
358=== modified file 'lib/lp/bugs/model/bugtask.py'
359--- lib/lp/bugs/model/bugtask.py 2010-06-16 16:46:06 +0000
360+++ lib/lp/bugs/model/bugtask.py 2010-06-23 13:33:39 +0000
361@@ -992,7 +992,7 @@
362 if not self.canTransitionToAssignee(assignee):
363 raise UserCannotEditBugTaskAssignee(
364 'Regular users can assign and unassign only themselves and '
365- 'their teams. Only project onwers, bug supervisors, drivers '
366+ 'their teams. Only project owners, bug supervisors, drivers '
367 'and release managers can assign others.')
368
369 now = datetime.datetime.now(pytz.UTC)
370
371=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
372--- lib/lp/bugs/tests/test_bugchanges.py 2010-06-09 09:03:51 +0000
373+++ lib/lp/bugs/tests/test_bugchanges.py 2010-06-23 13:33:39 +0000
374@@ -1044,8 +1044,8 @@
375 expected_notification=expected_notification)
376
377 def test_unassign_bugtask(self):
378- # Unassigning a bug task to someone adds entries to the bug
379- # activity and notifications sets.
380+ # Unassigning a bug task assigned to someone adds entries to the
381+ # bug activity and notifications sets.
382 old_assignee = self.factory.makePerson()
383 self.bug_task.transitionToAssignee(old_assignee)
384 self.saveOldChanges()
385
386=== modified file 'lib/lp/bugs/tests/test_bugtask.py'
387--- lib/lp/bugs/tests/test_bugtask.py 2010-05-27 13:51:06 +0000
388+++ lib/lp/bugs/tests/test_bugtask.py 2010-06-23 13:33:39 +0000
389@@ -506,7 +506,7 @@
390 layer = LaunchpadFunctionalLayer
391
392 def setUp(self):
393- """Crate the test setup.
394+ """Create the test setup.
395
396 We need
397 - bug task targets (a product and a product series, or
398
399=== modified file 'lib/lp/services/mail/notificationrecipientset.py'
400--- lib/lp/services/mail/notificationrecipientset.py 2010-02-16 20:36:48 +0000
401+++ lib/lp/services/mail/notificationrecipientset.py 2010-06-23 13:33:39 +0000
402@@ -91,7 +91,7 @@
403
404 for person in persons:
405 assert IPerson.providedBy(person), (
406- 'You can only add() IPerson: %r' % person)
407+ 'You can only add() an IPerson: %r' % person)
408 # If the person already has a rationale, keep the first one.
409 if person in self._personToRationale:
410 continue
411@@ -111,6 +111,24 @@
412 or (old_person.isTeam() and not person.isTeam())):
413 self._emailToPerson[email] = person
414
415+ def remove(self, persons):
416+ """See `INotificationRecipientSet`."""
417+ from zope.security.proxy import removeSecurityProxy
418+ if IPerson.providedBy(persons):
419+ persons = [persons]
420+ for person in persons:
421+ assert IPerson.providedBy(person), (
422+ 'You can only remove() an IPerson: %r' % person)
423+ if person in self._personToRationale:
424+ del self._personToRationale[person]
425+ for removed_person in emailPeople(person):
426+ # Bypass zope's security because IEmailAddress.email is
427+ # not public.
428+ preferred_email = removeSecurityProxy(
429+ removed_person.preferredemail)
430+ email = str(preferred_email.email)
431+ self._receiving_people.discard((email, removed_person))
432+
433 def update(self, recipient_set):
434 """See `INotificationRecipientSet`."""
435 for person in recipient_set: