[extensions-web] review: Exclude botched uploads when trying to grab diffs

[Jasper St. Pierre <jstpierre src gnome org>]

commit 7736ae3140d3a6801915fa7863205b36559a1339
Author: Jasper St. Pierre <jstpierre mecheye net>
Date:   Fri Mar 2 23:18:56 2012 -0500

    review: Exclude botched uploads when trying to grab diffs
    
    In some cases, extension authors can "corrupt" an extension by uploading
    something that causes the server to 500 out. In this case, an admin usually
    has to delete the extension manually and make the author re-upload. While
    we should fix the cause of the 500, let's try and not "corrupt" the extension
    by looking for something good to diff against.

 sweettooth/extensions/tests.py |   13 ++-----------
 sweettooth/review/tests.py     |   29 +++++++++++++++++++++++++++++
 sweettooth/review/views.py     |   33 +++++++++++++++++++--------------
 sweettooth/testutils.py        |   12 ++++++++++++
 4 files changed, 62 insertions(+), 25 deletions(-)
---
diff --git a/sweettooth/extensions/tests.py b/sweettooth/extensions/tests.py
index 9c2f197..e0dfb45 100644
--- a/sweettooth/extensions/tests.py
+++ b/sweettooth/extensions/tests.py
@@ -6,11 +6,12 @@ from zipfile import ZipFile
 from django.test import TestCase
 from django.core.files.base import File
 from django.core.urlresolvers import reverse
-from django.contrib.auth.models import User
 from django.utils import simplejson as json
 from django.utils.unittest import expectedFailure
 from extensions import models
 
+from testutils import BasicUserTestCase
+
 testdata_dir = os.path.join(os.path.dirname(__file__), 'testdata')
 
 def get_test_zipfile(testname):
@@ -22,16 +23,6 @@ def get_test_zipfile(testname):
 
     return new_temp
 
-class BasicUserTestCase(object):
-    def setUp(self):
-        super(BasicUserTestCase, self).setUp()
-        self.username = 'TestUser1'
-        self.email = 'non-existant non-existant tld'
-        self.password = 'a random password'
-        self.user = User.objects.create_user(self.username, self.email, self.password)
-
-        self.client.login(username=self.username, password=self.password)
-
 class ParseZipfileTest(BasicUserTestCase, TestCase):
     def test_simple_metadata(self):
         metadata = {"uuid": "test-metadata mecheye net",
diff --git a/sweettooth/review/tests.py b/sweettooth/review/tests.py
new file mode 100644
index 0000000..7b29abb
--- /dev/null
+++ b/sweettooth/review/tests.py
@@ -0,0 +1,29 @@
+
+from django.test import TestCase
+from django.core.files.base import File, ContentFile, StringIO
+
+from extensions import models
+from review.views import get_old_version
+
+from testutils import BasicUserTestCase
+
+class DiffViewTest(BasicUserTestCase, TestCase):
+    def test_get_zipfiles(self):
+        metadata = {"uuid": "test-metadata mecheye net",
+                    "name": "Test Metadata"}
+
+        extension = models.Extension.objects.create_from_metadata(metadata, creator=self.user)
+        version1 = models.ExtensionVersion.objects.create(extension=extension,
+                                                          source=File(ContentFile("doot doo"), name="aa"),
+                                                          status=models.STATUS_NEW)
+
+        # This one is broken...
+        version2 = models.ExtensionVersion.objects.create(extension=extension,
+                                                          source="",
+                                                          status=models.STATUS_NEW)
+
+        version3 = models.ExtensionVersion.objects.create(extension=extension,
+                                                          source=File(ContentFile("doot doo"), name="bb"),
+                                                          status=models.STATUS_NEW)
+
+        self.assertEquals(version1, get_old_version(version3, None))
diff --git a/sweettooth/review/views.py b/sweettooth/review/views.py
index f6f3f23..de648e8 100644
--- a/sweettooth/review/views.py
+++ b/sweettooth/review/views.py
@@ -89,20 +89,28 @@ def html_for_file(filename, raw):
     else:
         return dict(raw=False, lines=split_lines(highlight_file(filename, raw, code_formatter)))
 
-def get_zipfiles(version, old_version_number=None):
+def get_old_version(version, old_version_number):
     extension = version.extension
 
-    new_zipfile = version.get_zipfile('r')
-    if version.version == 1:
-        return None, new_zipfile
+    if old_version_number is not None:
+        old_version = extension.versions.get(version=old_version_number)
+    else:
+        # Try to get the latest version that's less than the current version
+        # that actually has a source field. Sometimes the upload validation
+        # fails, so work around it here.
+        old_version = extension.versions.filter(version__lt=version.version).exclude(source="").latest()
 
-    if old_version_number is None:
-        old_version_number = version.version - 1
+    return old_version
 
-    old_version = extension.versions.get(version=old_version_number)
-    old_zipfile = old_version.get_zipfile('r')
+def get_zipfiles(version, old_version_number):
+    new_zipfile = version.get_zipfile('r')
+    old_version = get_old_version(version, old_version_number)
 
-    return old_zipfile, new_zipfile
+    if old_version is None:
+        return None, new_zipfile
+    else:
+        old_zipfile = old_version.get_zipfile('r')
+        return old_zipfile, new_zipfile
 
 def get_diff(old_zipfile, new_zipfile, filename):
     old, new = old_zipfile.open(filename, 'r'), new_zipfile.open(filename, 'r')
@@ -144,9 +152,7 @@ def get_filelist(zipfile):
 def ajax_get_file_list_view(request, obj):
     version, extension = obj, obj.extension
 
-    old_version_number = request.GET.get('oldver', None)
-
-    old_zipfile, new_zipfile = get_zipfiles(version, old_version_number)
+    old_zipfile, new_zipfile = get_zipfiles(version, request.GET.get('oldver', None))
 
     new_filelist = get_filelist(new_zipfile)
 
@@ -185,7 +191,6 @@ def ajax_get_file_diff_view(request, obj):
     version, extension = obj, obj.extension
 
     filename = request.GET['filename']
-    old_version_number = request.GET.get('oldver', None)
 
     file_base, file_extension = os.path.splitext(filename)
     if file_extension in IMAGE_TYPES:
@@ -194,7 +199,7 @@ def ajax_get_file_diff_view(request, obj):
     if file_extension in BINARY_TYPES:
         return None
 
-    old_zipfile, new_zipfile = get_zipfiles(version, old_version_number)
+    old_zipfile, new_zipfile = get_zipfiles(version, request.GET.get('oldver', None))
 
     new_filelist = set(new_zipfile.namelist())
     old_filelist = set(old_zipfile.namelist())
diff --git a/sweettooth/testutils.py b/sweettooth/testutils.py
new file mode 100644
index 0000000..57dc2c8
--- /dev/null
+++ b/sweettooth/testutils.py
@@ -0,0 +1,12 @@
+
+from django.contrib.auth.models import User
+
+class BasicUserTestCase(object):
+    def setUp(self):
+        super(BasicUserTestCase, self).setUp()
+        self.username = 'TestUser1'
+        self.email = 'non-existant non-existant tld'
+        self.password = 'a random password'
+        self.user = User.objects.create_user(self.username, self.email, self.password)
+
+        self.client.login(username=self.username, password=self.password)