launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02621
[Merge] lp:~gary/launchpad/bug164196-1 into lp:launchpad/db-devel
Gary Poster has proposed merging lp:~gary/launchpad/bug164196-1 into lp:launchpad/db-devel.
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-1/+merge/49973
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
--
https://code.launchpad.net/~gary/launchpad/bug164196-1/+merge/49973
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug164196-1 into lp:launchpad/db-devel.
=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql 2011-02-02 17:52:54 +0000
+++ database/schema/comments.sql 2011-02-16 14:02:03 +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-99-0.sql'
--- database/schema/patch-2208-99-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-99-0.sql 2011-02-16 14:02:03 +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, 99, 0);
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-02-10 23:16:48 +0000
+++ lib/lp/bugs/configure.zcml 2011-02-16 14:02:03 +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"
@@ -916,9 +913,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-16 14:02:03 +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-10 14:20:12 +0000
+++ lib/lp/bugs/interfaces/bug.py 2011-02-16 14:02:03 +0000
@@ -549,8 +549,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-16 14:02:03 +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-11 04:22:45 +0000
+++ lib/lp/bugs/model/bug.py 2011-02-16 14:02:03 +0000
@@ -1054,14 +1054,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`."""
@@ -1071,12 +1071,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:
@@ -1090,7 +1092,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-16 14:02:03 +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-16 14:02:03 +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-16 14:02:03 +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-16 14:02:03 +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):
