GNOME.org
 

[extensions-web] upload: Rework the upload code

commit 8d44e7e22282aa40cac63fb1cefd36d3633830c7
Author: Jasper St. Pierre <jstpierre mecheye net>
Date:   Mon Feb 27 17:43:56 2012 -0500

    upload: Rework the upload code
    
    First of all, use transactions. Second, refactor and split out the big
    mess that is parse_metadata_json, so we don't have to do any extraneous
    saving. Thirdly, fix and add new tests so that we're sure that this code
    is working. Fourthly, remove the dedicated "Upload a new version" page,
    as this just clutters the code up. Looking up an existing version by
    UUID should be more than enough.

 sweettooth/extensions/models.py                    |   51 +++++-----
 .../templates/extensions/detail_edit.html          |    2 +-
 sweettooth/extensions/tests.py                     |   98 +++++++-------------
 sweettooth/extensions/urls.py                      |    7 +-
 sweettooth/extensions/views.py                     |   62 ++++---------
 5 files changed, 80 insertions(+), 140 deletions(-)
---
diff --git a/sweettooth/extensions/models.py b/sweettooth/extensions/models.py
index 63042ef..b2c084b 100644
--- a/sweettooth/extensions/models.py
+++ b/sweettooth/extensions/models.py
@@ -45,6 +45,12 @@ class ExtensionManager(models.Manager):
     def visible(self):
         return self.filter(versions__status__in=VISIBLE_STATUSES).distinct()
 
+    def create_from_metadata(self, metadata, **kwargs):
+        instance = self.model(**kwargs)
+        instance.parse_metadata_json(metadata)
+        instance.save()
+        return instance
+
 def build_shell_version_map(versions):
     shell_version_map = {}
     for version in versions:
@@ -122,6 +128,12 @@ class Extension(models.Model):
         if not validate_uuid(self.uuid):
             raise ValidationError("Invalid UUID")
 
+    def parse_metadata_json(self, metadata):
+        self.name = metadata.pop('name', "")
+        self.description = metadata.pop('description', "")
+        self.url = metadata.pop('url', "")
+        self.uuid = metadata['uuid']
+
     def save(self, replace_metadata_json=True, *args, **kwargs):
         super(Extension, self).save(*args, **kwargs)
         if replace_metadata_json:
@@ -364,30 +376,9 @@ class ExtensionVersion(models.Model):
         zipfile.writestr("metadata.json", self.make_metadata_json_string())
         zipfile.close()
 
-    def parse_metadata_json(self, metadata):
-        """
-        Given the contents of a metadata.json file, fill in the fields
-        of the version and associated extension.
-        """
+    def save(self, *args, **kwargs):
         assert self.extension is not None
 
-        # Only parse the standard data for a new extension
-        if self.extension.pk is None:
-            self.extension.name = metadata.pop('name', "")
-            self.extension.description = metadata.pop('description', "")
-            self.extension.url = metadata.pop('url', "")
-            self.extension.uuid = metadata['uuid']
-            self.extension.save()
-
-            # Due to Django ORM magic and stupidity, this is unfortunately necessary
-            self.extension = self.extension
-
-        # FIXME: We shouldn't do this, but Django saving requires it.
-        if self.status is None:
-            self.status = STATUS_NEW
-
-        self.extra_json_fields = json.dumps(metadata)
-
         # get version number
         ver_ids = self.extension.versions.order_by('-version')
         try:
@@ -398,8 +389,18 @@ class ExtensionVersion(models.Model):
 
         self.version = ver_id
 
-        # ManyToManyField requires a PK, so we need to save.
-        self.save()
+        super(ExtensionVersion, self).save(*args, **kwargs)
+
+    def parse_metadata_json(self, metadata):
+        """
+        Given the contents of a metadata.json file, fill in the fields
+        of the version and associated extension.
+
+        NOTE: This needs to be called after this has been saved, as we
+        need a PK to be able to add ourselves to a PK.
+        """
+
+        self.extra_json_fields = json.dumps(metadata)
 
         for sv_string in metadata.pop('shell-version', []):
             try:
@@ -411,8 +412,6 @@ class ExtensionVersion(models.Model):
 
             self.shell_versions.add(sv)
 
-        self.save()
-
     def get_status_class(self):
         return STATUSES[self.status].lower()
 
