diff --git a/reviewboard/reviews/tests/test_views.py b/reviewboard/reviews/tests/test_views.py index 0b774bea0a742a3d5792bd312563f71960f331e1..894457a7e030927015b8951beb51ad624c458e31 100644 --- a/reviewboard/reviews/tests/test_views.py +++ b/reviewboard/reviews/tests/test_views.py @@ -1,12 +1,18 @@ from __future__ import unicode_literals -from datetime import timedelta +from datetime import datetime, timedelta from django.contrib.auth.models import User from django.core.urlresolvers import reverse +from django.utils import timezone +from djblets.features.testing import override_feature_check from djblets.siteconfig.models import SiteConfiguration -from reviewboard.reviews.models import Comment, GeneralComment, Review +from reviewboard.changedescs.models import ChangeDescription +from reviewboard.reviews.models import (Comment, + GeneralComment, + Review, + StatusUpdate) from reviewboard.site.urlresolvers import local_site_reverse from reviewboard.testing import TestCase @@ -183,6 +189,345 @@ class ViewTests(TestCase): self.assertEqual(comments[1].text, comment_text_3) self.assertEqual(comments[2].text, comment_text_2) + def test_user_visited_collapse_logic(self): + """Testing the collapse logic for someone who has seen the reviews""" + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + filediff = self.create_filediff(diffset) + + # Create the users who will be commenting. + user1 = User.objects.get(username='doc') + + self._build_test_reviews(review_request, user1, filediff) + expected_num_reviews = 6 + + # Login and visit the reviews. + self.client.login(username='doc', password='doc') + self.create_visit(review_request, True, user1, + timestamp=timezone.now()) + + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + entries = response.context['entries'] + self.assertEqual(len(entries), expected_num_reviews) + + # Reviews are returned from oldest to newest. + expected_values = [True, False, True, False, False, False] + self.assertEqual(len(expected_values), expected_num_reviews) + + self.assertEqual( + [entry.collapsed for entry in entries], + expected_values) + + def test_anonymous_user_collapse_logic(self): + """Testing the collapse logic for someone who is not logged in""" + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + filediff = self.create_filediff(diffset) + + # Create the users who will be commenting. + user1 = User.objects.get(username='doc') + + self._build_test_reviews(review_request, user1, filediff) + expected_num_reviews = 6 + + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + entries = response.context['entries'] + self.assertEqual(len(entries), expected_num_reviews) + + # All reviews should be unvisited. + for entry in entries: + self.assertFalse(entry.collapsed) + + def test_user_not_visited_collapse_logic(self): + """Testing the collapse logic for a user who has not + visited the reviews already""" + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + filediff = self.create_filediff(diffset) + + # Create the users who will be commenting. + user1 = User.objects.get(username='doc') + + self._build_test_reviews(review_request, user1, filediff) + expected_num_reviews = 6 + + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + entries = response.context['entries'] + self.assertEqual(len(entries), expected_num_reviews) + + # All reviews should be unvisited. + for entry in entries: + self.assertEqual(entry.collapsed, False) + # Test for a different user. + self.client.login(username='grumpy', password='grumpy') + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + entries = response.context['entries'] + self.assertEqual(len(entries), expected_num_reviews) + + # All reviews should be unvisited. + for entry in entries: + self.assertFalse(entry.collapsed) + + # Get the reviews again now that we visited them. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + entries = response.context['entries'] + + expected_values = [True, False, True, True, False, False] + + # Test that everything else remains the same as above. + self.assertEqual( + [entry.collapsed for entry in entries], + expected_values) + + def test_initial_status_update_collapse(self): + """Testing that the initial status update is not collapsed + when it is created""" + with override_feature_check('reviews.status_updates', True): + user1 = User.objects.get(username='doc') + review_request = self._setup_collapse(user1) + expected_initial_entries = 2 + + # Get the page. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(entries), 0) + self.assertIsNotNone(initial_entry) + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + + # Check that the we are displaying the initial entry. + self.assertEqual(initial_entry.collapsed, False) + + def test_status_update_basic_collapse(self): + """Testing that adding a new ChangeDescriptions with status_updates + collapses the older status updates""" + with override_feature_check('reviews.status_updates', True): + user1 = User.objects.get(username='doc') + + # Set up the initial entries and log in the user. + review_request = self._setup_collapse(user1) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + self._build_modifiable_change_description(review_request, user1) + expected_num_entries = 2 + + # Get the page. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(entries), expected_num_entries) + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + + # The old entries are collapsed and new entry is open. + self.assertTrue(initial_entry.collapsed) + self.assertTrue(entries[0].collapsed) + self.assertFalse(entries[1].collapsed) + + def test_status_update_new_reply_collapse(self): + """Testing that status_updates that receive new replies are + expanded to be visible""" + with override_feature_check('reviews.status_updates', True): + user = User.objects.get(username='doc') + review_request = self._setup_collapse(user) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + review = \ + self._build_modifiable_change_description(review_request, user) + expected_num_entries = 2 + + # review = tester_changedesc.status_updates[0].review + self.create_reply(review, user, publish=True) + + # Get the page. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(entries), expected_num_entries) + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + + self.assertTrue(initial_entry.collapsed) + + for entry in entries: + self.assertFalse(entry.collapsed) + + def test_status_update_draft_collapse(self): + """Testing that ChangeDescriptions with status_updates that have a draft + reply are expanded only for the user with the draft""" + with override_feature_check('reviews.status_updates', True): + user = User.objects.get(username='doc') + review_request = self._setup_collapse(user) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + review = \ + self._build_modifiable_change_description(review_request, user) + expected_num_entries = 2 + + # review = tester_changedesc.status_updates[0].review + self.create_reply(review, user, publish=False) + + # Get the page. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(entries), expected_num_entries) + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + + self.assertTrue(initial_entry.collapsed) + + for entry in entries: + self.assertFalse(entry.collapsed) + + # Check that the draft doesn't affect other users. + self.client.logout() + user2 = User.objects.get(username='grumpy') + self.client.login(username='grumpy', password='grumpy') + self.create_visit(review_request, True, user2) + + # Get the page for user2. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertTrue(initial_entry.collapsed) + # We shouldn't be affected by user1's draft. + self.assertTrue(entries[0].collapsed) + self.assertFalse(entries[1].collapsed) + + def test_status_update_open_issue_collapse(self): + """Testing that status_updates with open issues remain expanded""" + with override_feature_check('reviews.status_updates', True): + user = User.objects.get(username='doc') + review_request = self._setup_collapse(user) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + review = \ + self._build_modifiable_change_description(review_request, user) + expected_num_entries = 2 + + diffset = self.create_diffset(review_request) + filediff = self.create_filediff(diffset) + # review = tester_changedesc.status_updates[0].review + self.create_diff_comment(review, filediff, issue_opened=True) + + # Get the page. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(entries), expected_num_entries) + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + + self.assertTrue(initial_entry.collapsed) + + # The entry with the issue should be expanded. + for entry in entries: + self.assertFalse(entry.collapsed) + + def test_change_descriptions_no_user(self): + """Testing that ChangeDescriptions are expanded for + users who are not logged in""" + with override_feature_check('reviews.status_updates', True): + user = User.objects.get(username='doc') + review_request = self._setup_collapse(user) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + self._build_modifiable_change_description(review_request, user) + expected_num_entries = 2 + + # Logout and test for someone not logged in. + self.client.logout() + + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + self.assertEqual(len(entries), expected_num_entries) + + self.assertFalse(initial_entry.collapsed) + + # All reviews should be unvisited. + for entry in entries: + self.assertFalse(entry.collapsed) + + def test_change_description_different_user(self): + """Testing that ChangeDescriptions are expanded for users + who haven't seen them previously""" + with override_feature_check('reviews.status_updates', True): + user1 = User.objects.get(username='doc') + review_request = self._setup_collapse(user1) + expected_initial_entries = 2 + + # Create new ChangeDescriptions for testing. + self._build_modifiable_change_description(review_request, user1) + expected_num_entries = 2 + + # Logout and test for someone not logged in. + self.client.logout() + + # Test for a different user. + self.client.login(username='grumpy', password='grumpy') + # Get the reviews. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + self.assertEqual(len(initial_entry.status_updates), + expected_initial_entries) + self.assertEqual(len(entries), expected_num_entries) + + # All reviews should be unvisited. + self.assertFalse(initial_entry.collapsed) + + for entry in entries: + self.assertFalse(entry.collapsed) + + # Get the reviews again now that we visited them. + response = self._fetch_review_request(review_request.pk) + self.assertEqual(response.status_code, 200) + initial_entry = response.context['initial_status_entry'] + entries = response.context['entries'] + + # All reviews should be unvisited. + self.assertTrue(initial_entry.collapsed) + self.assertTrue(entries[0].collapsed) + self.assertFalse(entries[1].collapsed) + def test_review_detail_file_attachment_visibility(self): """Testing visibility of file attachments on review requests""" caption_1 = 'File Attachment 1' @@ -215,7 +560,7 @@ class ViewTests(TestCase): # Check that we can find all the objects we expect on the page. self.client.login(username='doc', password='doc') - response = self.client.get('/r/%d/' % review_request.pk) + response = self._fetch_review_request(review_request.pk) self.assertEqual(response.status_code, 200) file_attachments = response.context['file_attachments'] @@ -225,7 +570,7 @@ class ViewTests(TestCase): # Make sure that other users won't see the draft one. self.client.logout() - response = self.client.get('/r/%d/' % review_request.pk) + response = self._fetch_review_request(review_request.pk) self.assertEqual(response.status_code, 200) file_attachments = response.context['file_attachments'] @@ -1159,6 +1504,148 @@ class ViewTests(TestCase): return None + def _build_test_reviews(self, review_request, user, filediff): + """Create a set of reviews to be used for testing""" + # Time_now indicates that the review/reply is new. + time_now = timezone.now() + # Old_time indicates the review/reply is not new. + old_time = timezone.get_default_timezone().localize(datetime.min) + + # Create the reviews. + self.create_review(review_request, user=user, publish=True) + + self.create_review(review_request, user=user, publish=True) + + review = self.create_review(review_request, user=user, publish=True) + self.create_reply(review, user, publish=True) + + review = self.create_review(review_request, user=user, publish=True) + self.create_reply(review, user, publish=True) + + review = self.create_review(review_request, user=user, publish=True) + self.create_reply(review, user) + + review = self.create_review(review_request, user=user, publish=True) + self.create_diff_comment(review, filediff, issue_opened=True) + + # Create a draft review. This shouldn't show up later. + self.create_review(review_request, user=user, publish=False) + + timestamps = [time_now, old_time, old_time, time_now, old_time, + old_time, old_time, old_time, old_time, old_time, + old_time] + + for review, timestamp in zip(review_request.reviews.all(), timestamps): + Review.objects.filter(pk=review.pk).update(timestamp=timestamp) + + def _build_initial_status_update(self, review_request, user): + """Create the initial status updates for review_request""" + # Old_time indicates the review/reply is not new. + old_time = timezone.get_default_timezone().localize(datetime.min) + + # Create the initial status updates. + # These are not attached to a ChangeDescription. + initial_no_issue_review = \ + self.create_review(review_request, user=user, publish=True) + StatusUpdate.objects.create( + review_request=review_request, + service_id="S1", + summary="Summary", + state=StatusUpdate.DONE_SUCCESS, + user=user, + timestamp=old_time, + review=initial_no_issue_review + ) + + no_issue_review = \ + self.create_review(review_request, user=user, publish=True) + StatusUpdate.objects.create( + review_request=review_request, + service_id="S1", + summary="Summary", + state=StatusUpdate.DONE_SUCCESS, + user=user, + timestamp=old_time, + review=no_issue_review + ) + + def _build_modifiable_change_description(self, review_request, + user, time_delta=5): + """Builds two ChangeDescriptions and attaches them to the + review_request. Return the review of the ChangeDescription, + so that it can be modified if necessary.""" + # Old_time indicates the review/reply is not new. + old_time = timezone.get_default_timezone().localize(datetime.min) + + review1 = self.create_review(review_request, user=user, publish=True) + review2 = self.create_review(review_request, user=user, publish=True) + + for review in review_request.reviews.all(): + Review.objects.filter(pk=review.pk).update(timestamp=old_time) + + collapsed_change = \ + ChangeDescription(user=user, + timestamp=(old_time + + timedelta(seconds=time_delta)), + public=True) + collapsed_change.save() + review_request.changedescs.add(collapsed_change) + + change = \ + ChangeDescription(user=user, + timestamp=(old_time + + timedelta(seconds=time_delta + 5)), + public=True) + change.save() + review_request.changedescs.add(change) + + StatusUpdate.objects.create( + review_request=review_request, + service_id="S1", + summary="Summary", + state=StatusUpdate.DONE_SUCCESS, + user=user, + timestamp=old_time, + change_description=collapsed_change, + review=review1 + ) + + StatusUpdate.objects.create( + review_request=review_request, + service_id="S1", + summary="Summary", + state=StatusUpdate.DONE_SUCCESS, + user=user, + timestamp=old_time, + change_description=change, + review=review2 + ) + + return review1 + + def _setup_collapse(self, user): + """Create a review_request and have it be visited by the user""" + review_request = self.create_review_request(create_repository=True, + publish=True) + + self._build_initial_status_update(review_request, user) + + # Login and visit the page. + self.client.login(username='doc', password='doc') + self.create_visit(review_request, True, user, + timestamp=timezone.now()) + + return review_request + + def _fetch_review_request(self, review_pk): + """Fetch the specified review request from the local site""" + return self.client.get( + local_site_reverse( + 'review-request-detail', + kwargs={ + 'review_request_id': review_pk, + } + )) class UserInfoboxTests(TestCase): def test_unicode(self): diff --git a/reviewboard/reviews/views.py b/reviewboard/reviews/views.py index 93282ffbcadbc2dc8e913ea701c48004dd7858da..dd97e6b2f1ffdeffa732e26b33d441734eb0a598 100644 --- a/reviewboard/reviews/views.py +++ b/reviewboard/reviews/views.py @@ -2,6 +2,7 @@ from __future__ import unicode_literals import logging import time +from datetime import datetime, timedelta from django.conf import settings from django.contrib.auth.decorators import login_required @@ -338,8 +339,11 @@ def review_detail(request, data = ReviewRequestPageData(review_request, request) data.query_data_pre_etag() + # Reviews posted today will never be collapsed. + oldest_allowed_time = timezone.now() - timedelta(days=1) + min_time = timezone.get_default_timezone().localize(datetime.min) visited = None - last_visited = 0 + last_visited = min_time starred = False if request.user.is_authenticated(): @@ -347,7 +351,10 @@ def review_detail(request, visited, visited_is_new = \ ReviewRequestVisit.objects.get_or_create( user=request.user, review_request=review_request) - last_visited = visited.timestamp.replace(tzinfo=utc) + visited.timestamp.replace(tzinfo=utc) + + if not visited_is_new: + last_visited = visited.timestamp except ReviewRequestVisit.DoesNotExist: # Somehow, this visit was seen as created but then not # accessible. We need to log this and then continue on. @@ -359,7 +366,7 @@ def review_detail(request, # If the review request is public and pending review and if the user # is logged in, mark that they've visited this review request. if (review_request.public and - review_request.status == review_request.PENDING_REVIEW): + review_request.status == review_request.PENDING_REVIEW): visited.timestamp = timezone.now() visited.save() @@ -407,16 +414,8 @@ def review_detail(request, not review.is_reply() and not (status_updates_enabled and hasattr(review, 'status_update'))): - # Mark as collapsed if the review is older than the latest - # change, assuming there's no reply newer than last_visited. - latest_reply = data.latest_timestamps_by_review_id.get(review.pk) - collapsed = ( - review.timestamp < data.latest_changedesc_timestamp and - not (latest_reply and - last_visited and - last_visited < latest_reply)) - - entry = ReviewEntry(request, review_request, review, collapsed, + + entry = ReviewEntry(request, review_request, review, True, data) reviews_entry_map[review.pk] = entry entries.append(entry) @@ -460,11 +459,6 @@ def review_detail(request, assert review.pk not in reviews_entry_map assert base_reply_to_id in reviews_entry_map - - # Make sure that any review boxes containing draft replies are - # always expanded. - if comment.is_reply() and not review.public: - reviews_entry_map[base_reply_to_id].collapsed = False elif review.public: # This is a comment on a public review. assert review.id in reviews_entry_map @@ -472,6 +466,43 @@ def review_detail(request, entry = reviews_entry_map[review.id] entry.add_comment(comment._type, comment) + def determine_review_collapse(review): + """Determine whether or not to collapse a review""" + # Don't collapse reviews with open issues. + if review.has_comments(True) > 0: + return False + + # Don't collapse reviews with draft replies. + if review.get_pending_reply(request.user): + return False + + latest_reply_timestamp = \ + data.latest_timestamps_by_review_id.get(review.pk) + + if latest_reply_timestamp is None: + latest_reply_timestamp = min_time + + # Don't collapse if the review or latest reply is new. + review_timestamp = review.timestamp + + if (review_timestamp > oldest_allowed_time or + latest_reply_timestamp > oldest_allowed_time): + return False + + # Don't collapse if the review or latest reply hasn't been seen yet. + if (review_timestamp >= last_visited or + latest_reply_timestamp >= last_visited): + return False + + return True + + # Go through every review to decide if its entry should be collapsed. + for review_pk, entry in six.iteritems(reviews_entry_map): + review = data.reviews_by_id[review_pk] + + if entry.collapsed: + entry.collapsed = determine_review_collapse(review) + if status_updates_enabled: initial_status_entry.finalize()