Submitter:	Christian Hammond	Reviewers
Branch:	trunk	Groups:	reviewboard
Bugs:		People:
Change Number:	None	Repository:	Review Board SVN

Description:

Migrate file uploading support to use the newer ImageField and FileField, as well as the new uploading classes in Django. This ends up cleaning up our code a little bit.

We also now upload images to /year/month/day directories.

This also changes unit tests to write files to a temporary directory so we don't litter the actual upload directory.

Testing Done:

Verified that I could still upload diffs and images without problems. Also verified that the internal validation of correct image types works.

This passed all unit tests (well, as well as it did before -- there's some issue with writing 0-byte images to disk, and I think it's a Django bug. Still investigating.)

Diff revision 2 (Latest)

Jump to revision:	1 2
Changes between r2 and:	1 2

Files Changed:

/trunk/reviewboard/manage.py: 2 changes [ 1 2 ]
/trunk/reviewboard/test.py: 3 changes [ 1 2 3 ]
/trunk/reviewboard/diffviewer/forms.py: 6 changes [ 1 2 3 4 5 6 ]
/trunk/reviewboard/reviews/forms.py: 14 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 ]
/trunk/reviewboard/reviews/models.py: 2 changes [ 1 2 ]
/trunk/reviewboard/reviews/views.py: 2 changes [ 1 2 ]
/trunk/reviewboard/webapi/json.py: 3 changes [ 1 2 3 ]
/trunk/reviewboard/webapi/tests.py: 4 changes [ 1 2 3 4 ]

Expand changes

     try:
         import django
         if django.VERSION[0] == 0 and django.VERSION[1] < 97:
             raise ImportError
-        # QuerySet refactor (r7477)
+        # UploadedFile support (~r7829)
-        from django.core.exceptions import FieldError
+        from django.core.files.uploadedfile import UploadedFile
     except ImportError:
-        dependency_error("Django > 0.97 (or SVN >= r7477) is required.")
+        dependency_error("Django > 0.97 (or SVN >= r7829) is required.")
     # django-evolution
     try:
         import django_evolution
     except ImportError:

 # CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
 # TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
 # SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+#
 import os
 import os.path
 import sys
 import djblets
 import nose
     setup_test_environment()
     settings.DEBUG = False
     # Default to testing in a non-subdir install.
     settings.SITE_ROOT = "/"
     settings.MEDIA_ROOT = "/tmp/reviewboard-tests"
     images_dir = os.path.join(settings.MEDIA_ROOT, "uploaded", "images")
     if not os.path.exists(images_dir):
         os.makedirs(images_dir)
     settings.MEDIA_URL = settings.SITE_ROOT + 'media/'
     settings.ADMIN_MEDIA_PREFIX = settings.MEDIA_URL + 'admin/'
     old_name = settings.DATABASE_NAME
     create_test_db(verbosity, autoclobber=not interactive)
     if len(sys.argv) > 2:
         nose_argv += sys.argv[2:]
     nose.main(argv=nose_argv)
     for root, dirs, files in walk(settings.MEDIA_ROOT, topdown=False):
         for name in files:
             os.remove(os.path.join(root, name))
         for name in dirs:
             os.rmdir(os.path.join(root, name))
     destroy_test_db(old_name, verbosity)
     teardown_test_environment()

     pass
 class UploadDiffForm(forms.Form):
     basedir = forms.CharField(label=_("Base directory"))
