Summary

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

Owner

chipx86

Repository

Review Board SVN (deprecated)

Branch

trunk

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

Ran the unit tests. They worked.

Mostly looks good.  Did you forget to include the fixture file?

chipx86 June 2, 2007, 7:47 p.m.

Nope. It was just really big and ugly, so I left it out of the generated diff. It'll be committed.

/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.

chipx86 June 1, 2007, 1:02 p.m.

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 :)

/trunk/reviewboard/reviews/email.py (Diff revision 2)

This could probably be slimmed down with a list comprehension, but not critical.

chipx86 June 1, 2007, 1:03 p.m.

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?

david June 1, 2007, 1:17 p.m.

I think [recipient_list += harvest_people_from_review(reply) for reply in review.replies.all()] will work.

chipx86 June 2, 2007, 7:47 p.m.

Found a solution, as we discussed in IRC. I'm quite happy by it, though it feels so backwards. Thanks Python.

/trunk/reviewboard/reviews/email.py (Diff revision 2)
```
Same for this.
```

Ship it!

```
Looks good.
```