Summary:

Improve diff comment bubble tooltips to show rendered comment content

Review Request #6932 — Created Feb. 10, 2015 and submitted March 26, 2015, 12:36 p.m.

Information
Owner:	wudi
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Commit:	f1b0a6b...
Reviewers
Groups:	reviewboard, students
People:

Description

This is the first part in improving the diff comment bubbles.

The tooltip previews for the comment text will show the full rendered comment text together with the name of the reviewer, with the size of the tooltip maximized to the screen width.

Updated js tests as the comment reviewer needs to be loaded with each comment.

Testing Done

Ran tests, added markdown and plain text comments to check visually.

Added unit testing for XSS vulnerabilites. Manually checked by creating comments containing script tags for both markdown and plain text, scripts were escaped and not executed for both first load of the page and subsequent asynchronous updating of comments.

Files

Issues

Description	From	Last Updated
Can we set a maximum width for this popup? The entire width of the page is a little excessive.	david	Feb. 22, 2015, 2:37 a.m.
Can you go through and make sure that the styles of this match exactly what you see on the "View …	david	March 13, 2015, 11:18 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 11, 2015, 12:13 a.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 11, 2015, 12:13 a.m.
Col: 25 E131 continuation line unaligned for hanging indent	reviewbot	Feb. 11, 2015, 12:13 a.m.
This will allow non-Markdown text to inject tags into the page. Make sure you escape the text.	chipx86	March 1, 2015, 8:37 p.m.
Blank line between these. Though, you probably want RB.UserSession.instance instead.	chipx86	March 1, 2015, 8:37 p.m.
Can you make use of Underscore.js templates instead? That will help to keep all this more readable and maintainable.	chipx86	March 1, 2015, 8:37 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 3, 2015, 3:46 a.m.
The opening ' characters should all line up, and indentation for the templates should happen within the string rather than …	david	March 4, 2015, 8:48 p.m.
What if the text is neither expected type nor SafeText?	Chester	March 18, 2015, 12:44 a.m.
This should end in a period, and use the imperative mood ("Return" rather than "Returns").	david	March 16, 2015, 3:56 a.m.
Have seen the "insert in alphabetical order" comment before, perhaps applicable in this case.	SU Sunxperous	March 15, 2015, 7:39 a.m.
Same here for alphabetical order...	SU Sunxperous	March 15, 2015, 7:39 a.m.
A bit nitpicking, but is it better for row to be $row instead?	SU Sunxperous	March 15, 2015, 7:39 a.m.
I feel the name of the function doesn't clearly describe what this is for. Without the context of this being …	chipx86	March 16, 2015, 3:56 a.m.
"HTML".	chipx86	March 16, 2015, 3:56 a.m.
This isn't the right thing to pass to the comments in the serializedCommentBlocks. You should just be passing a dictionary, …	chipx86	March 16, 2015, 3:57 a.m.
This should stay a standalone function. If you're having a problem with this, then you can instead pull out this.tooltipTemplate …	chipx86	March 16, 2015, 3:57 a.m.
This can be one statement: list.append(tooltipTemplate({ ... }));	chipx86	March 16, 2015, 3:57 a.m.
Leftover debug code?	david	March 18, 2015, 12:30 a.m.
Can you rename list to be $list?	david	March 18, 2015, 12:30 a.m.
Because this is just one statement, it doesn't make a lot of sense to define a new function for it, …	david	March 18, 2015, 12:31 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