diff --git a/sweettooth/extensions/templates/extensions/detail_edit.html b/sweettooth/extensions/templates/extensions/detail_edit.html
index 2c9774b..ea20319 100644
--- a/sweettooth/extensions/templates/extensions/detail_edit.html
+++ b/sweettooth/extensions/templates/extensions/detail_edit.html
@@ -45,7 +45,7 @@
   {% if not is_preview %}
     <h2 class="expandy_header expanded"> Admin </h2>
     <ul>
-      <li><a href="{% url extensions-upload-file pk=extension.pk %}">Upload a new version</a></li>
+      <li><a href="{% url extensions-upload-file %}">Upload a new version</a></li>
       {% if version %}
       <li><a href="{% url review-version pk=version.pk %}">View review and source</a></li>
       {% endif %}
diff --git a/sweettooth/extensions/tests.py b/sweettooth/extensions/tests.py
index 8262f17..30c9fb2 100644
--- a/sweettooth/extensions/tests.py
+++ b/sweettooth/extensions/tests.py
@@ -33,13 +33,13 @@ class BasicUserTestCase(object):
 
 class ParseZipfileTest(BasicUserTestCase, TestCase):
     def test_simple_metadata(self):
-        metadata = {"name": "Test Metadata",
+        metadata = {"uuid": "test-metadata mecheye net",
+                    "name": "Test Metadata",
                     "description": "Simple test metadata",
                     "url": "http://test-metadata.gnome.org"}
 
-        extension = models.Extension(creator=self.user)
-        version = models.ExtensionVersion()
-        version.extension = extension
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion(extension=extension)
         version.parse_metadata_json(metadata)
 
         self.assertEquals(extension.name, "Test Metadata")
@@ -47,12 +47,11 @@ class ParseZipfileTest(BasicUserTestCase, TestCase):
         self.assertEquals(extension.url, "http://test-metadata.gnome.org";)
 
     def test_simple_zipdata_data(self):
-        extension = models.Extension(creator=self.user)
-        version = models.ExtensionVersion()
-        version.extension = extension
-
         with get_test_zipfile('SimpleExtension') as f:
             metadata = models.parse_zipfile_metadata(f)
+
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion(extension=extension)
         version.parse_metadata_json(metadata)
 
         self.assertEquals(extension.uuid, "test-extension mecheye net")
@@ -61,12 +60,11 @@ class ParseZipfileTest(BasicUserTestCase, TestCase):
         self.assertEquals(extension.url, "http://test-metadata.gnome.org";)
 
     def test_extra_metadata(self):
-        extension = models.Extension(creator=self.user)
-        version = models.ExtensionVersion()
-        version.extension = extension
-
         with get_test_zipfile('ExtraMetadata') as f:
             metadata = models.parse_zipfile_metadata(f)
+
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion(extension=extension)
         version.parse_metadata_json(metadata)
 
         extra = json.loads(version.extra_json_fields)
@@ -77,19 +75,17 @@ class ParseZipfileTest(BasicUserTestCase, TestCase):
 
 class ReplaceMetadataTest(BasicUserTestCase, TestCase):
     def test_replace_metadata(self):
-        extension = models.Extension(creator=self.user)
-        version = models.ExtensionVersion()
-        version.extension = extension
-
         old_zip_file = get_test_zipfile('LotsOfFiles')
 
         metadata = models.parse_zipfile_metadata(old_zip_file)
         old_zip_file.seek(0)
 
-        version.source = File(old_zip_file)
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion(extension=extension,
+                                          source=File(old_zip_file))
 
         version.parse_metadata_json(metadata)
-        version.replace_metadata_json()
+
         new_zip = version.get_zipfile('r')
 
         old_zip = ZipFile(File(old_zip_file), 'r')
@@ -166,13 +162,9 @@ class ExtensionVersionTest(BasicUserTestCase, TestCase):
                     "description": "Simple test metadata",
                     "url": "http://test-metadata.gnome.org"}
 
-        extension = models.Extension(creator=self.user)
-        version = models.ExtensionVersion()
-        version.extension = extension
-        version.parse_metadata_json(metadata)
-
-        version.status = models.STATUS_ACTIVE
-        version.save()
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion.objects.create(extension=extension,
+                                                         status=models.STATUS_ACTIVE)
 
         self.assertEquals(version.version, 1)
         self.assertEquals(extension.latest_version, version)
@@ -183,20 +175,14 @@ class ExtensionVersionTest(BasicUserTestCase, TestCase):
                     "description": "Simple test metadata",
                     "url": "http://test-metadata.gnome.org"}
 
-        extension = models.Extension(creator=self.user)
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
 
