Add Issue Status and Counts to Dashboard

Review Request #3949 — Created March 8, 2013 and discarded

Information

Review Board

Reviewers

This patch adds support for showing the following in the dashboard as well as "All Review Request" Pages
a) The issue count in the format of <open issues> / <total issues>
b) Status of a review request:
   + If there are no open issues and a shipit, it shows up with a green background and the shipit icon
   + If there are open issues and a shipit, it shows up with a red background and an exclamation sign.
    
In addition, the review detail page shows the "ship it" emblem if there are no issues and a "ship it with issues" emblem if there are issues and a ship-it.
    
Summary of changes:
* reviewboard/reviews/datagrids.py
  + Added StatusColumn as a non-sortable(as there will be issues and ship it counts) column.
  + Added IssueCount column as a sortable column that displays the issue count in the format of <open issues> / <total issues>
    
* reviewboard/reviews/evolutions/issue_status_count.py
  + Added migration scripts to add issue_{open,closed,total}_count to the reviews_reviewrequest table.
    
* reviewboard/reviews/fixtures/test_reviewrequests.json
  + Added init values for issue counts in the review request fixtures.
    
* reviewboard/reviews/managers.py
  + Added methods to calculate the issue open, closed and total counts for existing review requests. These will be calculated for review requests when they are accessed the first time.
  + Added methods to calculate the issue open, closed and total count for a new review. These will be called when a new review is published
    
* reviewboard/reviews/models.py
  + Added initializer fields for issue_{open,closed,total}_count.
  + Since issue states can be modified by just a PUT request (and no need for a publish after that), check the old and new states when a comment is saved. Only if an issue is opened and the states change, increment/decrement the issue counts.
  + Calculate the issue counts when a review is published.
    
* reviewboard/reviews/tests.py
  + Added couple of tests where all types of comments (screenshot, attachment, diff) are added, issues created against them and closed. The issue counts are verified at every stage.
    
* reviewboard/templates/reviews/review_detail.html
  + Modified the review_detail template to show "ship it" or "ship it with issues" depending on whether there are open issues with a Ship It.
    
* reviewboard/webapi/resources.py
  + Added issue_open_count and issue_total_count when GET-ting a ReviewRequestResource
  + Provide filtering capabilities so open-issues can be queried when getting a list of review-requests
    
* reviewboard/webapi/tests.py
  + Modified couple of tests related to creating issues to include issue counts and verified whether the counts are appearing properly.

Note: The djblets changes associated with this are in review request 3948
Manual testing as well as running the test suite with additional tests. Also tested webapi resources manually and through the webapi tests.

Description From Last Updated

Others get this wrong, but this should follow the proper docstring model: """One-line summary. Multi-line description. """

chipx86chipx86

Other columns get this wrong, but this should be: super(StatusColumn, self).__init__...) Same with IssueCountColumn.

chipx86chipx86

This can be flattened by having this check == 0 and return ''. Fewer nesting if statements is good.

chipx86chipx86

The strings are aligned weirdly.

chipx86chipx86

Col: 25 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 25 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 30 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Same comment about the docstring.

chipx86chipx86

Same comment about alignment.

chipx86chipx86

Col: 21 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 16 E221 multiple spaces before operator

reviewbotreviewbot

Col: 11 E221 multiple spaces before operator

reviewbotreviewbot

Each of these functions should have a docstring. The "review_request" shouldn't be necessary. I think these would be best added …

chipx86chipx86

Blank line after imports.

chipx86chipx86

I think I'd prefer to just use return statements instead of setting a variable and returning in both cases. Shortens …

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

This sounds like it's the count of review requests. Maybe _get_comment_count.

chipx86chipx86

This is really inefficient. First off, these can be simplified with: for review in review_request.reviews.filter(public=True) for comment in review.get_all_comments(issue_status__in=issue_status) That …

chipx86chipx86

