Added ability to delete versions for packaged apps (bug 785979)

apps/addons/models.py

@@ -808,8 +808,7 @@ def get_icon_url(self, size, use_default=True):
 
     def update_status(self, using=None):
         if (self.status in [amo.STATUS_NULL, amo.STATUS_DELETED]
-            or self.is_disabled
-            or self.is_webapp() or self.is_persona()):
+            or self.is_disabled or self.is_persona()):
             return
 
         def logit(reason, old=self.status):

apps/amo/tests/__init__.py

@@ -282,7 +282,7 @@ def assertNoFormErrors(self, response):
     def assertLoginRedirects(self, response, to, status_code=302):
         # Not using urlparams, because that escapes the variables, which
         # is good, but bad for assertRedirects which will fail.
-        self.assertRedirects(response,
+        self.assert3xx(response,
             '%s?to=%s' % (reverse('users.login'), to), status_code)
 
     def assert3xx(self, response, expected_url, status_code=302,

apps/versions/models.py

@@ -153,6 +153,7 @@ def get_url_path(self):
         return reverse('addons.versions', args=[self.addon.slug, self.version])
 
     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()
 

media/css/devreg/typography.less

@@ -86,6 +86,9 @@ pre {
     }
     p {
         margin-top: 1em;
+        &.call-to-action {
+            font-weight: bold;
+        }
     }
     > p:first-child {
         margin: 0;

media/js/devreg/devhub.js

@@ -200,12 +200,36 @@ $(document).ready(function() {
     });
     $('#modal-disable').modal('#disable-addon', {
         width: 400,
-        callback: function(d){
+        callback: function(d) {
             $('.version_id', this).val($(d.click_target).attr('data-version'));
             return true;
         }
     });
 
+    $('#version-list').exists(function() {
+        var status = $('#version-status').data('status');
+        var versions = $('#modal-delete-version').data('versions');
+        $('#modal-delete-version').modal('.delete-version', {
+            width: 400,
+            callback: function(d) {
+                var version = versions[$(d.click_target).data('version')],
+                    $header = $('h3', this);
+                $header.text(format($header.attr('data-tmpl'), version));
+                $('.version-id', this).val(version.id);
+                if (versions.num == 1) {
+                    $('#last-version, #last-version-other').show();
+                    if (status == 2) {  // PENDING
+                        $('#last-version-pending').show();
+                    } else if (status == 4) {  // PUBLIC
+                        $('#last-version-public').show();
+                    }
+                } else {
+                    $('#not-last-version').show();
+                }
+            }
+        });
+    });
+
     // In-app payments config.
     if ($('#in-app-config').length) {
         initInAppConfig($('#in-app-config'));

mkt/developers/templates/developers/apps/listing/item_actions_app.html

@@ -36,7 +36,7 @@ <h5>{{ _('Actions') }}</h5>
   {% endif %}
   {% if waffle.switch('disabled-payments') %}
     <li>
-      <a class="action-link status-link" href="{{ addon.get_dev_url('versions') }}">{{ _('Manage Status') }}</a>
+      <a class="action-link status-link" href="{{ addon.get_dev_url('versions') }}">{% if addon.is_packaged %}{{ _('Manage Status &amp; Versions') }}{% else %}{{ _('Manage Status') }}{% endif %}</a>
     </li>
   {% endif %}
   {% if request.can_view_consumer %}
@@ -54,7 +54,7 @@ <h5>{{ _('Actions') }}</h5>
       <a href="#" class="more-actions">{{ _('More') }}</a>
       <div class="more-actions-popup popup hidden">
         {% set manage_urls = [
-          (addon.get_dev_url('versions'), _('Manage Status')),
+          (addon.get_dev_url('versions'), _('Manage Status &amp; Versions') if addon.is_packaged else _('Manage Status')),
         ] %}
         {% if waffle.switch('allow-refund') and addon.needs_paypal() and
               check_addon_ownership(request, addon, support=True) %}

mkt/developers/templates/developers/apps/status.html

@@ -1,6 +1,10 @@
 {% extends 'developers/base_impala.html' %}
 
-{% set title = _('Manage Status') %}
+{% if addon.is_packaged %}
+  {% set title = _('Manage Status & Versions') %}
+{% else %}
+  {% set title = _('Manage Status') %}
+{% endif %}
 {% block title %}{{ hub_page_title(title, addon) }}{% endblock %}
 
 {% macro status(msg) %}
@@ -14,7 +18,7 @@ <h1>{{ title }}</h1>
   </header>
   <section id="edit-addon" class="primary devhub-form manage">
     <h2>{{ _('Current Status') }}</h2>
-    <div class="island" id="version-status">
+    <div class="island" id="version-status" data-status="{{ addon.status }}">
       <p class="status">
         {% if addon.disabled_by_user and addon.status != amo.STATUS_DISABLED %}
           {{ status(_('You have <b>disabled</b> this app.')|safe) }}
@@ -114,7 +118,7 @@ <h4><a href="{{ addon.get_dev_url('versions.edit', [version.pk]) }}">Version {{
                 <small>Submitted <span title="{{ version.created|isotime }}">{{ version.created|datetime }}</span></small>
                 <div class="buttons">
                   <a href="{{ version.all_files[0].get_url_path('devhub') }}" class="button download">{{ _('Download') }}</a>
-                  {# TODO: Install and delete buttons #}
+                  <a href="#" class="button delete-button delete-version" data-version="{{ version.id }}">{{ _('Delete') }}</a>
                 </div>
               </li>
             {% endfor %}
@@ -171,9 +175,32 @@ <h3>{{ _('Disable App') }}</h3>
             {{ _('or') }} <a href="#" class="cancel close">{{ _('Cancel') }}</a>
           </p>
         </form>
-        <a href="#" class="close">{{ _('Cancel') }}</a>
       </div>
     {% endif %}
+    {% if addon.is_packaged %}
+      <div id="modal-delete-version" class="modal modal-delete" data-versions="{{ version_strings }}">
+        <form method="post" action="{{ addon.get_dev_url('versions.delete') }}">
+          {{ csrf() }}
+          <h3 data-tmpl="{{ _('Delete Version &quot;{version}&quot;') }}"></h3>
+          <p id="last-version" class="hidden">{{ _('Deleting the only version will result in the following:') }}</p>
+          <p id="not-last-version" class="hidden">{{ _('Deleting this version will result in the following:') }}</p>
+          <ul class="indent">
+            <li>{{ _('all associated user reviews will be deleted') }}</li>
+            <li id="last-version-other" class="hidden">{{ _('the app status will be marked as &quot;incomplete&quot;') }}</li>
+            <li id="last-version-pending" class="hidden">{{ _('the app will be removed from our review queue') }}</li>
+            <li id="last-version-public" class="hidden">{{ _('the app will not be listed in our public pages') }}</li>
+          </ul>
+          <p>{{ _('These changes cannot be undone.') }}</p>
+          <p class="call-to-action">{{ _('Are you sure you wish to delete this version?') }}</label></p>
+          <input type="hidden" name="version_id" class="version-id">
+          <p class="listing-footer">
+            <button type="submit">{{ _('Delete Version') }}</button>
+            {{ _('or') }} <a href="#" class="cancel close">{{ _('Cancel') }}</a>
+          </p>
+        </form>
+      </div>
+    {% endif %}
+
   </div>
   {% include 'developers/includes/addons_edit_nav.html' %}
 {% endblock %}

mkt/developers/templates/developers/includes/addons_edit_nav.html

@@ -2,7 +2,6 @@
   (addon.get_dev_url(), _('Edit Listing')),
   (addon.get_dev_url('owner'), _('Manage Authors')),
 ] %}
-{# TODO(cvan): Remove this when we fix/remove all the add-ons tests. #}
 {% if not waffle.switch('disabled-payments') or addon.is_premium() %}
   {% do urls.append((addon.get_dev_url('payments'), _('Manage Payments'))) %}
 {% endif %}
@@ -18,7 +17,9 @@
     {% do urls.insert(4, (addon.get_dev_url('refunds'), _('Manage Refunds'))) %}
   {% endif %}
 {% endif %}
-{% if addon.is_webapp() %}
+{% if addon.is_packaged %}
+  {% do urls.append((addon.get_dev_url('versions'), _('Manage Status & Versions'))) %}
+{% else %}
   {% do urls.append((addon.get_dev_url('versions'), _('Manage Status'))) %}
 {% endif %}
 {% if request.can_view_consumer %}

mkt/developers/tests/test_views_versions.py

@@ -4,12 +4,14 @@
 import amo
 import amo.tests
 from addons.models import Addon, AddonUser
+from devhub.models import ActivityLog
 from users.models import UserProfile
+from versions.models import Version
 
 from mkt.submit.tests.test_views import BasePackagedAppTest
 
 
-class TestAppStatus(amo.tests.TestCase):
+class TestVersion(amo.tests.TestCase):
     fixtures = ['base/apps', 'base/users', 'webapps/337141-steamcube']
 
     def setUp(self):
@@ -32,6 +34,7 @@ def test_items(self):
         eq_(doc('#delete-addon').length, 0)
         eq_(doc('#modal-delete').length, 0)
         eq_(doc('#modal-disable').length, 1)
+        eq_(doc('#modal-delete-version').length, 0)
 
     def test_soft_delete_items(self):
         self.create_switch(name='soft_delete')
@@ -41,6 +44,7 @@ def test_soft_delete_items(self):
         eq_(doc('#delete-addon').length, 1)
         eq_(doc('#modal-delete').length, 1)
         eq_(doc('#modal-disable').length, 1)
+        eq_(doc('#modal-delete-version').length, 0)
 
     def test_delete_link(self):
         # Hard "Delete App" link should be visible for only incomplete apps.
@@ -94,35 +98,6 @@ def test_rejected(self):
             'Files for reapplied apps should get marked as pending')
         eq_(unicode(webapp.versions.all()[0].approvalnotes), my_reply)
 
-    def test_items_packaged(self):
-        self.webapp.update(is_packaged=True)
-        doc = pq(self.client.get(self.url).content)
-        eq_(doc('#version-status').length, 1)
-        eq_(doc('#version-list').length, 1)
-        eq_(doc('#delete-addon').length, 0)
-        eq_(doc('#modal-delete').length, 0)
-        eq_(doc('#modal-disable').length, 1)
-
-    def test_version_list_packaged(self):
-        self.webapp.update(is_packaged=True)
-        amo.tests.version_factory(addon=self.webapp, version='2.0',
-                                  file_kw=dict(status=amo.STATUS_PENDING))
-        self.webapp = self.get_webapp()
-        doc = pq(self.client.get(self.url).content)
-        eq_(doc('#version-status').length, 1)
-        eq_(doc('#version-list li').length, 2)
-        # 1 pending and 1 public.
-        eq_(doc('#version-list span.status-pending').length, 1)
-        eq_(doc('#version-list span.status-public').length, 1)
-        # Check version strings and order of versions.
-        eq_(map(lambda x: x.text, doc('#version-list h4 a')),
-            ['Version 2.0', 'Version 1.0'])
-        # Check download url.
-        eq_(doc('#version-list a.button.download').eq(0).attr('href'),
-            self.webapp.versions.all()[0].all_files[0].get_url_path('devhub'))
-        eq_(doc('#version-list a.button.download').eq(1).attr('href'),
-            self.webapp.versions.all()[1].all_files[0].get_url_path('devhub'))
-
 
 class TestAddVersion(BasePackagedAppTest):
 
@@ -177,3 +152,80 @@ def test_post(self):
         ver = self.app.versions.latest()
         eq_(ver.releasenotes, rn)
         eq_(ver.approvalnotes, an)
+
+
+class TestVersionPackaged(amo.tests.WebappTestCase):
+    fixtures = ['base/apps', 'base/users', 'webapps/337141-steamcube']
+
+    def setUp(self):
+        super(TestVersionPackaged, self).setUp()
+        assert self.client.login(username='steamcube@mozilla.com',
+                                 password='password')
+        self.app.update(is_packaged=True)
+        self.app = self.get_app()
+        self.url = self.app.get_dev_url('versions')
+        self.delete_url = self.app.get_dev_url('versions.delete')
+
+    def test_items_packaged(self):
+        self.create_switch(name='soft_delete')
+        doc = pq(self.client.get(self.url).content)
+        eq_(doc('#version-status').length, 1)
+        eq_(doc('#version-list').length, 1)
+        eq_(doc('#delete-addon').length, 1)
+        eq_(doc('#modal-delete').length, 1)
+        eq_(doc('#modal-disable').length, 1)
+        eq_(doc('#modal-delete-version').length, 1)
+
+    def test_version_list_packaged(self):
+        self.app.update(is_packaged=True)
+        amo.tests.version_factory(addon=self.app, version='2.0',
+                                  file_kw=dict(status=amo.STATUS_PENDING))
+        self.app = self.get_app()
+        doc = pq(self.client.get(self.url).content)
+        eq_(doc('#version-status').length, 1)
+        eq_(doc('#version-list li').length, 2)
+        # 1 pending and 1 public.
+        eq_(doc('#version-list span.status-pending').length, 1)
+        eq_(doc('#version-list span.status-public').length, 1)
+        # Check version strings and order of versions.
+        eq_(map(lambda x: x.text, doc('#version-list h4 a')),
+            ['Version 2.0', 'Version 1.0'])
+        # There should be 2 delete buttons.
+        eq_(doc('#version-list a.delete-version').length, 2)
+        # Check download url.
+        eq_(doc('#version-list a.button.download').eq(0).attr('href'),
+            self.app.versions.all()[0].all_files[0].get_url_path('devhub'))
+        eq_(doc('#version-list a.button.download').eq(1).attr('href'),
+            self.app.versions.all()[1].all_files[0].get_url_path('devhub'))
+
+    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
+        # Now do the POST to delete.
+        res = self.client.post(self.delete_url, dict(version_id=version.pk),
+                               follow=True)
+        assert not Version.objects.filter(pk=version.pk).exists()
+        eq_(ActivityLog.objects.filter(action=amo.LOG.DELETE_VERSION.id)
+                               .count(), 1)
+        # Since this was the last version, the app status should be incomplete.
+        eq_(self.get_app().status, amo.STATUS_NULL)
+        # Check xss in success flash message.
+        assert not '<script>alert(' in res.content
+        assert '&lt;script&gt;alert(' in res.content
+
+    def test_anonymous_delete_redirects(self):
+        self.client.logout()
+        version = self.app.versions.latest()
+        res = self.client.post(self.delete_url, dict(version_id=version.pk))
+        self.assertLoginRedirects(res, self.delete_url)
+
+    def test_non_dev_no_delete_for_you(self):
+        self.client.logout()
+        assert self.client.login(username='regular@mozilla.com',
+                                 password='password')
+        version = self.app.versions.latest()
+        res = self.client.post(self.delete_url, dict(version_id=version.pk))
+        eq_(res.status_code, 403)

mkt/developers/urls.py

@@ -46,6 +46,8 @@ def paypal_patterns(prefix):
     # TODO: '^versions/$'
     url('^versions/(?P<version_id>\d+)$', views.version_edit,
         name='mkt.developers.apps.versions.edit'),
+    url('^versions/delete$', views.version_delete,
+        name='mkt.developers.apps.versions.delete'),
 
     url('^payments$', views.payments, name='mkt.developers.apps.payments'),
     # PayPal-specific stuff.

mkt/developers/views.py

@@ -216,6 +216,13 @@ def status(request, addon_id, addon, webapp=False):
     ctx = {'addon': addon, 'webapp': webapp, 'form': form,
            'upload_form': upload_form}
 
+    # Used in the delete version modal.
+    if addon.is_packaged:
+        versions = addon.versions.values('id', 'version')
+        version_strings = dict((v['id'], v) for v in versions)
+        version_strings['num'] = len(versions)
+        ctx['version_strings'] = json.dumps(version_strings)
+
     if addon.status == amo.STATUS_REJECTED:
         try:
             entry = (AppLog.objects
@@ -244,6 +251,18 @@ def version_edit(request, addon_id, addon, version_id):
         'addon': addon, 'version': version, 'form': form})
 
 
+@dev_required
+@post_required
+@transaction.commit_on_success
+def version_delete(request, addon_id, addon):
+    version_id = request.POST.get('version_id')
+    version = get_object_or_404(Version, pk=version_id, addon=addon)
+    version.delete()
+    messages.success(request,
+                     _('Version "{0}" deleted.').format(version.version))
+    return redirect(addon.get_dev_url('versions'))
+
+
 @dev_required(owner_for_post=True, webapp=True)
 def ownership(request, addon_id, addon, webapp=False):
     # Authors.

mkt/reviewers/templates/reviewers/review.html

@@ -168,28 +168,6 @@ <h3>
         {{ form.canned_response }}
       </div>
 
-      <div class="review-actions-section review-actions-files data-toggle"
-           data-value="{{ actions_minimal|join("|") }}"{% if allow_unchecking_files %} data-uncheckable="1"{% endif %}>
-        <label for="id_addon_files"><strong>{{ form.addon_files.label }}</strong></label>
-        <ul>
-            {% for pk, label in form.fields.get('addon_files').choices %}
-            <li>
-              <label for="file-{{ pk }}"{% if pk in form.addon_files_disabled %} class="light"{% endif %}>
-                <input id="file-{{ pk }}" type="checkbox" value="{{ pk }}" name="addon_files"
-                       {% if pk in form.addon_files_disabled %}disabled=""{% endif %} />
-                {{ label }}
-              </label>
-            </li>
-            {% endfor %}
-        </ul>
-
-        <div id="review-actions-files-warning">
-          {{ _('Notice: Only review more than one file if you have tested <strong>every</strong> file you select.') }}
-        </div>
-
-        {{ form.addon_files.errors }}
-      </div>
-
       <div class="review-actions-section review-actions-tested data-toggle"
            data-value="{{ actions_minimal|join("|") }}">
         <strong>{{ _('Tested with:') }}</strong>