Implementation of Review Request Infobox to the Dashboard

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

Sharleen Fisher
Review Board
master
7b14813...
reviewboard, students

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.

Loading file attachments...

  • 0
  • 0
  • 54
  • 20
  • 74
Description From Last Updated
Review Bot
  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)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 44
     W291 trailing whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 1)
     
     
     undefined name 'format_html_join'
    
  8. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 13
     E126 continuation line over-indented for hanging indent
    
  9. reviewboard/reviews/views.py (Diff revision 1)
     
     
     undefined name 'format_html'
    
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
     undefined name 'format_html'
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  12. 
      
Barret Rennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     

    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)
     
     

    Comments should have a period.

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

    We do 4 space indentation.

  5. We use one space indentation for html files.

  6. {% 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. If you use a {# django template comment #} it wont get put in the resulting HTML.

  8. 
      
Sharleen Fisher
Review Bot
  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)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
Sharleen Fisher
Review Bot
  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)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
Sharleen Fisher
Review Bot
  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 Trowbridge
  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)
     
     
     
     
     

    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)
     
     

    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. Can we name all the classes review-request-infobox-* instead of reviewRequest-infobox-*?

  5. 
      
Sharleen Fisher
Review Bot
  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)
     
     
     'format_html' imported but unused
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
     'format_html_join' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  5. 
      
Sharleen Fisher
Review Bot
  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)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
     'format_html' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
     'format_html_join' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  6. 
      
Sharleen Fisher
Review Bot
  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)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
     'format_html_join' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
     'format_html' imported but unused
    
  6. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  7. 
      
Sharleen Fisher
Review Bot
  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)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 8)
     
     
     'format_html' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 8)
     
     
     'format_html_join' imported but unused
    
  6. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  7. 
      
Sharleen Fisher
Review Bot
  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 Trowbridge
  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)
     
     
     

    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. 
      
Sharleen Fisher
Sharleen Fisher
Anni Cao
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 10)
     
     

    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. 
      
Anni Cao
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     

    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. 
      
Anni Cao
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     

    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. 
      
Anni Cao
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     

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

  3. 
      
Anni Cao
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     

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

  3. 
      
Anni Cao
  1. 
      
  2. 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. 
      
Erik Johansson
  1. 
      
  2. Would be nice if there was a place for extensions to extend the infobox. Perhaps a template hook?

  3. 
      
Sharleen Fisher
David Trowbridge
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 11)
     
     

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

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

    Type should be django.http.HttpRequest.

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

    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)
     
     

    django.http.HttpResponse

  6. 
      
David Trowbridge
  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. 
      
Sharleen Fisher
Sharleen Fisher
Barret Rennie
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     

    Blank line between these. Same below.

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

    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)
     
     
     
     
     
     
     
     
     
     

    Alphabetize.

  7. Missing period & trailing space.

  8. 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. 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. This is missing the colon?

  11. This is missing the colon?

  12. The colon should be in the trans templatetag.

  13. 
      
Sharleen Fisher
Christian Hammond
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     

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

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

    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)
     
     

    No parens here.

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

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

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

    Missing periods.

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

    This should be one statement.

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

    Too long for a line. This should wrap.

  11. This line is too long.

  12. This line is too long.

    Same with all others in this file.

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

    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. 
      
Sharleen Fisher
Sharleen Fisher
Review request changed

Status: Closed (submitted)

Change Summary:

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

  2. 
      
Loading...