diff --git a/reviewboard/reviews/models/base_comment.py b/reviewboard/reviews/models/base_comment.py index 4a2320cbc360d929a22a43718a3e2fd1cbd01eac..e88f2c1163cac9f45550da2689b51c624fca6e99 100644 --- a/reviewboard/reviews/models/base_comment.py +++ b/reviewboard/reviews/models/base_comment.py @@ -13,6 +13,7 @@ from djblets.db.fields import CounterField, JSONField from reviewboard.admin.read_only import is_site_read_only_for from reviewboard.reviews.managers import CommentManager +from reviewboard.reviews.signals import comment_issue_status_updated if TYPE_CHECKING: from reviewboard.reviews.models import Review @@ -275,18 +276,32 @@ class BaseComment(models.Model): user.pk == review.user_id or (local_site and local_site.is_mutable_by(user))) - def save(self, **kwargs): + def save(self, *args, **kwargs) -> None: """Save the comment. Args: + *args (tuple): + Positional arguments to pass to the save method. + **kwargs (dict): - Keyword arguments passed to the method (unused). + Keyword arguments passed to the save method. """ from reviewboard.reviews.models.review_request import ReviewRequest self.timestamp = timezone.now() - super(BaseComment, self).save() + if update_fields := kwargs.get('update_fields'): + # Add the timestamp to update_fields if it was passed. + kwargs['update_fields'] = { + *update_fields, + 'timestamp', + } + + super().save(*args, **kwargs) + + issue_status_updated: bool = False + prev_issue_status = self._loaded_issue_status + issue_status = self.issue_status try: # Update the review timestamp, but only if it's a draft. @@ -300,13 +315,16 @@ class BaseComment(models.Model): else: if (not self.is_reply() and self.issue_opened and - self._loaded_issue_status != self.issue_status): + prev_issue_status != issue_status): # The user has toggled the issue status of this comment, # so update the issue counts for the review request. + assert prev_issue_status is not None + assert issue_status is not None + old_field = ReviewRequest.ISSUE_COUNTER_FIELDS[ - self._loaded_issue_status] + prev_issue_status] new_field = ReviewRequest.ISSUE_COUNTER_FIELDS[ - self.issue_status] + issue_status] if old_field != new_field: CounterField.increment_many( @@ -316,8 +334,18 @@ class BaseComment(models.Model): new_field: 1, }) + self._loaded_issue_status = issue_status + issue_status_updated = True + q = ReviewRequest.objects.filter(pk=review.review_request_id) q.update(last_review_activity_timestamp=self.timestamp) + + if issue_status_updated: + comment_issue_status_updated.send( + sender=self.__class__, + comment=self, + prev_status=prev_issue_status, + cur_status=issue_status) except ObjectDoesNotExist: pass diff --git a/reviewboard/reviews/signals.py b/reviewboard/reviews/signals.py index 782f13827987302940a6752ce69d9513180bd174..12c8422adb0803f1d73c97919d642d9e4c51c983 100644 --- a/reviewboard/reviews/signals.py +++ b/reviewboard/reviews/signals.py @@ -190,7 +190,7 @@ review_published = Signal() #: user (django.contrib.auth.models.User): #: The user publishing the reply. #: -#: review (reviewboard.reviews.models.Review): +#: reply (reviewboard.reviews.models.Review): #: The reply that's being published. reply_publishing = Signal() @@ -201,7 +201,7 @@ reply_publishing = Signal() #: user (django.contrib.auth.models.User): #: The user who published the reply. #: -#: review (reviewboard.reviews.models.Review): +#: reply (reviewboard.reviews.models.Review): #: The reply that was published. #: #: trivial (bool): @@ -209,6 +209,23 @@ reply_publishing = Signal() reply_published = Signal() +#: Emitted when a comment's issue status has been updated. +#: +#: Version Added: +#: 7.1: +#: +#: Args: +#: comment (reviewboard.reviews.models.BaseComment): +#: The comment that had its issue status updated. +#: +#: prev_status (str): +#: The previous value for the issue status. +#: +#: cur_status (str): +#: The current value for the issue status. +comment_issue_status_updated = Signal() + + #: Emitted when a StatusUpdate should run or re-run. #: #: Version Changed: diff --git a/reviewboard/reviews/tests/test_base_comment.py b/reviewboard/reviews/tests/test_base_comment.py new file mode 100644 index 0000000000000000000000000000000000000000..34f9a8db48dcc772cc04ee85cf84e1fa86f3ab7c --- /dev/null +++ b/reviewboard/reviews/tests/test_base_comment.py @@ -0,0 +1,184 @@ +"""Unit tests for reviewboard.reviews.models.BaseComment. + +Version Added: + 7.1 +""" + +from __future__ import annotations + +import kgb +from django.db import models + +from reviewboard.reviews.models import BaseComment +from reviewboard.reviews.signals import comment_issue_status_updated +from reviewboard.testing import TestCase + + +class BaseCommentTests(kgb.SpyAgency, TestCase): + """Unit tests for reviewboard.reviews.models.BaseComment. + + Version Added: + 7.1 + """ + + fixtures = ['test_users'] + + def setUp(self) -> None: + """Set up the test case.""" + super().setUp() + + review_request = self.create_review_request(publish=True) + review = self.create_review(review_request=review_request) + self.review_request = review_request + self.review_draft = review + + def test_save_kwargs(self) -> None: + """Testing BaseComment.save passes its arguments to the parent save + method + """ + comment = self.create_general_comment(review=self.review_draft) + + self.spy_on(models.Model.save, owner=models.Model) + + comment.issue_opened = True + comment.issue_status = BaseComment.OPEN + + comment.save( + None, # Passing `force_insert` as a positional argument. + update_fields=['issue_opened', 'issue_status', 'timestamp']) + + self.assertSpyCalledWith( + models.Model.save, + force_insert=None, + update_fields={'issue_opened', 'issue_status', 'timestamp'}) + + def test_issue_updated_signal_with_created(self) -> None: + """Testing comment creation does not emit the + comment_issue_status_updated signal + """ + def on_issue_status_updated(**kwargs) -> None: + pass + + self.addCleanup(comment_issue_status_updated.disconnect, + on_issue_status_updated) + + comment_issue_status_updated.connect(on_issue_status_updated) + self.spy_on(on_issue_status_updated) + + # Create a comment that opens an issue, and one that doesn't. + self.create_general_comment(review=self.review_draft) + self.create_general_comment( + review=self.review_draft, + issue_opened=True, + issue_status=BaseComment.OPEN) + + self.assertSpyNotCalled(on_issue_status_updated) + + def test_issue_updated_signal_with_draft_update(self) -> None: + """Testing updating the issue status on a draft comment does not + emit the comment_issue_status_updated signal + """ + def on_issue_status_updated(**kwargs) -> None: + pass + + self.addCleanup(comment_issue_status_updated.disconnect, + on_issue_status_updated) + + comment = self.create_general_comment( + review=self.review_draft, + issue_opened=True, + issue_status=BaseComment.OPEN) + + comment_issue_status_updated.connect(on_issue_status_updated) + self.spy_on(on_issue_status_updated) + + comment.issue_status = BaseComment.DROPPED + comment.save(update_fields=['issue_status']) + + self.assertSpyNotCalled(on_issue_status_updated) + + def test_issue_updated_signal_with_issue_dropped(self) -> None: + """Testing dropping the issue on a published comment emits the + comment_issue_status_updated signal + """ + def on_issue_status_updated(**kwargs) -> None: + pass + + self.addCleanup(comment_issue_status_updated.disconnect, + on_issue_status_updated) + + review_draft = self.review_draft + comment = self.create_general_comment( + review=review_draft, + issue_opened=True, + issue_status=BaseComment.OPEN) + review_draft.publish() + + comment_issue_status_updated.connect(on_issue_status_updated) + self.spy_on(on_issue_status_updated) + + comment.issue_status = BaseComment.DROPPED + comment.save(update_fields=['issue_status']) + + self.assertSpyCalledWith(on_issue_status_updated, + comment=comment, + prev_status=BaseComment.OPEN, + cur_status=BaseComment.DROPPED) + + def test_issue_updated_signal_with_issue_reopened(self) -> None: + """Testing re-opening the issue on a published comment emits the + comment_issue_status_updated signal + """ + def on_issue_status_updated(**kwargs) -> None: + pass + + self.addCleanup(comment_issue_status_updated.disconnect, + on_issue_status_updated) + + review_draft = self.review_draft + comment = self.create_general_comment( + review=review_draft, + issue_opened=True, + issue_status=BaseComment.OPEN) + review_draft.publish() + comment.issue_status = BaseComment.DROPPED + comment.save(update_fields=['issue_status']) + + comment_issue_status_updated.connect(on_issue_status_updated) + self.spy_on(on_issue_status_updated) + + comment.issue_status = BaseComment.OPEN + comment.save(update_fields=['issue_status']) + + self.assertSpyCalledWith(on_issue_status_updated, + comment=comment, + prev_status=BaseComment.DROPPED, + cur_status=BaseComment.OPEN) + + def test_issue_updated_signal_with_issue_resolved(self) -> None: + """Testing resolving the issue on a published comment emits the + comment_issue_status_updated signal + """ + def on_issue_status_updated(**kwargs) -> None: + pass + + self.addCleanup(comment_issue_status_updated.disconnect, + on_issue_status_updated) + + review_draft = self.review_draft + comment = self.create_general_comment( + review=review_draft, + issue_opened=True, + issue_status=BaseComment.OPEN) + review_draft.publish() + + comment_issue_status_updated.connect(on_issue_status_updated) + self.spy_on(on_issue_status_updated) + + comment.issue_status = BaseComment.RESOLVED + comment.save(update_fields=['issue_status']) + + self.assertSpyCalledWith(on_issue_status_updated, + comment=comment, + prev_status=BaseComment.OPEN, + cur_status=BaseComment.RESOLVED)