Highlighting Active Discussions: Attract user attention to new comments and reviews

Review Request #7008 — Created March 4, 2015 and discarded

Information

Review Board
master

Reviewers

Highlight Active Discussions,Frontend and Backend Completed

Applied last_visited token as a a parameter to the front end, so that both reviews and reply's can check in the template tag if
they are new since the last time the user has visited the site. The main backend changes occured in the reviews/views.py file in the review_detail function. Before,
the fate of the reply's collapsing depended on if the reply was new or not, and individual comments/replies could not be highlighted. Now, it is possible to determine which comments or reviews are new(not seen before) through the following rules on the front end side.

Views are modelled by the following rules:

If a review is new or has new comments, then there exists a blue circle on the username tab of the whole review reply.
If the user has seen a review before, but there are new comments, then the user will have a blue circle on the review reply and the new comments as well as a highlighted background.
If the review is old and there are no new comments, then the review is collapsed as before.

Manual Testing so far with a few simple review requests. Attempted to populate database, but I think some bugs in the populate database function prevented proper execution.
Test Case: 1 review, 2 replies
Test Case: 2 reviews, 2 replies
Test Case: 1 review, 1 reply
Test Case: 2 reiews, 2 replies

Next Steps: Create a few more complicated testcases and see if any bugs arise.


Description From Last Updated

This doesn't look used.

daviddavid

I think it would be better to put last_visited in here, rather than duplicating it into each entry.

daviddavid

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 23 E231 missing whitespace after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 21 E225 missing whitespace around operator

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 23 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 65 E231 missing whitespace after ','

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 21 E225 missing whitespace around operator

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

'render' imported but unused

reviewbotreviewbot

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

Undo this change

brenniebrennie

Undo this change.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Undo this change.

brenniebrennie

Undo this change.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

ndo this change.

brenniebrennie

Undo these changes.

brenniebrennie

The { should be in the first column. The if should line up with the endif above. e.g., {% endif …

brenniebrennie

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

reviewbotreviewbot

'render' imported but unused

reviewbotreviewbot

Space before the {

daviddavid

Space before the {

daviddavid

Blank line between these two.

daviddavid

Add another space before "if" (so it's aligned at the same level as the "endif" above it).

daviddavid

Can we break this up into multiple lines? Also, in templates, high-level tags (like if/block/etc) should be left-aligned. {% if …

daviddavid

Same here.

daviddavid
RM
david
  1. What in the world happened with deleting the file reviewboard/webapi/resources/review_reply.py ?

    1. I'm not sure where it went but I readded it in this commit.

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

    This doesn't look used.

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

    I think it would be better to put last_visited in here, rather than duplicating it into each entry.

  4. 
      
RM
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/webapi/resources/review_reply.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/webapi/resources/review_reply.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 23
     E231 missing whitespace after ':'
    
  7. 
      
david
  1. So the way you posted this makes it so the diff isn't particularly useful (since we only see the latest commit on the branch). We'd really like to see the diff between master and the tip of your branch (which is what you get when just running rbt post).

  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 21
     E225 missing whitespace around operator
    
  10. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  12. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  13. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  14. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  15. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 23
     E231 missing whitespace after ':'
    
  16. 
      
RM
RM
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
  2. Show all issues
    Col: 65
     E231 missing whitespace after ','
    
  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 21
     E225 missing whitespace around operator
    
  6. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/test3
        reviewboard/test4
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/test
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. 
      
SU
  1. Are the three test files test, test3 and test4 in the diff supposed to be there?

    1. Thanks, got rid of them

  2. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     'render' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. 
      
brennie
  1. A few notes. You made a lot of whitespace changes that break our style conventions. Firstly, there should be a blank line between a statement and a block and a blank line after a block. e.g.,

    x = 3
    
    while x == 3:
       x = foo(x)
    
    if x:
       bar()
    

    Also your description needs some work. Descriptions shouldn't mention the files modified (we can see from the diff what files were modified). Also you should avoid persons in descriptions (e.g., I, you, me). Finally, your summary wraps weirdly. If you are going to wrap it at all, wrap it at 72 characters because that is what Git commit messages expect. For more information on writing good summaries and descriptions, have a look through this guide.

    If this is a WIP like it says in the description, please put a [WIP] tag in your summary.

  2. Show all issues

    Undo this change.

  3. Show all issues

    Undo this change.

  4. Show all issues

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

  15. Show all issues

    No blank line here.

  16. Show all issues

    No blank line here.

  17. Show all issues

    ndo this change.

  18. Show all issues

    Undo these changes.

  19. Show all issues

    The { should be in the first column. The if should line up with the endif above. e.g.,

    {%  endif %}
       <div>...</div>
    {%  if last_visited ... %}
       ...
    {%  endif %}
    
  20. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
  2. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     'render' imported but unused
    
  4. 
      
david
  1. Please upload some screenshots of what this looks like.

  2. Show all issues

    Space before the {

  3. Show all issues

    Space before the {

  4. Show all issues

    Blank line between these two.

  5. Show all issues

    Add another space before "if" (so it's aligned at the same level as the "endif" above it).

  6. Show all issues

    Can we break this up into multiple lines?

    Also, in templates, high-level tags (like if/block/etc) should be left-aligned.

    {% if last_visited < timestamp and last_visited > entry.review.timestamp %}
        <dt class="new-reply">
    {% else %}
        <dt>
    {% endif %}
    
  7. Show all issues

    Same here.

  8. 
      
brennie
  1. Rohan, could you please update this review request?

  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
    
    
  2. 
      
RM
david
Review request changed
Status:
Discarded