Merge lp:~abentley/launchpad/bugcomment-interface into lp:launchpad

[Aaron Bentley]

Make BugComment implement IBugComment

= Summary =
Make BugComment provide IBugComment

== Proposed fix ==
Delegate IMessage (a base class of IBugComment) to BugComment._message

== Pre-implementation notes ==
Discussed with deryck, discussed eager-loading approach with lifeless

== Implementation details ==
BugComment claimed to implement IMessage, but verifyObject showed it did not. The 'followup_title' and 'has_new_title' IMessage properties turned out to be unused, so I deleted them. Ultimately, I decided to modernize our code by delegating to Message.

This means BugComment no longer directly provides title, datecreated, owner, rfc822msgid, chunks and visible. It provides bugattachments because, unlike IMessage, BugComments do not consider patches to be BugAttachments.

This caused database queries to increase, because BugComment had been acting as an eager-loaded of MessageChunks and BugAttachments. To restore this eager loading, I converted Message.bugattachments and Message.chunks into cachedproperties. Then I could pre-populate them from the original call sites.

I also adapted the comment hiding and truncation functionality so that the parameters are supplied in the constructor, and applied in text_for_display.

== Tests ==
bin/test -v test_bugcomment

== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugcomment.py
  lib/lp/services/messages/model/message.py
  lib/lp/bugs/browser/bugcomment.py
  lib/lp/testing/factory.py
  lib/lp/services/messages/interfaces/message.py
  lib/lp/bugs/browser/bugtask.py

[Deryck Hodge (deryck)]

This looks fantastic, Aaron. This is a nice refactor, thanks!

review: Approve

=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/browser/bugcomment.py	2012-01-17 14:36:25 +0000
@@ -25,6 +25,7 @@
     )
 from operator import itemgetter
 
+from lazr.delegates import delegates
 from lazr.restful.interfaces import IWebServiceClientRequest
 from pytz import utc
 from zope.component import (
@@ -36,15 +37,22 @@
     implements,
     Interface,
     )
+from zope.security.proxy import removeSecurityProxy
 
+from lp.bugs.interfaces.bugattachment import BugAttachmentType
 from lp.bugs.interfaces.bugmessage import IBugComment
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.services.webapp import (
     canonical_url,
     LaunchpadView,
     )
+from lp.services.messages.interfaces.message import IMessage
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.interfaces import ILaunchBag
 
@@ -54,7 +62,7 @@
 
 def build_comments_from_chunks(
         bugtask, truncate=False, slice_info=None, show_spam_controls=False,
-        user=None):
+        user=None, hide_first=False):
     """Build BugComments from MessageChunks.
 
     :param truncate: Perform truncation of large messages.
@@ -64,11 +72,22 @@
     # This would be better as part of indexed_messages eager loading.
     comments = {}
     for bugmessage, message, chunk in chunks:
+        cache = get_property_cache(message)
+        if getattr(cache, 'chunks', None) is None:
+            cache.chunks = []
+        cache.chunks.append(removeSecurityProxy(chunk))
         bug_comment = comments.get(message.id)
         if bug_comment is None:
+            if bugmessage.index == 0 and hide_first:
+                display = 'hide'
+            elif truncate:
+                display = 'truncate'
+            else:
+                display = 'full'
             bug_comment = BugComment(
-                bugmessage.index, message, bugtask, visible=message.visible,
-                show_spam_controls=show_spam_controls, user=user)
+                bugmessage.index, message, bugtask,
+                show_spam_controls=show_spam_controls, user=user,
+                display=display)
             comments[message.id] = bug_comment
             # This code path is currently only used from a BugTask view which
             # has already loaded all the bug watches. If we start lazy loading
@@ -78,12 +97,6 @@
                 bug_comment.bugwatch = bugmessage.bugwatch
                 bug_comment.synchronized = (
                     bugmessage.remote_comment_id is not None)
-        bug_comment.chunks.append(chunk)
-
-    for comment in comments.values():
-        # 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
 
 
@@ -176,22 +189,19 @@
     """
     implements(IBugComment)
 
+    delegates(IMessage, '_message')
+
     def __init__(
             self, index, message, bugtask, activity=None,
-            visible=True, show_spam_controls=False, user=None):
+            show_spam_controls=False, user=None, display='full'):
 
         self.index = index
         self.bugtask = bugtask
         self.bugwatch = None
 
-        self.title = message.title
+        self._message = message
         self.display_title = False
-        self.datecreated = message.datecreated
-        self.owner = message.owner
-        self.rfc822msgid = message.rfc822msgid
 
