Merge lp:~gary/launchpad/bug164196-2 into lp:launchpad/db-devel

[Gary Poster]

use the activity attributes on bug notifications to implement the logic to silence emails for actions that are done and then undone in rapid succession.

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

[Данило Шеган (danilo)]

Conclusions from IRC.

To increase trust in what's going on in parsing whatchanged, we should:
 1) add docstring and unittest for get_key
 2) move duplicateof code to target computed attribute code, and add test
 3) change get_key to use target/attribute

As for the rest, I really like the test coverage: great job there!

And a side note for s/person_causing_change/actor/: wordy name was introduced instead of 'person' to help make code easier to understand/read. It's up to you to decide which you want to keep as long as you take code readability into account :)

Considering we have agreed on a few changes, I am marking this as 'approved', though I'd be happy to take a look at incremental diff if it's a significant change.

review: Approve

[Gary Poster (gary)]

I'll wait for Данило's review of my changes, because they ended up being fairly extensive.

[Данило Шеган (danilo)]

Gary, thanks for the improvements.

This looks much, much, nicer — sorry for putting you through the
trouble, but I think it was worth it. :)

  review approve
  merge approve

Just one minor tidbit below:

>=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
...
>+
>+class MockBugActivity:
>+ """A mock BugActivity user for testing."""

A typo most likely: s/user/used/.

Cheers,
Danilo

review: Approve

[Robert Collins (lifeless)]

BTW, there are mocks for bugactivity etc in the bugcomment tests
today; may want to snarf those and reused (if you haven't already, I
haven't read your diff).

Cheers,
Rob

=== 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-17 17:46:51 +0000
@@ -5,8 +5,20 @@
 
 __metaclass__ = type
 __all__ = [
+    'ATTACHMENT_ADDED',
+    'ATTACHMENT_REMOVED',
+    'BRANCH_LINKED',
+    'BRANCH_UNLINKED',
+    'BUG_WATCH_ADDED',
+    'BUG_WATCH_REMOVED',
+    'CHANGED_DUPLICATE_MARKER',
+    'CVE_LINKED',
+    'CVE_UNLINKED',
+    'MARKED_AS_DUPLICATE',
+    'REMOVED_DUPLICATE_MARKER',
     'BranchLinkedToBug',
     'BranchUnlinkedFromBug',
+    'BugAttachmentChange',
     'BugConvertedToQuestion',
     'BugDescriptionChange',
     'BugDuplicateChange',
@@ -40,13 +52,27 @@
 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,
     )
 from lp.registry.interfaces.product import IProduct
 
+# These are used lp.bugs.model.bugactivity.BugActivity.attribute to normalize
+# the output from these change objects into the attribute that actually
+# changed.  It is fragile, but a reasonable incremental step.
+ATTACHMENT_ADDED = "attachment added"
+ATTACHMENT_REMOVED = "attachment removed"
+BRANCH_LINKED = 'branch linked'
+BRANCH_UNLINKED = 'branch unlinked'
+BUG_WATCH_ADDED = 'bug watch added'
+BUG_WATCH_REMOVED = 'bug watch removed'
+CHANGED_DUPLICATE_MARKER = 'changed duplicate marker'
+CVE_LINKED = 'cve linked'
+CVE_UNLINKED = 'cve unlinked'
+MARKED_AS_DUPLICATE = 'marked as duplicate'
+REMOVED_DUPLICATE_MARKER = 'removed duplicate marker'
+
 
 class NoBugChangeFoundError(Exception):
     """Raised when a BugChange class can't be found for an object."""
@@ -254,7 +280,7 @@
     def getBugActivity(self):
         """See `IBugChange`."""
         return dict(
-            whatchanged='bug watch added',
+            whatchanged=BUG_WATCH_ADDED,
             newvalue=self.bug_watch.url)
 
     def getBugNotification(self):
@@ -278,7 +304,7 @@
     def getBugActivity(self):
         """See `IBugChange`."""
         return dict(
-            whatchanged='bug watch removed',
+            whatchanged=BUG_WATCH_REMOVED,
             oldvalue=self.bug_watch.url)
 
     def getBugNotification(self):
@@ -305,7 +331,7 @@
         if self.branch.private:
             return None
         return dict(
-            whatchanged='branch linked',
+            whatchanged=BRANCH_LINKED,
             newvalue=self.branch.bzr_identity)
 
     def getBugNotification(self):
@@ -328,7 +354,7 @@
         if self.branch.private:
             return None
         return dict(
-            whatchanged='branch unlinked',
+            whatchanged=BRANCH_UNLINKED,
             oldvalue=self.branch.bzr_identity)
 
     def getBugNotification(self):
@@ -397,18 +423,18 @@
     def getBugActivity(self):
         if self.old_value is not None and self.new_value is not None:
             return {
-                'whatchanged': 'changed duplicate marker',
+                'whatchanged': CHANGED_DUPLICATE_MARKER,
                 'oldvalue': str(self.old_value.id),
                 'newvalue': str(self.new_value.id),
                 }
         elif self.old_value is None:
             return {
-                'whatchanged': 'marked as duplicate',
+                'whatchanged': MARKED_AS_DUPLICATE,
                 'newvalue': str(self.new_value.id),
                 }
         elif self.new_value is None:
             return {
-                'whatchanged': 'removed duplicate marker',
+                'whatchanged': REMOVED_DUPLICATE_MARKER,
                 'oldvalue': str(self.old_value.id),
                 }
         else:
@@ -605,13 +631,13 @@
 
     def getBugActivity(self):
         if self.old_value is None:
-            what_changed = "attachment added"
+            what_changed = ATTACHMENT_ADDED
             old_value = None
             new_value = "%s %s" % (
                 self.new_value.title,
                 download_url_of_bugattachment(self.new_value))
         else:
-            what_changed = "attachment removed"
+            what_changed = ATTACHMENT_REMOVED
             attachment = self.new_value
             old_value = "%s %s" % (
                 self.old_value.title,
@@ -656,7 +682,7 @@
         """See `IBugChange`."""
         return dict(
             newvalue=self.cve.sequence,
-            whatchanged='cve linked')
+            whatchanged=CVE_LINKED)
 
     def getBugNotification(self):
         """See `IBugChange`."""
@@ -674,7 +700,7 @@
         """See `IBugChange`."""
         return dict(
             oldvalue=self.cve.sequence,
-            whatchanged='cve unlinked')
+            whatchanged=CVE_UNLINKED)
 
     def getBugNotification(self):
         """See `IBugChange`."""

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-02-16 02:55:17 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-02-17 17:46:51 +0000
@@ -3814,7 +3814,13 @@
     @property
     def change_summary(self):
         """Return a formatted summary of the change."""
