Merge lp:~lifeless/launchpad/bug-607935 into lp:launchpad

Proposed by Robert Collins
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 12398
Proposed branch: lp:~lifeless/launchpad/bug-607935
Merge into: lp:launchpad
Diff against target: 940 lines (+266/-223)
12 files modified
lib/lp/bugs/browser/bugcomment.py (+39/-38)
lib/lp/bugs/browser/bugtask.py (+84/-77)
lib/lp/bugs/browser/tests/bug-views.txt (+5/-4)
lib/lp/bugs/browser/tests/test_bugcomment.py (+33/-31)
lib/lp/bugs/configure.zcml (+1/-1)
lib/lp/bugs/doc/bug.txt (+22/-4)
lib/lp/bugs/doc/bugcomment.txt (+18/-30)
lib/lp/bugs/interfaces/bug.py (+9/-2)
lib/lp/bugs/interfaces/bugmessage.py (+1/-0)
lib/lp/bugs/model/bug.py (+50/-32)
lib/lp/bugs/templates/bugcomment-macros.pt (+1/-1)
lib/lp/bugs/templates/bugtask-index.pt (+3/-3)
To merge this branch: bzr merge lp:~lifeless/launchpad/bug-607935
Reviewer Review Type Date Requested Status
Stuart Bishop Approve
Review via email: mp+49915@code.launchpad.net

Commit message

[r=stub][bug=607935] Reduce overhead when showing only some bug comments.

Description of the change

In this iteration I start doing range requests on BugMessage.index and no longer read in every bug message for every page load.

This required refactoring the stack by which such messages are obtained - we really should ditch BugComment I think and just use BugMessage directly (having it delegate to Message for the message fields). But thats a different project.

Anyhow, there were some properties on the bug task view that required access to all messages to work - e.g. the concept of 'how many bug comments are visible with ?show=all present' requires us to load the text of every bug message because of the algorithm in play. I've taken the view that our users will value speed of page loading over absolute precision and adjusted these to show the total messages in the system, rather than the total that would be shown. This is massively faster.

The largest expected benefit from this branch is less Disk IO - e.g. in bug one, we should skip messages 41->1280 or so. Thats a good 5 seconds of IO when it occurs - and it occurs frequently.

As a drive by I fixed the bug message grouping logic to be stable on bugmessage.index if/when two bug comments have the same datetime - which can happen in tests (there was a fragile test failing 1/3rds of the time for me).

I also fixed the logic for putting 'show all message' fillers in to show them where any hidden message is, though still guarded by the overall check for whether we're bulk-hiding messages. This was needed to avoid going up to two identical queries to get different ranges of comments.

To post a comment you must log in.
Stuart Bishop (stub) wrote :

Mainly fine.

 - Remove the commented out eager bugwatch loader if it isn't needed.
 - Add a comment in build_comments_from_chunks() explaining that bugwatches will have been preloaded at this point so the code is not retrieving them one at a time from the db.
 - I see no tests for getMessagesForView's new slice behavior, in particular checking for off-by-one errors.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugcomment.py'
