launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02622
[Merge] lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel
Gary Poster has proposed merging lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel with lp:~gary/launchpad/bug164196-1 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#164196 Quickly-undone actions shouldn't send mail notifications
https://bugs.launchpad.net/bugs/164196
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug164196-2/+merge/49977
This is the second of three branches that address bug 164196. The other two are lp:~gary/launchpad/bug164196-1 and lp:~gary/launchpad/bug164196-3. My pre-implementation call for these changes was with Graham Binns.
This branch uses the activity attributes on bug notifications introduced in the first branch to implement the logic to silence notifications for actions that are done and then undone in rapid succession (that is, to do what the bug requests). Undone actions are omitted from emails, and if the emails then have no content, they are not sent.
The only thing left to do is to make sure that the omitted notification objects are marked as processed so that they are not perpetually considered for subsequent notification cronscript runs. This is tackled in the last branch of the series.
The implementation is all in lib/lp/bugs/scripts/bugnotification.py. Within construct_email_notifications, there were already two loops through the notification objects, so I rearranged them so I could hook into one of them to determine what could be omitted. The changes are a bit smaller than what a quick scan of the diff suggests, because I reordered the two loops.
`get_key` probably holds the subtlest code here: we normalize the bug activity's data to give us a key that will work for items that are linked/added and unlinked/removed. For instance, if a change adds one branch but then removes another, both of these should be reported. We can only squelch the notification if the same branch is added and removed. A less subtle but important part of get_key is that it normalizes the "duplicate" names. Unlike other changes, the activity text changes depending on what happens, so we need to normalize that here.
I renamed the local variable "person_causing_change" to "actor" as an easy solution to some long lines.
The bulk of the branch is the tests for these relatively small changes. I changed the doctest enough for it to pass (and mention the behavior in passing), and updated some test infrastructure in lib/lp/bugs/scripts/tests/test_bugnotification.py (by adding the "activity" attribute to the mock bug notifications), but the main changes are brand new tests at the bottom of lib/lp/bugs/scripts/tests/test_bugnotification.py . I don't test all change objects, just the ones that were unusual. In this case, it means I omitted tests for non-collection bug attributes other than title (representative) and duplicate (exceptional), expecting the rest to work out fine; and I omitted tests for non-collection bugtask attributes other than status. I can add them relatively easily--with fun test subclasses--if you like.
Speaking of subclasses, I don't love what I've done to assemble these tests from subclasses and mixins, but it certainly has precedence in our codebase, and the alternative, with lots of repetition, is even less attractive.
I removed the "def test_suite():" boilerplate because, AIUI, we have now 'fessed up to the fact that it didn't accomplish anything practical, and are actively encouraging removal.
Thank you
--
https://code.launchpad.net/~gary/launchpad/bug164196-2/+merge/49977
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel.
=== modified file 'lib/lp/bugs/adapters/bugchange.py'
--- lib/lp/bugs/adapters/bugchange.py 2011-02-04 01:15:13 +0000
+++ lib/lp/bugs/adapters/bugchange.py 2011-02-16 14:47:00 +0000
@@ -7,6 +7,7 @@
__all__ = [
'BranchLinkedToBug',
'BranchUnlinkedFromBug',
+ 'BugAttachmentChange',
'BugConvertedToQuestion',
'BugDescriptionChange',
'BugDuplicateChange',
@@ -40,7 +41,6 @@
from lp.bugs.enum import BugNotificationLevel
from lp.bugs.interfaces.bugchange import IBugChange
from lp.bugs.interfaces.bugtask import (
- BugTaskStatus,
IBugTask,
RESOLVED_BUGTASK_STATUSES,
UNRESOLVED_BUGTASK_STATUSES,
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-08 14:34:55 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-16 14:47:00 +0000
@@ -296,14 +296,10 @@
- Old summary
+ New summary
<BLANKLINE>
- ** Visibility changed to: Private
- <BLANKLINE>
** Summary changed:
- New summary
+ Another summary
<BLANKLINE>
- ** Visibility changed to: Public
- <BLANKLINE>
--
You received this bug notification because you are a member of Ubuntu
Team, which is the registrant for Ubuntu.
@@ -315,6 +311,29 @@
To: test@xxxxxxxxxxxxx
...
+If you look carefully, there's a surprise in that output: the visibility
+changes are not reported. This is because they are done and then undone
+within the same notification. Undone changes like that are omitted.
+moreover, if the email only would have reported done/undone changes, it
+is not sent at all. This is tested elsewhere (see
+lp/bugs/tests/test_bugnotification.py), and not demonstrated here.
+
+Another thing worth noting is that there's a blank line before the
+signature, and the signature marker has a trailing space.
+
+ >>> message.get_payload(decode=True).splitlines()
+ [...,
+ '',
+ '-- ',
+ 'You received this bug notification because you are a direct subscriber',
+ 'of the bug.',
+ 'http://bugs.launchpad.dev/bugs/1',
+ '',
+ 'Title:',
+ ' Firefox does not support SVG']
+
+ >>> flush_notifications()
+
We send the notification only if the user hasn't done any other changes
for the last 5 minutes:
@@ -331,22 +350,6 @@
>>> flush_notifications()
-There's a blank line before the signature, and the signature marker has
-a trailing space.
-
- >>> message.get_payload(decode=True).splitlines()
- [...,
- '',
- '-- ',
- 'You received this bug notification because you are a direct subscriber',
- 'of the bug.',
- 'http://bugs.launchpad.dev/bugs/1',
- '',
- 'Title:',
- ' Firefox does not support SVG']
-
- >>> flush_notifications()
-
If a team without a contact address is subscribed to the bug, the
notification will be sent to all members individually.
@@ -382,7 +385,6 @@
>>> flush_notifications()
-
Duplicates
----------
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-02-04 16:23:08 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-02-16 14:47:00 +0000
@@ -31,6 +31,25 @@
from lp.services.mail.mailwrapper import MailWrapper
+def get_key(notification):
+ activity = notification.activity
+ if activity is not None:
+ key = activity.whatchanged
+ if key in ('changed duplicate marker',
+ 'marked as duplicate',
+ 'removed duplicate marker'):
+ key = 'duplicate'
+ elif key.endswith(' added'):
+ key = key[:-len('added')] + activity.newvalue
+ elif key.endswith(' removed'):
+ key = key[:-len('removed')] + activity.oldvalue
+ elif key.endswith(' linked'):
+ key = key[:-len('linked')] + activity.newvalue
+ elif key.endswith(' unlinked'):
+ key = key[:-len('unlinked')] + activity.oldvalue
+ return key
+
+
def construct_email_notifications(bug_notifications):
"""Construct an email from a list of related bug notifications.
@@ -39,31 +58,45 @@
"""
first_notification = bug_notifications[0]
bug = first_notification.bug
- person_causing_change = first_notification.message.owner
+ actor = first_notification.message.owner
subject = first_notification.message.subject
comment = None
references = []
text_notifications = []
-
- recipients = {}
- for notification in bug_notifications:
- for recipient in notification.recipients:
- email_people = emailPeople(recipient.person)
- if (not person_causing_change.selfgenerated_bugnotifications and
- person_causing_change in email_people):
- email_people.remove(person_causing_change)
- for email_person in email_people:
- recipients[email_person] = recipient
+ old_values = {}
+ new_values = {}
for notification in bug_notifications:
assert notification.bug == bug, bug.id
- assert notification.message.owner == person_causing_change, (
- person_causing_change.id)
+ assert notification.message.owner == actor, actor.id
if notification.is_comment:
assert comment is None, (
"Only one of the notifications is allowed to be a comment.")
comment = notification.message
+ else:
+ key = get_key(notification)
+ if key is not None:
+ if key not in old_values:
+ old_values[key] = notification.activity.oldvalue
+ new_values[key] = notification.activity.newvalue
+
+ recipients = {}
+ filtered_notifications = []
+ for notification in bug_notifications:
+ key = get_key(notification)
+ if (notification.is_comment or
+ key is None or
+ old_values[key] != new_values[key]):
+ # We will report this notification.
+ filtered_notifications.append(notification)
+ for recipient in notification.recipients:
+ email_people = emailPeople(recipient.person)
+ if (not actor.selfgenerated_bugnotifications and
+ actor in email_people):
+ email_people.remove(actor)
+ for email_person in email_people:
+ recipients[email_person] = recipient
if bug.duplicateof is not None:
text_notifications.append(
@@ -88,7 +121,7 @@
msgid = first_notification.message.rfc822msgid
email_date = first_notification.message.datecreated
- for notification in bug_notifications:
+ for notification in filtered_notifications:
if notification.message == comment:
# Comments were just handled in the previous if block.
continue
@@ -104,9 +137,8 @@
messages = []
mail_wrapper = MailWrapper(width=72)
content = '\n\n'.join(text_notifications)
- from_address = get_bugmail_from_address(person_causing_change, bug)
- bug_notification_builder = BugNotificationBuilder(
- bug, person_causing_change)
+ from_address = get_bugmail_from_address(actor, bug)
+ bug_notification_builder = BugNotificationBuilder(bug, actor)
sorted_recipients = sorted(
recipients.items(), key=lambda t: t[0].preferredemail.email)
for email_person, recipient in sorted_recipients:
@@ -157,7 +189,7 @@
rationale, references, msgid)
messages.append(msg)
- return bug_notifications, messages
+ return filtered_notifications, messages
def notification_comment_batches(notifications):
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2010-10-06 18:53:53 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2011-02-16 14:47:00 +0000
@@ -4,24 +4,43 @@
__metaclass__ = type
-from datetime import datetime
+from datetime import datetime, timedelta
import unittest
import pytz
+from testtools.matchers import Not
+from transaction import commit
from zope.component import getUtility
from zope.interface import implements
from canonical.config import config
-from canonical.database.sqlbase import commit
-from lp.bugs.model.bugtask import BugTask
+from canonical.database.sqlbase import flush_database_updates
+from canonical.launchpad.ftests import login
from canonical.launchpad.helpers import get_contact_email_addresses
from canonical.launchpad.interfaces.message import IMessageSet
from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.bugs.adapters.bugchange import (
+ BranchLinkedToBug,
+ BranchUnlinkedFromBug,
+ BugAttachmentChange,
+ BugDuplicateChange,
+ BugTagsChange,
+ BugTaskStatusChange,
+ BugTitleChange,
+ BugVisibilityChange,
+ BugWatchAdded,
+ BugWatchRemoved,
+ CveLinkedToBug,
+ CveUnlinkedFromBug,
+ )
from lp.bugs.interfaces.bug import (
IBug,
IBugSet,
)
+from lp.bugs.interfaces.bugnotification import IBugNotificationSet
+from lp.bugs.interfaces.bugtask import BugTaskStatus
from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
+from lp.bugs.model.bugtask import BugTask
from lp.bugs.scripts.bugnotification import (
get_email_notifications,
notification_batches,
@@ -29,6 +48,10 @@
)
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProductSet
+from lp.services.propertycache import cachedproperty
+from lp.testing import TestCaseWithFactory
+from lp.testing.dbuser import lp_dbuser
+from lp.testing.matchers import Contains
class MockBug:
@@ -105,6 +128,7 @@
self.is_comment = is_comment
self.date_emailed = date_emailed
self.recipients = [MockBugNotificationRecipient()]
+ self.activity = None
class TestGetEmailNotifications(unittest.TestCase):
@@ -251,6 +275,7 @@
self.bug = bug
self.message = self.Message()
self.message.owner = owner
+ self.activity = None
class TestNotificationCommentBatches(unittest.TestCase):
@@ -458,5 +483,337 @@
self.assertEquals(expected, observed)
-def test_suite():
- return unittest.TestLoader().loadTestsFromName(__name__)
+class EmailNotificationTestBase(TestCaseWithFactory):
+
+ layer = LaunchpadZopelessLayer
+
+ def setUp(self):
+ super(EmailNotificationTestBase, self).setUp()
+ login('foo.bar@xxxxxxxxxxxxx')
+ self.product_owner = self.factory.makePerson(name="product-owner")
+ self.person = self.factory.makePerson(name="sample-person")
+ self.product = self.factory.makeProduct(owner=self.product_owner)
+ self.product_subscriber = self.factory.makePerson(
+ name="product-subscriber")
+ self.product.addBugSubscription(
+ self.product_subscriber, self.product_subscriber)
+ self.bug_subscriber = self.factory.makePerson(name="bug-subscriber")
+ self.bug_owner = self.factory.makePerson(name="bug-owner")
+ self.bug = self.factory.makeBug(
+ product=self.product, private=False, owner=self.bug_owner)
+ self.reporter = self.bug.owner
+ self.bug.subscribe(self.bug_subscriber, self.reporter)
+ [self.product_bugtask] = self.bug.bugtasks
+ commit()
+ login('test@xxxxxxxxxxxxx')
+ self.layer.switchDbUser(config.malone.bugnotification_dbuser)
+ self.now = datetime.now(pytz.timezone('UTC'))
+ self.ten_minutes_ago = self.now - timedelta(minutes=10)
+ self.notification_set = getUtility(IBugNotificationSet)
+ for notification in self.notification_set.getNotificationsToSend():
+ notification.date_emailed = self.now
+ flush_database_updates()
+
+ def tearDown(self):
+ for notification in self.notification_set.getNotificationsToSend():
+ notification.date_emailed = self.now
+ flush_database_updates()
+ super(EmailNotificationTestBase, self).tearDown()
+
+ def get_messages(self):
+ notifications = self.notification_set.getNotificationsToSend()
+ email_notifications = get_email_notifications(notifications)
+ for bug_notifications, messages in email_notifications:
+ for message in messages:
+ yield message, message.get_payload(decode=True)
+
+
+class EmailNotificationsBugMixin:
+
+ change_class = change_name = old = new = alt = unexpected_text = None
+
+ def change(self, old, new):
+ self.bug.addChange(
+ self.change_class(
+ self.ten_minutes_ago, self.person, self.change_name,
+ old, new))
+
+ def change_other(self):
+ self.bug.addChange(
+ BugVisibilityChange(
+ self.ten_minutes_ago, self.person, "private",
+ False, True))
+
+ def test_change_seen(self):
+ # A smoketest.
+ self.change(self.old, self.new)
+ message, body = self.get_messages().next()
+ self.assertThat(body, Contains(self.unexpected_text))
+
+ def test_undone_change_sends_no_emails(self):
+ self.change(self.old, self.new)
+ self.change(self.new, self.old)
+ self.assertEqual(list(self.get_messages()), [])
+
+ def test_undone_change_is_not_included(self):
+ self.change(self.old, self.new)
+ self.change(self.new, self.old)
+ self.change_other()
+ message, body = self.get_messages().next()
+ self.assertThat(body, Not(Contains(self.unexpected_text)))
+
+ def test_multiple_undone_changes_sends_no_emails(self):
+ self.change(self.old, self.new)
+ self.change(self.new, self.alt)
+ self.change(self.alt, self.old)
+ self.assertEqual(list(self.get_messages()), [])
+
+
+class EmailNotificationsBugNotRequiredMixin(EmailNotificationsBugMixin):
+ # This test collection is for attributes that can be None.
+ def test_added_removed_sends_no_emails(self):
+ self.change(None, self.old)
+ self.change(self.old, None)
+ self.assertEqual(list(self.get_messages()), [])
+
+ def test_removed_added_sends_no_emails(self):
+ self.change(self.old, None)
+ self.change(None, self.old)
+ self.assertEqual(list(self.get_messages()), [])
+
+ def test_duplicate_marked_changed_removed_sends_no_emails(self):
+ self.change(None, self.old)
+ self.change(self.old, self.new)
+ self.change(self.new, None)
+ self.assertEqual(list(self.get_messages()), [])
+
+
+class EmailNotificationsBugTaskMixin(EmailNotificationsBugMixin):
+
+ def change(self, old, new, index=0):
+ self.bug.addChange(
+ self.change_class(
+ self.bug.bugtasks[index], self.ten_minutes_ago,
+ self.person, self.change_name, old, new))
+
+ def test_changing_on_different_bugtasks_is_not_undoing(self):
+ with lp_dbuser():
+ product2 = self.factory.makeProduct(owner=self.product_owner)
+ self.bug.addTask(self.product_owner, product2)
+ self.change(self.old, self.new, index=0)
+ self.change(self.new, self.old, index=1)
+ message, body = self.get_messages().next()
+ self.assertThat(body, Contains(self.unexpected_text))
+
+
+class EmailNotificationsAddedRemovedMixin:
+
+ old = new = added_message = removed_message = None
+
+ def add(self, item):
+ raise NotImplementedError
+ remove = add
+
+ def test_added_seen(self):
+ self.add(self.old)
+ message, body = self.get_messages().next()
+ self.assertThat(body, Contains(self.added_message))
+
+ def test_added_removed_sends_no_emails(self):
+ self.add(self.old)
+ self.remove(self.old)
+ self.assertEqual(list(self.get_messages()), [])
+
+ def test_removed_added_sends_no_emails(self):
+ self.remove(self.old)
+ self.add(self.old)
+ self.assertEqual(list(self.get_messages()), [])
+
+ def test_added_another_removed_sends_emails(self):
+ self.add(self.old)
+ self.remove(self.new)
+ message, body = self.get_messages().next()
+ self.assertThat(body, Contains(self.added_message))
+ self.assertThat(body, Contains(self.removed_message))
+
+
+class TestEmailNotificationsBugTitle(
+ EmailNotificationsBugMixin, EmailNotificationTestBase):
+
+ change_class = BugTitleChange
+ change_name = "title"
+ old = "Old summary"
+ new = "New summary"
+ alt = "Another summary"
+ unexpected_text = '** Summary changed:'
+
+
+class TestEmailNotificationsBugTags(
+ EmailNotificationsBugMixin, EmailNotificationTestBase):
+
+ change_class = BugTagsChange
+ change_name = "tags"
+ old = ['foo', 'bar', 'baz']
+ new = ['foo', 'bar']
+ alt = ['bing', 'shazam']
+ unexpected_text = '** Tags'
+
+ def test_undone_ordered_set_sends_no_email(self):
+ # Tags use ordered sets to generate change descriptions, which we
+ # demonstrate here.
+ self.change(['foo', 'bar', 'baz'], ['foo', 'bar'])
+ self.change(['foo', 'bar'], ['baz', 'bar', 'foo', 'bar'])
+ self.assertEqual(list(self.get_messages()), [])
+
+
+class TestEmailNotificationsBugDuplicate(
+ EmailNotificationsBugNotRequiredMixin, EmailNotificationTestBase):
+
+ change_class = BugDuplicateChange
+ change_name = "duplicateof"
+ unexpected_text = 'duplicate'
+
+ def _bug(self):
+ with lp_dbuser():
+ return self.factory.makeBug()
+
+ old = cachedproperty('old')(_bug)
+ new = cachedproperty('new')(_bug)
+ alt = cachedproperty('alt')(_bug)
+
+
+class TestEmailNotificationsBugTaskStatus(
+ EmailNotificationsBugTaskMixin, EmailNotificationTestBase):
+
+ change_class = BugTaskStatusChange
+ change_name = "status"
+ old = BugTaskStatus.TRIAGED
+ new = BugTaskStatus.INPROGRESS
+ alt = BugTaskStatus.INVALID
+ unexpected_text = 'Status: '
+
+
+class TestEmailNotificationsBugWatch(
+ EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase):
+
+ # Note that this is for bugwatches added to bugs. Bugwatches added
+ # to bugtasks are separate animals AIUI, and we don't try to combine
+ # them here for notifications. Bugtasks have only zero or one
+ # bugwatch, so they can be handled just as a simple bugtask attribute
+ # change, like status.
+
+ added_message = '** Bug watch added:'
+ removed_message = '** Bug watch removed:'
+
+ @cachedproperty
+ def tracker(self):
+ with lp_dbuser():
+ return self.factory.makeBugTracker()
+
+ def _watch(self, identifier='123'):
+ with lp_dbuser():
+ # This actually creates a notification all by itself. However,
+ # it won't be sent out for another five minutes. Therefore,
+ # we send out separate change notifications.
+ return self.bug.addWatch(
+ self.tracker, identifier, self.product_owner)
+
+ old = cachedproperty('old')(_watch)
+ new = cachedproperty('new')(lambda self: self._watch('456'))
+
+ def add(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BugWatchAdded(
+ self.ten_minutes_ago, self.product_owner, item))
+
+ def remove(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BugWatchRemoved(
+ self.ten_minutes_ago, self.product_owner, item))
+
+
+class TestEmailNotificationsBranch(
+ EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase):
+
+ added_message = '** Branch linked:'
+ removed_message = '** Branch unlinked:'
+
+ def _branch(self):
+ with lp_dbuser():
+ return self.factory.makeBranch()
+
+ old = cachedproperty('old')(_branch)
+ new = cachedproperty('new')(_branch)
+
+ def add(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BranchLinkedToBug(
+ self.ten_minutes_ago, self.person, item, self.bug))
+
+ def remove(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BranchUnlinkedFromBug(
+ self.ten_minutes_ago, self.person, item, self.bug))
+
+
+class TestEmailNotificationsCVE(
+ EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase):
+
+ added_message = '** CVE added:'
+ removed_message = '** CVE removed:'
+
+ def _cve(self, sequence):
+ with lp_dbuser():
+ return self.factory.makeCVE(sequence)
+
+ old = cachedproperty('old')(lambda self: self._cve('2020-1234'))
+ new = cachedproperty('new')(lambda self: self._cve('2020-5678'))
+
+ def add(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ CveLinkedToBug(
+ self.ten_minutes_ago, self.person, item))
+
+ def remove(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ CveUnlinkedFromBug(
+ self.ten_minutes_ago, self.person, item))
+
+
+class TestEmailNotificationsAttachments(
+ EmailNotificationsAddedRemovedMixin, EmailNotificationTestBase):
+
+ added_message = '** Attachment added:'
+ removed_message = '** Attachment removed:'
+
+ def _attachment(self):
+ with lp_dbuser():
+ # This actually creates a notification all by itself, via an
+ # event subscriber. However, it won't be sent out for
+ # another five minutes. Therefore, we send out separate
+ # change notifications.
+ return self.bug.addAttachment(
+ self.person, 'content', 'a comment', 'stuff.txt')
+
+ old = cachedproperty('old')(_attachment)
+ new = cachedproperty('new')(_attachment)
+
+ def add(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BugAttachmentChange(
+ self.ten_minutes_ago, self.person, 'attachment',
+ None, item))
+
+ def remove(self, item):
+ with lp_dbuser():
+ self.bug.addChange(
+ BugAttachmentChange(
+ self.ten_minutes_ago, self.person, 'attachment',
+ item, None))
Follow ups
