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

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 10215
Proposed branch: lp:~gary/launchpad/bug164196-1
Merge into: lp:launchpad/db-devel
Diff against target: 293 lines (+50/-52)
11 files modified
database/schema/comments.sql (+1/-0)
database/schema/patch-2208-46-0.sql (+8/-0)
lib/lp/bugs/configure.zcml (+0/-6)
lib/lp/bugs/doc/bugnotifications.txt (+16/-0)
lib/lp/bugs/interfaces/bug.py (+5/-2)
lib/lp/bugs/interfaces/bugnotification.py (+6/-1)
lib/lp/bugs/model/bug.py (+6/-4)
lib/lp/bugs/model/bugnotification.py (+4/-2)
lib/lp/bugs/subscribers/bug.py (+0/-10)
lib/lp/bugs/subscribers/bugactivity.py (+2/-26)
lib/lp/bugs/tests/test_bugnotification.py (+2/-1)
To merge this branch: bzr merge lp:~gary/launchpad/bug164196-1
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Stuart Bishop (community) db Approve
Данило Шеган (community) code Approve
Review via email: mp+49973@code.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) wrote :

<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)
Revision history for this message
Stuart Bishop (stub) 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);

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)
Revision history for this message
Gary Poster (gary) wrote :

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

Revision history for this message
Данило Шеган (danilo) 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.

Revision history for this message
Gary Poster (gary) wrote :

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.

