Improve e-mail recipient logic, switch to Django's e-mail support, and add unit tests

Review Request #44 — Created June 1, 2007 and submitted

Information

Review Board SVN (deprecated)
trunk

Reviewers

Several improvements to the e-mail support here.

* Recipient logic is now improved. We always e-mail to the review request author and the target reviewers, but we also add to this in some cases:
  * Updates to a diff or review request go to every person who has been involved in a discussion on the review request.
  * A reply on a review goes to every person who has been involved in that review's discussion.

* We now use Django's e-mail support instead of our own, now that we can subclass their new EmailMessage object and inject the headers we need. This also gives us the next feature..
* Unit tests! We now unit test e-mail. We specifically check the subject and the To: list.
Ran the unit tests. They worked.
david
  1. Mostly looks good.  Did you forget to include the fixture file?
    1. Nope. It was just really big and ugly, so I left it out of the generated diff. It'll be committed.
  2. /trunk/reviewboard/reviews/email.py (Diff revision 2)
     
     
    I'd prefer to see this called "ThreadedEmailMessage" or somesuch.  "Spiffy" just doesn't give me any information about what it does.
    
    Alternatively, explain it with a docstring.
    1. Yep. I couldn't come up with a name I liked, so I left it as-is, expecting that you'd come up with one :)
  3. /trunk/reviewboard/reviews/email.py (Diff revision 2)
     
     
     
     
     
     
     
    This could probably be slimmed down with a list comprehension, but not critical.
    1. I was trying to figure out how to do that without ending up with a [[...], [...], ...].
      
      If I did:
      
      recipient_list += [harvest_people_from_review(reply) for reply in review.replies.all()]
      
      I would get:
      
      [["chipx86", "admin"], ["foo", "bar"]] and such.
      
      I'd love to slim this down. Suggestions?
    2. I think [recipient_list += harvest_people_from_review(reply) for reply in review.replies.all()] will work.
    3. Found a solution, as we discussed in IRC. I'm quite happy by it, though it feels so backwards. Thanks Python.
  4. /trunk/reviewboard/reviews/email.py (Diff revision 2)
     
     
    Same for this.
  5. 
      
david
  1. Looks good.
  2.