Implementation of Review Request Infobox to the Dashboard

Review Request #8661 — Created Jan. 22, 2017 and submitted

Information

Review Board
master
7b14813...

Reviewers

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.

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.


Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 44 W291 trailing whitespace

reviewbotreviewbot

This is indented too far.

brenniebrennie

Comments should have a period.

brenniebrennie

undefined name 'format_html_join'

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'format_html'

reviewbotreviewbot

We do 4 space indentation.

brenniebrennie

undefined name 'format_html'

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

We use one space indentation for html files.

brenniebrennie

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

brenniebrennie

If you use a {# django template comment #} it wont get put in the resulting HTML.

brenniebrennie

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

The other infobox views here have an old style of docstring, but because this is new code, we should use …

daviddavid

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

daviddavid

Can we name all the classes review-request-infobox-* instead of reviewRequest-infobox-*?

daviddavid

'format_html' imported but unused

reviewbotreviewbot

'format_html_join' imported but unused

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

'format_html' imported but unused

reviewbotreviewbot

'format_html_join' imported but unused

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

'format_html_join' imported but unused

reviewbotreviewbot

'format_html' imported but unused

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

'format_html' imported but unused

reviewbotreviewbot

'format_html_join' imported but unused

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Python strings concatenate in the same way that C strings do, so you can just elide the + operator here. …

daviddavid

Yep, you do.

daviddavid

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

AN anni_cao

request should be type of HTTP Get request

AN anni_cao

str is not a type, should be String

AN anni_cao

I guess local_site should be name/address of local site, maybe type of String

AN anni_cao

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

AN anni_cao

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

AN anni_cao

The " is now on the inside of the }, which isn't right.

daviddavid

Type should be django.http.HttpRequest.

daviddavid

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

daviddavid

django.http.HttpResponse

daviddavid

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

brenniebrennie

Instead of doing a bunch of additions, can we make a list and then do display_data = mark_safe(''.join(display_data_parts)) ? String …

brenniebrennie

Blank line between these. Same below.

brenniebrennie

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

brenniebrennie

Alphabetize.

brenniebrennie

Missing period & trailing space.

brenniebrennie

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

brenniebrennie

This is missing the colon?

brenniebrennie

This is missing the colon?

brenniebrennie

This is missing the colon?

brenniebrennie

The colon should be in the trans templatetag.

brenniebrennie

The class name "review_request" is way too generic, and is bound to conflict with other things. I know we do …

chipx86chipx86

I know this was here before your code, but string concatenation is slow in Python. Instead, can you build this …

chipx86chipx86

Since this isn't taking potentially HTML-unsafe values, you don't need format_html.

chipx86chipx86

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

chipx86chipx86

No parens here.

chipx86chipx86

The function isn't embedded. The returned HTML is.

chipx86chipx86

Missing periods.

chipx86chipx86

This should be one statement.

chipx86chipx86

Too long for a line. This should wrap.

chipx86chipx86

This line is too long.

chipx86chipx86

This line is too long. Same with all others in this file.

chipx86chipx86

It should be fine to just use the value directly without checking for it. If it's blank, it's blank.

chipx86chipx86

There's some alignment problems with the HTML and with the Django tags. HTML and Django templating have their own indentation …

chipx86chipx86
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 44
     W291 trailing whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     undefined name 'format_html_join'
    
  8. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  9. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     undefined name 'format_html'
    
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     undefined name 'format_html'
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues

    This is indented too far.

    1. 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')))

    2. What you listed here is correct.

  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues

    Comments should have a period.

  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues

    We do 4 space indentation.

  5. Show all issues

    We use one space indentation for html files.

  6. 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 %}
    
    1. 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?

    2. 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>
      
    3. 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 %}

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

    5. 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 %}
      
    6. I'll fix that too! Thanks :)

  7. Show all issues

    If you use a {# django template comment #} it wont get put in the resulting HTML.

  8. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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:

  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     
     
    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.

  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    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)

  4. Show all issues

    Can we name all the classes review-request-infobox-* instead of reviewRequest-infobox-*?

  5. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     'format_html' imported but unused
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     'format_html_join' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  5. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/builtin_fields.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     'format_html' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     'format_html_join' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  6. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/datagrids/columns.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     'format_html_join' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     'format_html' imported but unused
    
  6. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  7. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. reviewboard/datagrids/columns.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
     'format_html' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
     'format_html_join' imported but unused
    
  6. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  7. 
      
sharleenfisher
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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?

  2. reviewboard/reviews/builtin_fields.py (Diff revision 9)
     
     
     
    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>',
        ...
    
    1. 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"?

  3. Show all issues

    Yep, you do.

  4. 
      
sharleenfisher
sharleenfisher
AN
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 10)
     
     
    Show all issues

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

    1. 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?

  3. 
      
AN
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    request should be type of HTTP Get request

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

  3. 
      
AN
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    str is not a type, should be String

    1. 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

    2. 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).

  3. 
      
AN
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    I guess local_site should be name/address of local site, maybe type of String

  3. 
      
AN
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

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

  3. 
      
AN
  1. 
      
  2. 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

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

  3. 
      
sharleenfisher
david
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 11)
     
     
    Show all issues

    The " is now on the inside of the }, which isn't right.

  3. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

    Type should be django.http.HttpRequest.

  4. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

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

  5. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

    django.http.HttpResponse

  6. 
      
david
  1. 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).

  2. 
      
sharleenfisher
sharleenfisher
brennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
    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

  3. reviewboard/datagrids/columns.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

  4. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
    Show all issues

    Blank line between these. Same below.

  5. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  6. reviewboard/static/rb/css/mixins/style.less (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Alphabetize.

  7. Show all issues

    Missing period & trailing space.

  8. 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.

  9. Show all issues

    This is missing the colon?

    1. 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:".

  10. Show all issues

    This is missing the colon?

  11. Show all issues

    This is missing the colon?

  12. Show all issues

    The colon should be in the trans templatetag.

  13. 
      
sharleenfisher
chipx86
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

  3. reviewboard/datagrids/columns.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

  4. reviewboard/datagrids/columns.py (Diff revision 13)
     
     
    Show all issues

    Since this isn't taking potentially HTML-unsafe values, you don't need format_html.

  5. reviewboard/reviews/builtin_fields.py (Diff revision 13)
     
     
     
    Show all issues

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

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

    2. Is it okay to leave it this way then?

  6. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues

    No parens here.

  7. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues

    The function isn't embedded. The returned HTML is.

  8. reviewboard/reviews/views.py (Diff revision 13)
     
     
     
    Show all issues

    Missing periods.

  9. reviewboard/reviews/views.py (Diff revision 13)
     
     
     
     
    Show all issues

    This should be one statement.

  10. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues

    Too long for a line. This should wrap.

  11. Show all issues

    This line is too long.

  12. Show all issues

    This line is too long.

    Same with all others in this file.

  13. Show all issues

    It should be fine to just use the value directly without checking for it. If it's blank, it's blank.

  14. reviewboard/templates/reviews/review_request_infobox.html (Diff revision 13)
     
     
     
     
     
     
     
     
     
    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.

  15. 
      
sharleenfisher
sharleenfisher
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (05e32b6)
david
  1. Landing this with some code cleanup and visual tweaks. Thanks!

  2. 
      
Loading...