From 0432e85450886373e58b05011ed8d701667d8226 Mon Sep 17 00:00:00 2001 From: Mathieu Agopian Date: Thu, 27 Aug 2015 16:18:26 +0200 Subject: [PATCH] Add the approve_addons management command (bug 1199234) --- .../management/commands/approve_addons.py | 100 ++++++++++++ apps/addons/tests/test_commands.py | 144 ++++++++++++++++++ apps/constants/base.py | 6 + apps/editors/helpers.py | 8 +- settings_test.py | 2 +- 5 files changed, 257 insertions(+), 3 deletions(-) create mode 100644 apps/addons/management/commands/approve_addons.py diff --git a/apps/addons/management/commands/approve_addons.py b/apps/addons/management/commands/approve_addons.py new file mode 100644 index 0000000000..c7a657acab --- /dev/null +++ b/apps/addons/management/commands/approve_addons.py @@ -0,0 +1,100 @@ +# -*- coding: utf-8 -*- +import logging + +from django.core.management.base import BaseCommand, CommandError + +import amo +from addons.models import Addon +from amo.utils import chunked +from editors.helpers import ReviewHelper + +log = logging.getLogger('z.addons') + + +class Command(BaseCommand): + args = u'' + help = u'Approve a list of add-ons, given their GUIDs.' + + def handle(self, *args, **options): + if len(args) == 0: # No GUID provided? + raise CommandError( + u'Please provide at least one add-on guid to approve.') + + confirm = raw_input( + u'Are you sure you want to bulk approve and sign all those {0} ' + u'addons? (yes/no)'.format(len(args))) + if confirm != 'yes': + raise CommandError(u'Aborted.') + + for chunk in chunked(args, 100): + files = get_files(chunk) + log.info(u'Bulk approving chunk of %s files', len(files)) + + files_with_review_type = [ + (file_, get_review_type(file_)) for file_ in files] + + approve_files(files_with_review_type) + + +def get_files(addon_guids): + """Return the list of files that need approval, given a list of GUIDs. + + A file needs approval if: + - it's unreviewed + - it's preliminary reviewed, but its addon is requesting a full review + """ + # Get all the add-ons that have a GUID from the list, and which are either + # reviewed or awaiting a review. + addons = Addon.with_unlisted.filter( + guid__in=addon_guids, + status__in=amo.UNDER_REVIEW_STATUSES + amo.REVIEWED_STATUSES) + # Of all those add-ons, we return the list of latest_version files that are + # under review, or add-ons that are under review. + files = [] + for addon in addons: + for file_ in addon.latest_version.files.all(): + if (file_.status in amo.UNDER_REVIEW_STATUSES or + addon.status in amo.UNDER_REVIEW_STATUSES): + files.append(file_) + return files + + +def approve_files(files_with_review_type): + """Approve the files (and sign them). + + A file will be fully approved if: + - it's waiting for a full review + - it's preliminary reviewed, and its addon is waiting for a full review + A file will be prelim approved if: + - it's waiting for a prelim review + """ + for file_, review_type in files_with_review_type: + version = file_.version + addon = version.addon + helper = ReviewHelper(request=None, addon=addon, + version=file_.version) + # Provide the file to review/sign to the helper. + helper.set_data({'addon_files': [file_], + 'comments': u'bulk approval'}) + if review_type == 'full': + # Already fully reviewed, or waiting for a full review. + helper.handler.process_public() + log.info(u'File %s (addon %s) fully reviewed', file_.pk, addon.pk) + elif review_type == 'prelim': + # Already prelim reviewed, or waiting for a prelim review. + helper.handler.process_preliminary() + log.info(u'File %s (addon %s) prelim reviewed', file_.pk, addon.pk) + else: + log.info(u'File %s (addon %s) not reviewed: ' + u'addon status: %s, file status: %s', + file_.pk, addon.pk, addon.status, file_.status) + + +def get_review_type(file_): + """Return 'full', 'prelim' or None depending on the file/addon status.""" + if file_.status == amo.STATUS_NOMINATED: + return 'full' + if file_.status == amo.STATUS_UNREVIEWED: + return 'prelim' + if file_.version.addon.status == amo.STATUS_LITE_AND_NOMINATED: + return 'full' diff --git a/apps/addons/tests/test_commands.py b/apps/addons/tests/test_commands.py index a7237fe4f0..9dd420dd78 100644 --- a/apps/addons/tests/test_commands.py +++ b/apps/addons/tests/test_commands.py @@ -1,11 +1,21 @@ +import pytest + from django.conf import settings from django.core.management import call_command +import amo +import amo.tests +from addons.management.commands import approve_addons +from devhub.models import AddonLog +from editors.models import ReviewerScore + # Where to monkeypatch "lib.crypto.tasks.sign_addons" so it's correctly mocked. SIGN_ADDONS = 'addons.management.commands.sign_addons.sign_addons' +# Test the "sign_addons" command. + def test_no_overridden_settings(monkeypatch): assert not settings.SIGNING_SERVER assert not settings.PRELIMINARY_SIGNING_SERVER @@ -52,3 +62,137 @@ def test_force_signing(monkeypatch): assert force monkeypatch.setattr(SIGN_ADDONS, is_forced) call_command('sign_addons', 123, force=True) + + +# Test the "approve_addons" command. + +@pytest.mark.django_db +def test_approve_addons_get_files_incomplete(): + """An incomplete add-on can't be approved.""" + addon = amo.tests.addon_factory(status=amo.STATUS_NULL) + assert approve_addons.get_files([addon.guid]) == [] + + +@pytest.mark.django_db +def test_approve_addons_get_files_bad_guid(): + """An add-on with another guid doesn't get approved.""" + addon1 = amo.tests.addon_factory(status=amo.STATUS_UNREVIEWED, guid='foo') + addon1_file = addon1.latest_version.files.get() + addon1_file.update(status=amo.STATUS_UNREVIEWED) + # Create another add-on that we won't get the files for. + addon2 = amo.tests.addon_factory(status=amo.STATUS_UNREVIEWED, guid='bar') + addon2_file = addon2.latest_version.files.get() + addon2_file.update(status=amo.STATUS_UNREVIEWED) + # There's only the addon1's file returned, no other. + assert approve_addons.get_files(['foo']) == [addon1_file] + + +@pytest.fixture( + params=[(amo.STATUS_UNREVIEWED, amo.STATUS_UNREVIEWED, 'prelim'), + (amo.STATUS_LITE, amo.STATUS_UNREVIEWED, 'prelim'), + (amo.STATUS_NOMINATED, amo.STATUS_NOMINATED, 'full'), + (amo.STATUS_PUBLIC, amo.STATUS_NOMINATED, 'full'), + (amo.STATUS_LITE_AND_NOMINATED, amo.STATUS_LITE, 'full')], + ids=['1-1-prelim', # Those are used to build better test names, for the + '8-1-prelim', # tests using this fixture, eg: + '3-3-full', # > test_approve_addons_get_files[1-1-prelim] + '4-3-full', # instead of simply: + '9-8-full']) # > test_approve_addons_get_files[usecase0] +def use_case(request, db): + """This fixture will return quadruples for different use cases. + + Addon | File1 and 2 | Review type + ============================================================== + waiting for prelim | waiting for prelim | prelim reviewed + prelim reviewed | waiting for prelim | prelim reviewed + waiting for full | waiting for full | fully reviewed + fully reviewed | waiting for full | fully reviewed + prelim waiting for full | prelim reviewed | fully reviewed + """ + addon_status, file_status, review_type = request.param + + addon = amo.tests.addon_factory(status=addon_status, guid='foo') + version = addon.latest_version + file1 = version.files.get() + file1.update(status=file_status) + # A second file for good measure. + file2 = amo.tests.file_factory(version=version, status=file_status) + # If the addon is public, and we change its only file to something else + # than public, it'll change to unreviewed. + addon.update(status=addon_status) + assert addon.reload().status == addon_status + assert file1.reload().status == file_status + assert file2.reload().status == file_status + + return (addon, file1, file2, review_type) + + +@pytest.fixture +def mozilla_user(db): + """Create and return the "mozilla" user used to auto approve addons.""" + return amo.tests.user_factory(id=settings.TASK_USER_ID) + + +def test_approve_addons_get_files(use_case): + """Files that need to get approved are returned in the list. + + Use cases are quadruples taken from the "use_case" fixture above. + """ + addon, file1, file2, review_type = use_case + assert approve_addons.get_files([addon.guid]) == [file1, file2] + + +@pytest.mark.django_db +def test_approve_addons_approve_files_no_review_type(): + """Files which don't need approval don't change status.""" + # Create the "mozilla" user, needed for the log. + amo.tests.user_factory(id=settings.TASK_USER_ID) + addon = amo.tests.addon_factory(status=amo.STATUS_PUBLIC) + file_ = addon.versions.get().files.get() + file_.update(status=amo.STATUS_PUBLIC) + approve_addons.approve_files([(file_, None)]) + # Nothing changed. + assert addon.reload().status == amo.STATUS_PUBLIC + assert file_.reload().status == amo.STATUS_PUBLIC + + +def test_approve_addons_approve_files(use_case, mozilla_user): + """Files are approved using the correct review type. + + Use cases are quadruples taken from the "use_case" fixture above. + """ + addon, file1, file2, review_type = use_case + approve_addons.approve_files([(file1, review_type), + (file2, review_type)]) + assert file1.reload().status == ( + amo.STATUS_LITE if review_type == 'prelim' else amo.STATUS_PUBLIC) + assert file2.reload().status == ( + amo.STATUS_LITE if review_type == 'prelim' else amo.STATUS_PUBLIC) + logs = AddonLog.objects.filter(addon=addon) + assert len(logs) == 2 # One per file. + file1_log, file2_log = logs + # An AddonLog has been created for each approval. + assert file1_log.activity_log.details['comments'] == u'bulk approval' + assert file1_log.activity_log.user == mozilla_user + assert file2_log.activity_log.details['comments'] == u'bulk approval' + assert file2_log.activity_log.user == mozilla_user + # No ReviewerScore was granted, it's an automatic approval. + assert not ReviewerScore.objects.all() + + +@pytest.mark.django_db +def test_approve_addons_get_review_type_already_approved(): + """The review type for a file that doesn't need approval is None.""" + addon = amo.tests.addon_factory(status=amo.STATUS_PUBLIC) + file_ = addon.versions.get().files.get() + file_.update(status=amo.STATUS_PUBLIC) + assert approve_addons.get_review_type(file_) is None + + +def test_approve_addons_get_review_type(use_case): + """Review type depends on the file and addon status. + + Use cases are quadruples taken from the "use_case" fixture above. + """ + addon, file1, _, review_type = use_case + assert approve_addons.get_review_type(file1) == review_type diff --git a/apps/constants/base.py b/apps/constants/base.py index 914141fc80..fd2dca0543 100644 --- a/apps/constants/base.py +++ b/apps/constants/base.py @@ -109,6 +109,12 @@ LITE_STATUSES = (STATUS_LITE, STATUS_LITE_AND_NOMINATED) MIRROR_STATUSES = (STATUS_PUBLIC, STATUS_BETA, STATUS_LITE, STATUS_LITE_AND_NOMINATED) +# Fully reviewed of waiting for a full review. +FULL_REVIEW_STATUSES = [STATUS_NOMINATED, STATUS_LITE_AND_NOMINATED, + STATUS_PUBLIC] +# Prelim reviewed of waiting for a prelim review. +PRELIM_REVIEW_STATUSES = [STATUS_UNREVIEWED, STATUS_LITE] + # An add-on in one of these statuses can become premium. PREMIUM_STATUSES = (STATUS_NULL,) + UNDER_REVIEW_STATUSES diff --git a/apps/editors/helpers.py b/apps/editors/helpers.py index e36d8ca728..b0e5f2699d 100644 --- a/apps/editors/helpers.py +++ b/apps/editors/helpers.py @@ -711,7 +711,9 @@ class ReviewAddon(ReviewBase): log.info(u'Sending email for %s' % (self.addon)) # Assign reviewer incentive scores. - ReviewerScore.award_points(self.request.amo_user, self.addon, status) + if self.request: + ReviewerScore.award_points(self.request.amo_user, self.addon, + status) def process_sandbox(self): """Set an addon back to sandbox.""" @@ -741,7 +743,9 @@ class ReviewAddon(ReviewBase): log.info(u'Sending email for %s' % (self.addon)) # Assign reviewer incentive scores. - ReviewerScore.award_points(self.request.amo_user, self.addon, status) + if self.request: + ReviewerScore.award_points(self.request.amo_user, self.addon, + status) def process_preliminary(self, auto_validation=False): """Set an addon to preliminary.""" diff --git a/settings_test.py b/settings_test.py index ea16b7dc7a..8a848f555a 100644 --- a/settings_test.py +++ b/settings_test.py @@ -105,7 +105,7 @@ AMO_LANGUAGES = ( ) LANGUAGES = lazy(lazy_langs, dict)(AMO_LANGUAGES) LANGUAGE_URL_MAP = dict([(i.lower(), i) for i in AMO_LANGUAGES]) -TASK_USER_ID = '4043307' +TASK_USER_ID = 4043307 PASSWORD_HASHERS = ( 'django.contrib.auth.hashers.MD5PasswordHasher',