I couldnt understand why I had to remove the "+ 1" from here but had to do it to get the tests to pass. Sourcecode History dint give me much clue either.

Same as above

"when review requests are closed"

This shouldn't be in this change.

These should all align with the import on the previous line, where it aligned before.

type is a reserved word. Try close_type.

**kwargs should align properly with sender.

I know the other functions aren't doing this, but the first line should be a 1-line summary of the function, like:

"""Sends e-mail when a review request is closed.

<multi-line description here>
"""

More alignment problems.

Are you perhaps mixing tabs and spaces?

New paragraph. Should have a blank line above it.

Alignment problems, and should use parens instead of \ in conditionals.

I know the others aren't like this, but this should be test_review_close_email. We're slowly fixing other test names.

Technically, this is one test, and is separate from the code below. There should be one unit test that tests with the setting set, and one without the setting set.

We're finding that fixtures are very costly. Every fixture we add increases the load time for nearly all unit tests.

It'd be nice to instead create a review request here instead. You can make one that doesn't have an associated repository, to make it easier.

You'll then be able to clean up a lot of the code below that's accommodating the new fixture.

"""blah""" is a function/class docstring. You should be using a # comment instead. Same below.

Trailing whitespace.

Instead of find, do:

self.assertTrue('This review request has been submitted' in message.as_string())

Since this is a plain-text e-mail, we have to be careful about spacing. The {% if changes ... %} line is going to introduce a newline. To compensate, remove the newline above that line.

You'll also need to be careful with the lines after that use template tags.

Wrong number of dashes.

Missing indentation inside the {% .. %}, since these are inside a {% if %} above. So:

{% if changes and .... %}
..
{%  if review_request.status == 'S' %}

...

These were because of your new fixture. Remove that and you won't need these lines.

Shouldn't this be 'email is being sent due to a request being closed'?

Above comment notwithstanding, this looks fine to me, FWIW...

I really appreciate the inclusion of tests. I have some comments about the presentation of this in the templates:

The "status" line here looks indented by 4 spaces. Did you produce this example with a different version of the template?

The email template tries to mimic the look and feel of the main web UI. In this case, what I'd like to see is this moved into a new grey box above the main "Review request for ..." box that looks like what the user would see on the reviews page.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/notifications/tests.py
    reviewboard/notifications/email.py
  Ignored Files:
    reviewboard/templates/notifications/review_request_email.html
    reviewboard/templates/notifications/review_request_email.txt

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 80
 E501 line too long (93 > 79 characters)

Col: 80
 E501 line too long (88 > 79 characters)

Col: 80
 E501 line too long (88 > 79 characters)

This is much better. I still have some suggestions on the look.

I'd like if this matched the review request template a little more closely. Namely, use a background color of "#e0e0e0", a border of "1px #808080 solid", and bold h1 text that says either "This change has been marked as submitted." or "This change has been discarded."


For examples of the look, see these:
http://reviews.reviewboard.org/r/3747/
http://reviews.reviewboard.org/r/3559/

We'd essentially want that without the "Reopen" button or the text field (since the close email would be sent before the user has the opportunity to fill in the text).

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/notifications/tests.py
    reviewboard/notifications/email.py
  Ignored Files:
    reviewboard/templates/notifications/review_request_email.html
    reviewboard/templates/notifications/review_request_email.txt

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 80
 E501 line too long (93 > 79 characters)

Col: 80
 E501 line too long (88 > 79 characters)

Ship It!