From 1d89978fbb8fc03a520c04f76b12eccb4c5db9e1 Mon Sep 17 00:00:00 2001 From: Andrea Marchesini Date: Thu, 31 Oct 2024 14:39:11 +0100 Subject: [PATCH] Migrate from BlockVersion.soft to BlockVersion.block_type This patch does the following: - Introduce `PositiveTinyIntegerField` to support `tinyint unsigned` fields - Rename `soft` to `block_type` via 2 separate migrations (add + remove) - Introduce `BlockType` enum and use it instead of the `soft` boolean - Unify the string versions of enum values --- src/olympia/amo/fields.py | 5 ++ src/olympia/amo/tests/__init__.py | 6 +-- src/olympia/blocklist/admin.py | 2 +- ..._alter_blockversion_block_type_and_more.py | 46 ++++++++++++++++ src/olympia/blocklist/mlbf.py | 4 +- src/olympia/blocklist/models.py | 42 ++++++--------- src/olympia/blocklist/serializers.py | 14 ++--- .../admin/blocklist/widgets/blocks.html | 4 +- src/olympia/blocklist/tests/test_admin.py | 52 +++++++++++-------- src/olympia/blocklist/tests/test_cron.py | 12 +++-- src/olympia/blocklist/tests/test_forms.py | 14 ++--- src/olympia/blocklist/tests/test_mlbf.py | 46 ++++++++++------ src/olympia/blocklist/tests/test_models.py | 6 +-- .../blocklist/tests/test_serializers.py | 18 +++++-- src/olympia/blocklist/tests/test_tasks.py | 10 ++-- src/olympia/blocklist/tests/test_utils.py | 27 ++++++---- src/olympia/blocklist/utils.py | 14 ++--- src/olympia/constants/blocklist.py | 12 ----- .../devhub/tests/test_views_versions.py | 3 +- 19 files changed, 209 insertions(+), 128 deletions(-) create mode 100644 src/olympia/blocklist/migrations/0037_alter_blockversion_block_type_and_more.py diff --git a/src/olympia/amo/fields.py b/src/olympia/amo/fields.py index 9fad7739cc86..2fdce5a56e4c 100644 --- a/src/olympia/amo/fields.py +++ b/src/olympia/amo/fields.py @@ -143,3 +143,8 @@ def formfield(self, **kwargs): defaults = {'form_class': fields.CharField, 'validators': self.validators} defaults.update(kwargs) return super().formfield(**defaults) + + +class PositiveTinyIntegerField(models.PositiveSmallIntegerField): + def db_type(self, connection): + return 'tinyint unsigned' diff --git a/src/olympia/amo/tests/__init__.py b/src/olympia/amo/tests/__init__.py index 9532e614858b..97b0ebcc37b8 100644 --- a/src/olympia/amo/tests/__init__.py +++ b/src/olympia/amo/tests/__init__.py @@ -53,7 +53,7 @@ from olympia.api.tests import JWTAuthKeyTester from olympia.applications.models import AppVersion from olympia.bandwagon.models import Collection -from olympia.blocklist.models import Block, BlockVersion +from olympia.blocklist.models import Block, BlockType, BlockVersion from olympia.constants.categories import CATEGORIES from olympia.files.models import File from olympia.promoted.models import ( @@ -989,13 +989,13 @@ def version_factory(file_kw=None, **kw): return ver -def block_factory(*, version_ids=None, soft=False, **kwargs): +def block_factory(*, version_ids=None, block_type=BlockType.BLOCKED, **kwargs): block = Block.objects.create(**kwargs) if version_ids is None and block.addon: version_ids = list(block.addon.versions.values_list('id', flat=True)) if version_ids is not None: BlockVersion.objects.bulk_create( - BlockVersion(block=block, version_id=version_id, soft=soft) + BlockVersion(block=block, version_id=version_id, block_type=block_type) for version_id in version_ids ) return block diff --git a/src/olympia/blocklist/admin.py b/src/olympia/blocklist/admin.py index 60bf8bef0db1..7935bafb2cd7 100644 --- a/src/olympia/blocklist/admin.py +++ b/src/olympia/blocklist/admin.py @@ -574,7 +574,7 @@ def users(self, obj): def blocked_versions(self, obj): return ', '.join( - f'{version.version} ({version.get_soft_display()})' + f'{version.version} ({version.get_block_type_display()})' for version in obj.blockversion_set.all() ) diff --git a/src/olympia/blocklist/migrations/0037_alter_blockversion_block_type_and_more.py b/src/olympia/blocklist/migrations/0037_alter_blockversion_block_type_and_more.py new file mode 100644 index 000000000000..52c699178162 --- /dev/null +++ b/src/olympia/blocklist/migrations/0037_alter_blockversion_block_type_and_more.py @@ -0,0 +1,46 @@ +# Generated by Django 4.2.16 on 2024-10-31 13:26 + +from django.db import models, migrations +import olympia.amo.fields + + +def convert_soft_to_block_type(apps, schema_editor): + BlockVersion = apps.get_model('blocklist', 'BlockVersion') + # soft=True -> block_type=1 + # soft=False -> block_type=0 + BlockVersion.objects.filter(soft=True).update(block_type=1) + BlockVersion.objects.filter(soft=False).update(block_type=0) + + +def convert_block_type_to_soft(apps, schema_editor): + BlockVersion = apps.get_model('blocklist', 'BlockVersion') + # block_type=1 -> soft=True + # block_type=0 -> soft=False + BlockVersion.objects.filter(block_type=1).update(soft=True) + BlockVersion.objects.filter(block_type=0).update(soft=False) + + +class Migration(migrations.Migration): + + dependencies = [ + ('blocklist', '0036_blocklistsubmission_block_type_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='blockversion', + name='block_type', + field=olympia.amo.fields.PositiveTinyIntegerField(choices=[(0, 'Blocked'), (1, 'Restricted')], default=0, null=True), + ), + migrations.AlterField( + model_name='blockversion', + name='soft', + field=models.BooleanField(default=False, null=True), + ), + migrations.AlterField( + model_name='blocklistsubmission', + name='block_type', + field=olympia.amo.fields.PositiveTinyIntegerField(choices=[(0, 'Blocked'), (1, 'Restricted')], default=0), + ), + migrations.RunPython(convert_soft_to_block_type, convert_block_type_to_soft), + ] diff --git a/src/olympia/blocklist/mlbf.py b/src/olympia/blocklist/mlbf.py index 868e87f50fbb..1efd024283ff 100644 --- a/src/olympia/blocklist/mlbf.py +++ b/src/olympia/blocklist/mlbf.py @@ -11,7 +11,7 @@ import olympia.core.logger from olympia.amo.utils import SafeStorage -from olympia.blocklist.models import BlockVersion +from olympia.blocklist.models import BlockType, BlockVersion from olympia.blocklist.utils import datetime_to_ts @@ -61,7 +61,7 @@ class MLBFDataType(Enum): def fetch_blocked_from_db(): qs = BlockVersion.objects.filter( - version__file__is_signed=True, soft=False + version__file__is_signed=True, block_type=BlockType.BLOCKED ).values_list('block__guid', 'version__version', 'version_id', named=True) return set(qs) diff --git a/src/olympia/blocklist/models.py b/src/olympia/blocklist/models.py index e41a6be89735..bcf7d4582554 100644 --- a/src/olympia/blocklist/models.py +++ b/src/olympia/blocklist/models.py @@ -6,16 +6,16 @@ from django.urls import reverse from django.utils.functional import cached_property from django.utils.html import format_html +from django.utils.translation import gettext_lazy as _ -from extended_choices import Choices from multidb import get_replica from olympia import amo from olympia.addons.models import Addon +from olympia.amo.fields import PositiveTinyIntegerField from olympia.amo.models import BaseQuerySet, ManagerBase, ModelBase from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.utils import chunked -from olympia.constants.blocklist import BLOCK_TYPE_END_USERS_CHOICES from olympia.users.models import UserProfile from olympia.versions.models import Version @@ -157,24 +157,29 @@ def get_blocks_from_guids(cls, guids): return blocks +class BlockType(models.IntegerChoices): + BLOCKED = 0, '๐Ÿ›‘ Hard-Blocked' + SOFT_BLOCKED = 1, 'โš ๏ธ Soft-Blocked' + + class BlockVersion(ModelBase): - BLOCK_TYPE_CHOICES = Choices( - ('BLOCKED', 0, '๐Ÿ›‘ Hard-Blocked'), - ('SOFT_BLOCKED', 1, 'โš ๏ธ Soft-Blocked'), - ) version = models.OneToOneField(Version, on_delete=models.CASCADE) block = models.ForeignKey(Block, on_delete=models.CASCADE) - soft = models.BooleanField(default=False, choices=BLOCK_TYPE_CHOICES) + block_type = PositiveTinyIntegerField( + default=BlockType.BLOCKED, + choices=BlockType.choices, + null=True, + ) def __str__(self) -> str: return ( - f'Block.id={self.block_id} ({self.get_soft_display()}) ' + f'Block.id={self.block_id} ({self.get_block_type_display()}) ' f'-> Version.id={self.version_id}' ) def get_user_facing_block_type_display(self): - """Like get_soft_display(), but using strings meant for end-users.""" - return BLOCK_TYPE_END_USERS_CHOICES.for_value(int(self.soft)).display + """Like get_block_type_display(), but using strings meant for end-users.""" + return _('Blocked') if self.block_type == BlockType.BLOCKED else _('Restricted') class BlocklistSubmissionQuerySet(BaseQuerySet): @@ -216,18 +221,6 @@ class BlocklistSubmission(ModelBase): ACTION_ADDCHANGE: 'Add/Change', ACTION_DELETE: 'Delete', } - BLOCK_TYPE_CHOICES = Choices( - ( - BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED.constant, - BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED.value, - '๐Ÿ›‘ Hard-Block', - ), - ( - BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED.constant, - BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED.value, - 'โš ๏ธ Soft-Block', - ), - ) FakeBlockAddonVersion = namedtuple( 'FakeBlockAddonVersion', ( @@ -276,9 +269,8 @@ class BlocklistSubmission(ModelBase): help_text='The submission will not be published into blocks before this time.', ) disable_addon = models.BooleanField(default=True) - block_type = models.IntegerField( - default=BLOCK_TYPE_CHOICES.BLOCKED, - choices=BLOCK_TYPE_CHOICES, + block_type = PositiveTinyIntegerField( + default=BlockType.BLOCKED, choices=BlockType.choices ) objects = BlocklistSubmissionManager() diff --git a/src/olympia/blocklist/serializers.py b/src/olympia/blocklist/serializers.py index bd63f88dc7e2..deabab84d112 100644 --- a/src/olympia/blocklist/serializers.py +++ b/src/olympia/blocklist/serializers.py @@ -6,7 +6,7 @@ from olympia.api.utils import is_gate_active from olympia.versions.models import Version -from .models import Block +from .models import Block, BlockType class BlockSerializer(AMOModelSerializer): @@ -31,24 +31,24 @@ class Meta: 'is_all_versions', ) - def _get_blocked_for(self, obj, *, is_soft): + def _get_blocked_for(self, obj, *, block_type): if not hasattr(obj, '_blockversion_set_qs_values_list'): obj._blockversion_set_qs_values_list = sorted( obj.blockversion_set.order_by('version__version').values_list( - 'version__version', 'soft' + 'version__version', 'block_type' ) ) return [ version - for version, soft in obj._blockversion_set_qs_values_list - if soft == is_soft + for version, _block_type in obj._blockversion_set_qs_values_list + if _block_type == block_type ] def get_blocked(self, obj): - return self._get_blocked_for(obj, is_soft=False) + return self._get_blocked_for(obj, block_type=BlockType.BLOCKED) def get_soft_blocked(self, obj): - return self._get_blocked_for(obj, is_soft=True) + return self._get_blocked_for(obj, block_type=BlockType.SOFT_BLOCKED) def get_is_all_versions(self, obj): cannot_upload_new_versions = not obj.addon or obj.addon.status in ( diff --git a/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html b/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html index 6ab5e8a4567c..95c484347619 100644 --- a/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html +++ b/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html @@ -29,10 +29,10 @@

{{ blocks|length|intcomma }} Add-on GUIDs with {{ total_adu|intcomma }} user name="changed_version_ids" value="{{ version.id }}" {% if version.id in widget.value %}checked{% endif %} - > {% if is_add_change %}Block{% else %}Unblock{% endif %} {{ version.version }} {% if version.blockversion %}({{ version.blockversion.get_soft_display }}){% endif %} + > {% if is_add_change %}Block{% else %}Unblock{% endif %} {{ version.version }} {% if version.blockversion %}({{ version.blockversion.get_block_type_display }}){% endif %} {% else %} - {{ version.version }} ({% if version.is_blocked %}{{ version.blockversion.get_soft_display }}{% else %}🟢 Not Blocked{% endif %}) + {{ version.version }} ({% if version.is_blocked %}{{ version.blockversion.get_block_type_display }}{% else %}🟢 Not Blocked{% endif %}) {% if version.blocklist_submission_id %} [Edit Submission] {% endif %} diff --git a/src/olympia/blocklist/tests/test_admin.py b/src/olympia/blocklist/tests/test_admin.py index 18b1e4c20c96..ab664a5d5c32 100644 --- a/src/olympia/blocklist/tests/test_admin.py +++ b/src/olympia/blocklist/tests/test_admin.py @@ -26,7 +26,7 @@ ) from olympia.reviewers.models import NeedsHumanReview -from ..models import Block, BlocklistSubmission, BlockVersion +from ..models import Block, BlocklistSubmission, BlockType FANCY_QUOTE_OPEN = 'โ€œ' @@ -191,7 +191,9 @@ def test_view_versions(self): updated_by=user, ) # Make one of the blocks soft. - block.blockversion_set.get(version=third_version).update(soft=True) + block.blockversion_set.get(version=third_version).update( + block_type=BlockType.SOFT_BLOCKED + ) response = self.client.get( reverse('admin:blocklist_block_change', args=(block.id,)), @@ -308,7 +310,7 @@ def test_version_checkboxes(self): block_factory( addon=addon, version_ids=[ver_block.id, ver_soft_block.id], updated_by=user ) - ver_soft_block.blockversion.update(soft=True) + ver_soft_block.blockversion.update(block_type=BlockType.SOFT_BLOCKED) response = self.client.get( self.submission_url, @@ -420,7 +422,7 @@ def test_add_single(self): { 'input_guids': 'guid@', 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': changed_version_ids, 'url': 'dfd', 'reason': 'some reason', @@ -586,7 +588,7 @@ def _test_add_multiple_submit(self, addon_adu, delay=0): { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, partial_addon.current_version.id, @@ -808,7 +810,7 @@ def test_submit_dual_signoff(self): # and existing block wasn't updated multi = BlocklistSubmission.objects.get() - assert multi.block_type == BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED + assert multi.block_type == BlockType.BLOCKED multi.update( signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, signoff_by=user_factory(), @@ -858,7 +860,7 @@ def test_submit_no_metadata_updates(self): { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, partial_addon.current_version.id, @@ -1025,7 +1027,7 @@ def test_can_not_add_without_create_permission(self): { 'input_guids': 'guid@\nfoo@baa\ninvalid@', 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'url': 'dfd', 'reason': 'some reason', 'update_url_value': True, @@ -1142,7 +1144,7 @@ def test_edit_with_blocklist_create(self): doc = pq(response.content.decode('utf-8')) # Can't change block type when approving assert not doc('.field_block_type select') - assert doc('.field-block_type .readonly').text() == '๐Ÿ›‘ Hard-Block' + assert doc('.field-block_type .readonly').text() == '๐Ÿ›‘ Hard-Blocked' buttons = doc('.submit-row input') assert buttons[0].attrib['value'] == 'Update' assert len(buttons) == 1 @@ -1747,7 +1749,7 @@ def test_blocked_deleted_keeps_addon_status(self): { 'input_guids': 'guid@', 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [version.id], 'disable_addon': True, 'url': 'dfd', @@ -1795,7 +1797,7 @@ def test_blocking_addon_guid_already_denied(self): { 'input_guids': 'guid@', 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [version.id], 'url': 'dfd', 'reason': 'some reason', @@ -2050,7 +2052,7 @@ def test_not_disable_addon(self): { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, partial_addon.current_version.id, @@ -2081,9 +2083,14 @@ def test_not_disable_addon(self): assert partial_addon.status != amo.STATUS_DISABLED assert partial_addon_version.file.status == (amo.STATUS_DISABLED) - assert not new_addon_version.blockversion.soft - assert not partial_addon_version.blockversion.soft - assert not already_blocked_version.blockversion.soft + assert not new_addon_version.blockversion.block_type == BlockType.SOFT_BLOCKED + assert ( + not partial_addon_version.blockversion.block_type == BlockType.SOFT_BLOCKED + ) + assert ( + not already_blocked_version.blockversion.block_type + == BlockType.SOFT_BLOCKED + ) def test_soft_block(self): user = user_factory(email='someone@mozilla.com') @@ -2115,7 +2122,7 @@ def test_soft_block(self): { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED, + 'block_type': BlockType.SOFT_BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, new_partial_version.id, @@ -2133,14 +2140,13 @@ def test_soft_block(self): assert Block.objects.count() == 2 assert BlocklistSubmission.objects.count() == 1 + assert BlocklistSubmission.objects.get().block_type == BlockType.SOFT_BLOCKED + assert ( - BlocklistSubmission.objects.get().block_type - == BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED + new_addon.current_version.blockversion.block_type == BlockType.SOFT_BLOCKED ) - - assert new_addon.current_version.blockversion.soft - assert new_partial_version.blockversion.soft - assert not already_blocked_version.blockversion.soft + assert new_partial_version.blockversion.block_type == BlockType.SOFT_BLOCKED + assert already_blocked_version.blockversion.block_type == BlockType.BLOCKED todaysdate = datetime.now().date() response = self.client.get( @@ -2372,7 +2378,7 @@ def test_version_checkboxes(self): updated_by=user, version_ids=[ver_del_subm.id, ver_block.id, ver_soft_block.id], ) - ver_soft_block.blockversion.update(soft=True) + ver_soft_block.blockversion.update(block_type=BlockType.SOFT_BLOCKED) response = self.client.get( self.submission_url, { diff --git a/src/olympia/blocklist/tests/test_cron.py b/src/olympia/blocklist/tests/test_cron.py index ae10bc63a012..63e0e7caf7f2 100644 --- a/src/olympia/blocklist/tests/test_cron.py +++ b/src/olympia/blocklist/tests/test_cron.py @@ -25,7 +25,7 @@ upload_mlbf_to_remote_settings, ) from olympia.blocklist.mlbf import MLBF -from olympia.blocklist.models import Block, BlocklistSubmission, BlockVersion +from olympia.blocklist.models import Block, BlocklistSubmission, BlockType, BlockVersion from olympia.blocklist.utils import datetime_to_ts from olympia.constants.blocklist import MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY from olympia.zadmin.models import set_config @@ -73,12 +73,16 @@ def setUp(self): MLBF.generate_from_db(self.base_time) MLBF.generate_from_db(self.last_time) - def _block_version(self, block=None, version=None, soft=False, is_signed=True): + def _block_version( + self, block=None, version=None, block_type=BlockType.BLOCKED, is_signed=True + ): block = block or self.block version = version or version_factory( addon=self.addon, file_kw={'is_signed': is_signed} ) - return BlockVersion.objects.create(block=block, version=version, soft=soft) + return BlockVersion.objects.create( + block=block, version=version, block_type=block_type + ) def test_skip_update_unless_force_base(self): """ @@ -334,7 +338,7 @@ def test_dont_skip_update_if_all_blocked_or_not_blocked(self): version = self._block_version(is_signed=True) upload_mlbf_to_remote_settings(force_base=True) assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].called - version.update(soft=True) + version.update(block_type=BlockType.SOFT_BLOCKED) upload_mlbf_to_remote_settings(force_base=True) assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].called diff --git a/src/olympia/blocklist/tests/test_forms.py b/src/olympia/blocklist/tests/test_forms.py index 3caeeb63f425..4f3902848897 100644 --- a/src/olympia/blocklist/tests/test_forms.py +++ b/src/olympia/blocklist/tests/test_forms.py @@ -13,7 +13,7 @@ from ..admin import BlocklistSubmissionAdmin from ..forms import MultiAddForm, MultiDeleteForm -from ..models import Block, BlocklistSubmission, BlockVersion +from ..models import Block, BlocklistSubmission, BlockType, BlockVersion class TestBlocklistSubmissionForm(TestCase): @@ -65,7 +65,7 @@ def test_changed_version_ids_choices_add_action(self): f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' 'invalid@guid', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, } Form = self.get_form() form = Form(data=data) @@ -119,7 +119,7 @@ def test_changed_version_ids_choices_delete_action(self): f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' 'invalid@guid', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, } form = Form(data=data) assert form.fields['changed_version_ids'].choices == [ @@ -212,7 +212,7 @@ def test_changed_version_ids_widget(self): f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' 'invalid@guid', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [self.new_addon.current_version.id], } form = self.get_form()(data=data) @@ -300,7 +300,7 @@ def test_new_blocks_must_have_changed_versions(self): f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' 'invalid@guid', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, # only one version selected 'changed_version_ids': [self.partial_existing_addon_v_notblocked.id], } @@ -343,7 +343,7 @@ def test_duplicate_version_strings_must_all_be_changed(self): 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ self.new_addon.current_version.id, ], @@ -376,7 +376,7 @@ def test_clean(self): f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' 'invalid@guid', - 'block_type': BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, + 'block_type': BlockType.BLOCKED, 'url': 'new url', 'update_url_value': False, 'reason': 'new reason', diff --git a/src/olympia/blocklist/tests/test_mlbf.py b/src/olympia/blocklist/tests/test_mlbf.py index e9026f699766..efa966698e58 100644 --- a/src/olympia/blocklist/tests/test_mlbf.py +++ b/src/olympia/blocklist/tests/test_mlbf.py @@ -11,7 +11,7 @@ version_factory, ) from olympia.amo.utils import SafeStorage -from olympia.blocklist.models import BlockVersion +from olympia.blocklist.models import BlockType, BlockVersion from ..mlbf import ( MLBF, @@ -35,11 +35,11 @@ def _blocked_addon(self, **kwargs): def _version(self, addon, is_signed=True): return version_factory(addon=addon, file_kw={'is_signed': is_signed}) - def _block_version(self, block, version, soft=False): + def _block_version(self, block, version, block_type=BlockType.BLOCKED): return BlockVersion.objects.create( block=block, version=version, - soft=soft, + block_type=block_type, ) @@ -86,7 +86,7 @@ def test_invalid_key_access_raises(self): with self.assertRaises(AttributeError): loader['invalid'] with self.assertRaises(AttributeError): - loader[BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED] + loader[BlockType.BLOCKED] def test_valid_key_access_returns_expected_data(self): loader = self.TestStaticLoader(self.storage) @@ -135,7 +135,9 @@ def test_load_returns_expected_data(self): addon, block = self._blocked_addon() notblocked_version = addon.current_version - block_version = self._block_version(block, self._version(addon), soft=False) + block_version = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) mlbf_data = MLBFDataBaseLoader(self.storage) assert mlbf_data[MLBFDataType.HARD_BLOCKED] == MLBF.hash_filter_inputs( @@ -151,7 +153,9 @@ def test_blocked_items_caches_excluded_version_ids(self): to exclude in the notblocked_items property. """ addon, block = self._blocked_addon() - block_version = self._block_version(block, self._version(addon), soft=False) + block_version = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) with self.assertNumQueries(2): mlbf_data = MLBFDataBaseLoader(self.storage) assert mlbf_data._version_excludes == [block_version.version.id] @@ -226,7 +230,7 @@ def test_diff_returns_stateless_without_previous(self): def test_diff_no_changes(self): addon, block = self._blocked_addon() - self._block_version(block, self._version(addon), soft=False) + self._block_version(block, self._version(addon), block_type=BlockType.BLOCKED) base_mlbf = MLBF.generate_from_db('test') next_mlbf = MLBF.generate_from_db('test_two') @@ -236,7 +240,9 @@ def test_diff_block_added(self): addon, block = self._blocked_addon() base_mlbf = MLBF.generate_from_db('test') - new_block = self._block_version(block, self._version(addon), soft=False) + new_block = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) next_mlbf = MLBF.generate_from_db('test_two') @@ -252,7 +258,9 @@ def test_diff_block_added(self): def test_diff_block_removed(self): addon, block = self._blocked_addon() - block_version = self._block_version(block, self._version(addon), soft=False) + block_version = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) base_mlbf = MLBF.generate_from_db('test') block_version.delete() next_mlbf = MLBF.generate_from_db('test_two') @@ -269,10 +277,14 @@ def test_diff_block_removed(self): def test_diff_block_added_and_removed(self): addon, block = self._blocked_addon() - block_version = self._block_version(block, self._version(addon), soft=False) + block_version = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) base_mlbf = MLBF.generate_from_db('test') - new_block = self._block_version(block, self._version(addon), soft=False) + new_block = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) block_version.delete() next_mlbf = MLBF.generate_from_db('test_two') @@ -293,7 +305,9 @@ def test_diff_block_added_and_removed(self): def test_generate_stash_returns_expected_stash(self): addon, block = self._blocked_addon() - block_version = self._block_version(block, self._version(addon), soft=False) + block_version = self._block_version( + block, self._version(addon), block_type=BlockType.BLOCKED + ) mlbf = MLBF.generate_from_db('test') mlbf.generate_and_write_stash() @@ -318,11 +332,11 @@ def test_generate_stash_returns_expected_stash(self): def test_changed_count_returns_expected_count(self): addon, block = self._blocked_addon() - self._block_version(block, self._version(addon), soft=False) + self._block_version(block, self._version(addon), block_type=BlockType.BLOCKED) first_mlbf = MLBF.generate_from_db('first') # Include the new blocked version assert first_mlbf.blocks_changed_since_previous() == 1 - self._block_version(block, self._version(addon), soft=False) + self._block_version(block, self._version(addon), block_type=BlockType.BLOCKED) # The count should not change because the data is already calculated assert first_mlbf.blocks_changed_since_previous() == 1 next_mlbf = MLBF.generate_from_db('next') @@ -373,7 +387,9 @@ def test_duplicate_guid_is_blocked(self): reused_addon.update(guid=GUID_REUSE_FORMAT.format(addon.id)) reused_addon.addonguid.update(guid=addon.guid) - self._block_version(block, self._version(addon), soft=False) + self._block_version( + block, self._version(addon), block_type=BlockType.SOFT_BLOCKED + ) mlbf = MLBF.generate_from_db('test') diff --git a/src/olympia/blocklist/tests/test_models.py b/src/olympia/blocklist/tests/test_models.py index e1fdf03de95f..965c459d98bf 100644 --- a/src/olympia/blocklist/tests/test_models.py +++ b/src/olympia/blocklist/tests/test_models.py @@ -10,7 +10,7 @@ version_factory, ) -from ..models import BlocklistSubmission, BlockVersion +from ..models import BlocklistSubmission, BlockType, BlockVersion class TestBlockVersion(TestCase): @@ -23,7 +23,7 @@ def setUp(self): def test_str(self): hard_block_version = BlockVersion.objects.create( - block=self.block, version=self.version, soft=False + block=self.block, version=self.version, block_type=BlockType.BLOCKED ) assert str(hard_block_version) == ( f'Block.id={self.block.id} ' @@ -31,7 +31,7 @@ def test_str(self): ) soft_block_version = BlockVersion.objects.create( - block=self.block, version=self.version_2, soft=True + block=self.block, version=self.version_2, block_type=BlockType.SOFT_BLOCKED ) assert str(soft_block_version.reload()) == ( f'Block.id={self.block.id} ' diff --git a/src/olympia/blocklist/tests/test_serializers.py b/src/olympia/blocklist/tests/test_serializers.py index 1e5b364d0897..f4a4bbd42ddb 100644 --- a/src/olympia/blocklist/tests/test_serializers.py +++ b/src/olympia/blocklist/tests/test_serializers.py @@ -6,7 +6,7 @@ from olympia.amo.tests import TestCase, addon_factory, user_factory, version_factory from olympia.amo.urlresolvers import get_outgoing_url -from ..models import Block, BlockVersion +from ..models import Block, BlockType, BlockVersion from ..serializers import BlockSerializer @@ -60,7 +60,9 @@ def test_with_addon(self): version_6 = version_factory(addon=addon, version='123456') BlockVersion.objects.create(block=self.block, version=version_2) BlockVersion.objects.create(block=self.block, version=version_4) - BlockVersion.objects.create(block=self.block, version=version_6, soft=True) + BlockVersion.objects.create( + block=self.block, version=version_6, block_type=BlockType.SOFT_BLOCKED + ) expected = { 'id': self.block.id, @@ -103,7 +105,9 @@ def test_addon_all_blocked(self): version_3 = version_factory(addon=addon, version='3b1') BlockVersion.objects.create(block=self.block, version=version_1) BlockVersion.objects.create(block=self.block, version=version_2) - BlockVersion.objects.create(block=self.block, version=version_3, soft=True) + BlockVersion.objects.create( + block=self.block, version=version_3, block_type=BlockType.SOFT_BLOCKED + ) expected = { 'id': self.block.id, @@ -160,10 +164,14 @@ def test_is_all_versions(self): guid=self.block.guid, name='Addรณn nรกme', file_kw={'is_signed': True} ) BlockVersion.objects.create( - block=self.block, version=addon.current_version, soft=False + block=self.block, + version=addon.current_version, + block_type=BlockType.BLOCKED, ) BlockVersion.objects.create( - block=self.block, version=version_factory(addon=addon), soft=True + block=self.block, + version=version_factory(addon=addon), + block_type=BlockType.SOFT_BLOCKED, ) version_factory(addon=addon, file_kw={'is_signed': False}) # not signed self.block.refresh_from_db() diff --git a/src/olympia/blocklist/tests/test_tasks.py b/src/olympia/blocklist/tests/test_tasks.py index 822c9243be97..40e924621a8e 100644 --- a/src/olympia/blocklist/tests/test_tasks.py +++ b/src/olympia/blocklist/tests/test_tasks.py @@ -19,7 +19,7 @@ from olympia.blocklist.mlbf import MLBF from olympia.constants.blocklist import MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY -from ..models import BlocklistSubmission, BlockVersion +from ..models import BlocklistSubmission, BlockType, BlockVersion from ..tasks import ( BLOCKLIST_RECORD_MLBF_BASE, cleanup_old_files, @@ -118,12 +118,16 @@ def setUp(self): self.generation_time = datetime_to_ts(datetime.now()) - def _block_version(self, block=None, version=None, soft=False, is_signed=True): + def _block_version( + self, block=None, version=None, block_type=BlockType.BLOCKED, is_signed=True + ): block = block or self.block version = version or version_factory( addon=self.addon, file_kw={'is_signed': is_signed} ) - return BlockVersion.objects.create(block=block, version=version, soft=soft) + return BlockVersion.objects.create( + block=block, version=version, block_type=block_type + ) def test_upload_base_filter(self): self._block_version(is_signed=True) diff --git a/src/olympia/blocklist/tests/test_utils.py b/src/olympia/blocklist/tests/test_utils.py index 04e40a10a539..9f3dc7c47b84 100644 --- a/src/olympia/blocklist/tests/test_utils.py +++ b/src/olympia/blocklist/tests/test_utils.py @@ -10,7 +10,7 @@ from olympia.activity.models import ActivityLog from olympia.amo.tests import TestCase, addon_factory, block_factory, user_factory -from ..models import Block, BlocklistSubmission, BlockVersion +from ..models import Block, BlocklistSubmission, BlockType from ..utils import datetime_to_ts, save_versions_to_blocks @@ -110,7 +110,7 @@ def test_log_soft_block(self): url=None, updated_by=user_new, disable_addon=True, - block_type=BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED, + block_type=BlockType.SOFT_BLOCKED, changed_version_ids=[addon.current_version.pk], signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, ) @@ -173,33 +173,42 @@ def test_no_empty_new_blocks(self): def test_save_blocks_override_existing_block_type_soft_to_hard(self): user_new = user_factory() addon = addon_factory() - block_factory(guid=addon.guid, updated_by=self.task_user, soft=True) - assert addon.current_version.blockversion.soft + block_factory( + guid=addon.guid, + updated_by=self.task_user, + block_type=BlockType.SOFT_BLOCKED, + ) + assert addon.current_version.blockversion.block_type == BlockType.SOFT_BLOCKED submission = BlocklistSubmission.objects.create( input_guids=addon.guid, reason='some reason', url=None, updated_by=user_new, - block_type=BlockVersion.BLOCK_TYPE_CHOICES.BLOCKED, # Hard-block override. + block_type=BlockType.BLOCKED, # Hard-block override. changed_version_ids=[addon.current_version.pk], signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, ) save_versions_to_blocks([addon.guid], submission) - assert not addon.current_version.blockversion.reload().soft + assert ( + addon.current_version.blockversion.reload().block_type == BlockType.BLOCKED + ) def test_save_blocks_override_existing_block_type_hard_to_soft(self): user_new = user_factory() addon = addon_factory() block_factory(guid=addon.guid, updated_by=self.task_user) - assert not addon.current_version.blockversion.soft + assert addon.current_version.blockversion.block_type == BlockType.BLOCKED submission = BlocklistSubmission.objects.create( input_guids=addon.guid, reason='some reason', url=None, updated_by=user_new, - block_type=BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED, + block_type=BlockType.SOFT_BLOCKED, changed_version_ids=[addon.current_version.pk], signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, ) save_versions_to_blocks([addon.guid], submission) - assert addon.current_version.blockversion.reload().soft + assert ( + addon.current_version.blockversion.reload().block_type + == BlockType.SOFT_BLOCKED + ) diff --git a/src/olympia/blocklist/utils.py b/src/olympia/blocklist/utils.py index 896e5274f1e5..b24b08b683e5 100644 --- a/src/olympia/blocklist/utils.py +++ b/src/olympia/blocklist/utils.py @@ -15,7 +15,7 @@ def block_activity_log_save( change, submission_obj=None, ): - from .models import BlockVersion + from .models import BlockType action = amo.LOG.BLOCKLIST_BLOCK_EDITED if change else amo.LOG.BLOCKLIST_BLOCK_ADDED addon_versions = {ver.id: ver.version for ver in obj.addon_versions} @@ -46,7 +46,7 @@ def block_activity_log_save( if submission_obj.signoff_by: details['signoff_by'] = submission_obj.signoff_by.id details['soft'] = version_details['soft'] = ( - submission_obj.block_type == BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED + submission_obj.block_type == BlockType.SOFT_BLOCKED ) log_create(action, obj.addon, obj.guid, obj, details=details, user=obj.updated_by) @@ -202,7 +202,6 @@ def save_versions_to_blocks(guids, submission): # And now update the BlockVersion instances - instances to add first block_versions_to_create = [] block_versions_to_update = [] - is_soft = submission.block_type == BlockVersion.BLOCK_TYPE_CHOICES.SOFT_BLOCKED for version in block.addon_versions: if version.id in submission.changed_version_ids: if version.is_blocked: @@ -212,15 +211,18 @@ def save_versions_to_blocks(guids, submission): block_version = BlockVersion(block=block, version=version) block_versions_to_create.append(block_version) version.blockversion = block_version - block_version.soft = is_soft + block_version.block_type = submission.block_type if not block_versions_to_create and not change: # If we have no versions to block and it's a new Block don't do anything. # Note: we shouldn't have gotten this far with such a guid - it would have # been raised as a validation error in the form. continue block.save() - # Update existing BlockVersion in bulk - the only field to update is 'soft'. - BlockVersion.objects.bulk_update(block_versions_to_update, fields=['soft']) + # Update existing BlockVersion in bulk - the only field to update is + # 'block_type'. + BlockVersion.objects.bulk_update( + block_versions_to_update, fields=['block_type'] + ) # Add new BlockVersion in bulk. BlockVersion.objects.bulk_create(block_versions_to_create) diff --git a/src/olympia/constants/blocklist.py b/src/olympia/constants/blocklist.py index 17117b4de875..8cda2e5c9b11 100644 --- a/src/olympia/constants/blocklist.py +++ b/src/olympia/constants/blocklist.py @@ -1,8 +1,3 @@ -from django.utils.translation import gettext_lazy as _ - -from extended_choices import Choices - - # How many guids should there be in the stashes before we make a new base. BASE_REPLACE_THRESHOLD = 5_000 @@ -11,10 +6,3 @@ MLBF_BASE_ID_CONFIG_KEY = 'blocklist_mlbf_base_id' REMOTE_SETTINGS_COLLECTION_MLBF = 'addons-bloomfilters' - - -# As BlockVersion.BLOCK_TYPE_CHOICES except meant to be exposed to end-users. -BLOCK_TYPE_END_USERS_CHOICES = Choices( - ('BLOCKED', 0, _('Blocked')), - ('SOFT_BLOCKED', 1, _('Restricted')), -) diff --git a/src/olympia/devhub/tests/test_views_versions.py b/src/olympia/devhub/tests/test_views_versions.py index efefdb50c08c..6d87879042e1 100644 --- a/src/olympia/devhub/tests/test_views_versions.py +++ b/src/olympia/devhub/tests/test_views_versions.py @@ -27,6 +27,7 @@ version_factory, ) from olympia.applications.models import AppVersion +from olympia.blocklist.models import BlockType from olympia.constants.promoted import RECOMMENDED from olympia.files.models import File from olympia.reviewers.models import AutoApprovalSummary @@ -95,7 +96,7 @@ def test_blocked_version(self): == 'Blocked' ) - v4.blockversion.update(soft=True) + v4.blockversion.update(block_type=BlockType.SOFT_BLOCKED) doc = self.get_doc() assert ( doc('#version-list .file-status-text')[0].text_content().strip()