diff --git a/reviewboard/reviews/models/review.py b/reviewboard/reviews/models/review.py index 5bdcc35690aeec767b62d78d0c70a5267fb113f2..ea3e1274893d9f21a89c71af75dec72d9613791d 100644 --- a/reviewboard/reviews/models/review.py +++ b/reviewboard/reviews/models/review.py @@ -355,8 +355,9 @@ class Review(models.Model): # Update the last_updated timestamp and the last review activity # timestamp on the review request. self.review_request.last_review_activity_timestamp = self.timestamp - self.review_request.save( - update_fields=['last_review_activity_timestamp', 'last_updated']) + self.review_request.last_updated = self.timestamp + self.review_request.save(update_fields=( + 'last_review_activity_timestamp', 'last_updated')) 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 2e3e22b2f7295fdd2b9592610048d3dbac9ae4f8..3f5ded37a06529054b74d9bbbd76aa1cef7bfa82 100644 --- a/reviewboard/reviews/models/review_request.py +++ b/reviewboard/reviews/models/review_request.py @@ -1029,11 +1029,21 @@ class ReviewRequest(BaseReviewRequestDetails): if self.public: self._decrement_reviewer_counts() + # Calculate the timestamp once and use it for all things that are + # considered as happening now. If we do not do this, there will be + # millisecond timestamp differences between review requests and their + # changedescs, diffsets, and reviews. + # + # Keeping them in sync means that get_last_activity() can work as + # intended. Otherwise, the review request will always have the most + # recent timestamp since it gets saved last. + timestamp = timezone.now() + if draft is not None: # This will in turn save the review request, so we'll be done. try: changes = draft.publish(self, send_notification=False, - user=user) + user=user, timestamp=timestamp) except Exception: # The draft failed to publish, for one reason or another. # Check if we need to re-increment those counters we @@ -1050,9 +1060,10 @@ class ReviewRequest(BaseReviewRequestDetails): if not self.public and self.changedescs.count() == 0: # This is a brand new review request that we're publishing # for the first time. Set the creation timestamp to now. - self.time_added = timezone.now() + self.time_added = timestamp self.public = True + self.last_updated = timestamp self.save(update_counts=True, old_submitter=old_submitter) review_request_published.send(sender=self.__class__, user=user, diff --git a/reviewboard/reviews/models/review_request_draft.py b/reviewboard/reviews/models/review_request_draft.py index 2ed58357d9a6e90bf029d14c471c1df4656027bf..ff0d822742cbfc1241df8988707615a7e55fc2ca 100644 --- a/reviewboard/reviews/models/review_request_draft.py +++ b/reviewboard/reviews/models/review_request_draft.py @@ -14,12 +14,12 @@ from reviewboard.attachments.models import FileAttachment from reviewboard.changedescs.models import ChangeDescription from reviewboard.diffviewer.models import DiffSet from reviewboard.reviews.errors import NotModifiedError, PublishError +from reviewboard.reviews.fields import get_review_request_fields from reviewboard.reviews.models.group import Group from reviewboard.reviews.models.base_review_request_details import \ BaseReviewRequestDetails from reviewboard.reviews.models.review_request import ReviewRequest from reviewboard.reviews.models.screenshot import Screenshot -from reviewboard.reviews.fields import get_review_request_fields from reviewboard.reviews.signals import review_request_published from reviewboard.scmtools.errors import InvalidChangeNumberError @@ -193,16 +193,17 @@ class ReviewRequestDraft(BaseReviewRequestDetails): return draft def publish(self, review_request=None, user=None, trivial=False, - send_notification=True): - """Publishes this draft. + send_notification=True, timestamp=None): + """Publish this draft. + + This is an internal method. Programmatic publishes should use + :py:meth:`reviewboard.reviews.models.review_request.ReviewRequest.publish` + instead. This updates and returns the draft's ChangeDescription, which contains the changed fields. This is used by the e-mail template to tell people what's new and interesting. - The draft's associated ReviewRequest object will be used if one isn't - passed in. - The keys that may be saved in ``fields_changed`` in the ChangeDescription are: @@ -234,10 +235,45 @@ class ReviewRequestDraft(BaseReviewRequestDetails): For the ``diff`` field, there is only ever an ``added`` field, containing the ID of the new diffset. - The ``send_notification`` parameter is intended for internal use only, - and is there to prevent duplicate notifications when being called by - ReviewRequest.publish. + Args: + review_request (reviewboard.reviews.models.review_request. + ReviewRequest, optional): + The review request associated with this diff. If not provided, + it will be looked up. + + user (django.contrib.auth.models.User, optional): + The user publishing the draft. If not provided, this defaults + to the review request submitter. + + trivial (bool, optional): + Whether or not this is a trivial publish. + + Trivial publishes do not result in e-mail notifications. + + send_notification (bool, optional): + Whether or not this will emit the + :py:data:`reviewboard.reviews.signals.review_request_published` + signal. + + This parameter is intended for internal use **only**. + + timestamp (datetime.datetime, optional): + The datetime that should be used for all timestamps for objects + published + (:py:class:`~reviewboard.diffviewer.models.diff_set.DiffSet`, + :py:class:`~reviewboard.changedescs.models.ChangeDescription`) + over the course of the method. + + Returns: + reviewboard.changedescs.models.ChangeDescription: + The change description that results from this publish (if any). + + If this is an initial publish, there will be no change description + (and this function will return ``None``). """ + if timestamp is None: + timestamp = timezone.now() + if not review_request: review_request = self.review_request @@ -252,10 +288,6 @@ class ReviewRequestDraft(BaseReviewRequestDetails): self.copy_fields_to_request(review_request) - if self.diffset: - self.diffset.history = review_request.diffset_history - self.diffset.save(update_fields=['history']) - # If no changes were made, raise exception and do not save if self.changedesc and not self.changedesc.has_modified_fields(): raise NotModifiedError() @@ -271,9 +303,14 @@ class ReviewRequestDraft(BaseReviewRequestDetails): if not review_request.description.strip(): raise PublishError(ugettext('The draft must have a description.')) + if self.diffset: + self.diffset.history = review_request.diffset_history + self.diffset.timestamp = timestamp + self.diffset.save(update_fields=('history', 'timestamp')) + if self.changedesc: self.changedesc.user = user - self.changedesc.timestamp = timezone.now() + self.changedesc.timestamp = timestamp self.changedesc.public = True self.changedesc.save() review_request.changedescs.add(self.changedesc) @@ -284,7 +321,7 @@ class ReviewRequestDraft(BaseReviewRequestDetails): review_request.save() if send_notification: - review_request_published.send(sender=review_request.__class__, + review_request_published.send(sender=type(review_request), user=user, review_request=review_request, trivial=trivial, diff --git a/reviewboard/reviews/tests/test_review_request.py b/reviewboard/reviews/tests/test_review_request.py index 4f0353b317357c836a05238976aae03a8cf21468..fa15cd7499398f900df32c0fbff9ee5172ab4ffd 100644 --- a/reviewboard/reviews/tests/test_review_request.py +++ b/reviewboard/reviews/tests/test_review_request.py @@ -8,6 +8,7 @@ from djblets.testing.decorators import add_fixtures from kgb import SpyAgency from reviewboard.changedescs.models import ChangeDescription +from reviewboard.diffviewer.models import DiffSet from reviewboard.reviews.errors import PublishError from reviewboard.reviews.models import (Comment, ReviewRequest, ReviewRequestDraft) @@ -344,6 +345,83 @@ class ReviewRequestTests(SpyAgency, TestCase): self.assertEqual(change3.user, grumpy) self.assertEqual(change4.user, grumpy) + @add_fixtures(['test_scmtools']) + def test_last_updated(self): + """Testing ReviewRequest.last_updated stays in sync with + Review.timestamp when a review is published + """ + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + filediff = self.create_filediff(diffset) + + review1 = self.create_review(review_request, publish=True) + self.assertEqual(review_request.last_updated, review1.timestamp) + + review2 = self.create_review(review_request, publish=True) + self.assertEqual(review_request.last_updated, review2.timestamp) + + # Create a diff review. + diff_review = self.create_review(review_request) + self.create_diff_comment(diff_review, filediff) + diff_review.publish() + self.assertEqual(review_request.last_updated, diff_review.timestamp) + + @add_fixtures(['test_scmtools']) + def test_get_last_activity_for_updated_review(self): + """Testing ReviewRequest.get_last_activity returns the latest review + when a new review is published + """ + review_request = self.create_review_request(publish=True) + review = self.create_review(review_request, publish=True) + + timestamp, updated_object = review_request.get_last_activity() + + self.assertEqual(updated_object, review) + self.assertEqual(timestamp, review.timestamp) + + @add_fixtures(['test_scmtools']) + def test_get_last_activity_for_updated_diffset(self): + """Testing ReviewRequest.get_last_activity returns the latest + diffset when a new diff revision is published + """ + user = User.objects.get(username='doc') + review_request = self.create_review_request(publish=True, + create_repository=True, + target_people=[user]) + diffset = self.create_diffset(review_request=review_request, + revision=2, + draft=True) + + review_request.publish(user=review_request.submitter) + + timestamp, updated_object = review_request.get_last_activity() + + diffset = DiffSet.objects.get(pk=diffset.pk) + + self.assertEqual(updated_object, diffset) + self.assertEqual(timestamp, diffset.timestamp) + + @add_fixtures(['test_scmtools']) + def test_get_last_activity_for_updated_review_request(self): + """Testing ReviewRequest.get_last_activity returns the review request + when it is updated + """ + user = User.objects.get(username='doc') + review_request = self.create_review_request(publish=True, + create_repository=True, + target_people=[user]) + draft = ReviewRequestDraft.create(review_request) + draft.summary = 'This is a new summary.' + draft.save() + review_request.publish(user=review_request.submitter) + + timestamp, updated_object = review_request.get_last_activity() + changedesc = review_request.changedescs.latest() + + self.assertEqual(updated_object, review_request) + self.assertEqual(timestamp, changedesc.timestamp) + class IssueCounterTests(TestCase): """Unit tests for review request issue counters."""