Code review comment for lp:~thumper/launchpad/fix-inline-diff-encoding

Basically looks fine. I wonder slightly what happens if it's not
really a diff? I guess it won't be harmful.

> === modified file 'lib/lp/code/browser/codereviewcomment.py'
> --- lib/lp/code/browser/codereviewcomment.py 2009-10-21 17:45:53 +0000
> +++ lib/lp/code/browser/codereviewcomment.py 2009-10-30 04:14:25 +0000
> @@ -16,11 +16,13 @@
> from zope.interface import Interface, implements
> from zope.schema import Text
>
> -from canonical.cachedproperty import cachedproperty
> from lazr.delegates import delegates
> from lazr.restful.interface import copy_field
>
> +from canonical.cachedproperty import cachedproperty
> +from canonical.config import config
> from canonical.launchpad import _
> +from canonical.launchpad.interfaces import ILibraryFileAlias
> from canonical.launchpad.webapp import (
> action, canonical_url, ContextMenu, custom_widget, LaunchpadFormView,
> LaunchpadView, Link)
> @@ -71,6 +73,36 @@
> return Link('+reply', 'Reply', icon='add', enabled=enabled)
>
>
> +class DiffAttachment:
> + """An attachment that we are going to display."""
> +
> + implements(ILibraryFileAlias)
> +
> + delegates(ILibraryFileAlias, 'alias')
> +
> + def __init__(self, alias):
> + self.alias = alias
> +
> + @cachedproperty
> + def text(self):
> + """Read the text out of the librarin."""
> + self.alias.open()
> + try:
> + return self.alias.read(config.diff.max_read_size)
> + finally:
> + self.alias.close()
> +
> + @cachedproperty
> + def diff_text(self):
> + """Get the text and attempt to decode it."""
> + try:
> + diff = self.text.decode('utf-8')
> + except UnicodeDecodeError:
> + diff = self.text.decode('windows-1252', 'replace')
> + # Strip off the trailing carriage returns.
> + return diff.rstrip('\n')
> +
> +
> class CodeReviewCommentView(LaunchpadView):
> """Standard view of a CodeReviewComment"""
> __used_for__ = ICodeReviewComment
> @@ -114,7 +146,7 @@
> @cachedproperty
> def display_attachments(self):
> # Attachments to show.
> - return self.all_attachments[0]
> + return [DiffAttachment(alias) for alias in self.all_attachments[0]]
>
> @cachedproperty
> def other_attachments(self):

This all looks nice.

> === modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
> --- lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-09-18 22:28:26 +0000
> +++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-10-30 04:14:25 +0000
> @@ -25,7 +25,9 @@
> from lp.code.enums import BranchMergeProposalStatus, CodeReviewVote
> from lp.testing import (
> login_person, TestCaseWithFactory, time_counter)
> +from lp.testing.views import create_initialized_view
> from lp.code.model.diff import PreviewDiff, StaticDiff
> +from canonical.launchpad.database.message import MessageSet
> from canonical.launchpad.webapp.interfaces import IPrimaryContext
> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> from canonical.testing import (
> @@ -655,5 +657,40 @@
> user=self.proposal.target_branch.owner)
>
>
> +class TestCommentAttachmentRendering(TestCaseWithFactory):
> + """Test diff attachments are rendered correctly."""
> +
> + layer = LaunchpadFunctionalLayer
> +
> +
> + def _makeCommentFromEmailWithAttachment(self, attachment_body):
> + # Make an email message with an attachment, and create a code
> + # review comment from it.
> + bmp = self.factory.makeBranchMergeProposal()
> + login_person(bmp.registrant)
> + msg = self.factory.makeEmailMessage(
> + body='testing',
> + attachments=[('test.diff', 'text/plain', attachment_body)])
> + message = MessageSet().fromEmail(msg.as_string())
> + return bmp.createCommentFromMessage(message, None, None, msg)
> +
> + def test_unicode_in_attachment_renders(self):
> + # The view should render without errors.
> + comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')

Strictly that's not unicode, it's a bytestring with non ascii
characters...

> + # Need to commit in order to read the diff out of the librarian.
> + transaction.commit()
> + view = create_initialized_view(comment, '+comment-body')
> + view()
> +
> + def test_unicode_in_attachment_decoded(self):
> + # The view should render without errors.

This comment needs updating?

> + # Need to commit in order to read the diff out of the librarian.
> + comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')
> + transaction.commit()
> + view = create_initialized_view(comment, '+comment-body')
> + [diff_attachment] = view.display_attachments
> + self.assertEqual(u'\u2615', diff_attachment.diff_text)
> +
> +
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)

> === modified file 'lib/lp/code/mail/codereviewcomment.py'
> --- lib/lp/code/mail/codereviewcomment.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/code/mail/codereviewcomment.py 2009-10-30 04:14:25 +0000
> @@ -52,7 +52,8 @@
> else:
> content_type = part['content-type']
> if (filename, content_type) in include_attachments:
> - self.attachments.append(part)
> + self.attachments.append(
> + (part.get_payload(), filename, content_type))
> self._generateBodyBits()
>
> @classmethod
> @@ -132,6 +133,7 @@
> def _addAttachments(self, ctrl, email):
> """Add the attachments from the original message."""
> # Only reattach the display_aliases.
> - for attachment in self.attachments:
> + for content, content_type, filename in self.attachments:
> # Append directly to the controller's list.
> - ctrl.attachments.append(attachment)
> + ctrl.addAttachment(
> + content, content_type=content_type, filename=filename)

> === modified file 'lib/lp/code/templates/codereviewcomment-body.pt'
> --- lib/lp/code/templates/codereviewcomment-body.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/code/templates/codereviewcomment-body.pt 2009-10-30 04:14:25 +0000
> @@ -6,9 +6,9 @@
> <tal:message replace="structure view/body_text/fmt:obfuscate-email/fmt:nice_pre" />
>
> <tal:good-attachments repeat="attachment view/display_attachments">
> - <div class="boardComment attachment" tal:define="body_text python:str(attachment.read())">
> + <div class="boardComment attachment">
> <div class="boardCommentDetails filename"><a tal:content="attachment/filename" tal:attributes="href attachment/getURL"/></div>
> - <div class="boardCommentBody attachmentBody" tal:content="structure body_text/fmt:nice_pre"/>
> + <div class="boardCommentBody attachmentBody" tal:content="structure attachment/diff_text/fmt:diff"/>
> </div>
> </tal:good-attachments>
> <tal:other-attachments repeat="attachment view/other_attachments">

Hooray for getting rid of a python: expression!

Cheers,
mwh