More robustness in Attachments plugin. Give a good error message upon…

More robustness in Attachments plugin. Give a good error message upon non-allowed uploads. Fix JSON decorator error. Ignore media files in test project.

testproject/testproject/.gitignore

-./media/*
+media/*

wiki/decorators.py

@@ -3,19 +3,16 @@ from django.core.urlresolvers import reverse
 from django.shortcuts import redirect, get_object_or_404
 from django.http import HttpResponse, HttpResponseForbidden,\
     HttpResponseNotFound
+from django.utils import simplejson as json
 
 from wiki.core.exceptions import NoRootURL
-from django.core import serializers
-
-JSONSerializer = serializers.get_serializer("json")
 
 def json_view(func):
-    def wrap(request, *a, **kw):
-        obj = func(request, *a, **kw)
-        json_serializer = JSONSerializer()
-        json_serializer.serialize(obj)
-        data = json_serializer.getvalue()
-        response = HttpResponse(mimetype='application/json')
+    def wrap(request, *args, **kwargs):
+        obj = func(request, *args, **kwargs)
+        data = json.dumps(obj, ensure_ascii=False)
+        status = kwargs.get('status', 200)
+        response = HttpResponse(mimetype='application/json', status=status)
         response.write(data)
         return response
     return wrap

wiki/forms.py

@@ -17,14 +17,14 @@ class CreateRoot(forms.Form):
     title = forms.CharField(label=_(u'Title'), help_text=_(u'Initial title of the article. May be overridden with revision titles.'))
     content = forms.CharField(label=_(u'Type in some contents'),
                               help_text=_(u'This is just the initial contents of your article. After creating it, you can use more complex features like adding plugins, meta data, related articles etc...'),
-                              required=False, widget=editor.get_widget())
+                              required=False, widget=editor.get_widget()) #@UndefinedVariable
     
 
 class EditForm(forms.Form):
     
     title = forms.CharField(label=_(u'Title'),)
     content = forms.CharField(label=_(u'Contents'),
-                              required=False, widget=editor.get_widget())
+                              required=False, widget=editor.get_widget()) #@UndefinedVariable
     
     summary = forms.CharField(label=_(u'Summary'), help_text=_(u'Give a short reason for your edit, which will be stated in the revision log.'),
                               required=False)
@@ -171,15 +171,12 @@ class CreateForm(forms.Form):
     
     def __init__(self, urlpath_parent, *args, **kwargs):
         super(CreateForm, self).__init__(*args, **kwargs)
-        # Todo: Don't change the widget in the default form, do it in
-        # a class based view...
-        self.fields['slug'].widget = TextInputPrepend(prepend='/'+urlpath_parent.path)
         self.urlpath_parent = urlpath_parent
     
     title = forms.CharField(label=_(u'Title'),)
     slug = forms.SlugField(label=_(u'Slug'), help_text=_(u"This will be the address where your article can be found. Use only alphanumeric characters and '-' or '_'."),)
     content = forms.CharField(label=_(u'Contents'),
-                              required=False, widget=editor.get_widget())
+                              required=False, widget=editor.get_widget()) #@UndefinedVariable
     
     summary = forms.CharField(label=_(u'Summary'), help_text=_(u"Write a brief message for the article's history log."),
                               required=False)

wiki/models/article.py

@@ -236,6 +236,14 @@ class ArticleRevision(BaseRevision):
         self.redirect = predecessor.redirect
     
     def save(self, *args, **kwargs):
+        if (not self.id and
+            not self.previous_revision and 
+            self.attachment and
+            self.attachment.current_revision and 
+            self.attachment.current_revision != self):
+            
+            self.previous_revision = self.attachment.current_revision
+
         if not self.revision_number:
             try:
                 previous_revision = self.article.articlerevision_set.latest()
@@ -243,9 +251,6 @@ class ArticleRevision(BaseRevision):
             except ArticleRevision.DoesNotExist:
                 self.revision_number = 1
 
-        if not self.previous_revision and self.article.current_revision != self:
-            self.previous_revision = self.article.current_revision
-
         super(ArticleRevision, self).save(*args, **kwargs)
         
         if not self.article.current_revision:

wiki/plugins/attachments/models.py

@@ -30,6 +30,9 @@ class Attachment(ReusablePlugin):
         verbose_name = _(u'attachment')
         verbose_name_plural = _(u'attachments')
         app_label = wiki_settings.APP_LABEL
+    
+    def __unicode__(self):
+        return "%s: %s" % (self.article.current_revision.title, self.original_filename)    
         
 def upload_path(instance, filename):
     from os import path
@@ -41,8 +44,8 @@ def upload_path(instance, filename):
     
     # Must be an allowed extension
     if not extension.lower() in map(lambda x: x.lower(), settings.FILE_EXTENSIONS):
-        raise IllegalFileExtension("The following filename is illegal: %s. Extension has to be on of %s" % 
-                                   (filename, str(settings.FILE_EXTENSIONS)))
+        raise IllegalFileExtension("The following filename is illegal: %s. Extension has to be one of %s" % 
+                                   (filename, ", ".join(settings.FILE_EXTENSIONS)))
 
     # Has to match original extension filename
     if instance.id and instance.attachment and instance.attachment.original_filename:
@@ -78,26 +81,47 @@ class AttachmentRevision(BaseRevision):
         app_label = wiki_settings.APP_LABEL
         
     def get_filename(self):
+        """Used to retrieve the filename of a revision.
+        But attachment.original_filename should always be used in the frontend
+        such that filenames stay consistent."""
+        # TODO: Perhaps we can let file names change when files are replaced?
         if not self.file:
             return None
         filename = self.file.path.split("/")[-1]
         return ".".join(filename.split(".")[:-1])
-
+    
+    def get_size(self):
+        """Used to retrieve the file size and not cause exceptions."""
+        try:
+            return self.file.size
+        except ValueError:
+            return None
+    
     def save(self, *args, **kwargs):
+        if (not self.id and
+            not self.previous_revision and 
+            self.attachment and
+            self.attachment.current_revision and 
+            self.attachment.current_revision != self):
+            
+            self.previous_revision = self.attachment.current_revision
+
         if not self.revision_number:
             try:
                 previous_revision = self.attachment.attachmentrevision_set.latest()
                 self.revision_number = previous_revision.revision_number + 1
             # NB! The above should not raise the below exception, but somehow it does.
-            except Attachment.DoesNotExist:
+            except AttachmentRevision.DoesNotExist, Attachment.DoesNotExist:
                 self.revision_number = 1
         
-        if not self.previous_revision and self.attachment.current_revision != self:
-            self.previous_revision = self.attachment.current_revision
-
         super(AttachmentRevision, self).save(*args, **kwargs)
         
         if not self.attachment.current_revision:
             # If I'm saved from Django admin, then article.current_revision is me!
             self.attachment.current_revision = self
             self.attachment.save()
+
+    def __unicode__(self):
+        return "%s: %s (r%d)" % (self.attachment.article.current_revision.title, 
+                                 self.attachment.original_filename,
+                                 self.revision_number)    

wiki/plugins/attachments/settings.py

@@ -2,7 +2,14 @@ from django.conf import settings as django_settings
 
 SLUG = "attachments"
 
+# Maximum file sizes: Please using something like LimitRequestBody on
+# your web server.
+# http://httpd.apache.org/docs/2.2/mod/core.html#LimitRequestBody
+
 # Where to store article attachments, relative to MEDIA_ROOT
+# You should NEVER enable directory indexing in MEDIA_ROOT/UPLOAD_PATH !
+# Actually, you can completely disable serving it, if you want. Files are
+# sent to the user through a Django view that reads and streams a file.
 UPLOAD_PATH = getattr(django_settings, 'WIKI_UPLOAD_PATH', 'wiki/uploads/%aid/')
 
 # Should the upload path be obscurified? If so, a random hash will be added to the path
@@ -10,8 +17,10 @@ UPLOAD_PATH = getattr(django_settings, 'WIKI_UPLOAD_PATH', 'wiki/uploads/%aid/')
 # restricted permissions and the files are still located within the web server's
 UPLOAD_PATH_OBSCURIFY = getattr(django_settings, 'WIKI_UPLOAD_PATH_OBSCURIFY', True)
 
-# Allowed non-image extensions. Empty to disallow completely.
+# Allowed extensions. Empty to disallow uploads completely.
 # No files are saved without appending ".upload" to the file to ensure that
 # your web server never actually executes some script.
 # Case insensitive.
+# You are asked to explicitly enter all file extensions that you want
+# to allow. For your own safety.
 FILE_EXTENSIONS = getattr(django_settings, 'WIKI_FILE_EXTENSIONS', ['pdf', 'doc', 'odt', 'docx', 'txt'])

wiki/plugins/attachments/templates/wiki/plugins/attachments/index.html

@@ -35,7 +35,7 @@
                   <h3>
                     <a href="{% url 'wiki:attachments_download' path=urlpath.path attachment_id=attachment.id %}">{{ attachment.current_revision.get_filename }}</a>
                     <span class="badge">{{ attachment.current_revision.created|naturaltime }}</span>
-                    <span class="badge badge-info">{{ attachment.current_revision.file.size|filesizeformat }}</span>
+                    <span class="badge badge-info">{{ attachment.current_revision.get_size|filesizeformat }}</span>
                     {% if attachment.current_revision.deleted %}
                     <span class="badge badge-important">{% trans "deleted" %}</span>
                     {% endif %}
@@ -63,6 +63,7 @@
                         <a href="{% url 'wiki:attachments_delete' path=urlpath.path attachment_id=attachment.id %}" class="btn"><span class="icon-minus-sign"></span> {% trans "Detach" %}</a>
                       {% endif %}
                     {% else %}
+                      {% if attachment.current_revision.previous_revision.id %}
                       <form method="POST" action="{% url 'wiki:attachments_revision_change' path=urlpath.path attachment_id=attachment.id revision_id=attachment.current_revision.previous_revision.id %}">
                         {% csrf_token %}
                         <button class="btn">
@@ -70,6 +71,7 @@
                           {% trans "Restore" %}
                         </button>
                       </form>
+                      {% endif %}
                     {% endif %}
                   {% else %}
                     <em>{% trans "none" %}</em>

wiki/plugins/attachments/views.py

@@ -8,11 +8,11 @@ from wiki.views.mixins import ArticleMixin
 from wiki.decorators import get_article
 from wiki.plugins.attachments import forms
 from wiki.plugins.attachments import models
-from wiki.plugins.attachments import settings
 from django.contrib import messages
 from django.views.generic.base import TemplateView, View
 from wiki.core.http import send_file
 from django.http import Http404
+from django.db import transaction
 
 class AttachmentView(ArticleMixin, FormView):
     
@@ -27,21 +27,28 @@ class AttachmentView(ArticleMixin, FormView):
             self.attachments = models.Attachment.active_objects.filter(articles=article)
         return super(AttachmentView, self).dispatch(request, article, *args, **kwargs)
     
+    @transaction.commit_manually
     def form_valid(self, form):
         
-        attachment_revision = form.save(commit=False)
-        attachment = models.Attachment()
-        attachment.article = self.article
-        attachment.original_filename = attachment_revision.get_filename()
-        attachment.save()
-        attachment.articles.add(self.article)
-        attachment_revision.attachment = attachment
-        attachment_revision.set_from_request(self.request)
-        attachment_revision.save()
+        try:
+            attachment_revision = form.save(commit=False)
+            attachment = models.Attachment()
+            attachment.article = self.article
+            attachment.original_filename = attachment_revision.get_filename()
+            attachment.save()
+            attachment.articles.add(self.article)
+            attachment_revision.attachment = attachment
+            attachment_revision.set_from_request(self.request)
+            attachment_revision.save()
+            transaction.commit()
+            messages.success(self.request, _(u'%s was successfully added.') % attachment_revision.get_filename())
+        except models.IllegalFileExtension, e:
+            transaction.rollback()
+            messages.error(self.request, _(u'Your file could not be saved: %s') % e)
         
-        messages.success(self.request, _(u'%s was successfully added.') % attachment_revision.get_filename())
-        
-        return redirect("wiki:plugin_url", self.urlpath.path, settings.SLUG)
+        if self.urlpath:
+            return redirect("wiki:attachments_index", self.urlpath.path)
+        # TODO: What if no urlpath?        
     
     def get_context_data(self, **kwargs):
         kwargs['attachments'] = self.attachments
@@ -87,7 +94,7 @@ class AttachmentReplaceView(ArticleMixin, FormView):
         
         messages.success(self.request, _(u'%s uploaded and replaces old attachment.') % attachment_revision.get_filename())
         if self.urlpath:
-            return redirect("wiki:attachments_index", self.urlpath.path, settings.SLUG)
+            return redirect("wiki:attachments_index", self.urlpath.path)
         # TODO: What if we do not have a urlpath?
         
     
@@ -155,7 +162,10 @@ class AttachmentDeleteView(ArticleMixin, FormView):
     
     @method_decorator(get_article(can_write=True))
     def dispatch(self, request, article, attachment_id, *args, **kwargs):
-        self.attachment = get_object_or_404(models.Attachment.active_objects, id=attachment_id, articles=article)
+        if request.user.has_perm("wiki.moderator"):
+            self.attachment = get_object_or_404(models.Attachment, id=attachment_id, articles=article)
+        else:
+            self.attachment = get_object_or_404(models.Attachment.active_objects, id=attachment_id, articles=article)
         return super(AttachmentDeleteView, self).dispatch(request, article, *args, **kwargs)
     
     def form_valid(self, form):
@@ -165,8 +175,8 @@ class AttachmentDeleteView(ArticleMixin, FormView):
             revision.attachment = self.attachment
             revision.set_from_request(self.request)
             revision.deleted = True
-            revision.file = self.attachment.current_revision.file
-            revision.description = self.attachment.current_revision.description
+            revision.file = self.attachment.current_revision.file if self.attachment.current_revision else None
+            revision.description = self.attachment.current_revision.description if self.attachment.current_revision else ""
             revision.save()
             self.attachment.current_revision = revision
             self.attachment.save()

wiki/views/article.py

@@ -44,7 +44,9 @@ class Create(FormView, ArticleMixin):
         initial = kwargs.get('initial', {})
         initial['slug'] = self.request.GET.get('slug', None)
         kwargs['initial'] = initial
-        return form_class(self.urlpath, **kwargs)
+        form = form_class(self.urlpath, **kwargs)
+        form.fields['slug'].widget = forms.TextInputPrepend(prepend='/'+self.urlpath.path)        
+        return form
     
     def form_valid(self, form):
         user=None