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

[Gary Poster]

add a link from bug notifications (which are about sending emails) to bug activities (which are about keeping an activity log for each bug, and are more normalized in their description of what happened) as part of work towards bug 164196.

This is the first of three branches that address bug 164196, and one of two that have a database patch. The other two are lp:~gary/launchpad/bug164196-2 and lp:~gary/launchpad/bug164196-3. My pre-implementation call for these changes was with Graham Binns.

The primary job of this branch is to add a link from bug notifications (which are about sending emails) to bug activities (which are about keeping an activity log for each bug, and are more normalized in their description of what happened). This allows me in branch 2 to use the bug activity records to determine what was done and undone.

To do this, I added an activity column in the database, and made the code that generates bug notifications and bug activities synchronize the two. The first of those is straightforward, but the second has some wrinkles.

I'll highlight what I did for the second part of the task in bullet form.

 * In the set of bug notifications, I added an activity object to the signature of addNotification.
 * Bug target's addCommentNotification and addNotification include activities when they get them (from the bug change object in addNotification, and explicitly in addCommentNotification). Note for this bullet item and later ones that comment notifications do not need activities for my needs for this bug, but I looked at them for completeness.
 * I combined lp.bugs.subscribers.bug.notify_bug_added and lp.bugs.subscribers.bugactivity.record_bug_added into the latter, so that the notification could have a link to the activity.

In the course of doing this, I discovered that lp.bugs.subscribers.bugactivity.record_bug_edited duplicated a small part (only the 'name' attribute, as found in BUG_INTERESTING_FIELDS) of the functionality in lp.bugs.subscribers.bug.notify_bug_modified. They were both registered for the same event and had the same result (for the one attribute). Therefore, I deleted it from the file, deleted BUG_INTERESTING_FIELDS since it was the only user, and removed the zcml registration, and ran tests to confirm: tests passed.

Thank you

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

<danilos> gary_poster, re -1, in lib/lp/bugs/model/bug.py you modify addCommentNotification to add activity parameter, but you don't modify addChange which I believe does a similar thing for non-comment changes — is that intentional or not?
<gary_poster> danilos, intentional. addChange uses change objects, which handle activities themselves

review: Approve (code)

[Stuart Bishop (stub)]

Will be be looking up BugNotification records by BugActivity? If so, we will need an index:
   CREATE INDEX bugnotification__activity__idx ON BugNotification(activity);

I don't think the BugActivity information has been useful in the past. Perhaps this will give it some life?

patch-2208-46-0.sql

review: Approve (db)

[Gary Poster (gary)]

On Feb 17, 2011, at 7:17 AM, Stuart Bishop wrote:

> Will be be looking up BugNotification records by BugActivity? If so, we will need an index:
> CREATE INDEX bugnotification__activity__idx ON BugNotification(activity);
...

My branches are not doing that, and I don't know of a reason to do so, so I'll leave it out for now.

Thank you.

Gary

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

Also, I just remembered one thing: we might need to add the column on BugNotificationArchive as well: not sure how important that is, but that table contains all notifications older than one month.

[Gary Poster (gary)]

On Feb 17, 2011, at 7:49 AM, Данило Шеган wrote:

> Also, I just remembered one thing: we might need to add the column on BugNotificationArchive as well: not sure how important that is, but that table contains all notifications older than one month.

Good thought. However, we have no use case for that at the moment, so I'm inclined to leave that alone until we do.

[Robert Collins (lifeless)]

review: Approve

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2011-02-02 17:52:54 +0000
+++ database/schema/comments.sql	2011-02-17 17:41:21 +0000
@@ -280,6 +280,7 @@
 COMMENT ON COLUMN BugNotification.message IS 'The message the contains the textual representation of the change.';
 COMMENT ON COLUMN BugNotification.is_comment IS 'Is the change a comment addition.';
 COMMENT ON COLUMN BugNotification.date_emailed IS 'When this notification was emailed to the bug subscribers.';
+COMMENT ON COLUMN BugNotification.activity IS 'The BugActivity record corresponding to this notification, if any.';
 
 
 -- BugNotificationAttachment
 
=== added file 'database/schema/patch-2208-46-0.sql'
--- database/schema/patch-2208-46-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-46-0.sql	2011-02-17 17:41:21 +0000
@@ -0,0 +1,8 @@
+-- Copyright 2011 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+ALTER TABLE BugNotification
+    ADD COLUMN activity INTEGER REFERENCES BugActivity;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 46, 0);
 
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-02-17 01:03:34 +0000
+++ lib/lp/bugs/configure.zcml	2011-02-17 17:41:21 +0000
@@ -58,9 +58,6 @@
         handler="lp.bugs.subscribers.bugcreation.at_least_one_task"/>
     <subscriber
         for="lp.bugs.interfaces.bug.IBug                 lazr.lifecycle.interfaces.IObjectCreatedEvent"
