Summary

Add line numbers to reviews in text emails.

Review Request #6864 — Created Jan. 31, 2015 and submitted Feb. 16, 2015, 11:27 a.m.

Information

Owner

Sunxperous

Repository

Review Board

Branch

master

Bugs

3739

Depends On

Commit

f3babef...

Reviewers

Groups

reviewboard, students

People

Description

When a review has comments on the source code, text email notifications only show the files that have been changed, while HTML email notifications show the diff view. This change adds the line numbers of the patched files that have been commented on in a review to the email text body.

Now, the line numbers are shown as follows:

<file name> (Lines 18 - 25)

A new template tag has been implemented to render the line numbers of the code that have been commented on. It will render singular "Line" or plural "Lines" or none depending on the changes.

In reviewboard/reviews/views.py, we have added and exposed the chunks attribute to each comment entry within comment_entries. Also, the text email template will now make use of this new exposed attribute to render the individual commented files with line number information in each review.

While testing this, we found a new bug:
https://code.google.com/p/reviewboard/issues/detail?id=3748

Testing Done

No unit tests have been written.

Manual testing has been conducted:
When no lines are selected in a review, nothing is affected.
When a range of lines over multiple chunks are selected, the line numbers display properly.
Chunks which have been deleted will not have their original line numbers shown, only line numbers in the patched file will be shown.

Issues

Description	From	Last Updated
Col: 1 W391 blank line at end of file	reviewbot	Jan. 31, 2015, 7:04 p.m.
Can you combine these? """Renders ...	david	Jan. 31, 2015, 9:07 p.m.
Remove this blank line.	david	Jan. 31, 2015, 9:07 p.m.
Can we wrap the format strings in gettext? We already import it, so you can just do _('(Line %d)')	david	Jan. 31, 2015, 9:07 p.m.
We probably need to define an empty 'chunks' list here, in case there was an exception in get_line_counts() or get_file_chunks_in_range()	david	Jan. 31, 2015, 9:07 p.m.
Blank line between a statement and a new block	brennie	Feb. 6, 2015, 9:37 p.m.
Has the error template changed to require the chunks parameter?	brennie	Feb. 6, 2015, 9:37 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

Removed the blank lines.

Commit:

289bee879b18e581a68d7a63c5b883f8c7741c43

3cd06e630df317f57bdaa93e73fb340cb9e3c60a

Diff:

Revision 2 (+43 -11)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/notifications/review_email.txt

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you combine these? """Renders ...
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Remove this blank line.
```

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we wrap the format strings in gettext? We already import it, so you can just do _('(Line %d)')

reviewboard/reviews/views.py (Diff revision 2)

The issue has been resolved. Show all issues

We probably need to define an empty 'chunks' list here, in case there was an exception in get_line_counts() or get_file_chunks_in_range()

Description:

		When a review has comments on the source code, text email notifications only show the files that have been changed, while HTML email notifications show the diff view. This change adds the line numbers of the patched files that have been commented on in a review to the email text body.

		Now, the line numbers are shown as follows:

		`<file name> (Lines 18 - 25)`

		A new template tag has been implemented to render the line numbers of the code that have been commented on. It will render singular "Line" or plural "Lines" or none depending on the changes.

		In reviewboard/reviews/views.py, we have added and exposed the `chunks` attribute to each comment entry within `comment_entries`. Also, the text email template will now make use of this new exposed attribute to render the individual commented files with line number information in each review.

~		While testing this, we found a new bug where a single review request with multiple commit history will cause its line numbers to render extra lines if that line is part of a chunk from an older commit.
	~	While testing this, we found a new bug:
	+	https://code.google.com/p/reviewboard/issues/detail?id=3748

Commit:

3cd06e630df317f57bdaa93e73fb340cb9e3c60a

dd43bec1fba7260cab051f44d0cfb73ca45a3fa1

Diff:

Revision 3 (+42 -11)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/notifications/review_email.txt

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between a statement and a new block
```

reviewboard/reviews/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Has the error template changed to require the chunks parameter?

Sunxperous Feb. 1, 2015, 10:04 a.m.

No, the error template does not require the chunks parameter. Neither does it contain the domain and domain_method parameter, but those are still passed into the renderer. Should they be removed?

Groups:: students

Change Summary:

Added a line break as reviewed, and removed the chunks parameter since the error template has not been modified to display any information regarding it.

Commit:

dd43bec1fba7260cab051f44d0cfb73ca45a3fa1

f3babef4f1a1fddeb9d600fb5fb24850732a75f0

Diff:

Revision 4 (+42 -11)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/notifications/review_email.txt

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/notifications/review_email.txt

Ship it!

```
Looks good to me!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (c396156)