Merge pull request #3332 from diox/extensions-versions-make-old-pendi…

…ng-obsolete

When uploading a new FxOS Add-on version, make older pending versions obsolete (bug 1203594)

mkt/extensions/models.py

@@ -15,8 +15,9 @@
 from uuidfield.fields import UUIDField
 
 from lib.crypto.packaged import sign_app, SigningError
-from mkt.constants.base import (STATUS_CHOICES, STATUS_NULL, STATUS_PENDING,
-                                STATUS_PUBLIC, STATUS_REJECTED)
+from mkt.constants.base import (MKT_STATUS_FILE_CHOICES, STATUS_DISABLED,
+                                STATUS_NULL, STATUS_PENDING, STATUS_PUBLIC,
+                                STATUS_REJECTED)
 from mkt.extensions.indexers import ExtensionIndexer
 from mkt.extensions.utils import ExtensionParser
 from mkt.files.models import cleanup_file, nfd_str
@@ -40,11 +41,20 @@ def public(self):
         return self.filter(status=STATUS_PUBLIC).order_by('id')
 
 
+class ExtensionVersionManager(ManagerBase):
+    def pending(self):
+        return self.filter(status=STATUS_PENDING).order_by('id')
+
+    def public(self):
+        return self.filter(status=STATUS_PUBLIC).order_by('id')
+
+
 class Extension(ModelBase):
     # Automatically handled fields.
     uuid = UUIDField(auto=True)
     status = models.PositiveSmallIntegerField(
-        choices=STATUS_CHOICES.items(), db_index=True, default=STATUS_NULL)
+        choices=MKT_STATUS_FILE_CHOICES.items(), db_index=True,
+        default=STATUS_NULL)
 
     # Fields for which the manifest is the source of truth - can't be
     # overridden by the API.
@@ -61,7 +71,7 @@ class Extension(ModelBase):
 
     @cached_property(writable=True)
     def latest_public_version(self):
-        return self.versions.filter(status=STATUS_PUBLIC).latest('pk')
+        return self.versions.public().latest('pk')
 
     @cached_property(writable=True)
     def latest_version(self):
@@ -186,7 +196,10 @@ class ExtensionVersion(ModelBase):
     manifest = JSONField()
     version = models.CharField(max_length=23, default='')
     status = models.PositiveSmallIntegerField(
-        choices=STATUS_CHOICES.items(), db_index=True, default=STATUS_NULL)
+        choices=MKT_STATUS_FILE_CHOICES.items(), db_index=True,
+        default=STATUS_NULL)
+
+    objects = ExtensionVersionManager()
 
     class Meta:
         unique_together = (('extension', 'version'),)
@@ -247,10 +260,14 @@ def from_upload(cls, upload, parent=None, parsed_data=None):
             raise ValidationError(
                 _('An extension with this version number already exists.'))
 
-        # Now that the instance has been saved, we can generate a file path,
-        # move the file and set it to PENDING. That should also set the status
-        # on the parent.
+        # Now that the instance has been saved, we can generate a file path
+        # and move the file.
         instance.handle_file_operations(upload)
+
+        # Now that the file is there, all that's left is making older pending
+        # versions obsolete and setting this one as pending. That should also
+        # set the status # on the parent.
+        instance.set_older_pending_versions_as_obsolete()
         instance.update(status=STATUS_PENDING)
         return instance
 
@@ -285,6 +302,22 @@ def remove_signed_file(self):
         if public_storage.exists(self.signed_file_path):
             public_storage.delete(self.signed_file_path)
 
