When user opens an issue and selects "Ship it!", the email will say "Fix it, then Ship it!"

Review Request #7685 — Created Oct. 9, 2015 and submitted

Information

Review Board
release-2.0.x

Reviewers

When user opens an issue and selects "Ship it!", the email will say "Fix it, then Ship it!" This bug was fixed by checking if the user opened an issue and saying "Fix it, then Ship it!" if they did.

Bug Description:
1. A user makes a review request
2. A reviewer opens an issue but gives a "Ship it!"
3. The review request page updates with the "Fix it, then Ship it!" label on the submitted review
4. The sent notification email incorrectly labels the review as "Ship it!". It should read "Fix it, then Ship it!"

Tested the behavior using the local development server and printed email to the terminal. The emails were sent with the correct content.

Ran and passed all unit tests.

Description From Last Updated

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

With this change, we're now showing "Fix it, then ship it!" always, even if the review didn't include the "ship …

daviddavid

Col: 13 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 13 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 21 E701 multiple statements on one line (colon)

reviewbotreviewbot

You fixed the .html template but it needs the same fix here.

daviddavid

Likewise here. Also, we can put the <p> inside the first {% if %} block tag.

brenniebrennie

The indentation of this is off. it should be: ``` {% if ship_it %} {% if has_issues %} Fix it, …

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  4. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Show all issues
    Col: 14
     E126 continuation line over-indented for hanging indent
    
  5. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/templates/notifications/review_email.txt (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    With this change, we're now showing "Fix it, then ship it!" always, even if the review didn't include the "ship it" flag. What we should have is a nested conditional:

    {% if ship_it %}
    {%  if has_issues %}
    Fix it, then Ship it!
    {%  else %}
    Ship it!
    {%  endif %}
    {% endif %}
    
    1. The can_ship_it flag in the mail_review method of email.py checks for issues like so:

      can_ship_it = (                                                            
           review.ship_it and                                                     
           not (review.comments.filter(issue_opened=True).exists() or             
                (review.file_attachment_comments                                  
                 .filter(issue_opened=True)                                       
                 .exists()) or                                                    
                review.screenshot_comments.filter(issue_opened=True).exists())    
       )
      

      The flag is then used in the html template

    2. Given this logic, if there's a ship-it and no issues, then it will print "Ship it", and otherwise, it will print "Fix it, then ship it". This is incorrect in the case where the "ship-it" check-box was not checked at all. In that case, we don't want to include either of these lines.

  3. 
      
SH
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Show all issues
    Col: 13
     E131 continuation line unaligned for hanging indent
    
  3. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Show all issues
    Col: 13
     E131 continuation line unaligned for hanging indent
    
  4. reviewboard/notifications/email.py (Diff revision 3)
     
     
    Show all issues
    Col: 21
     E701 multiple statements on one line (colon)
    
  5. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. 
      
SH
david
  1. 
      
  2. reviewboard/templates/notifications/review_email.txt (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    You fixed the .html template but it needs the same fix here.

  3. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/templates/notifications/review_email.html (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues

    Likewise here. Also, we can put the <p> inside the first {% if %} block tag.

  3. reviewboard/templates/notifications/review_email.txt (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues

    The indentation of this is off. it should be:

    ```
    {% if ship_it %}
    {% if has_issues %}
    Fix it, then Ship it!
    {% else %}
    Ship it!
    {% endif %}
    {% endif %}

  4. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    Ignored Files:
        reviewboard/templates/notifications/review_email.txt
        reviewboard/templates/notifications/review_email.html
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
SH
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (8f2dd35)