Merge lp:~thumper/launchpad/fix-inline-diff-encoding into lp:launchpad

[Tim Penhey]

The inline diffs are now decoded appropriately and rendered as a diff.

[Tim Penhey (thumper)]

= Summary =

Fix bug 414852. This is due to the diff text of the emailed attachments not
being decoded correctly.

== Proposed fix ==

Decorate the attachment and decode the text that will be rendered.

== Implementation details ==

First added a test that replicates the current oops
(test_unicode_in_attachment_renders), although this found another error where
the attachments that were being emailed out were not being encoded correctly.
The production emailer probably handles this, but the test one doesn't like
non-ascii. This was fixed in the code review comment mailer. Instead of
directly adding message parts to be added as attachments, it now calls the
controller addAttachment method which does optimistic encoding of the
attachment.

Then changed the view to use the DiffAttachment class as it decodes the diff
text, and changed the template to use that. Added the other test to confirm
the decoding.

Finally changed the rendering of the diff from fmt:nice_pre to fmt:diff.

== Tests ==

TestCommentAttachmentRendering

== Demo and Q/A ==

make harness is your friend. You can copy the test case example and look at
it in the web browser.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/mail/codereviewcomment.py
  lib/lp/code/templates/codereviewcomment-body.pt
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/browser/codereviewcomment.py

== Pylint notices ==

lib/lp/code/browser/codereviewcomment.py
    19: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    20: [F0401] Unable to import 'lazr.restful.interface' (No module named
restful)

[Michael Hudson-Doyle (mwhudson)]

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_bra...

review: Approve

[Michael Nelson (michael.nelson)]

Hi Tim, thanks for getting this in early!

This is the correct display there right?
http://people.canonical.com/~michaeln/tmp/tim_mp.png

If so, please add it as the first entry at https://dev.launchpad.net/CurrentRolloutBlockers :) and land.

Thanks,
Michael

review: Approve (release-critical)

=== 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-11-01 23:19:18 +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):
 
=== 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-11-01 23:19:18 +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_nonascii_in_attachment_renders(self):
+        # The view should render without errors.
+        comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')
+        # 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_nonascii_in_attachment_decoded(self):
+        # The diff_text should be a unicode string.
+        comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')
+        # Need to commit in order to read the diff out of the librarian.
+        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-11-01 23:19:18 +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/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2009-07-17 00:26:05 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2009-11-01 23:19:18 +0000
@@ -212,8 +212,9 @@
         comment = bmp.createCommentFromMessage(message, None, None, msg)
         mailer = CodeReviewCommentMailer.forCreation(comment, msg)
         # The attachments of the mailer should have only the diff.
-        first, diff, image = msg.get_payload()
-        self.assertEqual([diff], mailer.attachments)
+        [outgoing_attachment] = mailer.attachments
+        self.assertEqual('inc.diff', outgoing_attachment[1])
+        self.assertEqual('text/x-diff', outgoing_attachment[2])
 
     def makeCommentAndParticipants(self):
         """Create a merge proposal and comment.
 
=== 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-11-01 23:19:18 +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">