-        self.chunks = []
-        self.bugattachments = []
         self.patches = []
 
         if activity is None:
@@ -200,13 +210,22 @@
         self.activity = activity
 
         self.synchronized = False
-        self.visible = visible
         # We use a feature flag to control users deleting their own comments.
         user_owns_comment = False
         flag = 'disclosure.users_hide_own_bug_comments.enabled'
         if bool(getFeatureFlag(flag)):
             user_owns_comment = user is not None and user == self.owner
         self.show_spam_controls = show_spam_controls or user_owns_comment
+        if display == 'truncate':
+            self.comment_limit = config.malone.max_comment_size
+        else:
+            self.comment_limit = None
+        self.hide_text = (display == 'hide')
+
+    @cachedproperty
+    def bugattachments(self):
+        return [attachment for attachment in self._message.bugattachments if
+         attachment.type != BugAttachmentType.PATCH]
 
     @property
     def show_for_admin(self):
@@ -220,31 +239,28 @@
         """
         return not self.visible
 
-    def setupText(self, truncate=False):
-        """Set the text for display and truncate it if necessary.
-
-        Note that this method must be called before either isIdenticalTo() or
-        isEmpty() are called, since to do otherwise would mean that they could
-        return false positives and negatives respectively.
-        """
-        comment_limit = config.malone.max_comment_size
-
-        bits = [unicode(chunk.content)
-                for chunk in self.chunks
-                if chunk.content is not None and len(chunk.content) > 0]
-        text = self.text_contents = '\n\n'.join(bits)
-
-        if truncate and comment_limit and len(text) > comment_limit:
-            # Note here that we truncate at comment_limit, and not
-            # comment_limit - 3; while it would be nice to account for
-            # the ellipsis, this breaks down when the comment limit is
-            # less than 3 (which can happen in a testcase) and it makes
-            # counting the strings harder.
-            self.text_for_display = "%s..." % text[:comment_limit]
-            self.was_truncated = True
-        else:
-            self.text_for_display = text
-            self.was_truncated = False
+    @property
+    def needs_truncation(self):
+        if self.comment_limit is None:
+            return False
+        return len(self.text_contents) > self.comment_limit
+
+    @property
+    def was_truncated(self):
+        return self.needs_truncation
+
+    @cachedproperty
+    def text_for_display(self):
+        if self.hide_text:
+            return ''
+        if not self.needs_truncation:
+            return self.text_contents
+        # Note here that we truncate at comment_limit, and not
+        # comment_limit - 3; while it would be nice to account for
+        # the ellipsis, this breaks down when the comment limit is
+        # less than 3 (which can happen in a testcase) and it makes
+        # counting the strings harder.
+        return "%s..." % self.text_contents[:self.comment_limit]
 
     def isIdenticalTo(self, other):
         """Compare this BugComment to another and return True if they are

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-01-16 17:20:45 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-01-17 14:36:25 +0000
@@ -347,9 +347,12 @@
     """
     comments = build_comments_from_chunks(bugtask, truncate=truncate,
         slice_info=slice_info, show_spam_controls=show_spam_controls,
-        user=user)
+        user=user, hide_first=for_display)
     # TODO: further fat can be shaved off here by limiting the attachments we
     # query to those that slice_info would include.
+    for comment in comments.values():
+        get_property_cache(comment._message).bugattachments = []
+
     for attachment in bugtask.bug.attachments_unpopulated:
         message_id = attachment.message.id
         # All attachments are related to a message, so we can be
@@ -359,8 +362,8 @@
             break
         if attachment.type == BugAttachmentType.PATCH:
             comments[message_id].patches.append(attachment)
-        else:
-            comments[message_id].bugattachments.append(attachment)
+        cache = get_property_cache(attachment.message)
+        cache.bugattachments.append(attachment)
     comments = sorted(comments.values(), key=attrgetter("index"))
     current_title = bugtask.bug.title
     for comment in comments:
@@ -371,12 +374,6 @@
             # 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
 
 

=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2012-01-17 14:36:25 +0000
@@ -126,7 +126,8 @@
     ... --boundary--
     ... """
     >>> import transaction
-    >>> from lp.services.temporaryblobstorage.interfaces import ITemporaryStorageManager
+    >>> from lp.services.temporaryblobstorage.interfaces import (
+    ...     ITemporaryStorageManager)
     >>> token = getUtility(ITemporaryStorageManager).new(debug_data)
 
 We need to commit the transaction since the data will be stored in the
