Merge lp:~allenap/launchpad/bug-activity-grouping-bug-353890 into lp:launchpad

[Gavin Panella]

[r=leonardr][ui=none][bug=353890] Group bug comments and activities that occur within a small time window. Previously these were grouped only when they happened at exactly the same moment.

This branch groups together related comments and activity for display
on bug pages.

Related means: a string of 2 or more activities by a single user
happening within a given time window (5 minutes for now). There can be
zero or one comments in any given string of related activities.

Previously only activities happening at exactly the same moment would
be grouped, but this doesn't work so well with the AJAXy stuff.

The steps I took:

- Break out the code that identifies interesting activities into a new
  interesting_activity property.

  I have added a single test for this property. I am considering
  testing this more extensively, but I'm not yet sure if I would be
  repeating work done elsewhere.

- Ditch the activity_by_date property.

- New function group_comments_with_activity() that takes a sequence
  (or iterable) of comments and another of activity and merges the
  two, working out what is related according to the rule defined
  earlier.

  This is tested exclusively using stubs, so it's very fast to
  test. Existing tests (which I have updated) already provide
  integration tests.

- The activity_and_comments property was updated to further process
  what comes out of group_comments_with_activity().

- A big pile of doctest changes and improvements.

[Gavin Panella (allenap)]

I also fixed some lint in utilities/format-imports.

[Gavin Panella (allenap)]

Ignore the last comment; wrong merge proposal.

[Leonard Richardson (leonardr)]

Approved with minor changes, as discussed on IRC.

On line 292, the lambda expression should just be moved into the comprehension. It doesn't add anything.

first_newest_comment is a confusing name. You suggested renaming newest_comments to recent_comments, and having oldest_recent_comment instead.

Typo on line 651: "no activities is"

test_common_actor_over_a_prolonged_time() would be easier to understand if you mentioned that the grouping window is 5 minutes.

test_interleaved_activity_with_comments_by_common_actor() has an inaccurate name since there is only one comment.

On line 878, &\x238594; => →

You also showed me, and I approve, this change to interesting_activity: http://paste.ubuntu.com/546305/

review: Approve

=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py	2010-09-24 22:30:48 +0000
+++ lib/lp/bugs/browser/bugcomment.py	2010-12-21 16:34:36 +0000
@@ -6,18 +6,24 @@
 __metaclass__ = type
 __all__ = [
     'BugComment',
+    'BugCommentBoxExpandedReplyView',
+    'BugCommentBoxView',
+    'BugCommentBreadcrumb',
     'BugCommentView',
-    'BugCommentBoxView',
-    'BugCommentBoxExpandedReplyView',
     'BugCommentXHTMLRepresentation',
-    'BugCommentBreadcrumb',
     'build_comments_from_chunks',
+    'group_comments_with_activity',
     ]
 
 from datetime import (
     datetime,
     timedelta,
     )
+from itertools import (
+    chain,
+    groupby,
+    )
+from operator import itemgetter
 
 from lazr.restful.interfaces import IWebServiceClientRequest
 from pytz import utc
@@ -44,7 +50,9 @@
     IBugComment,
     IBugMessageSet,
     )
-from lp.registry.interfaces.person import IPersonSet
+
+
+COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5)
 
 
 def build_comments_from_chunks(chunks, bugtask, truncate=False):
@@ -87,6 +95,72 @@
     return comments
 
 
+def group_comments_with_activity(comments, activities):
+    """Group comments and activity together for human consumption.
+
+    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 activities: An iterable of `BugActivity` instances.
+    """
+    window = COMMENT_ACTIVITY_GROUPING_WINDOW
+
+    comment_kind = "comment"
+    comments = (
+        (comment.datecreated, comment.owner, comment_kind, comment)
+        for comment in comments)
+    activity_kind = "activity"
+    activity = (
+        (activity.datechanged, activity.person, activity_kind, activity)
+        for activity in activities)
+    events = sorted(chain(comments, activity), key=itemgetter(0, 1))
+
+    def gen_event_windows(events):
+        """Generate event windows.
+
+        Yields `(window_index, kind, event)` tuples, where `window_index` is
+        an integer, and is incremented each time the windowing conditions are
+        triggered.
+
+        :param events: An iterable of `(date, 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:
+            window_ended = (
+                # A window may contain only one comment.
+                (window_comment is not None and kind is comment_kind) or
+                # All events must have happened within a given timeframe.
+                (window_end is None or date >= window_end) or
+                # All events within the window must belong to the same actor.
+                (window_actor is None or actor != window_actor))
+            if window_ended:
+                window_comment, window_actor = None, actor
+                window_index, window_end = window_index + 1, date + window
+            if kind is comment_kind:
+                window_comment = event
+            yield window_index, kind, event
+
+    event_windows = gen_event_windows(events)
+    event_windows_grouper = groupby(event_windows, itemgetter(0))
+    for window_index, window_group in event_windows_grouper:
+        window_group = [
+            (kind, event) for (index, kind, event) in window_group]
+        for kind, event in window_group:
+            if kind is comment_kind:
+                window_comment = event
+                window_comment.activity.extend(
+                    event for (kind, event) in window_group
+                    if kind is activity_kind)
+                yield window_comment
+                # There's only one comment per window.
+                break
+        else:
+            yield [event for (kind, event) in window_group]
+
+
 class BugComment:
     """Data structure that holds all data pertaining to a bug comment.
 
