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

Proposed by Tim Penhey
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/fix-inline-diff-encoding
Merge into: lp:launchpad
Diff against target: 175 lines
5 files modified
lib/lp/code/browser/codereviewcomment.py (+34/-2)
lib/lp/code/browser/tests/test_branchmergeproposal.py (+37/-0)
lib/lp/code/mail/codereviewcomment.py (+5/-3)
lib/lp/code/mail/tests/test_codereviewcomment.py (+3/-2)
lib/lp/code/templates/codereviewcomment-body.pt (+2/-2)
To merge this branch: bzr merge lp:~thumper/launchpad/fix-inline-diff-encoding
Reviewer Review Type Date Requested Status
Michael Nelson (community) release-critical Approve
Michael Hudson-Doyle Approve
Review via email: mp+14196@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

= 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)

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :
Download full text (7.1 KiB)

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

Read more...

review: Approve
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/codereviewcomment.py'
2--- lib/lp/code/browser/codereviewcomment.py 2009-10-21 17:45:53 +0000
3+++ lib/lp/code/browser/codereviewcomment.py 2009-11-01 23:19:18 +0000
4@@ -16,11 +16,13 @@
5 from zope.interface import Interface, implements
6 from zope.schema import Text
7
8-from canonical.cachedproperty import cachedproperty
9 from lazr.delegates import delegates
10 from lazr.restful.interface import copy_field
11
12+from canonical.cachedproperty import cachedproperty
13+from canonical.config import config
14 from canonical.launchpad import _
15+from canonical.launchpad.interfaces import ILibraryFileAlias
16 from canonical.launchpad.webapp import (
17 action, canonical_url, ContextMenu, custom_widget, LaunchpadFormView,
18 LaunchpadView, Link)
19@@ -71,6 +73,36 @@
20 return Link('+reply', 'Reply', icon='add', enabled=enabled)
21
22
23+class DiffAttachment:
24+ """An attachment that we are going to display."""
25+
26+ implements(ILibraryFileAlias)
27+
28+ delegates(ILibraryFileAlias, 'alias')
29+
30+ def __init__(self, alias):
31+ self.alias = alias
32+
33+ @cachedproperty
34+ def text(self):
35+ """Read the text out of the librarin."""
36+ self.alias.open()
37+ try:
38+ return self.alias.read(config.diff.max_read_size)
39+ finally:
40+ self.alias.close()
41+
42+ @cachedproperty
43+ def diff_text(self):
44+ """Get the text and attempt to decode it."""
45+ try:
46+ diff = self.text.decode('utf-8')
47+ except UnicodeDecodeError:
48+ diff = self.text.decode('windows-1252', 'replace')
49+ # Strip off the trailing carriage returns.
50+ return diff.rstrip('\n')
51+
52+
53 class CodeReviewCommentView(LaunchpadView):
54 """Standard view of a CodeReviewComment"""
55 __used_for__ = ICodeReviewComment
56@@ -114,7 +146,7 @@
57 @cachedproperty
58 def display_attachments(self):
59 # Attachments to show.
60- return self.all_attachments[0]
61+ return [DiffAttachment(alias) for alias in self.all_attachments[0]]
62
63 @cachedproperty
64 def other_attachments(self):
65
66=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
67--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-09-18 22:28:26 +0000
68+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-11-01 23:19:18 +0000
69@@ -25,7 +25,9 @@
70 from lp.code.enums import BranchMergeProposalStatus, CodeReviewVote
71 from lp.testing import (
72 login_person, TestCaseWithFactory, time_counter)
73+from lp.testing.views import create_initialized_view
74 from lp.code.model.diff import PreviewDiff, StaticDiff
75+from canonical.launchpad.database.message import MessageSet
76 from canonical.launchpad.webapp.interfaces import IPrimaryContext
77 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
78 from canonical.testing import (
79@@ -655,5 +657,40 @@
80 user=self.proposal.target_branch.owner)
81
82
83+class TestCommentAttachmentRendering(TestCaseWithFactory):
84+ """Test diff attachments are rendered correctly."""
85+
86+ layer = LaunchpadFunctionalLayer
87+
88+
89+ def _makeCommentFromEmailWithAttachment(self, attachment_body):
90+ # Make an email message with an attachment, and create a code
91+ # review comment from it.
92+ bmp = self.factory.makeBranchMergeProposal()
93+ login_person(bmp.registrant)
94+ msg = self.factory.makeEmailMessage(
95+ body='testing',
96+ attachments=[('test.diff', 'text/plain', attachment_body)])
97+ message = MessageSet().fromEmail(msg.as_string())
98+ return bmp.createCommentFromMessage(message, None, None, msg)
99+
100+ def test_nonascii_in_attachment_renders(self):
101+ # The view should render without errors.
102+ comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')
103+ # Need to commit in order to read the diff out of the librarian.
104+ transaction.commit()
105+ view = create_initialized_view(comment, '+comment-body')
106+ view()
107+
108+ def test_nonascii_in_attachment_decoded(self):
109+ # The diff_text should be a unicode string.
110+ comment = self._makeCommentFromEmailWithAttachment('\xe2\x98\x95')
111+ # Need to commit in order to read the diff out of the librarian.
112+ transaction.commit()
113+ view = create_initialized_view(comment, '+comment-body')
114+ [diff_attachment] = view.display_attachments
115+ self.assertEqual(u'\u2615', diff_attachment.diff_text)
116+
117+
118 def test_suite():
119 return unittest.TestLoader().loadTestsFromName(__name__)
120
121=== modified file 'lib/lp/code/mail/codereviewcomment.py'
122--- lib/lp/code/mail/codereviewcomment.py 2009-07-17 00:26:05 +0000
123+++ lib/lp/code/mail/codereviewcomment.py 2009-11-01 23:19:18 +0000
124@@ -52,7 +52,8 @@
125 else:
126 content_type = part['content-type']
127 if (filename, content_type) in include_attachments:
128- self.attachments.append(part)
129+ self.attachments.append(
130+ (part.get_payload(), filename, content_type))
131 self._generateBodyBits()
132
133 @classmethod
134@@ -132,6 +133,7 @@
135 def _addAttachments(self, ctrl, email):
136 """Add the attachments from the original message."""
137 # Only reattach the display_aliases.
138- for attachment in self.attachments:
139+ for content, content_type, filename in self.attachments:
140 # Append directly to the controller's list.
141- ctrl.attachments.append(attachment)
142+ ctrl.addAttachment(
143+ content, content_type=content_type, filename=filename)
144
145=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
146--- lib/lp/code/mail/tests/test_codereviewcomment.py 2009-07-17 00:26:05 +0000
147+++ lib/lp/code/mail/tests/test_codereviewcomment.py 2009-11-01 23:19:18 +0000
148@@ -212,8 +212,9 @@
149 comment = bmp.createCommentFromMessage(message, None, None, msg)
150 mailer = CodeReviewCommentMailer.forCreation(comment, msg)
151 # The attachments of the mailer should have only the diff.
152- first, diff, image = msg.get_payload()
153- self.assertEqual([diff], mailer.attachments)
154+ [outgoing_attachment] = mailer.attachments
155+ self.assertEqual('inc.diff', outgoing_attachment[1])
156+ self.assertEqual('text/x-diff', outgoing_attachment[2])
157
158 def makeCommentAndParticipants(self):
159 """Create a merge proposal and comment.
160
161=== modified file 'lib/lp/code/templates/codereviewcomment-body.pt'
162--- lib/lp/code/templates/codereviewcomment-body.pt 2009-07-17 17:59:07 +0000
163+++ lib/lp/code/templates/codereviewcomment-body.pt 2009-11-01 23:19:18 +0000
164@@ -6,9 +6,9 @@
165 <tal:message replace="structure view/body_text/fmt:obfuscate-email/fmt:nice_pre" />
166
167 <tal:good-attachments repeat="attachment view/display_attachments">
168- <div class="boardComment attachment" tal:define="body_text python:str(attachment.read())">
169+ <div class="boardComment attachment">
170 <div class="boardCommentDetails filename"><a tal:content="attachment/filename" tal:attributes="href attachment/getURL"/></div>
171- <div class="boardCommentBody attachmentBody" tal:content="structure body_text/fmt:nice_pre"/>
172+ <div class="boardCommentBody attachmentBody" tal:content="structure attachment/diff_text/fmt:diff"/>
173 </div>
174 </tal:good-attachments>
175 <tal:other-attachments repeat="attachment view/other_attachments">