-        return self.attribute
+        if self.target is not None:
+            # This is a bug task.  We want the attribute, as filtered out.
+            return self.attribute
+        else:
+            # Otherwise, the attribute is more normalized than what we want.
+            # Use "whatchanged," which sometimes is more descriptive.
+            return self.whatchanged
 
     @property
     def _formatted_tags_change(self):
@@ -3858,30 +3864,31 @@
             'old_value': self.oldvalue,
             'new_value': self.newvalue,
             }
-        if self.attribute == 'summary':
+        attribute = self.attribute
+        if attribute == 'title':
             # We display summary changes as a unified diff, replacing
             # \ns with <br />s so that the lines are separated properly.
             diff = cgi.escape(
                 get_unified_diff(self.oldvalue, self.newvalue, 72), True)
             return diff.replace("\n", "<br />")
 
-        elif self.attribute == 'description':
+        elif attribute == 'description':
             # Description changes can be quite long, so we just return
             # 'updated' rather than returning the whole new description
             # or a diff.
             return 'updated'
 
-        elif self.attribute == 'tags':
+        elif attribute == 'tags':
             # We special-case tags because we can work out what's been
             # added and what's been removed.
             return self._formatted_tags_change.replace('\n', '<br />')
 
-        elif self.attribute == 'assignee':
+        elif attribute == 'assignee':
             for key in return_dict:
                 if return_dict[key] is None:
                     return_dict[key] = 'nobody'
 
-        elif self.attribute == 'milestone':
+        elif attribute == 'milestone':
             for key in return_dict:
                 if return_dict[key] is None:
                     return_dict[key] = 'none'

=== modified file 'lib/lp/bugs/doc/bugactivity.txt'
--- lib/lp/bugs/doc/bugactivity.txt	2011-02-11 18:33:04 +0000
+++ lib/lp/bugs/doc/bugactivity.txt	2011-02-17 17:46:51 +0000
@@ -113,13 +113,18 @@
     u'status'
 
 If the activity is not for a bug task, `target` is None, and `attribute` is
-the same as `whatchanged`.  For instance, look at the attributes on the
-previous activity.
+typically the same as `whatchanged`.  However, in some cases (ideally,
+whenever necessary), it is normalized to show the actual attribute name.
+For instance, look at the attributes on the previous activity.
 
     >>> print bug.activity[-2].target
     None
+    >>> bug.activity[-2].whatchanged
+    u'summary'
     >>> bug.activity[-2].attribute
