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)
     
     
     

    This doesn't look used.

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

    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. Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Col: 1
     W293 blank line contains whitespace
    
  4. Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/reviews/views.py (Diff revision 2)
     
     
    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. Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Col: 1
     W293 blank line contains whitespace
    
  4. Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 21
     E225 missing whitespace around operator
    
  10. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  12. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  13. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  14. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  15. reviewboard/reviews/views.py (Diff revision 3)
     
     
    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. Col: 65
     E231 missing whitespace after ','
    
  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 21
     E225 missing whitespace around operator
    
  6. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/reviews/views.py (Diff revision 4)
     
     
    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. Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
    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. Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
     'render' imported but unused
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
    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. Undo this change.

  3. Undo this change.

  4. Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

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

    Undo this change

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

    Undo this change.

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

    Undo this change.

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

    Undo this change.

  15. No blank line here.

  16. No blank line here.

  17. ndo this change.

  18. Undo these changes.

  19. 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. Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 7)
     
     
     'render' imported but unused
    
  4. 
      
david
  1. Please upload some screenshots of what this looks like.

  2. Space before the {

  3. Space before the {

  4. Blank line between these two.

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

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

Loading...