Summary

Add visual indicators to new page entries and new comments, in order to highlight active/new discussions.

Review Request #8673 — Created Jan. 28, 2017 and discarded Sept. 8, 2017, 4:32 p.m.

Information

Owner

szhang

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

On review requests, it can be difficult to discern new discussion from the old. This particularly pertains to finding new reviews and new comments on review requests with a lot of discussion.
This set of changes adds visual indicators/highlighting to all page entries and all comments that were posted since the user last visited the review request.
It essentially does this by adding reviews and comments to a CSS class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.

Testing Done


Checked if last_visited is passed down to the appropriate templates correctly.
Checked if the new-page-entry class and the new-comment class are applied when the conditional is true.
Checked if the page is updated with the correct styling when page-entries and comments are newer than last_visited (ie. indicators are added).
Checked if the visual indicators are removed when page-entries and comments are older than last_visited (ie. page refreshed on a page with page-entries/comments with the indicators).
Checked if the visual indicators are added and removed correctly in spite of any caching that might happen (ie. caching does not interfere with addition/removal).
Ran unit tests for checking if new-page-entry and new-comment are both added and removed correctly.

Files

Issues

Description	From	Last Updated
I think if both the review and replies are new, we can just leave the whole box shaded and avoid …	david	Feb. 8, 2017, 4:18 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Jan. 28, 2017, 2:31 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Jan. 28, 2017, 2:31 p.m.
Col: 80 E501 line too long (116 > 79 characters)	reviewbot	Jan. 28, 2017, 2:31 p.m.
Col: 114 E202 whitespace before ')'	reviewbot	Jan. 28, 2017, 2:31 p.m.
Col: 77 W291 trailing whitespace	reviewbot	Jan. 28, 2017, 2:38 p.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	Jan. 28, 2017, 2:38 p.m.
Col: 74 W291 trailing whitespace	reviewbot	Jan. 28, 2017, 2:39 p.m.
Undo this change.	brennie	Feb. 7, 2017, 1:04 p.m.
No newline here.	brennie	Feb. 7, 2017, 1:04 p.m.
CSS selectors should be ordered as: &.foo {} a {} .foo {} #foo{} Within each group the selectors should be …	brennie	Feb. 7, 2017, 1:07 p.m.
I do not know that this comment. is necessary.	brennie	Feb. 7, 2017, 1:07 p.m.
Alphabetize.	brennie	Feb. 7, 2017, 1:07 p.m.
The draft rendering and the new-comment rendering should be mutually exclusive. Additionally, producing an element with two different class= attribtues …	brennie	Feb. 7, 2017, 1:10 p.m.
This line looks like it got unintentionally indented 1 space.	david	Feb. 8, 2017, 4:18 p.m.
It doesn't seem useful to have separate definitions for entry and comment which are the same. Can we merge these?	david	Feb. 8, 2017, 6:37 p.m.
The #reviews .review-request-page-entry part can be consolidated, like: #reviews .review-request-page-entry { &.new-page-entry { ... } &:not(.new-page-entry) { ... } }	chipx86	Feb. 19, 2017, 6:53 p.m.
The 4px should also be in a constant.	chipx86	Feb. 19, 2017, 6:54 p.m.
What's this here for?	chipx86	March 3, 2017, 11:32 p.m.
Maybe when wrapping and using multiple keyword arguments, you can have one keyword argument per line since there are lots …	RK rkdhatt	March 4, 2017, 6:48 p.m.
'FileAttachment' imported but unused	reviewbot	March 9, 2017, 10:59 p.m.
'Screenshot' imported but unused	reviewbot	March 9, 2017, 10:59 p.m.
'ReviewRequest' imported but unused	reviewbot	March 9, 2017, 10:59 p.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 9, 2017, 10:59 p.m.
For unit test descriptions (modern ones, anyway), we try to start off with the component/page that's being tested, followed by …	chipx86	March 12, 2017, 7:05 p.m.
self.client.login will return the user, so you can use that directly. Same below.	chipx86	March 12, 2017, 7:05 p.m.
These can be one statement. Same below.	chipx86	March 10, 2017, 11:06 p.m.
local variable 'review' is assigned to but never used	reviewbot	March 9, 2017, 11:01 p.m.
Older tests don't do this, but we should actually use local_site_reverse to get the URL instead of hard-coding it. Also, …	chipx86	March 11, 2017, 12:19 a.m.
It would be better to check for something more specific here (and below), since "new-page-entry" could theoretically be anywhere.	chipx86	March 12, 2017, 10:55 p.m.
Can you wrap the comparison in parenthesis? Helps to visually distinguish those.	chipx86	March 10, 2017, 9:49 p.m.
Let's put the ( on the line with the format strings. That way, all entries have a nice 4 space …	chipx86	March 10, 2017, 9:56 p.m.
Can you add a comment about this? I know we talked about this a bit, but wouldn't we want this …	chipx86	March 14, 2017, 12:10 p.m.
There's actually a little template tag you can use here for the class building: <li{% attr "class" %}{% if draft …	chipx86	March 10, 2017, 10:53 p.m.
'FileAttachment' imported but unused	reviewbot	March 12, 2017, 11:08 p.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 12, 2017, 11:08 p.m.
'ReviewRequest' imported but unused	reviewbot	March 12, 2017, 11:08 p.m.
'Screenshot' imported but unused	reviewbot	March 12, 2017, 11:08 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 12, 2017, 11 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 12, 2017, 11 p.m.
'FileAttachment' imported but unused	reviewbot	March 12, 2017, 11:09 p.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 12, 2017, 11:09 p.m.
'ReviewRequest' imported but unused	reviewbot	March 12, 2017, 11:09 p.m.
'Screenshot' imported but unused	reviewbot	March 12, 2017, 11:09 p.m.
I would use docstring comment here (triple quotes)	MO Mons	March 13, 2017, 5:09 p.m.
maybe you should remove this blank line (unless that's the style guideline used elsewhere)	MO Mons	March 13, 2017, 5:15 p.m.
I like the fact that you put these on new lines. Makes it more readable in my opinion.	MO Mons	March 13, 2017, 5:12 p.m.
I would add a new blank line here. Seems like that's the general style in this file (blank line after …	MO Mons	March 13, 2017, 5:43 p.m.
is it possible to split this div into two lines?	MO Mons	March 13, 2017, 5:48 p.m.
same here.	MO Mons	March 13, 2017, 5:48 p.m.
is it possible to split this li tag into two lines?	MO Mons	March 13, 2017, 5:48 p.m.
'FileAttachment' imported but unused	reviewbot	March 13, 2017, 7:02 p.m.
'Screenshot' imported but unused	reviewbot	March 13, 2017, 7:02 p.m.
'ReviewRequest' imported but unused	reviewbot	March 13, 2017, 7:02 p.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 13, 2017, 7:02 p.m.
We prefer to format formatting statements like this as : text = ( ' .... ' % foo )	brennie	March 13, 2017, 6:51 p.m.
Same here.	brennie	March 13, 2017, 6:51 p.m.
And here.	brennie	March 13, 2017, 6:51 p.m.
Here too.	brennie	March 13, 2017, 6:51 p.m.
ETag	brennie	March 13, 2017, 6:53 p.m.
'FileAttachment' imported but unused	reviewbot	March 14, 2017, 8:27 a.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 14, 2017, 8:27 a.m.
'ReviewRequest' imported but unused	reviewbot	March 14, 2017, 8:27 a.m.
'Screenshot' imported but unused	reviewbot	March 14, 2017, 8:27 a.m.
'FileAttachment' imported but unused	reviewbot	March 14, 2017, 12:16 p.m.
'ReviewRequestDraft' imported but unused	reviewbot	March 14, 2017, 12:20 p.m.
'ReviewRequest' imported but unused	reviewbot	March 14, 2017, 12:20 p.m.
'Screenshot' imported but unused	reviewbot	March 14, 2017, 12:20 p.m.
'FileAttachment' imported but unused	reviewbot	March 14, 2017, 4:03 p.m.
'search' imported but unused	reviewbot	March 22, 2017, 1:36 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	March 22, 2017, 1:35 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	March 22, 2017, 1:36 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	March 22, 2017, 1:36 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	March 22, 2017, 1:36 p.m.

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (116 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 114
 E202 whitespace before ')'
```

Commit:

8d8857fa60927ceb0d204a695d3948f1bf3a2c22

ae29947085b2875e1b7abfea81b790d3fd0f1969

Diff:

Revision 2 (+17 -3)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html

reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 77
 W291 trailing whitespace
```
reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 9
 E128 continuation line under-indented for visual indent
```

Commit:

ae29947085b2875e1b7abfea81b790d3fd0f1969

39feb29627bd98d83d84526f75725da47ba75ac2

Diff:

Revision 3 (+18 -4)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html

reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 74
 W291 trailing whitespace
```

Change Summary:

Add LessCSS styling changes

Clean up code (eg. removing debug output)

Commit:

39feb29627bd98d83d84526f75725da47ba75ac2

89e5bf9d6b739a7a44bda454f3b848cfb509f1db

Diff:

Revision 4 (+42 -5)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

Change Summary:


Remove WIP from this review request. :)
Update Description and Testing Done.
Add screenshots of the change
Remove statement for debugging
Clean up other small code style-related issues.

Summary:

[WIP] Add visual indicators to new reviews and new comments, in order to highlight active/new discussions

Add visual indicators to new reviews and new comments, in order to highlight active/new discussions.

Description:

		On review requests, it can be difficult to discern new discussion from the old. This particularly pertains to finding new reviews and new comments on review requests with a lot of discussion.

~		This set of changes is a work-in-progress, but is the first step to adding visual indicators to new reviews/comments. It adds reviews and comments to a class if they were posted since the user last visited the review request. This allows for styling to occur as the next step, for example, but this may change depending on how the visual indicator should look like.
	~	This set of changes adds visual indicators to all page entries and all comments that were posted since the user last visited the review request.

~		Currently, the conditional specifying if a review or comment is inverted for debugging purposes. Some debugging output is still left in the code as this code is still a work-in-progress.
	~	It does this by adding reviews and comments to an HTML class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.

Testing Done:

~		Checked if `last_visited` is passed down to the appropriate templates correctly.
~		Checked if the `new-page-entry` class and the `new-comment` class are applied when the conditional is true.
	~	Checked if `last_visited` is passed down to the appropriate templates correctly.
	~	Checked if the `new-page-entry` class and the `new-comment` class are applied when the conditional is true.
	+	Checked if the page is updated with the correct styling when page-entries and comments are newer than `last_visited`.

Commit:

89e5bf9d6b739a7a44bda454f3b848cfb509f1db

ff45a2313339d4400f600d73c79cb3c7d906a4e3

Diff:

Revision 5 (+40 -5)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

Added Files:

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)
The issue has been resolved. Show all issues
```
No newline here.
```

reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)

The issue has been resolved. Show all issues

CSS selectors should be ordered as:

&.foo {}
a {}
.foo {}
#foo{}


Within each group the selectors should be ordered alphabetically.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)
The issue has been resolved. Show all issues
```
I do not know that this comment. is necessary.
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Alphabetize.
```

reviewboard/templates/reviews/review_reply.html (Diff revision 5)

The issue has been resolved. Show all issues

The draft rendering and the new-comment rendering should be mutually exclusive. Additionally, producing an element with two different class= attribtues is not valid. Move the class attribute generation into an if-elif.

Change Summary:

Address the issues raised by Barret.

Commit:

ff45a2313339d4400f600d73c79cb3c7d906a4e3

de579bc886d29014c5a453a34d1785be3ebe8449

Diff:

Revision 6 (+38 -4)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

Ship it!

```
Ship It!
```

A review and comments with the indicators applied. (Revision 1)

The issue has been resolved. Show all issues

I think if both the review and replies are new, we can just leave the whole box shaded and avoid the extra border in the middle.

reviewboard/templates/reviews/boxes/review.html (Diff revision 6)
The issue has been resolved. Show all issues
```
This line looks like it got unintentionally indented 1 space.
```

Change Summary:

Fix issues raised in David's review

Commit:

de579bc886d29014c5a453a34d1785be3ebe8449

