Summary

Implementation of Review Request Infobox to the Dashboard

Review Request #8661 — Created Jan. 22, 2017 and submitted June 5, 2017, 4:20 p.m.

Information

Owner

sharleenfisher

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

7b14813...

Reviewers

Groups

reviewboard, students

People

Description

This change introduces a new infobox to the dashboard upon hover, over a review request. The idea is similar to the infobox for users and bugs - the infobox will display some summary information regarding the review request, including the submitter, the summary and description, issue counts, and a few of the most recent reviews/activity on the request. This will allow users to get a quick overview of the review request without having to actually move to the page - in particular, what should be useful is the ability to see what recent activity has been done to a review request.

Testing Done

Code has been tested by running the development server and adjusting the tester review request for different scenarios.

Jasmine tests have been run - but at this point in time, the Jasmine tests find that the class label for the infobox is being generated on the Diff links, where it should not be.

Python tests have not been run.

Files

Issues

Description	From	Last Updated
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 29, 2017, 6:11 a.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	Jan. 29, 2017, 6:13 a.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	Jan. 29, 2017, 6:13 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 29, 2017, 6:13 a.m.
Col: 44 W291 trailing whitespace	reviewbot	Jan. 29, 2017, 6:13 a.m.
This is indented too far.	brennie	Feb. 8, 2017, 1:37 p.m.
Comments should have a period.	brennie	Jan. 29, 2017, 6:21 a.m.
undefined name 'format_html_join'	reviewbot	Jan. 29, 2017, 6:17 a.m.
Col: 13 E126 continuation line over-indented for hanging indent	reviewbot	Jan. 29, 2017, 6:18 a.m.
undefined name 'format_html'	reviewbot	Jan. 29, 2017, 6:20 a.m.
We do 4 space indentation.	brennie	Jan. 29, 2017, 6:22 a.m.
undefined name 'format_html'	reviewbot	Jan. 29, 2017, 6:20 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 29, 2017, 6:20 a.m.
We use one space indentation for html files.	brennie	Jan. 29, 2017, 6:34 a.m.
{% if %} etc should be formatted wiht the { in column 1 and a number of spaces between % …	brennie	Feb. 8, 2017, 2:06 p.m.
If you use a {# django template comment #} it wont get put in the resulting HTML.	brennie	Jan. 29, 2017, 6:30 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Feb. 5, 2017, 9:48 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Feb. 5, 2017, 9:47 a.m.
The other infobox views here have an old style of docstring, but because this is new code, we should use …	david	April 7, 2017, 11:34 a.m.
The last item in python dictionaries should also end in a comma (so that it's easier to add new stuff …	david	Feb. 8, 2017, 2:10 p.m.
Can we name all the classes review-request-infobox-* instead of reviewRequest-infobox-*?	david	Feb. 8, 2017, 2:27 p.m.
'format_html' imported but unused	reviewbot	Feb. 10, 2017, 11:25 a.m.
'format_html_join' imported but unused	reviewbot	Feb. 10, 2017, 11:25 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Feb. 10, 2017, 11:25 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 5, 2017, 9:19 a.m.
'format_html' imported but unused	reviewbot	March 5, 2017, 9:19 a.m.
'format_html_join' imported but unused	reviewbot	March 5, 2017, 9:19 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	March 5, 2017, 9:19 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2017, 12:22 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 13, 2017, 12:21 a.m.
'format_html_join' imported but unused	reviewbot	March 13, 2017, 12:21 a.m.
'format_html' imported but unused	reviewbot	March 13, 2017, 12:21 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	March 13, 2017, 12:21 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 19, 2017, 11:08 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 19, 2017, 11:02 a.m.
'format_html' imported but unused	reviewbot	March 19, 2017, 11:03 a.m.
'format_html_join' imported but unused	reviewbot	March 19, 2017, 11:04 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	March 19, 2017, 11:03 a.m.
Python strings concatenate in the same way that C strings do, so you can just elide the + operator here. …	david	March 26, 2017, 10:29 a.m.
Yep, you do.	david	March 26, 2017, 10:29 a.m.
I think this line can be merged with display_data on line 680	AN anni_cao	April 7, 2017, 11:34 a.m.
request should be type of HTTP Get request	AN anni_cao	April 7, 2017, 11:34 a.m.
str is not a type, should be String	AN anni_cao	April 7, 2017, 11:34 a.m.
I guess local_site should be name/address of local site, maybe type of String	AN anni_cao	April 2, 2017, 12:15 p.m.
To be consistent with all the other comments, the description for HttpResponse should be on a new line	AN anni_cao	April 2, 2017, 12:19 p.m.
Suggest to move <div class=...> to a new line so that it will be easier to read. The name of …	AN anni_cao	April 2, 2017, 12:21 p.m.
The " is now on the inside of the }, which isn't right.	david	April 7, 2017, 11:33 a.m.
Type should be django.http.HttpRequest.	david	April 7, 2017, 11:32 a.m.
Type should be reviewboard.site.models.LocalSite. The description can be "The review request's local site, if available."	david	April 7, 2017, 11:32 a.m.
django.http.HttpResponse	david	April 7, 2017, 11:31 a.m.
We could refactor this bit into a new method, e.g., reviewboard.accounts.query.attach_visibility_to_queryset(queryset, user.id) or something similar	brennie	June 5, 2017, 4:17 p.m.
Instead of doing a bunch of additions, can we make a list and then do display_data = mark_safe(''.join(display_data_parts)) ? String …	brennie	April 7, 2017, 11:13 p.m.
Blank line between these. Same below.	brennie	April 7, 2017, 2:55 p.m.
If you do the other refactor I suggested, you should be able to refactor this into a utility method.	brennie	June 5, 2017, 4:17 p.m.
Alphabetize.	brennie	April 7, 2017, 3 p.m.
Missing period & trailing space.	brennie	April 7, 2017, 3:02 p.m.
Block tags like this (for, if, etc.) should be left-aligned. The indentation is done the tag itself. The same applied …	brennie	April 7, 2017, 3:06 p.m.
This is missing the colon?	brennie	April 7, 2017, 3:20 p.m.
This is missing the colon?	brennie	April 7, 2017, 3:20 p.m.
This is missing the colon?	brennie	April 7, 2017, 3:20 p.m.
The colon should be in the trans templatetag.	brennie	April 7, 2017, 3:07 p.m.
The class name "review_request" is way too generic, and is bound to conflict with other things. I know we do …	chipx86	April 7, 2017, 10:41 p.m.
I know this was here before your code, but string concatenation is slow in Python. Instead, can you build this …	chipx86	April 7, 2017, 10:59 p.m.
Since this isn't taking potentially HTML-unsafe values, you don't need format_html.	chipx86	April 7, 2017, 10:59 p.m.
Better not to have the extra spaces there. Just one.	chipx86	May 31, 2017, 1:41 p.m.
No parens here.	chipx86	April 7, 2017, 10:42 p.m.
The function isn't embedded. The returned HTML is.	chipx86	April 7, 2017, 10:45 p.m.
Missing periods.	chipx86	April 7, 2017, 10:45 p.m.
This should be one statement.	chipx86	April 7, 2017, 10:46 p.m.
Too long for a line. This should wrap.	chipx86	April 7, 2017, 11 p.m.
This line is too long.	chipx86	April 7, 2017, 11:03 p.m.
This line is too long. Same with all others in this file.	chipx86	April 7, 2017, 11:06 p.m.
It should be fine to just use the value directly without checking for it. If it's blank, it's blank.	chipx86	April 7, 2017, 10:47 p.m.
There's some alignment problems with the HTML and with the Django tags. HTML and Django templating have their own indentation …	chipx86	April 7, 2017, 10:50 p.m.

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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html



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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 44
 W291 trailing whitespace
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'format_html_join'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 E126 continuation line over-indented for hanging indent
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'format_html'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'format_html'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

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

The issue has been dropped. Show all issues

This is indented too far.

sharleenfisher Jan. 29, 2017, 10:14 a.m.

I'm not sure how I should address this change. I'm thinking that maybe the indentation looks strange because the conditional is split into two lines, and I've indented the second line so that it lines up with the bracket. How should I treat this scenario?

        elif (not review_request.public and

              review_request.status == ReviewRequest.PENDING_REVIEW):

            labels.append(('label-draft', _('Draft')))

david Feb. 6, 2017, 11:52 a.m.
```
What you listed here is correct.
```

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Comments should have a period.
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We do 4 space indentation.
```
reviewboard/templates/reviews/review_request_infobox.html (Diff revision 1)
The issue has been resolved. Show all issues
```
We use one space indentation for html files.
```

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 1)

The issue has been resolved. Show all issues

{% if %} etc should be formatted wiht the { in column 1 and a number of spaces between % and if for nesting.

e.g.
{% if foo %}
{%  if bar %}
{%  endif %}
{% endif %}

sharleenfisher Jan. 29, 2017, 10:10 a.m.

Sorry, I'm not entirely sure what should be fixed on this column, since the 'if' is not nested with another if statement. Does the nesting apply to more than just nested 'if's?

david Feb. 6, 2017, 11:52 a.m.

We have two "separate" nestings for HTML text and for django tags interleaved in the same file. If you mentally filter out one or the other, you should see a valid nesting.
For HTML, indentation happens with whitespace at the beginning of the line. For example:

<html>
 <head>
  <title></title>
 </head>
</html>


For django tags, indentation happens within the tag itself:
{% if foo %}
{%  if bar %}
{%  endif %}
{% endif %}


When they're interleaved, it might look like:

  <div>
{% if X %}
   <div>
{%  if Y %}
    Hello
{%  else %}
    Goodbye
{%  endif %}
   </div>
{% endif %}
  </div>

sharleenfisher Feb. 8, 2017, 1:41 p.m.

Which would be correct in this case then?

{% if {{review_request.submitter.full_name}} %}

   {{review_request.submitter.full_name}}

{% endif %}
Or would it be:

{% if {{review_request.submitter.full_name}} %}

{  {review_request.submitter.full_name}}

{% endif %}

david Feb. 8, 2017, 2:04 p.m.

The first one. When I said "django tags" I meant specifically "block tags" (like if/endif)

david Feb. 8, 2017, 2:05 p.m.

Although, actually, you've got some extra stuff in there. {{x}} is used for dropping the contents of a variable into the page, and isn't necessary inside tags:

{% if review_request.submitter.full_name %}
   {{review_request.submitter.full_name}}
{% endif %}

sharleenfisher Feb. 8, 2017, 2:09 p.m.
```
I'll fix that too! Thanks :)
```

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 1)
The issue has been resolved. Show all issues
```
If you use a {# django template comment #} it wont get put in the resulting HTML.
```

Commit:

c44917c173c054f6e7d3351ff931a05f47687c96

b886466a24b06ba6b3210a813d9f84b0f73004f3

Diff:

Revision 2 (+103 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_request_infobox.html

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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html



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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html

reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Commit:

b886466a24b06ba6b3210a813d9f84b0f73004f3

7da81eeb534a86adf993fe4911966ed786cb1722

Diff:

Revision 3 (+113 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_request_infobox.html

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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html



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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html

reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Commit:

7da81eeb534a86adf993fe4911966ed786cb1722

beaa5c373220fd2b1f06aee82c9fdd365c694175

Diff:

Revision 4 (+114 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_request_infobox.html

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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html



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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html

It looks like you're making good progress so far. Don't forget to close issues on old reviews as you address them. A couple small comments to keep the code clean as you go:

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

The issue has been resolved. Show all issues

The other infobox views here have an old style of docstring, but because this is new code, we should use our more modern style (which we're moving to). This means a few things:

The voice of the summary line should be the imperative mood rather than passive ("Display ..." rather than "Displays ...")
We should include "Args" and "Returns" sections.

See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5 for info on Args/Returns.

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

The issue has been resolved. Show all issues

The last item in python dictionaries should also end in a comma (so that it's easier to add new stuff at the end)

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we name all the classes review-request-infobox-* instead of reviewRequest-infobox-*?
```

Commit:

beaa5c373220fd2b1f06aee82c9fdd365c694175

bc50fdfdcd6df38ec4a24fb98b4132b7948ceb17

Diff:

Revision 5 (+146 -1)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/common.es6.js
	reviewboard/templates/reviews/review_request_infobox.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js



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

Ignored Files:
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js

reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'format_html' imported but unused
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'format_html_join' imported but unused
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Commit:

bc50fdfdcd6df38ec4a24fb98b4132b7948ceb17

57d7e46a554be149d0350b1c3f407ef0084265c0

Diff:

Revision 6 (+149 -4)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/common.es6.js
	reviewboard/static/rb/js/utils/linkifyUtils.es6.js
	reviewboard/templates/reviews/review_request_infobox.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js

reviewboard/reviews/builtin_fields.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
 'format_html' imported but unused
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
 'format_html_join' imported but unused
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Commit:

57d7e46a554be149d0350b1c3f407ef0084265c0

eda9b4d675cc7fd4cb3b28c33580ba68a9a0c5e0

Diff:

Revision 7 (+170 -8)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.es6.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/utils/linkifyUtils.es6.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js

reviewboard/datagrids/columns.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/builtin_fields.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 'format_html_join' imported but unused
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 'format_html' imported but unused
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Commit:

eda9b4d675cc7fd4cb3b28c33580ba68a9a0c5e0

21add9ba7896b876e2e123a7446ef50244b2522a

Diff:

Revision 8 (+232 -8)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.es6.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/utils/linkifyUtils.es6.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js

reviewboard/datagrids/columns.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/builtin_fields.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 'format_html' imported but unused
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 'format_html_join' imported but unused
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Commit:

21add9ba7896b876e2e123a7446ef50244b2522a

06365e6970ee2c45e6b63af7ff36ecb2e592443e

Diff:

Revision 9 (+224 -7)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.es6.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/utils/linkifyUtils.es6.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/utils/linkifyUtils.es6.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/review_request_infobox.html
    reviewboard/static/rb/js/common.es6.js

Now that you've started actually working on the CSS, can you start including a screenshot of what things look like whenever you do your code updates?

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

The issue has been resolved. Show all issues

Python strings concatenate in the same way that C strings do, so you can just elide the + operator here. That said, the way things concatenate now, there won't be any space between title="..." and class="review_request". This probably parses OK on the browser side but it's not totally correct. Let's add some spaces at the beginning of the second line, which also helps make it more readable:

rendered_item = format_html(
    '<a href="{url}" title="{summary"}'
    '   class="review_request">{id}</a>',
    ...

sharleenfisher March 26, 2017, 10:29 a.m.

I noticed there are a number of spaces before "class". Is this to line up "class" with "href" on the line above? Or should there only be one space before "class"?

reviewboard/static/rb/js/pages/views/datagridPageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Yep, you do.
```

Commit:

06365e6970ee2c45e6b63af7ff36ecb2e592443e

ce464827f7332b73719829ad76c5d780d08cc272

Diff:

Revision 10 (+273 -44)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/common.es6.js
	3 more

Added Files:

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Summary:: Implementation of Review Request Infobox to the Dashboard [WIP]
Implementation of Review Request Infobox to the Dashboard

reviewboard/datagrids/columns.py (Diff revision 10)

The issue has been dropped. Show all issues

I think this line can be merged with display_data on line 680

sharleenfisher April 2, 2017, 12:36 p.m.

I think the code was like this before I touched it. Do you have any suggestions on how to do this? I tried to directly merge like so, but it didn't turn out so well:
display_data = format_html('<a class="review_request" href="{0}"><label class="{1}">{2}</label>', url, labels)
I'm thinking it was like this originally because of the labels array?

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

The issue has been resolved. Show all issues

request should be type of HTTP Get request

sharleenfisher April 2, 2017, 12:15 p.m.

I fixed it to be request (HTTP GET), is there a better way to write the TYPE than this?

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

The issue has been resolved. Show all issues

str is not a type, should be String

sharleenfisher April 2, 2017, 12:18 p.m.

When referring to the following link, under Value Types, it says "str" is one of the standard Python types. Is "String" more appropriate? https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6

david April 7, 2017, 10:43 a.m.

str is almost correct. We should actually say "unicode", since it's a unicode object instead of an str (which is a bytestring in python 2).

reviewboard/reviews/views.py (Diff revision 10)
The issue has been resolved. Show all issues
```
I guess local_site should be name/address of local site, maybe type of String
```

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

The issue has been resolved. Show all issues

To be consistent with all the other comments, the description for HttpResponse should be on a new line

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 10)

The issue has been resolved. Show all issues

Suggest to move <div class=...> to a new line so that it will be easier to read.

The name of class review-request-latest-container is a bit confusing, sounds like it should refer to the very outer layer for all the latest reviews

Would be nice if there was a place for extensions to extend the infobox. Perhaps a template hook?

Commit:

ce464827f7332b73719829ad76c5d780d08cc272

353cd3782e0649e78597ea9500d2bfe09b83c7f7

Diff:

Revision 11 (+284 -51)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/common.es6.js
	4 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

reviewboard/reviews/builtin_fields.py (Diff revision 11)
The issue has been resolved. Show all issues
```
The " is now on the inside of the }, which isn't right.
```
reviewboard/reviews/views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Type should be django.http.HttpRequest.
```

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

The issue has been resolved. Show all issues

Type should be reviewboard.site.models.LocalSite. The description can be "The review request's local site, if available."

reviewboard/reviews/views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
django.http.HttpResponse
```

Oh: it would be very nice to have some details of the testing you've been doing (obviously you've done some since you were able to make a screenshot).

Commit:

353cd3782e0649e78597ea9500d2bfe09b83c7f7

5bfd2f1e17db07b6dd06772a1ec6dfe7f2e4bbac

Diff:

Revision 12 (+340 -52)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/common.es6.js
	4 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Testing Done:: ~
Code isn't ready to be run or tested at this point in time.
~
Code has been tested by running the development server and adjusting the tester review request for different scenarios.
+ Jasmine tests have been run - but at this point in time, the Jasmine tests find that the class label for the infobox is being generated on the Diff links, where it should not be.
+ Python tests have not been run.

reviewboard/datagrids/columns.py (Diff revision 12)

The issue has been dropped. Show all issues

We could refactor this bit into a new method, e.g., reviewboard.accounts.query.attach_visibility_to_queryset(queryset, user.id) or something similar

reviewboard/datagrids/columns.py (Diff revision 12)

The issue has been resolved. Show all issues

Instead of doing a bunch of additions, can we make a list and then do display_data = mark_safe(''.join(display_data_parts)) ? String addition is quite slow bceause it has to do N allocations instead of just 1 for str.join.

reviewboard/reviews/views.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Blank line between these. Same below.
```

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

The issue has been dropped. Show all issues

If you do the other refactor I suggested, you should be able to refactor this into a utility method.

reviewboard/static/rb/css/mixins/style.less (Diff revision 12)

The issue has been resolved. Show all issues

Alphabetize.

reviewboard/static/rb/js/utils/tests/linkifyUtilsTests.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Missing period & trailing space.
```

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 12)

The issue has been resolved. Show all issues

Block tags like this (for, if, etc.) should be left-aligned. The indentation is done the tag itself.

The same applied throughout the rest of the file.

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 12)

The issue has been dropped. Show all issues

This is missing the colon?

sharleenfisher April 7, 2017, 3:20 p.m.

I don't think any of these are missing a colon, "Open issues", "Resolved issues", etc, all appear only when hovering over the number and icon, and it's modelled after the issues summary on the actual review request page, which doesn't have a colon. I think the idea here is to make explicit which each icon means upon hover, but it's not actually a text feature in the infobox, nor does the number follow "Open issues", so if I were to add a colon it would just say "Open issues:".

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 12)
The issue has been dropped. Show all issues
```
This is missing the colon?
```
reviewboard/templates/reviews/review_request_infobox.html (Diff revision 12)
The issue has been dropped. Show all issues
```
This is missing the colon?
```
reviewboard/templates/reviews/review_request_infobox.html (Diff revision 12)
The issue has been resolved. Show all issues
```
The colon should be in the trans templatetag.
```

Commit:

5bfd2f1e17db07b6dd06772a1ec6dfe7f2e4bbac

a04f9e4a12e93a8e0d15e2bf63cf474415dd6d0c

Diff:

Revision 13 (+341 -52)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/common.es6.js
	4 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

reviewboard/datagrids/columns.py (Diff revision 13)

The issue has been resolved. Show all issues

The class name "review_request" is way too generic, and is bound to conflict with other things. I know we do this with "user," but frankly that was a mistake too.
Can we call this "review-request-link" instead?
(Also note that we should use - and not _ in class names).
Same with other parts of the codebase.

reviewboard/datagrids/columns.py (Diff revision 13)

The issue has been resolved. Show all issues

I know this was here before your code, but string concatenation is slow in Python. Instead, can you build this as a list like:

result = [
    format_html(...),
    format_html(...)
]

if summary:
    result.append(format_html(...))

...

return ''.join(result)


This is actually faster.

reviewboard/datagrids/columns.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Since this isn't taking potentially HTML-unsafe values, you don't need format_html.
```

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

The issue has been dropped. Show all issues

Better not to have the extra spaces there. Just one.

david April 7, 2017, 4:08 p.m.

I recommended formatting it this way for readability. The extra spaces don't hurt and they make it easier on the eyes.

sharleenfisher April 7, 2017, 10:42 p.m.
```
Is it okay to leave it this way then?
```

reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
No parens here.
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
The function isn't embedded. The returned HTML is.
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Missing periods.
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
This should be one statement.
```
reviewboard/reviews/views.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Too long for a line. This should wrap.
```
reviewboard/static/rb/js/utils/linkifyUtils.es6.js (Diff revision 13)
The issue has been resolved. Show all issues
```
This line is too long.
```
reviewboard/static/rb/js/utils/tests/linkifyUtilsTests.js (Diff revision 13)
The issue has been resolved. Show all issues
```
This line is too long.
Same with all others in this file.
```
reviewboard/templates/reviews/review_request_infobox.html (Diff revision 13)
The issue has been resolved. Show all issues
```
It should be fine to just use the value directly without checking for it. If it's blank, it's blank.
```

reviewboard/templates/reviews/review_request_infobox.html (Diff revision 13)

The issue has been resolved. Show all issues

There's some alignment problems with the HTML and with the Django tags.

HTML and Django templating have their own indentation context. HTML should always be indented 1 space relative to parent HTML tags. The content within {% ... %} should be indented 1 relative to their parents.

Commit:

a04f9e4a12e93a8e0d15e2bf63cf474415dd6d0c

7b14813bb96b77c017fb0df620d6a6e8c1a5dc1a

Diff:

Revision 14 (+352 -58)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/common.es6.js
	4 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Status:: Completed
Change Summary:: Pushed to release-3.0.x (05e32b6)

Ship it!

Landing this with some code cleanup and visual tweaks. Thanks!

~		Code isn't ready to be run or tested at this point in time.
	~	Code has been tested by running the development server and adjusting the tester review request for different scenarios.
	+	Jasmine tests have been run - but at this point in time, the Jasmine tests find that the class label for the infobox is being generated on the Diff links, where it should not be.
	+	Python tests have not been run.