@@ -208,10 +282,17 @@
         obfuscated for unauthenticated users.
         """
         now = datetime.now(tz=utc)
-        # The major factor in how long we can cache a bug comment is
-        # the timestamp. The rendering of the timestamp changes every
-        # minute for the first hour because we say '7 minutes ago'.
-        if self.datecreated > now - timedelta(hours=1):
+
+        # The major factor in how long we can cache a bug comment is the
+        # timestamp. For up to 5 minutes comments and activity can be grouped
+        # together as related, so do not cache.
+        if self.datecreated > now - COMMENT_ACTIVITY_GROUPING_WINDOW:
+            # Don't return 0 because that indicates no time limit.
+            return -1
+
+        # The rendering of the timestamp changes every minute for the first
+        # hour because we say '7 minutes ago'.
+        elif self.datecreated > now - timedelta(hours=1):
             return 60
 
         # Don't cache for long if we are waiting for synchronization.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-11-24 10:10:07 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-12-21 16:34:36 +0000
@@ -11,12 +11,14 @@
     'BugListingPortletInfoView',
     'BugListingPortletStatsView',
     'BugNominationsView',
+    'BugsBugTaskSearchListingView',
     'bugtarget_renderer',
     'BugTargetTraversalMixin',
     'BugTargetView',
+    'bugtask_heat_html',
+    'BugTaskBreadcrumb',
     'BugTaskContextMenu',
     'BugTaskCreateQuestionView',
-    'BugTaskBreadcrumb',
     'BugTaskEditView',
     'BugTaskExpirableListingView',
     'BugTaskListingItem',
@@ -25,22 +27,20 @@
     'BugTaskPortletView',
     'BugTaskPrivacyAdapter',
     'BugTaskRemoveQuestionView',
+    'BugTasksAndNominationsView',
     'BugTaskSearchListingView',
     'BugTaskSetNavigation',
     'BugTaskStatusView',
     'BugTaskTableRowView',
     'BugTaskTextView',
     'BugTaskView',
-    'BugTasksAndNominationsView',
-    'bugtask_heat_html',
-    'BugsBugTaskSearchListingView',
     'calculate_heat_display',
-    'NominationsReviewTableBatchNavigatorView',
-    'TextualBugTaskSearchListingView',
     'get_buglisting_search_filter_url',
     'get_comments_for_bugtask',
     'get_sortorder_from_request',
     'get_visible_comments',
+    'NominationsReviewTableBatchNavigatorView',
+    'TextualBugTaskSearchListingView',
     ]
 
 import cgi
@@ -48,14 +48,15 @@
     datetime,
     timedelta,
     )
+from itertools import (
+    chain,
+    groupby,
+    )
 from math import (
     floor,
     log,
     )
-from operator import (
-    attrgetter,
-    itemgetter,
-    )
+from operator import attrgetter
 import re
 import urllib
 
@@ -75,7 +76,7 @@
     IWebServiceClientRequest,
     )
 from lazr.uri import URI
-import pytz
+from pytz import utc
 from simplejson import dumps
 from z3c.ptcompat import ViewPageTemplateFile
 from zope import (
@@ -203,7 +204,10 @@
     BugTextView,
     BugViewMixin,
     )
-from lp.bugs.browser.bugcomment import build_comments_from_chunks
+from lp.bugs.browser.bugcomment import (
+    build_comments_from_chunks,
+    group_comments_with_activity,
+    )
 from lp.bugs.interfaces.bug import (
     IBug,
     IBugSet,
@@ -812,62 +816,20 @@
         return comments
 
     @cachedproperty
-    def activity_by_date(self):
-        """Return a dict of `BugActivityItem`s for the current bug.
-
-        The `BugActivityItem`s will be keyed by the date on which they
-        occurred.
-        """
-        activity_by_date = {}
+    def interesting_activity(self):
+        """A sequence of interesting bug activity."""
+        bug_change_re = (
+            'affects|description|security vulnerability|'
+            'summary|tags|visibility')
         bugtask_change_re = (
             '[a-z0-9][a-z0-9\+\.\-]+( \([A-Za-z0-9\s]+\))?: '
             '(assignee|importance|milestone|status)')
-        interesting_changes = [
-             'description',
-             'security vulnerability',
-             'summary',
-             'tags',
-             'visibility',
-             'affects',
-             bugtask_change_re,
-             ]
-
-        # Turn the interesting_changes list into a regex so that we can
-        # do complex matches.
-        interesting_changes_expression = "|".join(interesting_changes)
-        interesting_changes_regex = re.compile(
-            "^(%s)$" % interesting_changes_expression)
-
-        for activity in self.context.bug.activity:
-            # If we're not interested in the change, skip it.
-            if interesting_changes_regex.match(activity.whatchanged) is None:
-                continue
-
-            activity = BugActivityItem(activity)
-            if activity.datechanged not in activity_by_date:
-                activity_by_date[activity.datechanged] = {
-                    activity.target: [activity]}
-            else:
-                activity_dict = activity_by_date[activity.datechanged]
-                if activity.target in activity_dict:
-                    activity_dict[activity.target].append(activity)
-                else:
-                    activity_dict[activity.target] = [activity]
-
-        # Sort all the lists to ensure that changes are written out in
-        # alphabetical order.
-        for date, activity_by_target in activity_by_date.items():
-            # We convert each {target: activity_list} mapping into a list
-            # of {target, activity_list} dicts for the sake of making
-            # them usable in templates.
-            activity_by_date[date] = [{
-                'target': target,
-                'activity': sorted(
-                    activity_list, key=attrgetter('attribute')),
-                }
-                for target, activity_list in activity_by_target.items()]
-
-        return activity_by_date
+        interesting_match = re.compile(
+            "^(%s|%s)$" % (bug_change_re, bugtask_change_re)).match
+        return tuple(
+            BugActivityItem(activity)
+            for activity in self.context.bug.activity
+            if interesting_match(activity.whatchanged) is not None)
 
     @cachedproperty
     def activity_and_comments(self):
@@ -878,66 +840,82 @@
         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 newest and oldest is marked
-        by an entry in the list with the key 'num_hidden' defined.
+        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.
         """
