diff --git a/reviewboard/reviews/templatetags/reviewtags.py b/reviewboard/reviews/templatetags/reviewtags.py index 60899c03b04a0c83fd2e1660242a1a3922012103..c600830320765a3a8d4e2a438fa49a183f993f92 100644 --- a/reviewboard/reviews/templatetags/reviewtags.py +++ b/reviewboard/reviews/templatetags/reviewtags.py @@ -249,6 +249,7 @@ def reply_section(context, review, comment, context_type, context_id, 'local_site_name': context.get('local_site_name'), 'reply_to_is_empty': reply_to_text == '', 'request': context['request'], + 'last_visited': context['last_visited'], } diff --git a/reviewboard/reviews/tests/test_views.py b/reviewboard/reviews/tests/test_views.py index cf5d10770396aff7e794ac6691926ceb93efd05a..52da0fd893ba7e0d7b5a4b4a5b256fb5ff3bb656 100644 --- a/reviewboard/reviews/tests/test_views.py +++ b/reviewboard/reviews/tests/test_views.py @@ -1,11 +1,14 @@ from __future__ import unicode_literals from datetime import timedelta +from re import compile from django.contrib.auth.models import User from django.core.urlresolvers import reverse +from django.utils import timezone from djblets.siteconfig.models import SiteConfiguration +from reviewboard.accounts.models import ReviewRequestVisit from reviewboard.extensions.tests import TestService from reviewboard.hostingsvcs.service import (register_hosting_service, unregister_hosting_service) @@ -1164,6 +1167,180 @@ class ViewTests(TestCase): })) self.assertEqual(response.status_code, 404) + def test_css_class_added_to_new_reviews(self): + """Testing review_detail view sets new-page-entry CSS class on + reviews that have been posted since the review request was last + visited. + """ + review_request = self.create_review_request(publish=True) + + self.client.login(username='doc', password='doc') + user = User.objects.get(username='doc') + + # Create a visit on the review request in the past. + self.create_visit(review_request, + ReviewRequestVisit.VISIBLE, + user, + timestamp=(timezone.now() - timedelta(1))) + + review = self.create_review(review_request, user, publish=True) + + response = self.client.get(local_site_reverse( + 'review-request-detail', + args=[review_request.display_id])) + + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'reviews/boxes/review.html') + self.assertLessEqual(response.context['last_visited'], + review.timestamp) + + # Check that the new-page-entry class is in the DOM. + # Do this by making sure there is an element with the id of the review + # and the new-page-entry class. The order of these attributes does not + # matter. + text = compile( + '(<.*id="review%d".*class=".*new-page-entry.*".*>)|' + '(<.*class=".*new-page-entry.*".*id="review%d".*>)' + % (review.pk, review.pk) + ) + self.assertTrue(text.search(response.content)) + + def test_css_class_removed_from_new_reviews(self): + """Testing review_detail view removes new-page-entry CSS class from + reviews on review requests that have been visited since the review + was posted. + """ + review_request = self.create_review_request(publish=True) + + self.client.login(username='doc', password='doc') + user = User.objects.get(username='doc') + + # Create a visit on the review request in the past. + self.create_visit(review_request, + ReviewRequestVisit.VISIBLE, + user, + timestamp=(timezone.now() - timedelta(1))) + + review = self.create_review(review_request, user, publish=True) + + self.client.get(local_site_reverse('review-request-detail', + args=[review_request.display_id])) + + # We are only interested in the response when the review request has + # been visited since the review was posted. + response = self.client.get(local_site_reverse( + 'review-request-detail', + args=[review_request.display_id])) + + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'reviews/boxes/review.html') + self.assertGreaterEqual(response.context['last_visited'], + review.timestamp) + + # Check that the new-page-entry class is not in the DOM. + # Do this by making sure that an element with the id of the review + # and the new-page-entry class does not exist. The order of these + # attributes does not matter. + text = compile( + '(<.*id="review%d".*class=".*new-page-entry.*".*>)|' + '(<.*class=".*new-page-entry.*".*id="review%d".*>)' + % (review.pk, review.pk) + ) + self.assertFalse(text.search(response.content)) + + def test_css_class_added_to_new_review_replies(self): + """Testing review_detail view sets new-comment CSS class on review + replies that have been posted since the review request was last + visited. + """ + review_request = self.create_review_request(publish=True) + + self.client.login(username='doc', password='doc') + user = User.objects.get(username='doc') + + # Create a visit on the review request in the past. + self.create_visit(review_request, + ReviewRequestVisit.VISIBLE, + user, + timestamp=(timezone.now() - timedelta(1))) + + review = self.create_review(review_request, user=user) + comment = self.create_general_comment(review) + review.publish() + + review_reply = self.create_reply(review, user) + self.create_general_comment(review_reply, reply_to=comment) + review_reply.publish() + + response = self.client.get(local_site_reverse( + 'review-request-detail', + args=[review_request.display_id])) + + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'reviews/review_reply.html') + self.assertLessEqual(response.context['last_visited'], + review_reply.timestamp) + + # Check that the new-comment class is in the DOM. + # Do this by making sure there is an element with the id of the review + # reply and the new-page-entry class. The order of these attributes + # does not matter. + text = compile( + '(<.*data-comment-id="%d".*class=".*new-comment.*".*>)|' + '(<.*class=".*new-comment.*".*data-comment-id="%d".*>)' + % (review_reply.pk, review_reply.pk) + ) + self.assertTrue(text.search(response.content)) + + def test_css_class_removed_from_new_review_replies(self): + """Testing review_detail view removes new-comment CSS class from review + replies on review requests that have been visited since the review + reply was posted. + """ + review_request = self.create_review_request(publish=True) + + self.client.login(username='doc', password='doc') + user = User.objects.get(username='doc') + + # Create a visit on the review request in the past. + self.create_visit(review_request, + ReviewRequestVisit.VISIBLE, + user, + timestamp=(timezone.now() - timedelta(1))) + + review = self.create_review(review_request, user=user) + comment = self.create_general_comment(review) + review.publish() + + review_reply = self.create_reply(review, user) + self.create_general_comment(review_reply, reply_to=comment) + review_reply.publish() + + self.client.get(local_site_reverse('review-request-detail', + args=[review_request.display_id])) + + # We are only interested in the response when the review request has + # been visited since the review reply was posted. + response = self.client.get(local_site_reverse( + 'review-request-detail', + args=[review_request.display_id])) + + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'reviews/review_reply.html') + self.assertGreaterEqual(response.context['last_visited'], + review_reply.timestamp) + + # Check that the new-page-entry class is not in the DOM. + # Do this by making sure that an element with the id of the review + # reply and the new-page-entry class does not exist. The order of + # these attributes does not matter. + text = compile( + '(<.*data-comment-id="%d".*class=".*new-comment.*".*>)|' + '(<.*class=".*new-comment.*".*data-comment-id="%d".*>)' + % (review.pk, review.pk) + ) + self.assertFalse(text.search(response.content)) + def _get_context_var(self, response, varname): for context in response.context: if varname in context: diff --git a/reviewboard/reviews/views.py b/reviewboard/reviews/views.py index 4ab6fb18d40bb411ff3ff255d3c758abf51f1c77..a6898013c2883d242855a077cd84da51e04ef3d5 100644 --- a/reviewboard/reviews/views.py +++ b/reviewboard/reviews/views.py @@ -345,6 +345,9 @@ def review_detail(request, visited = None last_visited = 0 starred = False + new_activity_since_last_visited = None + last_activity_time, updated_object = \ + review_request.get_last_activity(data.diffsets, data.reviews) if request.user.is_authenticated(): try: @@ -375,8 +378,12 @@ def review_detail(request, except Profile.DoesNotExist: pass - last_activity_time, updated_object = \ - review_request.get_last_activity(data.diffsets, data.reviews) + # We need to generate a new ETag if the state of whether a new entry + # was added since the user last visited the review request has changed. + # This ensures that the highlighting on new reviews, updates, and + # comments always appears/disappears despite any caching that might + # happen. + new_activity_since_last_visited = (last_visited <= last_activity_time) if data.draft: draft_timestamp = data.draft.last_updated @@ -387,13 +394,18 @@ def review_detail(request, # Find out if we can bail early. Generate an ETag for this. etag = encode_etag( - '%s:%s:%s:%s:%s:%s:%s:%s:%s:%s' % - (request.user, last_activity_time, draft_timestamp, - data.latest_review_timestamp, - review_request.last_review_activity_timestamp, - is_rich_text_default_for_user(request.user), - [r.pk for r in blocks], - starred, visited and visited.visibility, settings.AJAX_SERIAL)) + '%s:%s:%s:%s:%s:%s:%s:%s:%s:%s:%s' % ( + request.user, + last_activity_time, + draft_timestamp, + data.latest_review_timestamp, + review_request.last_review_activity_timestamp, + is_rich_text_default_for_user(request.user), + [r.pk for r in blocks], + starred, + visited and visited.visibility, + settings.AJAX_SERIAL, + new_activity_since_last_visited)) if etag_if_none_match(request, etag): return HttpResponseNotModified() @@ -506,6 +518,7 @@ def review_detail(request, 'initial_status_entry': initial_status_entry, 'entries': entries, 'last_activity_time': last_activity_time, + 'last_visited': last_visited, 'review': review_request.get_pending_review(request.user), 'request': request, 'close_description': close_description, diff --git a/reviewboard/static/rb/css/pages/reviews.less b/reviewboard/static/rb/css/pages/reviews.less index b72cad41741b6896f62c7c1e67a0a6d2ae03aa04..7bca7648432b7f3c7aec65b8cc6daf538286d018 100644 --- a/reviewboard/static/rb/css/pages/reviews.less +++ b/reviewboard/static/rb/css/pages/reviews.less @@ -10,6 +10,9 @@ @base-entry-bg: #ffffff; @collapsed-entry-bg: #ececee; @changedesc-bg: lighten(@review-request-bg, 5%); +@new-entry-accent: #ede1b2; +@new-entry-bg: #fffeee; +@new-entry-border-width: 4px; /**************************************************************************** @@ -645,8 +648,6 @@ #reviews .review { .body { - clear: both; - pre { white-space: pre-wrap; } @@ -707,6 +708,32 @@ /**************************************************************************** +* New Page Entries, New Comments +****************************************************************************/ + +#reviews .review-request-page-entry { + &.new-page-entry { + .review-request-page-entry-contents { + background-color: @new-entry-bg; + + &:after { + border-right-color: @new-entry-accent; + } + + .body, .header { + border-left: @new-entry-border-width solid @new-entry-accent; + } + } + } + + &:not(.new-page-entry) .new-comment { + background-color: @new-entry-bg; + border-left: @new-entry-border-width solid @new-entry-accent; + } +} + + +/**************************************************************************** * Change Descriptions ****************************************************************************/ diff --git a/reviewboard/templates/reviews/boxes/change.html b/reviewboard/templates/reviews/boxes/change.html index 9062041a6a52d3e5a7abc936822b143f0e0f2505..0511192f500d9538104845cc9b6c9f39364a361b 100644 --- a/reviewboard/templates/reviews/boxes/change.html +++ b/reviewboard/templates/reviews/boxes/change.html @@ -1,7 +1,7 @@ {% load changedescs avatars djblets_deco djblets_utils i18n reviewtags tz %} {% with changedesc=entry.changedesc %} -
+
diff --git a/reviewboard/templates/reviews/boxes/review.html b/reviewboard/templates/reviews/boxes/review.html index e33be0b816901e8dabcd900c758933df46282a9c..a3df56b526173923ee766746173614c48a33d4d3 100644 --- a/reviewboard/templates/reviews/boxes/review.html +++ b/reviewboard/templates/reviews/boxes/review.html @@ -3,7 +3,7 @@ {% with review=entry.review %} -
+
{% if forloop.last %} {% endif %} diff --git a/reviewboard/templates/reviews/review_reply.html b/reviewboard/templates/reviews/review_reply.html index fd45dbf7f3512cf44e3951b23f71915ec0b3bc20..be04e3015fd923106c6af07320cec8e4c6afefbd 100644 --- a/reviewboard/templates/reviews/review_reply.html +++ b/reviewboard/templates/reviews/review_reply.html @@ -1,6 +1,6 @@ {% load avatars djblets_utils i18n reviewtags tz %} - +