to prevent counting issue_status=O+ opened_issue=0 bug #2984 https://code.google.com/p/reviewboard/issues/detail?id=2984 I'd propose to prefix test by "comment.issue_opened and" => if comment.issue_opened and …

DE delyn

Same comments apply to all these.

chipx86chipx86

to prevent counting issue_status=O+ opened_issue=0 bug #2984 https://code.google.com/p/reviewboard/issues/detail?id=2984 I'd propose to prefix test by "comment.issue_opened and" => if comment.issue_opened and …

DE delyn

what can be done to prevent updating reviewrequest last_updated during database evolution and init of new fields? Indeed, all existing …

DE delyn

Indentation would be best as: initializer=lambda r: (r.pk and r.get_open_issue_count() or 0)

chipx86chipx86

Blank line before this.

chipx86chipx86

Weird indentation. Should use parens for multi-line conditionals. Also, blank line before the first if statement here.

chipx86chipx86

This is all getting quite repetitive. How about: comment_fields = ['comments', 'screenshot_comments', 'file_attachment_comments'] for comment_field in comment_fields: comments = getattr(self, …

chipx86chipx86

Each type of operation should be tested separately, so that we can know exactly what is failing when.

chipx86chipx86

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

reviewbotreviewbot

This is too long. Can you do: {% if entry.review.ship_it %} {% trans ... %} {% if issue_open_count %} ... …

chipx86chipx86

Might as well add closed counts too.

chipx86chipx86

Might as well do this for closed and total counts too.

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/tests.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
        reviewboard/reviews/datagrids.py
        reviewboard/reviews/evolutions/issue_status_count.py
      Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/css/common.less
        reviewboard/reviews/fixtures/test_reviewrequests.json
        reviewboard/static/rb/css/reviews.less
    
    
  2. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 25
     E127 continuation line over-indented for visual indent
    
    1. Maintaining the same style as in the other render_data methods
  3. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 25
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 30
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 16
     E221 multiple spaces before operator
    
    1. Maintaining consistency with other definitions in this class
  7. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Col: 11
     E221 multiple spaces before operator
    
  8. reviewboard/reviews/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
    1. Couldnt make the docstring short enough to convey the meaning of what it does
  9. 
      
RA
MW
  1. Nitpicks:
    
    - Would be nice if issues had a subtle gradient background like 'ship it'.
    - Would be nice if the status bubble didn't have so much extra empty space to the right. (For that matter, should the number be right-justified?)
    
    ...but I'd rather have it land without those tweaks than not at all :-).
    
    Thanks for working on this!
    1. I have no design skills, but will try to see if I can incorporate that. 
      
      Christian/David, where do you get icons for reviewboard images. I tried to get an exclamation mark transparent icon, but dint find any.
  2. 
      
DE
  1. as discussed on issue #2723
    http://code.google.com/p/reviewboard/issues/detail?id=2723#c14
  2. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    to prevent counting issue_status=O+ opened_issue=0 bug #2984
    https://code.google.com/p/reviewboard/issues/detail?id=2984
    
    I'd propose to prefix test by "comment.issue_opened and"
    =>
                       if comment.issue_opened and
                          comment.issue_status in issue_status)
    
  3. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    to prevent counting issue_status=O+ opened_issue=0 bug #2984
    https://code.google.com/p/reviewboard/issues/detail?id=2984
    
    I'd propose to prefix test by "comment.issue_opened and"
    =>
                       if comment.issue_opened and
                          comment.issue_status in issue_status)
    
  4. 
      