+    def set_older_pending_versions_as_obsolete(self):
+        """Set all pending versions older than this one attached to the same
+        Extension as DISABLED (obsolete).
+
+        To be on the safe side this method does not trigger signals and needs
+        to be called when creating a new pending version, before actually
+        changing its status to PENDING. That way we avoid having extra versions
+        laying around when we automatically update the status on the parent
+        Extension."""
+        qs = self.__class__.objects.pending().filter(
+            extension=self.extension, pk__lt=self.pk)
+
+        # Call <queryset>.update() directly, bypassing signals etc, that should
+        # not be needed since it should be followed by a self.save().
+        qs.update(status=STATUS_DISABLED)
+
     def sign_and_move_file(self):
         """Sign and move extension file from the unsigned path (`file_path`) on
         private storage to the signed path (`signed_file_path`) on public
@@ -352,7 +385,6 @@ def delete_search_index(sender, instance, **kw):
 @receiver([models.signals.post_delete, models.signals.post_save],
           sender=ExtensionVersion, dispatch_uid='extension_version_change')
 def update_extension_status(sender, instance, **kw):
-    # FIXME: should render obsolete older pending uploads as well.
     instance.extension.update_status_according_to_versions()
 
 

mkt/extensions/tests/test_models.py

@@ -9,8 +9,8 @@
 from django.test.utils import override_settings
 
 from lib.crypto.packaged import SigningError
-from mkt.constants.base import (STATUS_NULL, STATUS_PENDING, STATUS_PUBLIC,
-                                STATUS_REJECTED)
+from mkt.constants.base import (STATUS_DISABLED, STATUS_NULL, STATUS_PENDING,
+                                STATUS_PUBLIC, STATUS_REJECTED)
 from mkt.extensions.models import Extension, ExtensionVersion
 from mkt.files.tests.test_models import UploadCreationMixin, UploadTest
 from mkt.site.storage_utils import private_storage
@@ -113,6 +113,41 @@ def test_upload_new_version(self):
         eq_(version.manifest, self.expected_manifest)
         eq_(version.status, STATUS_PENDING)
 
+    def test_upload_new_version_existing_pending_are_rendered_obsolete(self):
+        extension = Extension.objects.create()
+        older_version = ExtensionVersion.objects.create(
+            extension=extension, version='0.0.0', status=STATUS_PENDING)
+        old_version = ExtensionVersion.objects.create(
+            extension=extension, version='0.0', status=STATUS_PENDING)
+        eq_(extension.latest_version, old_version)
+        eq_(extension.status, STATUS_PENDING)
+        upload = self.upload('extension')
+        # Instead of calling Extension.from_upload(), we need to call
+        # ExtensionVersion.from_upload() directly, since an Extension already
+        # exists.
+        version = ExtensionVersion.from_upload(upload, parent=extension)
+
+        eq_(extension.latest_version, version)
+        eq_(extension.status, STATUS_PENDING)
+        eq_(version.status, STATUS_PENDING)
+        old_version.reload()
+        older_version.reload()
+        eq_(old_version.status, STATUS_DISABLED)
+        eq_(older_version.status, STATUS_DISABLED)
+
+    def test_upload_new_version_other_extension_are_not_affected(self):
+        other_extension = Extension.objects.create()
+        other_version = ExtensionVersion.objects.create(
+            extension=other_extension, version='0.0', status=STATUS_PENDING)
+        eq_(other_extension.status, STATUS_PENDING)
+        eq_(other_version.status, STATUS_PENDING)
+        self.test_upload_new_version_existing_pending_are_rendered_obsolete()
+        other_extension.reload()
+        other_version.reload()
+        # other_extension and other_version should not have been affected.
+        eq_(other_extension.status, STATUS_PENDING)
+        eq_(other_version.status, STATUS_PENDING)
+
     def test_upload_new_version_existing_version(self):
         extension = Extension.objects.create()
         ExtensionVersion.objects.create(extension=extension, version='0.1')
@@ -514,6 +549,12 @@ def test_reject(self, mocked_remove_signed_file):
 
 
 class TestExtensionManager(TestCase):
+    def test_public(self):
+        extension1 = Extension.objects.create(status=STATUS_PUBLIC)
+        extension2 = Extension.objects.create(status=STATUS_PUBLIC)
+        Extension.objects.create(status=STATUS_NULL)
+        eq_(list(Extension.objects.public()), [extension1, extension2])
+
     def test_pending(self):
         extension1 = Extension.objects.create()
         ExtensionVersion.objects.create(
@@ -527,3 +568,31 @@ def test_pending(self):
         Extension.objects.create(status=STATUS_NULL)
 
         eq_(list(Extension.objects.pending()), [extension1, extension2])
+
+
+class TestExtensionVersionManager(TestCase):
+    def test_public(self):
+        extension = Extension.objects.create(status=STATUS_PUBLIC)
+        version1 = ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PUBLIC, version='1.0')
+        version2 = ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PUBLIC, version='1.1')
+        ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_REJECTED, version='1.2')
+        ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PENDING, version='1.3')
+        eq_(list(ExtensionVersion.objects.public()), [version1, version2])
+        eq_(list(extension.versions.public()), [version1, version2])
+
+    def test_pending(self):
+        extension = Extension.objects.create()
+        ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_REJECTED, version='2.0')
+        version1 = ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PENDING, version='2.1')
+        version2 = ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PENDING, version='2.2')
+        ExtensionVersion.objects.create(
+            extension=extension, status=STATUS_PUBLIC, version='2.3')
+
+        eq_(list(ExtensionVersion.objects.pending()), [version1, version2])