diff --git a/reviewboard/diffviewer/forms.py b/reviewboard/diffviewer/forms.py index 27161003e35bbc94292a27284d6bdcc1f288900d..011dabbb0ad28882d4e95a47f15cb28f80e37478 100644 --- a/reviewboard/diffviewer/forms.py +++ b/reviewboard/diffviewer/forms.py @@ -1,3 +1,5 @@ +"""Forms for uploading diffs.""" + from __future__ import unicode_literals from django import forms @@ -7,23 +9,21 @@ from django.utils.translation import ugettext_lazy as _ from reviewboard.diffviewer.models import DiffSet -class NoBaseDirError(ValueError): - pass - - class UploadDiffForm(forms.Form): + """The form for uploading a diff and creating a DiffSet.""" + basedir = forms.CharField( - label=_("Base Directory"), - help_text=_("The absolute path in the repository the diff was " - "generated in.")) + label=_('Base Directory'), + help_text=_('The absolute path in the repository the diff was ' + 'generated in.')) path = forms.FileField( - label=_("Diff"), - help_text=_("The new diff to upload.")) + label=_('Diff'), + help_text=_('The new diff to upload.')) parent_diff_path = forms.FileField( - label=_("Parent Diff"), - help_text=_("An optional diff that the main diff is based on. " - "This is usually used for distributed revision control " - "systems (Git, Mercurial, etc.)."), + label=_('Parent Diff'), + help_text=_('An optional diff that the main diff is based on. ' + 'This is usually used for distributed revision control ' + 'systems (Git, Mercurial, etc.).'), required=False) base_commit_id = forms.CharField( @@ -31,10 +31,23 @@ class UploadDiffForm(forms.Form): help_text=_('The ID/revision this change is built upon.'), required=False) - def __init__(self, repository, data=None, files=None, request=None, - *args, **kwargs): - super(UploadDiffForm, self).__init__(data=data, files=files, - *args, **kwargs) + def __init__(self, repository, request=None, *args, **kwargs): + """Initialize the form. + + Args: + repository (reviewboard.scmtools.models.Repository): + The repository the DiffSet is to be created against. + + request (django.http.HttpRequest): + The current HTTP request. + + *args (tuple): + Additional positional arguments. + + **kwrgs (dict): + Additional keyword arguments. + """ + super(UploadDiffForm, self).__init__(*args, **kwargs) self.repository = repository self.request = request @@ -44,26 +57,46 @@ class UploadDiffForm(forms.Form): del(self.fields['basedir']) def clean_base_commit_id(self): + """Clean the ``base_commit_id`` field. + + Returns: + unicode: + The ``base_commit_id`` field stripped of leading and trailing + whitespace, or ``None`` if that value would be empty. + """ return self.cleaned_data['base_commit_id'].strip() or None - def create(self, diff_file, parent_diff_file=None, diffset_history=None): - tool = self.repository.get_scmtool() + def clean_basedir(self): + """Clean the ``basedir`` field. + + Returns: + unicode: + The basedir field as a unicode string with leading and trailing + whitespace removed. + """ + if self.repository.get_scmtool().diffs_use_absolute_paths: + return '' + + return smart_unicode(self.cleaned_data['basedir'].strip()) + + def create(self, diffset_history=None): + """Create the DiffSet. + + Args: + diffset_history (reviewboard.diffviewer.models.DiffSetHistory): + The DiffSet history to attach the created DiffSet to. - # Grab the base directory if there is one. - if not tool.diffs_use_absolute_paths: - try: - basedir = smart_unicode(self.cleaned_data['basedir'].strip()) - except AttributeError: - raise NoBaseDirError( - _('The "Base Diff Path" field is required')) - else: - basedir = '' + Returns: + reviewboard.diffviewer.models.DiffSet: + The created DiffSet. + """ + assert self.is_valid() return DiffSet.objects.create_from_upload( repository=self.repository, - diff_file=diff_file, - parent_diff_file=parent_diff_file, + diff_file=self.cleaned_data['path'], + parent_diff_file=self.cleaned_data.get('parent_diff_path'), diffset_history=diffset_history, - basedir=basedir, + basedir=self.cleaned_data.get('basedir', ''), base_commit_id=self.cleaned_data['base_commit_id'], request=self.request) diff --git a/reviewboard/diffviewer/tests/test_forms.py b/reviewboard/diffviewer/tests/test_forms.py index 8503e52307d007c99734efe5d4339b565b6ad9fd..9882bcc01e4237a221e5a82452e22530dccdcb0e 100644 --- a/reviewboard/diffviewer/tests/test_forms.py +++ b/reviewboard/diffviewer/tests/test_forms.py @@ -39,7 +39,7 @@ class UploadDiffFormTests(SpyAgency, TestCase): }) self.assertTrue(form.is_valid()) - diffset = form.create(diff_file) + diffset = form.create() self.assertEqual(diffset.files.count(), 1) self.assertEqual(diffset.basedir, '/') self.assertEqual(diffset.base_commit_id, '1234') @@ -91,7 +91,7 @@ class UploadDiffFormTests(SpyAgency, TestCase): }) self.assertTrue(form.is_valid()) - diffset = form.create(diff_file, parent_diff_file) + diffset = form.create() self.assertEqual(diffset.files.count(), 1) filediff = diffset.files.get() @@ -151,7 +151,7 @@ class UploadDiffFormTests(SpyAgency, TestCase): }) self.assertTrue(form.is_valid()) - diffset = form.create(diff_file, parent_diff_file) + diffset = form.create() self.assertEqual(diffset.files.count(), 1) filediff = diffset.files.get() @@ -207,7 +207,7 @@ class UploadDiffFormTests(SpyAgency, TestCase): }) self.assertTrue(form.is_valid()) - diffset = form.create(diff, parent_diff) + diffset = form.create() self.assertEqual(diffset.files.count(), 1) f = diffset.files.get() @@ -280,7 +280,7 @@ class UploadDiffFormTests(SpyAgency, TestCase): }) self.assertTrue(form.is_valid()) - diffset = form.create(diff, parent_diff) + diffset = form.create() self.assertEqual(diffset.files.count(), 1) f = diffset.files.get() @@ -294,3 +294,47 @@ class UploadDiffFormTests(SpyAgency, TestCase): patched_file = get_patched_file(original_file, f, None) self.assertEqual(patched_file, b'Foo\nBar\nBaz\n') self.assertEqual(len(patch.spy.calls), 2) + + def test_crate_missing_basedir(self): + """Testing UploadDiffForm with a missing basedir field that is + required + """ + repository = self.create_repository(tool_name='Test') + scmtool = repository.get_scmtool() + + self.spy_on(repository.get_file_exists, + call_fake=lambda *args, **kwargs: True) + + revisions = [ + b'93e6b3e8944c48737cb11a1e52b046fa30aea7a9', + b'4839fc480f47ca59cf05a9c39410ea744d1e17a2', + ] + + diff = SimpleUploadedFile( + 'diff', + (b'diff --git a/bar b/bar\n' + b'index %s..%s 100644\n' + b'--- a/bar\n' + b'+++ b/bar\n' + b'@@ -1,2 +1,3 @@\n' + b' Foo\n' + b'+Bar\n') % (revisions[0], revisions[1]), + content_type='text/x-patch') + + try: + orig_use_abs_paths = scmtool.diffs_use_absolute_paths + scmtool.diffs_use_absolute_paths = True + + form = UploadDiffForm( + repository=repository, + files={ + 'path': diff, + } + ) + + self.assertFalse(form.is_valid()) + finally: + scmtool.diffs_use_absolute_paths = orig_use_abs_paths + + self.assertIn('basedir', form.errors) + self.assertIn('This field is required.', form.errors['basedir']) diff --git a/reviewboard/reviews/forms.py b/reviewboard/reviews/forms.py index 535a24e699287f5ee504af8332999ffb6f85628e..b186db3930c2d07172a7d3308b9f4b35793b032b 100644 --- a/reviewboard/reviews/forms.py +++ b/reviewboard/reviews/forms.py @@ -120,17 +120,32 @@ class GroupForm(forms.ModelForm): class UploadDiffForm(diffviewer_forms.UploadDiffForm): - """ - A specialized UploadDiffForm that knows how to interact with review - requests. - """ - def __init__(self, review_request, data=None, *args, **kwargs): + """A specialized UploadDiffForm for interacting with review requests.""" + + def __init__(self, review_request, request=None, *args, **kwargs): + """Initialize the form. + + Args: + review_request (reviewboard.reviews.models.ReviewRequest): + The review request that the uploaded diff will be attached to. + + request (django.http.HttpRequest): + The current HTTP request. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + """ super(UploadDiffForm, self).__init__(review_request.repository, - data, *args, **kwargs) + request, + *args, + **kwargs) self.review_request = review_request if ('basedir' in self.fields and - (not data or 'basedir' not in data)): + (not self.data or 'basedir' not in self.data)): try: diffset = DiffSet.objects.filter( history=review_request.diffset_history_id).latest() @@ -138,16 +153,29 @@ class UploadDiffForm(diffviewer_forms.UploadDiffForm): except DiffSet.DoesNotExist: pass - def create(self, diff_file, parent_diff_file=None, - attach_to_history=False): + def create(self, attach_to_history=False): + """Create the DiffSet and optionally attach it to the history. + + Args: + attach_to_history (bool): + Whether or not the created + :py:class:`~reviewboard.diffviewer.models.DiffSet` will be + attached to the diffset history of the + :py:class:`reviewboard.reviews.model.ReviewRequest`. + + Defaults to ``False``. + + Returns: + reviewboard.diffviewer.models.DiffSet: + The created DiffSet. + """ + assert self.is_valid() history = None if attach_to_history: history = self.review_request.diffset_history - diffset = super(UploadDiffForm, self).create(diff_file, - parent_diff_file, - history) + diffset = super(UploadDiffForm, self).create(history) if not attach_to_history: # Set the initial revision to be one newer than the most recent diff --git a/reviewboard/webapi/resources/diff.py b/reviewboard/webapi/resources/diff.py index 5021bf9e9d849b881db8535c366198b5e232cdce..8d1dce4d3773ac843db63489fbda3d41c99c25d6 100644 --- a/reviewboard/webapi/resources/diff.py +++ b/reviewboard/webapi/resources/diff.py @@ -281,7 +281,9 @@ class DiffResource(WebAPIResource): } form_data = request.POST.copy() - form = UploadDiffForm(review_request, form_data, request.FILES, + form = UploadDiffForm(review_request, + data=form_data, + files=request.FILES, request=request) if not form.is_valid(): @@ -290,8 +292,7 @@ class DiffResource(WebAPIResource): } try: - diffset = form.create(request.FILES['path'], - request.FILES.get('parent_diff_path')) + diffset = form.create() except FileNotFoundError as e: return REPO_FILE_NOT_FOUND, { 'file': e.path,