-        activity_and_comments = []
-        activity_log = self.activity_by_date
-
         # Ensure truncation results in < max_length comments as expected
         assert(config.malone.comments_list_truncate_oldest_to
                + config.malone.comments_list_truncate_newest_to
                < config.malone.comments_list_max_length)
 
-        newest_comments = self.visible_newest_comments_for_display
+        recent_comments = self.visible_recent_comments_for_display
         oldest_comments = self.visible_oldest_comments_for_display
 
-        # Oldest comments and activities
-        for comment in oldest_comments:
-            # Move any corresponding activities into the comment
-            comment.activity = activity_log.pop(comment.datecreated, [])
-            comment.activity.sort(key=itemgetter('target'))
-
-            activity_and_comments.append({
-                'comment': comment,
-                'date': comment.datecreated,
-                })
-
-        # Insert blank if we're showing only a subset of the comment list
-        if len(newest_comments) > 0:
-            activity_and_comments.append({
-                'num_hidden': (len(self.visible_comments)
-                               - len(oldest_comments)
-                               - len(newest_comments)),
-                'date': newest_comments[0].datecreated,
-                })
-
-        # Most recent comments and activities (if showing a subset)
-        for comment in newest_comments:
-            # Move any corresponding activities into the comment
-            comment.activity = activity_log.pop(comment.datecreated, [])
-            comment.activity.sort(key=itemgetter('target'))
-
-            activity_and_comments.append({
-                'comment': comment,
-                'date': comment.datecreated,
-                })
-
-        # Add the remaining activities not associated with any visible
-        # comments to the activity_for_comments list.  For each
-        # activity dict we use the person responsible for the first
-        # activity item as the owner of the list of activities.
-        for date, activity_dict in activity_log.items():
-            activity_and_comments.append({
-                'activity': activity_dict,
-                'date': date,
-                'person': activity_dict[0]['activity'][0].person,
-                })
-
-        activity_and_comments.sort(key=itemgetter('date'))
-        return activity_and_comments
+        event_groups = group_comments_with_activity(
+            comments=chain(oldest_comments, recent_comments),
+            activities=self.interesting_activity)
+
+        def group_activities_by_target(activities):
+            activities = sorted(
+                activities, key=attrgetter(
+                    "datechanged", "target", "attribute"))
+            return [
+                {"target": target, "activity": list(activity)}
+                for target, activity in groupby(
+                    activities, attrgetter("target"))]
+
+        def comment_event_dict(comment):
+            actors = set(activity.person for activity in comment.activity)
+            actors.add(comment.owner)
+            assert len(actors) == 1, actors
+            dates = set(activity.datechanged for activity in comment.activity)
+            dates.add(comment.datecreated)
+            comment.activity = group_activities_by_target(comment.activity)
+            return {
+                "comment": comment,
+                "date": min(dates),
+                "person": actors.pop(),
+                }
+
+        def activity_event_dict(activities):
+            actors = set(activity.person for activity in activities)
+            assert len(actors) == 1, actors
+            dates = set(activity.datechanged for activity in activities)
+            return {
+                "activity": group_activities_by_target(activities),
+                "date": min(dates),
+                "person": actors.pop(),
+                }
+
+        def event_dict(event_group):
+            if isinstance(event_group, list):
+                return activity_event_dict(event_group)
+            else:
+                return comment_event_dict(event_group)
+
+        events = map(event_dict, event_groups)
+
+        # Insert blank if we're showing only a subset of the comment list.
+        if len(recent_comments) > 0:
+            # 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))
+                    separator = {
+                        'date': oldest_recent_comment.datecreated,
+                        'num_hidden': num_hidden,
+                        }
+                    events.insert(index, separator)
+                    break
+
+        return events
 
     @cachedproperty
     def visible_comments(self):
