diff --git a/src/olympia/blocklist/admin.py b/src/olympia/blocklist/admin.py index 7935bafb2c..832d50fc6c 100644 --- a/src/olympia/blocklist/admin.py +++ b/src/olympia/blocklist/admin.py @@ -25,8 +25,8 @@ from .utils import splitlines class BlocklistSubmissionStateFilter(admin.SimpleListFilter): title = 'Signoff State' parameter_name = 'signoff_state' - default_value = BlocklistSubmission.SIGNOFF_PENDING - field_choices = BlocklistSubmission.SIGNOFF_STATES.items() + default_value = BlocklistSubmission.SIGNOFF_STATES.PENDING + field_choices = BlocklistSubmission.SIGNOFF_STATES.choices ALL = 'all' DELAYED = 'delayed' @@ -185,15 +185,15 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): return False def is_approvable(self, obj): - return obj and obj.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + return obj and obj.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING def is_changeable(self, obj): return obj and obj.signoff_state in ( - BlocklistSubmission.SIGNOFF_PENDING, - BlocklistSubmission.SIGNOFF_AUTOAPPROVED, + BlocklistSubmission.SIGNOFF_STATES.PENDING, + BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED, ) - def get_value(self, name, request, obj=None, default=None): + def get_value(self, name, request, *, obj=None, default=None): """Gets the named property from the obj if provided, or POST or GET.""" return ( getattr(obj, name, default) @@ -201,11 +201,6 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): else request.POST.get(name, request.GET.get(name, default)) ) - def is_add_change_submission(self, request, obj): - return str(self.get_value('action', request, obj, 0)) == str( - BlocklistSubmission.ACTION_ADDCHANGE - ) - def has_change_permission(self, request, obj=None, strict=False): """While a block submission is pending we want it to be partially editable (the url and reason). Once it's been rejected or approved it @@ -248,7 +243,10 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): def get_fieldsets(self, request, obj): is_new = obj is None show_canned = self.has_change_permission(request, obj, strict=True) - is_delete_submission = not self.is_add_change_submission(request, obj) + is_delete_submission = ( + int(self.get_value('action', request, obj=obj, default=0)) + == BlocklistSubmission.ACTIONS.DELETE + ) input_guids_section = ( 'Input Guids', @@ -319,6 +317,10 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): return tuple(section for section in sections if section) def get_readonly_fields(self, request, obj=None): + is_delete_submission = ( + int(self.get_value('action', request, obj=obj, default=0)) + == BlocklistSubmission.ACTIONS.DELETE + ) ro_fields = [ 'ro_changed_version_ids', 'updated_by', @@ -335,7 +337,7 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): ro_fields += admin.utils.flatten_fieldsets( self.get_fieldsets(request, obj) ) - if obj or not self.is_add_change_submission(request, obj): + if obj or is_delete_submission: ro_fields += ['block_type', 'delay_days'] return ro_fields @@ -352,7 +354,14 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): raise PermissionDenied MultiBlockForm = self.get_form(request, change=False, **kwargs) - is_delete = not self.is_add_change_submission(request, None) + action = int( + self.get_value( + 'action', + request, + obj=None, + default=BlocklistSubmission.ACTIONS.ADDCHANGE, + ) + ) guids_data = self.get_value('guids', request) if guids_data and 'input_guids' not in request.POST: # If we get a guids param it's a redirect from input_guids_view. @@ -397,7 +406,7 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): ) context = { # standard context django admin expects - 'title': 'Delete Blocks' if is_delete else 'Block Add-ons', + 'title': BlocklistSubmission(action=action).get_action_display(), 'subtitle': None, 'adminform': admin_form, 'object_id': None, @@ -422,7 +431,7 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): request, self.model._meta, object_id ) extra_context['can_change_object'] = ( - obj.action == BlocklistSubmission.ACTION_ADDCHANGE + obj.action == BlocklistSubmission.ACTIONS.ADDCHANGE and self.has_change_permission(request, obj, strict=True) ) extra_context['can_approve'] = self.is_approvable( @@ -452,12 +461,12 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): if is_approve and self.is_approvable(obj): if not self.has_signoff_approve_permission(request, obj): raise PermissionDenied - obj.signoff_state = BlocklistSubmission.SIGNOFF_APPROVED + obj.signoff_state = BlocklistSubmission.SIGNOFF_STATES.APPROVED obj.signoff_by = request.user elif is_reject: if not self.has_signoff_reject_permission(request, obj): raise PermissionDenied - obj.signoff_state = BlocklistSubmission.SIGNOFF_REJECTED + obj.signoff_state = BlocklistSubmission.SIGNOFF_STATES.REJECTED elif not self.has_change_permission(request, obj, strict=True): # users without full change permission should only do signoff raise PermissionDenied @@ -506,7 +515,7 @@ class BlocklistSubmissionAdmin(AMOModelAdmin): def ro_changed_version_ids(self, obj): # Annoyingly, we don't have the full context, but we stashed blocks # earlier in render_change_form(). - published = obj.signoff_state == obj.SIGNOFF_PUBLISHED + published = obj.signoff_state == obj.SIGNOFF_STATES.PUBLISHED total_adu = sum( (bl.current_adu if not published else bl.average_daily_users_snapshot) or 0 for bl in obj._blocks @@ -601,7 +610,7 @@ class BlockAdmin(BlockAdminAddMixin, AMOModelAdmin): def get_fieldsets(self, request, obj): details = ( - None, + 'Add-on', { 'fields': ( 'addon_guid', @@ -613,18 +622,18 @@ class BlockAdmin(BlockAdminAddMixin, AMOModelAdmin): }, ) history = ('Block History', {'fields': ('block_history',)}) - edit = ( - 'Edit Block', + versions = ('Block Versions', {'fields': ('blocked_versions',)}) + block = ( + 'Block Metadata', { 'fields': ( - 'blocked_versions', ('url', 'url_link'), 'reason', ), }, ) - return (details, history, edit) + return (details, history, versions, block) def has_change_permission(self, request, obj=None): return False @@ -641,6 +650,8 @@ class BlockAdmin(BlockAdminAddMixin, AMOModelAdmin): def changeform_view(self, request, obj_id=None, form_url='', extra_context=None): extra_context = extra_context or {} + extra_context['ACTION_SOFTEN'] = BlocklistSubmission.ACTIONS.SOFTEN + extra_context['ACTION_HARDEN'] = BlocklistSubmission.ACTIONS.HARDEN if obj_id: obj = ( self.get_object(request, obj_id) @@ -675,5 +686,5 @@ class BlockAdmin(BlockAdminAddMixin, AMOModelAdmin): raise PermissionDenied return redirect( reverse('admin:blocklist_blocklistsubmission_add') - + f'?guids={obj.guid}&action={BlocklistSubmission.ACTION_DELETE}' + + f'?guids={obj.guid}&action={BlocklistSubmission.ACTIONS.DELETE}' ) diff --git a/src/olympia/blocklist/cron.py b/src/olympia/blocklist/cron.py index 29399e83c6..58cd878d08 100644 --- a/src/olympia/blocklist/cron.py +++ b/src/olympia/blocklist/cron.py @@ -138,7 +138,9 @@ def _upload_mlbf_to_remote_settings(*, force_base=False): def process_blocklistsubmissions(): qs = BlocklistSubmission.objects.filter( - signoff_state__in=BlocklistSubmission.SIGNOFF_STATES_APPROVED, + signoff_state__in=( + BlocklistSubmission.SIGNOFF_STATES.STATES_APPROVED.values.keys() + ), delayed_until__lte=datetime.now(), ) for sub in qs: diff --git a/src/olympia/blocklist/forms.py b/src/olympia/blocklist/forms.py index 4bd1e1fd18..f6b1b1bbde 100644 --- a/src/olympia/blocklist/forms.py +++ b/src/olympia/blocklist/forms.py @@ -9,7 +9,7 @@ from olympia.amo.admin import HTML5DateTimeInput from olympia.amo.forms import AMOModelForm from olympia.reviewers.models import ReviewActionReason -from .models import Block, BlocklistSubmission +from .models import Block, BlocklistSubmission, BlockType from .utils import splitlines @@ -77,6 +77,15 @@ class BlocksWidget(forms.widgets.SelectMultiple): def optgroups(self, name, value, attrs=None): return [] + def get_verb(self, action): + """Return the verb to use when displaying a given version, depending + on the action.""" + try: + verb = BlocklistSubmission.ACTIONS.for_value(action).short + except KeyError: + verb = '?' + return verb + def get_context(self, name, value, attrs): context = super().get_context(name, value, attrs) return { @@ -88,7 +97,7 @@ class BlocksWidget(forms.widgets.SelectMultiple): }, 'blocks': self.blocks, 'total_adu': sum(block.current_adu for block in self.blocks), - 'is_add_change': self.is_add_change, + 'verb': self.get_verb(self.action), } @@ -122,17 +131,39 @@ class BlocklistSubmissionForm(AMOModelForm): def __init__(self, data=None, *args, **kw): super().__init__(data, *args, **kw) - self.is_add_change = str(BlocklistSubmission.ACTION_ADDCHANGE) == str( - self.get_value('action', BlocklistSubmission.ACTION_ADDCHANGE) + self.action = int( + self.get_value('action', BlocklistSubmission.ACTIONS.ADDCHANGE) ) + self.is_add_change = self.action == BlocklistSubmission.ACTIONS.ADDCHANGE input_guids = self.get_value('input_guids', '') load_full_objects = len(splitlines(input_guids)) <= GUID_FULL_LOAD_LIMIT - is_published = self.instance.signoff_state == self.instance.SIGNOFF_PUBLISHED + is_published = ( + self.instance.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + ) if not self.instance.id: self.fields['input_guids'].widget = HiddenInput() self.fields['action'].widget = HiddenInput() self.fields['delayed_until'].widget = HiddenInput() + if self.action in ( + BlocklistSubmission.ACTIONS.HARDEN, + BlocklistSubmission.ACTIONS.SOFTEN, + ): + # When softening/hardening, the widget needs to be present for + # us to record the block type on the blocklistsubmission, but + # we're hiding it and forcing the value depending on the action + # to make the UI less confusing. + block_type = ( + BlockType.BLOCKED + if self.action == BlocklistSubmission.ACTIONS.HARDEN + else BlockType.SOFT_BLOCKED + ) + self.fields['block_type'].widget = HiddenInput() + self.fields['block_type'].choices = ( + (block_type, dict(BlockType.choices)[block_type]), + ) + self.fields['block_type'].initial = block_type + self.fields['block_type'].label = '' objects = BlocklistSubmission.process_input_guids( input_guids, @@ -162,6 +193,23 @@ class BlocklistSubmissionForm(AMOModelForm): getattr(self.instance, field_name) is not None ) + def should_version_be_available_for_action(self, version): + """Return whether or not the given version should be available as a + choice for the action we're currently doing.""" + conditions = { + BlocklistSubmission.ACTIONS.ADDCHANGE: not version.is_blocked, + BlocklistSubmission.ACTIONS.DELETE: version.is_blocked, + BlocklistSubmission.ACTIONS.HARDEN: ( + version.is_blocked + and version.blockversion.block_type == BlockType.SOFT_BLOCKED + ), + BlocklistSubmission.ACTIONS.SOFTEN: ( + version.is_blocked + and version.blockversion.block_type == BlockType.BLOCKED + ), + } + return conditions.get(self.action) + def setup_changed_version_ids_field(self, field, data): if not self.instance.id: field.choices = [ @@ -170,10 +218,7 @@ class BlocklistSubmissionForm(AMOModelForm): [ (version.id, version.version) for version in block.addon_versions - # ^ is XOR - # - for add action it allows the version when it is NOT blocked - # - for delete action it allows the version when it IS blocked - if (version.is_blocked ^ self.is_add_change) + if self.should_version_be_available_for_action(version) and not version.blocklist_submission_id ], ) @@ -193,7 +238,7 @@ class BlocklistSubmissionForm(AMOModelForm): self.changed_version_ids_choices = self.instance.changed_version_ids field.widget.choices = self.changed_version_ids_choices field.widget.blocks = self.blocks - field.widget.is_add_change = self.is_add_change + field.widget.action = self.action def get_value(self, field_name, default): return ( @@ -207,20 +252,20 @@ class BlocklistSubmissionForm(AMOModelForm): errors = [] # First, check we're not creating empty blocks - # we're checking new blocks for add/change; and all blocks for delete + # we're checking new blocks for add/change; and all blocks for other actions for block in (bl for bl in self.blocks if not bl.id or not self.is_add_change): version_ids = [v.id for v in block.addon_versions] changed_ids = (v_id for v_id in data if v_id in version_ids) blocked_ids = (v.id for v in block.addon_versions if v.is_blocked) # for add/change we raise if there are no changed ids for this addon - # for delete, only if there is also at least one existing blocked version + # for other actions, only if there is also at least one existing blocked + # version if (self.is_add_change or any(blocked_ids)) and not any(changed_ids): errors.append(ValidationError(f'{block.guid} has no changed versions')) # Second, check for duplicate version strings in reused guids. error_string = ( - '{}:{} exists more than once. All {} versions must be ' - f'{"blocked" if self.is_add_change else "unblocked"} together' + '{}:{} exists more than once. All {} versions must be selected together.' ) for block in self.blocks: version_strs = defaultdict(set) # collect version strings together diff --git a/src/olympia/blocklist/migrations/0038_tweak_blocksubmission_choices.py b/src/olympia/blocklist/migrations/0038_tweak_blocksubmission_choices.py new file mode 100644 index 0000000000..d4d8f183f5 --- /dev/null +++ b/src/olympia/blocklist/migrations/0038_tweak_blocksubmission_choices.py @@ -0,0 +1,29 @@ +# Generated by Django 4.2.16 on 2024-11-12 11:24 + +from django.db import migrations, models +import olympia.amo.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('blocklist', '0037_alter_blockversion_block_type_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='blocklistsubmission', + name='action', + field=models.SmallIntegerField(choices=[(0, 'Add/Change Block'), (1, 'Delete Block'), (2, 'Harden Block'), (3, 'Soften Block')], default=0), + ), + migrations.AlterField( + model_name='blocklistsubmission', + name='block_type', + field=olympia.amo.fields.PositiveTinyIntegerField(choices=[(0, '🛑 Hard-Blocked'), (1, '⚠️ Soft-Blocked')], default=0), + ), + migrations.AlterField( + model_name='blockversion', + name='block_type', + field=olympia.amo.fields.PositiveTinyIntegerField(choices=[(0, '🛑 Hard-Blocked'), (1, '⚠️ Soft-Blocked')], default=0, null=True), + ), + ] diff --git a/src/olympia/blocklist/models.py b/src/olympia/blocklist/models.py index bcf7d45825..9aee9c6770 100644 --- a/src/olympia/blocklist/models.py +++ b/src/olympia/blocklist/models.py @@ -8,6 +8,7 @@ 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 @@ -97,6 +98,12 @@ class Block(ModelBase): for block in blocks: block.addon_versions = all_addon_versions[block.guid] + def has_soft_blocked_versions(self): + return self.blockversion_set.filter(block_type=BlockType.SOFT_BLOCKED).exists() + + def has_hard_blocked_versions(self): + return self.blockversion_set.filter(block_type=BlockType.BLOCKED).exists() + def review_listed_link(self): has_listed = any( True @@ -195,32 +202,25 @@ class BlocklistSubmissionManager(ManagerBase): class BlocklistSubmission(ModelBase): - SIGNOFF_PENDING = 0 - SIGNOFF_APPROVED = 1 - SIGNOFF_REJECTED = 2 - SIGNOFF_AUTOAPPROVED = 3 - SIGNOFF_PUBLISHED = 4 - SIGNOFF_STATES = { - SIGNOFF_PENDING: 'Pending Sign-off', - SIGNOFF_APPROVED: 'Approved', - SIGNOFF_REJECTED: 'Rejected', - SIGNOFF_AUTOAPPROVED: 'Auto Sign-off', - SIGNOFF_PUBLISHED: 'Published', - } - SIGNOFF_STATES_APPROVED = ( - SIGNOFF_APPROVED, - SIGNOFF_AUTOAPPROVED, + SIGNOFF_STATES = Choices( + ('PENDING', 0, 'Pending Sign-off'), + ('APPROVED', 1, 'Approved'), + ('REJECTED', 2, 'Rejected'), + ('AUTOAPPROVED', 3, 'Auto Sign-off'), + ('PUBLISHED', 4, 'Published'), ) - SIGNOFF_STATES_FINISHED = ( - SIGNOFF_REJECTED, - SIGNOFF_PUBLISHED, + SIGNOFF_STATES.add_subset('STATES_APPROVED', ('APPROVED', 'AUTOAPPROVED')) + SIGNOFF_STATES.add_subset('STATES_FINISHED', ('REJECTED', 'PUBLISHED')) + ACTIONS = Choices( + # 'short' extra property is added to describe the short verb we use for + # each action when displayed next to a version. + ('ADDCHANGE', 0, 'Add/Change Block', {'short': 'Block'}), + ('DELETE', 1, 'Delete Block', {'short': 'Unblock'}), + ('HARDEN', 2, 'Harden Block', {'short': 'Harden'}), + ('SOFTEN', 3, 'Soften Block', {'short': 'Soften'}), ) - ACTION_ADDCHANGE = 0 - ACTION_DELETE = 1 - ACTIONS = { - ACTION_ADDCHANGE: 'Add/Change', - ACTION_DELETE: 'Delete', - } + ACTIONS.add_subset('SAVE_TO_BLOCK_OBJECTS', ('ADDCHANGE', 'HARDEN', 'SOFTEN')) + ACTIONS.add_subset('DELETE_TO_BLOCK_OBJECTS', ('DELETE',)) FakeBlockAddonVersion = namedtuple( 'FakeBlockAddonVersion', ( @@ -240,7 +240,9 @@ class BlocklistSubmission(ModelBase): ), ) - action = models.SmallIntegerField(choices=ACTIONS.items(), default=ACTION_ADDCHANGE) + action = models.SmallIntegerField( + choices=ACTIONS.choices, default=ACTIONS.ADDCHANGE + ) input_guids = models.TextField() changed_version_ids = models.JSONField(default=list) to_block = models.JSONField(default=list) @@ -261,7 +263,7 @@ class BlocklistSubmission(ModelBase): UserProfile, null=True, on_delete=models.SET_NULL, related_name='+' ) signoff_state = models.SmallIntegerField( - choices=SIGNOFF_STATES.items(), default=SIGNOFF_PENDING + choices=SIGNOFF_STATES.choices, default=SIGNOFF_STATES.PENDING ) delayed_until = models.DateTimeField( null=True, @@ -323,19 +325,19 @@ class BlocklistSubmission(ModelBase): return bool(self.changed_version_ids) def update_signoff_for_auto_approval(self): - is_pending = self.signoff_state == self.SIGNOFF_PENDING - add_action = self.action == self.ACTION_ADDCHANGE + is_pending = self.signoff_state == self.SIGNOFF_STATES.PENDING + add_action = self.action == self.ACTIONS.ADDCHANGE if is_pending and ( self.all_adu_safe() or add_action and not self.has_version_changes() ): - self.update(signoff_state=self.SIGNOFF_AUTOAPPROVED) + self.update(signoff_state=self.SIGNOFF_STATES.AUTOAPPROVED) @property def is_submission_ready(self): """Has this submission been signed off, or sign-off isn't required.""" - is_auto_approved = self.signoff_state == self.SIGNOFF_AUTOAPPROVED + is_auto_approved = self.signoff_state == self.SIGNOFF_STATES.AUTOAPPROVED is_signed_off = ( - self.signoff_state == self.SIGNOFF_APPROVED + self.signoff_state == self.SIGNOFF_STATES.APPROVED and self.can_user_signoff(self.signoff_by) ) return not self.is_delayed and (is_auto_approved or is_signed_off) @@ -355,7 +357,7 @@ class BlocklistSubmission(ModelBase): processed = self.process_input_guids( self.input_guids, load_full_objects=False, - filter_existing=(self.action == self.ACTION_ADDCHANGE), + filter_existing=(self.action == self.ACTIONS.ADDCHANGE), ) return [serialize_block(block) for block in processed.get('blocks', [])] @@ -389,7 +391,7 @@ class BlocklistSubmission(ModelBase): named=True, ) ) - all_submission_versions = BlocklistSubmission.get_all_submission_versions() + all_submission_versions = cls.get_all_submission_versions() all_addon_versions = defaultdict(list) for version in version_qs: @@ -485,7 +487,7 @@ class BlocklistSubmission(ModelBase): def save_to_block_objects(self): assert self.is_submission_ready - assert self.action == self.ACTION_ADDCHANGE + assert self.action in self.ACTIONS.SAVE_TO_BLOCK_OBJECTS all_guids_to_block = [block['guid'] for block in self.to_block] @@ -493,11 +495,11 @@ class BlocklistSubmission(ModelBase): save_versions_to_blocks(guids_chunk, self) self.save() - self.update(signoff_state=self.SIGNOFF_PUBLISHED) + self.update(signoff_state=self.SIGNOFF_STATES.PUBLISHED) def delete_block_objects(self): assert self.is_submission_ready - assert self.action == self.ACTION_DELETE + assert self.action in self.ACTIONS.DELETE_TO_BLOCK_OBJECTS all_guids_to_block = [block['guid'] for block in self.to_block] @@ -506,24 +508,24 @@ class BlocklistSubmission(ModelBase): delete_versions_from_blocks(guids_chunk, self) self.save() - self.update(signoff_state=self.SIGNOFF_PUBLISHED) + self.update(signoff_state=self.SIGNOFF_STATES.PUBLISHED) @classmethod - def get_submissions_from_guid(cls, guid, excludes=SIGNOFF_STATES_FINISHED): - return cls.objects.exclude(signoff_state__in=excludes).filter( - to_block__contains={'guid': guid} - ) + def get_submissions_from_guid(cls, guid): + return cls.objects.exclude( + signoff_state__in=cls.SIGNOFF_STATES.STATES_FINISHED.values.keys() + ).filter(to_block__contains={'guid': guid}) @classmethod def get_submissions_from_version_id(cls, version_id): return cls.objects.exclude( - signoff_state__in=cls.SIGNOFF_STATES_FINISHED + signoff_state__in=cls.SIGNOFF_STATES.STATES_FINISHED.values.keys() ).filter(changed_version_ids__contains=version_id) @classmethod def get_all_submission_versions(cls): - submission_qs = BlocklistSubmission.objects.exclude( - signoff_state__in=BlocklistSubmission.SIGNOFF_STATES_FINISHED + submission_qs = cls.objects.exclude( + signoff_state__in=cls.SIGNOFF_STATES.STATES_FINISHED.values.keys() ).values_list('id', 'changed_version_ids') return { ver_id: sub_id for sub_id, id_list in submission_qs for ver_id in id_list diff --git a/src/olympia/blocklist/tasks.py b/src/olympia/blocklist/tasks.py index 713f3f2332..dbb0f85622 100644 --- a/src/olympia/blocklist/tasks.py +++ b/src/olympia/blocklist/tasks.py @@ -44,15 +44,15 @@ def process_blocklistsubmission(multi_block_submit_id, **kw): obj = BlocklistSubmission.objects.get(pk=multi_block_submit_id) try: with transaction.atomic(): - if obj.action == BlocklistSubmission.ACTION_ADDCHANGE: - # create the blocks from the guids in the multi_block + if obj.action in BlocklistSubmission.ACTIONS.SAVE_TO_BLOCK_OBJECTS: + # create/update the blocks from the guids in the multi_block obj.save_to_block_objects() - elif obj.action == BlocklistSubmission.ACTION_DELETE: - # delete the blocks + elif obj.action in BlocklistSubmission.ACTIONS.DELETE_TO_BLOCK_OBJECTS: + # delete/update the blocks obj.delete_block_objects() except Exception as exc: # If something failed reset the submission back to Pending. - obj.update(signoff_state=BlocklistSubmission.SIGNOFF_PENDING) + obj.update(signoff_state=BlocklistSubmission.SIGNOFF_STATES.PENDING) message = f'Exception in task: {exc}' LogEntry.objects.log_action( user_id=settings.TASK_USER_ID, diff --git a/src/olympia/blocklist/templates/admin/blocklist/block/submit_line.html b/src/olympia/blocklist/templates/admin/blocklist/block/submit_line.html new file mode 100644 index 0000000000..85a80c1ba8 --- /dev/null +++ b/src/olympia/blocklist/templates/admin/blocklist/block/submit_line.html @@ -0,0 +1,8 @@ +{% extends "admin/submit_line.html" %} +{% block submit-row %} + {% if original.pk %} + Harden + Soften + {% endif %} + {{ block.super }} +{% endblock %} diff --git a/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html b/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html index 95c4843476..ea0001a6d8 100644 --- a/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html +++ b/src/olympia/blocklist/templates/admin/blocklist/widgets/blocks.html @@ -29,7 +29,7 @@ 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_block_type_display }}){% endif %} + > {{ verb }} {{ version.version }} {% if version.blockversion %}({{ version.blockversion.get_block_type_display }}){% endif %} {% else %} {{ version.version }} ({% if version.is_blocked %}{{ version.blockversion.get_block_type_display }}{% else %}🟢 Not Blocked{% endif %}) diff --git a/src/olympia/blocklist/tests/test_admin.py b/src/olympia/blocklist/tests/test_admin.py index 60e14b3d28..7f52d1a218 100644 --- a/src/olympia/blocklist/tests/test_admin.py +++ b/src/olympia/blocklist/tests/test_admin.py @@ -204,6 +204,85 @@ class TestBlockAdmin(TestCase): 'Blocked versions:\n2.0 (🛑 Hard-Blocked), 3.0 (⚠️ Soft-Blocked)' ) + def test_soften_harden(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + addon = addon_factory(version_kw={'version': '1.0'}) + second_version = version_factory(addon=addon, version='2.0') + third_version = version_factory(addon=addon, version='3.0') + block = block_factory( + addon=addon, + version_ids=[second_version.id, third_version.id], + updated_by=user, + ) + # Make one of the blocks soft. + 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,)), + ) + assert response.status_code == 200 + doc = pq(response.content.decode('utf-8')) + assert doc('.softenlink').attr('href') == ( + reverse('admin:blocklist_blocklistsubmission_add') + + f'?guids={addon.guid}&action={BlocklistSubmission.ACTIONS.SOFTEN}' + ) + assert doc('.hardenlink').attr('href') == ( + reverse('admin:blocklist_blocklistsubmission_add') + + f'?guids={addon.guid}&action={BlocklistSubmission.ACTIONS.HARDEN}' + ) + assert 'disabled' not in doc('.hardenlink').attr('class') + assert 'disabled' not in doc('.softenlink').attr('class') + + def test_harden_disabled_only_hard_blocked_versions_already(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + addon = addon_factory(version_kw={'version': '1.0'}) + second_version = version_factory(addon=addon, version='2.0') + third_version = version_factory(addon=addon, version='3.0') + block = block_factory( + addon=addon, + version_ids=[second_version.id, third_version.id], + updated_by=user, + ) + + response = self.client.get( + reverse('admin:blocklist_block_change', args=(block.id,)), + ) + assert response.status_code == 200 + doc = pq(response.content.decode('utf-8')) + assert 'disabled' in doc('.hardenlink').attr('class') + assert 'disabled' not in doc('.softenlink').attr('class') + + def test_soften_disabled_only_soft_blocked_versions_already(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + addon = addon_factory(version_kw={'version': '1.0'}) + second_version = version_factory(addon=addon, version='2.0') + third_version = version_factory(addon=addon, version='3.0') + block = block_factory( + addon=addon, + version_ids=[second_version.id, third_version.id], + updated_by=user, + block_type=BlockType.SOFT_BLOCKED, + ) + + response = self.client.get( + reverse('admin:blocklist_block_change', args=(block.id,)), + ) + assert response.status_code == 200 + doc = pq(response.content.decode('utf-8')) + assert 'disabled' not in doc('.hardenlink').attr('class') + assert 'disabled' in doc('.softenlink').attr('class') + def check_checkbox(checkbox, version): assert checkbox.attrib['value'] == str(version.id) @@ -324,8 +403,8 @@ class TestBlocklistSubmissionAdmin(TestCase): check_checkbox(checkboxes[1], ver_deleted) check_checkbox(checkboxes[2], ver_other) - # not a checkbox because in a submission, green circle because not - # blocked yet technically. + # not a checkbox because already part of a submission, green circle + # because not blocked yet technically. assert doc(f'li[data-version-id="{ver_add_subm.id}"]').text() == ( f'{ver_add_subm.version} (🟢 Not Blocked) [Edit Submission]' ) @@ -362,6 +441,110 @@ class TestBlocklistSubmissionAdmin(TestCase): check_checkbox(checkboxes[0], ver_deleted) check_checkbox(checkboxes[1], ver_other) + def test_version_checkboxes_hardening_action(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + addon = addon_factory(guid='guid@', average_daily_users=100) + ver_deleted = version_factory(addon=addon, channel=amo.CHANNEL_UNLISTED) + ver_deleted.delete() # shouldn't affect it's status + # these next three versions shouldn't be possible choices + ver_add_subm = version_factory(addon=addon) + add_submission = BlocklistSubmission.objects.create( + input_guids=addon.guid, changed_version_ids=[ver_add_subm.id] + ) + ver_other = addon_factory(average_daily_users=99).current_version + ver_block = version_factory(addon=ver_other.addon) + ver_soft_block = version_factory(addon=ver_other.addon) + block_factory( + addon=addon, version_ids=[ver_block.id, ver_soft_block.id], updated_by=user + ) + ver_soft_block.blockversion.update(block_type=BlockType.SOFT_BLOCKED) + + response = self.client.get( + self.submission_url, + { + 'guids': f'{addon.guid}\n {ver_block.addon.guid}\n', + 'action': BlocklistSubmission.ACTIONS.HARDEN, + }, + ) + doc = pq(response.content.decode('utf-8')) + checkboxes = doc('input[name=changed_version_ids]') + + assert len(checkboxes) == 1 + check_checkbox(checkboxes[0], ver_soft_block) + + # not a checkbox because already part of a submission, green circle + # because not blocked yet technically. + assert doc(f'li[data-version-id="{ver_add_subm.id}"]').text() == ( + f'{ver_add_subm.version} (🟢 Not Blocked) [Edit Submission]' + ) + submission_link = doc(f'li[data-version-id="{ver_add_subm.id}"] a') + assert submission_link.text() == 'Edit Submission' + assert submission_link.attr['href'] == reverse( + 'admin:blocklist_blocklistsubmission_change', + args=(add_submission.id,), + ) + + # not a checkbox because hard-blocked already and this is an harden + # action + assert doc(f'li[data-version-id="{ver_block.id}"]').text() == ( + f'{ver_block.version} (🛑 Hard-Blocked)' + ) + + def test_version_checkboxes_softening_action(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + addon = addon_factory(guid='guid@', average_daily_users=100) + ver_deleted = version_factory(addon=addon, channel=amo.CHANNEL_UNLISTED) + ver_deleted.delete() # shouldn't affect it's status + # these next three versions shouldn't be possible choices + ver_add_subm = version_factory(addon=addon) + add_submission = BlocklistSubmission.objects.create( + input_guids=addon.guid, changed_version_ids=[ver_add_subm.id] + ) + ver_other = addon_factory(average_daily_users=99).current_version + ver_block = version_factory(addon=ver_other.addon) + ver_soft_block = version_factory(addon=ver_other.addon) + block_factory( + addon=addon, version_ids=[ver_block.id, ver_soft_block.id], updated_by=user + ) + ver_soft_block.blockversion.update(block_type=BlockType.SOFT_BLOCKED) + + response = self.client.get( + self.submission_url, + { + 'guids': f'{addon.guid}\n {ver_block.addon.guid}\n', + 'action': BlocklistSubmission.ACTIONS.SOFTEN, + }, + ) + doc = pq(response.content.decode('utf-8')) + checkboxes = doc('input[name=changed_version_ids]') + + assert len(checkboxes) == 1 + check_checkbox(checkboxes[0], ver_block) + + # not a checkbox because already part of a submission, green circle + # because not blocked yet technically. + assert doc(f'li[data-version-id="{ver_add_subm.id}"]').text() == ( + f'{ver_add_subm.version} (🟢 Not Blocked) [Edit Submission]' + ) + submission_link = doc(f'li[data-version-id="{ver_add_subm.id}"] a') + assert submission_link.text() == 'Edit Submission' + assert submission_link.attr['href'] == reverse( + 'admin:blocklist_blocklistsubmission_change', + args=(add_submission.id,), + ) + + # not a checkbox because soft-blocked already and this is an soften + # action + assert doc(f'li[data-version-id="{ver_soft_block.id}"]').text() == ( + f'{ver_soft_block.version} (⚠️ Soft-Blocked)' + ) + def test_add_single(self): user = user_factory(email='someone@mozilla.com') self.grant_permission(user, 'Blocklist:Create') @@ -421,7 +604,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': 'guid@', - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': changed_version_ids, 'url': 'dfd', @@ -600,7 +783,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, @@ -794,6 +977,95 @@ class TestBlocklistSubmissionAdmin(TestCase): assert partial_addon.status == amo.STATUS_DISABLED assert partial_addon_version.file.status == (amo.STATUS_DISABLED) + def test_soften(self): + addon = addon_factory(guid='guid@') + version = addon.current_version + block = block_factory( + guid=addon.guid, + updated_by=user_factory(), + ) + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + response = self.client.get( + self.submission_url, + { + 'guids': str(addon.guid), + 'action': str(BlocklistSubmission.ACTIONS.SOFTEN), + }, + ) + doc = pq(response.content) + assert doc('#id_block_type').attr('value') == str(BlockType.SOFT_BLOCKED) + + response = self.client.post( + self.submission_url, + { + 'input_guids': str(addon.guid), + 'action': str(BlocklistSubmission.ACTIONS.SOFTEN), + 'block_type': str(BlockType.SOFT_BLOCKED), + 'changed_version_ids': [ + version.id, + ], + 'disable_addon': False, + 'url': 'dfd', + 'reason': 'some reason', + 'update_url_value': True, + 'update_reason_value': True, + 'delay_days': 0, + '_save': 'Save', + }, + follow=True, + ) + assert response.status_code == 200 + assert version.blockversion.reload().block_type == BlockType.SOFT_BLOCKED + assert block.reload().updated_by == user + + def test_harden(self): + addon = addon_factory(guid='guid@') + version = addon.current_version + block = block_factory( + guid=addon.guid, + updated_by=user_factory(), + block_type=BlockType.SOFT_BLOCKED, + ) + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Blocklist:Create') + self.client.force_login(user) + + response = self.client.get( + self.submission_url, + { + 'guids': str(addon.guid), + 'action': str(BlocklistSubmission.ACTIONS.HARDEN), + }, + ) + doc = pq(response.content) + assert doc('#id_block_type').attr('value') == str(BlockType.BLOCKED) + + response = self.client.post( + self.submission_url, + { + 'input_guids': str(addon.guid), + 'action': str(BlocklistSubmission.ACTIONS.HARDEN), + 'block_type': str(BlockType.BLOCKED), + 'changed_version_ids': [ + version.id, + ], + 'disable_addon': False, + 'url': 'dfd', + 'reason': 'some reason', + 'update_url_value': True, + 'update_reason_value': True, + 'delay_days': 0, + '_save': 'Save', + }, + follow=True, + ) + assert response.status_code == 200 + assert version.blockversion.reload().block_type == BlockType.BLOCKED + assert block.reload().updated_by == user + def test_submit_no_dual_signoff(self): addon_adu = settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD ( @@ -825,7 +1097,7 @@ class TestBlocklistSubmissionAdmin(TestCase): multi = BlocklistSubmission.objects.get() assert multi.block_type == BlockType.BLOCKED multi.update( - signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, signoff_by=user_factory(), ) assert multi.is_submission_ready @@ -872,7 +1144,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, @@ -1039,7 +1311,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': 'guid@\nfoo@baa\ninvalid@', - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'url': 'dfd', 'reason': 'some reason', @@ -1067,12 +1339,12 @@ class TestBlocklistSubmissionAdmin(TestCase): add_change_subm = BlocklistSubmission.objects.create( input_guids='guid@\ninvalid@\nblock@', updated_by=user_factory(display_name='Bób'), - action=BlocklistSubmission.ACTION_ADDCHANGE, + action=BlocklistSubmission.ACTIONS.ADDCHANGE, ) delete_subm = BlocklistSubmission.objects.create( input_guids='block@', updated_by=user_factory(display_name='Sué'), - action=BlocklistSubmission.ACTION_DELETE, + action=BlocklistSubmission.ACTIONS.DELETE, ) add_change_subm.save() delete_subm.save() @@ -1106,7 +1378,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert 'Sué' in response.content.decode('utf-8') doc = pq(response.content) assert doc('th.field-blocks_count').text() == '1 add-ons 2 add-ons' - assert doc('.field-action').text() == ('Delete Add/Change') + assert doc('.field-action').text() == ('Delete Block Add/Change Block') assert doc('.field-state').text() == 'Pending Sign-off Pending Sign-off' def test_can_list_with_blocklist_create(self): @@ -1190,7 +1462,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert mbs.disable_addon is False # The blocklistsubmission wasn't approved or rejected though - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING assert Block.objects.count() == 0 log_entry = LogEntry.objects.get() @@ -1259,7 +1531,7 @@ class TestBlocklistSubmissionAdmin(TestCase): multi_url, { 'input_guids': 'guid2@\nfoo@baa', - 'action': str(BlocklistSubmission.ACTION_DELETE), + 'action': str(BlocklistSubmission.ACTIONS.DELETE), 'changed_version_ids': [], 'url': 'new.url', 'reason': 'a reason', @@ -1280,7 +1552,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert mbs.reason != 'a reason' # The blocklistsubmission wasn't approved or rejected either - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING assert Block.objects.count() == 0 assert LogEntry.objects.count() == 0 @@ -1455,7 +1727,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert mbs.reason != 'a reason' # And the blocklistsubmission was rejected, so no Blocks created - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_REJECTED + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.REJECTED assert Block.objects.count() == 0 assert not mbs.is_submission_ready @@ -1524,7 +1796,7 @@ class TestBlocklistSubmissionAdmin(TestCase): mbs = mbs.reload() # It wasn't signed off assert not mbs.signoff_by - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING # And the details weren't updated either assert mbs.url != 'new.url' assert mbs.reason != 'a reason' @@ -1567,7 +1839,7 @@ class TestBlocklistSubmissionAdmin(TestCase): submission = submission.reload() # It wasn't signed off assert not submission.signoff_by - assert submission.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING # And the details weren't updated either assert submission.url != 'new.url' assert submission.reason != 'a reason' @@ -1598,7 +1870,7 @@ class TestBlocklistSubmissionAdmin(TestCase): ) assert response.status_code == 200 submission = submission.reload() - assert submission.signoff_state == BlocklistSubmission.SIGNOFF_REJECTED + assert submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.REJECTED assert not submission.signoff_by assert submission.url == 'new.url' assert submission.reason == 'a reason' @@ -1609,7 +1881,7 @@ class TestBlocklistSubmissionAdmin(TestCase): input_guids='guid@\ninvalid@\nsecond@invalid', updated_by=user_factory(), signoff_by=user_factory(), - signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, changed_version_ids=[addon.current_version.id], ) assert mbs.to_block == [ @@ -1621,7 +1893,7 @@ class TestBlocklistSubmissionAdmin(TestCase): ] mbs.save_to_block_objects() block = Block.objects.get() - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PUBLISHED + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED # update addon adu to something different assert block.average_daily_users_snapshot == addon.average_daily_users addon.update(average_daily_users=1234) @@ -1672,16 +1944,16 @@ class TestBlocklistSubmissionAdmin(TestCase): addon_factory(guid='published@') BlocklistSubmission.objects.create( input_guids='pending1@\npending2@', - signoff_state=BlocklistSubmission.SIGNOFF_PENDING, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PENDING, ) BlocklistSubmission.objects.create( input_guids='missing@', - signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, delayed_until=now + timedelta(days=1), ) BlocklistSubmission.objects.create( input_guids='published@', - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, delayed_until=now - timedelta(days=1), ) @@ -1761,7 +2033,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': 'guid@', - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': [version.id], 'disable_addon': True, @@ -1809,7 +2081,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': 'guid@', - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': [version.id], 'url': 'dfd', @@ -1841,13 +2113,13 @@ class TestBlocklistSubmissionAdmin(TestCase): # no new Block objects yet even though under the threshold assert Block.objects.count() == 2 multi = BlocklistSubmission.objects.get() - assert multi.signoff_state == BlocklistSubmission.SIGNOFF_AUTOAPPROVED + assert multi.signoff_state == BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED assert not multi.is_submission_ready frozen_time.tick(delta=timedelta(days=delay_days, seconds=1)) # Now we're past, the submission is ready assert multi.is_submission_ready - assert multi.signoff_state == BlocklistSubmission.SIGNOFF_AUTOAPPROVED + assert multi.signoff_state == BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED multi.save_to_block_objects() self._test_add_multiple_verify_blocks( @@ -1857,7 +2129,9 @@ class TestBlocklistSubmissionAdmin(TestCase): existing_and_partial, has_signoff=False, ) - assert multi.reload().signoff_state == BlocklistSubmission.SIGNOFF_PUBLISHED + assert ( + multi.reload().signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + ) def test_approve_delayed(self): now = datetime.now() @@ -1871,7 +2145,7 @@ class TestBlocklistSubmissionAdmin(TestCase): delayed_until=now + timedelta(days=2), ) assert mbs.to_block[0]['guid'] == addon.guid - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING user = user_factory(email='someone@mozilla.com') self.grant_permission(user, 'Blocklist:Signoff') @@ -1891,7 +2165,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert mbs.signoff_by == user # Approved but not published - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_APPROVED + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.APPROVED # And no blocks have been created assert not Block.objects.exists() @@ -1908,7 +2182,7 @@ class TestBlocklistSubmissionAdmin(TestCase): input_guids='guid@\ninvalid@', changed_version_ids=[version.id], updated_by=user_factory(), - signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED, delayed_until=datetime.now() + timedelta(days=1), ) assert mbs.to_block[0]['guid'] == 'guid@' @@ -1942,7 +2216,7 @@ class TestBlocklistSubmissionAdmin(TestCase): assert mbs.reason != 'a reason' # And the blocklistsubmission was rejected, so no Blocks created - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_REJECTED + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.REJECTED assert Block.objects.count() == 0 assert not mbs.is_submission_ready @@ -1971,7 +2245,7 @@ class TestBlocklistSubmissionAdmin(TestCase): input_guids=addon.guid, changed_version_ids=[addon.current_version.id], updated_by=user_factory(), - signoff_state=BlocklistSubmission.SIGNOFF_PENDING, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PENDING, delayed_until=now + timedelta(days=1), ) assert mbs.to_block[0]['guid'] == addon.guid @@ -1997,7 +2271,7 @@ class TestBlocklistSubmissionAdmin(TestCase): 'adminform' ].form.errors # The blocklistsubmission wasn't approved or rejected though - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING assert Block.objects.count() == 0 # Then to a date that is already past @@ -2012,12 +2286,12 @@ class TestBlocklistSubmissionAdmin(TestCase): # new delayed date assert mbs.delayed_until == now # No change in state because it still needs signoff - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING assert Block.objects.count() == 0 # But if the submission didn't need dual signoff then it will be auto approved # reset the submission state first - mbs.signoff_state = BlocklistSubmission.SIGNOFF_AUTOAPPROVED + mbs.signoff_state = BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED mbs.delayed_until = now + timedelta(days=5) mbs.to_block[0]['average_daily_users'] = threshold - 1 mbs.save() @@ -2032,7 +2306,7 @@ class TestBlocklistSubmissionAdmin(TestCase): # new delayed date assert mbs.delayed_until == now # The submission is auto approved - assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_PUBLISHED + assert mbs.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED assert Block.objects.count() == 1 def test_not_disable_addon(self): @@ -2064,7 +2338,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, @@ -2134,7 +2408,7 @@ class TestBlocklistSubmissionAdmin(TestCase): self.submission_url, { 'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'), - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'block_type': BlockType.SOFT_BLOCKED, 'changed_version_ids': [ new_addon.current_version.id, @@ -2244,14 +2518,14 @@ class TestBlockAdminDelete(TestCase): self.submission_url, { 'guids': 'guid@\n{12345-6789}\nlegacy@', - 'action': str(BlocklistSubmission.ACTION_DELETE), + 'action': str(BlocklistSubmission.ACTIONS.DELETE), }, follow=True, ) content = response.content.decode('utf-8') # meta data for block: assert 'Add-on GUIDs (one per line)' not in content - assert 'Delete Blocks' in content + assert 'Unblock' in content assert 'guid@' in content assert 'Normal' in content assert f'{block_one_ver.addon.average_daily_users} users' in content @@ -2268,7 +2542,7 @@ class TestBlockAdminDelete(TestCase): self.submission_url, { 'input_guids': 'guid@\n{12345-6789}\nlegacy@', - 'action': str(BlocklistSubmission.ACTION_DELETE), + 'action': str(BlocklistSubmission.ACTIONS.DELETE), 'changed_version_ids': [ block_one_ver.addon.current_version.id, partial_new_version.id, @@ -2353,7 +2627,7 @@ class TestBlockAdminDelete(TestCase): submission = BlocklistSubmission.objects.get() submission.update( - signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, signoff_by=user_factory(), ) assert submission.is_submission_ready @@ -2380,7 +2654,7 @@ class TestBlockAdminDelete(TestCase): del_submission = BlocklistSubmission.objects.create( input_guids=other_addon.guid, changed_version_ids=[ver_del_subm.id], - action=BlocklistSubmission.ACTION_DELETE, + action=BlocklistSubmission.ACTIONS.DELETE, ) ver_deleted = version_factory(addon=other_addon) ver_deleted.delete() # shouldn't affect it's status @@ -2396,7 +2670,7 @@ class TestBlockAdminDelete(TestCase): self.submission_url, { 'guids': f'{addon.guid}\n {other_addon.guid}\n', - 'action': BlocklistSubmission.ACTION_DELETE, + 'action': BlocklistSubmission.ACTIONS.DELETE, }, ) doc = pq(response.content.decode('utf-8')) @@ -2462,7 +2736,7 @@ class TestBlockAdminDelete(TestCase): mbs = BlocklistSubmission.objects.create( input_guids='guid@', updated_by=user_factory(), - action=BlocklistSubmission.ACTION_DELETE, + action=BlocklistSubmission.ACTIONS.DELETE, ) assert mbs.to_block == [ { diff --git a/src/olympia/blocklist/tests/test_cron.py b/src/olympia/blocklist/tests/test_cron.py index 63e0e7caf7..a6899c620d 100644 --- a/src/olympia/blocklist/tests/test_cron.py +++ b/src/olympia/blocklist/tests/test_cron.py @@ -377,7 +377,7 @@ def test_process_blocklistsubmissions(): input_guids=past_guid, updated_by=user, delayed_until=datetime.now() - timedelta(days=1), - signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED, changed_version_ids=[ addon_factory( guid=past_guid, @@ -391,7 +391,7 @@ def test_process_blocklistsubmissions(): input_guids=past_signoff_guid, updated_by=user, delayed_until=datetime.now() - timedelta(days=1), - signoff_state=BlocklistSubmission.SIGNOFF_APPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, signoff_by=user_factory(), changed_version_ids=[ addon_factory( @@ -406,7 +406,7 @@ def test_process_blocklistsubmissions(): input_guids=future_guid, updated_by=user, delayed_until=datetime.now() + timedelta(days=1), - signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED, changed_version_ids=[ addon_factory( guid=future_guid, @@ -418,9 +418,14 @@ def test_process_blocklistsubmissions(): process_blocklistsubmissions() - assert past.reload().signoff_state == BlocklistSubmission.SIGNOFF_PUBLISHED - assert past_signoff.reload().signoff_state == BlocklistSubmission.SIGNOFF_PUBLISHED - assert future.reload().signoff_state == BlocklistSubmission.SIGNOFF_AUTOAPPROVED + assert past.reload().signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + assert ( + past_signoff.reload().signoff_state + == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + ) + assert ( + future.reload().signoff_state == BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED + ) assert Block.objects.filter(guid=past_guid).exists() assert Block.objects.filter(guid=past_signoff_guid).exists() diff --git a/src/olympia/blocklist/tests/test_forms.py b/src/olympia/blocklist/tests/test_forms.py index 4f39028488..210fbb6106 100644 --- a/src/olympia/blocklist/tests/test_forms.py +++ b/src/olympia/blocklist/tests/test_forms.py @@ -60,7 +60,7 @@ class TestBlocklistSubmissionForm(TestCase): def test_changed_version_ids_choices_add_action(self): data = { - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' @@ -114,7 +114,7 @@ class TestBlocklistSubmissionForm(TestCase): def test_changed_version_ids_choices_delete_action(self): Form = self.get_form() data = { - 'action': str(BlocklistSubmission.ACTION_DELETE), + 'action': str(BlocklistSubmission.ACTIONS.DELETE), 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' @@ -187,7 +187,7 @@ class TestBlocklistSubmissionForm(TestCase): submission = BlocklistSubmission.objects.create( input_guids=f'{self.new_addon.guid}\n{self.existing_block_partial.guid}', ) - for action in BlocklistSubmission.ACTIONS: + for action in BlocklistSubmission.ACTIONS.values.keys(): # they're the same for each of the actions submission.update( action=action, @@ -207,7 +207,7 @@ class TestBlocklistSubmissionForm(TestCase): def test_changed_version_ids_widget(self): data = { - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' @@ -224,7 +224,7 @@ class TestBlocklistSubmissionForm(TestCase): v_id for _guid, opts in field.choices for (v_id, _text) in opts ] assert field.widget.get_context(name, value, attrs) == { - 'is_add_change': True, + 'verb': 'Block', 'widget': { 'attrs': {'multiple': True, **attrs}, 'choices': flattened_choices, @@ -295,7 +295,7 @@ class TestBlocklistSubmissionForm(TestCase): def test_new_blocks_must_have_changed_versions(self): Form = self.get_form() data = { - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}\n' @@ -313,7 +313,7 @@ class TestBlocklistSubmissionForm(TestCase): # delete action is similar, but we allow deleting a block with no versions Block.objects.create(addon=self.new_addon, updated_by=self.user) - data['action'] = str(BlocklistSubmission.ACTION_DELETE) + data['action'] = str(BlocklistSubmission.ACTIONS.DELETE) data['changed_version_ids'] = [ self.partial_existing_addon_v_blocked.id, self.full_existing_addon_v1.id, @@ -339,7 +339,7 @@ class TestBlocklistSubmissionForm(TestCase): version=old_blocked_addon_version, block=self.existing_block_partial ) data = { - 'action': str(BlocklistSubmission.ACTION_ADDCHANGE), + 'action': str(BlocklistSubmission.ACTIONS.ADDCHANGE), 'input_guids': f'{self.new_addon.guid}\n' f'{self.existing_block_full.guid}\n' f'{self.existing_block_partial.guid}', @@ -360,7 +360,7 @@ class TestBlocklistSubmissionForm(TestCase): assert form.errors == { 'changed_version_ids': [ f'{self.partial_existing_addon.guid}:{ver_string} exists more than once' - f'. All {ver_string} versions must be blocked together' + f'. All {ver_string} versions must be selected together.' ] } diff --git a/src/olympia/blocklist/tests/test_models.py b/src/olympia/blocklist/tests/test_models.py index 965c459d98..a01fc62932 100644 --- a/src/olympia/blocklist/tests/test_models.py +++ b/src/olympia/blocklist/tests/test_models.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta +from unittest import mock from django.test.utils import override_settings @@ -13,6 +14,38 @@ from olympia.amo.tests import ( from ..models import BlocklistSubmission, BlockType, BlockVersion +class TestBlock(TestCase): + def setUp(self): + self.user = user_factory() + self.addon = addon_factory() + self.version = self.addon.current_version + self.version_2 = version_factory(addon=self.addon, version='2.0') + self.block = block_factory(updated_by=self.user, guid=self.addon.guid) + + def test_has_hard_blocked_versions(self): + assert self.block.has_hard_blocked_versions() + self.block.blockversion_set.filter(version=self.version).update( + block_type=BlockType.SOFT_BLOCKED + ) + assert self.block.has_hard_blocked_versions() + self.block.blockversion_set.filter(version=self.version_2).update( + block_type=BlockType.SOFT_BLOCKED + ) + assert not self.block.has_hard_blocked_versions() + + def test_has_soft_blocked_versions(self): + BlockVersion.objects.update(block_type=BlockType.SOFT_BLOCKED) + assert self.block.has_soft_blocked_versions() + self.block.blockversion_set.filter(version=self.version).update( + block_type=BlockType.BLOCKED + ) + assert self.block.has_soft_blocked_versions() + self.block.blockversion_set.filter(version=self.version_2).update( + block_type=BlockType.BLOCKED + ) + assert not self.block.has_soft_blocked_versions() + + class TestBlockVersion(TestCase): def setUp(self): self.user = user_factory() @@ -62,12 +95,12 @@ class TestBlocklistSubmission(TestCase): assert not block.signoff_by assert not block.is_submission_ready - # Except when the state is SIGNOFF_AUTOAPPROVED. - block.update(signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED) + # Except when the state is AUTOAPPROVED. + block.update(signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED) assert block.is_submission_ready - # But if the state is SIGNOFF_APPROVED we need to know the signoff user - block.update(signoff_state=BlocklistSubmission.SIGNOFF_APPROVED) + # But if the state is APPROVED we need to know the signoff user + block.update(signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED) assert not block.is_submission_ready # If different users update and signoff, it's permitted. @@ -85,7 +118,7 @@ class TestBlocklistSubmission(TestCase): def test_is_delayed_submission_ready(self): now = datetime.now() submission = BlocklistSubmission.objects.create( - signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED ) # auto approved submissions with no delay are ready assert submission.is_submission_ready @@ -119,14 +152,9 @@ class TestBlocklistSubmission(TestCase): ] # But by default we ignored "finished" BlocklistSubmissions - block_subm.update(signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED) + block_subm.update(signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED) assert list(BlocklistSubmission.get_submissions_from_guid('guid@')) == [] - # Except when we override the states to exclude - assert list( - BlocklistSubmission.get_submissions_from_guid('guid@', excludes=()) - ) == [block_subm] - # And check that a guid that doesn't exist in any submissions is empty assert list(BlocklistSubmission.get_submissions_from_guid('ggguid@')) == [] @@ -170,10 +198,30 @@ class TestBlocklistSubmission(TestCase): def test_is_delayed(self): now = datetime.now() submission = BlocklistSubmission.objects.create( - signoff_state=BlocklistSubmission.SIGNOFF_AUTOAPPROVED + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED ) assert not submission.is_delayed submission.update(delayed_until=now + timedelta(minutes=1)) assert submission.is_delayed submission.update(delayed_until=now - timedelta(minutes=1)) assert not submission.is_delayed + + @mock.patch('olympia.blocklist.models.save_versions_to_blocks') + def test_calls_to_save_versions_to_blocks_depending_on_action( + self, save_versions_to_blocks_mock + ): + addon_factory(guid='guid1@example') + addon_factory(guid='guid2@example') + for action in ( + BlocklistSubmission.ACTIONS.ADDCHANGE, + BlocklistSubmission.ACTIONS.HARDEN, + BlocklistSubmission.ACTIONS.SOFTEN, + ): + submission = BlocklistSubmission.objects.create( + input_guids='guid1@example\nguid2@example', + signoff_state=BlocklistSubmission.SIGNOFF_STATES.AUTOAPPROVED, + action=action, + ) + save_versions_to_blocks_mock.reset_mock() + submission.save_to_block_objects() + assert save_versions_to_blocks_mock.call_count == 1 diff --git a/src/olympia/blocklist/tests/test_tasks.py b/src/olympia/blocklist/tests/test_tasks.py index 40e924621a..cf17fc07dc 100644 --- a/src/olympia/blocklist/tests/test_tasks.py +++ b/src/olympia/blocklist/tests/test_tasks.py @@ -73,13 +73,15 @@ def test_cleanup_old_files(): class TestProcessBlocklistSubmission(TransactionTestCase): - def test_state_reset(self): - addon_factory(guid='guid@') + def setUp(self): user_factory(id=settings.TASK_USER_ID) - - submission = BlocklistSubmission.objects.create( - input_guids='guid@', signoff_state=BlocklistSubmission.SIGNOFF_APPROVED + self.addon = addon_factory(guid='guid@') + self.submission = BlocklistSubmission.objects.create( + input_guids=self.addon.guid, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.APPROVED, ) + + def test_state_reset(self): with mock.patch.object( BlocklistSubmission, 'save_to_block_objects', @@ -87,13 +89,39 @@ class TestProcessBlocklistSubmission(TransactionTestCase): ): with self.assertRaises(SuspiciousOperation): # we know it's going to raise, we just want to capture it safely - process_blocklistsubmission.delay(submission.id) - submission.reload() - assert submission.signoff_state == BlocklistSubmission.SIGNOFF_PENDING + process_blocklistsubmission.delay(self.submission.id) + self.submission.reload() + assert ( + self.submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING + ) log_entry = LogEntry.objects.get() assert log_entry.user.id == settings.TASK_USER_ID assert log_entry.change_message == 'Exception in task: Something happened!' + @mock.patch.object(BlocklistSubmission, 'save_to_block_objects') + @mock.patch.object(BlocklistSubmission, 'delete_block_objects') + def test_calls_save_to_block_objects_or_delete_block_objects_depending_on_action( + self, delete_block_objects_mock, save_to_block_objects_mock + ): + for action in ( + BlocklistSubmission.ACTIONS.ADDCHANGE, + BlocklistSubmission.ACTIONS.HARDEN, + BlocklistSubmission.ACTIONS.SOFTEN, + ): + save_to_block_objects_mock.reset_mock() + delete_block_objects_mock.reset_mock() + self.submission.update(action=action) + process_blocklistsubmission(self.submission.id) + assert save_to_block_objects_mock.call_count == 1 + assert delete_block_objects_mock.call_count == 0 + for action in (BlocklistSubmission.ACTIONS.DELETE,): + save_to_block_objects_mock.reset_mock() + delete_block_objects_mock.reset_mock() + self.submission.update(action=action) + process_blocklistsubmission(self.submission.id) + assert save_to_block_objects_mock.call_count == 0 + assert delete_block_objects_mock.call_count == 1 + @pytest.mark.django_db class TestUploadMLBFToRemoteSettings(TestCase): diff --git a/src/olympia/blocklist/tests/test_utils.py b/src/olympia/blocklist/tests/test_utils.py index 9a0908906f..b368c40352 100644 --- a/src/olympia/blocklist/tests/test_utils.py +++ b/src/olympia/blocklist/tests/test_utils.py @@ -9,7 +9,13 @@ import responses from olympia import amo from olympia.activity.models import ActivityLog -from olympia.amo.tests import TestCase, addon_factory, block_factory, user_factory +from olympia.amo.tests import ( + TestCase, + addon_factory, + block_factory, + user_factory, + version_factory, +) from ..models import Block, BlocklistSubmission, BlockType from ..utils import datetime_to_ts, disable_versions_for_block, save_versions_to_blocks @@ -58,7 +64,7 @@ class TestSaveVersionsToBlocks(TestCase): updated_by=user_new, disable_addon=True, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) ActivityLog.objects.all().delete() @@ -113,7 +119,7 @@ class TestSaveVersionsToBlocks(TestCase): disable_addon=True, block_type=BlockType.SOFT_BLOCKED, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) ActivityLog.objects.all().delete() @@ -171,7 +177,7 @@ class TestSaveVersionsToBlocks(TestCase): assert not Block.objects.exists() - def test_save_blocks_override_existing_block_type_soft_to_hard(self): + def test_save_blocks_harden_existing_block(self): user_new = user_factory() addon = addon_factory() block_factory( @@ -181,32 +187,34 @@ class TestSaveVersionsToBlocks(TestCase): ) assert addon.current_version.blockversion.block_type == BlockType.SOFT_BLOCKED submission = BlocklistSubmission.objects.create( + action=BlocklistSubmission.ACTIONS.HARDEN, input_guids=addon.guid, reason='some reason', url=None, updated_by=user_new, block_type=BlockType.BLOCKED, # Hard-block override. changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) save_versions_to_blocks([addon.guid], submission) assert ( addon.current_version.blockversion.reload().block_type == BlockType.BLOCKED ) - def test_save_blocks_override_existing_block_type_hard_to_soft(self): + def test_save_blocks_soften_existing_block(self): user_new = user_factory() addon = addon_factory() block_factory(guid=addon.guid, updated_by=self.task_user) assert addon.current_version.blockversion.block_type == BlockType.BLOCKED submission = BlocklistSubmission.objects.create( + action=BlocklistSubmission.ACTIONS.SOFTEN, input_guids=addon.guid, reason='some reason', url=None, updated_by=user_new, block_type=BlockType.SOFT_BLOCKED, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) save_versions_to_blocks([addon.guid], submission) assert ( @@ -228,7 +236,7 @@ class TestSaveVersionsToBlocks(TestCase): disable_addon=True, block_type=BlockType.BLOCKED, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) block = block_factory(addon=addon, updated_by=submission.updated_by) disable_versions_for_block(block, submission) @@ -254,7 +262,7 @@ class TestSaveVersionsToBlocks(TestCase): disable_addon=True, block_type=BlockType.BLOCKED, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) block = block_factory(addon=addon, updated_by=submission.updated_by) # We can't save a block with updated_by = None, but check that we still @@ -284,7 +292,7 @@ class TestSaveVersionsToBlocks(TestCase): disable_addon=True, block_type=BlockType.BLOCKED, changed_version_ids=[addon.current_version.pk], - signoff_state=BlocklistSubmission.SIGNOFF_PUBLISHED, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, ) block = block_factory(addon=addon, updated_by=submission.updated_by) disable_versions_for_block(block, submission) @@ -296,3 +304,62 @@ class TestSaveVersionsToBlocks(TestCase): 'review_type': 'pending', 'human_review': False, # False because it's the task user. } + + def test_save_blocks_harden_block_leaving_other_versions_alone(self): + user_new = user_factory() + addon = addon_factory() + version1 = addon.current_version + version2 = version_factory(addon=addon) + block_factory( + guid=addon.guid, + updated_by=self.task_user, + block_type=BlockType.SOFT_BLOCKED, + ) + for version in addon.versions.all(): + assert version.blockversion.block_type == BlockType.SOFT_BLOCKED + submission = BlocklistSubmission.objects.create( + action=BlocklistSubmission.ACTIONS.HARDEN, + input_guids=addon.guid, + reason='some reason', + url=None, + updated_by=user_new, + block_type=BlockType.BLOCKED, + changed_version_ids=[version1.pk], + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, + ) + save_versions_to_blocks([addon.guid], submission) + assert ( + version1.blockversion.reload().block_type + == BlockType.BLOCKED # Now hard-blocked. + ) + assert ( + version2.blockversion.reload().block_type + == BlockType.SOFT_BLOCKED # Untouched. + ) + + def test_save_blocks_soften_block_leaving_other_versions_alone(self): + user_new = user_factory() + addon = addon_factory() + version1 = addon.current_version + version2 = version_factory(addon=addon) + block_factory(guid=addon.guid, updated_by=self.task_user) + for version in addon.versions.all(): + assert version.blockversion.block_type == BlockType.BLOCKED + submission = BlocklistSubmission.objects.create( + action=BlocklistSubmission.ACTIONS.SOFTEN, + input_guids=addon.guid, + reason='some reason', + url=None, + updated_by=user_new, + block_type=BlockType.SOFT_BLOCKED, + changed_version_ids=[version1.pk], + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, + ) + save_versions_to_blocks([addon.guid], submission) + assert ( + version1.blockversion.reload().block_type + == BlockType.SOFT_BLOCKED # Now soft-blocked. + ) + assert ( + version2.blockversion.reload().block_type == BlockType.BLOCKED # Untouched. + ) diff --git a/src/olympia/blocklist/utils.py b/src/olympia/blocklist/utils.py index 4a8b2745be..498719ae98 100644 --- a/src/olympia/blocklist/utils.py +++ b/src/olympia/blocklist/utils.py @@ -40,9 +40,9 @@ def block_activity_log_save( } version_details = {} if submission_obj: - details['signoff_state'] = submission_obj.SIGNOFF_STATES.get( + details['signoff_state'] = submission_obj.SIGNOFF_STATES.for_value( submission_obj.signoff_state - ) + ).display if submission_obj.signoff_by: details['signoff_by'] = submission_obj.signoff_by.id details['soft'] = version_details['soft'] = ( @@ -104,9 +104,9 @@ def block_activity_log_delete(obj, deleted, *, submission_obj=None, delete_user= ) if submission_obj: - details['signoff_state'] = submission_obj.SIGNOFF_STATES.get( + details['signoff_state'] = submission_obj.SIGNOFF_STATES.for_value( submission_obj.signoff_state - ) + ).display if submission_obj.signoff_by: details['signoff_by'] = submission_obj.signoff_by.id diff --git a/static/css/admin/blocklist_block.css b/static/css/admin/blocklist_block.css index e0bfe4eab0..b120451837 100644 --- a/static/css/admin/blocklist_block.css +++ b/static/css/admin/blocklist_block.css @@ -24,3 +24,20 @@ list-style-type: none; font-style: italic; } + +.submit-row a.hardenlink, .submit-row a.softenlink { + display: inline-block; + padding: 10px 15px; + height: 0.9375rem; + line-height: 0.9375rem; + order: 3; +} + +.submit-row a.disabled { + opacity: 0.4; + pointer-events: none; +} + +.submit-row a.deletelink { + order: 2; +}