@@ -167,7 +168,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     This should be added to the description.
@@ -226,7 +228,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     This should be added to the description.
@@ -331,7 +334,7 @@
 
     >>> for comment in filebug_view.added_bug.messages[1:]:
     ...     print "Comment by %s: %s attachment(s)" % (
-    ...         comment.owner.displayname, comment.bugattachments.count())
+    ...         comment.owner.displayname, len(comment.bugattachments))
     Comment by No Privileges Person: 2 attachment(s)
 
 
@@ -375,7 +378,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     This bug should be private.
@@ -438,7 +442,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     This bug should be public.
@@ -502,7 +507,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     Other people are interested in this bug.
@@ -572,7 +578,8 @@
     >>> filebug_view.added_bug.title
     u'Test Title'
 
-    >>> print filebug_view.added_bug.description #doctest: -NORMALIZE_WHITESPACE
+    >>> description = filebug_view.added_bug.description
+    >>> print description #doctest: -NORMALIZE_WHITESPACE
     Test description.
     <BLANKLINE>
     This bug should be private, and Mark Shuttleworth subscribed.

=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2012-01-17 14:36:25 +0000
@@ -20,18 +20,24 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
-from lp.bugs.browser.bugcomment import group_comments_with_activity
+from lp.bugs.interfaces.bugmessage import IBugComment
+from lp.bugs.browser.bugcomment import (
+    BugComment,
+    group_comments_with_activity,
+    )
 from lp.coop.answersbugs.visibility import (
     TestHideMessageControlMixin,
     TestMessageVisibilityMixin,
     )
 from lp.services.features.testing import FeatureFixture
+from lp.services.webapp.testing import verifyObject
 from lp.testing import (
     BrowserTestCase,
     celebrity_logged_in,
     login_person,
     person_logged_in,
     TestCase,
+    TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.pages import find_tag_by_id
@@ -313,3 +319,15 @@
                     itemprop='commentTime',
                     title=True,
                     datetime=iso_date))))
+
+
+class TestBugCommentImplementsInterface(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_bug_comment_implements_interface(self):
+        """Ensure BugComment implements IBugComment"""
+        bug_message = self.factory.makeBugComment()
+        bugtask = bug_message.bugs[0].bugtasks[0]
+        bug_comment = BugComment(1, bug_message, bugtask)
+        verifyObject(IBugComment, bug_comment)

=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
--- lib/lp/bugs/scripts/tests/test_bugimport.py	2012-01-05 00:23:45 +0000
+++ lib/lp/bugs/scripts/tests/test_bugimport.py	2012-01-17 14:36:25 +0000
@@ -38,7 +38,6 @@
     PersonCreationRationale,
     )
 from lp.registry.interfaces.product import IProductSet
-from lp.registry.model.person import generate_nick
 from lp.services.config import config
 from lp.services.database.sqlbase import cursor
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
@@ -532,7 +531,7 @@
             message1.datecreated.isoformat(), '2004-10-12T12:00:00+00:00')
         self.assertEqual(message1.subject, 'A test bug')
         self.assertEqual(message1.text_contents, 'Original description')
-        self.assertEqual(message1.bugattachments.count(), 1)
+        self.assertEqual(len(message1.bugattachments), 1)
         attachment = message1.bugattachments[0]
         self.assertEqual(attachment.type, BugAttachmentType.UNSPECIFIED)
         self.assertEqual(attachment.title, 'Hello')
@@ -558,7 +557,7 @@
             message3.text_contents,
             'A comment from mark about CVE-2005-2730\n\n'
             ' * list item 1\n * list item 2\n\nAnother paragraph')
-        self.assertEqual(message3.bugattachments.count(), 2)
+        self.assertEqual(len(message3.bugattachments), 2)
         # grab the attachments in the appropriate order
         [attachment1, attachment2] = list(message3.bugattachments)
         if attachment1.type == BugAttachmentType.PATCH:
@@ -583,7 +582,7 @@
             message4.datecreated.isoformat(), '2005-01-01T14:00:00+00:00')
         self.assertEqual(message4.subject, 'Re: A test bug')
         self.assertEqual(message4.text_contents, '<empty comment>')
-        self.assertEqual(message4.bugattachments.count(), 0)
+        self.assertEqual(len(message4.bugattachments), 0)
 
         # Message 5:
         self.assertEqual(
@@ -592,7 +591,7 @@
             message5.datecreated.isoformat(), '2005-01-01T15:00:00+00:00')
         self.assertEqual(message5.subject, 'Re: A test bug')
         self.assertEqual(message5.text_contents, '')