-        handler="lp.bugs.subscribers.bug.notify_bug_added"/>
-    <subscriber
-        for="lp.bugs.interfaces.bug.IBug                 lazr.lifecycle.interfaces.IObjectCreatedEvent"
         handler="canonical.launchpad.subscribers.karma.bug_created"/>
     <subscriber
         for="lp.bugs.interfaces.bug.IBug                 lazr.lifecycle.interfaces.IObjectModifiedEvent"
@@ -915,9 +912,6 @@
         for="lp.bugs.interfaces.bug.IBug                            zope.lifecycleevent.interfaces.IObjectCreatedEvent"
         handler="lp.bugs.subscribers.bugactivity.record_bug_added"/>
     <subscriber
-        for="lp.bugs.interfaces.bug.IBug                            lazr.lifecycle.interfaces.IObjectModifiedEvent"
-        handler="lp.bugs.subscribers.bugactivity.record_bug_edited"/>
-    <subscriber
         for="lp.bugs.interfaces.bugcve.IBugCve                            lazr.lifecycle.interfaces.IObjectCreatedEvent"
         handler="lp.bugs.subscribers.bugactivity.record_cve_linked_to_bug"/>
     <subscriber
 
=== modified file 'lib/lp/bugs/doc/bugnotifications.txt'
--- lib/lp/bugs/doc/bugnotifications.txt	2010-10-19 18:44:31 +0000
+++ lib/lp/bugs/doc/bugnotifications.txt	2011-02-17 17:41:21 +0000
@@ -51,6 +51,16 @@
     >>> print latest_notification.message.text_contents
     this is a comment
 
+Notifications usually have references to a corresponding bug activity.  These
+can get you details on precisely what changed from a more programmatic
+perspective.  You can get details on what the activity provides in
+bugactivity.txt, but for now here is a small demo.
+
+    >>> print latest_notification.activity.whatchanged
+    bug
+    >>> print latest_notification.activity.person.displayname
+    Sample Person
+
 
 === Editing a bug report ===
 
@@ -72,6 +82,12 @@
     >>> print latest_notification.message.text_contents
     ** Description changed:
     ...
+    >>> print latest_notification.activity.attribute
+    description
+    >>> print latest_notification.activity.oldvalue
+    this is a comment
+    >>> print latest_notification.activity.newvalue
+    a new description
 
 
 === Filing a new task on an existing bug ===
 
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-02-17 01:03:34 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-02-17 17:41:21 +0000
@@ -546,8 +546,11 @@
         True to include the master bug's subscribers as recipients.
         """
 
-    def addCommentNotification(message, recipients=None):
-        """Add a bug comment notification."""
+    def addCommentNotification(message, recipients=None, activity=None):
+        """Add a bug comment notification.
+        
+        If a BugActivity instance is provided as an `activity`, it is linked
+        to the notification."""
 
     def addChange(change, recipients=None):
         """Record a change to the bug.
 
=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py	2011-02-17 17:41:21 +0000
@@ -34,6 +34,11 @@
     message = Attribute(
         "The message containing the text representation of the changes"
         " to the bug.")
