launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02615
[Merge] lp:~lifeless/launchpad/bug-607935 into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-607935 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-607935/+merge/49915
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).
--
https://code.launchpad.net/~lifeless/launchpad/bug-607935/+merge/49915
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-607935 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py 2010-12-20 13:39:41 +0000
+++ lib/lp/bugs/browser/bugcomment.py 2011-02-16 03:06:21 +0000
@@ -55,41 +55,29 @@
COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5)
-def build_comments_from_chunks(chunks, bugtask, truncate=False):
- """Build BugComments from MessageChunks."""
+def build_comments_from_chunks(bugtask, truncate=False, slice_info=None):
+ """Build BugComments from MessageChunks.
+
+ :param truncate: Perform truncation of large messages.
+ :param slice_info: If not None, an iterable of slices to retrieve.
+ """
+ chunks = bugtask.bug.getMessagesForView(slice_info=slice_info)
+ # This would be better as part of indexed_messages eager loading.
comments = {}
- index = 0
- for chunk in chunks:
- message_id = chunk.message.id
- bug_comment = comments.get(message_id)
+ for bugmessage, message, chunk in chunks:
+ bug_comment = comments.get(message.id)
if bug_comment is None:
- bug_comment = BugComment(
- index, chunk.message, bugtask)
- comments[message_id] = bug_comment
- index += 1
+ bug_comment = BugComment(bugmessage.index, message, bugtask,
+ visible=bugmessage.visible)
+ comments[message.id] = bug_comment
+ if bugmessage.bugwatchID is not None:
+ bug_comment.bugwatch = bugmessage.bugwatch
+ bug_comment.synchronized = (
+ bugmessage.remote_comment_id is not None)
bug_comment.chunks.append(chunk)
- # Set up the bug watch for all the imported comments. We do it
- # outside the for loop to avoid issuing one db query per comment.
- imported_bug_messages = getUtility(IBugMessageSet).getImportedBugMessages(
- bugtask.bug)
- for bug_message in imported_bug_messages:
- message_id = bug_message.message.id
- comments[message_id].bugwatch = bug_message.bugwatch
- comments[message_id].synchronized = (
- bug_message.remote_comment_id is not None)
-
- for bug_message in bugtask.bug.bug_messages:
- comment = comments.get(bug_message.messageID, None)
- # XXX intellectronica 2009-04-22, bug=365092: Currently, there are
- # some bug messages for which no chunks exist in the DB, so we need to
- # make sure that we skip them, since the corresponding message wont
- # have been added to the comments dictionary in the section above.
- if comment is not None:
- comment.visible = bug_message.visible
-
for comment in comments.values():
- # Once we have all the chunks related to a comment set up,
+ # Once we have all the chunks related to a comment populated,
# we get the text set up for display.
comment.setupText(truncate=truncate)
return comments
@@ -101,20 +89,28 @@
Generates a stream of comment instances (with the activity grouped within)
or `list`s of grouped activities.
- :param comments: An iterable of `BugComment` instances.
+ :param comments: An iterable of `BugComment` instances, which should be
+ sorted by index already.
:param activities: An iterable of `BugActivity` instances.
"""
window = COMMENT_ACTIVITY_GROUPING_WINDOW
comment_kind = "comment"
+ if comments:
+ max_index = comments[-1].index + 1
+ else:
+ max_index = 0
comments = (
- (comment.datecreated, comment.owner, comment_kind, comment)
+ (comment.datecreated, comment.index, comment.owner, comment_kind, comment)
for comment in comments)
activity_kind = "activity"
activity = (
- (activity.datechanged, activity.person, activity_kind, activity)
+ (activity.datechanged, max_index, activity.person, activity_kind, activity)
for activity in activities)
- events = sorted(chain(comments, activity), key=itemgetter(0, 1))
+ # when an action and a comment happen at the same time, the action comes
+ # second, when two events are tied the comment index is used to
+ # disambiguate.
+ events = sorted(chain(comments, activity), key=itemgetter(0, 1, 2))
def gen_event_windows(events):
"""Generate event windows.
@@ -123,12 +119,12 @@
an integer, and is incremented each time the windowing conditions are
triggered.
- :param events: An iterable of `(date, actor, kind, event)` tuples in
- order.
+ :param events: An iterable of `(date, ignored, actor, kind, event)`
+ tuples in order.
"""
window_comment, window_actor = None, None
window_index, window_end = 0, None
- for date, actor, kind, event in events:
+ for date, _, actor, kind, event in events:
window_ended = (
# A window may contain only one comment.
(window_comment is not None and kind is comment_kind) or
@@ -174,7 +170,7 @@
"""
implements(IBugComment)
- def __init__(self, index, message, bugtask, activity=None):
+ def __init__(self, index, message, bugtask, activity=None, visible=True):
self.index = index
self.bugtask = bugtask
self.bugwatch = None
@@ -195,6 +191,7 @@
self.activity = activity
self.synchronized = False
+ self.visible = visible
@property
def show_for_admin(self):
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-02-11 18:15:28 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-02-16 03:06:21 +0000
@@ -293,20 +293,30 @@
return title.strip()
-def get_comments_for_bugtask(bugtask, truncate=False):
+def get_comments_for_bugtask(bugtask, truncate=False, for_display=False,
+ slice_info=None):
"""Return BugComments related to a bugtask.
This code builds a sorted list of BugComments in one shot,
requiring only two database queries. It removes the titles
for those comments which do not have a "new" subject line
+
+ :param for_display: If true, the zeroth comment is given an empty body so
+ that it will be filtered by get_visible_comments.
+ :param slice_info: If not None, defines a list of slices of the comments to
+ retrieve.
"""
- chunks = bugtask.bug.getMessageChunks()
- comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)
+ comments = build_comments_from_chunks(bugtask, truncate=truncate,
+ slice_info=slice_info)
+ # TODO: further fat can be shaved off here by limiting the attachments we
+ # query to those that slice_info would include.
for attachment in bugtask.bug.attachments_unpopulated:
message_id = attachment.message.id
# All attachments are related to a message, so we can be
# sure that the BugComment is already created.
- assert message_id in comments, message_id
+ if message_id not in comments:
+ # We are not showing this message.
+ break
if attachment.type == BugAttachmentType.PATCH:
comments[message_id].patches.append(attachment)
else:
@@ -321,6 +331,12 @@
# this comment has a new title, so make that the rolling focus
current_title = comment.title
comment.display_title = True
+ if for_display and comments and comments[0].index==0:
+ # We show the text of the first comment as the bug description,
+ # or via the special link "View original description", but we want
+ # to display attachments filed together with the bug in the
+ # comment list.
+ comments[0].text_for_display = ''
return comments
@@ -344,7 +360,8 @@
# These two lines are here to fill the ValidPersonOrTeamCache cache,
# so that checking owner.is_valid_person, when rendering the link,
- # won't issue a DB query.
+ # won't issue a DB query. Note that this should be obsolete now with
+ # getMessagesForView improvements.
commenters = set(comment.owner for comment in visible_comments)
getUtility(IPersonSet).getValidPersons(commenters)
@@ -686,7 +703,8 @@
# This line of code keeps the view's query count down,
# possibly using witchcraft. It should be rewritten to be
# useful or removed in favour of making other queries more
- # efficient.
+ # efficient. The witchcraft is because the subscribers are accessed
+ # in the initial page load, so the data is actually used.
if self.user is not None:
list(bug.getSubscribersForPerson(self.user))
@@ -789,14 +807,8 @@
@cachedproperty
def comments(self):
"""Return the bugtask's comments."""
- comments = get_comments_for_bugtask(self.context, truncate=True)
- # We show the text of the first comment as the bug description,
- # or via the special link "View original description", but we want
- # to display attachments filed together with the bug in the
- # comment list.
- comments[0].text_for_display = ''
- assert len(comments) > 0, "A bug should have at least one comment."
- return comments
+ return get_comments_for_bugtask(self.context, truncate=True,
+ for_display=True)
@cachedproperty
def interesting_activity(self):
@@ -834,11 +846,22 @@
+ config.malone.comments_list_truncate_newest_to
< config.malone.comments_list_max_length)
- recent_comments = self.visible_recent_comments_for_display
- oldest_comments = self.visible_oldest_comments_for_display
+ if not self.visible_comments_truncated_for_display:
+ comments=self.comments
+ else:
+ # the comment function takes 0-offset counts where comment 0 is
+ # the initial description, so we need to add one to the limits
+ # to adjust.
+ oldest_count = 1 + self.visible_initial_comments
+ new_count = 1 + self.total_comments-self.visible_recent_comments
+ comments = get_comments_for_bugtask(
+ self.context, truncate=True, for_display=True,
+ slice_info=[
+ slice(None, oldest_count), slice(new_count, None)])
+ visible_comments = get_visible_comments(comments)
event_groups = group_comments_with_activity(
- comments=chain(oldest_comments, recent_comments),
+ comments=visible_comments,
activities=self.interesting_activity)
def group_activities_by_target(activities):
@@ -881,77 +904,61 @@
events = map(event_dict, event_groups)
- # Insert blank if we're showing only a subset of the comment list.
- if len(recent_comments) > 0:
+ # Insert blanks if we're showing only a subset of the comment list.
+ if self.visible_comments_truncated_for_display:
# Find the oldest recent comment in the event list.
- oldest_recent_comment = recent_comments[0]
- for index, event in enumerate(events):
- if event.get("comment") is oldest_recent_comment:
- num_hidden = (
- len(self.visible_comments)
- - len(oldest_comments)
- - len(recent_comments))
+ index = 0
+ prev_comment = None
+ while index < len(events):
+ event = events[index]
+ comment = event.get("comment")
+ if prev_comment is None:
+ prev_comment = comment
+ index += 1
+ continue
+ if comment is None:
+ index += 1
+ continue
+ if prev_comment.index + 1 != comment.index:
+ # There is a gap here, record it.
separator = {
- 'date': oldest_recent_comment.datecreated,
- 'num_hidden': num_hidden,
+ 'date': prev_comment.datecreated,
+ 'num_hidden': comment.index - prev_comment.index
}
events.insert(index, separator)
- break
-
+ index += 1
+ prev_comment = comment
+ index += 1
return events
- @cachedproperty
- def visible_comments(self):
- """All visible comments.
-
- See `get_visible_comments` for the definition of a "visible"
- comment.
- """
- return get_visible_comments(self.comments)
-
- @cachedproperty
- def visible_oldest_comments_for_display(self):
- """The list of oldest visible comments to be rendered.
-
- This considers truncating the comment list if there are tons
- of comments, but also obeys any explicitly requested ways to
- display comments (currently only "all" is recognised).
- """
- show_all = (self.request.form_ng.getOne('comments') == 'all')
- max_comments = config.malone.comments_list_max_length
- if show_all or len(self.visible_comments) <= max_comments:
- return self.visible_comments
- else:
- oldest_count = config.malone.comments_list_truncate_oldest_to
- return self.visible_comments[:oldest_count]
-
- @cachedproperty
- def visible_recent_comments_for_display(self):
- """The list of recent visible comments to be rendered.
-
- If the number of comments is beyond the maximum threshold, this
- returns the most recent few comments. If we're under the threshold,
- then visible_oldest_comments_for_display will be returning the bugs,
- so this routine will return an empty set to avoid duplication.
- """
- show_all = (self.request.form_ng.getOne('comments') == 'all')
- max_comments = config.malone.comments_list_max_length
- total = len(self.visible_comments)
- if show_all or total <= max_comments:
- return []
- else:
- start = total - config.malone.comments_list_truncate_newest_to
- return self.visible_comments[start:total]
-
- @property
+ @property
+ def visible_initial_comments(self):
+ """How many initial comments are being shown."""
+ return config.malone.comments_list_truncate_oldest_to
+
+ @property
+ def visible_recent_comments(self):
+ """How many recent comments are being shown."""
+ return config.malone.comments_list_truncate_newest_to
+
+ @cachedproperty
def visible_comments_truncated_for_display(self):
"""Whether the visible comment list is truncated for display."""
- return (len(self.visible_comments) >
- len(self.visible_oldest_comments_for_display))
+ show_all = (self.request.form_ng.getOne('comments') == 'all')
+ if show_all:
+ return False
+ max_comments = config.malone.comments_list_max_length
+ return self.total_comments > max_comments
+
+ @cachedproperty
+ def total_comments(self):
+ """We count all comments because the db cannot do visibility yet."""
+ return self.context.bug.bug_messages.count() - 1
def wasDescriptionModified(self):
"""Return a boolean indicating whether the description was modified"""
- return self.comments[0].text_contents != self.context.bug.description
+ return (self.context.bug.indexed_messages[0].text_contents !=
+ self.context.bug.description)
@cachedproperty
def linked_branches(self):
=== modified file 'lib/lp/bugs/browser/tests/bug-views.txt'
--- lib/lp/bugs/browser/tests/bug-views.txt 2010-12-21 20:57:11 +0000
+++ lib/lp/bugs/browser/tests/bug-views.txt 2011-02-16 03:06:21 +0000
@@ -206,16 +206,17 @@
True
The displayable comments for a bug can be obtained from the view
-property visible_oldest_comments_for_display.
+property activity_and_comments.
- >>> viewable_comments = ubuntu_bugview.visible_oldest_comments_for_display
+ >>> comments = [event.get('comment') for event in
+ ... ubuntu_bugview.activity_and_comments if event.get('comment')]
Because we omit the first comment, and because the third comment is
indentical to the second, we really only display one comment:
- >>> print len(viewable_comments)
+ >>> print len(comments)
1
- >>> [(c.index, c.owner.name, c.text_contents) for c in viewable_comments]
+ >>> [(c.index, c.owner.name, c.text_contents) for c in comments]
[(1, u'name16', u'I can reproduce this bug.')]
(Unregister our listener, since we no longer need it.)
=== 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 03:06:21 +0000
@@ -735,7 +735,7 @@
reindexMessages
security_related
tags
- getMessageChunks
+ getMessagesForView
isSubscribedToDupes
getSubscribersFromDuplicates
getSubscriptionsFromDuplicates
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2011-02-02 09:55:25 +0000
+++ lib/lp/bugs/doc/bug.txt 2011-02-16 03:06:21 +0000
@@ -1116,15 +1116,15 @@
------------
A bug comment is actually made up of a number of chunks. The
-IBug.getMessageChunks() method allows you to retreive these chunks in a
-single shot.
+IBug.getMessagesForView() method allows you to get all the data needed to
+show messages in the bugtask index template in one shot.
>>> from canonical.ftests.pgsql import CursorWrapper
>>> CursorWrapper.record_sql = True
>>> queries = len(CursorWrapper.last_executed_sql)
- >>> chunks = bug_two.getMessageChunks()
- >>> for chunk in sorted(chunks, key=lambda x:x.id):
+ >>> chunks = bug_two.getMessagesForView(None)
+ >>> for _, _1, chunk in sorted(chunks, key=lambda x:x[2].id):
... (chunk.id, chunk.message.id, chunk.message.owner.id,
... chunk.content[:30])
(4, 1, 16, u'Problem exists between chair a')
=== modified file 'lib/lp/bugs/doc/bugcomment.txt'
--- lib/lp/bugs/doc/bugcomment.txt 2010-12-21 15:07:26 +0000
+++ lib/lp/bugs/doc/bugcomment.txt 2011-02-16 03:06:21 +0000
@@ -14,7 +14,7 @@
The bug's description starts out identical to its first comment. In the course
of a bug's life, the description may be updated, but the first comment stays
intact. To improve readability, we never display the first comment in the bug
-page, and this is why visible_oldest_comments_for_display doesn't include it:
+page, and this is why the event stream elides it doesn't include it:
>>> bug_ten = getUtility(IBugSet).get(10)
>>> bug_ten_bugtask = bug_ten.bugtasks[0]
@@ -22,8 +22,10 @@
>>> bug_view = getMultiAdapter(
... (bug_ten_bugtask, LaunchpadTestRequest()), name='+index')
>>> bug_view.initialize()
- >>> rendered_comments = bug_view.visible_oldest_comments_for_display
- >>> [bug_comment.index for bug_comment in rendered_comments]
+ >>> def bug_comments(bug_view):
+ ... return [event.get('comment') for event in
+ ... bug_view.activity_and_comments if event.get('comment')]
+ >>> [bug_comment.index for bug_comment in bug_comments(bug_view)]
[1]
In the case of bug 10, the first comment is identical to the bug's
@@ -42,11 +44,10 @@
stored as bug attchments of the first comment. Similary, the first
comment of bugs imported from other bug trackers may have attachments.
We display these attachments in the comment section of the Web UI,
-hence visible_oldest_comments_for_display contains the first comment, if
-it has attachments.
+hence the activity stream contains the first comment, if it has attachments.
Currently, the first comment of bug 11 has no attachments, hence
-BugTaskView.visible_oldest_comments_for_display does not return the
+BugTaskView.activity_and_comments does not return the
first comment.
>>> bug_11 = getUtility(IBugSet).get(11)
@@ -54,12 +55,12 @@
>>> bug_11_view = getMultiAdapter(
... (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
>>> bug_11_view.initialize()
- >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
+ >>> rendered_comments = bug_comments(bug_11_view)
>>> [bug_comment.index for bug_comment in rendered_comments]
[1, 2, 3, 4, 5, 6]
If we add an attachment to the first comment, this comment is included
-in visible_oldest_comments_for_display...
+in activity_and_comments...
>>> import StringIO
>>> login("test@xxxxxxxxxxxxx")
@@ -71,7 +72,7 @@
>>> bug_11_view = getMultiAdapter(
... (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
>>> bug_11_view.initialize()
- >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
+ >>> rendered_comments = bug_comments(bug_11_view)
>>> [bug_comment.index for bug_comment in rendered_comments]
[0, 1, 2, 3, 4, 5, 6]
>>>
@@ -112,13 +113,13 @@
comments have been truncated:
>>> [(bug_comment.index, bug_comment.was_truncated)
- ... for bug_comment in bug_view.visible_oldest_comments_for_display]
+ ... for bug_comment in bug_comments(bug_view)]
[(1, True), (2, True)]
Let's take a closer look at one of the truncated comments. We can
display the truncated text using text_for_display:
- >>> comment_one = bug_view.visible_oldest_comments_for_display[0]
+ >>> comment_one = bug_comments(bug_view)[0]
>>> print comment_one.text_for_display #doctest: -ELLIPSIS
This would be a real...
@@ -222,7 +223,7 @@
... (bug_three_bugtask, LaunchpadTestRequest()), name='+index')
>>> bug_view.initialize()
>>> [(c.index, c.title, c.text_for_display)
- ... for c in bug_view.visible_oldest_comments_for_display]
+ ... for c in bug_comments(bug_view)]
[(1, u'Hi', u'Hello there'),
(3, u'Ho', u'Hello there'),
(5, u'Ho', u'Hello there'),
@@ -236,9 +237,8 @@
comments: visible_comments_truncated_for_display.
This is normally false, but for bugs with lots of comments, the
-visible_comments_truncated_for_display property becomes True and the
-visible_oldest_comments_for_display list is truncated to just the oldest
-comments.
+visible_comments_truncated_for_display property becomes True and the activity
+stream has the middle comments elided.
The configuration keys comments_list_max_length,
comments_list_truncate_oldest_to, and comments_list_truncate_newest_to
@@ -272,24 +272,14 @@
>>> bug = factory.makeBug()
>>> add_comments(bug, 9)
-If we create a view for this, we can see that all 9 comments are
-visible, and we can see that the list has not been truncated.
+If we create a view for this, we can see that truncation is disabled.
>>> bug_view = getMultiAdapter(
... (bug.default_bugtask, LaunchpadTestRequest()), name='+index')
>>> bug_view.initialize()
-
- >>> len(bug_view.visible_oldest_comments_for_display)
- 9
>>> bug_view.visible_comments_truncated_for_display
False
-When comments aren't being truncated and empty set will be returned by
-visible_recent_comments_for_display.
-
- >>> len(bug_view.visible_recent_comments_for_display)
- 0
-
Add two more comments, and the list will be truncated to only 8 total.
>>> add_comments(bug, 2)
@@ -300,9 +290,9 @@
>>> bug_view.visible_comments_truncated_for_display
True
- >>> len(bug_view.visible_oldest_comments_for_display)
+ >>> bug_view.visible_initial_comments
3
- >>> len(bug_view.visible_recent_comments_for_display)
+ >>> bug_view.visible_recent_comments
5
The display of all comments can be requested with a form parameter.
@@ -312,8 +302,6 @@
... (bug.default_bugtask, request), name='+index')
>>> bug_view.initialize()
- >>> len(bug_view.visible_oldest_comments_for_display)
- 11
>>> bug_view.visible_comments_truncated_for_display
False
=== 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 03:06:21 +0000
@@ -698,8 +698,15 @@
remote bug tracker, if it's an imported comment.
"""
- def getMessageChunks():
- """Return MessageChunks corresponding to comments made on this bug"""
+ def getMessagesForView(slice_info):
+ """Return BugMessage,Message,MessageChunks for renderinger.
+
+ This eager loads message.owner validity associated with the
+ bugmessages.
+
+ :param slice_info: Either None or a list of slices to constraint the
+ returned rows. The step parameter in each slice is ignored.
+ """
def getNullBugTask(product=None, productseries=None,
sourcepackagename=None, distribution=None,
=== modified file 'lib/lp/bugs/interfaces/bugmessage.py'
--- lib/lp/bugs/interfaces/bugmessage.py 2011-01-31 08:08:04 +0000
+++ lib/lp/bugs/interfaces/bugmessage.py 2011-02-16 03:06:21 +0000
@@ -51,6 +51,7 @@
message = Object(schema=IMessage, title=u"The message.")
bugwatch = Object(schema=IBugWatch,
title=u"A bugwatch to which the message pertains.")
+ bugwatchID = Int(title=u'The bugwatch id.', readonly=True)
remote_comment_id = TextLine(
title=u"The id this comment has in the bugwatch's bug tracker.")
visible = Bool(title=u"This message is visible or not.", required=False,
=== 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 03:06:21 +0000
@@ -458,6 +458,8 @@
@property
def indexed_messages(self):
"""See `IMessageTarget`."""
+ # Note that this is a decorated result set, so will cache its value (in
+ # the absence of slices)
return self._indexed_messages(include_content=True)
def _indexed_messages(self, include_content=False, include_parents=True,
@@ -1407,39 +1409,66 @@
"""See `IBug`."""
return self._question_from_bug
- def getMessageChunks(self):
+ def getMessagesForView(self, slice_info):
"""See `IBug`."""
- query = """
- Message.id = MessageChunk.message AND
- BugMessage.message = Message.id AND
- BugMessage.bug = %s
- """ % sqlvalues(self)
-
- chunks = MessageChunk.select(query,
- clauseTables=["BugMessage", "Message"],
- # XXX: kiko 2006-09-16 bug=60745:
- # There is an issue that presents itself
- # here if we prejoin message.owner: because Message is
- # already in the clauseTables, the SQL generated joins
- # against message twice and that causes the results to
- # break.
- prejoinClauseTables=["Message"],
- # Note the ordering by Message.id here; while datecreated in
- # production is never the same, it can be in the test suite.
- orderBy=["Message.datecreated", "Message.id",
- "MessageChunk.sequence"])
- chunks = list(chunks)
-
- # Since we can't prejoin, cache all people at once so we don't
- # have to do it while rendering, which is a big deal for bugs
- # with a million comments.
- owner_ids = set()
- for chunk in chunks:
- if chunk.message.ownerID:
- owner_ids.add(str(chunk.message.ownerID))
- list(Person.select("ID in (%s)" % ",".join(owner_ids)))
-
- return chunks
+ # Note that this function and indexed_messages have significant overlap
+ # and could stand to be refactored.
+ slices = []
+ if slice_info is not None:
+ # NB: This isn't a full implementation of the slice protocol,
+ # merely the bits needed by BugTask:+index.
+ for slice in slice_info:
+ if not slice.start:
+ assert slice.stop > 0, slice.stop
+ slices.append(BugMessage.index < slice.stop)
+ elif not slice.stop:
+ if slice.start < 0:
+ slices.append(BugMessage.index >= SQL(
+ "(select max(index) from "
+ "bugmessage where bug=%s) - %s" % (
+ sqlvalues(self.id), sqlvalues(slice.start))))
+ else:
+ slices.append(BugMessage.index >= slice.start)
+ else:
+ slices.append(BugMessage.index >= slice.start)
+ slices.append(BugMessage.index < slice.stop)
+ if slices:
+ ranges = [Or(*slices)]
+ else:
+ ranges = []
+ # We expect:
+ # 1 bugmessage -> 1 message -> small N chunks. For now, using a wide
+ # query seems fine as we have to join out from bugmessage anyway.
+ result = Store.of(self).find((BugMessage, Message, MessageChunk),
+ Message.id==MessageChunk.messageID,
+ BugMessage.messageID==Message.id,
+ BugMessage.bug==self.id,
+ *ranges)
+ result.order_by(BugMessage.index, MessageChunk.sequence)
+ def eager_load_owners(rows):
+ owners = set()
+ for row in rows:
+ owners.add(row[1].ownerID)
+ owners.discard(None)
+ if not owners:
+ return
+ PersonSet().getPrecachedPersonsFromIDs(owners,
+ need_validity=True)
+ def eager_load_watches(rows):
+ watches = set()
+ for row in rows:
+ watches.add(row[0].bugwatchID)
+ watches.discard(None)
+ if not watches:
+ return
+ list(Store.of(self).find(BugWatch, BugWatch.id.is_in(watches)))
+ def eager_load(rows):
+ # We eager load owner validity to permit rendering in the Web UI.
+ eager_load_owners(rows)
+ # The web UI shows imported comments differently, but loading the
+ # bug eager loads all bug watches.
+ # eager_load_watches(rows)
+ return DecoratedResultSet(result, pre_iter_hook=eager_load)
def getNullBugTask(self, product=None, productseries=None,
sourcepackagename=None, distribution=None,
=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
--- lib/lp/bugs/templates/bugcomment-macros.pt 2010-07-01 21:53:22 +0000
+++ lib/lp/bugs/templates/bugcomment-macros.pt 2011-02-16 03:06:21 +0000
@@ -66,7 +66,7 @@
class="sprite retry"
style="white-space: nowrap">
view all <span
- tal:replace="view/visible_comments/count:len"
+ tal:replace="view/total_comments"
/> comments</a>
</td>
</tr>
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2011-02-15 18:16:28 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2011-02-16 03:06:21 +0000
@@ -320,16 +320,16 @@
tal:condition="view/visible_comments_truncated_for_display">
<div class="informational message">
Displaying first <span
- tal:replace="view/visible_oldest_comments_for_display/count:len">23</span>
+ tal:replace="view/visible_initial_comments">23</span>
and last <span
- tal:replace="view/visible_recent_comments_for_display/count:len">32</span>
+ tal:replace="view/visible_recent_comments">32</span>
comments.
<tal:what-next
define="view_all_href
string:${context/fmt:url}?comments=all">
<a href="#" tal:attributes="href view_all_href">
View all <span
- tal:replace="view/visible_comments/count:len" />
+ tal:replace="view/total_comments" />
comments</a> or <a href="#" tal:attributes="href
view_all_href">add a comment</a>.
</tal:what-next>
Follow ups