-        v1 = models.ExtensionVersion()
-        v1.extension = extension
-        v1.status = models.STATUS_ACTIVE
-        v1.parse_metadata_json(metadata)
-        v1.save()
+        v1 = models.ExtensionVersion.objects.create(extension=extension,
+                                                    status=models.STATUS_ACTIVE)
         self.assertEquals(v1.version, 1)
 
-        v2 = models.ExtensionVersion()
-        v2.extension = extension
-        v2.status = models.STATUS_ACTIVE
-        v2.parse_metadata_json(metadata)
-        v2.save()
+        v2 = models.ExtensionVersion.objects.create(extension=extension,
+                                                    status=models.STATUS_ACTIVE)
         self.assertEquals(v2.version, 2)
 
         self.assertEquals(list(extension.visible_versions.order_by('version')), [v1, v2])
@@ -208,30 +194,21 @@ class ExtensionVersionTest(BasicUserTestCase, TestCase):
                     "description": "Simple test metadata",
                     "url": "http://test-metadata.gnome.org"}
 
-        extension = models.Extension(creator=self.user)
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
 
-        v1 = models.ExtensionVersion()
-        v1.extension = extension
-        v1.parse_metadata_json(metadata)
-        v1.status = models.STATUS_ACTIVE
-        v1.save()
+        v1 = models.ExtensionVersion.objects.create(extension=extension,
+                                                    status=models.STATUS_ACTIVE)
         self.assertEquals(v1.version, 1)
 
-        v2 = models.ExtensionVersion()
-        v2.extension = extension
-        v2.status = models.STATUS_NEW
-        v2.parse_metadata_json(metadata)
-        v2.save()
+        v2 = models.ExtensionVersion.objects.create(extension=extension,
+                                                    status=models.STATUS_NEW)
         self.assertEquals(v2.version, 2)
 
         self.assertEquals(list(extension.visible_versions.order_by('version')), [v1])
         self.assertEquals(extension.latest_version, v1)
 
-        v3 = models.ExtensionVersion()
-        v3.extension = extension
-        v3.status = models.STATUS_ACTIVE
-        v3.parse_metadata_json(metadata)
-        v3.save()
+        v3 = models.ExtensionVersion.objects.create(extension=extension,
+                                                    status=models.STATUS_ACTIVE)
         self.assertEquals(v3.version, 3)
 
         self.assertEquals(list(extension.visible_versions.order_by('version')), [v1, v3])
@@ -244,14 +221,11 @@ class ExtensionVersionTest(BasicUserTestCase, TestCase):
                     "url": "http://test-metadata.gnome.org";,
                     "shell-version": ["3.0.0", "3.0.1", "3.0.2"]}
 
-        extension = models.Extension(creator=self.user)
-
-        version = models.ExtensionVersion()
-        version.extension = extension
-        version.status = models.STATUS_ACTIVE
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version = models.ExtensionVersion.objects.create(extension=extension,
+                                                         status=models.STATUS_ACTIVE)
         version.parse_metadata_json(metadata)
 
-        version.save()
         shell_versions = sorted(sv.version_string for sv in version.shell_versions.all())
         self.assertEquals(shell_versions, ["3.0.0", "3.0.1", "3.0.2"])
 
@@ -262,14 +236,12 @@ class ExtensionVersionTest(BasicUserTestCase, TestCase):
                     "url": "http://test-metadata.gnome.org";,
                     "shell-version": ["3.0", "3.2"]}
 
-        extension = models.Extension(creator=self.user)
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
 
-        version = models.ExtensionVersion()
-        version.extension = extension
-        version.status = models.STATUS_ACTIVE
+        version = models.ExtensionVersion.objects.create(extension=extension,
+                                                         status=models.STATUS_ACTIVE)
         version.parse_metadata_json(metadata)
 
-        version.save()
         shell_versions = sorted(sv.version_string for sv in version.shell_versions.all())
         self.assertEquals(shell_versions, ["3.0", "3.2"])
 
diff --git a/sweettooth/extensions/urls.py b/sweettooth/extensions/urls.py
index 6dc7214..88111b8 100644
--- a/sweettooth/extensions/urls.py
+++ b/sweettooth/extensions/urls.py
@@ -4,11 +4,6 @@ from django.views.generic.simple import direct_to_template
 
 from extensions import views, models, feeds
 
