Use a dropdown instead of checkboxes in reject multiple versions action

Also allow rejection of any public version, regardless of add-on state,
and use latest version when sending the activity notification email so
that the developer is always able to reply.

src/olympia/editors/forms.py

@@ -22,6 +22,7 @@ from olympia.editors.tasks import approve_rereview, reject_rereview, send_mail
 from olympia.lib import happyforms
 from olympia.reviews.helpers import user_can_delete_review
 from olympia.reviews.models import Review
+from olympia.versions.models import Version
 
 
 log = olympia.core.logger.getLogger('z.reviewers.forms')
@@ -289,7 +290,11 @@ class ReviewForm(happyforms.Form):
     canned_response = NonValidatingChoiceField(required=False)
     action = forms.ChoiceField(required=True, widget=forms.RadioSelect())
     versions = ModelMultipleChoiceField(
-        required=False, queryset=None)  # queryset is set later in __init__.
+        widget=forms.SelectMultiple(attrs={
+            'class': 'data-toggle', 'data-value': 'reject_multiple_versions'}),
+        required=False,
+        queryset=Version.objects.none())  # queryset is set later in __init__.
+
     operating_systems = forms.CharField(required=False,
                                         label=_(u'Operating systems:'))
     applications = forms.CharField(required=False,
@@ -323,11 +328,17 @@ class ReviewForm(happyforms.Form):
         super(ReviewForm, self).__init__(*args, **kw)
 
         # With the helper, we now have the add-on and can set queryset on the
-        # versions field correctly.
-        self.fields['versions'].queryset = self.helper.addon.versions.filter(
-            channel=amo.RELEASE_CHANNEL_LISTED)
+        # versions field correctly. Small optimization: we only need to do this
+        # if the reject_multiple_versions action is available, otherwise we
+        # don't really care about this field.
+        if 'reject_multiple_versions' in self.helper.actions:
+            self.fields['versions'].queryset = (
+                self.helper.addon.versions.filter(
+                    channel=amo.RELEASE_CHANNEL_LISTED,
+                    files__status=amo.STATUS_PUBLIC).order_by('created'))
 
-        # We're starting with an empty one, which will be hidden via CSS.
+        # For the canned responses, we're starting with an empty one, which
+        # will be hidden via CSS.
         canned_choices = [
             ['', [('', ugettext('Choose a canned response...'))]]]
 

src/olympia/editors/helpers.py

@@ -432,13 +432,13 @@ class ReviewHelper(object):
                              'from the queue. The comments will be sent '
                              'to the developer.'),
                 'minimal': False}
-        # If the addon current version was auto-approved, extra actions are
-        # available to users with post-review permission, allowing them to
-        # confirm the approval or reject versions.
+        if acl.action_allowed(request, amo.permissions.ADDONS_POST_REVIEW):
+            # Post-reviewers have 2 extra actions depending on the state of
+            # the add-on:
+            # If the addon current version was auto-approved, they can confirm
+            # the approval.
             if (self.addon.current_version and
-                self.addon.current_version.was_auto_approved and
-                acl.action_allowed(
-                    request, amo.permissions.ADDONS_POST_REVIEW)):
+                    self.addon.current_version.was_auto_approved):
                 actions['confirm_auto_approved'] = {
                     'method': self.handler.confirm_auto_approved,
                     'label': _('Confirm Approval'),
@@ -449,6 +449,7 @@ class ReviewHelper(object):
                     'minimal': True,
                     'comments': False,
                 }
