diff --git a/docs/manual/glossary.rst b/docs/manual/glossary.rst index 4ef880e6e89923d2e3b07c6070d6da14acfabbfc..fafd920e3822a7e5a26b2ee8d432a34ee960ae3f 100644 --- a/docs/manual/glossary.rst +++ b/docs/manual/glossary.rst @@ -24,6 +24,11 @@ Glossary lines with an exclamation point (``!``). Changes that only add lines only show one section. + Cumulative Diff + A diff that represents a squashed set of commits, each of which has + their own diffs. It is equivalent to applying all of the diffs for all + the commits in order. + Default Reviewer Default Reviewers A feature that allows individual ussers or :term:`review groups` to be diff --git a/reviewboard/reviews/models/diff_comment.py b/reviewboard/reviews/models/diff_comment.py index c99980cf764719729190164f389d5906a8c165de..94c63b11e188c07e405b36b2f37f735e406691fc 100644 --- a/reviewboard/reviews/models/diff_comment.py +++ b/reviewboard/reviews/models/diff_comment.py @@ -14,6 +14,9 @@ class Comment(BaseComment): A comment can belong to a single filediff or to an interdiff between two filediffs. It can also have multiple replies. """ + + _BASE_FILEDIFF_ID_KEY = '__base_filediff_id' + anchor_prefix = "comment" comment_type = "diff" filediff = models.ForeignKey(FileDiff, verbose_name=_('file diff'), @@ -32,6 +35,19 @@ class Comment(BaseComment): last_line = property(lambda self: self.first_line + self.num_lines - 1) + @property + def base_filediff_id(self): + """The base FileDiff ID for the cumulative diff this comment is on.""" + return (self.extra_data and + self.extra_data.get(self._BASE_FILEDIFF_ID_KEY)) + + @base_filediff_id.setter + def base_filediff_id(self, filediff_id): + if self.extra_data is None: + self.extra_data = {} + + self.extra_data[self._BASE_FILEDIFF_ID_KEY] = filediff_id + def get_absolute_url(self): revision_path = six.text_type(self.filediff.diffset.revision) if self.interfilediff: diff --git a/reviewboard/webapi/resources/base_comment.py b/reviewboard/webapi/resources/base_comment.py index 37acb5ab62a17c14997cdcc8d0993ff5582f43a1..3d44744a7727229a03f82c0eae29442a24120943 100644 --- a/reviewboard/webapi/resources/base_comment.py +++ b/reviewboard/webapi/resources/base_comment.py @@ -187,6 +187,7 @@ class BaseCommentResource(MarkdownFieldsMixin, WebAPIResource): issue_opened=False, text_type=MarkdownFieldsMixin.TEXT_TYPE_PLAIN, extra_fields={}, + save=True, **kwargs): """Create a comment based on the requested data. @@ -219,6 +220,12 @@ class BaseCommentResource(MarkdownFieldsMixin, WebAPIResource): API resource. Any ``extra_data`` modifications from this will be applied to the comment. + save (bool, optional): + Whether or not to save the field and update ``comments_m2m``. + + If ``False``, the caller is responsible for performing the + save. + **kwargs (dict): Keyword arguments representing additional fields handled by the API resource. Any that are also listed in ``fields`` will @@ -251,8 +258,9 @@ class BaseCommentResource(MarkdownFieldsMixin, WebAPIResource): else: new_comment.issue_status = None - new_comment.save() - comments_m2m.add(new_comment) + if save: + new_comment.save() + comments_m2m.add(new_comment) return 201, { self.item_result_key: new_comment, diff --git a/reviewboard/webapi/resources/review_diff_comment.py b/reviewboard/webapi/resources/review_diff_comment.py index fd5caf6a36c07f60e1f511b38b707f527a53a43e..95a8ac1a625bb8100cc53be451e050ec492f9a65 100644 --- a/reviewboard/webapi/resources/review_diff_comment.py +++ b/reviewboard/webapi/resources/review_diff_comment.py @@ -9,6 +9,7 @@ from djblets.webapi.errors import (DOES_NOT_EXIST, INVALID_FORM_DATA, NOT_LOGGED_IN, PERMISSION_DENIED) from djblets.webapi.fields import IntFieldType +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.diffviewer.models import FileDiff from reviewboard.webapi.decorators import webapi_check_local_site from reviewboard.webapi.resources import resources @@ -55,6 +56,14 @@ class ReviewDiffCommentResource(BaseDiffCommentResource): }, }, **BaseDiffCommentResource.REQUIRED_CREATE_FIELDS), optional=dict({ + 'base_filediff_id': { + 'type': IntFieldType, + 'description': 'The ID of the base filediff for the ' + ':term:`cumulative diff` the comment is on.\n' + '\n' + 'This is only supported for review requests ' + 'created with commit history support.', + }, 'interfilediff_id': { 'type': IntFieldType, 'description': 'The ID of the second file diff in the ' @@ -64,7 +73,7 @@ class ReviewDiffCommentResource(BaseDiffCommentResource): allow_unknown=True, ) def create(self, request, filediff_id, interfilediff_id=None, - *args, **kwargs): + base_filediff_id=None, *args, **kwargs): """Creates a new diff comment. This will create a new diff comment on this review. The review @@ -92,21 +101,76 @@ class ReviewDiffCommentResource(BaseDiffCommentResource): pk=filediff_id, diffset__history__review_request=review_request) except ObjectDoesNotExist: - invalid_fields['filediff_id'] = \ - ['This is not a valid filediff ID'] + invalid_fields['filediff_id'] = [ + 'This is not a valid filediff ID.', + ] + + if filediff is None or not dvcs_feature.is_enabled(request=request): + base_filediff_id = None + + if base_filediff_id is not None: + if not review_request.created_with_history: + invalid_fields['base_filediff_id'] = [ + 'This field cannot be specified on review requests ' + 'created without history support.' + ] + elif interfilediff_id is not None: + invalid_fields.update({ + 'base_filediff_id': [ + 'This field cannot be specified with ' + 'interfilediff_id.', + ], + 'interfilediff_id': [ + 'This field cannot be specified with ' + 'base_filediff_id.', + ], + }) + elif base_filediff_id == filediff_id: + invalid_fields['base_filediff_id'] = [ + 'This cannot be the same as filediff_id.', + ] + + elif base_filediff_id > filediff_id: + invalid_fields['base_filediff_id'] = [ + 'This is not a valid base filediff ID.', + ] + else: + base_filediff_exists = ( + FileDiff.objects + .filter(diffset_id=filediff.diffset_id, + pk=base_filediff_id) + .exclude(commit_id=filediff.commit_id) + .exists() + ) + + if not base_filediff_exists: + invalid_fields['base_filediff_id'] = [ + 'This is not a valid base filediff ID.', + ] + else: + ancestor_ids = ( + ancestor.pk + for ancestor in filediff.get_ancestors( + minimal=False) + ) + + if base_filediff_id not in ancestor_ids: + invalid_fields['base_filediff_id'] = [ + 'This is not a valid base filediff ID.', + ] if filediff and interfilediff_id: if interfilediff_id == filediff.id: - invalid_fields['interfilediff_id'] = \ - ['This cannot be the same as filediff_id'] + invalid_fields.setdefault('interfilediff_id', []).append( + 'This cannot be the same as filediff_id.') else: try: interfilediff = FileDiff.objects.get( pk=interfilediff_id, diffset__history=filediff.diffset.history) except ObjectDoesNotExist: - invalid_fields['interfilediff_id'] = \ - ['This is not a valid interfilediff ID'] + invalid_fields.setdefault('interfilediff_id', []).append( + 'This is not a valid interfilediff ID.') if invalid_fields: return INVALID_FORM_DATA, { @@ -114,11 +178,13 @@ class ReviewDiffCommentResource(BaseDiffCommentResource): } return self.create_comment( + request=request, review=review, comments_m2m=review.comments, filediff=filediff, interfilediff=interfilediff, fields=('filediff', 'interfilediff', 'first_line', 'num_lines'), + base_filediff_id=base_filediff_id, **kwargs) @webapi_check_local_site @@ -188,5 +254,77 @@ class ReviewDiffCommentResource(BaseDiffCommentResource): """ pass + def create_comment(self, request, comments_m2m, base_filediff_id=None, + **kwargs): + """Create a review comment. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + comments_m2m (django.db.models.ManyToManyField): + The review's comments relation, where the new comment will be + added. + + base_filediff_id (int, optional): + The ID of the base filediff for the :term:`cumulative diff` the + comment is on. + + **kwargs (dict): + Additional keyword arguments to pass on to the base class + method. + + Returns: + tuple or djblets.webapi.errors.WebAPIError: + Either a successful payload containing the comment, or an error + payload. + """ + rsp = super(ReviewDiffCommentResource, self).create_comment( + comments_m2m=comments_m2m, + save=False, + **kwargs) + + if (isinstance(rsp, tuple) and + isinstance(rsp[1], dict) and + self.item_result_key in rsp[1]): + comment = rsp[1][self.item_result_key] + + if (base_filediff_id is not None and + dvcs_feature.is_enabled(request=request)): + comment.base_filediff_id = base_filediff_id + + comment.save() + comments_m2m.add(comment) + + return rsp + + def serialize_object(self, obj, request=None, *args, **kwargs): + """Serialize a diff comment. + + Args: + obj (reviewboard.reviews.models.diff_comment.Comment): + The diff comment to serialize. + + request (django.http.HttpRequest, optional): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + The serialized diff comment. + """ + result = super(ReviewDiffCommentResource, self).serialize_object( + obj, request=request, *args, **kwargs) + + if not dvcs_feature.is_enabled(request=request): + result.pop('base_filediff_id', None) + + return result + review_diff_comment_resource = ReviewDiffCommentResource() diff --git a/reviewboard/webapi/tests/test_review_comment.py b/reviewboard/webapi/tests/test_review_comment.py index 9c31e6557e8865a8d0ca936b80e30f3925a6ef7b..edd19693ede543fa9c19540f99f2b897c89613b8 100644 --- a/reviewboard/webapi/tests/test_review_comment.py +++ b/reviewboard/webapi/tests/test_review_comment.py @@ -2,8 +2,13 @@ from __future__ import unicode_literals from django.contrib.auth.models import User from django.utils import six -from djblets.webapi.errors import PERMISSION_DENIED +from djblets.features.testing import override_feature_check +from djblets.webapi.errors import INVALID_FORM_DATA, PERMISSION_DENIED +from djblets.webapi.testing.decorators import webapi_test_template +from kgb import SpyAgency +from reviewboard.diffviewer.features import dvcs_feature +from reviewboard.diffviewer.models import FileDiff from reviewboard.reviews.models import Comment from reviewboard.webapi.resources import resources from reviewboard.webapi.tests.base import BaseWebAPITestCase @@ -58,14 +63,22 @@ class BaseResourceTestCase(BaseWebAPITestCase): return comment, review, review_request - def _create_diff_review_request(self, with_local_site=False): + def _create_diff_review_request(self, with_local_site=False, + with_history=False): review_request = self.create_review_request( create_repository=True, submitter=self.user, with_local_site=with_local_site, + create_with_history=with_history, publish=True) diffset = self.create_diffset(review_request) - filediff = self.create_filediff(diffset) + + if with_history: + commit = self.create_diffcommit(diffset=diffset) + else: + commit = None + + filediff = self.create_filediff(diffset=diffset, commit=commit) return review_request, filediff @@ -78,14 +91,45 @@ class BaseResourceTestCase(BaseWebAPITestCase): return review +def _compare_item(self, item_rsp, comment): + """Compare the API response with the object that was serialized. + + Args: + item_rsp (dict): + The serialized comment. + + comment (reviewboard.reviews.models.diff_comment.Comment): + The comment that was serialized. + + Raises: + AssertionError: + The API response was not equivalent to the object. + """ + self.assertEqual(item_rsp['id'], comment.pk) + self.assertEqual(item_rsp['text'], comment.text) + self.assertEqual(item_rsp['issue_opened'], comment.issue_opened) + self.assertEqual(item_rsp['first_line'], comment.first_line) + self.assertEqual(item_rsp['num_lines'], comment.num_lines) + + self.assertEqual(item_rsp['extra_data'], + self.resource._strip_private_data(comment.extra_data)) + + if comment.rich_text: + self.assertEqual(item_rsp['text_type'], 'markdown') + else: + self.assertEqual(item_rsp['text_type'], 'plain') + + @six.add_metaclass(BasicTestsMetaclass) -class ResourceListTests(CommentListMixin, ReviewRequestChildListMixin, - BaseResourceTestCase): +class ResourceListTests(SpyAgency, CommentListMixin, + ReviewRequestChildListMixin, BaseResourceTestCase): """Testing the ReviewDiffCommentResource list APIs.""" fixtures = ['test_users', 'test_scmtools'] sample_api_url = 'review-requests//reviews//diff-comments/' resource = resources.review_diff_comment + compare_item = _compare_item + def setup_review_request_child_test(self, review_request): if not review_request.repository_id: # The group tests don't create a repository by default. @@ -99,19 +143,6 @@ class ResourceListTests(CommentListMixin, ReviewRequestChildListMixin, return (get_review_diff_comment_list_url(review), review_diff_comment_list_mimetype) - def compare_item(self, item_rsp, comment): - self.assertEqual(item_rsp['id'], comment.pk) - self.assertEqual(item_rsp['text'], comment.text) - self.assertEqual(item_rsp['issue_opened'], comment.issue_opened) - self.assertEqual(item_rsp['first_line'], comment.first_line) - self.assertEqual(item_rsp['num_lines'], comment.num_lines) - self.assertEqual(item_rsp['extra_data'], comment.extra_data) - - if comment.rich_text: - self.assertEqual(item_rsp['text_type'], 'markdown') - else: - self.assertEqual(item_rsp['text_type'], 'plain') - # # HTTP GET tests # @@ -258,6 +289,551 @@ class ResourceListTests(CommentListMixin, ReviewRequestChildListMixin, self.assertEqual(comment.filediff_id, filediff.pk) self.assertEqual(comment.interfilediff_id, interfilediff.pk) + @webapi_test_template + def test_post_with_interfilediff_same_filediff(self): + """Testing the POST API with interfilediff_id == filediff_id""" + review_request, filediff = self._create_diff_review_request() + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'interfilediff_id': filediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'interfilediff_id': ['This cannot be the same as filediff_id.'], + }) + + @webapi_test_template + def test_post_with_interfilediff_outside_diffset_history(self): + """Testing the POST API with interfilediff_id corresponding to a + FileDiff outside the current DiffSetHistory + """ + review_request, filediff = self._create_diff_review_request() + review = self.create_review(review_request, user=self.user) + + other_filediff = self._create_diff_review_request()[1] + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'interfilediff_id': other_filediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'interfilediff_id': ['This is not a valid interfilediff ID.'], + }) + + @webapi_test_template + def test_post_with_base_filediff_dvcs_enabled_with_history(self): + """Testing the POST API with base_filediff_id with DVCS enabled + on a review reqest created with commit history + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffset = self.create_diffset(review_request) + + commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + filediffs = [ + self.create_filediff(diffset=diffset, commit=commits[0], + source_file='/foo', source_revision='1', + dest_file='/foo', dest_detail='2'), + self.create_filediff(diffset=diffset, commit=commits[1], + source_file='/foo', source_revision='2', + dest_file='/foo', dest_detail='3'), + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[1].pk, + 'base_filediff_id': filediffs[0].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + comment = Comment.objects.get(pk=item_rsp['id']) + + self.compare_item(item_rsp, comment) + + @webapi_test_template + def test_post_with_base_filediff_dvcs_disabled(self): + """Testing the POST API with base_filediff_id when DVCS feature + disabled + """ + with override_feature_check(dvcs_feature.feature_id, enabled=False): + review_request, filediff = self._create_diff_review_request() + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': filediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + comment = Comment.objects.get(pk=item_rsp['id']) + + self.compare_item(item_rsp, comment) + + @webapi_test_template + def test_post_with_base_filediff_dvcs_enabled_no_history(self): + """Testing the POST API with base_filediff_id when DVCS feature + enabled and review request not created with commit history + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request, filediff = self._create_diff_review_request() + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': filediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual( + rsp['fields']['base_filediff_id'], + ['This field cannot be specified on review requests created ' + 'without history support.']) + + @webapi_test_template + def test_post_with_base_filediff_dvcs_enabled_with_history_same_id(self): + """Testing the POST API with base_filediff_id=filediff_id""" + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request, filediff = self._create_diff_review_request( + with_history=True) + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': filediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'base_filediff_id': [ + 'This cannot be the same as filediff_id.', + ], + }, + }) + + @webapi_test_template + def test_post_with_base_filediff_interdiff_dvcs_disabled(self): + """Testing the POST API with base_filediff_id and interdiff_id + when DVCS feature disabled + """ + with override_feature_check(dvcs_feature.feature_id, enabled=False): + review_request, filediff = self._create_diff_review_request() + review = self.create_review(review_request, user=self.user) + + interdiffset = self.create_diffset(review_request) + interfilediff = self.create_filediff(interdiffset) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': filediff.pk, + 'interfilediff_id': interfilediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + comment = Comment.objects.get(pk=item_rsp['id']) + + self.compare_item(item_rsp, comment) + + @webapi_test_template + def test_post_with_base_filediff_interdiff_dvcs_enabled_with_history(self): + """Testing the POST API with base_filediff_id and + interfilediff_id when DVCS feature enabled + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request, filediff = self._create_diff_review_request( + with_history=True) + review = self.create_review(review_request, user=self.user) + + interdiffset = self.create_diffset(review_request) + interfilediff = self.create_filediff(interdiffset) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': filediff.pk, + 'interfilediff_id': interfilediff.pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This field cannot be specified with interfilediff_id.', + ], + 'interfilediff_id': [ + 'This field cannot be specified with base_filediff_id.', + ], + }) + + @webapi_test_template + def test_post_with_base_filediff_newer(self): + """Testing the POST API with base_filediff_id newer than + filediff_id + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffset = self.create_diffset(review_request) + + commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + filediffs = [ + self.create_filediff(diffset=diffset, commit=commit) + for commit in commits + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[0].pk, + 'base_filediff_id': filediffs[1].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This is not a valid base filediff ID.', + ], + }) + + self.assertFalse(FileDiff.get_ancestors.called) + + @webapi_test_template + def test_post_with_base_filediff_same_commit(self): + """Testing the POST API with base_filediff_id belonging to a + different FileDiff in the same commit + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset) + filediffs = [ + self.create_filediff(diffset=diffset, commit=commit), + self.create_filediff(diffset=diffset, commit=commit), + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[0].pk, + 'base_filediff_id': filediffs[1].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This is not a valid base filediff ID.', + ] + }) + + self.assertFalse(FileDiff.get_ancestors.called) + + @webapi_test_template + def test_post_with_base_filediff_not_exists(self): + """Testing the POST API with base_filediff_id set to a + non-existant ID + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request, filediff = self._create_diff_review_request( + with_history=True) + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediff.pk, + 'base_filediff_id': 12321, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This is not a valid base filediff ID.', + ] + }) + + self.assertFalse(FileDiff.get_ancestors.called) + + @webapi_test_template + def test_post_with_base_filediff_outside_diffset(self): + """Testing the POST API with base_filediff_id belonging to a + different DiffSet + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffsets = [ + self.create_diffset(review_request, revision=1), + self.create_diffset(review_request, revision=2) + ] + + commits = [ + self.create_diffcommit(diffset=diffsets[0], commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffsets[1], commit_id='r2', + parent_id='r1'), + ] + + filediffs = [ + self.create_filediff(diffset=diffset, commit=commit) + for diffset, commit in zip(diffsets, commits) + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[1].pk, + 'base_filediff_id': filediffs[0].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This is not a valid base filediff ID.', + ], + }) + + self.assertFalse(FileDiff.get_ancestors.called) + + @webapi_test_template + def test_post_with_base_filediff_outside_history(self): + """Testing the POST API with base_filediff_id not belonging to + the FileDiff's set of ancestors + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffset = self.create_diffset(review_request) + + commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + filediffs = [ + self.create_filediff(diffset=diffset, commit=commits[0], + source_file='/foo', dest_file='/foo'), + self.create_filediff(diffset=diffset, commit=commits[1], + source_file='/bar', dest_file='/bar'), + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[1].pk, + 'base_filediff_id': filediffs[0].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'base_filediff_id': [ + 'This is not a valid base filediff ID.', + ], + }) + + self.assertTrue(FileDiff.get_ancestors.called) + + @webapi_test_template + def test_post_with_base_filediff_ancestor(self): + """Testing the POST API with base_filediff_id belonging to + the FileDiff's set of ancestors + """ + self.spy_on(FileDiff.get_ancestors) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + publish=True) + + diffset = self.create_diffset(review_request) + + commits = [ + self.create_diffcommit(diffset=diffset, commit_id='r1', + parent_id='r0'), + self.create_diffcommit(diffset=diffset, commit_id='r2', + parent_id='r1'), + ] + + filediffs = [ + self.create_filediff(diffset=diffset, commit=commits[0], + source_revision='123', dest_detail='124'), + self.create_filediff(diffset=diffset, commit=commits[1], + source_revision='124', dest_detail='125'), + ] + review = self.create_review(review_request, user=self.user) + + rsp = self.api_post( + get_review_diff_comment_list_url(review), + { + 'filediff_id': filediffs[1].pk, + 'base_filediff_id': filediffs[0].pk, + 'issue_opened': True, + 'first_line': 1, + 'num_lines': 5, + 'text': 'foo', + }, + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + comment = Comment.objects.get(pk=item_rsp['id']) + + self.compare_item(item_rsp, comment) + + self.assertTrue(FileDiff.get_ancestors.called) @six.add_metaclass(BasicTestsMetaclass) class ResourceItemTests(CommentItemMixin, ReviewRequestChildItemMixin, @@ -267,6 +843,8 @@ class ResourceItemTests(CommentItemMixin, ReviewRequestChildItemMixin, sample_api_url = 'review-requests//reviews//diff-comments/' resource = resources.review_diff_comment + compare_item = _compare_item + def setup_review_request_child_test(self, review_request): if not review_request.repository_id: # The group tests don't create a repository by default. @@ -281,19 +859,6 @@ class ResourceItemTests(CommentItemMixin, ReviewRequestChildItemMixin, return (get_review_diff_comment_item_url(review, comment.pk), review_diff_comment_item_mimetype) - def compare_item(self, item_rsp, comment): - self.assertEqual(item_rsp['id'], comment.pk) - self.assertEqual(item_rsp['text'], comment.text) - self.assertEqual(item_rsp['issue_opened'], comment.issue_opened) - self.assertEqual(item_rsp['first_line'], comment.first_line) - self.assertEqual(item_rsp['num_lines'], comment.num_lines) - self.assertEqual(item_rsp['extra_data'], comment.extra_data) - - if comment.rich_text: - self.assertEqual(item_rsp['text_type'], 'markdown') - else: - self.assertEqual(item_rsp['text_type'], 'plain') - # # HTTP DELETE tests # @@ -365,6 +930,58 @@ class ResourceItemTests(CommentItemMixin, ReviewRequestChildItemMixin, get_review_diff_comment_item_url(review, comment.id), check_etags=True) + @webapi_test_template + def test_get_with_dvcs_disabled(self): + """Testing the GET API when DVCS feature disabled""" + with override_feature_check(dvcs_feature.feature_id, enabled=False): + review_request = self.create_review_request( + create_repository=True, + publish=True) + + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset) + + filediff = self.create_filediff(diffset=diffset, commit=commit) + + review = self.create_review(review_request, user=self.user) + comment = self.create_diff_comment(review, filediff) + + rsp = self.api_get( + get_review_diff_comment_item_url(review, comment.pk), + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + self.compare_item(item_rsp, comment) + + @webapi_test_template + def test_get_with_dvcs_enabled(self): + """Testing the GET API when DVCS feature enabled""" + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + publish=True) + + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset) + + filediff = self.create_filediff(diffset=diffset, commit=commit) + + review = self.create_review(review_request, user=self.user) + comment = self.create_diff_comment(review, filediff) + + rsp = self.api_get( + get_review_diff_comment_item_url(review, comment.pk), + expected_mimetype=review_diff_comment_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff_comment', rsp) + + item_rsp = rsp['diff_comment'] + self.compare_item(item_rsp, comment) + # # HTTP PUT tests #