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

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)

« Back to merge proposal