From 998a2b7cdc2d8b30f7e88f074629fdd93a01a9b1 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Fri, 16 Jun 2023 15:22:17 +0100 Subject: [PATCH] create BlockVersion instances for versions in Block.min - max (#20837) * create BlockVersion instances for versions in Block.min - max * review fixes --- docs/topics/api/blocklist.rst | 1 + docs/topics/api/overview.rst | 1 + .../management/commands/process_addons.py | 18 ---- src/olympia/addons/tests/test_commands.py | 34 -------- src/olympia/blocklist/admin.py | 12 +-- .../management/commands/bulk_add_blocks.py | 45 ---------- .../commands/create_blockversions.py | 32 +++++++ ...stsubmission_signoff_state_blockversion.py | 70 ++++++++++++++++ src/olympia/blocklist/models.py | 11 ++- src/olympia/blocklist/serializers.py | 11 +++ src/olympia/blocklist/tests/test_admin.py | 49 ++++++++--- src/olympia/blocklist/tests/test_commands.py | 83 ++++++++++--------- .../blocklist/tests/test_serializers.py | 10 ++- src/olympia/blocklist/utils.py | 17 +++- src/olympia/reviewers/tests/test_utils.py | 8 +- src/olympia/reviewers/utils.py | 13 +-- 16 files changed, 240 insertions(+), 175 deletions(-) delete mode 100644 src/olympia/blocklist/management/commands/bulk_add_blocks.py create mode 100644 src/olympia/blocklist/management/commands/create_blockversions.py create mode 100644 src/olympia/blocklist/migrations/0030_alter_blocklistsubmission_signoff_state_blockversion.py diff --git a/docs/topics/api/blocklist.rst b/docs/topics/api/blocklist.rst index d5461bf146..6c413555f3 100644 --- a/docs/topics/api/blocklist.rst +++ b/docs/topics/api/blocklist.rst @@ -30,3 +30,4 @@ This endpoint returns an add-on Block from the blocklist, specified by guid or i :>json string max_version: The maximum version of the add-on that will be blocked. "*" is the highest version, meaning all versions from min_version will be blocked. ("0" - "*" would be all versions). :>json string|null reason: Why the add-on needed to be blocked. :>json object|null url: A url to the report/request that detailed why the add-on should potentially be blocked. Typically a bug report on bugzilla.mozilla.org. (See :ref:`Outgoing Links `) + :>json string versions[]: The versions of this add-on that are blocked. diff --git a/docs/topics/api/overview.rst b/docs/topics/api/overview.rst index 2b7b2f96c0..15ed30a87e 100644 --- a/docs/topics/api/overview.rst +++ b/docs/topics/api/overview.rst @@ -462,6 +462,7 @@ These are `v5` specific changes - `v4` changes apply also. * 2023-04-13: removed signing api from api/v5+ in favor of addon submission api. https://github.com/mozilla/addons-server/issues/20560 * 2023-06-01: renamed add-ons search endpoint sort by ratings parameter to ``sort=ratings``, ``sort=rating`` is still supported for backwards-compatibility. https://github.com/mozilla/addons-server/issues/20763 * 2023-06-06: added the /addons/browser-mappings/ endpoint. https://github.com/mozilla/addons-server/issues/20798 +* 2023-06-22: added ``versions`` to blocklist block endpoint. https://github.com/mozilla/addons-server/issues/20748 .. _`#11380`: https://github.com/mozilla/addons-server/issues/11380/ .. _`#11379`: https://github.com/mozilla/addons-server/issues/11379/ diff --git a/src/olympia/addons/management/commands/process_addons.py b/src/olympia/addons/management/commands/process_addons.py index c8f5a86fd1..b7e85b61b3 100644 --- a/src/olympia/addons/management/commands/process_addons.py +++ b/src/olympia/addons/management/commands/process_addons.py @@ -1,20 +1,17 @@ from datetime import datetime from django.db.models import Exists, F, OuterRef, Q -from django.db.models.functions import Collate from olympia import amo from olympia.abuse.models import AbuseReport from olympia.addons.models import Addon from olympia.addons.tasks import ( delete_addons, - disable_addons, extract_colors_from_static_themes, find_inconsistencies_between_es_and_db, recreate_theme_previews, ) from olympia.amo.management import ProcessObjectsCommand -from olympia.blocklist.models import Block from olympia.constants.base import ( _ADDON_LPADDON, _ADDON_PERSONA, @@ -150,21 +147,6 @@ class Command(ProcessObjectsCommand): 'task': addon_rating_aggregates, 'queryset_filters': [Q(status=amo.STATUS_APPROVED)], }, - 'disable_blocked_addons': { - 'task': disable_addons, - 'queryset_filters': [ - Q( - status__in=( - amo.STATUS_NULL, - amo.STATUS_NOMINATED, - amo.STATUS_APPROVED, - ), - guid__in=Block.objects.filter( - min_version=Block.MIN, max_version=Block.MAX - ).values(guidc=Collate('guid', 'utf8mb4_unicode_ci')), - ) - ], - }, } def add_arguments(self, parser): diff --git a/src/olympia/addons/tests/test_commands.py b/src/olympia/addons/tests/test_commands.py index 7f30422326..58a3dea426 100644 --- a/src/olympia/addons/tests/test_commands.py +++ b/src/olympia/addons/tests/test_commands.py @@ -24,11 +24,9 @@ from olympia.amo.tests import ( version_factory, ) from olympia.applications.models import AppVersion -from olympia.blocklist.models import Block from olympia.files.models import FileValidation from olympia.ratings.models import Rating, RatingAggregate from olympia.reviewers.models import AutoApprovalSummary -from olympia.users.models import UserProfile from olympia.versions.models import ApplicationsVersions, Version, VersionPreview @@ -578,35 +576,3 @@ def test_delete_list_theme_previews(): assert VersionPreview.objects.filter(id=other_firefox_preview.id).exists() assert VersionPreview.objects.filter(id=other_amo_preview.id).exists() assert not VersionPreview.objects.filter(id=other_old_list_preview.id).exists() - - -@pytest.mark.django_db -def test_disable_blocked_addons(): - UserProfile.objects.create(pk=settings.TASK_USER_ID) - user = user_factory() - incomplete = addon_factory(status=amo.STATUS_NULL) - Block.objects.create(guid=incomplete.guid, updated_by=user) - nominated = addon_factory(status=amo.STATUS_NOMINATED) - Block.objects.create(guid=nominated.guid, updated_by=user) - approved = addon_factory(status=amo.STATUS_APPROVED) - Block.objects.create(guid=approved.guid, updated_by=user) - deleted = addon_factory(status=amo.STATUS_DELETED) - Block.objects.create(guid=deleted.guid, updated_by=user) - only_partially_blocked = addon_factory(status=amo.STATUS_APPROVED) - Block.objects.create( - guid=only_partially_blocked.guid, min_version='1', updated_by=user - ) - addon_factory() # just a random addon - assert Addon.unfiltered.filter(status=amo.STATUS_DISABLED).count() == 0 - - call_command('process_addons', task='disable_blocked_addons') - - # these should have changed - assert incomplete.reload().status == amo.STATUS_DISABLED - assert nominated.reload().status == amo.STATUS_DISABLED - assert approved.reload().status == amo.STATUS_DISABLED - # these shouldn't have been changed - assert deleted.reload().status == amo.STATUS_DELETED - assert only_partially_blocked.reload().status == amo.STATUS_APPROVED - - assert Addon.unfiltered.filter(status=amo.STATUS_DISABLED).count() == 3 diff --git a/src/olympia/blocklist/admin.py b/src/olympia/blocklist/admin.py index e4eaf99722..9d95753395 100644 --- a/src/olympia/blocklist/admin.py +++ b/src/olympia/blocklist/admin.py @@ -139,12 +139,12 @@ class BlockAdminAddMixin: def add_from_addon_pk_view(self, request, pk, **kwargs): addon = get_object_or_404(Addon.unfiltered, pk=pk or kwargs.get('pk')) get_params = request.GET.copy() - for key in ('min', 'max'): - if key in get_params: - version = get_object_or_404( - Version.unfiltered, pk=get_params.pop(key)[0] - ) - get_params[f'{key}_version'] = version.version + if changed_version_ids := get_params.pop('v', None): + version_strings = Version.unfiltered.filter( + id__in=(int(id_) for id_ in changed_version_ids) + ).values_list('version', flat=True) + get_params['min_version'] = min(version_strings) + get_params['max_version'] = max(version_strings) if 'min_version' in get_params or 'max_version' in get_params: warning_message = ( diff --git a/src/olympia/blocklist/management/commands/bulk_add_blocks.py b/src/olympia/blocklist/management/commands/bulk_add_blocks.py deleted file mode 100644 index ccbb6073e1..0000000000 --- a/src/olympia/blocklist/management/commands/bulk_add_blocks.py +++ /dev/null @@ -1,45 +0,0 @@ -from django.core.management.base import BaseCommand - -import olympia.core.logger -from olympia.amo.utils import chunked -from olympia.blocklist.models import BlocklistSubmission -from olympia.blocklist.utils import save_guids_to_blocks, splitlines -from olympia.users.utils import get_task_user - - -log = olympia.core.logger.getLogger('z.amo.blocklist') - - -class Command(BaseCommand): - help = 'Create Blocks for provided guids to add them to the v3 blocklist' - - def add_arguments(self, parser): - """Handle command arguments.""" - parser.add_argument('--min_version') - parser.add_argument('--max_version') - parser.add_argument('--reason') - parser.add_argument('--url'), - parser.add_argument( - '--guids-input', - help='Path to file with one guid per line that should be blocked', - required=True, - ) - - def handle(self, *args, **options): - with open(options.get('guids_input')) as guid_file: - input_guids = guid_file.read() - guids = splitlines(input_guids) - - block_args = { - prop: options.get(prop) - for prop in ('min_version', 'max_version', 'reason', 'url') - if options.get(prop) - } - block_args['updated_by'] = get_task_user() - submission = BlocklistSubmission(**block_args) - - for guids_chunk in chunked(guids, 100): - blocks = save_guids_to_blocks( - guids_chunk, submission, fields_to_set=block_args.keys() - ) - print(f'Added/Updated {len(blocks)} blocks from {len(guids_chunk)} guids') diff --git a/src/olympia/blocklist/management/commands/create_blockversions.py b/src/olympia/blocklist/management/commands/create_blockversions.py new file mode 100644 index 0000000000..34fa25c9fb --- /dev/null +++ b/src/olympia/blocklist/management/commands/create_blockversions.py @@ -0,0 +1,32 @@ +from django.core.management.base import BaseCommand + +import olympia.core.logger +from olympia.blocklist.models import Block, BlockVersion +from olympia.files.models import File + + +log = olympia.core.logger.getLogger('z.amo.blocklist') + + +class Command(BaseCommand): + help = 'Migration to create BlockVersion instances for every Block' + + def handle(self, *args, **options): + count = 0 + for block in Block.objects.all().iterator(1000): + versions_qs = ( + File.objects.filter(version__addon__addonguid__guid=block.guid) + .exclude(version__blockversion__id__isnull=False) + .values_list('version__version', 'version_id') + ) + block_versions = [ + BlockVersion(version_id=version_id, block=block) + for version_str, version_id in versions_qs + if block.min_version <= version_str and block.max_version >= version_str + ] + BlockVersion.objects.bulk_create(block_versions) + count += 1 + if (count % 1000) == 0: + log.info('Progress: %s Blocks processed so far' % count) + + log.info('Finished: %s Blocks processed' % count) diff --git a/src/olympia/blocklist/migrations/0030_alter_blocklistsubmission_signoff_state_blockversion.py b/src/olympia/blocklist/migrations/0030_alter_blocklistsubmission_signoff_state_blockversion.py new file mode 100644 index 0000000000..df18239ec5 --- /dev/null +++ b/src/olympia/blocklist/migrations/0030_alter_blocklistsubmission_signoff_state_blockversion.py @@ -0,0 +1,70 @@ +# Generated by Django 4.2.1 on 2023-06-16 09:01 + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import olympia.amo.models + + +class Migration(migrations.Migration): + dependencies = [ + ('blocklist', '0029_alter_blocklistsubmission_delayed_until_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='blocklistsubmission', + name='signoff_state', + field=models.SmallIntegerField( + choices=[ + (0, 'Pending Sign-off'), + (1, 'Approved'), + (2, 'Rejected'), + (3, 'Auto Sign-off'), + (4, 'Published'), + ], + default=0, + ), + ), + migrations.CreateModel( + name='BlockVersion', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID', + ), + ), + ( + 'created', + models.DateTimeField( + blank=True, default=django.utils.timezone.now, editable=False + ), + ), + ('modified', models.DateTimeField(auto_now=True)), + ( + 'block', + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to='blocklist.block', + ), + ), + ( + 'version', + models.OneToOneField( + on_delete=django.db.models.deletion.CASCADE, + to='versions.version', + ), + ), + ], + options={ + 'get_latest_by': 'created', + 'abstract': False, + 'base_manager_name': 'objects', + }, + bases=(olympia.amo.models.SaveUpdateMixin, models.Model), + ), + ] diff --git a/src/olympia/blocklist/models.py b/src/olympia/blocklist/models.py index 618028628d..45dd622194 100644 --- a/src/olympia/blocklist/models.py +++ b/src/olympia/blocklist/models.py @@ -100,6 +100,7 @@ class Block(ModelBase): .order_by('id') .no_transforms() .annotate(**{GUID: models.F(GUID)}) + .select_related('blockversion') ) all_addon_versions = defaultdict(list) @@ -185,6 +186,14 @@ class Block(ModelBase): return list(existing_blocks.values()) +class BlockVersion(ModelBase): + version = models.OneToOneField(Version, on_delete=models.CASCADE) + block = models.ForeignKey(Block, on_delete=models.CASCADE) + + def __str__(self) -> str: + return f'Block.id={self.block_id} -> Version.id={self.version_id}' + + class BlocklistSubmissionQuerySet(BaseQuerySet): def delayed(self): return self.filter(delayed_until__gt=datetime.now()) @@ -208,7 +217,7 @@ class BlocklistSubmission(ModelBase): SIGNOFF_APPROVED: 'Approved', SIGNOFF_REJECTED: 'Rejected', SIGNOFF_AUTOAPPROVED: 'Auto Sign-off', - SIGNOFF_PUBLISHED: 'Published to Blocks', + SIGNOFF_PUBLISHED: 'Published', } SIGNOFF_STATES_APPROVED = ( SIGNOFF_APPROVED, diff --git a/src/olympia/blocklist/serializers.py b/src/olympia/blocklist/serializers.py index ecef4b9c91..b21c71ee15 100644 --- a/src/olympia/blocklist/serializers.py +++ b/src/olympia/blocklist/serializers.py @@ -1,3 +1,5 @@ +from rest_framework import fields + from olympia.api.fields import OutgoingURLField, TranslationSerializerField from olympia.api.serializers import AMOModelSerializer @@ -7,6 +9,7 @@ from .models import Block class BlockSerializer(AMOModelSerializer): addon_name = TranslationSerializerField(source='addon.name') url = OutgoingURLField() + versions = fields.SerializerMethodField() class Meta: model = Block @@ -20,4 +23,12 @@ class BlockSerializer(AMOModelSerializer): 'max_version', 'reason', 'url', + 'versions', + ) + + def get_versions(self, obj): + return list( + obj.blockversion_set.order_by('version__version').values_list( + 'version__version', flat=True + ) ) diff --git a/src/olympia/blocklist/tests/test_admin.py b/src/olympia/blocklist/tests/test_admin.py index d02aacd167..2d1e0729ea 100644 --- a/src/olympia/blocklist/tests/test_admin.py +++ b/src/olympia/blocklist/tests/test_admin.py @@ -139,7 +139,7 @@ class TestBlockAdmin(TestCase): self.grant_permission(user, 'Blocklist:Create') self.client.force_login(user) - addon = addon_factory() + addon = addon_factory(version_kw={'version': '123.456'}) url = reverse('admin:blocklist_block_addaddon', args=(addon.id,)) response = self.client.post(url, follow=True) self.assertRedirects(response, self.submission_url + f'?guids={addon.guid}') @@ -154,37 +154,60 @@ class TestBlockAdmin(TestCase): # GET params are passed along version = addon.current_version + second_version = version_factory( + addon=addon, channel=amo.CHANNEL_UNLISTED, version='100.000.000' + ) response = self.client.post( - url + f'?min_version={version.version}', follow=True + url + f'?min_version={version.version}&', follow=True ) self.assertRedirects( response, self.submission_url + f'?guids={addon.guid}&min_version={version.version}', ) - # And version ids as short params are expanded and passed along - response = self.client.post(url + f'?max={version.pk}', follow=True) + # And version ids are expanded and passed along + response = self.client.post( + url + f'?v={version.pk}&v={second_version.pk}', follow=True + ) self.assertRedirects( response, - self.submission_url + f'?guids={addon.guid}&max_version={version.version}', + self.submission_url + + f'?guids={addon.guid}' + + f'&min_version={second_version.version}&max_version={version.version}', + ) + assert not response.context['messages'] + + # The order of the params doesn't determine max and min + response = self.client.post( + url + f'?v={second_version.pk}&v={version.pk}', follow=True + ) + self.assertRedirects( + response, + self.submission_url + + f'?guids={addon.guid}' + + f'&min_version={second_version.version}&max_version={version.version}', ) assert not response.context['messages'] # Existing blocks are redirected to the change view instead block = Block.objects.create(addon=addon, updated_by=user_factory()) - response = self.client.post(url + f'?max={version.pk}', follow=True) + response = self.client.post( + url + f'?v={version.pk}&v={second_version.pk}', follow=True + ) self.assertRedirects( response, reverse('admin:blocklist_block_change', args=(block.pk,)) ) # with a message warning the versions were ignored assert [msg.message for msg in response.context['messages']] == [ - f'The versions 0 to {version.version} could not be pre-selected ' - 'because some versions have been blocked already' + f'The versions {second_version.version} to {version.version} could not be ' + 'pre-selected because some versions have been blocked already' ] # Pending blocksubmissions are redirected to the submission view submission = BlocklistSubmission.objects.create(input_guids=addon.guid) - response = self.client.post(url + f'?max={version.pk}', follow=True) + response = self.client.post( + url + f'?v={version.pk}&v={second_version.pk}', follow=True + ) self.assertRedirects( response, reverse( @@ -193,8 +216,8 @@ class TestBlockAdmin(TestCase): ) # with a message warning the versions were ignored assert [msg.message for msg in response.context['messages']] == [ - f'The versions 0 to {version.version} could not be pre-selected ' - 'because this addon is part of a pending submission' + f'The versions {second_version.version} to {version.version} could not be ' + 'pre-selected because this addon is part of a pending submission' ] def test_guid_redirects(self): @@ -1626,7 +1649,7 @@ class TestBlocklistSubmissionAdmin(TestCase): ('Approved', '?signoff_state=1'), ('Rejected', '?signoff_state=2'), ('Auto Sign-off', '?signoff_state=3'), - ('Published to Blocks', '?signoff_state=4'), + ('Published', '?signoff_state=4'), ] filters = [(x.text, x.attrib['href']) for x in doc('#changelist-filter a')] assert filters == expected_filters @@ -1660,7 +1683,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert doc('#result_list tbody tr').length == 3 assert doc('#changelist-filter li.selected a').text() == 'All' assert doc('#changelist-form td.field-state').text() == ( - 'Published to Blocks Approved:Delayed Pending Sign-off' + 'Published Approved:Delayed Pending Sign-off' ) def test_blocked_deleted_keeps_addon_status(self): diff --git a/src/olympia/blocklist/tests/test_commands.py b/src/olympia/blocklist/tests/test_commands.py index 1b49e4717f..40ce432315 100644 --- a/src/olympia/blocklist/tests/test_commands.py +++ b/src/olympia/blocklist/tests/test_commands.py @@ -1,36 +1,28 @@ -import json import os from django.conf import settings from django.core.management import call_command -from django.core.management.base import CommandError from olympia import amo -from olympia.amo.tests import TestCase, addon_factory, user_factory +from olympia.amo.tests import TestCase, addon_factory, user_factory, version_factory -from ..models import Block - - -# This is a fragment of the actual json blocklist file -TESTS_DIR = os.path.dirname(os.path.abspath(__file__)) -blocklist_file = os.path.join(TESTS_DIR, 'blocklists', 'blocklist.json') -with open(blocklist_file) as file_object: - blocklist_json = json.load(file_object) +from ..models import Block, BlockVersion class TestExportBlocklist(TestCase): def test_command(self): - for idx in range(0, 5): + user = user_factory() + for _idx in range(0, 5): addon_factory() # one version, 0 - * Block.objects.create( addon=addon_factory(file_kw={'is_signed': True}), - updated_by=user_factory(), + updated_by=user, ) # one version, 0 - 9999 Block.objects.create( addon=addon_factory(file_kw={'is_signed': True}), - updated_by=user_factory(), + updated_by=user, max_version='9999', ) # one version, 0 - *, unlisted @@ -39,7 +31,7 @@ class TestExportBlocklist(TestCase): version_kw={'channel': amo.CHANNEL_UNLISTED}, file_kw={'is_signed': True}, ), - updated_by=user_factory(), + updated_by=user, ) call_command('export_blocklist', '1') @@ -47,34 +39,43 @@ class TestExportBlocklist(TestCase): assert os.path.exists(out_path) -class TestBulkAddBlocks(TestCase): +class TestCreateBlockversions(TestCase): def test_command(self): - user_factory(id=settings.TASK_USER_ID) + user = user_factory() + Block.objects.create(guid='missing@', min_version='123', updated_by=user) + addon = addon_factory(version_kw={'version': '0.1'}) + v1 = version_factory(addon=addon, version='1') + v2 = version_factory(addon=addon, version='2.0.0') + v2.delete() + minmax_block = Block.objects.create( + addon=addon, min_version='0.1.1', max_version='2', updated_by=user + ) + full_block = Block.objects.create(addon=addon_factory(), updated_by=user) - with self.assertRaises(CommandError): - # no input guids - call_command('bulk_add_blocks') - assert Block.objects.count() == 0 + call_command('create_blockversions') - guids_path = os.path.join(TESTS_DIR, 'blocklists', 'input_guids.txt') - call_command('bulk_add_blocks', guids_input=guids_path) - # no guids matching addons - assert Block.objects.count() == 0 + assert BlockVersion.objects.count() == 3 + v1.refresh_from_db() + v2.refresh_from_db() + full_block.refresh_from_db() + assert v1.blockversion.block == minmax_block + assert v2.blockversion.block == minmax_block + assert full_block.addon.current_version.blockversion.block == full_block - addon_factory(guid='{0020bd71-b1ba-4295-86af-db7f4e7eaedc}') - addon_factory(guid='{another-random-guid') - call_command('bulk_add_blocks', guids_input=guids_path) - # this time we have 1 matching guid so one Block created - assert Block.objects.count() == 1 - assert Block.objects.last().guid == ('{0020bd71-b1ba-4295-86af-db7f4e7eaedc}') + # we can run it again without a problem + call_command('create_blockversions') + assert BlockVersion.objects.count() == 3 - addon_factory(guid='{00116dc4-ba1f-42c5-b20c-da7f743f7377}') - call_command('bulk_add_blocks', guids_input=guids_path, min_version='44') - # The guid before shouldn't be added twice - # assert Block.objects.count() == 2 - prev_block = Block.objects.first() - assert prev_block.guid == '{0020bd71-b1ba-4295-86af-db7f4e7eaedc}' - assert prev_block.min_version == '44' - new_block = Block.objects.last() - assert new_block.guid == '{00116dc4-ba1f-42c5-b20c-da7f743f7377}' - assert new_block.min_version == '44' + # and extra versions / blocks are created + new_version = version_factory( + addon=addon, channel=amo.CHANNEL_UNLISTED, version='0.2' + ) + new_block = Block.objects.create(addon=addon_factory(), updated_by=user) + + call_command('create_blockversions') + + assert BlockVersion.objects.count() == 5 + new_block.refresh_from_db() + new_version.refresh_from_db() + assert new_version.blockversion.block == minmax_block + assert new_block.addon.current_version.blockversion.block == new_block diff --git a/src/olympia/blocklist/tests/test_serializers.py b/src/olympia/blocklist/tests/test_serializers.py index ed3a978358..bf93fa122b 100644 --- a/src/olympia/blocklist/tests/test_serializers.py +++ b/src/olympia/blocklist/tests/test_serializers.py @@ -1,7 +1,8 @@ from olympia.amo.tests import TestCase, addon_factory, user_factory from olympia.amo.urlresolvers import get_outgoing_url -from olympia.blocklist.models import Block -from olympia.blocklist.serializers import BlockSerializer + +from ..models import Block, BlockVersion +from ..serializers import BlockSerializer class TestBlockSerializer(TestCase): @@ -27,11 +28,16 @@ class TestBlockSerializer(TestCase): 'url': 'https://goo.gol', 'outgoing': get_outgoing_url('https://goo.gol'), }, + 'versions': [], 'created': self.block.created.isoformat()[:-7] + 'Z', 'modified': self.block.modified.isoformat()[:-7] + 'Z', } def test_with_addon(self): addon_factory(guid=self.block.guid, name='Addón náme') + BlockVersion.objects.create( + block=self.block, version=self.block.addon.current_version + ) serializer = BlockSerializer(instance=self.block) assert serializer.data['addon_name'] == {'en-US': 'Addón náme'} + assert serializer.data['versions'] == [self.block.addon.current_version.version] diff --git a/src/olympia/blocklist/utils.py b/src/olympia/blocklist/utils.py index 129dcf2ecd..f0b453e0e8 100644 --- a/src/olympia/blocklist/utils.py +++ b/src/olympia/blocklist/utils.py @@ -143,7 +143,7 @@ def disable_addon_for_block(block): def save_guids_to_blocks(guids, submission, *, fields_to_set): - from .models import Block + from .models import Block, BlockVersion common_args = {field: getattr(submission, field) for field in fields_to_set} modified_datetime = datetime.now() @@ -163,6 +163,21 @@ def save_guids_to_blocks(guids, submission, *, fields_to_set): setattr(block, field, val) block.average_daily_users_snapshot = block.current_adu block.save() + + if change: + # if not a new Block then delete any BlockVersions that are outside min-max + BlockVersion.objects.filter(block=block).exclude( + version__version__gte=block.min_version, + version__version__lte=block.max_version, + ).delete() + # create BlockVersions for versions in min-max range, that don't already exist + BlockVersion.objects.bulk_create( + BlockVersion(version=version, block=block) + for version in block.addon_versions + if not hasattr(version, 'blockversion') + and block.is_version_blocked(version.version) + ) + if submission.id: block.submission.add(submission) block_activity_log_save( diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index 82cbfaeeef..c0602a4160 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -2756,7 +2756,7 @@ class TestReviewHelper(TestReviewHelperBase): # We should have set redirect_url to point to the Block admin page if '%s' in redirect_url: - redirect_url = redirect_url % (old_version.pk, self.review_version.pk) + redirect_url = redirect_url % (self.review_version.pk, old_version.pk) assert self.helper.redirect_url == redirect_url def test_pending_blocklistsubmission_multiple_unlisted_versions(self): @@ -2765,7 +2765,7 @@ class TestReviewHelper(TestReviewHelperBase): ) redirect_url = ( reverse('admin:blocklist_block_addaddon', args=(self.addon.id,)) - + '?min=%s&max=%s' + + '?v=%s&v=%s' ) assert Block.objects.count() == 0 self._test_block_multiple_unlisted_versions(redirect_url) @@ -2773,7 +2773,7 @@ class TestReviewHelper(TestReviewHelperBase): def test_new_block_multiple_unlisted_versions(self): redirect_url = ( reverse('admin:blocklist_block_addaddon', args=(self.addon.id,)) - + '?min=%s&max=%s' + + '?v=%s&v=%s' ) assert Block.objects.count() == 0 self._test_block_multiple_unlisted_versions(redirect_url) @@ -2782,7 +2782,7 @@ class TestReviewHelper(TestReviewHelperBase): Block.objects.create(guid=self.addon.guid, updated_by=user_factory()) redirect_url = ( reverse('admin:blocklist_block_addaddon', args=(self.addon.id,)) - + '?min=%s&max=%s' + + '?v=%s&v=%s' ) self._test_block_multiple_unlisted_versions(redirect_url) diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 4e302b88f0..0943a1e2ea 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -8,6 +8,7 @@ from django.db.models import Count, F, Q from django.template import loader from django.urls import reverse from django.utils import translation +from django.utils.http import urlencode import django_tables2 as tables import markupsafe @@ -1452,16 +1453,8 @@ class ReviewUnlisted(ReviewBase): log.info('Sending email for %s' % (self.addon)) def block_multiple_versions(self): - min_version = ('0', None) - max_version = ('*', None) - for version in self.data['versions']: - version_str = version.version - if not min_version[1] or version_str < min_version[1]: - min_version = (version, version_str) - if not max_version[1] or version_str > max_version[1]: - max_version = (version, version_str) - - params = f'?min={min_version[0].pk}&max={max_version[0].pk}' + versions = self.data['versions'] + params = '?' + urlencode((('v', v.id) for v in versions), doseq=True) self.redirect_url = ( reverse('admin:blocklist_block_addaddon', args=(self.addon.pk,)) + params )