Send email when closing review requests

Review Request #3707 — Created Dec. 29, 2012 and submitted

Information

Review Board
master

Reviewers

Optionally send emails when closing review requests.
    
Add a flag in the Email settings page that will allow for sending emails when
a review request is closed (Submitted/Discarded).
    
Files Changed:
* reviewboard/admin/forms.py
  + Added Boolean to store the value of "mail_send_review_close_mail" in settings
    
* reviewboard/notifications/email.py
  + Added callback method for review_request_closed signal
    
* reviewboard/templates/notifications/review_request_email.html
* reviewboard/templates/notifications/review_request_email.txt
  + Added code to display the "Status" of the review request when its closed
    before the "Description" section.
    
* Tests:
  + Added test for review request closing
  + Modified other review related tests to fix counts or expected results
Added test for this feature and ran all tests. Verified in UI.

Description From Last Updated

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

daviddavid

I'd like if this matched the review request template a little more closely. Namely, use a background color of "#e0e0e0", …

daviddavid

"when review requests are closed"

chipx86chipx86

This shouldn't be in this change.

chipx86chipx86

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

chipx86chipx86

type is a reserved word. Try close_type. **kwargs should align properly with sender.

chipx86chipx86

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

chipx86chipx86

More alignment problems. Are you perhaps mixing tabs and spaces?

chipx86chipx86

New paragraph. Should have a blank line above it.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Technically, this is one test, and is separate from the code below. There should be one unit test that tests …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Trailing whitespace. Instead of find, do: self.assertTrue('This review request has been submitted' in message.as_string())

chipx86chipx86

Since this is a plain-text e-mail, we have to be careful about spacing. The {% if changes ... %} line …

chipx86chipx86

Wrong number of dashes.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

The email template tries to mimic the look and feel of the main web UI. In this case, what I'd …

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot
RA
  1. 
      
  2. reviewboard/webapi/tests.py (Diff revision 1)
     
     
    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.
  3. reviewboard/webapi/tests.py (Diff revision 1)
     
     
    Same as above
  4. 
      
chipx86
  1. 
      
  2. reviewboard/admin/forms.py (Diff revision 1)
     
     
    Show all issues
    "when review requests are closed"
  3. reviewboard/cmdline/rbsite.py (Diff revision 1)
     
     
    Show all issues
    This shouldn't be in this change.
  4. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues
    These should all align with the import on the previous line, where it aligned before.
  5. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues
    type is a reserved word. Try close_type.
    
    **kwargs should align properly with sender.
    1. Ive removed that argument as its not really required for this to work
  6. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues
    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>
    """
  7. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues
    More alignment problems.
    
    Are you perhaps mixing tabs and spaces?
  8. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Show all issues
    New paragraph. Should have a blank line above it.
  9. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues
    Alignment problems, and should use parens instead of \ in conditionals.
  10. reviewboard/notifications/tests.py (Diff revision 1)
     
     
    Show all issues
    I know the others aren't like this, but this should be test_review_close_email. We're slowly fixing other test names.
  11. reviewboard/notifications/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues
    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.
    1. Ive split it as multiple tests, one with default settings and one with setting to send email on review request close.
  12. reviewboard/notifications/tests.py (Diff revision 1)
     
     
     
    Show all issues
    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.
  13. reviewboard/notifications/tests.py (Diff revision 1)
     
     
    Show all issues
    """blah""" is a function/class docstring. You should be using a # comment instead. Same below.
  14. reviewboard/notifications/tests.py (Diff revision 1)
     
     
     
    Show all issues
    Trailing whitespace.
    
    Instead of find, do:
    
    self.assertTrue('This review request has been submitted' in message.as_string())
  15. Show all issues
    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.
    1. Ive verified that both plaintext and html emails dont have any unnecessary newlines before or after this change.
  16. Show all issues
    Wrong number of dashes.
  17. Show all issues
    Missing indentation inside the {% .. %}, since these are inside a {% if %} above. So:
    
    {% if changes and .... %}
    ..
    {%  if review_request.status == 'S' %}
    
    ...
  18. reviewboard/webapi/tests.py (Diff revision 1)
     
     
    Show all issues
    These were because of your new fixture. Remove that and you won't need these lines.
  19. 
      
RA
MW
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 2)
     
     
     
    Shouldn't this be 'email is being sent due to a request being closed'?
  3. Above comment notwithstanding, this looks fine to me, FWIW...
david
  1. I really appreciate the inclusion of tests. I have some comments about the presentation of this in the templates:
  2. Show all issues
    The "status" line here looks indented by 4 spaces. Did you produce this example with a different version of the template?
    1. Yes, I think this was with a earlier template, Im attaching the latest output , which doesnt have the indentation
  3. reviewboard/templates/notifications/review_request_email.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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.
    1. Thanks, Ive fixed this in the latest diff
  4. 
      
RA
reviewbot
  1. 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
    
    
  2. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Show all issues
    Col: 41
     E126 continuation line over-indented for hanging indent
    
    1. This is similar to how the earlier line in this is continued. Maybe this got flagged because this is a new change? Im dropping this issue as its similar to how its done in other places in this file
  3. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Show all issues
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. reviewboard/notifications/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
    1. Should single-line docstrings be also limited to 79 chars. It will reduce the meaning of the comment if it needs to be within 79 chars. Im dropping this as well. Please re-open if its not correct.
  5. reviewboard/notifications/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  6. reviewboard/notifications/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  7. 
      
RA
david
  1. This is much better. I still have some suggestions on the look.
  2. Show all issues
    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).
  3. 
      
RA
reviewbot
  1. 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
    
    
  2. reviewboard/notifications/email.py (Diff revision 4)
     
     
    Show all issues
    Col: 41
     E126 continuation line over-indented for hanging indent
    
    1. Dropped as this is similar to the line above this
  3. reviewboard/notifications/email.py (Diff revision 4)
     
     
    Show all issues
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. reviewboard/notifications/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
    1. Trying to squeeze this will make the text not very meaningful. Not sure how to handle this (I dropped this in an earlier diff for the same review)
  5. reviewboard/notifications/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  6. 
      
david
  1. Ship It!
  2. 
      
RA
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.7.x (4666a7e). Thanks!