d69b755ef2a5008d461a07763a8ba4ab42dfe066

Diff:

Revision 7 (+37 -3)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

Added Files:

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/static/rb/css/pages/reviews.less (Diff revision 7)

The issue has been resolved. Show all issues

It doesn't seem useful to have separate definitions for entry and comment which are the same. Can we merge these?

szhang Feb. 8, 2017, 6:44 p.m.

This was intentional, for the future in the case when someone wanted to set them each to different colours.

But you're right, that doesn't seem very useful, especially when we don't need it right now.

Change Summary:


Fix the caption in the updated image.
Make page-entries and comments use the same LESS variables, as they were using the same colours.

Commit:

d69b755ef2a5008d461a07763a8ba4ab42dfe066

43aa75a11e3c790b6b29c03d1fec8be7777de7c7

Diff:

Revision 8 (+35 -3)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

Added Files:

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)

The issue has been resolved. Show all issues

The #reviews .review-request-page-entry part can be consolidated, like:

#reviews .review-request-page-entry {
    &.new-page-entry {
        ...
    }

    &:not(.new-page-entry) {
        ...
    }
}

reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
The issue has been resolved. Show all issues
```
The 4px should also be in a constant.
```

reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)

The issue has been dropped. Show all issues

What's this here for?

szhang Feb. 19, 2017, 7:44 p.m.