2--- lib/lp/bugs/browser/bugcomment.py 2010-12-20 13:39:41 +0000
3+++ lib/lp/bugs/browser/bugcomment.py 2011-02-16 20:08:14 +0000
4@@ -55,41 +55,33 @@
5 COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5)
6
7
8-def build_comments_from_chunks(chunks, bugtask, truncate=False):
9- """Build BugComments from MessageChunks."""
10+def build_comments_from_chunks(bugtask, truncate=False, slice_info=None):
11+ """Build BugComments from MessageChunks.
12+
13+ :param truncate: Perform truncation of large messages.
14+ :param slice_info: If not None, an iterable of slices to retrieve.
15+ """
16+ chunks = bugtask.bug.getMessagesForView(slice_info=slice_info)
17+ # This would be better as part of indexed_messages eager loading.
18 comments = {}
19- index = 0
20- for chunk in chunks:
21- message_id = chunk.message.id
22- bug_comment = comments.get(message_id)
23+ for bugmessage, message, chunk in chunks:
24+ bug_comment = comments.get(message.id)
25 if bug_comment is None:
26- bug_comment = BugComment(
27- index, chunk.message, bugtask)
28- comments[message_id] = bug_comment
29- index += 1
30+ bug_comment = BugComment(bugmessage.index, message, bugtask,
31+ visible=bugmessage.visible)
32+ comments[message.id] = bug_comment
33+ # This code path is currently only used from BugTask view which
34+ # have already loaded all the bug watches. If we start lazy loading
35+ # those, or not needing them we will need to batch lookup watches
36+ # here.
37+ if bugmessage.bugwatchID is not None:
38+ bug_comment.bugwatch = bugmessage.bugwatch
39+ bug_comment.synchronized = (
40+ bugmessage.remote_comment_id is not None)
41 bug_comment.chunks.append(chunk)
42
43- # Set up the bug watch for all the imported comments. We do it
44- # outside the for loop to avoid issuing one db query per comment.
45- imported_bug_messages = getUtility(IBugMessageSet).getImportedBugMessages(
46- bugtask.bug)
47- for bug_message in imported_bug_messages:
48- message_id = bug_message.message.id
49- comments[message_id].bugwatch = bug_message.bugwatch
50- comments[message_id].synchronized = (
51- bug_message.remote_comment_id is not None)
52-
53- for bug_message in bugtask.bug.bug_messages:
54- comment = comments.get(bug_message.messageID, None)
55- # XXX intellectronica 2009-04-22, bug=365092: Currently, there are
56- # some bug messages for which no chunks exist in the DB, so we need to
57- # make sure that we skip them, since the corresponding message wont
58- # have been added to the comments dictionary in the section above.
59- if comment is not None:
60- comment.visible = bug_message.visible
61-
62 for comment in comments.values():
63- # Once we have all the chunks related to a comment set up,
64+ # Once we have all the chunks related to a comment populated,
65 # we get the text set up for display.
66 comment.setupText(truncate=truncate)
67 return comments
68@@ -101,20 +93,28 @@
69 Generates a stream of comment instances (with the activity grouped within)
70 or `list`s of grouped activities.
71
72- :param comments: An iterable of `BugComment` instances.
73+ :param comments: An iterable of `BugComment` instances, which should be
74+ sorted by index already.
75 :param activities: An iterable of `BugActivity` instances.
76 """
77 window = COMMENT_ACTIVITY_GROUPING_WINDOW
78
79 comment_kind = "comment"
80+ if comments:
81+ max_index = comments[-1].index + 1
82+ else:
83+ max_index = 0
84 comments = (
85- (comment.datecreated, comment.owner, comment_kind, comment)
86+ (comment.datecreated, comment.index, comment.owner, comment_kind, comment)
87 for comment in comments)
88 activity_kind = "activity"
89 activity = (
90- (activity.datechanged, activity.person, activity_kind, activity)
91+ (activity.datechanged, max_index, activity.person, activity_kind, activity)
92 for activity in activities)
93- events = sorted(chain(comments, activity), key=itemgetter(0, 1))
94+ # when an action and a comment happen at the same time, the action comes
95+ # second, when two events are tied the comment index is used to
96+ # disambiguate.
97+ events = sorted(chain(comments, activity), key=itemgetter(0, 1, 2))
98
99 def gen_event_windows(events):
100 """Generate event windows.
101@@ -123,12 +123,12 @@
102 an integer, and is incremented each time the windowing conditions are
103 triggered.
104
105- :param events: An iterable of `(date, actor, kind, event)` tuples in
106- order.
107+ :param events: An iterable of `(date, ignored, actor, kind, event)`
108+ tuples in order.
109 """
110 window_comment, window_actor = None, None
111 window_index, window_end = 0, None
112- for date, actor, kind, event in events:
113+ for date, _, actor, kind, event in events:
114 window_ended = (
115 # A window may contain only one comment.
116 (window_comment is not None and kind is comment_kind) or
117@@ -174,7 +174,7 @@
118 """
119 implements(IBugComment)
120
121- def __init__(self, index, message, bugtask, activity=None):
122+ def __init__(self, index, message, bugtask, activity=None, visible=True):
123 self.index = index
124 self.bugtask = bugtask
125 self.bugwatch = None
126@@ -195,6 +195,7 @@
127 self.activity = activity
128
129 self.synchronized = False
130+ self.visible = visible
131
132 @property
133 def show_for_admin(self):
134
135=== modified file 'lib/lp/bugs/browser/bugtask.py'
136--- lib/lp/bugs/browser/bugtask.py 2011-02-11 18:15:28 +0000
137+++ lib/lp/bugs/browser/bugtask.py 2011-02-16 20:08:14 +0000
138@@ -293,20 +293,30 @@
139 return title.strip()
140
141
142-def get_comments_for_bugtask(bugtask, truncate=False):
143+def get_comments_for_bugtask(bugtask, truncate=False, for_display=False,
144+ slice_info=None):
145 """Return BugComments related to a bugtask.
146
147 This code builds a sorted list of BugComments in one shot,
148 requiring only two database queries. It removes the titles
149 for those comments which do not have a "new" subject line
150+
151+ :param for_display: If true, the zeroth comment is given an empty body so
152+ that it will be filtered by get_visible_comments.
153+ :param slice_info: If not None, defines a list of slices of the comments to
154+ retrieve.
155 """
156- chunks = bugtask.bug.getMessageChunks()
157- comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)
158+ comments = build_comments_from_chunks(bugtask, truncate=truncate,
159+ slice_info=slice_info)
160+ # TODO: further fat can be shaved off here by limiting the attachments we
161+ # query to those that slice_info would include.
162 for attachment in bugtask.bug.attachments_unpopulated:
163 message_id = attachment.message.id
164 # All attachments are related to a message, so we can be
165 # sure that the BugComment is already created.
166- assert message_id in comments, message_id
167+ if message_id not in comments:
168+ # We are not showing this message.
169+ break
170 if attachment.type == BugAttachmentType.PATCH:
171 comments[message_id].patches.append(attachment)
172 else:
173@@ -321,6 +331,12 @@
174 # this comment has a new title, so make that the rolling focus
175 current_title = comment.title
176 comment.display_title = True
177+ if for_display and comments and comments[0].index==0:
178+ # We show the text of the first comment as the bug description,
179+ # or via the special link "View original description", but we want
180+ # to display attachments filed together with the bug in the
181+ # comment list.
182+ comments[0].text_for_display = ''
183 return comments
184
185
186@@ -344,7 +360,8 @@
187
188 # These two lines are here to fill the ValidPersonOrTeamCache cache,
189 # so that checking owner.is_valid_person, when rendering the link,
190- # won't issue a DB query.
191+ # won't issue a DB query. Note that this should be obsolete now with
192+ # getMessagesForView improvements.
193 commenters = set(comment.owner for comment in visible_comments)
194 getUtility(IPersonSet).getValidPersons(commenters)
195
196@@ -686,7 +703,8 @@
197 # This line of code keeps the view's query count down,
198 # possibly using witchcraft. It should be rewritten to be
199 # useful or removed in favour of making other queries more
200- # efficient.
201+ # efficient. The witchcraft is because the subscribers are accessed
202+ # in the initial page load, so the data is actually used.
203 if self.user is not None:
204 list(bug.getSubscribersForPerson(self.user))
205
206@@ -789,14 +807,8 @@
207 @cachedproperty
208 def comments(self):
209 """Return the bugtask's comments."""
210- comments = get_comments_for_bugtask(self.context, truncate=True)
211- # We show the text of the first comment as the bug description,
212- # or via the special link "View original description", but we want
213- # to display attachments filed together with the bug in the
214- # comment list.
215- comments[0].text_for_display = ''
216- assert len(comments) > 0, "A bug should have at least one comment."
217- return comments
218+ return get_comments_for_bugtask(self.context, truncate=True,
219+ for_display=True)
220
221 @cachedproperty
222 def interesting_activity(self):
223@@ -834,11 +846,22 @@
224 + config.malone.comments_list_truncate_newest_to
225 < config.malone.comments_list_max_length)
226
227- recent_comments = self.visible_recent_comments_for_display
228- oldest_comments = self.visible_oldest_comments_for_display
229+ if not self.visible_comments_truncated_for_display:
230+ comments=self.comments
231+ else:
232+ # the comment function takes 0-offset counts where comment 0 is
233+ # the initial description, so we need to add one to the limits
234+ # to adjust.
235+ oldest_count = 1 + self.visible_initial_comments
236+ new_count = 1 + self.total_comments-self.visible_recent_comments
237+ comments = get_comments_for_bugtask(
238+ self.context, truncate=True, for_display=True,
239+ slice_info=[
240+ slice(None, oldest_count), slice(new_count, None)])
241+ visible_comments = get_visible_comments(comments)
242
243 event_groups = group_comments_with_activity(
244- comments=chain(oldest_comments, recent_comments),
245+ comments=visible_comments,
246 activities=self.interesting_activity)
247
248 def group_activities_by_target(activities):
249@@ -881,77 +904,61 @@
250
251 events = map(event_dict, event_groups)
252
253- # Insert blank if we're showing only a subset of the comment list.
254- if len(recent_comments) > 0:
255+ # Insert blanks if we're showing only a subset of the comment list.
256+ if self.visible_comments_truncated_for_display:
257 # Find the oldest recent comment in the event list.
258- oldest_recent_comment = recent_comments[0]
259- for index, event in enumerate(events):
260- if event.get("comment") is oldest_recent_comment:
261- num_hidden = (
262- len(self.visible_comments)
263- - len(oldest_comments)
264- - len(recent_comments))
265+ index = 0
266+ prev_comment = None
267+ while index < len(events):
268+ event = events[index]
269+ comment = event.get("comment")
270+ if prev_comment is None:
271+ prev_comment = comment
272+ index += 1
273+ continue
274+ if comment is None:
275+ index += 1
276+ continue
277+ if prev_comment.index + 1 != comment.index:
278+ # There is a gap here, record it.
279 separator = {
280- 'date': oldest_recent_comment.datecreated,
281- 'num_hidden': num_hidden,
282+ 'date': prev_comment.datecreated,
283+ 'num_hidden': comment.index - prev_comment.index
284 }
285 events.insert(index, separator)
286- break
287-
288+ index += 1
289+ prev_comment = comment
290+ index += 1
291 return events
292
293- @cachedproperty
294- def visible_comments(self):
295- """All visible comments.
296-
297- See `get_visible_comments` for the definition of a "visible"
298- comment.
299- """
300- return get_visible_comments(self.comments)
301-
302- @cachedproperty
303- def visible_oldest_comments_for_display(self):
304- """The list of oldest visible comments to be rendered.
305-
306- This considers truncating the comment list if there are tons
307- of comments, but also obeys any explicitly requested ways to
308- display comments (currently only "all" is recognised).
309- """
310- show_all = (self.request.form_ng.getOne('comments') == 'all')
311- max_comments = config.malone.comments_list_max_length
312- if show_all or len(self.visible_comments) <= max_comments:
313- return self.visible_comments
314- else:
315- oldest_count = config.malone.comments_list_truncate_oldest_to
316- return self.visible_comments[:oldest_count]
317-
318- @cachedproperty
319- def visible_recent_comments_for_display(self):
320- """The list of recent visible comments to be rendered.
321-
322- If the number of comments is beyond the maximum threshold, this
323- returns the most recent few comments. If we're under the threshold,
324- then visible_oldest_comments_for_display will be returning the bugs,
325- so this routine will return an empty set to avoid duplication.
326- """
327- show_all = (self.request.form_ng.getOne('comments') == 'all')
328- max_comments = config.malone.comments_list_max_length
329- total = len(self.visible_comments)
330- if show_all or total <= max_comments:
331- return []
332- else:
333- start = total - config.malone.comments_list_truncate_newest_to
334- return self.visible_comments[start:total]
335-
336- @property
337+ @property
338+ def visible_initial_comments(self):
339+ """How many initial comments are being shown."""
340+ return config.malone.comments_list_truncate_oldest_to
341+
342+ @property
343+ def visible_recent_comments(self):
344+ """How many recent comments are being shown."""
345+ return config.malone.comments_list_truncate_newest_to
346+
347+ @cachedproperty
348 def visible_comments_truncated_for_display(self):
349 """Whether the visible comment list is truncated for display."""
350- return (len(self.visible_comments) >
351- len(self.visible_oldest_comments_for_display))
352+ show_all = (self.request.form_ng.getOne('comments') == 'all')
353+ if show_all:
354+ return False
355+ max_comments = config.malone.comments_list_max_length
356+ return self.total_comments > max_comments
357+
358+ @cachedproperty
359+ def total_comments(self):
360+ """We count all comments because the db cannot do visibility yet."""
361+ return self.context.bug.bug_messages.count() - 1
362
363 def wasDescriptionModified(self):
364 """Return a boolean indicating whether the description was modified"""
365- return self.comments[0].text_contents != self.context.bug.description
366+ return (self.context.bug.indexed_messages[0].text_contents !=
367+ self.context.bug.description)
368
369 @cachedproperty
370 def linked_branches(self):
371
372=== modified file 'lib/lp/bugs/browser/tests/bug-views.txt'
373--- lib/lp/bugs/browser/tests/bug-views.txt 2010-12-21 20:57:11 +0000
374+++ lib/lp/bugs/browser/tests/bug-views.txt 2011-02-16 20:08:14 +0000
375@@ -206,16 +206,17 @@
376 True
377
378 The displayable comments for a bug can be obtained from the view
379-property visible_oldest_comments_for_display.
380+property activity_and_comments.
381
382- >>> viewable_comments = ubuntu_bugview.visible_oldest_comments_for_display
383+ >>> comments = [event.get('comment') for event in
384+ ... ubuntu_bugview.activity_and_comments if event.get('comment')]
385
386 Because we omit the first comment, and because the third comment is
387 indentical to the second, we really only display one comment:
388
389- >>> print len(viewable_comments)
390+ >>> print len(comments)
391 1
392- >>> [(c.index, c.owner.name, c.text_contents) for c in viewable_comments]
393+ >>> [(c.index, c.owner.name, c.text_contents) for c in comments]
394 [(1, u'name16', u'I can reproduce this bug.')]
395
396 (Unregister our listener, since we no longer need it.)
397
398=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
399--- lib/lp/bugs/browser/tests/test_bugcomment.py 2010-12-21 15:31:17 +0000
400+++ lib/lp/bugs/browser/tests/test_bugcomment.py 2011-02-16 20:08:14 +0000
401@@ -19,11 +19,11 @@
402
403 class BugActivityStub:
404
405- def __init__(self, datechanged, person=None):
406+ def __init__(self, datechanged, owner=None):
407 self.datechanged = datechanged
408- if person is None:
409- person = PersonStub()
410- self.person = person
411+ if owner is None:
412+ owner = PersonStub()
413+ self.person = owner
414
415 def __repr__(self):
416 return "BugActivityStub(%r, %r)" % (
417@@ -32,16 +32,18 @@
418
419 class BugCommentStub:
420
421- def __init__(self, datecreated, owner=None):
422+ def __init__(self, datecreated, index, owner=None):
423 self.datecreated = datecreated
424 if owner is None:
425 owner = PersonStub()
426 self.owner = owner
427 self.activity = []
428+ self.index = index
429
430 def __repr__(self):
431- return "BugCommentStub(%r, %r)" % (
432- self.datecreated.strftime('%Y-%m-%d--%H%M'), self.owner)
433+ return "BugCommentStub(%r, %d, %r)" % (
434+ self.datecreated.strftime('%Y-%m-%d--%H%M'),
435+ self.index, self.owner)
436
437
438 class PersonStub:
439@@ -61,8 +63,8 @@
440 def setUp(self):
441 super(TestGroupCommentsWithActivities, self).setUp()
442 self.now = datetime.now(utc)
443- self.timestamps = (
444- self.now + timedelta(minutes=counter)
445+ self.time_index = (
446+ (self.now + timedelta(minutes=counter), counter)
447 for counter in count(1))
448
449 def group(self, comments, activities):
450@@ -79,7 +81,7 @@
451 # When no activities are passed in, and the comments passed in don't
452 # have any common actors, no grouping is possible.
453 comments = [
454- BugCommentStub(next(self.timestamps))
455+ BugCommentStub(*next(self.time_index))
456 for number in xrange(5)]
457 self.assertEqual(
458 comments, self.group(comments=comments, activities=[]))
459@@ -88,7 +90,7 @@
460 # When no comments are passed in, and the activities passed in don't
461 # have any common actors, no grouping is possible.
462 activities = [
463- BugActivityStub(next(self.timestamps))
464+ BugActivityStub(next(self.time_index)[0])
465 for number in xrange(5)]
466 self.assertEqual(
467 [[activity] for activity in activities], self.group(
468@@ -97,13 +99,13 @@
469 def test_no_common_actor(self):
470 # When each activities and comment given has a different actor then no
471 # grouping is possible.
472- activity1 = BugActivityStub(next(self.timestamps))
473- comment1 = BugCommentStub(next(self.timestamps))
474- activity2 = BugActivityStub(next(self.timestamps))
475- comment2 = BugCommentStub(next(self.timestamps))
476+ activity1 = BugActivityStub(next(self.time_index)[0])
477+ comment1 = BugCommentStub(*next(self.time_index))
478+ activity2 = BugActivityStub(next(self.time_index)[0])
479+ comment2 = BugCommentStub(*next(self.time_index))
480
481 activities = set([activity1, activity2])
482- comments = set([comment1, comment2])
483+ comments = list([comment1, comment2])
484
485 self.assertEqual(
486 [[activity1], comment1, [activity2], comment2],
487@@ -113,8 +115,8 @@
488 # An activity shortly after a comment by the same person is grouped
489 # into the comment.
490 actor = PersonStub()
491- comment = BugCommentStub(next(self.timestamps), actor)
492- activity = BugActivityStub(next(self.timestamps), actor)
493+ comment = BugCommentStub(*next(self.time_index), owner=actor)
494+ activity = BugActivityStub(next(self.time_index)[0], owner=actor)
495 grouped = self.group(comments=[comment], activities=[activity])
496 self.assertEqual([comment], grouped)
497 self.assertEqual([activity], comment.activity)
498@@ -123,8 +125,8 @@
499 # An activity shortly before a comment by the same person is grouped
500 # into the comment.
501 actor = PersonStub()
502- activity = BugActivityStub(next(self.timestamps), actor)
503- comment = BugCommentStub(next(self.timestamps), actor)
504+ activity = BugActivityStub(next(self.time_index)[0], owner=actor)
505+ comment = BugCommentStub(*next(self.time_index), owner=actor)
506 grouped = self.group(comments=[comment], activities=[activity])
507 self.assertEqual([comment], grouped)
508 self.assertEqual([activity], comment.activity)
509@@ -133,9 +135,9 @@
510 # Activities shortly before and after a comment are grouped into the
511 # comment's activity.
512 actor = PersonStub()
513- activity1 = BugActivityStub(next(self.timestamps), actor)
514- comment = BugCommentStub(next(self.timestamps), actor)
515- activity2 = BugActivityStub(next(self.timestamps), actor)
516+ activity1 = BugActivityStub(next(self.time_index)[0], owner=actor)
517+ comment = BugCommentStub(*next(self.time_index), owner=actor)
518+ activity2 = BugActivityStub(next(self.time_index)[0], owner=actor)
519 grouped = self.group(
520 comments=[comment], activities=[activity1, activity2])
521 self.assertEqual([comment], grouped)
522@@ -146,7 +148,7 @@
523 # Anything outside of that window is considered separate.
524 actor = PersonStub()
525 activities = [
526- BugActivityStub(next(self.timestamps), actor)
527+ BugActivityStub(next(self.time_index)[0], owner=actor)
528 for count in xrange(8)]
529 grouped = self.group(comments=[], activities=activities)
530 self.assertEqual(2, len(grouped))
531@@ -156,8 +158,8 @@
532 def test_two_comments_by_common_actor(self):
533 # Only one comment will ever appear in a group.
534 actor = PersonStub()
535- comment1 = BugCommentStub(next(self.timestamps), actor)
536- comment2 = BugCommentStub(next(self.timestamps), actor)
537+ comment1 = BugCommentStub(*next(self.time_index), owner=actor)
538+ comment2 = BugCommentStub(*next(self.time_index), owner=actor)
539 grouped = self.group(comments=[comment1, comment2], activities=[])
540 self.assertEqual([comment1, comment2], grouped)
541
542@@ -165,11 +167,11 @@
543 # Activity gets associated with earlier comment when all other factors
544 # are unchanging.
545 actor = PersonStub()
546- activity1 = BugActivityStub(next(self.timestamps), actor)
547- comment1 = BugCommentStub(next(self.timestamps), actor)
548- activity2 = BugActivityStub(next(self.timestamps), actor)
549- comment2 = BugCommentStub(next(self.timestamps), actor)
550- activity3 = BugActivityStub(next(self.timestamps), actor)
551+ activity1 = BugActivityStub(next(self.time_index)[0], owner=actor)
552+ comment1 = BugCommentStub(*next(self.time_index), owner=actor)
553+ activity2 = BugActivityStub(next(self.time_index)[0], owner=actor)
554+ comment2 = BugCommentStub(*next(self.time_index), owner=actor)
555+ activity3 = BugActivityStub(next(self.time_index)[0], owner=actor)
556 grouped = self.group(
557 comments=[comment1, comment2],
558 activities=[activity1, activity2, activity3])
559
560=== modified file 'lib/lp/bugs/configure.zcml'
561--- lib/lp/bugs/configure.zcml 2011-02-14 00:15:22 +0000
562+++ lib/lp/bugs/configure.zcml 2011-02-16 20:08:14 +0000
563@@ -734,7 +734,7 @@
564 hasBranch
565 security_related
566 tags
567- getMessageChunks
568+ getMessagesForView
569 isSubscribedToDupes
570 getSubscribersFromDuplicates
571 getSubscriptionsFromDuplicates
572
573=== modified file 'lib/lp/bugs/doc/bug.txt'
574--- lib/lp/bugs/doc/bug.txt 2011-02-02 09:55:25 +0000
575+++ lib/lp/bugs/doc/bug.txt 2011-02-16 20:08:14 +0000
576@@ -1116,15 +1116,15 @@
577 ------------
578
579 A bug comment is actually made up of a number of chunks. The
580-IBug.getMessageChunks() method allows you to retreive these chunks in a
581-single shot.
582+IBug.getMessagesForView() method allows you to get all the data needed to
583+show messages in the bugtask index template in one shot.
584
585 >>> from canonical.ftests.pgsql import CursorWrapper
586 >>> CursorWrapper.record_sql = True
587 >>> queries = len(CursorWrapper.last_executed_sql)
588
589- >>> chunks = bug_two.getMessageChunks()
590- >>> for chunk in sorted(chunks, key=lambda x:x.id):
591+ >>> chunks = bug_two.getMessagesForView(None)
592+ >>> for _, _1, chunk in sorted(chunks, key=lambda x:x[2].id):
593 ... (chunk.id, chunk.message.id, chunk.message.owner.id,
594 ... chunk.content[:30])
595 (4, 1, 16, u'Problem exists between chair a')
596@@ -1141,6 +1141,24 @@
597 >>> len(CursorWrapper.last_executed_sql) - queries <= 3
598 True
599
600+getMessagesForView supports slicing operations:
601+
602+ >>> def message_ids(slices):
603+ ... chunks = bug_two.getMessagesForView(slices)
604+ ... return sorted(set(
605+ ... bugmessage.index for bugmessage, _, _1 in chunks))
606+ >>> message_ids([slice(1, 2)])
607+ [1]
608+
609+We use this to get the first N and last M messages in big bugs:
610+ >>> message_ids([slice(None, 1), slice(2, None)])
611+ [0, 2]
612+
613+We also support a negative lookup though the bug view does not use that at the
614+moment:
615+ >>> message_ids([slice(None, 1), slice(-1, None)])
616+ [0, 2]
617+
618 Bugs have a special attribute, `indexed_messages` which returns the collection
619 of messages, each decorated with the index of that message in its context
620 (the bug) and the primary bug task. This is used for providing an efficient
621
622=== modified file 'lib/lp/bugs/doc/bugcomment.txt'
623--- lib/lp/bugs/doc/bugcomment.txt 2010-12-21 15:07:26 +0000
624+++ lib/lp/bugs/doc/bugcomment.txt 2011-02-16 20:08:14 +0000
625@@ -14,7 +14,7 @@
626 The bug's description starts out identical to its first comment. In the course
627 of a bug's life, the description may be updated, but the first comment stays
628 intact. To improve readability, we never display the first comment in the bug
629-page, and this is why visible_oldest_comments_for_display doesn't include it:
630+page, and this is why the event stream elides it doesn't include it:
631
632 >>> bug_ten = getUtility(IBugSet).get(10)
633 >>> bug_ten_bugtask = bug_ten.bugtasks[0]
634@@ -22,8 +22,10 @@
635 >>> bug_view = getMultiAdapter(
636 ... (bug_ten_bugtask, LaunchpadTestRequest()), name='+index')
637 >>> bug_view.initialize()
638- >>> rendered_comments = bug_view.visible_oldest_comments_for_display
639- >>> [bug_comment.index for bug_comment in rendered_comments]
640+ >>> def bug_comments(bug_view):
641+ ... return [event.get('comment') for event in
642+ ... bug_view.activity_and_comments if event.get('comment')]
643+ >>> [bug_comment.index for bug_comment in bug_comments(bug_view)]
644 [1]
645
646 In the case of bug 10, the first comment is identical to the bug's
647@@ -42,11 +44,10 @@
648 stored as bug attchments of the first comment. Similary, the first
649 comment of bugs imported from other bug trackers may have attachments.
650 We display these attachments in the comment section of the Web UI,
651-hence visible_oldest_comments_for_display contains the first comment, if
652-it has attachments.
653+hence the activity stream contains the first comment, if it has attachments.
654
655 Currently, the first comment of bug 11 has no attachments, hence
656-BugTaskView.visible_oldest_comments_for_display does not return the
657+BugTaskView.activity_and_comments does not return the
658 first comment.
659
660 >>> bug_11 = getUtility(IBugSet).get(11)
661@@ -54,12 +55,12 @@
662 >>> bug_11_view = getMultiAdapter(
663 ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
664 >>> bug_11_view.initialize()
665- >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
666+ >>> rendered_comments = bug_comments(bug_11_view)
667 >>> [bug_comment.index for bug_comment in rendered_comments]
668 [1, 2, 3, 4, 5, 6]
669
670 If we add an attachment to the first comment, this comment is included
671-in visible_oldest_comments_for_display...
672+in activity_and_comments...
673
674 >>> import StringIO
675 >>> login("test@canonical.com")
676@@ -71,7 +72,7 @@
677 >>> bug_11_view = getMultiAdapter(
678 ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
679 >>> bug_11_view.initialize()
680- >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
681+ >>> rendered_comments = bug_comments(bug_11_view)
682 >>> [bug_comment.index for bug_comment in rendered_comments]
683 [0, 1, 2, 3, 4, 5, 6]
684 >>>
685@@ -112,13 +113,13 @@
686 comments have been truncated:
687
688 >>> [(bug_comment.index, bug_comment.was_truncated)
689- ... for bug_comment in bug_view.visible_oldest_comments_for_display]
690+ ... for bug_comment in bug_comments(bug_view)]
691 [(1, True), (2, True)]
692
693 Let's take a closer look at one of the truncated comments. We can
694 display the truncated text using text_for_display:
695
696- >>> comment_one = bug_view.visible_oldest_comments_for_display[0]
697+ >>> comment_one = bug_comments(bug_view)[0]
698 >>> print comment_one.text_for_display #doctest: -ELLIPSIS
699 This would be a real...
700
701@@ -222,7 +223,7 @@
702 ... (bug_three_bugtask, LaunchpadTestRequest()), name='+index')
703 >>> bug_view.initialize()
704 >>> [(c.index, c.title, c.text_for_display)
705- ... for c in bug_view.visible_oldest_comments_for_display]
706+ ... for c in bug_comments(bug_view)]
707 [(1, u'Hi', u'Hello there'),
708 (3, u'Ho', u'Hello there'),
709 (5, u'Ho', u'Hello there'),
710@@ -236,9 +237,8 @@
711 comments: visible_comments_truncated_for_display.
712
713 This is normally false, but for bugs with lots of comments, the
714-visible_comments_truncated_for_display property becomes True and the
715-visible_oldest_comments_for_display list is truncated to just the oldest
716-comments.
717+visible_comments_truncated_for_display property becomes True and the activity
718+stream has the middle comments elided.
719
720 The configuration keys comments_list_max_length,
721 comments_list_truncate_oldest_to, and comments_list_truncate_newest_to
722@@ -272,24 +272,14 @@
723 >>> bug = factory.makeBug()
724 >>> add_comments(bug, 9)
725
726-If we create a view for this, we can see that all 9 comments are
727-visible, and we can see that the list has not been truncated.
728+If we create a view for this, we can see that truncation is disabled.
729
730 >>> bug_view = getMultiAdapter(
731 ... (bug.default_bugtask, LaunchpadTestRequest()), name='+index')
732 >>> bug_view.initialize()
733-
734- >>> len(bug_view.visible_oldest_comments_for_display)
735- 9
736 >>> bug_view.visible_comments_truncated_for_display
737 False
738
739-When comments aren't being truncated and empty set will be returned by
740-visible_recent_comments_for_display.
741-
742- >>> len(bug_view.visible_recent_comments_for_display)
743- 0
744-
745 Add two more comments, and the list will be truncated to only 8 total.
746
747 >>> add_comments(bug, 2)
748@@ -300,9 +290,9 @@
749
750 >>> bug_view.visible_comments_truncated_for_display
751 True
752- >>> len(bug_view.visible_oldest_comments_for_display)
753+ >>> bug_view.visible_initial_comments
754 3
755- >>> len(bug_view.visible_recent_comments_for_display)
756+ >>> bug_view.visible_recent_comments
757 5
758
759 The display of all comments can be requested with a form parameter.
760@@ -312,8 +302,6 @@
761 ... (bug.default_bugtask, request), name='+index')
762 >>> bug_view.initialize()
763
764- >>> len(bug_view.visible_oldest_comments_for_display)
765- 11
766 >>> bug_view.visible_comments_truncated_for_display
767 False
768
769
770=== modified file 'lib/lp/bugs/interfaces/bug.py'
771--- lib/lp/bugs/interfaces/bug.py 2011-02-16 13:42:51 +0000
772+++ lib/lp/bugs/interfaces/bug.py 2011-02-16 20:08:14 +0000
773@@ -695,8 +695,15 @@
774 remote bug tracker, if it's an imported comment.
775 """
776
777- def getMessageChunks():
778- """Return MessageChunks corresponding to comments made on this bug"""
779+ def getMessagesForView(slice_info):
780+ """Return BugMessage,Message,MessageChunks for renderinger.
781+
782+ This eager loads message.owner validity associated with the
783+ bugmessages.
784+
785+ :param slice_info: Either None or a list of slices to constraint the
786+ returned rows. The step parameter in each slice is ignored.
787+ """
788
789 def getNullBugTask(product=None, productseries=None,
790 sourcepackagename=None, distribution=None,
791
792=== modified file 'lib/lp/bugs/interfaces/bugmessage.py'
793--- lib/lp/bugs/interfaces/bugmessage.py 2011-01-31 08:08:04 +0000
794+++ lib/lp/bugs/interfaces/bugmessage.py 2011-02-16 20:08:14 +0000
795@@ -51,6 +51,7 @@
796 message = Object(schema=IMessage, title=u"The message.")
797 bugwatch = Object(schema=IBugWatch,
798 title=u"A bugwatch to which the message pertains.")
799+ bugwatchID = Int(title=u'The bugwatch id.', readonly=True)
800 remote_comment_id = TextLine(
801 title=u"The id this comment has in the bugwatch's bug tracker.")
802 visible = Bool(title=u"This message is visible or not.", required=False,
803
804=== modified file 'lib/lp/bugs/model/bug.py'
805--- lib/lp/bugs/model/bug.py 2011-02-14 00:15:22 +0000
806+++ lib/lp/bugs/model/bug.py 2011-02-16 20:08:14 +0000
807@@ -437,6 +437,8 @@
808 @property
809 def indexed_messages(self):
810 """See `IMessageTarget`."""
811+ # Note that this is a decorated result set, so will cache its value (in
812+ # the absence of slices)
813 return self._indexed_messages(include_content=True)
814
815 def _indexed_messages(self, include_content=False, include_parents=True):
816@@ -1369,39 +1371,55 @@
817 """See `IBug`."""
818 return self._question_from_bug
819
820- def getMessageChunks(self):
821+ def getMessagesForView(self, slice_info):
822 """See `IBug`."""
823- query = """
824- Message.id = MessageChunk.message AND
825- BugMessage.message = Message.id AND
826- BugMessage.bug = %s
827- """ % sqlvalues(self)
828-
829- chunks = MessageChunk.select(query,
830- clauseTables=["BugMessage", "Message"],
831- # XXX: kiko 2006-09-16 bug=60745:
832- # There is an issue that presents itself
833- # here if we prejoin message.owner: because Message is
834- # already in the clauseTables, the SQL generated joins
835- # against message twice and that causes the results to
836- # break.
837- prejoinClauseTables=["Message"],
838- # Note the ordering by Message.id here; while datecreated in
839- # production is never the same, it can be in the test suite.
840- orderBy=["Message.datecreated", "Message.id",
841- "MessageChunk.sequence"])
842- chunks = list(chunks)
843-
844- # Since we can't prejoin, cache all people at once so we don't
845- # have to do it while rendering, which is a big deal for bugs
846- # with a million comments.
847- owner_ids = set()
848- for chunk in chunks:
849- if chunk.message.ownerID:
850- owner_ids.add(str(chunk.message.ownerID))
851- list(Person.select("ID in (%s)" % ",".join(owner_ids)))
852-
853- return chunks
854+ # Note that this function and indexed_messages have significant overlap
855+ # and could stand to be refactored.
856+ slices = []
857+ if slice_info is not None:
858+ # NB: This isn't a full implementation of the slice protocol,
859+ # merely the bits needed by BugTask:+index.
860+ for slice in slice_info:
861+ if not slice.start:
862+ assert slice.stop > 0, slice.stop
863+ slices.append(BugMessage.index < slice.stop)
864+ elif not slice.stop:
865+ if slice.start < 0:
866+ # If the high index is N, a slice of -1: should
867+ # return index N - so we need to add one to the
868+ # range.
869+ slices.append(BugMessage.index >= SQL(
870+ "(select max(index) from "
871+ "bugmessage where bug=%s) + 1 - %s" % (
872+ sqlvalues(self.id, -slice.start))))
873+ else:
874+ slices.append(BugMessage.index >= slice.start)
875+ else:
876+ slices.append(And(BugMessage.index >= slice.start,
877+ BugMessage.index < slice.stop))
878+ if slices:
879+ ranges = [Or(*slices)]
880+ else:
881+ ranges = []
882+ # We expect:
883+ # 1 bugmessage -> 1 message -> small N chunks. For now, using a wide
884+ # query seems fine as we have to join out from bugmessage anyway.
885+ result = Store.of(self).find((BugMessage, Message, MessageChunk),
886+ Message.id==MessageChunk.messageID,
887+ BugMessage.messageID==Message.id,
888+ BugMessage.bug==self.id,
889+ *ranges)
890+ result.order_by(BugMessage.index, MessageChunk.sequence)
891+ def eager_load_owners(rows):
892+ owners = set()
893+ for row in rows:
894+ owners.add(row[1].ownerID)
895+ owners.discard(None)
896+ if not owners:
897+ return
898+ PersonSet().getPrecachedPersonsFromIDs(owners,
899+ need_validity=True)
900+ return DecoratedResultSet(result, pre_iter_hook=eager_load_owners)
901
902 def getNullBugTask(self, product=None, productseries=None,
903 sourcepackagename=None, distribution=None,
904
905=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
906--- lib/lp/bugs/templates/bugcomment-macros.pt 2010-07-01 21:53:22 +0000
907+++ lib/lp/bugs/templates/bugcomment-macros.pt 2011-02-16 20:08:14 +0000
908@@ -66,7 +66,7 @@
909 class="sprite retry"
910 style="white-space: nowrap">
911 view all <span
912- tal:replace="view/visible_comments/count:len"
913+ tal:replace="view/total_comments"
914 /> comments</a>
915 </td>
916 </tr>
917
918=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
919--- lib/lp/bugs/templates/bugtask-index.pt 2011-02-15 18:16:28 +0000
920+++ lib/lp/bugs/templates/bugtask-index.pt 2011-02-16 20:08:14 +0000
921@@ -320,16 +320,16 @@
922 tal:condition="view/visible_comments_truncated_for_display">
923 <div class="informational message">
924 Displaying first <span
925- tal:replace="view/visible_oldest_comments_for_display/count:len">23</span>
926+ tal:replace="view/visible_initial_comments">23</span>
927 and last <span
928- tal:replace="view/visible_recent_comments_for_display/count:len">32</span>
929+ tal:replace="view/visible_recent_comments">32</span>
930 comments.
931 <tal:what-next
932 define="view_all_href
933 string:${context/fmt:url}?comments=all">
934 <a href="#" tal:attributes="href view_all_href">
935 View all <span
936- tal:replace="view/visible_comments/count:len" />
937+ tal:replace="view/total_comments" />
938 comments</a> or <a href="#" tal:attributes="href
939 view_all_href">add a comment</a>.
940 </tal:what-next>