launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04812
[Merge] lp:~gmb/launchpad/bug-comments-lazy-load into lp:launchpad
Graham Binns has proposed merging lp:~gmb/launchpad/bug-comments-lazy-load into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #558642 in Launchpad itself: "BugTask:+index times out with comments=all on bugs with a lot of comments"
https://bugs.launchpad.net/launchpad/+bug/558642
For more details, see:
https://code.launchpad.net/~gmb/launchpad/bug-comments-lazy-load/+merge/73367
This branch adds feature-flagged asynchronous loading of bug comments. The use of this is twofold:
1. It should hopefully prevent - or at least reduce - the "BugTask:+index?comments=all
times out" problem, since that page will not get loaded except for non-JS browsers.
2. If it works sufficiently well in production, we can remove the currently-required
user interaction (i.e. clicking the "show more comments" link) and make the JS run
on page load, so that all bugs will have all comments displayed automatically and
asynchronously (we may want to implement pagination of some sort first, but that's
another bug.
The fix is feature-flagged since it's a UI change and I'd like to get some feedback from our beta-testers - or at least the LP team - before pushing it out to the whole wide world.
I tried working up a solution that involved pushing a lot of the logic for interleaved comments and bug activity back down to the model, but that code is so entwined with the display logic that it ended up being too much work and about 1500+ lines of changes.
Much simpler, then, to add a subclass of BugTaskView that can handle the batching of activity and comments. I've teased apart some of BugTaskView.activity_and_comments to allow the subclass to just reuse that whilst defining a few properties to make the batching work.
QA Plan:
1. Make sure that the bugs.batched_comment_loading.enabled is set to 'on'.
2. Look at a bug with a large number of comments (bug 1 is a good candidate).
3. The show all comments link should be green.
4. Clicking the link should load all the initially-hidden comments.
--
https://code.launchpad.net/~gmb/launchpad/bug-comments-lazy-load/+merge/73367
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/bug-comments-lazy-load into lp:launchpad.
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-08-24 21:17:35 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-08-30 12:55:31 +0000
@@ -1503,6 +1503,8 @@
comments_list_max_length: 100
comments_list_truncate_oldest_to: 40
comments_list_truncate_newest_to: 40
+# The default batch size for batched bug comment loading.
+comments_list_default_batch_size: 1000
# Should +filebug be disabled for Ubuntu ?
ubuntu_disable_filebug: false
=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py 2011-07-29 22:27:13 +0000
+++ lib/lp/bugs/browser/bugcomment.py 2011-08-30 12:55:31 +0000
@@ -85,7 +85,8 @@
return comments
-def group_comments_with_activity(comments, activities):
+def group_comments_with_activity(comments, activities, batch_size=None,
+ offset=None):
"""Group comments and activity together for human consumption.
Generates a stream of comment instances (with the activity grouped within)
@@ -115,6 +116,13 @@
# second, when two events are tied the comment index is used to
# disambiguate.
events = sorted(chain(comments, activity), key=itemgetter(0, 1, 2))
+ if batch_size is not None:
+ # If we're limiting to a given set of results, we work on just
+ # that subset of results from hereon in, which saves on
+ # processing time a bit.
+ if offset is None:
+ offset = 0
+ events = events[offset:offset+batch_size]
def gen_event_windows(events):
"""Generate event windows.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-08-29 18:21:28 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-08-30 12:55:31 +0000
@@ -674,7 +674,14 @@
def initialize(self):
"""Set up the needed widgets."""
bug = self.context.bug
- IJSONRequestCache(self.request).objects['bug'] = bug
+ cache = IJSONRequestCache(self.request)
+ cache.objects['bug'] = bug
+ cache.objects['total_comments_and_activity'] = (
+ self.total_comments + self.total_activity)
+ cache.objects['initial_comment_batch_offset'] = (
+ self.visible_initial_comments)
+ cache.objects['first visible_recent_comment'] = (
+ self.total_comments - self.visible_recent_comments)
# See render() for how this flag is used.
self._redirecting_to_bug_list = False
@@ -740,21 +747,7 @@
for activity in self.context.bug.activity
if interesting_match(activity.whatchanged) is not None)
- @cachedproperty
- def activity_and_comments(self):
- """Build list of comments interleaved with activities
-
- When activities occur on the same day a comment was posted,
- encapsulate them with that comment. For the remainder, group
- then as if owned by the person who posted the first action
- that day.
-
- If the number of comments exceeds the configured maximum limit, the
- list will be truncated to just the first and last sets of comments.
-
- The division between the most recent and oldest is marked by an entry
- in the list with the key 'num_hidden' defined.
- """
+ def _getEventGroups(self, batch_size=None, offset=None):
# Ensure truncation results in < max_length comments as expected
assert(config.malone.comments_list_truncate_oldest_to
+ config.malone.comments_list_truncate_newest_to
@@ -781,7 +774,37 @@
event_groups = group_comments_with_activity(
comments=visible_comments,
- activities=self.interesting_activity)
+ activities=self.interesting_activity,
+ batch_size=batch_size, offset=offset)
+ return event_groups
+
+ @cachedproperty
+ def _event_groups(self):
+ """Return a sorted list of event groups for the current BugTask.
+
+ This is a @cachedproperty wrapper around _getEventGroups(). It's
+ here so that we can override it in descendant views, passing
+ batch size parameters and suchlike to _getEventGroups() as we
+ go.
+ """
+ return self._getEventGroups()
+
+ @cachedproperty
+ def activity_and_comments(self):
+ """Build list of comments interleaved with activities
+
+ When activities occur on the same day a comment was posted,
+ encapsulate them with that comment. For the remainder, group
+ then as if owned by the person who posted the first action
+ that day.
+
+ If the number of comments exceeds the configured maximum limit, the
+ list will be truncated to just the first and last sets of comments.
+
+ The division between the most recent and oldest is marked by an entry
+ in the list with the key 'num_hidden' defined.
+ """
+ event_groups = self._event_groups
def group_activities_by_target(activities):
activities = sorted(
@@ -874,6 +897,11 @@
"""We count all comments because the db cannot do visibility yet."""
return self.context.bug.bug_messages.count() - 1
+ @cachedproperty
+ def total_activity(self):
+ """Return the count of all activity items for the bug."""
+ return self.context.bug.activity.count()
+
def wasDescriptionModified(self):
"""Return a boolean indicating whether the description was modified"""
return (self.context.bug._indexed_messages(
@@ -1027,6 +1055,54 @@
return html
+class BugTaskBatchedCommentsAndActivityView(BugTaskView):
+ """A view for displaying batches of bug comments and activity."""
+
+ # We never truncate comments in this view; there would be no point.
+ visible_comments_truncated_for_display = False
+
+ @property
+ def offset(self):
+ try:
+ return int(self.request.form_ng.getOne('offset'))
+ except TypeError:
+ # We return visible_initial_comments, since otherwise we'd
+ # end up repeating comments that are already visible on the
+ # page.
+ return self.visible_initial_comments
+
+ @property
+ def batch_size(self):
+ try:
+ return int(self.request.form_ng.getOne('batch_size'))
+ except TypeError:
+ return config.malone.comments_list_default_batch_size
+
+ @property
+ def next_batch_url(self):
+ return "%s?offset=%s&batch_size=%s" % (
+ canonical_url(self.context, view_name='+batched-comments'),
+ self.next_offset, self.batch_size)
+
+ @property
+ def next_offset(self):
+ return self.offset + self.batch_size
+
+ @cachedproperty
+ def _event_groups(self):
+ """See `BugTaskView`."""
+ return self._getEventGroups(
+ batch_size=self.batch_size,
+ offset=self.offset)
+
+ @cachedproperty
+ def has_more_comments_and_activity(self):
+ """Return True if there are more camments and activity to load."""
+ return (
+ len(self.activity_and_comments) > 0 and
+ self.next_offset < (self.total_comments + self.total_activity))
+
+
class BugTaskPortletView:
"""A portlet for displaying a bug's bugtasks."""
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml 2011-08-22 18:13:58 +0000
+++ lib/lp/bugs/browser/configure.zcml 2011-08-30 12:55:31 +0000
@@ -646,6 +646,12 @@
permission="launchpad.View"
template="../templates/bugtask-index.pt"/>
<browser:page
+ name="+batched-comments"
+ for="lp.bugs.interfaces.bugtask.IBugTask"
+ class="lp.bugs.browser.bugtask.BugTaskBatchedCommentsAndActivityView"
+ permission="launchpad.View"
+ template="../templates/bugtask-batched-comments.pt"/>
+ <browser:page
name="+choose-affected-product"
for="lp.bugs.interfaces.bugtask.IBugTask"
class="lp.bugs.browser.bugalsoaffects.BugAlsoAffectsProductMetaView"
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-08-29 18:21:28 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-08-30 12:55:31 +0000
@@ -5,7 +5,10 @@
import transaction
-from datetime import datetime
+from datetime import (
+ datetime,
+ timedelta,
+ )
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
@@ -20,6 +23,7 @@
from zope.interface import providedBy
from zope.security.proxy import removeSecurityProxy
+from canonical.config import config
from canonical.launchpad.ftests import (
ANONYMOUS,
login,
@@ -33,6 +37,7 @@
LaunchpadFunctionalLayer,
)
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.bugs.adapters.bugchange import BugTaskStatusChange
from lp.bugs.browser.bugtask import (
BugActivityItem,
BugTaskEditView,
@@ -1037,3 +1042,90 @@
self.assertEquals(
"- foo<br />+ bar &<>",
BugActivityItem(bug.activity[-1]).change_details)
+
+
+class TestBugTaskBatchedCommentsAndActivityView(TestCaseWithFactory):
+ """Tests for the BugTaskBatchedCommentsAndActivityView class."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def _makeNoisyBug(self, comments_only=False):
+ """Create and return a bug with a lot of comments and activity."""
+ bug = self.factory.makeBug(
+ date_created=datetime.now(UTC) - timedelta(days=30))
+ with person_logged_in(bug.owner):
+ if not comments_only:
+ for i in range(10):
+ task = self.factory.makeBugTask(bug=bug)
+ change = BugTaskStatusChange(
+ task, datetime.now(UTC), task.product.owner, 'status',
+ BugTaskStatus.NEW, BugTaskStatus.TRIAGED)
+ bug.addChange(change)
+ for i in range (10):
+ msg = self.factory.makeMessage(
+ owner=bug.owner, content="Message %i." % i,
+ datecreated=datetime.now(UTC) - timedelta(days=20-i))
+ bug.linkMessage(msg, user=bug.owner)
+ return bug
+
+ def test_offset(self):
+ # BugTaskBatchedCommentsAndActivityView.offset returns the
+ # current offset being used to select a batch of bug comments
+ # and activity. If one is not specified, the view's
+ # visible_initial_comments count will be returned (so that
+ # comments already shown on the page won't appear twice).
+ bug_task = self.factory.makeBugTask()
+ view = create_initialized_view(bug_task, '+batched-comments')
+ self.assertEqual(view.visible_initial_comments, view.offset)
+ view = create_initialized_view(
+ bug_task, '+batched-comments', form={'offset': 100})
+ self.assertEqual(100, view.offset)
+
+ def test_batch_size(self):
+ # BugTaskBatchedCommentsAndActivityView.batch_size returns the
+ # current batch_size being used to select a batch of bug comments
+ # and activity or the default configured batch size if one has
+ # not been specified.
+ bug_task = self.factory.makeBugTask()
+ view = create_initialized_view(bug_task, '+batched-comments')
+ self.assertEqual(
+ config.malone.comments_list_default_batch_size,
+ view.batch_size)
+ view = create_initialized_view(
+ bug_task, '+batched-comments', form={'batch_size': 20})
+ self.assertEqual(20, view.batch_size)
+
+ def test_event_groups_only_returns_batch_size_results(self):
+ # BugTaskBatchedCommentsAndActivityView._event_groups will
+ # return only batch_size results.
+ bug = self._makeNoisyBug()
+ view = create_initialized_view(
+ bug.default_bugtask, '+batched-comments',
+ form={'batch_size': 10})
+ self.assertEqual(10, len([group for group in view._event_groups]))
+
+ def test_activity_and_comments_matches_unbatched_version(self):
+ # BugTaskBatchedCommentsAndActivityView extends BugTaskView in
+ # order to add the batching logic and reduce rendering
+ # overheads. The results of activity_and_comments is the same
+ # for both.
+ # We create a bug with comments only so that we can test the
+ # contents of activity_and_comments properly. Trying to test it
+ # with multiply different datatypes is fragile at best.
+ bug = self._makeNoisyBug(comments_only=True)
+ # We create a batched view with an offset of 0 so that all the
+ # comments are returned.
+ batched_view = create_initialized_view(
+ bug.default_bugtask, '+batched-comments',
+ form={'offset': 0})
+ unbatched_view = create_initialized_view(
+ bug.default_bugtask, '+index')
+ self.assertEqual(
+ len(unbatched_view.activity_and_comments),
+ len(batched_view.activity_and_comments))
+ for i in range(len(unbatched_view.activity_and_comments)):
+ unbatched_item = unbatched_view.activity_and_comments[i]
+ batched_item = batched_view.activity_and_comments[i]
+ self.assertEqual(
+ unbatched_item['comment'].text_for_display,
+ batched_item['comment'].text_for_display)
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-08-22 12:13:22 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-08-30 12:55:31 +0000
@@ -152,6 +152,9 @@
}
setup_add_attachment();
setup_link_branch_picker();
+ if (batched_comment_loading_enabled) {
+ namespace.setup_load_comments();
+ }
}, window);
};
@@ -1078,6 +1081,73 @@
});
}
+/**
+ * Load more comments
+ * @method load_more_comments
+ * @param batched_comments_url {String} The URL from which to load
+ * comments.
+ * @param comments_container {Node} The Node into which to place the
+ * comments.
+ */
+namespace.load_more_comments = function(batched_comments_url,
+ comments_container) {
+ var spinner = Y.Node.create(
+ '<img src="/@@/spinner" style="text_align: center; display: none" />');
+ var spinner_span = Y.one('#more-comments-spinner');
+ spinner_span.setStyle('display', 'inline');
+ var handlers = {
+ success: function(transactionid, response, arguments) {
+ var new_comments_node =
+ Y.Node.create("<div></div>");
+ new_comments_node.set(
+ 'innerHTML', response.responseText);
+ spinner_span.setStyle('display', 'none');
+ comments_container.appendChild(new_comments_node);
+ var success_anim = Y.lp.anim.green_flash(
+ {node: new_comments_node});
+ success_anim.run();
+ batch_url_div = Y.one('#next-batch-url');
+ if (Y.Lang.isValue(batch_url_div)) {
+ batched_comments_url = batch_url_div.get(
+ 'innerHTML');
+ batch_url_div.remove();
+ namespace.load_more_comments(
+ batched_comments_url, comments_container);
+ } else {
+ // Remove the comments-hidden message to avoid
+ // confusion.
+ Y.one('#comments-hidden-message').remove();
+ }
+ }
+ };
+ var request = Y.io(batched_comments_url, {on: handlers});
+}
+
+
+/**
+ * Set up click handling to load the rest of the comments for the bug
+ * via javascript.
+ *
+ * @method setup_load_comments
+ */
+namespace.setup_load_comments = function() {
+ var show_comments_link = Y.one('#show-comments-link');
+ if (Y.Lang.isValue(show_comments_link)) {
+ var current_offset = LP.cache.initial_comment_batch_offset;
+ var batched_comments_url =
+ LP.cache.context.self_link.replace('/api/devel', '') +
+ "/+batched-comments?offset=" +
+ current_offset;
+ var comments_container = Y.one('#comments-container');
+ show_comments_link.on('click', function(e) {
+ e.preventDefault();
+ namespace.load_more_comments(
+ batched_comments_url, comments_container);
+ });
+ show_comments_link.addClass('js-action');
+ }
+}
+
}, "0.1", {"requires": ["base", "oop", "node", "event", "io-base",
"json-parse", "substitute", "widget-position-ext",
=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
--- lib/lp/bugs/templates/bugcomment-macros.pt 2011-05-31 16:48:39 +0000
+++ lib/lp/bugs/templates/bugcomment-macros.pt 2011-08-30 12:55:31 +0000
@@ -49,29 +49,36 @@
<div
xmlns:tal="http://xml.zope.org/namespaces/tal";
xmlns:metal="http://xml.zope.org/namespaces/metal";
- metal:define-macro="break"
- tal:attributes="class string:boardComment"
- style="border-bottom: 0">
- <div class="boardCommentDetails">
- <table>
- <tbody>
- <tr>
- <td>
- <span class="sprite arrowRight"> </span>
- <span tal:replace="num_hidden">42</span> comments hidden
- </td>
+ metal:define-macro="break">
+ <div id="comments-container"></div>
+ <div class="boardComment" style="border-bottom: 0"
+ id="comments-hidden-message">
+ <div class="boardCommentDetails">
+ <table>
+ <tbody>
+ <tr>
+ <td>
+ <span class="sprite arrowRight"> </span>
+ <span tal:replace="num_hidden">42</span> comments hidden
+ <span id="more-comments-spinner" style="display: none;">
+ Loading more comments
+ <img src="/@@/spinner" />
+ </span>
+ </td>
- <td class="bug-comment-index">
- <a href="?comments=all"
- class="sprite retry"
- style="white-space: nowrap">
- view all <span
- tal:replace="view/total_comments"
- /> comments</a>
- </td>
- </tr>
- </tbody>
- </table>
+ <td class="bug-comment-index">
+ <a href="?comments=all"
+ id="show-comments-link"
+ class="sprite retry"
+ style="white-space: nowrap">
+ view all <span
+ tal:replace="view/total_comments"
+ /> comments</a>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ </div>
</div>
</div>
=== added file 'lib/lp/bugs/templates/bugtask-batched-comments.pt'
--- lib/lp/bugs/templates/bugtask-batched-comments.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/templates/bugtask-batched-comments.pt 2011-08-30 12:55:31 +0000
@@ -0,0 +1,37 @@
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ omit-tag="">
+
+ <tal:comments>
+ <div
+ style="display: none"
+ id="next-batch-url"
+ tal:condition="view/has_more_comments_and_activity"
+ tal:content="view/next_batch_url"></div>
+ <tal:comment
+ repeat="comment_or_activity view/activity_and_comments">
+ <tal:is-comment
+ define="comment comment_or_activity/comment|nothing"
+ condition="comment">
+ <tal:comment-box replace="structure comment/@@+box" />
+ </tal:is-comment>
+
+ <tal:is-activity
+ define="activity_list comment_or_activity/activity|nothing;
+ activity_date comment_or_activity/date|nothing;
+ activity_person comment_or_activity/person|nothing"
+ condition="activity_list">
+ <metal:comment-box
+ metal:use-macro="context/@@bugcomment-macros/activity-box" />
+ </tal:is-activity>
+
+ <tal:is-break
+ define="num_hidden comment_or_activity/num_hidden|nothing"
+ condition="num_hidden">
+ <metal:comment-box
+ metal:use-macro="context/@@bugcomment-macros/break" />
+ </tal:is-break>
+ </tal:comment>
+ </tal:comments>
+</tal:root>
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2011-08-04 14:16:05 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2011-08-30 12:55:31 +0000
@@ -45,6 +45,18 @@
var privacy_notification_enabled = false;
</script>
</tal:if>
+ <tal:if
+ condition="request/features/bugs.batched_comment_loading.enabled">
+ <script type="text/javascript">
+ var batched_comment_loading_enabled = true;
+ </script>
+ </tal:if>
+ <tal:else
+ condition="not: request/features/bugs.batched_comment_loading.enabled">
+ <script type="text/javascript">
+ var batched_comment_loading_enabled = false;
+ </script>
+ </tal:else>
<style type="text/css">
/* Align the 'add comment' link to the right of the comment box. */
#add-comment-form textarea { width: 100%; }
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-08-30 00:07:34 +0000
+++ lib/lp/testing/factory.py 2011-08-30 12:55:31 +0000
@@ -2282,16 +2282,18 @@
return bmp.nominateReviewer(candidate, bmp.registrant)
def makeMessage(self, subject=None, content=None, parent=None,
- owner=None):
+ owner=None, datecreated=None):
if subject is None:
subject = self.getUniqueString()
if content is None:
content = self.getUniqueString()
if owner is None:
owner = self.makePerson()
+ if datecreated is None:
+ datecreated = datetime.now(UTC)
rfc822msgid = self.makeUniqueRFC822MsgId()
message = Message(rfc822msgid=rfc822msgid, subject=subject,
- owner=owner, parent=parent)
+ owner=owner, parent=parent, datecreated=datecreated)
MessageChunk(message=message, sequence=1, content=content)
return message
Follow ups