I put this here because setting clear: both (which is done in #reviews .review .body) causes a gap between the header and footer of reviews. This isn't normally visible, but adding a left border to both .header and .body shows the gap.

There are no visible differences I can see when I set this to clear: none, besides the gap disappearing. It might be legacy thing. However, I should have brought this up earlier, this isn't the best solution.

Is there a better way to do this?

chipx86 March 3, 2017, 6:06 p.m.

Can you show me in Slack what it looks like with/without this, so I can get a better idea of its impact?

Also, can you check in mobile mode? (Resize your browser to be smaller, width-wise, or go into the developer tools' Responsive Design Mode.) It may be that we have this for that purpose.

Change Summary:


Update etag set before the review request page is rendered to include whether an entry has been posted since the user last visited the review request.
Address issues raised by Christian.
Update summary/description/testing done for clarification.

Summary:

Add visual indicators to new reviews and new comments, in order to highlight active/new discussions.

Add visual indicators to new page entries and new comments, in order to highlight active/new discussions.

Description:

		On review requests, it can be difficult to discern new discussion from the old. This particularly pertains to finding new reviews and new comments on review requests with a lot of discussion.

~		This set of changes adds visual indicators to all page entries and all comments that were posted since the user last visited the review request.
	~	This set of changes adds visual indicators/highlighting to all page entries and all comments that were posted since the user last visited the review request.

~		It does this by adding reviews and comments to an HTML class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.
	~	It essentially does this by adding reviews and comments to an HTML class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.

Testing Done:

		Checked if `last_visited` is passed down to the appropriate templates correctly.
		Checked if the `new-page-entry` class and the `new-comment` class are applied when the conditional is true.
~		Checked if the page is updated with the correct styling when page-entries and comments are newer than `last_visited`.
	~	Checked if the page is updated with the correct styling when page-entries and comments are newer than `last_visited` (ie. indicators are added).
	+	Checked if the visual indicators are removed when page-entries and comments are older than `last_visited` (ie. page refreshed on a page with page-entries/comments with the indicators).

Commit:

43aa75a11e3c790b6b29c03d1fec8be7777de7c7

483085f22619b5cedce96b86e607e992751f6abf

Diff:

Revision 9 (+51 -7)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

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

The issue has been resolved. Show all issues

Maybe when wrapping and using multiple keyword arguments, you can have one keyword argument per line since there are lots of arguments (better readability).

ex:
etag = encode_tag(
'%s:%s:%s:%s:%s:%s:%s:%s:%s:%s:%s' %
request.user,
last_activity_time,
draft_timestamp,
...
)

mike_conley March 4, 2017, 10:25 a.m.

I agree with that point - though I wouldn't necessarily put the '%s...' string on a next line, like in the example above.

mike_conley March 4, 2017, 10:27 a.m.

Holy smokes, ignore my last comment about putting the %s... string on the next line. I didn't realize it was already that way (and properly indented the 4 spaces).

So please ignore my comment there. But I do support the idea of breaking up the individual elements being inserted into the string onto their own lines.

szhang March 4, 2017, 6:59 p.m.

Thanks for the feedback, guys!

I did not format everything according to Raman's example, but I did put each argument on its own line.

Mike, it seems that the '%s:%s... was NOT indented 4 spaces. It was indented 3 spaces for whatever reason. That's been corrected now.

Change Summary:

Fix styling related issue raised by Raman.

Commit:

483085f22619b5cedce96b86e607e992751f6abf

6bb47e299b013b501b8c8277e113457a504f0c0b

Diff:

Revision 10 (+60 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

Change Summary:


Add unit tests for checking if new-page-entry and new-comment are both added and removed correctly.

Testing Done:

		Checked if `last_visited` is passed down to the appropriate templates correctly.
		Checked if the `new-page-entry` class and the `new-comment` class are applied when the conditional is true.
		Checked if the page is updated with the correct styling when page-entries and comments are newer than `last_visited` (ie. indicators are added).
		Checked if the visual indicators are removed when page-entries and comments are older than `last_visited` (ie. page refreshed on a page with page-entries/comments with the indicators).
	+	Checked if the visual indicators are added and removed correctly in spite of any caching that might happen (ie. caching does not interfere with addition/removal).
	+	Ran unit tests for checking if `new-page-entry` and `new-comment` are both added and removed correctly.

Commit:

6bb47e299b013b501b8c8277e113457a504f0c0b

19d05b00569d58168f408ecd276211039d7bcd09

Diff:

Revision 11 (+118 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
 local variable 'review' is assigned to but never used
```

This is looking great! Some recommendations for styling and just some conventions we have in our codebase, but this is close to landing!

reviewboard/reviews/tests/test_views.py (Diff revision 11)

The issue has been resolved. Show all issues

For unit test descriptions (modern ones, anyway), we try to start off with the component/page that's being tested, followed by the condition. So, something more like: "Testing review_detail view sets new-page-entry CSS class on reviews when ..."
("CSS class" helps distinguish from Python classes.)
There should also be one unit test per condition, so one for ensuring the class is set when it should be, one for ensuring the class is not set when it you've visited since. It's fine to repeat some of the setup logic that may have been covered in another unit test. What having multiple tests ultimately does is help when things go wrong. For instance, if the latter case fails, but the former doesn't, there's less diagnostics that have to happen to know what actually failed.
Same applies below.

reviewboard/reviews/tests/test_views.py (Diff revision 11)

The issue has been dropped. Show all issues

self.client.login will return the user, so you can use that directly.
Same below.

szhang March 11, 2017, 12:18 a.m.

It is giving me errors when I set

user = self.client.login(username='doc', password='doc')

instead of what I had originally.

self.client.login is returning a boolean whether or not the login was successful, rather than the user. That is what the Django docs say: https://docs.djangoproject.com/en/1.10/topics/testing/tools/#django.test.Client.login

I also printed user to console and I get True.

reviewboard/reviews/tests/test_views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
These can be one statement.
Same below.
```

reviewboard/reviews/tests/test_views.py (Diff revision 11)

The issue has been resolved. Show all issues

Older tests don't do this, but we should actually use local_site_reverse to get the URL instead of hard-coding it.

Also, when referencing IDs, you should use .pk instead of .id. With review requests, though, when you're dealing with URLs or for display purposes, you should always use .display_id.

Same below.

reviewboard/reviews/tests/test_views.py (Diff revision 11)

The issue has been resolved. Show all issues

It would be better to check for something more specific here (and below), since "new-page-entry" could theoretically be anywhere.

reviewboard/reviews/views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Can you wrap the comparison in parenthesis? Helps to visually distinguish those.
```

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

The issue has been resolved. Show all issues

Let's put the ( on the line with the format strings. That way, all entries have a nice 4 space indentation.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 11)

The issue has been resolved. Show all issues

Can you add a comment about this?

I know we talked about this a bit, but wouldn't we want this to actually be in the base CSS for this element, rather than right here? It seems like we'd need to be consistent. Either always have it or never.

szhang March 10, 2017, 10:40 p.m.

Can you clarify what you mean by "the base CSS for this element"? Do you mean #reviews .review .body or do you mean something else?

chipx86 March 14, 2017, 1:27 a.m.

Yeah, #reviews .review .body. Basically, from the screenshot you showed me, clear: none is needed to solve a visual defect. That means it's either not at all required and there's no reason to clear here (so clear: none is appropriate) or we must clear here to work around something and that thing we're working around will still be broken for new entries (because you're undoing the clear: both). It's either correct to clear in both places, or it's incorrect to clear in both places, but there must be consistency.

I think we don't need the clear: both that we have in #reviews .review .body. That line has been there a long time, and I believe it's from when we used to show a little Ship It emblem on the top-left of the review. So instead of having clear: none here, just remove the clear: both.

szhang March 14, 2017, 12:40 p.m.

Thanks for the clarification. I agree. It's been removed now.

reviewboard/templates/reviews/review_reply.html (Diff revision 11)

The issue has been resolved. Show all issues

There's actually a little template tag you can use here for the class building:

<li{% attr "class" %}{% if draft %}draft{% elif last_visited <= timestamp %}new-comment{% endif %}{% endattr %} ...>


That will conditionally add the "class" attribute if there's any content to put in it.

Change Summary:


Resolve issues raised by Christian.

Commit:

19d05b00569d58168f408ecd276211039d7bcd09

7bdfa675bc9268ad8a2683754e45e8af2cd11c97

Diff:

Revision 12 (+212 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests/test_views.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:


Resolve issues raised by Review Bot.

Commit:

7bdfa675bc9268ad8a2683754e45e8af2cd11c97

369cec52218d50b61ad7f49bd0c83f485313a960

Diff:

Revision 13 (+215 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```

reviewboard/reviews/tests/test_views.py (Diff revision 13)

The issue has been dropped. Show all issues

I would use docstring comment here (triple quotes)

szhang March 13, 2017, 5:09 p.m.

I thought docstrings were only for "public modules, functions, classes, and methods" (from https://www.python.org/dev/peps/pep-0008/#documentation-strings)?
This is a block comment, so I think using only #'s works here.

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

The issue has been resolved. Show all issues

maybe you should remove this blank line (unless that's the style guideline used elsewhere)

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

The issue has been dropped. Show all issues

I like the fact that you put these on new lines. Makes it more readable in my opinion.

szhang March 13, 2017, 5:51 p.m.

Thank Raman! This was her suggestion! :)

reviewboard/static/rb/css/pages/reviews.less (Diff revision 13)

The issue has been resolved. Show all issues

I would add a new blank line here. Seems like that's the general style in this file (blank line after a multiline comment).

reviewboard/templates/reviews/boxes/change.html (Diff revision 13)

The issue has been dropped. Show all issues

is it possible to split this div into two lines?

szhang March 13, 2017, 5:57 p.m.

I don't think we're concerned with any character limit for each line when it comes to HTML. There are a lot of other templates that have even longer elements on some lines.

reviewboard/templates/reviews/boxes/review.html (Diff revision 13)
The issue has been dropped. Show all issues
```
same here.
```
reviewboard/templates/reviews/review_reply.html (Diff revision 13)
The issue has been dropped. Show all issues
```
is it possible to split this li tag into two lines?
```

Change Summary:


Small style fixes to issues raised by Mons.

Commit:

369cec52218d50b61ad7f49bd0c83f485313a960

756267b77823bff6994d6bd9cea72751f8ee07f8

Diff:

Revision 14 (+215 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```

reviewboard/reviews/tests/test_views.py (Diff revision 14)

The issue has been resolved. Show all issues

We prefer to format formatting statements like this as :

text = (
    ' .... '
    % foo
)

reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/reviews/tests/test_views.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Here too.
```
reviewboard/reviews/views.py (Diff revision 14)
The issue has been resolved. Show all issues
```
ETag
```

Change Summary:


Resolve issues raised by Barret.

Description:

		On review requests, it can be difficult to discern new discussion from the old. This particularly pertains to finding new reviews and new comments on review requests with a lot of discussion.

		This set of changes adds visual indicators/highlighting to all page entries and all comments that were posted since the user last visited the review request.

~		It essentially does this by adding reviews and comments to an HTML class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.
	~	It essentially does this by adding reviews and comments to a CSS class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.

Commit:

756267b77823bff6994d6bd9cea72751f8ee07f8

bd07115f24f8d687597fcc6153303f5585a1dbdb

Diff:

Revision 15 (+225 -12)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```

Change Summary:


Remove clear: both from #reviews .review .body as mentioned in Christian's review. This is old code that is unnecessary now.

Commit:

bd07115f24f8d687597fcc6153303f5585a1dbdb

d80324206127b7275ebc3c8b981978351eb64179

Diff:

Revision 16 (+215 -14)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 16)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 16)
The issue has been dropped. Show all issues
```
 'ReviewRequestDraft' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 16)
The issue has been dropped. Show all issues
```
 'ReviewRequest' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 16)
The issue has been dropped. Show all issues
```
 'Screenshot' imported but unused
```

Commit:

d80324206127b7275ebc3c8b981978351eb64179

c432541992727b0c9258cf85481381d83223453f

Diff:

Revision 17 (+216 -15)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 17)
The issue has been dropped. Show all issues
```
 'FileAttachment' imported but unused
```

Change Summary:


Merge rebasing.

Commit:

c432541992727b0c9258cf85481381d83223453f

fd1b1b1e375a9f26f2b8a98bbbf17b6e6610980a

Diff:

Revision 18 (+215 -14)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

Change Summary:


Use regular expressions to make checking for CSS classes more general yet still precise.
Note that the unit tests for the review replies have been changed from ..._new_replies to ..._new_review_replies.

Commit:

fd1b1b1e375a9f26f2b8a98bbbf17b6e6610980a

ada23dd18386e45125d7e5893a971f61e44cac12

Diff:

Revision 19 (+232 -14)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

reviewboard/reviews/tests/test_views.py (Diff revision 19)
The issue has been resolved. Show all issues
```
 'search' imported but unused
```
reviewboard/reviews/tests/test_views.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/reviews/tests/test_views.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/reviews/tests/test_views.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/reviews/tests/test_views.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```

Change Summary:


Fix issues raised by ReviewBot.

Commit:

ada23dd18386e45125d7e5893a971f61e44cac12

8d5e735fb8f47d1ef66d62234bbf73472b27a916

Diff:

Revision 20 (+232 -14)

Show changes

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/boxes/change.html
	reviewboard/templates/reviews/boxes/review.html

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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html



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

Ignored Files:
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/change.html

Status:: Discarded
Change Summary:: Superceded by https://reviews.reviewboard.org/r/9182/