-upload_patterns = patterns('',
-    url(r'^$', views.upload_file, dict(pk=None), name='extensions-upload-file'),
-    url(r'^new-version/(?P<pk>\d+)/$', views.upload_file, name='extensions-upload-file'),
-)
-
 ajax_patterns = patterns('',
     url(r'^edit/(?P<pk>\d+)', views.ajax_inline_edit_view, name='extensions-ajax-inline'),
     url(r'^submit/(?P<pk>\d+)', views.ajax_submit_and_lock_view, name='extensions-ajax-submit'),
@@ -53,7 +48,7 @@ urlpatterns = patterns('',
 
     url(r'^rss/', feeds.LatestExtensionsFeed(), name='extensions-rss-feed'),
 
-    url(r'^upload/', include(upload_patterns)),
+    url(r'^upload/', views.upload_file, name='extensions-upload-file'),
     url(r'^ajax/', include(ajax_patterns)),
     url(r'', include(shell_patterns)),
 )
diff --git a/sweettooth/extensions/views.py b/sweettooth/extensions/views.py
index 8db311b..524a2b2 100644
--- a/sweettooth/extensions/views.py
+++ b/sweettooth/extensions/views.py
@@ -4,6 +4,7 @@ from django.core.paginator import Paginator, InvalidPage
 from django.core.urlresolvers import reverse
 from django.contrib.auth.decorators import login_required
 from django.contrib import messages
+from django.db import transaction
 from django.http import HttpResponseForbidden, HttpResponseServerError, Http404
 from django.shortcuts import get_object_or_404, redirect
 from django.template.loader import render_to_string
@@ -341,16 +342,8 @@ def ajax_set_status_view(request, newstatus):
                 mvs=render_to_string('extensions/multiversion_status.html', context))
 
 @login_required
-def upload_file(request, pk):
-    if pk is None:
-        extension = models.Extension(creator=request.user)
-        extension_is_new = True
-    else:
-        extension = models.Extension.objects.get(pk=pk)
-        extension_is_new = False
-        if extension.creator != request.user:
-            return HttpResponseForbidden()
-
+ transaction commit_manually
+def upload_file(request):
     errors = []
     extra_debug = None
 
@@ -359,39 +352,27 @@ def upload_file(request, pk):
         if form.is_valid():
             file_source = form.cleaned_data['source']
 
+            sid = transaction.savepoint()
+
             try:
                 metadata = models.parse_zipfile_metadata(file_source)
                 uuid = metadata['uuid']
             except (models.InvalidExtensionData, KeyError), e:
                 messages.error(request, "Invalid extension data: %s" % (e.message,))
+                return redirect('extensions-upload-file')
 
-                if pk is not None:
-                    return redirect('extensions-upload-file', pk=pk)
-                else:
-                    return redirect('extensions-upload-file')
-
-            existing = models.Extension.objects.filter(uuid=uuid)
-            if pk is None and existing.exists():
-                # Error out if we already have an extension with the same
-                # uuid -- or correct their mistake if they're the same user.
-                ext = existing.get()
-                if request.user == ext.creator:
-                    return upload_file(request, ext.pk)
-                else:
+            try:
+                extension = models.Extension.objects.get(uuid=uuid)
+            except models.Extension.DoesNotExist:
+                extension = models.Extension.objects.create_from_metadata(metadata, creator=request.user)
+            else:
+                if request.user != extension.creator:
                     messages.error(request, "An extension with that UUID has already been added.")
                     return redirect('extensions-upload-file')
 
-            version = models.ExtensionVersion()
-            version.extension = extension
-            version.parse_metadata_json(metadata)
-
-            extension.creator = request.user
-
             try:
                 extension.full_clean()
             except ValidationError, e:
-                is_valid = False
-
                 # Output a specialized error message for a common mistake:
                 if getattr(e, 'message_dict', None) and 'url' in e.message_dict:
                     errors = [mark_safe("You have an invalid URL. Make sure your URL "
@@ -399,22 +380,15 @@ def upload_file(request, pk):
                 else:
                     errors = e.messages
 
-                version.delete()
-                if extension_is_new:
-                    extension.delete()
-
                 extra_debug = repr(e)
+                transaction.savepoint_rollback(sid)
             else:
-                is_valid = True
-
-            if is_valid:
-                extension.save()
-
-                version.extension = extension
-                version.source = file_source
-                version.status = models.STATUS_NEW
-                version.save()
+                transaction.savepoint_commit(sid)
 
+                version = models.ExtensionVersion.objects.create(extension=extension,
+                                                                 source=file_source,
+                                                                 status=models.STATUS_NEW)
+                version.parse_metadata_json(metadata)
                 version.replace_metadata_json()
 
                 return redirect('extensions-version-detail',
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]