From 6050b1ab942bc9a8c67b3c5287cf5cdea390f878 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 22 May 2017 13:22:48 +0200 Subject: [PATCH] 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 | 21 +++++-- src/olympia/editors/helpers.py | 47 +++++++-------- .../editors/templates/editors/review.html | 4 +- src/olympia/editors/tests/test_forms.py | 24 +++++--- src/olympia/editors/tests/test_helpers.py | 57 ++++++++++++++++--- static/css/zamboni/editors.styl | 5 ++ static/js/zamboni/editors.js | 5 -- 7 files changed, 115 insertions(+), 48 deletions(-) diff --git a/src/olympia/editors/forms.py b/src/olympia/editors/forms.py index 185a5a4765..144b4600a5 100644 --- a/src/olympia/editors/forms.py +++ b/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...'))]]] diff --git a/src/olympia/editors/helpers.py b/src/olympia/editors/helpers.py index cc3c1c2440..81a0faaee2 100644 --- a/src/olympia/editors/helpers.py +++ b/src/olympia/editors/helpers.py @@ -432,23 +432,24 @@ 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 (self.addon.current_version and - self.addon.current_version.was_auto_approved and - acl.action_allowed( - request, amo.permissions.ADDONS_POST_REVIEW)): - actions['confirm_auto_approved'] = { - 'method': self.handler.confirm_auto_approved, - 'label': _('Confirm Approval'), - 'details': _('The latest public version of this add-on ' - 'was automatically approved. This records ' - 'your confirmation of the approval, ' - 'without notifying the developer.'), - 'minimal': True, - 'comments': False, - } + 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): + actions['confirm_auto_approved'] = { + 'method': self.handler.confirm_auto_approved, + 'label': _('Confirm Approval'), + 'details': _('The latest public version of this add-on ' + 'was automatically approved. This records ' + 'your confirmation of the approval, ' + 'without notifying the developer.'), + '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' % ( diff --git a/src/olympia/editors/templates/editors/review.html b/src/olympia/editors/templates/editors/review.html index fb17d4b9b0..0dbf209351 100644 --- a/src/olympia/editors/templates/editors/review.html +++ b/src/olympia/editors/templates/editors/review.html @@ -61,7 +61,6 @@ {% for version in pager.object_list|reverse %} - {% trans version = version.version, created = version.created|datetime, version_status = version_status(addon, version) %} Version {{ version }} · {{ created }} · {{ version_status }} {% endtrans %} @@ -174,6 +173,9 @@ {{ _("You can still submit this form, however only do so if you know it won't conflict.") }} + {{ form.versions }} + {{ form.versions.errors }} +
diff --git a/src/olympia/editors/tests/test_forms.py b/src/olympia/editors/tests/test_forms.py index fb2d8ecbc5..b6f63c1ac3 100644 --- a/src/olympia/editors/tests/test_forms.py +++ b/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 == { diff --git a/src/olympia/editors/tests/test_helpers.py b/src/olympia/editors/tests/test_helpers.py index d8ae9d716a..40810d00be 100644 --- a/src/olympia/editors/tests/test_helpers.py +++ b/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 diff --git a/static/css/zamboni/editors.styl b/static/css/zamboni/editors.styl index 9e1e4d451b..a5e0c7fe4e 100644 --- a/static/css/zamboni/editors.styl +++ b/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; } diff --git a/static/js/zamboni/editors.js b/static/js/zamboni/editors.js index bf6c4c7bb1..03b595c6e7 100644 --- a/static/js/zamboni/editors.js +++ b/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(); });