Merge lp:~abentley/launchpad/fix-encoded-attachments into lp:launchpad

[Aaron Bentley]

Fix code review comments with encoded attachments

= Summary =
Fix bug #575185: Some merge proposal attachments are encoded as base64 but reported as qoted-printable

== Proposed fix ==
Decode message attachments before re-attaching them.

== Pre-implementation notes ==
None

== Implementation details ==
None

== Tests ==
bin/test -t test_encoded_attachments

== Demo and Q/A ==
Send an email with a base64-encoded attachment to a merge proposal on Staging.

The copy sent by launchpad should not display the attachment as base64-encoded.

= 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/mail/tests/test_codereviewcomment.py
  lib/lp/testing/factory.py

[Paul Hummer (rockstar)]

review: Approve (code)

=== modified file 'lib/lp/code/mail/codereviewcomment.py'
--- lib/lp/code/mail/codereviewcomment.py	2010-03-28 23:36:16 +0000
+++ lib/lp/code/mail/codereviewcomment.py	2010-05-07 17:45:47 +0000
@@ -61,8 +61,9 @@
                 else:
                     content_type = part['content-type']
                 if (filename, content_type) in include_attachments:
+                    payload = part.get_payload(decode=True)
                     self.attachments.append(
-                        (part.get_payload(), filename, content_type))
+                        (payload, filename, content_type))
         self._generateBodyBits()
 
     @classmethod

=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2010-04-13 19:15:48 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2010-05-07 17:45:47 +0000
@@ -205,6 +205,15 @@
         self.assertEqual(ctrl.body.splitlines()[1:-3],
                          mailer.message.text_contents.splitlines())
 
+    def makeComment(self, email_message):
+        message = getUtility(IMessageSet).fromEmail(email_message.as_string())
+        bmp = self.factory.makeBranchMergeProposal()
+        comment = bmp.createCommentFromMessage(
+            message, None, None, email_message)
+        # We need to make sure the Librarian is up-to-date, so we commit.
+        transaction.commit()
+        return comment
+
     def test_mailer_attachments(self):
         # Ensure that the attachments are attached.
         # Only attachments that we would show in the web ui are attached,
@@ -214,18 +223,14 @@
             attachments=[
                 ('inc.diff', 'text/x-diff', 'This is a diff.'),
                 ('pic.jpg', 'image/jpeg', 'Binary data')])
-        message = getUtility(IMessageSet).fromEmail(msg.as_string())
-        bmp = self.factory.makeBranchMergeProposal()
-        comment = bmp.createCommentFromMessage(message, None, None, msg)
-        # We need to make sure the Librarian is up-to-date, so we commit.
-        transaction.commit()
+        comment = self.makeComment(msg)
         mailer = CodeReviewCommentMailer.forCreation(comment)
         # The attachments of the mailer should have only the diff.
         [outgoing_attachment] = mailer.attachments
         self.assertEqual('inc.diff', outgoing_attachment[1])
         self.assertEqual('text/x-diff', outgoing_attachment[2])
         # The attachments are attached to the outgoing message.
-        person = bmp.target_branch.owner
+        person = comment.branch_merge_proposal.target_branch.owner
         message = mailer.generateEmail(
             person.preferredemail.email, person).makeMessage()
         self.assertTrue(message.is_multipart())
@@ -233,6 +238,21 @@
         self.assertEqual('inc.diff', attachment.get_filename())
         self.assertEqual('text/x-diff', attachment['content-type'])
 
+    def test_encoded_attachments(self):
+        msg = self.factory.makeEmailMessage(
+            body='This is the body of the email.',
+            attachments=[('inc.diff', 'text/x-diff', 'This is a diff.')],
+            encode_attachments=True)
+        comment = self.makeComment(msg)
+        mailer = CodeReviewCommentMailer.forCreation(comment)
+        person = comment.branch_merge_proposal.target_branch.owner
+        message = mailer.generateEmail(
+            person.preferredemail.email, person).makeMessage()
+        attachment = message.get_payload()[1]
+        self.assertEqual(
+            'This is a diff.', attachment.get_payload(decode=True))
+
+
     def makeCommentAndParticipants(self):
         """Create a merge proposal and comment.
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-05-07 14:39:20 +0000
+++ lib/lp/testing/factory.py	2010-05-07 17:45:47 +0000
@@ -2305,7 +2305,7 @@
         return DistributionSourcePackage(distribution, sourcepackagename)
 
     def makeEmailMessage(self, body=None, sender=None, to=None,
-                         attachments=None):
+                         attachments=None, encode_attachments=False):
         """Make an email message with possible attachments.
 
         :param attachments: Should be an interable of tuples containing
@@ -2335,6 +2335,8 @@
                 attachment['Content-Type'] = content_type
                 attachment['Content-Disposition'] = (
                     'attachment; filename="%s"' % filename)
+                if encode_attachments:
+                    encode_base64(attachment)
                 msg.attach(attachment)
         return msg