-        self.assertEqual(message5.bugattachments.count(), 1)
+        self.assertEqual(len(message5.bugattachments), 1)
         attachment = message5.bugattachments[0]
         self.assertEqual(attachment.type, BugAttachmentType.UNSPECIFIED)
         self.assertEqual(attachment.title, 'Hello')

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt	2011-12-30 07:38:46 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt	2012-01-17 14:36:25 +0000
@@ -2103,7 +2103,9 @@
     >>> sent_msg = email.message_from_string(raw_message)
     >>> failure_msg, original_msg = sent_msg.get_payload()
     >>> msg = original_msg.get_payload()[0]
-    >>> # Fudge due to new line added to the payload.
+
+Fudge due to new line added to the payload.
+
     >>> len(str(msg)) <= (max_return_size + 1)
     True
 
@@ -2367,7 +2369,7 @@
 criteria described below).
 
     >>> def print_attachments(attachments):
-    ...     if attachments.count() == 0:
+    ...     if len(list(attachments)) == 0:
     ...         print "No attachments"
     ...         return
     ...     commit()

=== modified file 'lib/lp/services/messages/interfaces/message.py'
--- lib/lp/services/messages/interfaces/message.py	2011-12-24 16:54:44 +0000
+++ lib/lp/services/messages/interfaces/message.py	2012-01-17 14:36:25 +0000
@@ -85,7 +85,7 @@
                     schema=ILibraryFileAlias, required=False, readonly=True)
     bugs = CollectionField(
         title=_('Bug List'),
-        value_type=Reference(schema=Interface)) # Redefined in bug.py
+        value_type=Reference(schema=Interface))  # Redefined in bug.py
 
     chunks = Attribute(_('Message pieces'))
 
@@ -94,16 +94,9 @@
                      'unicode string.')),
         exported_as='content')
 
-    followup_title = TextLine(
-        title=_('Candidate title for a followup message.'),
-        readonly=True)
     title = TextLine(
         title=_('The message title, usually just the subject.'),
         readonly=True)
-    has_new_title = Bool(
-        title=_("Whether or not the title of this message "
-                "is different to that of its parent."),
-        readonly=True)
     visible = Bool(title=u"This message is visible or not.", required=False,
         default=True)
 

=== modified file 'lib/lp/services/messages/model/message.py'
--- lib/lp/services/messages/model/message.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/messages/model/message.py	2012-01-17 14:36:25 +0000
@@ -126,10 +126,20 @@
     rfc822msgid = StringCol(notNull=True)
     bugs = SQLRelatedJoin('Bug', joinColumn='message', otherColumn='bug',
         intermediateTable='BugMessage')
-    chunks = SQLMultipleJoin('MessageChunk', joinColumn='message')
+    _chunks = SQLMultipleJoin('MessageChunk', joinColumn='message')
+
+    @cachedproperty
+    def chunks(self):
+        return list(self._chunks)
+
     raw = ForeignKey(foreignKey='LibraryFileAlias', dbName='raw',
                      default=None)
-    bugattachments = SQLMultipleJoin('BugAttachment', joinColumn='_message')
+    _bugattachments = SQLMultipleJoin('BugAttachment', joinColumn='_message')
+
+    @cachedproperty
+    def bugattachments(self):
+        return list(self._bugattachments)
+
     visible = BoolCol(notNull=True, default=True)
 
     def __repr__(self):
@@ -143,26 +153,11 @@
         self.visible = visible
 
     @property
-    def followup_title(self):
-        """See IMessage."""
-        if self.title.lower().startswith('re: '):
-            return self.title
-        return 'Re: ' + self.title
-
-    @property
     def title(self):
         """See IMessage."""
         return self.subject
 
     @property
-    def has_new_title(self):
-        """See IMessage."""
-        if self.parent is None:
-            return True
-        return self.title.lower().lstrip('re:').strip() != \
-        self.parent.title.lower().lstrip('re:').strip()
-
-    @property
     def sender(self):
         """See IMessage."""
         return self.owner

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-01-15 11:50:42 +0000
+++ lib/lp/testing/factory.py	2012-01-17 14:36:25 +0000
@@ -1944,9 +1944,10 @@
             subject = self.getUniqueString()
         if body is None:
             body = self.getUniqueString()
-        return bug.newMessage(owner=owner, subject=subject,
-                              content=body, parent=None, bugwatch=bug_watch,
-                              remote_comment_id=None)
+        with person_logged_in(owner):
+            return bug.newMessage(owner=owner, subject=subject, content=body,
+                                  parent=None, bugwatch=bug_watch,
+                                  remote_comment_id=None)
 
     def makeBugAttachment(self, bug=None, owner=None, data=None,
                           comment=None, filename=None, content_type=None,