diff --git a/reviewboard/reviews/admin.py b/reviewboard/reviews/admin.py index 78d7c031097828609be8f259f947f019b070779e..626b939815ea199b7c274d956bf759a1c8502c7a 100644 --- a/reviewboard/reviews/admin.py +++ b/reviewboard/reviews/admin.py @@ -133,7 +133,9 @@ class ReviewRequestAdmin(admin.ModelAdmin): 'modified unless something is wrong.

'), 'fields': ('email_message_id', 'time_emailed', 'last_review_activity_timestamp', - 'shipit_count', 'local_id'), + 'shipit_count', 'issue_open_count', + 'issue_resolved_count', 'issue_dropped_count', + 'local_id'), 'classes': ['collapse'], }), ) diff --git a/reviewboard/reviews/evolutions/__init__.py b/reviewboard/reviews/evolutions/__init__.py index 776ca4956eb790d2fa870fec18a8e1641a599039..1b9959f51dd77ed540e8f7126cf3c9fa2e542024 100644 --- a/reviewboard/reviews/evolutions/__init__.py +++ b/reviewboard/reviews/evolutions/__init__.py @@ -23,4 +23,5 @@ SEQUENCE = [ 'base_comment_extra_data', 'unique_together_baseline', 'extra_data', + 'review_request_issue_counts', ] diff --git a/reviewboard/reviews/evolutions/review_request_issue_counts.py b/reviewboard/reviews/evolutions/review_request_issue_counts.py new file mode 100644 index 0000000000000000000000000000000000000000..c0eddc67715322baf4a322e38991840e3f1d1fdf --- /dev/null +++ b/reviewboard/reviews/evolutions/review_request_issue_counts.py @@ -0,0 +1,11 @@ +from __future__ import unicode_literals + +from django_evolution.mutations import AddField +from djblets.db.fields import CounterField + + +MUTATIONS = [ + AddField('ReviewRequest', 'issue_dropped_count', CounterField, null=True), + AddField('ReviewRequest', 'issue_resolved_count', CounterField, null=True), + AddField('ReviewRequest', 'issue_open_count', CounterField, null=True), +] diff --git a/reviewboard/reviews/models/base_comment.py b/reviewboard/reviews/models/base_comment.py index 3231eb3ac74f1a912964b487b9bef262b027a558..5a495c70041ac69272b39e54e50d0c903ff70d63 100644 --- a/reviewboard/reviews/models/base_comment.py +++ b/reviewboard/reviews/models/base_comment.py @@ -6,7 +6,7 @@ from django.db.models import Q from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ -from djblets.db.fields import JSONField +from djblets.db.fields import CounterField, JSONField from djblets.db.managers import ConcurrencyManager @@ -63,6 +63,11 @@ class BaseComment(models.Model): else: raise Exception("Invalid issue status '%s'" % status) + def __init__(self, *args, **kwargs): + super(BaseComment, self).__init__(*args, **kwargs) + + self._loaded_issue_status = self.issue_status + def get_review_request(self): if hasattr(self, '_review_request'): return self._review_request @@ -133,7 +138,20 @@ class BaseComment(models.Model): # the review. review = self.get_review() - if not review.public: + if review.public: + if self._loaded_issue_status != self.issue_status: + old_field = ReviewRequest.ISSUE_COUNTER_FIELDS[ + self._loaded_issue_status] + new_field = ReviewRequest.ISSUE_COUNTER_FIELDS[ + self.issue_status] + + CounterField.increment_many( + self.get_review_request(), + { + old_field: -1, + new_field: 1, + }) + else: review.timestamp = self.timestamp review.save() diff --git a/reviewboard/reviews/models/review.py b/reviewboard/reviews/models/review.py index 6b872c220486c04e1dc47b53d2d81beb9197d02c..18716be2584c3586938cfb38436db87c4ef81035 100644 --- a/reviewboard/reviews/models/review.py +++ b/reviewboard/reviews/models/review.py @@ -6,15 +6,17 @@ from django.db.models import Q from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ -from djblets.db.fields import JSONField +from djblets.db.fields import CounterField, JSONField from djblets.db.query import get_object_or_none from reviewboard.diffviewer.models import DiffSet from reviewboard.reviews.managers import ReviewManager +from reviewboard.reviews.models.base_comment import BaseComment from reviewboard.reviews.models.diff_comment import Comment from reviewboard.reviews.models.file_attachment_comment import \ FileAttachmentComment -from reviewboard.reviews.models.review_request import ReviewRequest +from reviewboard.reviews.models.review_request import (ReviewRequest, + fetch_issue_counts) from reviewboard.reviews.models.screenshot_comment import ScreenshotComment from reviewboard.reviews.signals import reply_published, review_published @@ -195,9 +197,27 @@ class Review(models.Model): self.review_request.save( update_fields=['last_review_activity_timestamp']) - # Atomicly update the shipit_count + issue_counts = fetch_issue_counts(self.review_request, Q(pk=self.pk)) + + # Since we're publishing the review, all filed issues should be + # open. + assert issue_counts[BaseComment.RESOLVED] == 0 + assert issue_counts[BaseComment.DROPPED] == 0 + if self.ship_it: - self.review_request.increment_shipit_count() + ship_it_value = 1 + else: + ship_it_value = 0 + + # Atomically update the issue count and Ship It count. + CounterField.increment_many( + self.review_request, + { + 'issue_open_count': issue_counts[BaseComment.OPEN], + 'issue_dropped_count': 0, + 'issue_resolved_count': 0, + 'shipit_count': ship_it_value, + }) if self.is_reply(): reply_published.send(sender=self.__class__, diff --git a/reviewboard/reviews/models/review_request.py b/reviewboard/reviews/models/review_request.py index 1e4645451b624ec7b8e47b7597e89deeb5ace5f8..675bff487f92a3eae4f6c5e6c7d096bde80593cd 100644 --- a/reviewboard/reviews/models/review_request.py +++ b/reviewboard/reviews/models/review_request.py @@ -14,6 +14,7 @@ from reviewboard.changedescs.models import ChangeDescription from reviewboard.diffviewer.models import DiffSet, DiffSetHistory, FileDiff from reviewboard.reviews.errors import PermissionError from reviewboard.reviews.managers import ReviewRequestManager +from reviewboard.reviews.models.base_comment import BaseComment from reviewboard.reviews.models.base_review_request_details import \ BaseReviewRequestDetails from reviewboard.reviews.models.group import Group @@ -26,6 +27,66 @@ from reviewboard.site.models import LocalSite from reviewboard.site.urlresolvers import local_site_reverse +def fetch_issue_counts(review_request, extra_query=None): + """Fetches all issue counts for a review request. + + This queries all opened issues across all public comments on a + review request and returns them. + """ + issue_counts = { + BaseComment.OPEN: 0, + BaseComment.RESOLVED: 0, + BaseComment.DROPPED: 0 + } + + q = Q(public=True) + + if extra_query: + q = q & extra_query + + issue_statuses = review_request.reviews.filter(q).values( + 'comments__issue_status', + 'file_attachment_comments__issue_status', + 'screenshot_comments__issue_status') + + for issue_fields in issue_statuses: + for issue_status in issue_fields.itervalues(): + if issue_status: + issue_counts[issue_status] += 1 + + return issue_counts + + +def _initialize_issue_counts(review_request): + """Initializes the issue counter fields for a review request. + + This will fetch all the issue counts and populate the counter fields. + + Due to the way that CounterField works, this will only be called once + per review request, instead of once per field, due to all the fields + being set at once. This will also take care of the actual saving of + fields, rather than leaving that up to CounterField, in order to save + all at once, + """ + if review_request.pk is None: + return 0 + + issue_counts = fetch_issue_counts(review_request) + + review_request.issue_open_count = issue_counts[BaseComment.OPEN] + review_request.issue_resolved_count = issue_counts[BaseComment.RESOLVED] + review_request.issue_dropped_count = issue_counts[BaseComment.DROPPED] + + review_request.save(update_fields=[ + 'issue_open_count', + 'issue_resolved_count', + 'issue_dropped_count' + ]) + + # Tell CounterField not to set or save any values. + return None + + class ReviewRequest(BaseReviewRequestDetails): """A review request. @@ -46,6 +107,12 @@ class ReviewRequest(BaseReviewRequestDetails): (DISCARDED, _('Discarded')), ) + ISSUE_COUNTER_FIELDS = { + BaseComment.OPEN: 'issue_open_count', + BaseComment.RESOLVED: 'issue_resolved_count', + BaseComment.DROPPED: 'issue_dropped_count', + } + submitter = models.ForeignKey(User, verbose_name=_("submitter"), related_name="review_requests") time_added = models.DateTimeField(_("time added"), default=timezone.now) @@ -130,6 +197,18 @@ class ReviewRequest(BaseReviewRequestDetails): blank=True) shipit_count = CounterField(_("ship-it count"), default=0) + issue_open_count = CounterField( + _('open issue count'), + initializer=_initialize_issue_counts) + + issue_resolved_count = CounterField( + _('resolved issue count'), + initializer=_initialize_issue_counts) + + issue_dropped_count = CounterField( + _('dropped issue count'), + initializer=_initialize_issue_counts) + local_site = models.ForeignKey(LocalSite, blank=True, null=True) local_id = models.IntegerField('site-local ID', blank=True, null=True) diff --git a/reviewboard/reviews/tests.py b/reviewboard/reviews/tests.py index 957ea3538d1b80fbb08559bf3087b26d68ae75b3..cee3821bd983c9da6d75c6816e1d4296541a5bf4 100644 --- a/reviewboard/reviews/tests.py +++ b/reviewboard/reviews/tests.py @@ -1873,7 +1873,7 @@ class IfNeatNumberTagTests(TestCase): self.assertEqual(t.render(Context({})), expected) -class CounterTests(TestCase): +class ReviewRequestCounterTests(TestCase): fixtures = ['test_scmtools'] def setUp(self): @@ -2403,6 +2403,137 @@ class CounterTests(TestCase): self.group = Group.objects.get(pk=self.group.pk) +class IssueCounterTests(TestCase): + fixtures = ['test_users'] + + def setUp(self): + self.review_request = self.create_review_request(publish=True) + self.assertEqual(self.review_request.issue_open_count, 0) + self.assertEqual(self.review_request.issue_resolved_count, 0) + self.assertEqual(self.review_request.issue_dropped_count, 0) + + self._reset_counts() + + @add_fixtures(['test_scmtools']) + def test_init_with_diff_comments(self): + """Testing ReviewRequest issue counter initialization + from diff comments + """ + self.review_request.repository = self.create_repository() + + diffset = self.create_diffset(self.review_request) + filediff = self.create_filediff(diffset) + + self._test_issue_counts( + lambda review, issue_opened: self.create_diff_comment( + review, filediff, issue_opened=issue_opened)) + + def test_file_attachment_comments(self): + """Testing ReviewRequest issue counter initialization + from file attachment comments + """ + file_attachment = self.create_file_attachment(self.review_request) + + self._test_issue_counts( + lambda review, issue_opened: self.create_file_attachment_comment( + review, file_attachment, issue_opened=issue_opened)) + + def test_screenshot_comments(self): + """Testing ReviewRequest issue counter initialization + from screenshot comments + """ + screenshot = self.create_screenshot(self.review_request) + + self._test_issue_counts( + lambda review, issue_opened: self.create_screenshot_comment( + review, screenshot, issue_opened=issue_opened)) + + def _test_issue_counts(self, create_comment_func): + review = self.create_review(self.review_request) + + # One comment without an issue opened. + create_comment_func(review, issue_opened=False) + + # Three comments with an issue opened. + open_comments = [ + create_comment_func(review, issue_opened=True) + for i in range(3) + ] + + # Two comments with an issue dropped. + dropped_comments = [ + create_comment_func(review, issue_opened=True) + for i in range(2) + ] + + # One comment with an issue fixed. + resolved_comments = [ + create_comment_func(review, issue_opened=True) + ] + + # The issue counts should be end up being 0, since they'll initialize + # during load. + self._reload_object(clear_counters=True) + self.assertEqual(self.review_request.issue_open_count, 0) + self.assertEqual(self.review_request.issue_resolved_count, 0) + self.assertEqual(self.review_request.issue_dropped_count, 0) + + # Now publish. We should have 6 open issues, by way of incrementing + # during publish. + review.publish() + + self._reload_object() + self.assertEqual(self.review_request.issue_open_count, 6) + self.assertEqual(self.review_request.issue_dropped_count, 0) + self.assertEqual(self.review_request.issue_resolved_count, 0) + + # Make sure we get the same number back when initializing counters. + self._reload_object(clear_counters=True) + self.assertEqual(self.review_request.issue_open_count, 6) + self.assertEqual(self.review_request.issue_dropped_count, 0) + self.assertEqual(self.review_request.issue_resolved_count, 0) + + # Set the issue statuses. + for comment in dropped_comments: + comment.issue_status = Comment.DROPPED + comment.save() + + for comment in resolved_comments: + comment.issue_status = Comment.RESOLVED + comment.save() + + self._reload_object() + self.assertEqual(self.review_request.issue_open_count, 3) + self.assertEqual(self.review_request.issue_dropped_count, 2) + self.assertEqual(self.review_request.issue_resolved_count, 1) + + # Make sure we get the same number back when initializing counters. + self._reload_object(clear_counters=True) + self.assertEqual(self.review_request.issue_open_count, 3) + self.assertEqual(self.review_request.issue_dropped_count, 2) + self.assertEqual(self.review_request.issue_resolved_count, 1) + + def _reload_object(self, clear_counters=False): + if clear_counters: + # 3 queries: One for the review request fetch, one for + # the issue status load, and one for updating the issue counts. + expected_query_count = 3 + self._reset_counts() + else: + # One query for the review request fetch. + expected_query_count = 1 + + with self.assertNumQueries(expected_query_count): + self.review_request = \ + ReviewRequest.objects.get(pk=self.review_request.pk) + + def _reset_counts(self): + self.review_request.issue_open_count = None + self.review_request.issue_resolved_count = None + self.review_request.issue_dropped_count = None + self.review_request.save() + + class PolicyTests(TestCase): fixtures = ['test_users'] diff --git a/reviewboard/testing/testcase.py b/reviewboard/testing/testcase.py index 71bb1def35c607f07c1031c413a492334980ff93..feef73b175e7064c12f1039d35b08c80787b8a68 100644 --- a/reviewboard/testing/testcase.py +++ b/reviewboard/testing/testcase.py @@ -125,6 +125,7 @@ class TestCase(DjbletsTestCase): def create_diff_comment(self, review, filediff, interfilediff=None, text='My comment', issue_opened=False, + issue_status=None, first_line=1, num_lines=5, extra_fields=None, reply_to=None, **kwargs): """Creates a Comment for testing. @@ -133,10 +134,8 @@ class TestCase(DjbletsTestCase): an interfilediff). It's populated with default data that can be overridden by the caller. """ - if issue_opened: + if issue_opened and not issue_status: issue_status = Comment.OPEN - else: - issue_status = None comment = Comment( filediff=filediff, @@ -315,8 +314,11 @@ class TestCase(DjbletsTestCase): diffset_history=DiffSetHistory.objects.create(), repository=repository, public=public, - status=status, - id=id) + status=status) + + # Set this separately to avoid issues with CounterField updates. + review_request.id = id + review_request.save() if publish: diff --git a/reviewboard/webapi/resources/base_comment.py b/reviewboard/webapi/resources/base_comment.py index 3e2be299257485c335fb84617ab42cf3a1616699..48b33d453932cd77fa93551dbd47ea7ab477fdb6 100644 --- a/reviewboard/webapi/resources/base_comment.py +++ b/reviewboard/webapi/resources/base_comment.py @@ -217,11 +217,13 @@ class BaseCommentResource(MarkdownFieldsMixin, WebAPIResource): if not comment.issue_opened: raise PermissionDenied + comment._review_request = review_request + # We can only update the status of the issue issue_status = \ BaseComment.issue_string_to_status(kwargs.get('issue_status')) comment.issue_status = issue_status - comment.save() + comment.save(update_fields=['issue_status']) last_activity_time, updated_object = review_request.get_last_activity() comment.timestamp = localize(comment.timestamp)