diff --git a/reviewboard/diffviewer/parser.py b/reviewboard/diffviewer/parser.py index fbe0234e339b18a74c418171fc30fa76cf255c0c..882c81bd5c5f7ba0fea6a4088722f48a483c14b4 100644 --- a/reviewboard/diffviewer/parser.py +++ b/reviewboard/diffviewer/parser.py @@ -326,12 +326,20 @@ class DiffParser(object): "found in the diff header", linenum) - def raw_diff(self, diffset): - """Returns a raw diff as a string. + def raw_diff(self, collection): + """Return a raw diff as a string. - The returned diff as composed of all FileDiffs in the provided diffset. + Args: + collection (reviewboard.diffviewer.models.mixins. + FileDiffCollectionMixin) + The model whose :py:class:`FileDiffs + ` are to be rendered. + + Returns: + bytes: + The diff composed of all the component FileDiffs. """ - return b''.join([filediff.diff for filediff in diffset.files.all()]) + return b''.join([filediff.diff for filediff in collection.files.all()]) def get_orig_commit_id(self): """Returns the commit ID of the original revision for the diff. diff --git a/reviewboard/reviews/forms.py b/reviewboard/reviews/forms.py index 0f3ee9124efc6088dfe65f15982e0e90d6cf068a..c2c60bdfc9a3a21210371cb999aed3d79e876360 100644 --- a/reviewboard/reviews/forms.py +++ b/reviewboard/reviews/forms.py @@ -119,6 +119,49 @@ class GroupForm(forms.ModelForm): fields = '__all__' +class UploadCommitForm(diffviewer_forms.UploadCommitForm): + """A specialized UploadCommitForm for interacting with review requests.""" + + def __init__(self, review_request, *args, **kwargs): + """Initialize the form. + + Args: + review_request (reviewboard.reviews.models.review_request. + ReviewRequest): + The review request that the uploaded commit will be attached + to. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + """ + super(UploadCommitForm, self).__init__(*args, **kwargs) + + self.review_request = review_request + + def clean(self): + """Clean the form. + + Returns: + dict: + The cleaned form data. + + Raises: + django.core.exceptions.ValidationError: + The form failed validation. + """ + super(UploadCommitForm, self).clean() + + if not self.review_request.created_with_history: + raise ValidationError( + 'This review request was created without commit history ' + 'support.') + + return self.cleaned_data + + class UploadDiffForm(diffviewer_forms.UploadDiffForm): """A specialized UploadDiffForm for interacting with review requests.""" @@ -138,10 +181,11 @@ class UploadDiffForm(diffviewer_forms.UploadDiffForm): **kwargs (dict): Additional keyword arguments. """ - super(UploadDiffForm, self).__init__(review_request.repository, - request, - *args, - **kwargs) + super(UploadDiffForm, self).__init__( + repository=review_request.repository, + request=request, + *args, + **kwargs) self.review_request = review_request if ('basedir' in self.fields and diff --git a/reviewboard/reviews/models/review_request_draft.py b/reviewboard/reviews/models/review_request_draft.py index 39aae8b630a37953f84edfe5ecd0a84033925c0b..abf3db7731ef3d7058716084cd1cbe2aa7db0587 100644 --- a/reviewboard/reviews/models/review_request_draft.py +++ b/reviewboard/reviews/models/review_request_draft.py @@ -314,6 +314,12 @@ class ReviewRequestDraft(BaseReviewRequestDetails): raise PublishError( ugettext('The draft must have a description.')) + if (review_request.created_with_history and + self.diffset and + self.diffset.commit_count == 0): + raise PublishError( + ugettext('There are no commits attached to the diff.')) + if self.diffset: self.diffset.history = review_request.diffset_history self.diffset.timestamp = timestamp diff --git a/reviewboard/reviews/tests/test_review_request_draft.py b/reviewboard/reviews/tests/test_review_request_draft.py index 0bd8902f58079554d4d76c262800f4de109b5e3e..bbe06928d37ee2e44ed047b47befd5741a71ef92 100644 --- a/reviewboard/reviews/tests/test_review_request_draft.py +++ b/reviewboard/reviews/tests/test_review_request_draft.py @@ -601,3 +601,21 @@ class PostCommitTests(SpyAgency, TestCase): with self.assertRaisesMessage(PublishError, error_message): draft.publish() + + def test_publish_with_history_no_commits_in_diffset(self): + """Testing ReviewRequestDraft.publish when the diffset has no commits + """ + review_request = self.create_review_request(create_with_history=True, + create_repository=True) + self.create_diffset(review_request, draft=True) + + draft = review_request.get_draft() + draft.target_people = [self.user] + draft.summary = 'Summary' + draft.description = 'Description' + draft.save() + + error_msg = 'There are no commits attached to the diff.' + + with self.assertRaisesMessage(PublishError, error_msg): + draft.publish() diff --git a/reviewboard/webapi/resources/__init__.py b/reviewboard/webapi/resources/__init__.py index c013df5ffe995ce13b6a089c549a621ef1f4dd0d..c434125c5eb2f315a2b15a219828ac14452a3438 100644 --- a/reviewboard/webapi/resources/__init__.py +++ b/reviewboard/webapi/resources/__init__.py @@ -7,8 +7,8 @@ from djblets.webapi.resources.registry import (ResourcesRegistry, from oauth2_provider.models import AccessToken from reviewboard.attachments.models import FileAttachment -from reviewboard.diffviewer.models import DiffSet, FileDiff from reviewboard.changedescs.models import ChangeDescription +from reviewboard.diffviewer.models import DiffCommit, DiffSet, FileDiff from reviewboard.hostingsvcs.models import HostingServiceAccount from reviewboard.notifications.models import WebHookTarget from reviewboard.oauth.models import Application @@ -54,6 +54,11 @@ class Resources(ResourcesRegistry): self.review_reply_diff_comment or self.review_diff_comment)) register_resource_for_model(DefaultReviewer, self.default_reviewer) + register_resource_for_model( + DiffCommit, + lambda obj: (self.diffcommit + if obj.diffset.history_id + else self.draft_diffcommit)) register_resource_for_model( DiffSet, lambda obj: obj.history_id and self.diff or self.draft_diff) diff --git a/reviewboard/webapi/resources/diff.py b/reviewboard/webapi/resources/diff.py index 6073d2886e21e655a452e89a908c70b8eab6f50c..a7e0d34c026a7e0e9dce92db40362dbb287a8378 100644 --- a/reviewboard/webapi/resources/diff.py +++ b/reviewboard/webapi/resources/diff.py @@ -20,6 +20,7 @@ from djblets.webapi.fields import (DateTimeFieldType, StringFieldType) from reviewboard.diffviewer.errors import DiffTooBigError, EmptyDiffError +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.diffviewer.models import DiffSet from reviewboard.reviews.forms import UploadDiffForm from reviewboard.reviews.models import ReviewRequest, ReviewRequestDraft @@ -92,6 +93,7 @@ class DiffResource(WebAPIResource): }, } item_child_resources = [ + resources.diffcommit, resources.filediff, ] @@ -199,13 +201,11 @@ class DiffResource(WebAPIResource): REPO_FILE_NOT_FOUND, INVALID_FORM_DATA, INVALID_ATTRIBUTE, DIFF_EMPTY, DIFF_TOO_BIG) @webapi_request_fields( - required={ + optional={ 'path': { 'type': FileFieldType, 'description': 'The main diff to upload.', }, - }, - optional={ 'basedir': { 'type': StringFieldType, 'description': 'The base directory that will prepended to ' @@ -231,7 +231,8 @@ class DiffResource(WebAPIResource): }, allow_unknown=True ) - def create(self, request, extra_fields={}, *args, **kwargs): + def create(self, request, extra_fields={}, local_site=None, *args, + **kwargs): """Creates a new diff by parsing an uploaded diff file. This will implicitly create the new Review Request draft, which can @@ -286,48 +287,62 @@ class DiffResource(WebAPIResource): 'only, with no repository.', } elif review_request.created_with_history: - return INVALID_ATTRIBUTE, { - 'reason': 'This review request was created with support for ' - 'multiple commits. A regular diff cannot be ' - 'uploaded.', - } + assert dvcs_feature.is_enabled(request=request) + + if 'path' in request.FILES: + return INVALID_FORM_DATA, { + 'reason': ( + 'This review request was created with support for ' + 'multiple commits.\n' + '\n' + 'Create an empty diff revision and upload commits to ' + 'that instead.' + ), + } - form_data = request.POST.copy() - form = UploadDiffForm(review_request, - data=form_data, - files=request.FILES, - request=request) + diffset = DiffSet.objects.create_empty( + repository=review_request.repository, + base_commit_id=request.POST.get('base_commit_id')) + diffset.update_revision_from_history( + review_request.diffset_history) + diffset.save(update_fields=('revision',)) + else: + form_data = request.POST.copy() + form = UploadDiffForm(review_request, + data=form_data, + files=request.FILES, + request=request) + + if not form.is_valid(): + return INVALID_FORM_DATA, { + 'fields': self._get_form_errors(form), + } - if not form.is_valid(): - return INVALID_FORM_DATA, { - 'fields': self._get_form_errors(form), - } + try: + diffset = form.create() + except FileNotFoundError as e: + return REPO_FILE_NOT_FOUND, { + 'file': e.path, + 'revision': six.text_type(e.revision) + } + except EmptyDiffError as e: + return DIFF_EMPTY + except DiffTooBigError as e: + return DIFF_TOO_BIG, { + 'reason': six.text_type(e), + 'max_size': e.max_diff_size, + } + except Exception as e: + # This could be very wrong, but at least they'll see the error. + # We probably want a new error type for this. + logging.error("Error uploading new diff: %s", e, exc_info=1, + request=request) - try: - diffset = form.create() - except FileNotFoundError as e: - return REPO_FILE_NOT_FOUND, { - 'file': e.path, - 'revision': six.text_type(e.revision) - } - except EmptyDiffError as e: - return DIFF_EMPTY - except DiffTooBigError as e: - return DIFF_TOO_BIG, { - 'reason': six.text_type(e), - 'max_size': e.max_diff_size, - } - except Exception as e: - # This could be very wrong, but at least they'll see the error. - # We probably want a new error type for this. - logging.error("Error uploading new diff: %s", e, exc_info=1, - request=request) - - return INVALID_FORM_DATA, { - 'fields': { - 'path': [six.text_type(e)] + return INVALID_FORM_DATA, { + 'fields': { + 'path': [six.text_type(e)] + } } - } discarded_diffset = None @@ -407,5 +422,42 @@ class DiffResource(WebAPIResource): self.item_result_key: diffset, } + def get_links(self, child_resources=[], obj=None, request=None, + *args, **kwargs): + """Return the links for the resource. + + If the DVCS feature is disabled, links to resources that require the + feature will not be included. + + Args: + child_resource (list of reviewboard.webapi.base.WebAPIResource): + The list of child resources for which links are to be + serialized. + + obj (reviewboard.diffviewer.models.diffset.DiffSet, optional): + The object whose links are being serialized. + + request (django.http.HttpRequest, optional): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + A dictionary of serialized links for the resource. + """ + if (obj is not None and + not dvcs_feature.is_enabled(request=request) and + resources.diffcommit in child_resources): + child_resources = list(child_resources) + child_resources.remove(resources.diffcommit) + + return super(DiffResource, self).get_links( + child_resources, obj=obj, request=request, *args, **kwargs) + diff_resource = DiffResource() diff --git a/reviewboard/webapi/resources/diffcommit.py b/reviewboard/webapi/resources/diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..77f0154a3020cac56c8731bf5c5c8d19c92dcbbd --- /dev/null +++ b/reviewboard/webapi/resources/diffcommit.py @@ -0,0 +1,394 @@ +"""Resource representing the commits in a multi-commit review request.""" + +from __future__ import unicode_literals + +from django.core.exceptions import ObjectDoesNotExist +from django.http import HttpResponse +from django.utils.six.moves.urllib.parse import urlencode +from djblets.util.decorators import augment_method_from +from djblets.util.http import get_http_requested_mimetype, set_last_modified +from djblets.webapi.decorators import webapi_request_fields +from djblets.webapi.errors import (DOES_NOT_EXIST, NOT_LOGGED_IN, + PERMISSION_DENIED) +from djblets.webapi.fields import (DateTimeFieldType, DictFieldType, + IntFieldType, StringFieldType) + +from reviewboard.diffviewer.features import dvcs_feature +from reviewboard.diffviewer.models import DiffCommit, DiffSet +from reviewboard.diffviewer.validators import COMMIT_ID_LENGTH +from reviewboard.webapi.base import ImportExtraDataError, WebAPIResource +from reviewboard.webapi.decorators import (webapi_check_local_site, + webapi_check_login_required, + webapi_login_required, + webapi_response_errors) +from reviewboard.webapi.resources import resources + + +class DiffCommitResource(WebAPIResource): + """Provides information on a collection of commits in a review request. + + Each diff commit resource contains individual per-file diffs as child + resources, as well as metadata to reproduce the actual commits in a + version control system. + """ + + added_in = '4.0' + model = DiffCommit + name = 'commit' + + model_parent_key = 'diffset' + model_object_key = 'commit_id' + + uri_object_key = 'commit_id' + uri_object_key_regex = r'[A-Za-z0-9]{1,%s}' % COMMIT_ID_LENGTH + + allowed_methods = ('GET', 'PUT') + + required_features = [dvcs_feature] + + allowed_mimetypes = WebAPIResource.allowed_mimetypes + [ + {'item': 'text/x-patch'}, + ] + + fields = { + 'id': { + 'type': IntFieldType, + 'description': 'The numeric ID of the commit resource.', + }, + 'author_name': { + 'type': StringFieldType, + 'description': 'The name of the author of this commit.', + }, + 'author_date': { + 'type': DateTimeFieldType, + 'description': 'The date and time this commit was authored in ISO ' + '8601 format (YYYY-MM-DD HH:MM:SS+ZZZZ).', + }, + 'author_email': { + 'type': StringFieldType, + 'description': 'The e-mail address of the author of this commit.', + }, + 'commit_id': { + 'type': StringFieldType, + 'description': 'The ID of this commit.', + }, + 'committer_name': { + 'type': StringFieldType, + 'description': 'The name of the the committer of this commit, if ' + 'applicable.', + }, + 'committer_date': { + 'type': StringFieldType, + 'description': 'The date and time this commit was committed in ' + 'ISO 8601 format (YYYY-MM-DD HH:MM:SS+ZZZZ).', + }, + 'committer_email': { + 'type': StringFieldType, + 'description': 'The e-mail address of the committer of this ' + 'commit.', + }, + 'commit_message': { + 'type': StringFieldType, + 'description': 'The commit message.', + }, + 'extra_data': { + 'type': DictFieldType, + 'description': 'Extra data as part of the commit. This can be set ' + 'by the API or extensions.', + }, + 'filename': { + 'type': StringFieldType, + 'description': 'The name of the corresponding diff.', + }, + 'parent_id': { + 'type': StringFieldType, + 'description': 'The ID of the parent commit.', + }, + } + + def get_queryset(self, request, *args, **kwargs): + """Return the queryset for the available commits. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + django.db.models.query.QuerySet: + The queryset of all available commits. + """ + try: + diffset = resources.diff.get_object(request, *args, **kwargs) + except DiffSet.DoesNotExist: + return self.model.objects.none() + + return self.model.objects.filter(diffset=diffset) + + def has_access_permissions(self, request, commit, *args, **kwargs): + """Return whether or not the user has access permissions to the commit. + + A user has access permissions for a commit if they have permission to + access the parent review request. + + Args: + request (django.http.HttpRequest): + The current HTTP request. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to access the commit. + """ + review_request = resources.review_request.get_object(request, *args, + **kwargs) + return review_request.is_accessible_by(request.user) + + def has_list_access_permissions(self, request, *args, **kwargs): + """Return whether the user has access permissions to the list resource. + + A user has list access permissions if they have premission to access + the parent review request. + + Args: + request (django.http.HttpRequest): + The current HTTP request. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to access the list resource. + """ + review_request = resources.review_request.get_object(request, *args, + **kwargs) + return review_request.is_accessible_by(request.user) + + def has_modify_permissions(self, request, obj, *args, **kwargs): + """Return whether the user has access permissions to modify the object. + + A user has modify permissions for a commit if they have permission to + modify the parent review request. + + Args: + request (django.http.HttpRequest): + The current HTTP request. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to modify the object. + """ + review_request = resources.review_request.get_object(request, *args, + **kwargs) + return review_request.is_mutable_by(request.user) + + @webapi_check_login_required + @webapi_check_local_site + @webapi_response_errors(DOES_NOT_EXIST) + @augment_method_from(WebAPIResource) + def get_list(self, request, *args, **kwargs): + """Return the list of commits.""" + + @webapi_check_login_required + @webapi_check_local_site + @webapi_response_errors(DOES_NOT_EXIST) + def get(self, request, *args, **kwargs): + """Return information about a commit. + + If the :mimetype:`text/x-patch` mimetype is requested, the contents of + the patch will be returned. + + Otherwise, metadata about the commit (such as author name, author date, + etc.) will be returned. + """ + mimetype = get_http_requested_mimetype( + request, + [mimetype['item'] for mimetype in self.allowed_mimetypes]) + + if mimetype != 'text/x-patch': + return super(DiffCommitResource, self).get(request, *args, + **kwargs) + + try: + review_request = resources.review_request.get_object( + request, *args, **kwargs) + commit = self.get_object(request, *args, **kwargs) + except ObjectDoesNotExist: + return DOES_NOT_EXIST + + if not self.has_access_permissions(request, commit, *args, **kwargs): + return self.get_no_access_error(request) + + tool = review_request.repository.get_scmtool() + data = tool.get_parser('').raw_diff(commit) + + rsp = HttpResponse(data, content_type=mimetype) + rsp['Content-Disposition'] = ('inline; filename=%s.patch' + % commit.commit_id) + + set_last_modified(rsp, commit.last_modified) + return rsp + + @webapi_login_required + @webapi_check_local_site + @webapi_response_errors(DOES_NOT_EXIST, NOT_LOGGED_IN, PERMISSION_DENIED) + @webapi_request_fields(allow_unknown=True) + def update(self, request, extra_fields=None, *args, **kwargs): + """Update a commit. + + This is used solely for modifying the extra data on a commit. The + contents of a commit cannot be modified. + + Extra data can be stored for later lookup. See + :ref:`webapi2.0-extra-data` for more information. + """ + try: + commit = self.get_object(request, *args, **kwargs) + except DiffCommit.DoesNotExist: + return DOES_NOT_EXIST + + if not self.has_modify_permissions(request, commit, *args, **kwargs): + return self.get_no_access_error(request) + + if extra_fields: + try: + self.import_extra_data(commit, commit.extra_data, extra_fields) + except ImportExtraDataError as e: + return e.error_payload + + commit.save(update_fields=['extra_data']) + + return 200, { + self.item_result_key: commit, + } + + def _get_files_link(self, commit, request, diff_resource, + filediff_resource, files_key, *args, **kwargs): + """Return the link for the files. + + As an alternative to having a per-commit files resources (and draft + resource equivalent), this method generates link to the + :py:class:`~reviewboard.webapi.resources.filediff.FileDiffResource` + (or :py:class:`~reviewboard.webapi.resources.draft_filediff. + DraftFileDiffResource` for commits on a review request draft) filtered + specifically for this commit. + + Args: + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The commit to retrieve the link for. + + request (django.http.HttpRequest): + The current HTTP request. + + diff_resource (reviewboard.webapi.resources.diff.DiffResource): + Either the diff resource (if this is the diff commit resource) + or the draft diff resource (if this is the draft diff commit + resource). + + filediff_resource (reviewboard.webapi.resources.filediff. + FileDiffResource): + Either the filedif resource (if this is the diff commit + resource) or the draft filediff resource (if this is the draft + diff commit resource). + + files_key (unicode): + The key that maps to the files link in the ``links`` field of + the ``diff_resource``. + + *args (tuple): + Additional positional argument. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + The link information for the file resource that contains only the + :py:class:`FileDiffs + ` of the + requested commit. + """ + files_link = diff_resource.get_links( + [filediff_resource], + commit.diffset, + request, + *args, + **kwargs)[files_key] + + files_link['href'] = '%s?%s' % ( + files_link['href'], + urlencode({'commit-id': commit.commit_id}), + ) + + return files_link + + def get_related_links(self, obj=None, request=None, *args, **kwargs): + """Return the related links for the resource. + + If this is for an item resource, this will return links for all the + associated + :py:class:`~reviewboard.diffviewer.models.filediff.FileDiffs`. + + Args: + obj (reviewboard.diffviewer.models.diffcommit.DiffCommit, + optional): + The DiffCommit to get links for. + + request (django.http.HttpRequest): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + The related links. + """ + links = {} + + if obj is not None and request: + links['files'] = self._get_files_link( + commit=obj, + request=request, + diff_resource=resources.diff, + filediff_resource=resources.filediff, + files_key='files', + *args, + **kwargs) + + return links + + +diffcommit_resource = DiffCommitResource() diff --git a/reviewboard/webapi/resources/draft_diff.py b/reviewboard/webapi/resources/draft_diff.py index 030fb320c750ed24aadb1ef39ded88002a5dedc3..f70ae0dbdff47aefbaa5fb7d365097f5c4abc0f0 100644 --- a/reviewboard/webapi/resources/draft_diff.py +++ b/reviewboard/webapi/resources/draft_diff.py @@ -29,6 +29,7 @@ class DraftDiffResource(DiffResource): mimetype_item_resource_name = 'diff' item_child_resources = [ + resources.draft_diffcommit, resources.draft_filediff, ] diff --git a/reviewboard/webapi/resources/draft_diffcommit.py b/reviewboard/webapi/resources/draft_diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..2e7421bb03035df601431a458cf0dfc3e5f50021 --- /dev/null +++ b/reviewboard/webapi/resources/draft_diffcommit.py @@ -0,0 +1,369 @@ +"""Resources representing commits on a multi-commit review request draft.""" + +from __future__ import unicode_literals + +import logging + +from django.utils import six +from djblets.util.decorators import augment_method_from +from djblets.webapi.decorators import webapi_request_fields +from djblets.webapi.errors import (DOES_NOT_EXIST, INVALID_ATTRIBUTE, + INVALID_FORM_DATA) +from djblets.webapi.fields import (DateTimeFieldType, + FileFieldType, + StringFieldType) + +from reviewboard.diffviewer.errors import DiffTooBigError, EmptyDiffError +from reviewboard.reviews.forms import UploadCommitForm +from reviewboard.reviews.models import ReviewRequest, ReviewRequestDraft +from reviewboard.scmtools.core import FileNotFoundError +from reviewboard.webapi.decorators import (webapi_check_local_site, + webapi_login_required, + webapi_response_errors) +from reviewboard.webapi.errors import (DIFF_EMPTY, + DIFF_TOO_BIG, + REPO_FILE_NOT_FOUND) +from reviewboard.webapi.resources import resources +from reviewboard.webapi.resources.diffcommit import DiffCommitResource + + +logger = logging.getLogger(__name__) + + +class DraftDiffCommitResource(DiffCommitResource): + """Provides information on pending draft commits for a review request. + + POSTing to this resource will update a review request draft with the + provided diff. + """ + + name = 'draft_commit' + model_parent_key = 'diffset' + + allowed_methods = ('GET', 'POST', 'PUT') + + def get_queryset(self, request, *args, **kwargs): + """Return a QuerySet limited to the review request draft. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + django.db.models.query.QuerySet: + The generated QuerySet. + """ + try: + draft = resources.review_request_draft.get_object(request, *args, + **kwargs) + except ReviewRequestDraft.DoesNotExist: + return self.model.objects.none() + + if draft.diffset_id is None: + return self.model.objects.none() + + return self.model.objects.filter(diffset__pk=draft.diffset_id) + + def has_access_permissions(self, request, commit, *args, **kwargs): + """Return whether or not the user has access permissions to the commit. + + A user has access permissions for a commit if they have permission to + access the review request draft. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to access the commit. + """ + draft = resources.review_request_draft.get_object(request, *args, + **kwargs) + return draft.is_accessible_by(request.user) + + def has_list_access_permissions(self, request, *args, **kwargs): + """Return whether the user has access permissions to the list resource. + + A user has list access permissions if they have premission to access + the review request draft. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to access the list resource. + """ + draft = resources.review_request_draft.get_object(request, *args, + **kwargs) + return draft.is_accessible_by(request.user) + + def has_modify_permissions(self, request, commit, *args, **kwargs): + """Return whether the user has access permissions to modify the object. + + A user has modify permissions for a commit if they have permission to + modify the review request draft. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The object to check access permissions for. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + bool: + Whether or not the user has permission to modify the object. + """ + draft = resources.review_request_draft.get_object(request, *args, + **kwargs) + return draft.is_mutable_by(request.user) + + @webapi_login_required + @augment_method_from(DiffCommitResource) + def get(self, *args, **kwargs): + pass + + @webapi_login_required + @augment_method_from(DiffCommitResource) + def get_list(self, *args, **kwargs): + pass + + @webapi_login_required + @webapi_check_local_site + @webapi_response_errors(DIFF_EMPTY, DIFF_TOO_BIG, DOES_NOT_EXIST, + INVALID_ATTRIBUTE, INVALID_FORM_DATA, + REPO_FILE_NOT_FOUND) + @webapi_request_fields( + required={ + 'diff': { + 'type': FileFieldType, + 'description': 'The corresponding diff for this commit.', + }, + 'commit_id': { + 'type': StringFieldType, + 'description': 'The ID of this commit.', + }, + 'author_name': { + 'type': StringFieldType, + 'description': 'The name of the author of this commit.', + }, + 'author_date': { + 'type': DateTimeFieldType, + 'description': 'The date and time this commit was authored in ' + 'ISO 8601 format (YYYY-MM-DD HH:MM:SS+ZZZZ).', + }, + 'author_email': { + 'type': StringFieldType, + 'description': 'The e-mail address of the author of this ' + 'commit.', + }, + 'commit_message': { + 'type': StringFieldType, + 'description': 'The commit message.', + }, + }, + optional={ + 'committer_name': { + 'type': StringFieldType, + 'description': ( + 'The name of the the committer of this commit, if ' + 'applicable.\n' + '\n' + 'If this field is specified, the "committer_date" and ' + '"committer_email" fields must also be specified.' + ), + }, + 'committer_date': { + 'type': StringFieldType, + 'description': ( + 'The date and time this commit was committed in ISO 8601 ' + 'format (YYYY-MM-DD HH:MM:SS+ZZZZ).\n' + '\n' + 'If this field is specified, the "committer_name" and ' + '"committer_email" fields must also be specified.' + ), + }, + 'committer_email': { + 'type': StringFieldType, + 'description': ( + 'The e-mail address of the committer of this commit.\n' + '\n' + 'If this field is specified, the "committer_name" and ' + '"committer_date" fields must also be specified.' + ), + }, + 'parent_diff': { + 'type': FileFieldType, + 'description': 'The optional parent diff to upload.', + }, + }, + allow_unknown=True + ) + @webapi_check_local_site + def create(self, request, extra_fields=None, *args, **kwargs): + """Create a new commit. + + A draft must exist and the review request must be created with history + support in order to post to this resource. + """ + try: + review_request = resources.review_request.get_object( + request, *args, **kwargs) + except ReviewRequest.DoesNotExist: + return DOES_NOT_EXIST + + if not self.has_modify_permissions(request, review_request, *args, + **kwargs): + return self.get_no_access_error(request) + + if review_request.repository is None: + return INVALID_ATTRIBUTE, { + 'reason': 'This review request was created as attachments-' + 'only, with no repository.', + } + elif not review_request.created_with_history: + reverse_kwargs = { + 'review_request_id': kwargs['review_request_id'], + } + + if request.local_site: + reverse_kwargs['local_site_name'] = request.local_site.name + + return INVALID_ATTRIBUTE, { + 'reason': ( + 'This review request was not created with support for ' + 'multiple commits.\n\n' + 'Use the %(name)s resource to upload diffs instead. See ' + 'the %(name)s link on the parent resource for the URL.' + % { + 'name': resources.draft_diff.name, + } + ), + } + + try: + draft = review_request.draft.get() + except ReviewRequestDraft.DoesNotExist: + return DOES_NOT_EXIST, { + 'reason': 'Review request draft does not exist.', + } + + diffset = draft.diffset + + if diffset is None: + return DOES_NOT_EXIST, { + 'reason': 'An empty diff must be created first.', + } + + form = UploadCommitForm( + review_request=review_request, + diffset=diffset, + request=request, + data=request.POST.copy(), + files=request.FILES) + + if not form.is_valid(): + return INVALID_FORM_DATA, { + 'fields': self._get_form_errors(form), + } + + try: + commit = form.create() + except FileNotFoundError as e: + return REPO_FILE_NOT_FOUND, { + 'file': e.path, + 'revision': six.text_type(e.revision), + } + except EmptyDiffError as e: + return DIFF_EMPTY + except DiffTooBigError as e: + return DIFF_TOO_BIG, { + 'reason': six.text_type(e), + 'max_size': e.max_diff_size, + } + except Exception as e: + logger.exception('Error uploading new commit: %s', e, + request=request) + + return INVALID_FORM_DATA, { + 'fields': { + 'path': [six.text_type(e)], + }, + } + + return 201, { + self.item_result_key: commit, + } + + def get_related_links(self, obj, request=None, *args, **kwargs): + """Return the related links for the resource. + + If this is for an item resource, this will return links for all the + associated FileDiffs. + + Args: + obj (reviewboard.diffviewer.models.DiffCommit, optional): + The DiffCommit to get links for. + + request (django.http.HttpRequest): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + The related links. + """ + links = {} + + if obj and request: + links['draft_files'] = self._get_files_link( + commit=obj, + request=request, + diff_resource=resources.draft_diff, + filediff_resource=resources.draft_filediff, + files_key='draft_files', + *args, + **kwargs) + + return links + + +draft_diffcommit_resource = DraftDiffCommitResource() diff --git a/reviewboard/webapi/resources/review_request_draft.py b/reviewboard/webapi/resources/review_request_draft.py index 323557b39d2c313714a5e586c61a47ecf27bb655..b61ee5227ac1301853b84d1a8c87c3a0aa6b642f 100644 --- a/reviewboard/webapi/resources/review_request_draft.py +++ b/reviewboard/webapi/resources/review_request_draft.py @@ -24,6 +24,7 @@ from djblets.webapi.fields import (BooleanFieldType, ResourceListFieldType, StringFieldType) +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.reviews.builtin_fields import BuiltinFieldMixin from reviewboard.reviews.errors import NotModifiedError, PublishError from reviewboard.reviews.fields import (get_review_request_fields, @@ -585,6 +586,44 @@ class ReviewRequestDraftResource(MarkdownFieldsMixin, WebAPIResource): """Returns the current draft of a review request.""" pass + def get_links(self, child_resources=[], obj=None, request=None, + *args, **kwargs): + """Return the links for the resource. + + This method will filter out the draft diffcommit resource when the DVCS + feature is disabled. + + Args: + child_resources (list of djblets.webapi.resources.base. + WebAPIResource): + The child resources for which links will be serialized. + + review_request_id (unicode): + A string represenation of the ID of the review request for + which links are being returned. + + request (django.http.HttpRequest): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + + Returns: + dict: + A dictionary of the links for the resource. + """ + if (obj is not None and + not dvcs_feature.is_enabled() and + resources.draft_diffcommit in child_resources): + child_resources = list(child_resources) + child_resources.remove(resources.draft_diffcommit) + + return super(ReviewRequestDraftResource, self).get_links( + child_resources, obj=obj, request=request, *args, **kwargs) + def _set_draft_field_data(self, draft, field_name, data, local_site_name, request): """Sets a field on a draft. diff --git a/reviewboard/webapi/tests/mimetypes.py b/reviewboard/webapi/tests/mimetypes.py index 044f2a3fa647c0080b4fd87417866038e58851ec..34a0779f257fd7903bc3bab9a2601e37e012492b 100644 --- a/reviewboard/webapi/tests/mimetypes.py +++ b/reviewboard/webapi/tests/mimetypes.py @@ -27,10 +27,18 @@ diff_list_mimetype = _build_mimetype('diffs') diff_item_mimetype = _build_mimetype('diff') +diffcommit_list_mimetype = _build_mimetype('commits') +diffcommit_item_mimetype = _build_mimetype('commit') + + diff_file_attachment_list_mimetype = _build_mimetype('diff-file-attachments') diff_file_attachment_item_mimetype = _build_mimetype('diff-file-attachment') +draft_diffcommit_list_mimetype = _build_mimetype('draft-commits') +draft_diffcommit_item_mimetype = _build_mimetype('draft-commit') + + draft_file_attachment_list_mimetype = _build_mimetype('draft-file-attachments') draft_file_attachment_item_mimetype = _build_mimetype('draft-file-attachment') diff --git a/reviewboard/webapi/tests/mixins.py b/reviewboard/webapi/tests/mixins.py index 083d7d5e7f5ac7e1156194adde3f56210db1a9df..75e326239b6c4c7cd5c4d5ff69b6bfb8fa974557 100644 --- a/reviewboard/webapi/tests/mixins.py +++ b/reviewboard/webapi/tests/mixins.py @@ -1536,6 +1536,7 @@ class BaseReviewRequestChildMixin(object): "%s doesn't implement setup_review_request_child_test" % self.__class__.__name__) + @add_fixtures(['test_scmtools']) @webapi_test_template def test_get_with_private_group(self): """Testing the GET API @@ -1543,7 +1544,9 @@ class BaseReviewRequestChildMixin(object): """ group = self.create_review_group(invite_only=True) group.users.add(self.user) - review_request = self.create_review_request(publish=True) + repository = self.create_repository(tool_name='Test') + review_request = self.create_review_request(publish=True, + repository=repository) review_request.target_groups.add(group) url, mimetype = self.setup_review_request_child_test(review_request) @@ -1553,13 +1556,16 @@ class BaseReviewRequestChildMixin(object): expected_mimetype=mimetype, expected_json=self.basic_get_returns_json) + @add_fixtures(['test_scmtools']) @webapi_test_template def test_get_with_private_group_no_access(self): """Testing the GET API without access to review request on a private group """ group = self.create_review_group(invite_only=True) - review_request = self.create_review_request(publish=True) + repository = self.create_repository(tool_name='Test') + review_request = self.create_review_request(publish=True, + repository=repository) review_request.target_groups.add(group) url, mimetype = self.setup_review_request_child_test(review_request) diff --git a/reviewboard/webapi/tests/test_diff.py b/reviewboard/webapi/tests/test_diff.py index 7fb3ae1d24eb2c93ca89f0578de414e1bf42c977..5f6318a467d9016ee716e46ce59827e4df8576e5 100644 --- a/reviewboard/webapi/tests/test_diff.py +++ b/reviewboard/webapi/tests/test_diff.py @@ -4,11 +4,13 @@ import os from django.core.files.uploadedfile import SimpleUploadedFile from django.utils import six +from djblets.features.testing import override_feature_check from djblets.webapi.errors import (INVALID_ATTRIBUTE, INVALID_FORM_DATA, PERMISSION_DENIED) from djblets.webapi.testing.decorators import webapi_test_template from reviewboard import scmtools +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.diffviewer.models import DiffSet from reviewboard.webapi.errors import DIFF_TOO_BIG from reviewboard.webapi.resources import resources @@ -223,20 +225,62 @@ class ResourceListTests(ExtraDataListMixin, ReviewRequestChildListMixin, self.DEFAULT_GIT_FILEDIFF_DATA, content_type='text/x-patch') - rsp = self.api_post( - get_diff_list_url(review_request), - { - 'path': diff, - 'basedir': '', - }, - expected_status=400) + with override_feature_check(dvcs_feature.feature_id, enabled=True): + rsp = self.api_post( + get_diff_list_url(review_request), + { + 'path': diff, + }, + expected_status=400) self.assertEqual(rsp['stat'], 'fail') - self.assertEqual(rsp['err']['code'], INVALID_ATTRIBUTE.code) + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) self.assertEqual( rsp['reason'], 'This review request was created with support for multiple ' - 'commits. A regular diff cannot be uploaded.') + 'commits.\n\n' + 'Create an empty diff revision and upload commits to that ' + 'instead.') + + @webapi_test_template + def test_post_empty_with_history(self): + """Testing the POST API creates an empty DiffSet for a review + request created with history support with the DVCS feature enabled + """ + review_request = self.create_review_request(submitter=self.user, + create_repository=True, + create_with_history=True) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + rsp = self.api_post(get_diff_list_url(review_request), {}, + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + item_rsp = rsp['diff'] + + diff = DiffSet.objects.get(pk=item_rsp['id']) + self.compare_item(item_rsp, diff) + self.assertEqual(diff.file_count, 0) + self.assertEqual(diff.revision, 1) + + @webapi_test_template + def test_post_empty_dvcs_disabled(self): + """Testing the POST API without a diff with the DVCS feature + disabled + """ + review_request = self.create_review_request(submitter=self.user, + create_repository=True, + create_with_history=False) + + with override_feature_check(dvcs_feature.feature_id, enabled=False): + rsp = self.api_post(get_diff_list_url(review_request), {}, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'path': ['This field is required.'], + }) @six.add_metaclass(BasicTestsMetaclass) @@ -298,6 +342,47 @@ class ResourceItemTests(ExtraDataItemMixin, ReviewRequestChildItemMixin, get_diff_item_url(review_request, diffset.revision), check_etags=True) + @webapi_test_template + def test_get_links_dvcs_enabled(self): + """Testing the GET API does includes a link to the DiffCommit + resource when the DVCS feature is enabled + """ + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + rsp = self.api_get(get_diff_item_url(review_request, + diffset.revision), + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff', rsp) + + item_rsp = rsp['diff'] + self.assertIn('links', item_rsp) + + @webapi_test_template + def test_get_links_dvcs_disabled(self): + """Testing the GET API does not include a link to the DiffCommit + resource when the DVCS feature is disabled + """ + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + + with override_feature_check(dvcs_feature.feature_id, enabled=False): + rsp = self.api_get(get_diff_item_url(review_request, + diffset.revision), + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('diff', rsp) + + item_rsp = rsp['diff'] + self.assertIn('links', item_rsp) + self.assertNotIn('diffcommits', item_rsp['links']) + # # HTTP PUT tests # diff --git a/reviewboard/webapi/tests/test_diffcommit.py b/reviewboard/webapi/tests/test_diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..63f956edc5d750ef9d24e05155e2adcecd99df9b --- /dev/null +++ b/reviewboard/webapi/tests/test_diffcommit.py @@ -0,0 +1,285 @@ +"""Unit tests for the DiffCommitResource.""" + +from __future__ import unicode_literals + +from django.contrib.auth.models import User +from django.utils import six +from djblets.testing.decorators import add_fixtures +from djblets.webapi.errors import PERMISSION_DENIED +from djblets.webapi.testing.decorators import webapi_test_template + +from reviewboard.reviews.models import ReviewRequest +from reviewboard.webapi.resources import resources +from reviewboard.webapi.tests.base import BaseWebAPITestCase +from reviewboard.webapi.tests.mimetypes import (diffcommit_item_mimetype, + diffcommit_list_mimetype) +from reviewboard.webapi.tests.mixins import (BasicTestsMetaclass, + ReviewRequestChildItemMixin, + ReviewRequestChildListMixin) +from reviewboard.webapi.tests.mixins_extra_data import ExtraDataItemMixin +from reviewboard.webapi.tests.urls import (get_diffcommit_item_url, + get_diffcommit_list_url) + + +def compare_diffcommit(self, item_rsp, item): + """Compare a serialized DiffCommit to the original. + + Args: + item_rsp (dict): + The serialized response. + + item (reviewboard.diffviewer.models.diffcommit.DiffCommit): + The DiffCommit to compare against. + + Raises: + AssertionError: + The serialized response is not equivalent to the original + DiffCommit. + """ + self.assertEqual(item_rsp['id'], item.pk) + self.assertEqual(item_rsp['commit_id'], item.commit_id) + self.assertEqual(item_rsp['parent_id'], item.parent_id) + self.assertEqual(item_rsp['commit_message'], item.commit_message) + self.assertEqual(item_rsp['author_name'], item.author_name) + self.assertEqual(item_rsp['author_email'], item.author_email) + self.assertEqual(item_rsp['committer_name'], item.committer_name) + self.assertEqual(item_rsp['committer_email'], item.committer_email) + + +@six.add_metaclass(BasicTestsMetaclass) +class ResourceListTests(ReviewRequestChildListMixin, BaseWebAPITestCase): + """Tests for DiffCommitResource list resource.""" + + fixtures = ['test_users', 'test_scmtools'] + sample_api_url = 'review-request//diffs//commits/' + resource = resources.diffcommit + + compare_item = compare_diffcommit + + def setup_http_not_allowed_list_test(self, user): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + create_with_history=True, + public=True) + + diffset = self.create_diffset(review_request=review_request) + return get_diffcommit_list_url(review_request, diffset.revision) + + def setup_review_request_child_test(self, review_request): + review_request.extra_data = review_request.extra_data or {} + review_request.extra_data[ + ReviewRequest._CREATED_WITH_HISTORY_EXTRA_DATA_KEY] = True + review_request.save(update_fields=('extra_data',)) + + diffset = self.create_diffset(review_request=review_request) + + return (get_diffcommit_list_url(review_request, + diffset.revision), + diffcommit_list_mimetype) + + # + # HTTP GET tests + # + + def setup_basic_get_test(self, user, with_local_site, local_site_name, + populate_items): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + with_local_site=with_local_site, + repository=repository, + public=True) + diffset = self.create_diffset(review_request=review_request) + items = [] + + if populate_items: + items.append(self.create_diffcommit(diffset=diffset, + repository=repository)) + + return (get_diffcommit_list_url(review_request, + diffset.revision, + local_site_name=local_site_name), + diffcommit_list_mimetype, + items) + + +@six.add_metaclass(BasicTestsMetaclass) +class ResourceItemTests(ExtraDataItemMixin, ReviewRequestChildItemMixin, + BaseWebAPITestCase): + """Tests for DiffCommitResource item resource.""" + + fixtures = ['test_users', 'test_scmtools'] + sample_api_url = \ + 'review-request//diffs//commits//' + resource = resources.diffcommit + + compare_item = compare_diffcommit + + def setup_review_request_child_test(self, review_request): + diffset = self.create_diffset(review_request=review_request) + review_request.extra_data[ + ReviewRequest._CREATED_WITH_HISTORY_EXTRA_DATA_KEY] = True + review_request.save(update_fields=('extra_data',)) + commit = self.create_diffcommit(diffset=diffset, + repository=review_request.repository) + + return (get_diffcommit_item_url(review_request, + diffset.revision, + commit.commit_id), + diffcommit_item_mimetype) + + def setup_http_not_allowed_item_test(self, user): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + public=True) + diffset = self.create_diffset(review_request=review_request) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + return get_diffcommit_item_url(review_request, diffset.revision, + commit.commit_id) + + # + # HTTP GET tests + # + + def setup_basic_get_test(self, user, with_local_site, local_site_name): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + with_local_site=with_local_site) + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + + return (get_diffcommit_item_url(review_request, diffset.revision, + commit.commit_id, local_site_name), + diffcommit_item_mimetype, + commit) + + @webapi_test_template + def test_get_patch(self): + """Testing the GET API with Accept: text/x-patch""" + url = self.setup_basic_get_test(self.user, + with_local_site=False, + local_site_name=None)[0] + + rsp = self.api_get(url, + expected_mimetype='text/x-patch', + expected_json=False, + HTTP_ACCEPT='text/x-patch') + + self.assertEqual(self.DEFAULT_GIT_FILEDIFF_DATA, rsp) + + @add_fixtures(['test_site']) + @webapi_test_template + def test_get_patch_local_site(self): + """Testing the GET API with Accept: text/x-patch on a Local Site + """ + url = self.setup_basic_get_test( + User.objects.get(username='doc'), + with_local_site=True, + local_site_name=self.local_site_name)[0] + + self.client.login(username='doc', password='doc') + + rsp = self.api_get(url, + expected_mimetype='text/x-patch', + expected_json=False, + HTTP_ACCEPT='text/x-patch') + + self.assertEqual(self.DEFAULT_GIT_FILEDIFF_DATA, rsp) + + @add_fixtures(['test_site']) + @webapi_test_template + def test_get_patch_local_site_no_access(self): + """Testing the GET API with Accept: text/x-patch on a Local Site + without access + """ + url = self.setup_basic_get_test( + User.objects.get(username='doc'), + with_local_site=True, + local_site_name=self.local_site_name)[0] + + rsp = self.api_get(url, + expected_status=403, + HTTP_ACCEPT='text/x-patch') + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], PERMISSION_DENIED.code) + + @webapi_test_template + def test_get_patch_private_repository(self): + """Testing the GET API with Accept: text/x-patch on a private + repository + """ + doc = User.objects.get(username='doc') + + repository = self.create_repository(tool_name='Git', public=False) + repository.users = [doc] + + review_request = self.create_review_request(repository=repository, + submitter=doc) + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset, repository=repository) + + self.client.login(username='doc', password='doc') + rsp = self.api_get( + get_diffcommit_item_url(review_request, diffset.revision, + commit.commit_id), + expected_mimetype='text/x-patch', + expected_json=False, + HTTP_ACCEPT='text/x-patch') + + self.assertEqual(self.DEFAULT_GIT_FILEDIFF_DATA, rsp) + + @webapi_test_template + def test_get_patch_private_repository_no_access(self): + """Testing the GET API with Accept: text/x-patch on a private + repository + """ + doc = User.objects.get(username='doc') + + repository = self.create_repository(tool_name='Git', public=False) + repository.users = [doc] + + review_request = self.create_review_request(repository=repository, + submitter=doc) + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset, repository=repository) + + rsp = self.api_get( + get_diffcommit_item_url(review_request, diffset.revision, + commit.commit_id), + expected_status=403) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], PERMISSION_DENIED.code) + + # + # HTTP PUT tests + # + + def setup_basic_put_test(self, user, with_local_site, local_site_name, + put_valid_data): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + with_local_site=with_local_site) + diffset = self.create_diffset(review_request) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + + return (get_diffcommit_item_url(review_request, + diffset.revision, + commit.commit_id, + local_site_name=local_site_name), + diffcommit_item_mimetype, + {}, + commit, + []) + + def check_put_result(self, user, item_rsp, item): + self.compare_item(item_rsp, item) diff --git a/reviewboard/webapi/tests/test_draft_diff.py b/reviewboard/webapi/tests/test_draft_diff.py index 92803237936a2776e1fd50edac76d7cfafec16ff..9aee94866e643397d5ff9c5d6d3c80ec7a547158 100644 --- a/reviewboard/webapi/tests/test_draft_diff.py +++ b/reviewboard/webapi/tests/test_draft_diff.py @@ -4,10 +4,12 @@ import os from django.core.files.uploadedfile import SimpleUploadedFile from django.utils import six -from djblets.webapi.errors import INVALID_ATTRIBUTE, INVALID_FORM_DATA +from djblets.features.testing import override_feature_check +from djblets.webapi.errors import INVALID_FORM_DATA from djblets.webapi.testing.decorators import webapi_test_template from reviewboard import scmtools +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.diffviewer.models import DiffSet from reviewboard.webapi.errors import DIFF_TOO_BIG from reviewboard.webapi.resources import resources @@ -173,7 +175,7 @@ class ResourceListTests(ExtraDataListMixin, BaseWebAPITestCase): self.siteconfig.get('diffviewer_max_diff_size')) @webapi_test_template - def test_post_with_history(self): + def test_post_diff_with_history(self): """Testing the POST API with a diff and a review request created with history support """ @@ -191,11 +193,52 @@ class ResourceListTests(ExtraDataListMixin, BaseWebAPITestCase): expected_status=400) self.assertEqual(rsp['stat'], 'fail') - self.assertEqual(rsp['err']['code'], INVALID_ATTRIBUTE.code) + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) self.assertEqual( rsp['reason'], 'This review request was created with support for multiple ' - 'commits. A regular diff cannot be uploaded.') + 'commits.\n\n' + 'Create an empty diff revision and upload commits to that ' + 'instead.') + + @webapi_test_template + def test_post_empty_with_history(self): + """Testing the POST API creates an empty DiffSet for a review + request created with history support with the DVCS feature enabled + """ + review_request = self.create_review_request(submitter=self.user, + create_repository=True, + create_with_history=True) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + rsp = self.api_post(get_draft_diff_list_url(review_request), {}, + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + item_rsp = rsp['diff'] + + diff = DiffSet.objects.get(pk=item_rsp['id']) + self.compare_item(item_rsp, diff) + self.assertEqual(diff.file_count, 0) + + @webapi_test_template + def test_post_empty_dvcs_disabled(self): + """Testing the POST API without a diff with the DVCS feature + disabled + """ + review_request = self.create_review_request(submitter=self.user, + create_repository=True, + create_with_history=False) + + with override_feature_check(dvcs_feature.feature_id, enabled=False): + rsp = self.api_post(get_draft_diff_list_url(review_request), {}, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + self.assertEqual(rsp['fields'], { + 'path': ['This field is required.'], + }) @six.add_metaclass(BasicTestsMetaclass) diff --git a/reviewboard/webapi/tests/test_draft_diffcommit.py b/reviewboard/webapi/tests/test_draft_diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..d90f34420c134f97141d99441d9170755976ea17 --- /dev/null +++ b/reviewboard/webapi/tests/test_draft_diffcommit.py @@ -0,0 +1,444 @@ +"""Unit tests for the DraftDiffCommitResource.""" + +from __future__ import unicode_literals + +from django.core.files.uploadedfile import SimpleUploadedFile +from django.utils import six, timezone +from djblets.siteconfig.models import SiteConfiguration +from djblets.webapi.errors import INVALID_ATTRIBUTE, INVALID_FORM_DATA +from djblets.webapi.testing.decorators import webapi_test_template + +from reviewboard.diffviewer.models import DiffCommit +from reviewboard.reviews.models import ReviewRequestDraft +from reviewboard.webapi.errors import DIFF_EMPTY, DIFF_TOO_BIG +from reviewboard.webapi.resources import resources +from reviewboard.webapi.tests.base import BaseWebAPITestCase +from reviewboard.webapi.tests.mixins import BasicTestsMetaclass +from reviewboard.webapi.tests.mixins_extra_data import ExtraDataItemMixin +from reviewboard.webapi.tests.mimetypes import ( + draft_diffcommit_list_mimetype, + draft_diffcommit_item_mimetype) +from reviewboard.webapi.tests.test_diffcommit import compare_diffcommit +from reviewboard.webapi.tests.urls import (get_draft_diffcommit_item_url, + get_draft_diffcommit_list_url) + + +@six.add_metaclass(BasicTestsMetaclass) +class ResourceListTests(BaseWebAPITestCase): + """Tests for DraftDiffCommitResource list resource.""" + + fixtures = ['test_users', 'test_scmtools'] + sample_api_url = 'review-request//draft/commits/' + resource = resources.draft_diffcommit + + compare_item = compare_diffcommit + + _DEFAULT_DIFF_CONTENTS = ( + b'diff --git a/readme b/readme\n' + b'index d6613f5..5b50866 100644\n' + b'--- a/readme\n' + b'+++ b/readme\n' + b'@@ -1 +1,3 @@\n' + b' Hello there\n' + b'+\n' + b'+Oh hi!\n' + ) + + _DEFAULT_POST_DATA = { + 'author_name': 'Author', + 'author_date': timezone.now().strftime(DiffCommit.ISO_DATE_FORMAT), + 'author_email': 'author@example.com', + 'committer_name': 'Committer', + 'committer_date': timezone.now().strftime(DiffCommit.ISO_DATE_FORMAT), + 'committer_email': 'committer@example.com', + 'commit_message': 'Commit message', + 'commit_id': 'r1', + 'parent_id': 'r0', + } + + def setup_http_not_allowed_list_test(self, user): + review_request = self.create_review_request( + create_repository=True, + public=True) + diffset = self.create_diffset(review_request=review_request) + return get_draft_diffcommit_list_url(review_request, diffset.revision) + + # + # HTTP GET tests + # + + def setup_basic_get_test(self, user, with_local_site, local_site_name, + populate_items): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + publish=True, + with_local_site=with_local_site, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + items = [] + + if populate_items: + items.append(self.create_diffcommit(diffset=diffset, + repository=repository)) + + return ( + get_draft_diffcommit_list_url( + review_request, + diffset.revision, + local_site_name=local_site_name), + draft_diffcommit_list_mimetype, + items, + ) + + # + # HTTP POST tests + # + + def setup_basic_post_test(self, user, with_local_site=False, + local_site_name=None, post_valid_data=False): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + with_local_site=with_local_site, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + if post_valid_data: + diff = SimpleUploadedFile('diff', self._DEFAULT_DIFF_CONTENTS) + post_data = dict(self._DEFAULT_POST_DATA, + **{'diff': diff}) + else: + post_data = {} + + return ( + get_draft_diffcommit_list_url(review_request, + diffset.revision, + local_site_name=local_site_name), + draft_diffcommit_item_mimetype, + post_data, + [], + ) + + def check_post_result(self, user, rsp): + self.assertIn('draft_commit', rsp) + item = rsp['draft_commit'] + diffcommit = DiffCommit.objects.get(pk=item['id']) + + self.compare_item(item, diffcommit) + + @webapi_test_template + def test_post_empty(self): + """Testing the POST API with an empty diff""" + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'diff': SimpleUploadedFile('diff', b' ', + content_type='text/x-patch'), + }), + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], DIFF_EMPTY.code) + + @webapi_test_template + def test_post_too_large(self): + """Testing the POST API with a diff that is too large""" + siteconfig = SiteConfiguration.objects.get_current() + max_diff_size = siteconfig.get('diffviewer_max_diff_size') + siteconfig.set('diffviewer_max_diff_size', 1) + siteconfig.save() + + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + diff = SimpleUploadedFile('diff', + self._DEFAULT_DIFF_CONTENTS, + content_type='text/x-patch') + + try: + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, + diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'diff': diff, + }), + expected_status=400) + finally: + siteconfig.set('diffviewer_max_diff_size', max_diff_size) + siteconfig.save() + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], DIFF_TOO_BIG.code) + self.assertEqual(rsp['max_size'], 1) + + @webapi_test_template + def test_post_no_history_allowed(self): + """Testing the POST API for a review request created without + history support + """ + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=False) + ReviewRequestDraft.create(review_request) + diffset = self.create_diffset(review_request, draft=True) + + diff = SimpleUploadedFile('diff', + self._DEFAULT_DIFF_CONTENTS, + content_type='text/x-patch') + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'diff': diff, + }), + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_ATTRIBUTE.code) + self.assertEqual( + rsp['reason'], + 'This review request was not created with support for multiple ' + 'commits.\n\n' + 'Use the draft_diff resource to upload diffs instead. See the ' + 'draft_diff link on the parent resource for the URL.') + + @webapi_test_template + def test_post_parent_diff(self): + """Testing the POST API with a parent diff""" + parent_diff_contents = ( + b'diff --git a/foo b/foo\n' + b'new file mode 100644\n' + b'index 0000000..e69de29\n' + ) + + diff_contents = ( + b'diff --git a/foo b/foo\n' + b'index e69ded29..03b37a0 100644\n' + b'--- a/foo\n' + b'+++ b/foo\n' + b'@@ -0,0 +1 @@' + b'+foo bar baz qux\n' + ) + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + diff = SimpleUploadedFile('diff', diff_contents, + content_type='text/x-patch') + parent_diff = SimpleUploadedFile('parent_diff', + parent_diff_contents, + content_type='text/x-patch') + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'diff': diff, + 'parent_diff': parent_diff, + }), + expected_mimetype=draft_diffcommit_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('draft_commit', rsp) + + item_rsp = rsp['draft_commit'] + self.compare_item(item_rsp, DiffCommit.objects.get(pk=item_rsp['id'])) + + commit = DiffCommit.objects.get(pk=item_rsp['id']) + self.compare_item(item_rsp, commit) + + files = list(commit.files.all()) + self.assertEqual(len(files), 1) + + f = files[0] + self.assertIsNotNone(f.parent_diff) + + @webapi_test_template + def test_post_parent_diff_subsequent(self): + """Testing the POST API with a parent diff on a subsequent commit + """ + parent_diff_contents = ( + b'diff --git a/foo b/foo\n' + b'new file mode 100644\n' + b'index 0000000..e69de29\n' + ) + + diff_contents = ( + b'diff --git a/foo b/foo\n' + b'index e69ded29..03b37a0 100644\n' + b'--- a/foo\n' + b'+++ b/foo\n' + b'@@ -0,0 +1 @@' + b'+foo bar baz qux\n' + ) + + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + + diffset = self.create_diffset(review_request, draft=True) + self.create_diffcommit(repository, diffset) + + diff = SimpleUploadedFile('diff', diff_contents, + content_type='text/x-patch') + parent_diff = SimpleUploadedFile('parent_diff', parent_diff_contents, + content_type='text/x-patch') + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'commit_id': 'r0', + 'parent_id': 'r1', + 'diff': diff, + 'parent_diff': parent_diff, + }), + expected_mimetype=draft_diffcommit_item_mimetype, + expected_status=201) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('draft_commit', rsp) + + item_rsp = rsp['draft_commit'] + commit = DiffCommit.objects.get(pk=item_rsp['id']) + self.compare_item(item_rsp, commit) + + files = list(commit.files.all()) + self.assertEqual(len(files), 1) + + f = files[0] + self.assertIsNotNone(f.parent_diff) + + @webapi_test_template + def test_post_invalid_date_format(self): + """Testing the POST API with an invalid date format""" + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + diff = SimpleUploadedFile('diff', + self._DEFAULT_DIFF_CONTENTS, + content_type='text/x-patch') + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'commit_id': 'r0', + 'parent_id': 'r1', + 'diff': diff, + 'committer_date': 'Jun 1 1990', + 'author_date': 'Jun 1 1990', + }), + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_FORM_DATA.code) + + err_fields = rsp['fields'] + self.assertIn('author_date', err_fields) + self.assertIn('committer_date', err_fields) + + self.assertEqual(err_fields['author_date'], + ['This date must be in ISO 8601 format.']) + self.assertEqual(err_fields['committer_date'], + ['This date must be in ISO 8601 format.']) + + +@six.add_metaclass(BasicTestsMetaclass) +class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): + """Tests for DraftDiffCommitResource item resource.""" + + fixtures = ['test_users', 'test_scmtools'] + sample_api_url = 'review-request//draft/commits//' + resource = resources.draft_diffcommit + + compare_item = compare_diffcommit + + def setup_http_not_allowed_item_test(self, user): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request(repository=repository, + submitter=user) + diffset = self.create_diffset(review_request, draft=True) + commit = self.create_diffcommit(repository=repository, + diffset=diffset) + + return get_draft_diffcommit_item_url(review_request, + diffset.revision, + commit.commit_id) + + # + # HTTP GET tests + # + + def setup_basic_get_test(self, user, with_local_site, local_site_name): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + with_local_site=with_local_site) + diffset = self.create_diffset(review_request, draft=True) + commit = self.create_diffcommit(repository=repository, + diffset=diffset) + + return ( + get_draft_diffcommit_item_url(review_request, + diffset.revision, + commit.commit_id, + local_site_name=local_site_name), + draft_diffcommit_item_mimetype, + commit) + + # + # HTTP PUT tests + # + + def setup_basic_put_test(self, user, with_local_site, local_site_name, + put_valid_data): + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=user, + with_local_site=with_local_site) + diffset = self.create_diffset(review_request, draft=True) + commit = self.create_diffcommit(repository=repository, + diffset=diffset) + + if put_valid_data: + request_data = {} + else: + request_data = {} + + return ( + get_draft_diffcommit_item_url(review_request, + diffset.revision, + commit.commit_id, + local_site_name=local_site_name), + draft_diffcommit_item_mimetype, + request_data, + commit, + []) + + def check_put_result(self, user, item_rsp, commit): + commit = DiffCommit.objects.get(pk=commit.pk) + self.compare_item(item_rsp, commit) diff --git a/reviewboard/webapi/tests/test_review_request.py b/reviewboard/webapi/tests/test_review_request.py index 5af95f512ccc9adc5390826c062f4fdc2cd98265..5b8e887b34fa1bbf11148e481c10eb440af6a2be 100644 --- a/reviewboard/webapi/tests/test_review_request.py +++ b/reviewboard/webapi/tests/test_review_request.py @@ -1670,7 +1670,7 @@ class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): @webapi_test_template def test_get_dvcs_feature_enabled(self): - """Testing the GET API includes DVCS-specific fields in + """Testing the GET API includes DVCS-specific fields and links in the response when the DVCS feature is enabled """ with override_feature_check(dvcs_feature.feature_id, enabled=True): @@ -1686,8 +1686,8 @@ class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): @webapi_test_template def test_get_dvcs_feature_disabled(self): - """Testing the GET API does not include DVCS-specific fields in - the response when the DVCS feature is disabled + """Testing the GET API does not include DVCS-specific fields and + links in the response when the DVCS feature is disabled """ with override_feature_check(dvcs_feature.feature_id, enabled=False): review_request = self.create_review_request(publish=True) diff --git a/reviewboard/webapi/tests/urls.py b/reviewboard/webapi/tests/urls.py index a72e3b75ae813f4576b6f3ea627c7f5459daaf3c..53cda4a434922b06fe8954b92e95cec7b62cbf02 100644 --- a/reviewboard/webapi/tests/urls.py +++ b/reviewboard/webapi/tests/urls.py @@ -106,6 +106,26 @@ def get_diff_item_url(review_request, diff_revision, local_site_name=None): diff_revision=diff_revision) +# +# DiffCommitResource +# +def get_diffcommit_list_url(review_request, diff_revision, + local_site_name=None): + return resources.diffcommit.get_list_url( + local_site_name=local_site_name, + review_request_id=review_request.display_id, + diff_revision=diff_revision) + + +def get_diffcommit_item_url(review_request, diff_revision, commit_id, + local_site_name=None): + return resources.diffcommit.get_item_url( + local_site_name=local_site_name, + review_request_id=review_request.display_id, + diff_revision=diff_revision, + commit_id=commit_id) + + # # DiffFileAttachmentResource # @@ -123,6 +143,26 @@ def get_diff_file_attachment_item_url(attachment, repository, file_attachment_id=attachment.pk) +# +# DraftDiffCommitResource +# +def get_draft_diffcommit_list_url(review_request, diff_revision, + local_site_name=None): + return resources.draft_diffcommit.get_list_url( + review_request_id=review_request.display_id, + diff_revision=diff_revision, + local_site_name=local_site_name) + + +def get_draft_diffcommit_item_url(review_request, diff_revision, commit_id, + local_site_name=None): + return resources.draft_diffcommit.get_item_url( + review_request_id=review_request.display_id, + diff_revision=diff_revision, + commit_id=commit_id, + local_site_name=local_site_name) + + # # DraftDiffResource #