+    activity = Attribute(
+        "The bug activity object corresponding to this notification.  Will "
+        "be None for older notification objects, and will be None if the "
+        "bugchange object that provides the data for the change returns None "
+        "for getBugActivity.")
     bug = BugField(title=u"The bug this notification is for.",
                    required=True)
     is_comment = Bool(
@@ -54,7 +59,7 @@
     def getNotificationsToSend():
         """Returns the notifications pending to be sent."""
 
-    def addNotification(self, bug, is_comment, message, recipients):
+    def addNotification(self, bug, is_comment, message, recipients, activity):
         """Create a new `BugNotification`.
 
         Create a new `BugNotification` object and the corresponding
 
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-02-17 01:03:34 +0000
+++ lib/lp/bugs/model/bug.py	2011-02-17 17:41:21 +0000
@@ -1019,14 +1019,14 @@
         # original `Bug`.
         return recipients
 
-    def addCommentNotification(self, message, recipients=None):
+    def addCommentNotification(self, message, recipients=None, activity=None):
         """See `IBug`."""
         if recipients is None:
             recipients = self.getBugNotificationRecipients(
                 level=BugNotificationLevel.COMMENTS)
         getUtility(IBugNotificationSet).addNotification(
              bug=self, is_comment=True,
-             message=message, recipients=recipients)
+             message=message, recipients=recipients, activity=activity)
 
     def addChange(self, change, recipients=None):
         """See `IBug`."""
@@ -1036,12 +1036,14 @@
 
         activity_data = change.getBugActivity()
         if activity_data is not None:
-            getUtility(IBugActivitySet).new(
+            activity = getUtility(IBugActivitySet).new(
                 self, when, change.person,
                 activity_data['whatchanged'],
                 activity_data.get('oldvalue'),
                 activity_data.get('newvalue'),
                 activity_data.get('message'))
+        else:
+            activity = None
 
         notification_data = change.getBugNotification()
         if notification_data is not None:
@@ -1055,7 +1057,7 @@
                     level=BugNotificationLevel.METADATA)
             getUtility(IBugNotificationSet).addNotification(
                 bug=self, is_comment=False, message=message,
-                recipients=recipients)
+                recipients=recipients, activity=activity)
 
         self.updateHeat()
 
 
=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugnotification.py	2011-02-17 17:41:21 +0000
@@ -44,6 +44,8 @@
     implements(IBugNotification)
 
     message = ForeignKey(dbName='message', notNull=True, foreignKey='Message')
+    activity = ForeignKey(
+        dbName='activity', notNull=False, foreignKey='BugActivity')
     bug = ForeignKey(dbName='bug', notNull=True, foreignKey='Bug')
     is_comment = BoolCol(notNull=True)
     date_emailed = UtcDateTimeCol(notNull=False)
@@ -94,13 +96,13 @@
         pending_notifications.reverse()
         return pending_notifications
 
-    def addNotification(self, bug, is_comment, message, recipients):
+    def addNotification(self, bug, is_comment, message, recipients, activity):
         """See `IBugNotificationSet`."""
         if not recipients:
             return
         bug_notification = BugNotification(
             bug=bug, is_comment=is_comment,
-            message=message, date_emailed=None)
+            message=message, date_emailed=None, activity=activity)
         store = Store.of(bug_notification)
         # XXX jamesh 2008-05-21: these flushes are to fix ordering
         # problems in the bugnotification-sending.txt tests.
 
=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2011-02-07 09:39:54 +0000
+++ lib/lp/bugs/subscribers/bug.py	2011-02-17 17:41:21 +0000
@@ -6,7 +6,6 @@
     'add_bug_change_notifications',
     'get_bug_delta',
     'get_bugtask_indirect_subscribers',
-    'notify_bug_added',
     'notify_bug_attachment_added',
     'notify_bug_attachment_removed',
     'notify_bug_comment_added',
@@ -44,15 +43,6 @@
 
 
 @block_implicit_flushes
-def notify_bug_added(bug, event):
-    """Send an email notification that a bug was added.
-
-    Event must be an IObjectCreatedEvent.
-    """
-    bug.addCommentNotification(bug.initial_message)
-
-
-@block_implicit_flushes
 def notify_bug_modified(bug, event):
     """Handle bug change events.
 
 
=== modified file 'lib/lp/bugs/subscribers/bugactivity.py'
--- lib/lp/bugs/subscribers/bugactivity.py	2011-01-28 10:03:12 +0000
+++ lib/lp/bugs/subscribers/bugactivity.py	2011-02-17 17:41:21 +0000
@@ -33,11 +33,6 @@
 vocabulary_registry = getVocabularyRegistry()
 
 
-BUG_INTERESTING_FIELDS = [
-    'name',
-    ]
-
-
 def get_string_representation(obj):
     """Returns a string representation of an object.
 
@@ -100,32 +95,13 @@
 
 @block_implicit_flushes
 def record_bug_added(bug, object_created_event):
-    getUtility(IBugActivitySet).new(
+    activity = getUtility(IBugActivitySet).new(
         bug = bug.id,
         datechanged = UTC_NOW,
         person = IPerson(object_created_event.user),
         whatchanged = "bug",
         message = "added bug")
-
-
-@block_implicit_flushes
-def record_bug_edited(bug_edited, sqlobject_modified_event):
-    # If the event was triggered by a web service named operation, its
-    # edited_fields will be empty. We'll need to check all interesting
-    # fields to see which were actually changed.
-    sqlobject_modified_event.edited_fields = BUG_INTERESTING_FIELDS
-
-    changes = what_changed(sqlobject_modified_event)
-    for changed_field in changes:
-        oldvalue, newvalue = changes[changed_field]
-        getUtility(IBugActivitySet).new(
-            bug=bug_edited.id,
-            datechanged=UTC_NOW,
-            person=IPerson(sqlobject_modified_event.user),
-            whatchanged=changed_field,
-            oldvalue=oldvalue,
-            newvalue=newvalue,
-            message="")
+    bug.addCommentNotification(bug.initial_message, activity=activity)
 
 
 @block_implicit_flushes
 
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2010-12-24 15:15:21 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2011-02-17 17:41:21 +0000
@@ -157,7 +157,8 @@
         message = MessageSet().fromText(
             subject='subject', content='content')
         BugNotificationSet().addNotification(
-            bug=bug, is_comment=False, message=message, recipients=[])
+            bug=bug, is_comment=False, message=message, recipients=[],
+            activity=None)
 
 
 class TestNotificationsForDuplicates(TestCaseWithFactory):