-    u'summary'
+    'title'
+
+(This is covered more comprehensively in tests/test_bugchanges.py).
 
 Upstream product assignment edited
 ==================================

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-17 15:20:36 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-17 17:46:51 +0000
@@ -293,14 +293,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.
@@ -312,6 +308,29 @@
     To: [e-mail]
     ...
 
+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:
 
@@ -328,22 +347,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.
 
@@ -379,7 +382,6 @@
 
     >>> flush_notifications()
 
-
 Duplicates
 ----------
 

=== modified file 'lib/lp/bugs/model/bugactivity.py'
--- lib/lp/bugs/model/bugactivity.py	2011-02-11 18:15:28 +0000
+++ lib/lp/bugs/model/bugactivity.py	2011-02-17 17:46:51 +0000
@@ -17,6 +17,19 @@
 
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.sqlbase import SQLBase
+from lp.bugs.adapters.bugchange import (
+    ATTACHMENT_ADDED,
+    ATTACHMENT_REMOVED,
+    BRANCH_LINKED,
+    BRANCH_UNLINKED,
+    BUG_WATCH_ADDED,
+    BUG_WATCH_REMOVED,
+    CHANGED_DUPLICATE_MARKER,
+    CVE_LINKED,
+    CVE_UNLINKED,
+    MARKED_AS_DUPLICATE,
+    REMOVED_DUPLICATE_MARKER,
+    )
 from lp.bugs.interfaces.bugactivity import (
     IBugActivity,
     IBugActivitySet,
@@ -68,12 +81,31 @@
         `attribute` is determined based on the `whatchanged` string.
 
         :return: The attribute name of the item if `whatchanged` is of
-            the form <target_name>: <attribute>. Otherwise, return the
-            original `whatchanged` string.
+            the form <target_name>: <attribute>. If we know how to determine
+            the attribute by normalizing whatchanged, we return that.
+            Otherwise, return the original `whatchanged` string.
         """
         match = self.bugtask_change_re.match(self.whatchanged)
         if match is None:
-            return self.whatchanged
+            result = self.whatchanged
+            # Now we normalize names, as necessary.  This is fragile, but
+            # a reasonable incremental step.  These are consumed in
+            # lp.bugs.scripts.bugnotification.get_activity_key.
+            if result in (CHANGED_DUPLICATE_MARKER,
+                          MARKED_AS_DUPLICATE,
+                          REMOVED_DUPLICATE_MARKER):
+                result = 'duplicateof'
+            elif result in (ATTACHMENT_ADDED, ATTACHMENT_REMOVED):
+                result = 'attachments'
+            elif result in (BRANCH_LINKED, BRANCH_UNLINKED):
+                result = 'linked_branches'
+            elif result in (BUG_WATCH_ADDED, BUG_WATCH_REMOVED):
+                result = 'watches'
+            elif result in (CVE_LINKED, CVE_UNLINKED):
+                result = 'cves'
+            elif result == 'summary':
+                result = 'title'
+            return result
         else:
             return match.groupdict()['attribute']
 

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2011-02-16 14:11:46 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2011-02-17 17:46:51 +0000
@@ -31,6 +31,39 @@
 from lp.services.mail.mailwrapper import MailWrapper
 
 
+def get_activity_key(notification):
+    """Given a notification, return a key for the activity if it exists.
+    
+    The key will be used to determine whether changes for the activity are
+    undone within the same batch of notifications (which are supposed to
+    be all for the same bug when they get to this function).  Therefore,
+    the activity's attribute is a good start for the key.
+    
+    If the activity was on a bugtask, we will also want to distinguish
+    by bugtask, because, for instance, changing a status from INPROGRESS
+    to FIXCOMMITED on one bug task is not undone if the status changes
+    from FIXCOMMITTED to INPROGRESS on another bugtask.
+    
+    Similarly, if the activity is about adding or removing something
+    that we can have multiple of, like a branch or an attachment, the
+    key should include information on that value, because adding one
+    attachment is not undone by removing another one.
+    """
+    activity = notification.activity
+    if activity is not None:
+        key = activity.attribute
+        if activity.target is not None:
+            key = ':'.join((activity.target, key))
+        if key in ('attachments', 'watches', 'cves', 'linked_branches'):
+            # We are intentionally leaving bug task bugwatches out of this
+            # list, so we use the key rather than the activity.attribute.
+            if activity.oldvalue is not None:
+                key = ':'.join((key, activity.oldvalue))
+            elif activity.newvalue is not None:
+                key = ':'.join((key, activity.newvalue))
+        return key
+
+
 def construct_email_notifications(bug_notifications):
     """Construct an email from a list of related bug notifications.
 
@@ -39,31 +72,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_activity_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_activity_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 +135,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 +151,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 +203,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	2011-02-17 09:17:00 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-02-17 17:46:51 +0000
@@ -4,31 +4,57 @@
 
 __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,
+    get_activity_key,
     notification_batches,
     notification_comment_batches,
     )
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
+from lp.services.propertycache import cachedproperty
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory)
+from lp.testing.dbuser import lp_dbuser
+from lp.testing.matchers import Contains
 
 
 class MockBug:
@@ -105,6 +131,67 @@
         self.is_comment = is_comment
         self.date_emailed = date_emailed
         self.recipients = [MockBugNotificationRecipient()]
+        self.activity = None
+
+
+class FakeNotification:
+    """An even simpler fake notification.
+    
+    Used by TestGetActivityKey, TestNotificationCommentBatches and
+    TestNotificationBatches."""
+
+    class Message(object):
+        pass
+
+    def __init__(self, is_comment=False, bug=None, owner=None):
+        self.is_comment = is_comment
+        self.bug = bug
+        self.message = self.Message()
+        self.message.owner = owner
+        self.activity = None
+
+
+class MockBugActivity:
+    """A mock BugActivity used for testing."""
+    def __init__(self, target=None, attribute=None,
+                 oldvalue=None, newvalue=None):
+        self.target = target
+        self.attribute = attribute
+        self.oldvalue = oldvalue
+        self.newvalue = newvalue
+
+
+class TestGetActivityKey(TestCase):
+    """Tests for get_activity_key()."""
+
+    def test_no_activity(self):
+        self.assertEqual(get_activity_key(FakeNotification()), None)
+
+    def test_normal_bug_attribute_activity(self):
+        notification = FakeNotification()
+        notification.activity = MockBugActivity(attribute='title')
+        self.assertEqual(get_activity_key(notification), 'title')
+
+    def test_collection_bug_attribute_added_activity(self):
+        notification = FakeNotification()
+        notification.activity = MockBugActivity(
+            attribute='cves', newvalue='some cve identifier')
+        self.assertEqual(get_activity_key(notification),
+                         'cves:some cve identifier')
+
+    def test_collection_bug_attribute_removed_activity(self):
+        notification = FakeNotification()
+        notification.activity = MockBugActivity(
+            attribute='cves', oldvalue='some cve identifier')
+        self.assertEqual(get_activity_key(notification),
+                         'cves:some cve identifier')
+
+    def test_bugtask_attribute_activity(self):
+        notification = FakeNotification()
+        notification.activity = MockBugActivity(
+            attribute='status', target='some bug task identifier')
+        self.assertEqual(get_activity_key(notification),
+                         'some bug task identifier:status')
 
 
 class TestGetEmailNotifications(unittest.TestCase):
@@ -240,19 +327,6 @@
         self.assertEqual(bug_four.id, 4)
 
 
-class FakeNotification:
-    """Used by TestNotificationCommentBatches and TestNotificationBatches."""
-
-    class Message(object):
-        pass
-
-    def __init__(self, is_comment=False, bug=None, owner=None):
-        self.is_comment = is_comment
-        self.bug = bug
-        self.message = self.Message()
-        self.message.owner = owner
-
-
 class TestNotificationCommentBatches(unittest.TestCase):
     """Tests of `notification_comment_batches`."""
 
@@ -456,3 +530,339 @@
         expected = [notifications[0:3], notifications[3:4]]
         observed = list(notification_batches(notifications))
         self.assertEquals(expected, observed)
+
+
+class EmailNotificationTestBase(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(EmailNotificationTestBase, self).setUp()
+        login('[e-mail]')
+        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('[e-mail]')
+        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))

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-02-16 12:05:44 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-02-17 17:46:51 +0000
@@ -8,12 +8,12 @@
     ObjectModifiedEvent,
     )
 from lazr.lifecycle.snapshot import Snapshot
+from testtools.matchers import StartsWith
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import providedBy
 
 from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
-from lp.bugs.model.bugnotification import BugNotification
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -24,6 +24,7 @@
     BugTaskStatus,
     )
 from lp.bugs.interfaces.cve import ICveSet
+from lp.bugs.model.bugnotification import BugNotification
 from lp.bugs.scripts.bugnotification import construct_email_notifications
 from lp.testing import (
     person_logged_in,
@@ -220,6 +221,11 @@
         # the Bug's notifications.
         old_title = self.changeAttribute(self.bug, 'title', '42')
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'title')
+        self.assertEqual(activity.target, None)
+
         title_change_activity = {
             'whatchanged': 'summary',
             'oldvalue': old_title,
@@ -270,6 +276,11 @@
         bugtracker = self.factory.makeBugTracker()
         bug_watch = self.bug.addWatch(bugtracker, '42', self.user)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'watches')
