Summary

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 Oct. 13, 2015, 11:45 a.m.

Information

Owner

sherman

Repository

Review Board

Branch

release-2.0.x

Bugs

3904

Depends On

Reviewers

Groups

reviewboard, students

People

Description

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!"

Testing Done

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.

Issues

Description	From	Last Updated
Col: 5 E303 too many blank lines (2)	reviewbot	Oct. 9, 2015, 2:14 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 9, 2015, 2:14 p.m.
Col: 14 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 9, 2015, 2:14 p.m.
With this change, we're now showing "Fix it, then ship it!" always, even if the review didn't include the "ship …	david	Oct. 9, 2015, 3:33 p.m.
Col: 13 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 9, 2015, 3:39 p.m.
Col: 13 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 9, 2015, 3:39 p.m.
Col: 21 E701 multiple statements on one line (colon)	reviewbot	Oct. 9, 2015, 3:39 p.m.
You fixed the .html template but it needs the same fix here.	david	Oct. 10, 2015, 9:32 a.m.
Likewise here. Also, we can put the <p> inside the first {% if %} block tag.	brennie	Oct. 11, 2015, 9:22 a.m.
The indentation of this is off. it should be: ``` {% if ship_it %} {% if has_issues %} Fix it, …	brennie	Oct. 11, 2015, 9:22 a.m.

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

reviewboard/notifications/email.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

reviewboard/notifications/email.py (Diff revision 1)

Show all issues

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

reviewboard/notifications/email.py (Diff revision 1)

Show all issues

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

Diff:

Revision 2 (+16 -2)

Show changes

	reviewboard/notifications/email.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

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

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 %}

sherman Oct. 9, 2015, 3:15 p.m.

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

david Oct. 9, 2015, 3:19 p.m.

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.

Change Summary:

Updated to display no message if the user does not select "Ship it!" and there are no issues.

Diff:

Revision 3 (+17 -2)

Show changes

	reviewboard/notifications/email.py
	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

reviewboard/notifications/email.py (Diff revision 3)

Show all issues

Col: 13
 E131 continuation line unaligned for hanging indent

reviewboard/notifications/email.py (Diff revision 3)

Show all issues

Col: 13
 E131 continuation line unaligned for hanging indent

reviewboard/notifications/email.py (Diff revision 3)
Show all issues
```
Col: 21
 E701 multiple statements on one line (colon)
```

Change Summary:

Update code for PEP 8 compliance

Diff:

Revision 4 (+15 -2)

Show changes

	reviewboard/notifications/email.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

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

Change Summary:

Improve summary of the changes

Summary:

-	Fix EasyFix bug with incorrect message in email
+	When user opens an issue and selects "Ship it!", the email will say "Fix it, then Ship it!"

Description:

~		Fix EasyFix bug with incorrect message in email
	~	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!"
-
-		Fixed the bug so that when user opens and 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.

reviewboard/templates/notifications/review_email.txt (Diff revision 4)

Show all issues

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

Change Summary:

Update review_email.txt to reflect review_email.html

Diff:

Revision 5 (+18 -2)

Show changes

	reviewboard/notifications/email.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

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

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.

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 %}

Change Summary:

Fixed indentation and formatting for review_email html and txt

Diff:

Revision 6 (+21 -3)

Show changes

	reviewboard/notifications/email.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (8f2dd35)