@@ -965,14 +943,13 @@
             return self.visible_comments[:oldest_count]
 
     @cachedproperty
-    def visible_newest_comments_for_display(self):
-        """The list of newest visible comments to be rendered.
+    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 newest 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.
+        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
@@ -1010,7 +987,7 @@
 
         expire_after = timedelta(days=config.malone.days_before_expiration)
         expiration_date = self.context.bug.date_last_updated + expire_after
-        remaining_time = expiration_date - datetime.now(pytz.timezone('UTC'))
+        remaining_time = expiration_date - datetime.now(utc)
         return remaining_time.days
 
     @property

=== modified file 'lib/lp/bugs/browser/tests/bug-views.txt'
--- lib/lp/bugs/browser/tests/bug-views.txt	2010-12-20 23:13:36 +0000
+++ lib/lp/bugs/browser/tests/bug-views.txt	2010-12-21 16:34:36 +0000
@@ -760,16 +760,6 @@
     >>> view = getMultiAdapter(
     ...     (bug.bugtasks[0], request), name="+index")
 
-The activity_by_date property of BugTaskView returns a dict of
-BugActivityItems grouped by the date on which they occurred. The items
-are returned as a list of dicts containig a list of activity items for a
-given target.
-
-    >>> print pretty(view.activity_by_date)
-    {datetime.datetime(2009, 3, 26...):
-    [{'activity': [<...BugActivityItem...>],
-    'target': None}]}
-
 The activity_and_comments property of BugTaskView is a list of comments
 and activity on a bug, ordered by the date that they occurred. Each item
 is encapsulated in a dict, in the form: {'comment': <BugComment>} or
@@ -793,28 +783,40 @@
     ...     (bug.bugtasks[0], request), name="+index")
     >>> view.initialize()
 
-    >>> activity_and_comments = view.activity_and_comments
-    >>> for activity_or_comment in activity_and_comments:
-    ...     if 'activity' in activity_or_comment:
-    ...         activity_dict = activity_or_comment['activity']
-    ...         for activity_for_target in activity_dict:
-    ...             target_name = activity_for_target['target']
-    ...             activity_items = activity_for_target['activity']
-    ...             if target_name is not None:
-    ...                 print "Changed in %s:" % target_name
-    ...             for activity_item in activity_items:
-    ...                 print "%s: %s => %s" % (
-    ...                     activity_item.change_summary,
-    ...                     activity_item.oldvalue,
-    ...                     activity_item.newvalue)
-    ...
-    ...     if 'comment' in activity_or_comment:
-    ...         comment = activity_or_comment['comment']
-    ...         print comment.text_for_display
-    summary: A bug title => A new bug title
+    >>> def print_activities(activities):
+    ...     for activity in activities:
+    ...         target_name = activity['target']
+    ...         if target_name is None:
+    ...             print "Changed:"
+    ...         else:
+    ...             print "Changed in %s:" % target_name
+    ...         activity_items = activity['activity']
+    ...         for activity_item in activity_items:
+    ...             print "* %s: %s => %s" % (
+    ...                 activity_item.change_summary,
+    ...                 activity_item.oldvalue,
+    ...                 activity_item.newvalue)
+
+    >>> def print_comment(comment):
+    ...     print comment.text_for_display
+    ...     print_activities(comment.activity)
+
+    >>> def print_activity_and_comments(activity_and_comments):
+    ...     for activity_or_comment in activity_and_comments:
+    ...         print "-- {person.name} --".format(**activity_or_comment)
+    ...         if 'activity' in activity_or_comment:
+    ...             print_activities(activity_or_comment["activity"])
+    ...         if 'comment' in activity_or_comment:
+    ...             print_comment(activity_or_comment["comment"])
+
+    >>> print_activity_and_comments(view.activity_and_comments)
+    -- name16 --
+    Changed:
+    * summary: A bug title => A new bug title
+    -- name16 --
     A comment, for the reading of.
     Changed in testproduct:
-                    status: New => Confirmed
+    * status: New => Confirmed
 
 If a comment and a BugActivity item occur at the same time, the activity
 item will be returned in the comment's activity property rather than as
@@ -841,36 +843,17 @@
 before, plus the new comment, since the changes we just made were
 grouped with that comment.
 
-    >>> activity_and_comments = view.activity_and_comments
-    >>> for activity_or_comment in activity_and_comments:
-    ...     if 'activity' in activity_or_comment:
-    ...         for activity_dict in activity_or_comment['activity']:
-    ...             for activity in activity_dict['activity']:
-    ...                 print "%s: %s => %s" % (
-    ...                     activity.whatchanged, activity.oldvalue,
-    ...                     activity.newvalue)
-    ...     if 'comment' in activity_or_comment:
-    ...         comment = activity_or_comment['comment']
-    ...         print comment.text_for_display
-    summary: A bug title => A new bug title
+    >>> print_activity_and_comments(view.activity_and_comments)
+    -- name16 --
+    Changed:
+    * summary: A bug title => A new bug title
+    -- name16 --
     A comment, for the reading of.
+    -- name16 --
     I triaged it.
-
-But if we look at the most recent comment's activity property we'll see
-that the activity recorded there reflects the changes that go with the
-comment.
-
-    >>> comment = activity_and_comments[-1]['comment']
-    >>> for activity_dict in comment.activity:
-    ...     if activity_dict['target'] is not None:
-    ...         print "Changed in %s:" % activity_dict['target']
-    ...     for activity in activity_dict['activity']:
-    ...         print "%s: %s => %s" % (
-    ...             activity.attribute, activity.oldvalue,
-    ...             activity.newvalue)
     Changed in testproduct:
-                importance: Undecided => High
-                    status: New => Confirmed
+    * importance: Undecided => High
+    * status: New => Confirmed
 
 
 Getting the list of possible duplicates for a new bug

=== added file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2010-12-21 16:34:36 +0000
@@ -0,0 +1,178 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the bugcomment module."""
+
+__metaclass__ = type
+
+from datetime import (
+    datetime,
+    timedelta,
+    )
+from itertools import count
+
+from pytz import utc
+
+from lp.bugs.browser.bugcomment import group_comments_with_activity
+from lp.testing import TestCase
+
+
+class BugActivityStub:
+
+    def __init__(self, datechanged, person=None):
+        self.datechanged = datechanged
+        if person is None:
+            person = PersonStub()
+        self.person = person
+
+    def __repr__(self):
+        return "BugActivityStub(%r, %r)" % (
+            self.datechanged.strftime('%Y-%m-%d--%H%M'), self.person)
+
+
+class BugCommentStub:
+
+    def __init__(self, datecreated, owner=None):
+        self.datecreated = datecreated
+        if owner is None:
+            owner = PersonStub()
+        self.owner = owner
+        self.activity = []
+
+    def __repr__(self):
+        return "BugCommentStub(%r, %r)" % (
+            self.datecreated.strftime('%Y-%m-%d--%H%M'), self.owner)
+
+
+class PersonStub:
+
+    ids = count(1)
+
+    def __init__(self):
+        self.id = next(self.ids)
+
+    def __repr__(self):
+        return "PersonStub#%d" % self.id
+
+
+class TestGroupCommentsWithActivities(TestCase):
+    """Tests for `group_comments_with_activities`."""
+
+    def setUp(self):
+        super(TestGroupCommentsWithActivities, self).setUp()
+        self.now = datetime.now(utc)
+        self.timestamps = (
+            self.now + timedelta(minutes=counter)
+            for counter in count(1))
+
+    def group(self, comments, activities):
+        return list(
+            group_comments_with_activity(
+                comments=comments, activities=activities))
+
+    def test_empty(self):
+        # Given no comments or activities the result is also empty.
+        self.assertEqual(
+            [], self.group(comments=[], activities=[]))
+
+    def test_activity_empty_no_common_actor(self):
+        # When no activities are passed in, and the comments passed in don't
+        # have any common actors, no grouping is possible.
+        comments = [
+            BugCommentStub(next(self.timestamps))
+            for number in xrange(5)]
+        self.assertEqual(
+            comments, self.group(comments=comments, activities=[]))
+
+    def test_comments_empty_no_common_actor(self):
+        # When no comments are passed in, and the activities passed in don't
+        # have any common actors, no grouping is possible.
+        activities = [
+            BugActivityStub(next(self.timestamps))
+            for number in xrange(5)]
+        self.assertEqual(
+            [[activity] for activity in activities], self.group(
+                comments=[], activities=activities))
+
+    def test_no_common_actor(self):
+        # When each activities and comment given has a different actor then no
+        # grouping is possible.
+        activity1 = BugActivityStub(next(self.timestamps))
+        comment1 = BugCommentStub(next(self.timestamps))
+        activity2 = BugActivityStub(next(self.timestamps))
+        comment2 = BugCommentStub(next(self.timestamps))
+
+        activities = set([activity1, activity2])
+        comments = set([comment1, comment2])
+
+        self.assertEqual(
+            [[activity1], comment1, [activity2], comment2],
+            self.group(comments=comments, activities=activities))
+
+    def test_comment_then_activity_close_by_common_actor(self):
+        # An activity shortly after a comment by the same person is grouped
+        # into the comment.
+        actor = PersonStub()
+        comment = BugCommentStub(next(self.timestamps), actor)
+        activity = BugActivityStub(next(self.timestamps), actor)
+        grouped = self.group(comments=[comment], activities=[activity])
+        self.assertEqual([comment], grouped)
+        self.assertEqual([activity], comment.activity)
+
+    def test_activity_then_comment_close_by_common_actor(self):
+        # An activity shortly before a comment by the same person is grouped
+        # into the comment.
+        actor = PersonStub()
+        activity = BugActivityStub(next(self.timestamps), actor)
+        comment = BugCommentStub(next(self.timestamps), actor)
+        grouped = self.group(comments=[comment], activities=[activity])
+        self.assertEqual([comment], grouped)
+        self.assertEqual([activity], comment.activity)
+
+    def test_interleaved_activity_with_comment_by_common_actor(self):
+        # Activities shortly before and after a comment are grouped into the
+        # comment's activity.
+        actor = PersonStub()
+        activity1 = BugActivityStub(next(self.timestamps), actor)
+        comment = BugCommentStub(next(self.timestamps), actor)
+        activity2 = BugActivityStub(next(self.timestamps), actor)
+        grouped = self.group(
+            comments=[comment], activities=[activity1, activity2])
+        self.assertEqual([comment], grouped)
+        self.assertEqual([activity1, activity2], comment.activity)
+
+    def test_common_actor_over_a_prolonged_time(self):
+        # There is a timeframe for grouping events, 5 minutes by default.
+        # Anything outside of that window is considered separate.
+        actor = PersonStub()
+        activities = [
+            BugActivityStub(next(self.timestamps), actor)
+            for count in xrange(8)]
+        grouped = self.group(comments=[], activities=activities)
+        self.assertEqual(2, len(grouped))
+        self.assertEqual(activities[:5], grouped[0])
+        self.assertEqual(activities[5:], grouped[1])
+
+    def test_two_comments_by_common_actor(self):
+        # Only one comment will ever appear in a group.
+        actor = PersonStub()
+        comment1 = BugCommentStub(next(self.timestamps), actor)
+        comment2 = BugCommentStub(next(self.timestamps), actor)
+        grouped = self.group(comments=[comment1, comment2], activities=[])
+        self.assertEqual([comment1, comment2], grouped)
+
+    def test_two_comments_with_activity_by_common_actor(self):
+        # Activity gets associated with earlier comment when all other factors
+        # are unchanging.
+        actor = PersonStub()
+        activity1 = BugActivityStub(next(self.timestamps), actor)
+        comment1 = BugCommentStub(next(self.timestamps), actor)
+        activity2 = BugActivityStub(next(self.timestamps), actor)
+        comment2 = BugCommentStub(next(self.timestamps), actor)
+        activity3 = BugActivityStub(next(self.timestamps), actor)
+        grouped = self.group(
+            comments=[comment1, comment2],
+            activities=[activity1, activity2, activity3])
+        self.assertEqual([comment1, comment2], grouped)
+        self.assertEqual([activity1, activity2], comment1.activity)
+        self.assertEqual([activity3], comment2.activity)

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2010-10-26 15:47:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2010-12-21 16:34:36 +0000
@@ -3,13 +3,14 @@
 
 __metaclass__ = type
 
