Merge lp:~abentley/launchpad/abort-failed-vote into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
Fix bug #354544 by preventing creating an codereviewcomment from an
email whose subject line is empty

== Proposed fix ==
Until now, errors due to empty subject lines have been produced after
the CodeReviewComment was created, when sending the message to
recipients. Now, we perform the same validation used by
sendmail.sendmail to ensure that we never create an unsendable
codereviewcomment.

== Pre-implementation notes ==
None

== Implementation details ==
The orig_email parameter for createCommentFromMessage is now mandatory,
but may be supplied as None if _validate is supplied as False. This is
done by createComment, because we can assume that we create a valid
message that way.

Several tests had to be updated for to handle the new parameters, and
the makeSignedMessage had to be updated to create messages with a "to"
header.

== Tests ==
bin/test test_codehandler

== Demo and Q/A ==
Reply to a merge proposal with no subject. An error email should be
sent, and your comment should not appear on the merge proposal.

= 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/model/branchmergeproposal.py
  lib/lp/code/mail/tests/test_codehandler.py
  lib/lp/testing/factory.py
  lib/lp/services/mail/sendmail.py
  lib/lp/code/model/tests/test_codereviewcomment.py
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpopuMACgkQ0F+nu1YWqI320ACfcSEjLGHJUipn0sCeJ5mnJfnd
qtEAn1z1sphwJnCtsQgkG9iszb2k90HT
=h3uu
-----END PGP SIGNATURE-----

[Celso Providelo (cprov)]

Aaron,

We discussed your approach for being explicit about the validation, instead of validating any not-None messsage passed in. You convinced me that there is no reason to block internal callsites to pass not-None messages and skip validation if that's what they need to do.

We rely on the fact that external callsites should not mess with the 'private' argument.

Please add 'validate_message' to the lib/lp/services/mail/sendmail.py __all__ and r=me.

review: Approve

=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2009-07-17 00:26:05 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2009-07-23 17:47:50 +0000
@@ -732,6 +732,7 @@
             )
         self.assertEqual(notification['to'],
             mail['from'])
+        self.assertEqual(0, bmp.all_comments.count())
 
 
 class TestCodeHandlerProcessMergeDirective(TestCaseWithFactory):

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2009-07-17 00:26:05 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2009-07-23 17:54:31 +0000
@@ -44,13 +44,15 @@
 from lp.code.interfaces.branchmergeproposal import (
     BadBranchMergeProposalSearchContext, BadStateTransition,
     BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
-    IBranchMergeProposal, IBranchMergeProposalGetter, UserNotBranchReviewer, WrongBranchMergeProposal)
+    IBranchMergeProposal, IBranchMergeProposalGetter, UserNotBranchReviewer,
+    WrongBranchMergeProposal)
 from lp.code.interfaces.branchtarget import IHasBranchTarget
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import IProduct
 from lp.code.mail.branch import RecipientReason
 from lp.registry.interfaces.person import validate_public_person
+from lp.services.mail.sendmail import validate_message
 
 
 def is_valid_transition(proposal, from_state, next_state, user=None):
@@ -558,7 +560,8 @@
             subject=subject, datecreated=_date_created)
         chunk = MessageChunk(message=message, content=content, sequence=1)
         return self.createCommentFromMessage(
-            message, vote, review_type, _notify_listeners=_notify_listeners)
+            message, vote, review_type, original_email=None,
+            _notify_listeners=_notify_listeners, _validate=False)
 
     def getUsersVoteReference(self, user, review_type=None):
         """Get the existing vote reference for the given user."""
@@ -624,14 +627,17 @@
             review_type=review_type)
 
     def createCommentFromMessage(self, message, vote, review_type,
-                                 original_email=None, _notify_listeners=True):
+                                 original_email, _notify_listeners=True,
+                                 _validate=True):
         """See `IBranchMergeProposal`."""
+        if _validate:
+            validate_message(original_email)
         # Lower case the review type.
         if review_type is not None:
             review_type = review_type.lower()
         code_review_message = CodeReviewComment(
             branch_merge_proposal=self, message=message, vote=vote,
-            vote_tag=review_type)
+            vote_tag=review_type, original_email=original_email)
         # Get the appropriate CodeReviewVoteReference for the reviewer.
         # If there isn't one, then create one, otherwise set the comment
         # reference.

=== modified file 'lib/lp/code/model/tests/test_codereviewcomment.py'
--- lib/lp/code/model/tests/test_codereviewcomment.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/model/tests/test_codereviewcomment.py	2009-07-23 17:47:50 +0000
@@ -96,7 +96,8 @@
     def test_createCommentFromMessage(self):
         """Creating a CodeReviewComment from a message works."""
         message = self.factory.makeMessage(owner=self.submitter)
-        comment = self.bmp.createCommentFromMessage(message, None, None)
+        comment = self.bmp.createCommentFromMessage(
+            message, None, None, original_email=None, _validate=False)
         self.assertEqual(message, comment.message)
 
     def test_createCommentFromMessageNotifies(self):
@@ -104,7 +105,7 @@
         message = self.factory.makeMessage()
         self.assertNotifies(
             NewCodeReviewCommentEvent, self.bmp.createCommentFromMessage,
-            message, None, None)
+            message, None, None, original_email=None, _validate=False)
 
 
 class TestCodeReviewCommentGetAttachments(TestCaseWithFactory):

=== modified file 'lib/lp/services/mail/sendmail.py'
--- lib/lp/services/mail/sendmail.py	2009-07-17 18:46:25 +0000
+++ lib/lp/services/mail/sendmail.py	2009-07-23 17:47:50 +0000
@@ -292,6 +292,12 @@
         formataddr((name, address))
         for name, address in getaddresses([email_header])]
 
+def validate_message(message):
+    assert isinstance(message, Message), 'Not an email.Message.Message'
+    assert 'to' in message and bool(message['to']), 'No To: header'
+    assert 'from' in message and bool(message['from']), 'No From: header'
+    assert 'subject' in message and bool(message['subject']), \
+            'No Subject: header'
 
 def sendmail(message, to_addrs=None, bulk=True):
     """Send an email.Message.Message
@@ -316,12 +322,7 @@
 
     Returns the Message-Id
     """
-    assert isinstance(message, Message), 'Not an email.Message.Message'
-    assert 'to' in message and bool(message['to']), 'No To: header'
-    assert 'from' in message and bool(message['from']), 'No From: header'
-    assert 'subject' in message and bool(message['subject']), \
-            'No Subject: header'
-
+    validate_message(message)
     if to_addrs is None:
         to_addrs = get_addresses_from_header(message['to'])
         if message['cc']:

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-07-17 00:26:05 +0000
+++ lib/lp/testing/factory.py	2009-07-23 17:47:50 +0000
@@ -1014,6 +1014,7 @@
             person = self.makePerson()
             email_address = person.preferredemail.email
         mail['From'] = email_address
+        mail['To'] = self.makePerson().preferredemail.email
         if subject is None:
             subject = self.getUniqueString('subject')
         mail['Subject'] = subject