+        self.assertEqual(activity.target, None)
+
         bugwatch_activity = {
             'person': self.user,
             'whatchanged': 'bug watch added',
@@ -337,6 +348,11 @@
         self.saveOldChanges()
         self.bug.removeWatch(bug_watch, self.user)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'watches')
+        self.assertEqual(activity.target, None)
+
         bugwatch_activity = {
             'person': self.user,
             'whatchanged': 'bug watch removed',
@@ -413,6 +429,12 @@
         # sends an e-mail notification.
         branch = self.factory.makeBranch()
         self.bug.linkBranch(branch, self.user)
+
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'linked_branches')
+        self.assertEqual(activity.target, None)
+
         added_activity = {
             'person': self.user,
             'whatchanged': 'branch linked',
@@ -459,6 +481,12 @@
         self.bug.linkBranch(branch, self.user)
         self.saveOldChanges()
         self.bug.unlinkBranch(branch, self.user)
+
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'linked_branches')
+        self.assertEqual(activity.target, None)
+
         added_activity = {
             'person': self.user,
             'whatchanged': 'branch unlinked',
@@ -654,6 +682,11 @@
         cve = getUtility(ICveSet)['1999-8979']
         self.bug.linkCVE(cve, self.user)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'cves')
+        self.assertEqual(activity.target, None)
+
         cve_linked_activity = {
             'person': self.user,
             'whatchanged': 'cve linked',
@@ -680,6 +713,11 @@
         self.saveOldChanges()
         self.bug.unlinkCVE(cve, self.user)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'cves')
+        self.assertEqual(activity.target, None)
+
         cve_unlinked_activity = {
             'person': self.user,
             'whatchanged': 'cve unlinked',
@@ -708,6 +746,11 @@
         attachment = self.factory.makeBugAttachment(
             bug=self.bug, owner=self.user, comment=message)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'attachments')
+        self.assertEqual(activity.target, None)
+
         attachment_added_activity = {
             'person': self.user,
             'whatchanged': 'attachment added',
@@ -741,6 +784,11 @@
 
         attachment.removeFromBug(user=self.user)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'attachments')
+        self.assertEqual(activity.target, None)
+
         attachment_removed_activity = {
             'person': self.user,
             'whatchanged': 'attachment removed',
@@ -858,6 +906,11 @@
             self.bug_task, bug_task_before_modification,
             ['importance'], user=self.user))
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'importance')
+        self.assertThat(activity.target, StartsWith(u'product-name'))
+
         expected_activity = {
             'person': self.user,
             'whatchanged': '%s: importance' % self.bug_task.bugtargetname,
@@ -1304,6 +1357,11 @@
             level=BugNotificationLevel.METADATA).getRecipients()
         self.changeAttribute(duplicate_bug, 'duplicateof', self.bug)
 
+        # This checks the activity's attribute and target attributes.
+        activity = duplicate_bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'marked as duplicate',
@@ -1351,6 +1409,11 @@
         self.saveOldChanges(duplicate_bug)
         self.changeAttribute(duplicate_bug, 'duplicateof', None)
 
+        # This checks the activity's attribute and target attributes.
+        activity = duplicate_bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'removed duplicate marker',
@@ -1382,6 +1445,11 @@
         self.saveOldChanges()
         self.changeAttribute(self.bug, 'duplicateof', bug_two)
 
+        # This checks the activity's attribute and target attributes.
+        activity = self.bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'changed duplicate marker',
@@ -1429,6 +1497,11 @@
             level=BugNotificationLevel.METADATA).getRecipients()
         self.changeAttribute(public_bug, 'duplicateof', private_bug)
 
+        # This checks the activity's attribute and target attributes.
+        activity = public_bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'marked as duplicate',
@@ -1468,6 +1541,11 @@
 
         self.changeAttribute(public_bug, 'duplicateof', None)
 
+        # This checks the activity's attribute and target attributes.
+        activity = public_bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'removed duplicate marker',
@@ -1504,6 +1582,11 @@
 
         self.changeAttribute(duplicate_bug, 'duplicateof', public_bug)
 
+        # This checks the activity's attribute and target attributes.
+        activity = duplicate_bug.activity[-1]
+        self.assertEqual(activity.attribute, 'duplicateof')
+        self.assertEqual(activity.target, None)
+
         expected_activity = {
             'person': self.user,
             'whatchanged': 'changed duplicate marker',