Revision history for this message
Robert Collins (lifeless) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/comments.sql'
2--- database/schema/comments.sql 2011-02-02 17:52:54 +0000
3+++ database/schema/comments.sql 2011-02-17 17:41:21 +0000
4@@ -280,6 +280,7 @@
5 COMMENT ON COLUMN BugNotification.message IS 'The message the contains the textual representation of the change.';
6 COMMENT ON COLUMN BugNotification.is_comment IS 'Is the change a comment addition.';
7 COMMENT ON COLUMN BugNotification.date_emailed IS 'When this notification was emailed to the bug subscribers.';
8+COMMENT ON COLUMN BugNotification.activity IS 'The BugActivity record corresponding to this notification, if any.';
9
10
11 -- BugNotificationAttachment
12
13=== added file 'database/schema/patch-2208-46-0.sql'
14--- database/schema/patch-2208-46-0.sql 1970-01-01 00:00:00 +0000
15+++ database/schema/patch-2208-46-0.sql 2011-02-17 17:41:21 +0000
16@@ -0,0 +1,8 @@
17+-- Copyright 2011 Canonical Ltd. This software is licensed under the
18+-- GNU Affero General Public License version 3 (see the file LICENSE).
19+SET client_min_messages=ERROR;
20+
21+ALTER TABLE BugNotification
22+ ADD COLUMN activity INTEGER REFERENCES BugActivity;
23+
24+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 46, 0);
25
26=== modified file 'lib/lp/bugs/configure.zcml'
27--- lib/lp/bugs/configure.zcml 2011-02-17 01:03:34 +0000
28+++ lib/lp/bugs/configure.zcml 2011-02-17 17:41:21 +0000
29@@ -58,9 +58,6 @@
30 handler="lp.bugs.subscribers.bugcreation.at_least_one_task"/>
31 <subscriber
32 for="lp.bugs.interfaces.bug.IBug lazr.lifecycle.interfaces.IObjectCreatedEvent"
33- handler="lp.bugs.subscribers.bug.notify_bug_added"/>
34- <subscriber
35- for="lp.bugs.interfaces.bug.IBug lazr.lifecycle.interfaces.IObjectCreatedEvent"
36 handler="canonical.launchpad.subscribers.karma.bug_created"/>
37 <subscriber
38 for="lp.bugs.interfaces.bug.IBug lazr.lifecycle.interfaces.IObjectModifiedEvent"
39@@ -915,9 +912,6 @@
40 for="lp.bugs.interfaces.bug.IBug zope.lifecycleevent.interfaces.IObjectCreatedEvent"
41 handler="lp.bugs.subscribers.bugactivity.record_bug_added"/>
42 <subscriber
43- for="lp.bugs.interfaces.bug.IBug lazr.lifecycle.interfaces.IObjectModifiedEvent"
44- handler="lp.bugs.subscribers.bugactivity.record_bug_edited"/>
45- <subscriber
46 for="lp.bugs.interfaces.bugcve.IBugCve lazr.lifecycle.interfaces.IObjectCreatedEvent"
47 handler="lp.bugs.subscribers.bugactivity.record_cve_linked_to_bug"/>
48 <subscriber
49
50=== modified file 'lib/lp/bugs/doc/bugnotifications.txt'
51--- lib/lp/bugs/doc/bugnotifications.txt 2010-10-19 18:44:31 +0000
52+++ lib/lp/bugs/doc/bugnotifications.txt 2011-02-17 17:41:21 +0000
53@@ -51,6 +51,16 @@
54 >>> print latest_notification.message.text_contents
55 this is a comment
56
57+Notifications usually have references to a corresponding bug activity. These
58+can get you details on precisely what changed from a more programmatic
59+perspective. You can get details on what the activity provides in
60+bugactivity.txt, but for now here is a small demo.
61+
62+ >>> print latest_notification.activity.whatchanged
63+ bug
64+ >>> print latest_notification.activity.person.displayname
65+ Sample Person
66+
67
68 === Editing a bug report ===
69
70@@ -72,6 +82,12 @@
71 >>> print latest_notification.message.text_contents
72 ** Description changed:
73 ...
74+ >>> print latest_notification.activity.attribute
75+ description
76+ >>> print latest_notification.activity.oldvalue
77+ this is a comment
78+ >>> print latest_notification.activity.newvalue
79+ a new description
80
81
82 === Filing a new task on an existing bug ===
83
84=== modified file 'lib/lp/bugs/interfaces/bug.py'
85--- lib/lp/bugs/interfaces/bug.py 2011-02-17 01:03:34 +0000
86+++ lib/lp/bugs/interfaces/bug.py 2011-02-17 17:41:21 +0000
87@@ -546,8 +546,11 @@
88 True to include the master bug's subscribers as recipients.
89 """
90
91- def addCommentNotification(message, recipients=None):
92- """Add a bug comment notification."""
93+ def addCommentNotification(message, recipients=None, activity=None):
94+ """Add a bug comment notification.
95+
96+ If a BugActivity instance is provided as an `activity`, it is linked
97+ to the notification."""
98
99 def addChange(change, recipients=None):
100 """Record a change to the bug.
101
102=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
103--- lib/lp/bugs/interfaces/bugnotification.py 2010-08-20 20:31:18 +0000
104+++ lib/lp/bugs/interfaces/bugnotification.py 2011-02-17 17:41:21 +0000
105@@ -34,6 +34,11 @@
106 message = Attribute(
107 "The message containing the text representation of the changes"
108 " to the bug.")
109+ activity = Attribute(
110+ "The bug activity object corresponding to this notification. Will "
111+ "be None for older notification objects, and will be None if the "
112+ "bugchange object that provides the data for the change returns None "
113+ "for getBugActivity.")
114 bug = BugField(title=u"The bug this notification is for.",
115 required=True)
116 is_comment = Bool(
117@@ -54,7 +59,7 @@
118 def getNotificationsToSend():
119 """Returns the notifications pending to be sent."""
120
121- def addNotification(self, bug, is_comment, message, recipients):
122+ def addNotification(self, bug, is_comment, message, recipients, activity):
123 """Create a new `BugNotification`.
124
125 Create a new `BugNotification` object and the corresponding
126
127=== modified file 'lib/lp/bugs/model/bug.py'
128--- lib/lp/bugs/model/bug.py 2011-02-17 01:03:34 +0000
129+++ lib/lp/bugs/model/bug.py 2011-02-17 17:41:21 +0000
130@@ -1019,14 +1019,14 @@
131 # original `Bug`.
132 return recipients
133
134- def addCommentNotification(self, message, recipients=None):
135+ def addCommentNotification(self, message, recipients=None, activity=None):
136 """See `IBug`."""
137 if recipients is None:
138 recipients = self.getBugNotificationRecipients(
139 level=BugNotificationLevel.COMMENTS)
140 getUtility(IBugNotificationSet).addNotification(
141 bug=self, is_comment=True,
142- message=message, recipients=recipients)
143+ message=message, recipients=recipients, activity=activity)
144
145 def addChange(self, change, recipients=None):
146 """See `IBug`."""
147@@ -1036,12 +1036,14 @@
148
149 activity_data = change.getBugActivity()
150 if activity_data is not None:
151- getUtility(IBugActivitySet).new(
152+ activity = getUtility(IBugActivitySet).new(
153 self, when, change.person,
154 activity_data['whatchanged'],
155 activity_data.get('oldvalue'),
156 activity_data.get('newvalue'),
157 activity_data.get('message'))
158+ else:
159+ activity = None
160
161 notification_data = change.getBugNotification()
162 if notification_data is not None:
163@@ -1055,7 +1057,7 @@
164 level=BugNotificationLevel.METADATA)
165 getUtility(IBugNotificationSet).addNotification(
166 bug=self, is_comment=False, message=message,
167- recipients=recipients)
168+ recipients=recipients, activity=activity)
169
170 self.updateHeat()
171
172
173=== modified file 'lib/lp/bugs/model/bugnotification.py'
174--- lib/lp/bugs/model/bugnotification.py 2010-08-20 20:31:18 +0000
175+++ lib/lp/bugs/model/bugnotification.py 2011-02-17 17:41:21 +0000
176@@ -44,6 +44,8 @@
177 implements(IBugNotification)
178
179 message = ForeignKey(dbName='message', notNull=True, foreignKey='Message')
180+ activity = ForeignKey(
181+ dbName='activity', notNull=False, foreignKey='BugActivity')
182 bug = ForeignKey(dbName='bug', notNull=True, foreignKey='Bug')
183 is_comment = BoolCol(notNull=True)
184 date_emailed = UtcDateTimeCol(notNull=False)
185@@ -94,13 +96,13 @@
186 pending_notifications.reverse()
187 return pending_notifications
188
189- def addNotification(self, bug, is_comment, message, recipients):
190+ def addNotification(self, bug, is_comment, message, recipients, activity):
191 """See `IBugNotificationSet`."""
192 if not recipients:
193 return
194 bug_notification = BugNotification(
195 bug=bug, is_comment=is_comment,
196- message=message, date_emailed=None)
197+ message=message, date_emailed=None, activity=activity)
198 store = Store.of(bug_notification)
199 # XXX jamesh 2008-05-21: these flushes are to fix ordering
200 # problems in the bugnotification-sending.txt tests.
201
202=== modified file 'lib/lp/bugs/subscribers/bug.py'
203--- lib/lp/bugs/subscribers/bug.py 2011-02-07 09:39:54 +0000
204+++ lib/lp/bugs/subscribers/bug.py 2011-02-17 17:41:21 +0000
205@@ -6,7 +6,6 @@
206 'add_bug_change_notifications',
207 'get_bug_delta',
208 'get_bugtask_indirect_subscribers',
209- 'notify_bug_added',
210 'notify_bug_attachment_added',
211 'notify_bug_attachment_removed',
212 'notify_bug_comment_added',
213@@ -44,15 +43,6 @@
214
215
216 @block_implicit_flushes
217-def notify_bug_added(bug, event):
218- """Send an email notification that a bug was added.
219-
220- Event must be an IObjectCreatedEvent.
221- """
222- bug.addCommentNotification(bug.initial_message)
223-
224-
225-@block_implicit_flushes
226 def notify_bug_modified(bug, event):
227 """Handle bug change events.
228
229
230=== modified file 'lib/lp/bugs/subscribers/bugactivity.py'
231--- lib/lp/bugs/subscribers/bugactivity.py 2011-01-28 10:03:12 +0000
232+++ lib/lp/bugs/subscribers/bugactivity.py 2011-02-17 17:41:21 +0000
233@@ -33,11 +33,6 @@
234 vocabulary_registry = getVocabularyRegistry()
235
236
237-BUG_INTERESTING_FIELDS = [
238- 'name',
239- ]
240-
241-
242 def get_string_representation(obj):
243 """Returns a string representation of an object.
244
245@@ -100,32 +95,13 @@
246
247 @block_implicit_flushes
248 def record_bug_added(bug, object_created_event):
249- getUtility(IBugActivitySet).new(
250+ activity = getUtility(IBugActivitySet).new(
251 bug = bug.id,
252 datechanged = UTC_NOW,
253 person = IPerson(object_created_event.user),
254 whatchanged = "bug",
255 message = "added bug")
256-
257-
258-@block_implicit_flushes
259-def record_bug_edited(bug_edited, sqlobject_modified_event):
260- # If the event was triggered by a web service named operation, its
261- # edited_fields will be empty. We'll need to check all interesting
262- # fields to see which were actually changed.
263- sqlobject_modified_event.edited_fields = BUG_INTERESTING_FIELDS
264-
265- changes = what_changed(sqlobject_modified_event)
266- for changed_field in changes:
267- oldvalue, newvalue = changes[changed_field]
268- getUtility(IBugActivitySet).new(
269- bug=bug_edited.id,
270- datechanged=UTC_NOW,
271- person=IPerson(sqlobject_modified_event.user),
272- whatchanged=changed_field,
273- oldvalue=oldvalue,
274- newvalue=newvalue,
275- message="")
276+ bug.addCommentNotification(bug.initial_message, activity=activity)
277
278
279 @block_implicit_flushes
280
281=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
282--- lib/lp/bugs/tests/test_bugnotification.py 2010-12-24 15:15:21 +0000
283+++ lib/lp/bugs/tests/test_bugnotification.py 2011-02-17 17:41:21 +0000
284@@ -157,7 +157,8 @@
285 message = MessageSet().fromText(
286 subject='subject', content='content')
287 BugNotificationSet().addNotification(
288- bug=bug, is_comment=False, message=message, recipients=[])
289+ bug=bug, is_comment=False, message=message, recipients=[],
290+ activity=None)
291
292
293 class TestNotificationsForDuplicates(TestCaseWithFactory):

Subscribers

People subscribed via source and target branches

to status/vote changes: