launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/utf8-encode-diffs-861979 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #861979 in Launchpad itself: "utf-8 encode diffs attached to outgoing email"
  https://bugs.launchpad.net/launchpad/+bug/861979

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/utf8-encode-diffs-861979/+merge/78244

A fix for thumper

== Implementation ==

Diffs attached to code review and branch emails are utf-8 encoded. This is done by by extending the addAttachment() method on the lp MailController class to accept an optional charset parameter. A consequence is that the charset's body encoding is used rather than the MailController's encodeOptimally() method deciding. 

== Tests ==

Modify some existing tests:

TestBranchMailerDiff
 - test_generateEmail_with_diff()

TestMergeProposalMailing
 - test_generateEmail_attaches_diff()
 - test_generateEmail_attaches_diff_oversize_truncated()

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/mail/branch.py
  lib/lp/code/mail/branchmergeproposal.py
  lib/lp/code/mail/tests/test_branch.py
  lib/lp/code/mail/tests/test_branchmergeproposal.py
  lib/lp/services/mail/sendmail.py


-- 
https://code.launchpad.net/~wallyworld/launchpad/utf8-encode-diffs-861979/+merge/78244
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/utf8-encode-diffs-861979 into lp:launchpad.

=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py	2011-08-12 14:39:51 +0000
+++ lib/lp/code/mail/branch.py	2011-10-05 12:42:17 +0000
@@ -301,7 +301,7 @@
         # Using .txt as a file extension makes Gmail display it inline.
         ctrl.addAttachment(
             self.diff, content_type='text/x-diff', inline=True,
-                filename='revision-diff.txt')
+                filename='revision-diff.txt', charset='utf-8')
 
     @staticmethod
     def _format_user_address(user):

=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
--- lib/lp/code/mail/branchmergeproposal.py	2011-08-12 14:39:51 +0000
+++ lib/lp/code/mail/branchmergeproposal.py	2011-10-05 12:42:17 +0000
@@ -132,7 +132,7 @@
                 # inline.
                 ctrl.addAttachment(
                     self.preview_diff.text, content_type='text/x-diff',
-                    inline=True, filename='review-diff.txt')
+                    inline=True, filename='review-diff.txt', charset='utf-8')
 
     def _generateTemplateParams(self):
         """For template params that don't change, calculate just once."""

=== modified file 'lib/lp/code/mail/tests/test_branch.py'
--- lib/lp/code/mail/tests/test_branch.py	2011-08-12 11:37:08 +0000
+++ lib/lp/code/mail/tests/test_branch.py	2011-10-05 12:42:17 +0000
@@ -221,11 +221,11 @@
 
     def test_generateEmail_with_diff(self):
         """When there is a diff, it should be an attachment, not inline."""
-        ctrl = self.makeBobMailController(diff='hello')
+        ctrl = self.makeBobMailController(diff=u'hello \u03A3')
         self.assertEqual(1, len(ctrl.attachments))
         diff = ctrl.attachments[0]
-        self.assertEqual('hello', diff.get_payload(decode=True))
-        self.assertEqual('text/x-diff', diff['Content-type'])
+        self.assertEqual('hello \xce\xa3', diff.get_payload(decode=True))
+        self.assertEqual('text/x-diff; charset="utf-8"', diff['Content-type'])
         self.assertEqual('inline; filename="revision-diff.txt"',
                          diff['Content-disposition'])
         self.assertNotIn('hello', ctrl.body)

=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py	2011-09-28 09:49:04 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py	2011-10-05 12:42:17 +0000
@@ -347,7 +347,8 @@
         mailer = BMPMailer.forCreation(bmp, bmp.registrant)
         ctrl = mailer.generateEmail('baz.quxx@xxxxxxxxxxx', subscriber)
         (attachment,) = ctrl.attachments
-        self.assertEqual('text/x-diff', attachment['Content-Type'])
+        self.assertEqual(
+            'text/x-diff; charset="utf-8"', attachment['Content-Type'])
         self.assertEqual('inline; filename="review-diff.txt"',
                          attachment['Content-Disposition'])
         self.assertEqual(diff_text, attachment.get_payload(decode=True))
@@ -373,7 +374,8 @@
         mailer = BMPMailer.forCreation(bmp, bmp.registrant)
         ctrl = mailer.generateEmail('baz.quxx@xxxxxxxxxxx', subscriber)
         (attachment,) = ctrl.attachments
-        self.assertEqual('text/x-diff', attachment['Content-Type'])
+        self.assertEqual(
+            'text/x-diff; charset="utf-8"', attachment['Content-Type'])
         self.assertEqual('inline; filename="review-diff.txt"',
                          attachment['Content-Disposition'])
         self.assertEqual(diff_text[:25], attachment.get_payload(decode=True))

=== modified file 'lib/lp/services/mail/sendmail.py'
--- lib/lp/services/mail/sendmail.py	2011-09-26 07:21:53 +0000
+++ lib/lp/services/mail/sendmail.py	2011-10-05 12:42:17 +0000
@@ -216,10 +216,13 @@
         self.attachments = []
 
     def addAttachment(self, content, content_type='application/octet-stream',
-                      inline=False, filename=None):
+                      inline=False, filename=None, charset=None):
         attachment = Message()
-        attachment.set_payload(content)
-        attachment['Content-type'] = content_type
+        if charset:
+            attachment.add_header(
+                'Content-Type', content_type, charset=charset)
+        else:
+            attachment.add_header('Content-Type', content_type)
         if inline:
             disposition = 'inline'
         else:
@@ -229,6 +232,7 @@
             disposition_kwargs['filename'] = filename
         attachment.add_header(
             'Content-Disposition', disposition, **disposition_kwargs)
+        attachment.set_payload(content, charset)
         self.encodeOptimally(attachment)
         self.attachments.append(attachment)
 
@@ -248,6 +252,10 @@
         :param exact: If True, the encoding will ensure newlines are not
             mangled.  If False, 7-bit attachments will not be encoded.
         """
+        # If encoding has already been done by virtue of a charset being
+        # previously specified, then do nothing.
+        if part.has_key('Content-Transfer-Encoding'):
+            return
         orig_payload = part.get_payload()
         if not exact and is_ascii_only(orig_payload):
             return