+            # In any case, they can reject multiple versions in one action.
             actions['reject_multiple_versions'] = {
                 'method': self.handler.reject_multiple_versions,
                 'label': _('Reject Multiple Versions'),
@@ -721,6 +722,7 @@ class ReviewBase(object):
         # self.version and self.files won't point to the versions we want to
         # modify in this action, so set them to None before finding the right
         # versions.
+        latest_version = self.version
         self.version = None
         self.files = None
         for version in self.data['versions']:
@@ -732,15 +734,14 @@ class ReviewBase(object):
         self.data['version_numbers'] = u', '.join(
             unicode(v.version) for v in self.data['versions'])
 
-        # Send the email to the developer. We need to pass down a version for
-        # the ActivityLogToken generation to work - it might still result in
-        # an expired token if it's not the latest version available for the
-        # add-on, but it's the best we can do.
+        # Send the email to the developer. We need to pass the latest version
+        # of the add-on instead of one of the versions we rejected, it will be
+        # used to generate a token allowing the developer to reply, and that
+        # only works with the latest version.
         template = u'reject_multiple_versions'
         subject = (u"Mozilla Add-ons: One or more versions of %s%s didn't "
                    u"pass review")
-        self.notify_email(
-            template, subject, version=self.data['versions'].latest('pk'))
+        self.notify_email(template, subject, version=latest_version)
 
         log.info(
             u'Making %s versions %s disabled' % (

src/olympia/editors/templates/editors/review.html

@@ -61,7 +61,6 @@
         {% for version in pager.object_list|reverse %}
         <tr class="listing-header">
           <th colspan="2">
-            <input type="checkbox" name="versions" value="{{ version.pk }}" autocomplete="off" class="data-toggle" data-value="reject_multiple_versions" />
             {% trans version = version.version, created = version.created|datetime, version_status = version_status(addon, version) %}
             Version {{ version }} &middot; {{ created }} <span class="light">&middot; {{ version_status }}</span>
             {% endtrans %}
@@ -174,6 +173,9 @@
         {{ _("You can still submit this form, however only do so if you know it won't conflict.") }}
       </div>
 
+      {{ form.versions }}
+      {{ form.versions.errors }}
+
       <div class="review-actions-section data-toggle"
            data-value="{{ actions_comments|join("|") }}">
         <label for="id_comments">{{ form.comments.label }}</label>

src/olympia/editors/tests/test_forms.py

@@ -106,27 +106,37 @@ class TestReviewForm(TestCase):
             'comments': [u'This field is required.']
         }
 
-        # Alter the action to make it require versions regardless of what it
-        # actually is, what we want to test is the form behaviour.
+        # Alter the action to make it not require comments to be sent
+        # regardless of what the action actually is, what we want to test is
+        # the form behaviour.
         form = self.get_form(data={'action': 'info'})
         form.helper.actions['info']['comments'] = False
         assert form.is_bound
         assert form.is_valid()
         assert not form.errors
 
-    def test_versions_required_and_queryset(self):
+    def test_versions_queryset(self):
         addon_factory()
         version_factory(addon=self.addon, channel=amo.RELEASE_CHANNEL_UNLISTED)
         form = self.get_form()
         assert not form.is_bound
         assert form.fields['versions'].required is False
+        assert list(form.fields['versions'].queryset) == []
+
+        # With post-review permission, the reject_multiple_versions action will
+        # be available, resetting the queryset of allowed choices.
+        self.grant_permission(self.request.user, 'Addons:PostReview')
+        form = self.get_form()
+        assert not form.is_bound
+        assert form.fields['versions'].required is False
         assert list(form.fields['versions'].queryset) == [
             self.addon.current_version]
 
-        # Alter the action to make it require versions regardless of what it
-        # actually is, what we want to test is the form behaviour.
-        form = self.get_form(data={'action': 'info', 'comments': 'lol'})
-        form.helper.actions['info']['versions'] = True
+    def test_versions_required(self):
+        self.grant_permission(self.request.user, 'Addons:PostReview')
+        form = self.get_form(data={
+            'action': 'reject_multiple_versions', 'comments': 'lol'})
+        form.helper.actions['reject_multiple_versions']['versions'] = True
         assert form.is_bound
         assert not form.is_valid()
         assert form.errors == {

src/olympia/editors/tests/test_helpers.py

@@ -12,7 +12,7 @@ from pyquery import PyQuery as pq
 
 from olympia import amo
 from olympia.activity.models import ActivityLog, ActivityLogToken
-from olympia.amo.tests import TestCase, version_factory
+from olympia.amo.tests import file_factory, TestCase, version_factory
 from olympia.amo.utils import send_mail
 from olympia.addons.models import Addon, AddonApprovalsCounter
 from olympia.amo.urlresolvers import reverse
@@ -303,7 +303,7 @@ class TestReviewHelper(TestCase):
 
     def test_actions_public_post_reviewer(self):
         self.grant_permission(self.request.user, 'Addons:PostReview')
-        expected = ['info', 'super', 'comment']
+        expected = ['reject_multiple_versions', 'info', 'super', 'comment']
         assert self.get_review_actions(
             addon_status=amo.STATUS_PUBLIC,
             file_status=amo.STATUS_PUBLIC).keys() == expected
@@ -927,15 +927,17 @@ class TestReviewHelper(TestCase):
         assert self.get_helper()
 
     def test_reject_multiple_versions(self):
+        old_version = self.version
+        self.version = version_factory(addon=self.addon, version='3.0')
+        # An extra file should not change anything.
+        file_factory(version=self.version, platform=amo.PLATFORM_LINUX.id)
         self.setup_data(amo.STATUS_PUBLIC, file_status=amo.STATUS_PUBLIC)
-        version2 = version_factory(addon=self.addon, version='3.0')
 
         # Safeguards.
         assert isinstance(self.helper.handler, helpers.ReviewFiles)
         assert self.addon.status == amo.STATUS_PUBLIC
         assert self.file.status == amo.STATUS_PUBLIC
-        assert self.addon.current_version.files.all()[0].status == (
-            amo.STATUS_PUBLIC)
+        assert self.addon.current_version.is_public()
 
         data = self.get_data().copy()
         data['versions'] = self.addon.versions.all()
@@ -946,7 +948,7 @@ class TestReviewHelper(TestCase):
         self.file.reload()
         assert self.addon.status == amo.STATUS_NULL
         assert self.addon.current_version is None
-        assert list(self.addon.versions.all()) == [version2, self.version]
+        assert list(self.addon.versions.all()) == [self.version, old_version]
         assert self.file.status == amo.STATUS_DISABLED
 
         assert len(mail.outbox) == 1
@@ -956,7 +958,48 @@ class TestReviewHelper(TestCase):
             u"didn't pass review")
         assert ('Version(s) affected and disabled:\n3.0, 2.1.072'
                 in mail.outbox[0].body)
-        log_token = ActivityLogToken.objects.filter(version=version2).get()
+        log_token = ActivityLogToken.objects.get()
+        assert log_token.uuid.hex in mail.outbox[0].reply_to[0]
+
+        assert self.check_log_count(amo.LOG.REJECT_VERSION.id) == 2
+
+    def test_reject_multiple_versions_except_latest(self):
+        old_version = self.version
+        extra_version = version_factory(addon=self.addon, version='3.1')
+        # Add yet another version we don't want to reject.
+        self.version = version_factory(addon=self.addon, version='42.0')
+        self.setup_data(amo.STATUS_PUBLIC, file_status=amo.STATUS_PUBLIC)
+
+        # Safeguards.
+        assert isinstance(self.helper.handler, helpers.ReviewFiles)
+        assert self.addon.status == amo.STATUS_PUBLIC
+        assert self.file.status == amo.STATUS_PUBLIC
+        assert self.addon.current_version.is_public()
+
+        data = self.get_data().copy()
+        data['versions'] = self.addon.versions.all().exclude(
+            pk=self.version.pk)
+        self.helper.set_data(data)
+        self.helper.handler.reject_multiple_versions()
+
+        self.addon.reload()
+        self.file.reload()
+        # latest_version is still public so the add-on is still public.
+        assert self.addon.status == amo.STATUS_PUBLIC
+        assert self.addon.current_version == self.version
+        assert list(self.addon.versions.all().order_by('-pk')) == [
+            self.version, extra_version, old_version]
+        assert self.file.status == amo.STATUS_DISABLED
+
+        assert len(mail.outbox) == 1
+        assert mail.outbox[0].to == [self.addon.authors.all()[0].email]
+        assert mail.outbox[0].subject == (
+            u"Mozilla Add-ons: One or more versions of Delicious Bookmarks "
+            u"didn't pass review")
+        assert ('Version(s) affected and disabled:\n3.1, 2.1.072'
+                in mail.outbox[0].body)
+        log_token = ActivityLogToken.objects.filter(
+            version=self.version).get()
         assert log_token.uuid.hex in mail.outbox[0].reply_to[0]
 
         assert self.check_log_count(amo.LOG.REJECT_VERSION.id) == 2

static/css/zamboni/editors.styl

@@ -826,6 +826,11 @@ form.review-form .data-toggle {
     border-bottom: 1px solid #A5BFCE;
 }
 
+#review-actions-form #id_versions {
+    width: 100%;
+    height: 131px;
+}
+
 .review-actions-canned {
     text-align: right;
 }

static/js/zamboni/editors.js

@@ -138,11 +138,6 @@ function initReviewActions() {
     }
 
     /* Item History */
-    $('#review-files tr.listing-header input[type=checkbox]').click(function(e) {
-        /* clicking on the checkbox in the header should not toggle that item */
-        e.stopPropagation();
-    });
-
     $('#review-files tr.listing-header').click(function() {
         $(this).next('tr.listing-body').toggle();
     });