Add soft deletion to app versions (bug 800087)

apps/versions/models.py

@@ -1,17 +1,18 @@
 # -*- coding: utf-8 -*-
 import os
 
+import django.dispatch
 from django.conf import settings
 from django.core.exceptions import ObjectDoesNotExist
 from django.core.files.storage import default_storage as storage
 from django.db import models
-import django.dispatch
-import jinja2
 
-import commonware.log
 import caching.base
+import commonware.log
+import jinja2
 import waffle
 
+import addons.query
 import amo
 import amo.models
 import amo.utils
@@ -29,6 +30,20 @@
 log = commonware.log.getLogger('z.versions')
 
 
+class VersionManager(amo.models.ManagerBase):
+
+    def __init__(self, include_deleted=False):
+        amo.models.ManagerBase.__init__(self)
+        self.include_deleted = include_deleted
+
+    def get_query_set(self):
+        qs = super(VersionManager, self).get_query_set()
+        qs = qs._clone(klass=addons.query.IndexQuerySet)
+        if not self.include_deleted:
+            qs = qs.exclude(deleted=True)
+        return qs.transform(Version.transformer)
+
+
 class Version(amo.models.ModelBase):
     addon = models.ForeignKey('addons.Addon', related_name='versions')
     license = models.ForeignKey('License', null=True)
@@ -43,6 +58,11 @@ class Version(amo.models.ModelBase):
     has_info_request = models.BooleanField(default=False)
     has_editor_comment = models.BooleanField(default=False)
 
+    deleted = models.BooleanField(default=False)
+
+    objects = VersionManager()
+    with_deleted = VersionManager(include_deleted=True)
+
     class Meta(amo.models.ModelBase.Meta):
         db_table = 'versions'
         ordering = ['-created', '-modified']
@@ -166,7 +186,10 @@ def get_url_path(self):
     def delete(self):
         log.info(u'Version deleted: %r (%s)' % (self, self.id))
         amo.log(amo.LOG.DELETE_VERSION, self.addon, str(self.version))
-        super(Version, self).delete()
+        if settings.MARKETPLACE:
+            self.update(deleted=True)
+        else:
+            super(Version, self).delete()
 
     @property
     def current_queue(self):
@@ -286,9 +309,10 @@ def supported_platforms(self):
 
     @property
     def status(self):
-        status = dict([(f.status, amo.STATUS_CHOICES[f.status])
-                       for f in self.all_files])
-        return status.values()
+        if settings.MARKETPLACE and self.deleted:
+            return [amo.STATUS_CHOICES[amo.STATUS_DELETED]]
+        else:
+            return [amo.STATUS_CHOICES[f.status] for f in self.all_files]
 
     @property
     def statuses(self):
@@ -298,6 +322,7 @@ def statuses(self):
     def is_allowed_upload(self):
         """Check that a file can be uploaded based on the files
         per platform for that type of addon."""
+
         num_files = len(self.all_files)
         if self.addon.type == amo.ADDON_SEARCH:
             return num_files == 0

apps/versions/tests.py

@@ -168,7 +168,20 @@ def test_is_unreviewed(self):
     def test_version_delete(self):
         version = Version.objects.get(pk=81551)
         version.delete()
-        assert Addon.uncached.get(pk=3615)
+
+        addon = Addon.uncached.get(pk=3615)
+        assert not Version.objects.filter(addon=addon).exists()
+        assert not Version.with_deleted.filter(addon=addon).exists()
+
+    @mock.patch('versions.models.settings.MARKETPLACE', True)
+    def test_version_delete_marketplace(self):
+        version = Version.objects.get(pk=81551)
+        version.delete()
+        addon = Addon.uncached.get(pk=3615)
+        assert addon
+
+        assert not Version.objects.filter(addon=addon).exists()
+        assert Version.with_deleted.filter(addon=addon).exists()
 
     def test_version_delete_files(self):
         version = Version.objects.get(pk=81551)
