Description: |
|
|---|
Highlighting Active Discussions: Attract user attention to new comments and reviews
Review Request #7008 — Created March 4, 2015 and discarded
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 repliesNext Steps: Create a few more complicated testcases and see if any bugs arise.
| Description | From | Last Updated |
|---|---|---|
|
This doesn't look used. |
 david
|
|
|
I think it would be better to put last_visited in here, rather than duplicating it into each entry. |
david
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 23 E231 missing whitespace after ':' |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 21 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 23 E231 missing whitespace after ':' |
reviewbot
|
|
|
Col: 65 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 21 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Undo this change. |
 brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
'render' imported but unused |
reviewbot
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Undo this change. |
brennie
|
|
|
Undo this change. |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
ndo this change. |
brennie
|
|
|
Undo these changes. |
brennie
|
|
|
The { should be in the first column. The if should line up with the endif above. e.g., {% endif … |
brennie
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
'render' imported but unused |
reviewbot
|
|
|
Space before the { |
david
|
|
|
Space before the { |
david
|
|
|
Blank line between these two. |
david
|
|
|
Add another space before "if" (so it's aligned at the same level as the "endif" above it). |
david
|
|
|
Can we break this up into multiple lines? Also, in templates, high-level tags (like if/block/etc) should be left-aligned. {% if … |
david
|
|
|
Same here. |
david
|
RM
-
-
-
reviewboard/reviews/views.py (Diff revision 1) I think it would be better to put
last_visitedin here, rather than duplicating it into eachentry.
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2) Col: 1 W293 blank line contains whitespace
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2) Col: 1 W293 blank line contains whitespace
-
-
-
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).
RM
Change Summary:
Made Front End Changes according to the following rule:
For new reviews we haven't seen before, there is a blue circle in front of the review
For reviews we have seen before, but has new comments, we put a blue circle on the review's username, and highlight and put a blue circle on the new comments.
Old reviews are still collapsed
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+58 -9) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3) Col: 1 W293 blank line contains whitespace
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3) Col: 1 W293 blank line contains whitespace
-
-
-
-
-
-
-
-
-
-
-
RM
Change Summary:
Moved status of Highlighting Active Discussions from WIP to "completed"
Description: |
|
|---|
RM
Change Summary:
Fixed Reviewbot style issues
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+50 -12) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 4) Col: 65 E231 missing whitespace after ','
-
-
-
-
-
RM
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+47 -12) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5) Col: 80 E501 line too long (80 > 79 characters)
-
RM
Change Summary:
Removed Unneccary test3,4,5 files
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+44 -12) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6) Col: 80 E501 line too long (80 > 79 characters)
-
-
-
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.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
reviewboard/templates/reviews/boxes/review.html (Diff revision 6) The
{should be in the first column. Theifshould line up with theendifabove. e.g.,{% endif %} <div>...</div> {% if last_visited ... %} ... {% endif %}
RM
Change Summary:
Undid whitepsace changes, and other style issues, as well as made a few changes to description to be more in line with style guidelines
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+35 -4) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7) Col: 80 E501 line too long (80 > 79 characters)
-
-
Please upload some screenshots of what this looks like.
-
-
-
-
reviewboard/templates/reviews/boxes/review.html (Diff revision 7) Add another space before "if" (so it's aligned at the same level as the "endif" above it).
-
reviewboard/templates/reviews/review_reply.html (Diff revision 7) 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 %} -
RM
Change Summary:
Made minor style changes and uploaded screenshots of ui changes
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+42 -5) |
||||
Added Files: |
-
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