chipx86
  1. Hey,
    
    Thanks for the change. Sorry this has totally fallen off my radar.
    
    There's some things I'd like to see changed before this goes in.
  2. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Show all issues
    Others get this wrong, but this should follow the proper docstring model:
    
    """One-line summary.
    
    Multi-line description.
    """
  3. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    Other columns get this wrong, but this should be:
    
    super(StatusColumn, self).__init__...)
    
    Same with IssueCountColumn.
  4. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    Show all issues
    This can be flattened by having this check == 0 and return ''. Fewer nesting if statements is good.
  5. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Show all issues
    The strings are aligned weirdly.
  6. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Show all issues
    Same comment about the docstring.
  7. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Show all issues
    Same comment about alignment.
  8. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Each of these functions should have a docstring.
    
    The "review_request" shouldn't be necessary. I think these would be best added to the ReviewRequest object. Then these would just operate on the ReviewRequest they're called on. Also resolves the inline import stuff.
  9. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Blank line after imports.
  10. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    I think I'd prefer to just use return statements instead of setting a variable and returning in both cases. Shortens the code.
  11. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Blank line before this.
  12. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Blank line before this.
  13. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Blank line before this.
  14. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    This sounds like it's the count of review requests. Maybe _get_comment_count.
  15. reviewboard/reviews/managers.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    This is really inefficient.
    
    First off, these can be simplified with:
    
    for review in review_request.reviews.filter(public=True)
    for comment in review.get_all_comments(issue_status__in=issue_status)
    
    That reduces what's called. Still, it causes a large number of queries. That can be further reduced with:
    
    
    review_request.reviews.filter(
        Q(public=True) &
        (Q(comments__issue_status__in=issue_status)
        (Q(screenshot_comments__issue_status__in=issue_status)
        (Q(file_attachment_comments__issue_status__in=issue_status)))
    
    
    Then you can avoid sum(), and just use .count().
  16. reviewboard/reviews/managers.py (Diff revision 1)
     
     
    Show all issues
    Same comments apply to all these.
  17. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
    Show all issues
    Indentation would be best as:
    
    initializer=lambda r:
        (r.pk and r.get_open_issue_count() or 0)
  18. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    Blank line before this.
  19. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues
    Weird indentation.
    
    Should use parens for multi-line conditionals.
    
    Also, blank line before the first if statement here.
  20. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This is all getting quite repetitive. How about:
    
    comment_fields = ['comments', 'screenshot_comments', 'file_attachment_comments']
    
    for comment_field in comment_fields:
        comments = getattr(self, comment_field)
    
        for comment in comments:
            ...
  21. reviewboard/reviews/tests.py (Diff revision 1)
     
     
    Show all issues
    Each type of operation should be tested separately, so that we can know exactly what is failing when.
  22. Show all issues
    This is too long. Can you do:
    
    {% if entry.review.ship_it %}
    <div class="...">
     {% trans ... %}
     {% if issue_open_count %} ... {% endif %}
    </div>
    {% endif %}
  23. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Might as well add closed counts too.
  24. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    Might as well do this for closed and total counts too.
  25. 
      
DE
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    what can be done to prevent updating reviewrequest last_updated during database evolution and init of new fields?
    
    Indeed, all existing reviewrequests have their last_updated field modified during this migration :-(
    https://code.google.com/p/reviewboard/issues/detail?id=2723#c19
    1. Is that only happening with this change? I haven't seen this issue myself.
    2. How I can reproduce
      I've restored one of our mysql backup (>600 req, each with several reviews) on 1.7.6 vanilla
      reload datagrid => last_updated not changed ok
      I apply patches to rvb
      apply django evolution with manage.py --execute
      reload datagrid => last_updated of all req = now(), timestamp of comments and reviews seem not modified, only reviewrequest last_updated
      
      Have I made something wrong or miss some steps?
      
      Currently I have not still identified where the problem may come from
      
      Have you any ideas to investigate on?
  3. 
      
chipx86
  1. Hey,

    Just letting you know that I've put together a set of changes that accomplishes this same goal, with some small differences in design and implementation. Much of the code in Review Board has changed since this change, so there wasn't a ton to reuse, but it was heavily inspired by the change, so thanks!

    You'll see this in RB 2.0 beta 3.

  2. 
      
RA
Review request changed
Status:
Discarded