@@ -179,6 +192,7 @@ def test_version_delete_files(self):
     def test_version_delete_logs(self):
         user = UserProfile.objects.get(pk=55021)
         amo.set_user(user)
+        # The transform don't know bout my users.
         version = Version.objects.get(pk=81551)
         eq_(ActivityLog.objects.count(), 0)
         version.delete()
@@ -187,6 +201,8 @@ def test_version_delete_logs(self):
     def test_version_is_allowed_upload(self):
         version = Version.objects.get(pk=81551)
         version.files.all().delete()
+        # The transform don't know bout my deletions.
+        version = Version.objects.get(pk=81551)
         assert version.is_allowed_upload()
 
     def test_version_is_not_allowed_upload(self):
@@ -197,9 +213,11 @@ def test_version_is_not_allowed_upload(self):
                          amo.PLATFORM_BSD.id]:
             file = File(platform_id=platform, version=version)
             file.save()
+        version = Version.objects.get(pk=81551)
         assert version.is_allowed_upload()
         file = File(platform_id=amo.PLATFORM_MAC.id, version=version)
         file.save()
+        # The transform don't know bout my new files.
         version = Version.uncached.get(pk=81551)
         assert not version.is_allowed_upload()
 
@@ -211,13 +229,17 @@ def test_version_is_not_allowed_upload_full(self):
                          amo.PLATFORM_MAC.id]:
             file = File(platform_id=platform, version=version)
             file.save()
+        # The transform don't know bout my new files.
+        version = Version.objects.get(pk=81551)
         assert not version.is_allowed_upload()
 
     def test_version_is_allowed_upload_search(self):
         version = Version.objects.get(pk=81551)
         version.addon.type = amo.ADDON_SEARCH
         version.addon.save()
         version.files.all()[0].delete()
+        # The transform don't know bout my deletions.
+        version = Version.objects.get(pk=81551)
         assert version.is_allowed_upload()
 
     def test_version_is_not_allowed_upload_search(self):

migrations/535-version-soft-delete.sql

@@ -0,0 +1,2 @@
+ALTER TABLE versions
+    ADD COLUMN deleted tinyint(1) UNSIGNED NOT NULL DEFAULT '0';

mkt/developers/tests/test_views_versions.py

@@ -283,6 +283,7 @@ def test_version_list_packaged(self):
     def test_delete_version(self):
         version = self.app.versions.latest()
         version.update(version='<script>alert("xss")</script>')
+
         res = self.client.get(self.url)
         assert not '<script>alert(' in res.content
         assert '&lt;script&gt;alert(' in res.content
@@ -298,6 +299,15 @@ def test_delete_version(self):
         assert not '<script>alert(' in res.content
         assert '&lt;script&gt;alert(' in res.content
 
+        # Test that the soft deletion works pretty well.
+        eq_(self.app.versions.count(), 0)
+        # We can't use `.reload()` :(
+        version = Version.with_deleted.filter(addon=self.app)
+        eq_(version.count(), 1)
+        # Test that the status of the "deleted" version is STATUS_DELETED.
+        eq_(str(version[0].status[0]),
+            str(amo.STATUS_CHOICES[amo.STATUS_DELETED]))
+
     def test_anonymous_delete_redirects(self):
         self.client.logout()
         version = self.app.versions.latest()

mkt/reviewers/views.py

@@ -236,10 +236,10 @@ def _review(request, addon):
     # We only allow the user to check/uncheck files for "pending"
     allow_unchecking_files = form.helper.review_type == "pending"
 
-    versions = (Version.objects.filter(addon=addon)
-                               .order_by('-created')
-                               .transform(Version.transformer_activity)
-                               .transform(Version.transformer))
+    versions = (Version.with_deleted.filter(addon=addon)
+                                    .order_by('-created')
+                                    .transform(Version.transformer_activity)
+                                    .transform(Version.transformer))
 
     product_attrs = {
         'product': json.dumps(