-
+from datetime import datetime
 from doctest import DocTestSuite
 import unittest
 
+from pytz import UTC
 from storm.store import Store
 from testtools.matchers import LessThan
-
+from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import (
@@ -31,7 +32,9 @@
     BugTaskEditView,
     BugTasksAndNominationsView,
     )
+from lp.bugs.interfaces.bugactivity import IBugActivitySet
 from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.services.propertycache import get_property_cache
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -87,6 +90,32 @@
             LessThan(count_with_no_teams + 3),
             ))
 
+    def test_interesting_activity(self):
+        # The interesting_activity property returns a tuple of interesting
+        # `BugActivityItem`s.
+        bug = self.factory.makeBug()
+        view = create_initialized_view(
+            bug.default_bugtask, name=u'+index', rootsite='bugs')
+
+        def add_activity(what, old=None, new=None, message=None):
+            getUtility(IBugActivitySet).new(
+                bug, datetime.now(UTC), bug.owner, whatchanged=what,
+                oldvalue=old, newvalue=new, message=message)
+            del get_property_cache(view).interesting_activity
+
+        # A fresh bug has no interesting activity.
+        self.assertEqual((), view.interesting_activity)
+
+        # Some activity is not considered interesting.
+        add_activity("boring")
+        self.assertEqual((), view.interesting_activity)
+
+        # A description change is interesting.
+        add_activity("description")
+        self.assertEqual(1, len(view.interesting_activity))
+        [activity] = view.interesting_activity
+        self.assertEqual("description", activity.whatchanged)
+
 
 class TestBugTasksAndNominationsView(TestCaseWithFactory):
 