-    path = forms.CharField(label=_("Diff"), widget=forms.FileInput())
+    path = forms.FileField(label=_("Diff"))
-    parent_diff_path = forms.CharField(label=_("Parent diff (optional)"),
+    parent_diff_path = forms.FileField(label=_("Parent diff (optional)"),
-                                       widget=forms.FileInput(),
                                        required=False)
     # Extensions used for intelligent sorting of header files
     # before implementation files.
     HEADER_EXTENSIONS = ["h", "H", "hh", "hpp", "hxx", "h++"]
         if self.repository.get_scmtool().get_diffs_use_absolute_paths():
             # This SCMTool uses absolute paths, so there's no need to ask
             # the user for the base directory.
             del(self.fields['basedir'])
-    def create(self, diff_file, parent_diff_file=None, diffset_history=None):
+    def create(self, diffset_history=None):
         tool = self.repository.get_scmtool()
         # Grab the base directory if there is one.
         if not tool.get_diffs_use_absolute_paths():
             try:
                 basedir = smart_unicode(self.cleaned_data['basedir'])
             except AttributeError:
                 raise NoBaseDirError(_('The "Base Diff Path" field is required'))
         else:
             basedir = ''
         diff_file = self.cleaned_data['path']
         parent_diff_file = self.cleaned_data['parent_diff_path']
         # Parse the diff
         files = list(self._process_files(
             diff_file, basedir, check_existance=(parent_diff_file is not None)))
         if len(files) == 0:
             # later apply each of the files that are in the main diff
             for f in self._process_files(parent_diff_file, basedir,
                                          check_existance=True):
                 parent_files[f.origFile] = f
-        diffset = DiffSet(name=diff_file["filename"], revision=0,
+        diffset = DiffSet(name=diff_file.filename, revision=0,
                           history=diffset_history,
                           diffcompat=DEFAULT_DIFF_COMPAT_VERSION)
         diffset.repository = self.repository
         diffset.save()
         return diffset
     def _process_files(self, file, basedir, check_existance=False):
         tool = self.repository.get_scmtool()
-        for f in tool.get_parser(file["content"]).parse():
+        for f in tool.get_parser(file.data.read()).parse():
             f2, revision = tool.parse_diff_revision(f.origFile, f.origInfo)
             if f2.startswith("/"):
                 filename = f2
             else:
                 filename = os.path.join(basedir, f2).replace("\\", "/")

 import re
 import os.path
 from django import newforms as forms
 from django.utils.translation import ugettext as _
-from PIL import Image
 from reviewboard.diffviewer.forms import UploadDiffForm, EmptyDiffError
 from reviewboard.reviews.models import ReviewRequest, \
                                        ReviewRequestDraft, Screenshot, \
                                        ChangeNumberInUseError
     information on the diffs, the repository the diffs are against, and
     optionally a changelist number (for use in certain repository types
     such as Perforce).
     """
     basedir = forms.CharField(label=_("Base Diff Path"), required=False)
-    diff_path = forms.CharField(label=_("Diff"),
+    diff_path = forms.FileField(label=_("Diff"), required=True)
-                                widget=forms.FileInput, required=True)
+    parent_diff_path = forms.FileField(label=_("Parent Diff (optional)"),
-    parent_diff_path = forms.CharField(label=_("Parent Diff (optional)"),
+                                       required=False)
-                                       widget=forms.FileInput, required=False)
     repository = forms.ChoiceField(label=_("Repository"), required=True)
     changenum = forms.IntegerField(label=_("Change Number"), required=False)
     def __init__(self, *args, **kwargs):
         forms.Form.__init__(self, *args, **kwargs)
         """Helper function to combine the common bits of clean_target_people
            and clean_target_groups"""
         names = [x for x in map(str.strip, re.split(',\s*', data)) if x]
         return set([constructor(name) for name in names])
-    def create(self, user, diff_file, parent_diff_file):
+    def create(self, user):
         repository = Repository.objects.get(pk=self.cleaned_data['repository'])
         changenum = self.cleaned_data['changenum'] or None
         # It's a little odd to validate this here, but we want to have access to
         # the user.
         diff_form = UploadDiffForm(repository, data={
             'basedir': self.cleaned_data['basedir'],
         },
         files={
-            'path': self.cleaned_data['diff_path'],
+            'path': self.cleaned_data['diff_path'].data,
-            'parent_diff_path': self.cleaned_data['parent_diff_path'],
+            'parent_diff_path': getattr(self.cleaned_data['parent_diff_path'],
                                         "data", None),
         })
         diff_form.full_clean()
         try:
-            diff_form.create(diff_file, parent_diff_file,
+            diff_form.create(review_request.diffset_history)
-                             review_request.diffset_history)
             if 'path' in diff_form.errors:
                 review_request.delete()
                 self.errors['diff_path'] = diff_form.errors['path']
             elif 'base_diff_path' in diff_form.errors:
                 review_request.delete()
     """
     A form that handles uploading of new screenshots.
     A screenshot takes a path argument and optionally a caption.
     """
     caption = forms.CharField(required=False)
-    path = forms.CharField(widget=forms.FileInput())
+    path = forms.ImageField(required=True)
-    def create(self, data, review):
+    def create(self, review):
         screenshot = Screenshot(caption=self.cleaned_data['caption'],
                                 draft_caption=self.cleaned_data['caption'])
-        screenshot.save()
-        screenshot.save_image_file(data['filename'], data['content'])
-        try:
+        # This implies screenshot.save()
-            image = Image.open(screenshot.get_image_filename())
+        uploaded_file = self.cleaned_data['path']
-            image.load()
+        uploaded_file.data.open()
-        except:
+        screenshot.save_image_file(uploaded_file.filename, uploaded_file.data)
-            screenshot.delete()
-            raise ValueError('The file does not appear to be an image')
         draft = ReviewRequestDraft.create(review)
         review.screenshots.add(screenshot)
         draft.screenshots.add(screenshot)
         draft.save()
         return screenshot

     """
     caption = models.CharField(_("caption"), max_length=256, blank=True)
     draft_caption = models.CharField(_("draft caption"),
                                      max_length=256, blank=True)
     image = models.ImageField(_("image"),
-                              upload_to=os.path.join('uploaded', 'images'))
+                              upload_to=os.path.join('uploaded', 'images',
                                                      '%Y', '%m', '%d'))
     def thumb(self):
         """
         Creates a thumbnail of this screenshot and returns the HTML
         output embedding the thumbnail.

     if request.method == 'POST':
         form = NewReviewRequestForm(request.POST, request.FILES)
         if form.is_valid():
             try:
-                review_request = form.create(
+                review_request = form.create(request.user)
-                    user=request.user,
-                    diff_file=request.FILES['diff_path'],
-                    parent_diff_file=request.FILES.get('parent_diff_path'))
                 return HttpResponseRedirect(review_request.get_absolute_url())
             except:
                 # XXX - OwnershipError or ChangeSetError?
+                #
                 # We're preventing an exception from being thrown here so that

     if not form.is_valid():
         return WebAPIResponseFormError(request, form)
     try:
-        diffset = form.create(request.FILES['path'],
+        diffset = form.create()
-                              request.FILES.get('parent_diff_path'))
         # Set the initial revision to be one newer than the most recent
         # public revision, so we can reference it in the diff viewer.
+        #
         # TODO: It would be nice to later consolidate this with the logic in
     if not form.is_valid():
         return WebAPIResponseFormError(request, form)
     try:
-        screenshot = form.create(request.FILES['path'], review_request)
+        screenshot = form.create(review_request)
     except ValueError, e:
         return WebAPIResponseError(request, INVALID_FORM_DATA, {
             'fields': {
                 'path': [str(e)],
             },

         print "Response: %s" % rsp
         return rsp
     def apiPost(self, path, query={}):
         print "Posting to /api/json/%s/" % path
         print "Post data: %s" % query
         response = self.client.post("/api/json/%s/" % path, query)
         self.assertEqual(response.status_code, 200)
-        rsp = simplejson.loads(response.content)
+        rsp = simplejson.loads(response.

Review Board

beta

This change has been marked as submitted.

Improve file uploading on newer Djangos.

Diff revision 2 (Latest)