reviewboard/reviews/context.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/reviews/context.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```

Commit:

-	bdfa6c1c3471a59f4a3fbac68ab4567b178fcb02
+	25ffc3a19c9cda887b5b2dea6a0b9ac30bca31fd

Diff:

Revision 2 (+40 -10)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/diffCommentModel.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

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

Show all issues

Col: 25
 E131 continuation line unaligned for hanging indent

Commit:

-	25ffc3a19c9cda887b5b2dea6a0b9ac30bca31fd
+	fec6342e08464db639ea472057818b3132f93fc2

Diff:

Revision 3 (+44 -10)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/diffCommentModel.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/resources/models/diffCommentModel.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Change Summary:

Deserialize html_text_fields in baseCommentModel

Commit:

-	fec6342e08464db639ea472057818b3132f93fc2
+	50fde97295625a2c8dcc57566e902e91bf2fe169

Diff:

Revision 4 (+44 -10)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

plain text comments.png (Revision 1)

Show all issues

Can we set a maximum width for this popup? The entire width of the page is a little excessive.

Change Summary:

Reduce max-width of tooltip

Commit:

-	50fde97295625a2c8dcc57566e902e91bf2fe169
+	5b2342106e8af4f924600d94a22e76f0f8c090cf

Diff:

Revision 5 (+44 -10)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Looking good.
I want to make sure that the text is displayed just like it would be for reviews. I'd make use of the .rich-text CSS class for the tooltips, so that all styling is consistent.
Also, please add testing (manual and unit tests) on XSS vulnerabilities. Neither rich text nor plain text should be able to inject any tags.

reviewboard/reviews/context.py (Diff revision 5)

Show all issues

This will allow non-Markdown text to inject tags into the page. Make sure you escape the text.

reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js (Diff revision 5)
Show all issues
```
Blank line between these.
Though, you probably want RB.UserSession.instance instead.
```

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 5)

Show all issues

Can you make use of Underscore.js templates instead? That will help to keep all this more readable and maintainable.

Commit:

-	5b2342106e8af4f924600d94a22e76f0f8c090cf
+	5eefbbb15113452238a2f702f98145140ab48d76

Diff:

Revision 6 (+75 -13)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Change Summary:

Add testing for XSS vulnerabilities

Testing Done:

		Ran tests, added markdown and plain text comments to check visually.
	+
	+	Added unit testing for XSS vulnerabilites. Manually checked by creating comments containing script tags for both markdown and plain text, scripts were escaped and not executed for both first load of the page and subsequent asynchronous updating of comments.

Commit:

-	5eefbbb15113452238a2f702f98145140ab48d76
+	93a1b8f6feb4429d5d67f39ed4559df10cf6b2bc

Diff:

Revision 7 (+92 -13)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

reviewboard/reviews/tests.py (Diff revision 7)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

-	93a1b8f6feb4429d5d67f39ed4559df10cf6b2bc
+	f5f508b4e3a7c86c29b37ae46644ce357b5fd621

Diff:

Revision 8 (+93 -13)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 8)

Show all issues

The opening ' characters should all line up, and indentation for the templates should happen within the string rather than outside of it.

Commit:

-	f5f508b4e3a7c86c29b37ae46644ce357b5fd621
+	3742993d5d7a41e6f34946ecc0db4bc574907ba0

Diff:

Revision 9 (+93 -13)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Would you mind uploading a screenshot that compares what is shown in the tooltip to what is shown on the reviews page?

wudi March 8, 2015, 9:40 a.m.

I've added the reviews page screenshot, you can compare it with the first screenshot for markdown comments.

Added Files:

markdown comments.png (Revision 1)

Show all issues

Can you go through and make sure that the styles of this match exactly what you see on the "View Reviews" page? Notably, all text should be the same size, and all displayed in a fixed-width font.

wudi March 13, 2015, 11:22 a.m.

I've updated the screenshots for the new styling. (for some reason the old markdown comments.png disappeared when I uploaded and hit discard changes earlier)

One thing to note is that I've changed the html tag to pre, all line breaks are preserved so the tooltip can get very long.

Change Summary:

Make the tooltips styling consistent with the styling in the "View Reviews" page

Commit:

-	3742993d5d7a41e6f34946ecc0db4bc574907ba0
+	f169b4764e6f5256f45fd3dd8aea94400e2fc4dd

Diff:

Revision 10 (+90 -16)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	1 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

```
I only see one trivial thing left.
```

reviewboard/reviews/markdown_utils.py (Diff revision 10)

Show all issues

This should end in a period, and use the imperative mood ("Return" rather than "Returns").

wudi March 13, 2015, 10:24 p.m.

Using imperative mood will make it inconsistent with the rest of the functions.

chipx86 March 16, 2015, 12:03 a.m.

We're working on moving toward this for all functions, so that's fine.

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

Show all issues

What if the text is neither expected type nor SafeText?

wudi March 13, 2015, 10:33 p.m.

I'm not sure I get your question. The text is meant to be displayed as html thus it will contain html tags, making it not SafeText, even plain text will have <p> tags after being rendered as markdown.

What do you mean by expected type?

chipx86 March 18, 2015, 12:42 a.m.

I agree, it seems like the check is fine as you have it.

Commit:

-	f169b4764e6f5256f45fd3dd8aea94400e2fc4dd
+	5df25dd7948eb10c80e6d3bf0576b7603132bbde

Diff:

Revision 11 (+90 -16)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/resources/models/baseCommentModel.js
	1 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

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

Show all issues

Have seen the "insert in alphabetical order" comment before, perhaps applicable in this case.

reviewboard/reviews/tests.py (Diff revision 11)
Show all issues
```
Same here for alphabetical order...
```

reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js (Diff revision 11)

Just curious, should this not be placed in beforeEach, instead of being repeated three times for each it block?

WU

wudi March 15, 2015, 7:41 a.m.
```
You're right, I've changed it.
```

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 11)
Show all issues
```
A bit nitpicking, but is it better for row to be $row instead?
```

Commit:

-	5df25dd7948eb10c80e6d3bf0576b7603132bbde
+	6de235f14eea181b3306c741f010e0e9823c89f8

Diff:

Revision 12 (+80 -18)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/pages/search.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

chipx86 March 16, 2015, 12:18 a.m.

Also, I forgot to mention. It looks like this will only do the right thing for diffs. This also needs to work for file attachments (text-based files and image files). You'll want to look for the comment serialization in reviews/ui/base.py.

reviewboard/reviews/markdown_utils.py (Diff revision 12)

Show all issues

I feel the name of the function doesn't clearly describe what this is for. Without the context of this being in Markdown-related code, it's confusing what this is for.
How about markdown_render_conditional?

reviewboard/reviews/markdown_utils.py (Diff revision 12)
Show all issues
```
"HTML".
```

reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js (Diff revision 12)

Show all issues

This isn't the right thing to pass to the comments in the serializedCommentBlocks. You should just be passing a dictionary, just like what's passed to the page. a UserSession is a very different thing.

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 12)

Show all issues

This should stay a standalone function. If you're having a problem with this, then you can instead pull out this.tooltipTemplate into a variable in _updateTooltip.

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 12)

Show all issues

This can be one statement:

list.append(tooltipTemplate({
    ...
}));

Commit:

-	6de235f14eea181b3306c741f010e0e9823c89f8
+	ae33f2832ed2680528884c1555b561d6762b36f8

Diff:

Revision 13 (+75 -16)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/pages/search.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	3 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Commit:

-	ae33f2832ed2680528884c1555b561d6762b36f8
+	fab01f3abb16350c4a0ab4a9b9dabe7f0aedc0ba

Diff:

Revision 14 (+75 -16)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/pages/search.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	3 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

```
Just a few simple comments left.
```
reviewboard/reviews/ui/base.py (Diff revision 14)
Show all issues
```
Leftover debug code?
```
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 14)
Show all issues
```
Can you rename list to be $list?
```

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 14)

Show all issues

Because this is just one statement, it doesn't make a lot of sense to define a new function for it, even with the small amount of duplication below.
We should also not be calling gettext() on the username.

Commit:

-	fab01f3abb16350c4a0ab4a9b9dabe7f0aedc0ba
+	f1b0a6b67e4468a7f6497e00d05f62d078387390

Diff:

Revision 15 (+78 -19)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/markdown_utils.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/pages/search.less
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	3 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/markdown_utils.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/ui/base.py
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/models/commentEditorModel.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (5d758f0)