=== modified file 'lib/lp/bugs/doc/bugcomment.txt'
--- lib/lp/bugs/doc/bugcomment.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bugcomment.txt	2010-12-21 16:34:36 +0000
@@ -285,9 +285,9 @@
     False
 
 When comments aren't being truncated and empty set will be returned by
-visible_newest_comments_for_display.
+visible_recent_comments_for_display.
 
-    >>> len(bug_view.visible_newest_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.
@@ -302,7 +302,7 @@
     True
     >>> len(bug_view.visible_oldest_comments_for_display)
     3
-    >>> len(bug_view.visible_newest_comments_for_display)
+    >>> len(bug_view.visible_recent_comments_for_display)
     5
 
 The display of all comments can be requested with a form parameter.

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2010-09-24 22:30:48 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2010-12-21 16:34:36 +0000
@@ -48,11 +48,11 @@
 Some bug activity is show interleaved with comments on a bug's main
 page.
 
-    >>> def print_comments(page):
+    >>> def print_comments(page, subset=slice(-1, None)):
     ...     """Print all the comments on the page."""
     ...     comment_divs = find_tags_by_class(page, 'boardComment')
-    ...     for div_tag in comment_divs:
-    ...         print extract_text(div_tag)
+    ...     for div_tag in comment_divs[subset]:
+    ...         print extract_text(div_tag).replace("&#8594;", "=>")
     ...         print '-' * 8
 
     >>> user_browser.open(
@@ -60,13 +60,13 @@
     >>> user_browser.getControl(name='field.comment').value = (
     ...     "Here's a comment for testing, like.")
     >>> user_browser.getControl('Post Comment').click()
-    >>> print_comments(user_browser.contents)
+    >>> print_comments(user_browser.contents, slice(None))
     In...
     Bug Watch Updater
     on 2007-12-18
     Changed in thunderbird:
     status:
-    Unknown &#8594; New
+    Unknown => New
     --------
     No Privileges Person
     ...
@@ -84,7 +84,7 @@
 that have been added.
 
     >>> user_browser.open('http://launchpad.dev/bugs/15')
-    >>> print_comments(user_browser.contents)
+    >>> print_comments(user_browser.contents, slice(None))
     In...
     Bug
     ...
@@ -105,16 +105,19 @@
 
     >>> admin_browser.open('http://launchpad.dev/bugs/15')
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
-    ...
-    --------
-    Foo Bar ... ago
-    visibility: public  &#8594; private
+    Foo Bar
+    ... ago
+    summary:
+    - Nonsensical bugs are useless
+    + A new title for this bug
+    visibility:
+    public => private
     --------
 
-Changes to a bug's security_related field will be displayed as 'security
-vulnerability' changes, with values of 'yes' or 'no'.
+Changes to a bug's security_related field will be displayed as
+'security vulnerability' changes, with values of 'yes' or 'no'. Note
+that changes are grouped together if they occur at similar times, but
+are still shown in the order the changes were made.
 
     >>> admin_browser.open(
     ...     'http://bugs.launchpad.dev/redfish/+bug/15/+secrecy')
@@ -124,33 +127,12 @@
 
     >>> admin_browser.open('http://launchpad.dev/bugs/15')
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
-    ...
-    --------
-    Foo Bar ... ago
-    security vulnerability: no  &#8594; yes
-    --------
-
-Changes will be grouped together if they occur at the same time.
-
-    >>> admin_browser.open(
-    ...     'http://bugs.launchpad.dev/redfish/+bug/15/+secrecy')
-    >>> admin_browser.getControl(
-    ...     "This bug is a security vulnerability").selected = False
-    >>> admin_browser.getControl(
-    ...     "This bug report should be private").selected = False
-    >>> admin_browser.getControl("Change").click()
-
-    >>> admin_browser.open('http://launchpad.dev/bugs/15')
-    >>> print_comments(admin_browser.contents)
-    In...
-    Bug
-    ...
-    --------
-    Foo Bar ... ago
-    security vulnerability: yes &#8594; no
-    visibility: private  &#8594; public
+    Foo Bar
+    ... ago
+    summary:
+    ...
+    security vulnerability:
+    no => yes
     --------
 
 Changes to the bug's description will simply be displayed as 'description:
@@ -164,12 +146,12 @@
 
     >>> admin_browser.open('http://launchpad.dev/bugs/15')
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
+    Foo Bar
+    ... ago
+    summary:
     ...
-    --------
-    Foo Bar ... ago
-    description: updated
+    description:
+    updated
     --------
 
 Changes to the bug's tags will be show in the form tags removed or tags
@@ -183,13 +165,17 @@
 
     >>> admin_browser.open('http://launchpad.dev/bugs/15')
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
+    Foo Bar
+    ... ago
+    summary:
     ...
-    --------
-    Foo Bar ... ago
-    tags: added: tag1 tag2 tag3
-    --------
+    tags:
+    added: tag1 tag2 tag3
+    --------
+
+When two similar activities are grouped into the same comment - like
+two sets of tag changes - they are displayed in the order they were
+made.
 
     >>> admin_browser.open(
     ...     'http://bugs.launchpad.dev/redfish/+bug/15/+edit')
@@ -199,13 +185,15 @@
 
     >>> admin_browser.open('http://launchpad.dev/bugs/15')
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
+    Foo Bar
+    ... ago
+    summary:
     ...
-    --------
-    Foo Bar ... ago
-    tags: added: tag4
-        removed: tag3
+    tags:
+    added: tag1 tag2 tag3
+    tags:
+    added: tag4
+    removed: tag3
     --------
 
 Changes to a BugTask's attributes will show up listed under the task's
@@ -231,16 +219,19 @@
     >>> admin_browser.getControl("Save Changes").click()
 
     >>> print_comments(admin_browser.contents)
-    In...
-    Bug
+    Foo Bar
+    ... ago
+    summary:
     ...
-    --------
-    Foo Bar ... ago
     Changed in redfish:
-              assignee: nobody &#8594; Foo Bar (name16)
-            importance: Undecided &#8594; High
-             milestone: none &#8594; foo
-                status: New &#8594; Confirmed
+    assignee:
+    nobody => Foo Bar (name16)
+    importance:
+    Undecided => High
+    milestone:
+    none => foo
+    status:
+    New => Confirmed
     --------
 
 If a change is made to a bug task which is targeted to a distro source
@@ -252,13 +243,11 @@
     >>> admin_browser.getControl('Status').value = ['Confirmed']
     >>> admin_browser.getControl("Save Changes").click()
     >>> print_comments(admin_browser.contents)
-    Sample Person
-    ...
     Foo Bar
-    ...
+    ... ago
     Changed in mozilla-firefox (Ubuntu):
     status:
-    New &#8594; Confirmed
+    New => Confirmed
     --------
 
 If a change has a comment associated with it it will be displayed in the
@@ -273,18 +262,18 @@
     >>> admin_browser.getControl('Comment').value = "Lookit, a change!"
     >>> admin_browser.getControl("Save Changes").click()
     >>> print_comments(admin_browser.contents)
-    Sample Person
-    ...
     Foo Bar
     wrote
-    ...
+    ... ago:
     #2
     Lookit, a change!
     Changed in mozilla-firefox (Ubuntu):
+    status:
+    New => Confirmed
     importance:
-    Medium &#8594; Low
+    Medium => Low
     status:
-    Confirmed &#8594; New
+    Confirmed => New
     --------
 
 If a target of a bug task is changed the old and new value will be shown.
@@ -296,15 +285,11 @@
     ...     'Package').value = 'linux-source-2.6.15'
     >>> admin_browser.getControl("Save Changes").click()
     >>> print_comments(admin_browser.contents)
-    Sample Person
-    ...
     Foo Bar
     wrote
-    ...
-    --------
-    Foo Bar
+    ... ago:
+    #2
     ...
     affects:
-    mozilla-firefox (Ubuntu) &#8594; linux-source-2.6.15 (Ubuntu)
+    mozilla-firefox (Ubuntu) => linux-source-2.6.15 (Ubuntu)
     --------
-

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2010-10-26 21:26:27 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2010-12-21 16:34:36 +0000
@@ -312,7 +312,7 @@
             Displaying first <span
             tal:replace="view/visible_oldest_comments_for_display/count:len">23</span>
             and last <span
-            tal:replace="view/visible_newest_comments_for_display/count:len">32</span>
+            tal:replace="view/visible_recent_comments_for_display/count:len">32</span>
             comments.
             <tal:what-next
                 define="view_all_href