launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06056
[Merge] lp:~abentley/launchpad/bugcomment-interface into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/bugcomment-interface into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~abentley/launchpad/bugcomment-interface/+merge/87808
= 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
--
https://code.launchpad.net/~abentley/launchpad/bugcomment-interface/+merge/87808
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/bugcomment-interface into lp:launchpad.
=== 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-06 20:38:09 +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-05 18:12:05 +0000
+++ lib/lp/bugs/browser/bugtask.py 2012-01-06 20:38:09 +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/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-06 20:38:09 +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/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-06 20:38:09 +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-06 20:38:09 +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-04 23:49:46 +0000
+++ lib/lp/testing/factory.py 2012-01-06 20:38:09 +0000
@@ -1960,9 +1960,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,
