drop Block.include_in_legacy and derive legacy state from legacy_id (#15043)

docs/topics/blocklist.rst

@@ -171,7 +171,9 @@ Legacy Blocklist
 
 To populate the blocklist on AMO the legacy blocklist on Remote Settings was imported; all guids that matched addons on AMO (and that had at least one webextension version) were added; any guids that were :ref:`regular expressions<blocklist-doc-regex>` were "expanded" to individual records for each addon present in the AMO database.
 
-If the `include_in_legacy` is selected in AMO's admin tool the block will also be saved to the legacy blocklist.  Edits to Blocks will propagate to the legacy blocklist too (apart from :ref:`regular expression based blocks<blocklist-doc-regex>`). An existing Block can be edited to deselect `include_in_legacy` which would delete it from the legacy blocklist; or edited to add select which would add it to the legacy blocklist.
+If the `In legacy blocklist` checkbox is selected in AMO's admin tool the block will also be saved to the legacy blocklist.
+Edits to Blocks will propagate to the legacy blocklist too (apart from :ref:`regular expression based blocks<blocklist-doc-regex>`).
+An existing Block in the legacy blocklist can be removed (while keeping it in the current v3 blocklist) by deselecting the `In legacy blocklist` checkbox; or added to the legacy blocklist too by enabling the `In legacy blocklist` checkbox.
 
 
 ------------------
@@ -188,4 +190,4 @@ So Blocks imported from a regular expression in the legacy blocklist can be view
 A warning message is displayed and the user must manually make the changes to the legacy blocklist via the kinto admin tool.
 
 .. note::
-    Blocks with a `kinto-id` property starting with `*` were imported from regular expression based Remote Setting records.
+    Blocks with a `legacy_id` property starting with `*` were imported from regular expression based Remote Setting records.

settings.py

@@ -29,6 +29,7 @@ for logger in list(LOGGING['loggers'].keys()):
         # so we only update the level below:
         LOGGING['loggers'][logger]['level'] = logging.DEBUG
 
+
 # django-debug-doolbar middleware needs to be inserted as high as possible
 # but after GZip middleware
 def insert_debug_toolbar_middleware(middlewares):
@@ -145,7 +146,7 @@ CUSTOMS_API_KEY = 'customssecret'
 WAT_API_URL = 'http://wat:10102/'
 WAT_API_KEY = 'watsecret'
 
-KINTO_API_IS_TEST_SERVER = True
+REMOTE_SETTINGS_IS_TEST_SERVER = True
 
 # If you have settings you want to overload, put them in a local_settings.py.
 try:

src/olympia/blocklist/admin.py

@@ -17,7 +17,8 @@ from olympia.amo.urlresolvers import reverse
 from olympia.amo.utils import HttpResponseTemporaryRedirect
 from olympia.versions.models import Version
 
-from .forms import BlocklistSubmissionForm, MultiAddForm, MultiDeleteForm
+from .forms import (
+    BlockForm, BlocklistSubmissionForm, MultiAddForm, MultiDeleteForm)
 from .models import Block, BlocklistSubmission
 from .tasks import process_blocklistsubmission
 from .utils import splitlines
@@ -293,7 +294,7 @@ class BlocklistSubmissionAdmin(admin.ModelAdmin):
                     'reason',
                     'updated_by',
                     'signoff_by',
-                    'include_in_legacy',
+                    'legacy_id',
                     'submission_logs',
                 ),
             })
@@ -326,7 +327,7 @@ class BlocklistSubmissionAdmin(admin.ModelAdmin):
             'submission_logs',
         ]
         if not waffle.switch_is_active('blocklist_legacy_submit'):
-            ro_fields.append('include_in_legacy')
+            ro_fields.append('legacy_id')
         if obj:
             ro_fields += [
                 'input_guids',
@@ -586,6 +587,7 @@ class BlockAdmin(BlockAdminAddMixin, admin.ModelAdmin):
     list_select_related = ('updated_by',)
     change_list_template = 'admin/blocklist/block_change_list.html'
     change_form_template = 'admin/blocklist/block_change_form.html'
+    form = BlockForm
 
     class Media:
         css = {
@@ -644,7 +646,7 @@ class BlockAdmin(BlockAdminAddMixin, admin.ModelAdmin):
                     'max_version',
                     ('url', 'url_link'),
                     'reason',
-                    'include_in_legacy'),
+                    'legacy_id'),
             })
 
         return (details, history, edit)
@@ -652,7 +654,7 @@ class BlockAdmin(BlockAdminAddMixin, admin.ModelAdmin):
     def get_readonly_fields(self, request, obj=None):
         fields = list(self._readonly_fields)
         if not waffle.switch_is_active('blocklist_legacy_submit'):
-            fields.append('include_in_legacy')
+            fields.append('legacy_id')
         return fields
 
     def has_change_permission(self, request, obj=None):

src/olympia/blocklist/forms.py

@@ -124,3 +124,10 @@ class BlocklistSubmissionForm(forms.ModelForm):
                       'Max version has changed.')))
         if errors:
             raise ValidationError(errors)
+
+
+class BlockForm(forms.ModelForm):
+    legacy_id = forms.fields.BooleanField(
+        label='In legacy blocklist',
+        required=False,
+        help_text='Include in legacy xml blocklist too, as well as new v3')

src/olympia/blocklist/management/commands/bulk_add_blocks.py

@@ -35,7 +35,6 @@ class Command(BaseCommand):
             if options.get(prop)
         }
         block_args['updated_by'] = get_task_user()
-        block_args['include_in_legacy'] = False
         submission = BlocklistSubmission(**block_args)
 
         for guids_chunk in chunked(guids, 100):

src/olympia/blocklist/migrations/0017_auto_20200723_0906.py

@@ -0,0 +1,28 @@
+# Generated by Django 2.2.14 on 2020-07-23 09:06
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('blocklist', '0016_auto_20200521_1710'),
+    ]
+
+    operations = [
+        migrations.RenameField(
+            model_name='blocklistsubmission',
+            old_name='include_in_legacy',
+            new_name='legacy_id',
+        ),
+        migrations.AlterField(
+            model_name='blocklistsubmission',
+            name='legacy_id',
+            field=models.BooleanField(db_column='include_in_legacy', default=False, help_text='Include in legacy xml blocklist too, as well as new v3'),
+        ),
+        migrations.AlterField(
+            model_name='block',
+            name='include_in_legacy',
+            field=models.NullBooleanField(default=None, help_text='Include in legacy xml blocklist too, as well as new v3'),
+        ),
+    ]

src/olympia/blocklist/models.py

@@ -37,9 +37,6 @@ class Block(ModelBase):
     reason = models.TextField(blank=True)
     updated_by = models.ForeignKey(
         UserProfile, null=True, on_delete=models.SET_NULL)
-    include_in_legacy = models.BooleanField(
-        default=False,
-        help_text='Include in legacy xml blocklist too, as well as new v3')
     legacy_id = models.CharField(
         max_length=255, null=False, default='', db_index=True,
         db_column='kinto_id')
@@ -70,6 +67,10 @@ class Block(ModelBase):
     def is_imported_from_legacy_regex(self):
         return self.legacy_id.startswith('*')
 
+    @property
+    def in_legacy_blocklist(self):
+        return bool(self.legacy_id)
+
     @classmethod
     def get_addons_for_guids_qs(cls, guids):
         return Addon.unfiltered.filter(
@@ -238,9 +239,11 @@ class BlocklistSubmission(ModelBase):
     reason = models.TextField(blank=True)
     updated_by = models.ForeignKey(
         UserProfile, null=True, on_delete=models.SET_NULL)
-    include_in_legacy = models.BooleanField(
+    legacy_id = models.BooleanField(
         default=False,
-        help_text='Include in legacy xml blocklist too, as well as new v3')
+        help_text='Include in legacy xml blocklist too, as well as new v3',
+        db_column='include_in_legacy',
+        verbose_name='In legacy blocklist')
     signoff_by = models.ForeignKey(
         UserProfile, null=True, on_delete=models.SET_NULL, related_name='+')
     signoff_state = models.SmallIntegerField(
@@ -261,12 +264,18 @@ class BlocklistSubmission(ModelBase):
         trimmed = [rep if len(rep) < 40 else rep[0:37] + '...' for rep in repr]
         return f'{self.get_signoff_state_display()}: {"; ".join(trimmed)}'
 
+    @property
+    def in_legacy_blocklist(self):
+        # This is for consitency with Block.  Should be a boolean anyway.
+        return bool(self.legacy_id)
+
     def get_changes_from_block(self, block):
         # return a dict with properties that are different from a given block,
         # as a dict of property_name: (old_value, new_value).
         changes = {}
         properties = (
-            'min_version', 'max_version', 'url', 'reason', 'include_in_legacy')
+            'min_version', 'max_version', 'url', 'reason',
+            'in_legacy_blocklist')
         for prop in properties:
             if getattr(self, prop) != getattr(block, prop):
                 changes[prop] = (getattr(block, prop), getattr(self, prop))
@@ -465,16 +474,16 @@ class BlocklistSubmission(ModelBase):
             'reason',
             'updated_by',
         ]
-        if submit_legacy_switch:
-            fields_to_set.append('include_in_legacy')
-
         all_guids_to_block = [block['guid'] for block in self.to_block]
 
         for guids_chunk in chunked(all_guids_to_block, 100):
             blocks = save_guids_to_blocks(
                 guids_chunk, self, fields_to_set=fields_to_set)
             if submit_legacy_switch:
-                legacy_publish_blocks(blocks)
+                if self.in_legacy_blocklist:
+                    legacy_publish_blocks(blocks)
+                else:
+                    legacy_delete_blocks(blocks)
             self.save()
 
         self.update(signoff_state=self.SIGNOFF_PUBLISHED)

src/olympia/blocklist/tasks.py

@@ -89,7 +89,6 @@ def import_block_from_blocklist(record):
         'url': record.get('details', {}).get('bug') or '',
         'reason': record.get('details', {}).get('why') or '',
         'legacy_id': legacy_id,
-        'include_in_legacy': True,
         'updated_by': get_task_user(),
     }
     modified_date = datetime.fromtimestamp(

src/olympia/blocklist/templates/admin/blocklist/blocklistsubmission_add_form.html

@@ -104,10 +104,10 @@
     </div>
     <div class="form-row">
       <div class="checkbox-row">
-        {{ form.include_in_legacy }}
-        {{ form.include_in_legacy.label_tag }}
-        {{ form.include_in_legacy.errors }}
-        <div class="help">{{ form.include_in_legacy.help_text }}</div>
+        {{ form.legacy_id }}
+        {{ form.legacy_id.label_tag }}
+        {{ form.legacy_id.errors }}
+        <div class="help">{{ form.legacy_id.help_text }}</div>
       </div>
     </div>
     {% block submit_buttons_bottom %}

src/olympia/blocklist/tests/test_admin.py

@@ -145,7 +145,7 @@ class TestBlockAdmin(TestCase):
         self.assertRedirects(response, self.submission_url, status_code=307)
 
         # but not if it's imported from a legacy record
-        block.update(include_in_legacy=True, legacy_id='343545')
+        block.update(legacy_id='343545')
         response = self.client.post(
             self.add_url, post_data, follow=True)
         assert b'Add-on GUIDs (one per line)' in response.content
@@ -310,7 +310,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert f'Included in legacy blocklist' not in content
 
     @override_switch('blocklist_legacy_submit', active=False)
-    def test_include_in_legacy_property_readonly(self):
+    def test_legacy_id_property_readonly(self):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
@@ -319,10 +319,10 @@ class TestBlocklistSubmissionAdmin(TestCase):
         addon = addon_factory()
         response = self.client.get(
             self.submission_url + f'?guids={addon.guid}', follow=True)
-        assert not pq(response.content)('#id_include_in_legacy')
+        assert not pq(response.content)('#id_legacy_id')
         assert b'_save' in response.content
 
-        # Try to set include_in_legacy
+        # Try to set legacy_id
         response = self.client.post(
             self.submission_url, {
                 'input_guids': addon.guid,
@@ -332,7 +332,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
                 'existing_min_version': addon.current_version.version,
                 'existing_max_version': addon.current_version.version,
                 'url': '',
-                'include_in_legacy': True,
+                'legacy_id': True,
                 'reason': 'Added!',
                 '_save': 'Save',
             },
@@ -342,11 +342,14 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert Block.objects.exists()
         block = Block.objects.get()
         assert block.reason == 'Added!'
-        assert block.include_in_legacy is False
+        assert block.in_legacy_blocklist is False
+        assert BlocklistSubmission.objects.get().in_legacy_blocklist is False
 
     @override_switch('blocklist_legacy_submit', active=True)
+    @mock.patch('olympia.blocklist.models.legacy_delete_blocks')
     @mock.patch('olympia.blocklist.models.legacy_publish_blocks')
-    def test_include_in_legacy_enabled_with_legacy_submit_waffle_on(self, pbm):
+    def test_legacy_id_enabled_with_legacy_submit_waffle_on(self, publish_mock,
+                                                            delete_mock):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
@@ -355,10 +358,10 @@ class TestBlocklistSubmissionAdmin(TestCase):
         addon = addon_factory()
         response = self.client.get(
             self.submission_url + f'?guids={addon.guid}', follow=True)
-        assert pq(response.content)('#id_include_in_legacy')
+        assert pq(response.content)('#id_legacy_id')
         assert b'_save' in response.content
 
-        # Try to set include_in_legacy
+        # Try to set legacy_id
         response = self.client.post(
             self.submission_url, {
                 'input_guids': addon.guid,
@@ -368,7 +371,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
                 'existing_min_version': addon.current_version.version,
                 'existing_max_version': addon.current_version.version,
                 'url': '',
-                'include_in_legacy': True,
+                'legacy_id': True,
                 'reason': 'Added!',
                 '_save': 'Save',
             },
@@ -378,10 +381,11 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert Block.objects.exists()
         block = Block.objects.get()
         assert block.reason == 'Added!'
-        assert block.include_in_legacy is True
-        pbm.assert_called_once()
+        publish_mock.assert_called_once()
+        delete_mock.assert_not_called()
 
         # And again with the opposite
+        publish_mock.reset_mock()
         addon = addon_factory()
         response = self.client.post(
             self.submission_url, {
@@ -397,7 +401,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert response.status_code == 200
         block = Block.objects.latest()
         assert block.reason == 'Added again!'
-        assert block.include_in_legacy is False
+        publish_mock.assert_not_called()
+        delete_mock.assert_called_once()
 
     def _test_add_multiple_submit(self, addon_adu):
         """addon_adu is important because whether dual signoff is needed is
@@ -500,7 +505,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert existing_and_partial.max_version == '*'
         assert existing_and_partial.reason == 'some reason'
         assert existing_and_partial.url == 'dfd'
-        assert existing_and_partial.include_in_legacy is False
+        assert existing_and_partial.in_legacy_blocklist is False
         edit_log = ActivityLog.objects.for_addons(partial_addon).last()
         assert edit_log.action == amo.LOG.BLOCKLIST_BLOCK_EDITED.id
         assert edit_log.arguments == [
@@ -684,7 +689,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         assert existing_zero_to_max.max_version == '10'
         assert existing_zero_to_max.reason == 'some reason'
         assert existing_zero_to_max.url == 'dfd'
-        assert existing_zero_to_max.include_in_legacy is False
+        assert existing_zero_to_max.in_legacy_blocklist is False
         log = ActivityLog.objects.for_addons(existing_zero_to_max.addon).get()
         assert log.action == amo.LOG.BLOCKLIST_BLOCK_EDITED.id
         assert log.arguments == [
@@ -705,7 +710,7 @@ class TestBlocklistSubmissionAdmin(TestCase):
         # confirm properties *were not* updated.
         assert existing_one_to_ten.reason != 'some reason'
         assert existing_one_to_ten.url != 'dfd'
-        assert existing_one_to_ten.include_in_legacy is False
+        assert existing_one_to_ten.in_legacy_blocklist is False
         assert not ActivityLog.objects.for_addons(
             existing_one_to_ten.addon).exists()
         assert not ActivityLog.objects.for_version(
@@ -741,7 +746,6 @@ class TestBlocklistSubmissionAdmin(TestCase):
             addon=addon_factory(guid='full@existing', name='Full Danger'),
             min_version='0',
             max_version='*',
-            include_in_legacy=True,
             legacy_id='34345',
             updated_by=user_factory())
         partial_addon = addon_factory(
@@ -750,14 +754,12 @@ class TestBlocklistSubmissionAdmin(TestCase):
             addon=partial_addon,
             min_version='1',
             max_version='99',
-            include_in_legacy=True,
             legacy_id='75456',
             updated_by=user_factory())
         Block.objects.create(
             addon=addon_factory(guid='regex@legacy'),
             min_version='23',
             max_version='567',
-            include_in_legacy=True,
             legacy_id='*regexlegacy',
             updated_by=user_factory())
         response = self.client.post(
@@ -799,7 +801,6 @@ class TestBlocklistSubmissionAdmin(TestCase):
             addon=addon_factory(guid='full@existing', name='Full Danger'),
             min_version='0',
             max_version='*',
-            include_in_legacy=True,
             legacy_id='5656',
             updated_by=user_factory())
         partial_addon = addon_factory(
@@ -808,14 +809,12 @@ class TestBlocklistSubmissionAdmin(TestCase):
             addon=partial_addon,
             min_version='1',
             max_version='99',
-            include_in_legacy=True,
             legacy_id='74356',
             updated_by=user_factory())
         Block.objects.create(
             addon=addon_factory(guid='regex@legacy'),
             min_version='23',
             max_version='567',
-            include_in_legacy=True,
             legacy_id='*regexlegacy',
             updated_by=user_factory())
         response = self.client.post(
@@ -1161,7 +1160,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
         addon = addon_factory(guid='guid@', name='Danger Danger')
         mbs = BlocklistSubmission.objects.create(
             input_guids='guid@\ninvalid@',
-            updated_by=user_factory())
+            updated_by=user_factory(),
+            legacy_id=True)
         assert mbs.to_block == [
             {'guid': 'guid@',
              'id': None,
@@ -1533,7 +1533,6 @@ class TestBlockAdminEdit(TestCase):
                 'max_version': self.addon.current_version.version,
                 'url': 'https://foo.baa',
                 'reason': 'some other reason',
-                'include_in_legacy': True,
                 '_continue': 'Save and continue editing',
             },
             follow=True)
@@ -1640,7 +1639,6 @@ class TestBlockAdminEdit(TestCase):
             'action': '0',
             'url': 'https://foo.baa',
             'reason': 'some other reason',
-            'include_in_legacy': True,
             '_save': 'Update',
         }
         # Try saving the form with the same min_version
@@ -1801,7 +1799,7 @@ class TestBlockAdminEdit(TestCase):
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
         self.client.login(email=user.email)
-        self.block.update(legacy_id='123456', include_in_legacy=True)
+        self.block.update(legacy_id='123456')
 
         response = self.client.get(self.change_url, follow=True)
         content = response.content.decode('utf-8')
@@ -1829,12 +1827,12 @@ class TestBlockAdminEdit(TestCase):
 
     @override_switch('blocklist_legacy_submit', active=True)
     @mock.patch('olympia.blocklist.models.legacy_publish_blocks')
-    def test_can_edit_imported_block_if_legacy_submit_waffle_on(self, pb_mock):
+    def test_can_edit_imported_block_if_legacy_submit_waffle_on(self, pub_mck):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
         self.client.login(email=user.email)
-        self.block.update(legacy_id='123456', include_in_legacy=True)
+        self.block.update(legacy_id='123456')
 
         response = self.client.get(self.change_url, follow=True)
         content = response.content.decode('utf-8')
@@ -1844,6 +1842,7 @@ class TestBlockAdminEdit(TestCase):
         assert 'Close' not in content
         assert '_save' in content
         assert 'deletelink' in content
+        assert self.block.in_legacy_blocklist is True
 
         # We can edit the block
         assert not BlocklistSubmission.objects.exists()
@@ -1855,6 +1854,7 @@ class TestBlockAdminEdit(TestCase):
                 'max_version': self.addon.current_version.version,
                 'url': 'dfd',
                 'reason': 'some reason',
+                'legacy_id': '123456',
                 '_save': 'Save',
             },
             follow=True)
@@ -1862,21 +1862,23 @@ class TestBlockAdminEdit(TestCase):
         assert BlocklistSubmission.objects.exists()
         BlocklistSubmission.objects.get(
             input_guids=self.block.guid)
-        pb_mock.assert_called_with([self.block])
+        pub_mck.assert_called_with([self.block])
+        self.block.reload()
+        assert self.block.in_legacy_blocklist is True
 
     @override_switch('blocklist_legacy_submit', active=False)
-    def test_include_in_legacy_property_is_readonly(self):
+    def test_legacy_id_property_is_readonly(self):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
         self.client.login(email=user.email)
-        self.block.update(include_in_legacy=True)
+        self.block.update(legacy_id='')
 
         response = self.client.get(self.change_url, follow=True)
-        assert pq(response.content)('.field-include_in_legacy .readonly')
+        assert pq(response.content)('.field-legacy_id .readonly')
         assert b'_save' in response.content
 
-        assert self.block.include_in_legacy is True
+        assert self.block.in_legacy_blocklist is False
         # Try to edit the block
         response = self.client.post(
             self.change_url, {
@@ -1886,44 +1888,27 @@ class TestBlockAdminEdit(TestCase):
                 'max_version': self.block.max_version,
                 'url': '',
                 'reason': 'Changed!',
+                'legacy_id': '34344545',
                 '_save': 'Save',
             },
             follow=True)
         assert response.status_code == 200
-        assert BlocklistSubmission.objects.exists()
         self.block.reload()
         assert self.block.reason == 'Changed!'
-        assert self.block.include_in_legacy is True
-
-        # And again with the opposite
-        self.block.update(include_in_legacy=False)
-        response = self.client.post(
-            self.change_url, {
-                'input_guids': self.block.guid,
-                'action': '0',
-                'min_version': self.block.min_version,
-                'max_version': self.block.max_version,
-                'url': '',
-                'reason': 'Changed again!',
-                'include_in_legacy': True,
-                '_save': 'Save',
-            },
-            follow=True)
-        assert response.status_code == 200
-        self.block.reload()
-        assert self.block.reason == 'Changed again!'
-        assert self.block.include_in_legacy is False
+        assert self.block.in_legacy_blocklist is False
 
     @override_switch('blocklist_legacy_submit', active=True)
-    def test_include_in_legacy_is_enabled_with_legacy_submit_waffle_on(self):
+    @mock.patch('olympia.blocklist.models.legacy_delete_blocks')
+    def test_legacy_id_is_enabled_with_legacy_submit_waffle_on(self, del_mock):
+        del_mock.side_effect = lambda blocks: blocks[0].update(legacy_id='')
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Blocklist:Create')
         self.client.login(email=user.email)
-        self.block.update(include_in_legacy=True)
+        self.block.update(legacy_id='3467635')
 
         response = self.client.get(self.change_url, follow=True)
-        assert pq(response.content)('.field-include_in_legacy input')
+        assert pq(response.content)('.field-legacy_id input')
         assert b'_save' in response.content
 
         # Try to edit the block
@@ -1935,6 +1920,7 @@ class TestBlockAdminEdit(TestCase):
                 'max_version': self.block.max_version,
                 'url': '',
                 'reason': 'Changed!',
+                # no legacy_id so clearing it
                 '_save': 'Save',
             },
             follow=True)
@@ -1942,7 +1928,8 @@ class TestBlockAdminEdit(TestCase):
         assert BlocklistSubmission.objects.exists()
         self.block.reload()
         assert self.block.reason == 'Changed!'
-        assert self.block.include_in_legacy is False
+        del_mock.assert_called_with([self.block])
+        assert self.block.in_legacy_blocklist is False
 
 
 class TestBlockAdminDelete(TestCase):
@@ -2006,11 +1993,14 @@ class TestBlockAdminDelete(TestCase):
             updated_by=user_factory())
         block_no_addon = Block.objects.create(
             guid='{12345-6789}',
-            updated_by=user_factory())  #
+            updated_by=user_factory())
+        block_legacy = Block.objects.create(
+            addon=addon_factory(guid='legacy@'),
+            legacy_id='123456', updated_by=user_factory())
 
         response = self.client.post(
             self.submission_url, {
-                'guids': 'guid@\n{12345-6789}',
+                'guids': 'guid@\n{12345-6789}\nlegacy@',
                 'action': '1',
             },
             follow=True)
@@ -2026,25 +2016,25 @@ class TestBlockAdminDelete(TestCase):
         assert '"min_version"' not in content
         assert '"max_version"' not in content
         assert 'reason' not in content
-        assert 'include_in_legacy' not in content
+        assert 'legacy_id' not in content
         # Check we didn't delete the blocks already
-        assert Block.objects.count() == 2
+        assert Block.objects.count() == 3
         assert BlocklistSubmission.objects.count() == 0
 
         # Create the block submission
         response = self.client.post(
             self.submission_url, {
                 'input_guids': (
-                    'guid@\n{12345-6789}'),
+                    'guid@\n{12345-6789}\nlegacy@'),
                 'action': '1',
                 '_save': 'Save',
             },
             follow=True)
         assert response.status_code == 200
-        return block_normal, block_no_addon
+        return block_normal, block_no_addon, block_legacy
 
     def _test_delete_verify(self, block_with_addon, block_no_addon,
-                            has_signoff=True):
+                            block_legacy, has_signoff=True):
         block_from_addon = block_with_addon.addon
         assert Block.objects.count() == 0
         assert BlocklistSubmission.objects.count() == 1
@@ -2065,13 +2055,15 @@ class TestBlockAdminDelete(TestCase):
         assert vlog == add_log
 
         assert submission.input_guids == (
-            'guid@\n{12345-6789}')
+            'guid@\n{12345-6789}\nlegacy@')
 
         assert submission.to_block == [
             {'guid': 'guid@', 'id': block_with_addon.id,
              'average_daily_users': block_from_addon.average_daily_users},
+            {'guid': 'legacy@', 'id': block_legacy.id,
+             'average_daily_users': block_legacy.addon.average_daily_users},
             {'guid': '{12345-6789}', 'id': block_no_addon.id,
-             'average_daily_users': -1}
+             'average_daily_users': -1},
         ]
         assert not submission.block_set.all().exists()
 
@@ -2079,23 +2071,26 @@ class TestBlockAdminDelete(TestCase):
     @mock.patch('olympia.blocklist.models.legacy_delete_blocks')
     def test_submit_no_dual_signoff(self, legacy_delete_blocks_mock):
         addon_adu = settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD
-        block_with_addon, block_no_addon = self._test_delete_multiple_submit(
-            addon_adu=addon_adu)
+        block_with_addon, block_no_addon, block_legacy = (
+            self._test_delete_multiple_submit(addon_adu=addon_adu))
         self._test_delete_verify(
             block_with_addon,
             block_no_addon,
+            block_legacy,
             has_signoff=False)
-        legacy_delete_blocks_mock.assert_called_with(
-            [block_with_addon, block_no_addon])
+        legacy_delete_blocks_mock.assert_called_with([
+            block_with_addon,
+            block_no_addon,
+            block_legacy])
 
     @override_switch('blocklist_legacy_submit', active=True)
     @mock.patch('olympia.blocklist.models.legacy_delete_blocks')
     def test_submit_dual_signoff(self, legacy_delete_blocks_mock):
         addon_adu = settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD + 1
-        block_with_addon, block_no_addon = self._test_delete_multiple_submit(
-            addon_adu=addon_adu)
+        block_with_addon, block_no_addon, block_legacy = (
+            self._test_delete_multiple_submit(addon_adu=addon_adu))
         # Blocks shouldn't have been deleted yet
-        assert Block.objects.count() == 2, Block.objects.all()
+        assert Block.objects.count() == 3, Block.objects.all()
 
         submission = BlocklistSubmission.objects.get()
         submission.update(
@@ -2106,9 +2101,12 @@ class TestBlockAdminDelete(TestCase):
         self._test_delete_verify(
             block_with_addon,
             block_no_addon,
+            block_legacy,
             has_signoff=True)
-        legacy_delete_blocks_mock.assert_called_with(
-            [block_with_addon, block_no_addon])
+        legacy_delete_blocks_mock.assert_called_with([
+            block_with_addon,
+            block_no_addon,
+            block_legacy])
 
     def test_edit_with_delete_submission(self):
         threshold = settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD

src/olympia/blocklist/tests/test_commands.py

@@ -83,7 +83,7 @@ class TestImportBlocklist(TestCase):
             assert block.max_version == (
                 this_block['versionRange'][0]['maxVersion'])
             assert block.legacy_id == '*' + this_block['id']
-            assert block.include_in_legacy
+            assert block.in_legacy_blocklist
             assert block.modified == datetime(2019, 11, 29, 22, 22, 46, 785000)
             assert block.is_imported_from_legacy_regex
         assert LegacyImport.objects.count() == 8
@@ -135,7 +135,7 @@ class TestImportBlocklist(TestCase):
         assert blocks[0].max_version == (
             blocklist_json['data'][1]['versionRange'][0]['maxVersion'])
         assert blocks[0].legacy_id == blocklist_json['data'][1]['id']
-        assert blocks[0].include_in_legacy
+        assert blocks[0].in_legacy_blocklist
         assert blocks[0].modified == datetime(2019, 11, 29, 15, 32, 56, 477000)
         assert not blocks[0].is_imported_from_legacy_regex
 
@@ -147,7 +147,7 @@ class TestImportBlocklist(TestCase):
         assert blocks[1].max_version == (
             blocklist_json['data'][2]['versionRange'][0]['maxVersion'])
         assert blocks[1].legacy_id == blocklist_json['data'][2]['id']
-        assert blocks[1].include_in_legacy
+        assert blocks[1].in_legacy_blocklist
         assert blocks[1].modified == datetime(2019, 11, 22, 16, 49, 58, 416000)
         assert not blocks[1].is_imported_from_legacy_regex
 
@@ -227,7 +227,7 @@ class TestImportBlocklist(TestCase):
             assert block.max_version == (
                 this_block['versionRange'][0]['maxVersion'])
             assert block.legacy_id == '*' + this_block['id']
-            assert block.include_in_legacy
+            assert block.in_legacy_blocklist
 
     def test_regex_syntax_changed_to_mysql(self):
         """mysql doesn't support /d special charactor, only [:digit:]."""

src/olympia/blocklist/tests/test_utils.py

@@ -11,27 +11,15 @@ from olympia.lib.remote_settings import RemoteSettings
 
 @pytest.mark.django_db
 @mock.patch.object(RemoteSettings, 'publish_record')
-@mock.patch.object(RemoteSettings, 'delete_record')
-def test_legacy_publish_blocks(delete_mock, publish_mock):
+def test_legacy_publish_blocks(publish_mock):
     publish_mock.return_value = {'id': 'a-legacy-id'}
     block_new = Block.objects.create(
-        guid='new@guid', include_in_legacy=True, updated_by=user_factory())
+        guid='new@guid', updated_by=user_factory())
     block_regex = Block.objects.create(
-        guid='regex@guid', include_in_legacy=True, updated_by=user_factory(),
-        legacy_id='*regex')
-    block_legacy_dropped = Block.objects.create(
-        guid='drop@guid', include_in_legacy=False, updated_by=user_factory(),
-        legacy_id='dropped_legacy')
-    block_legacy_dropped_regex = Block.objects.create(
-        guid='rgdrop@guid', include_in_legacy=False, updated_by=user_factory(),
-        legacy_id='*dropped_legacy')
-    block_never_legacy = Block.objects.create(
-        guid='never@guid', include_in_legacy=False, updated_by=user_factory())
+        guid='regex@guid', updated_by=user_factory(), legacy_id='*regex')
     block_update = Block.objects.create(
-        guid='update@guid', include_in_legacy=True, updated_by=user_factory(),
-        legacy_id='update')
-    # Currently we don't ever mix include_in_legacy=True and False together,
-    # but the function should handle it.
+        guid='update@guid', updated_by=user_factory(), legacy_id='update')
+
     data = {
         'details': {
             'bug': '',
@@ -49,19 +37,12 @@ def test_legacy_publish_blocks(delete_mock, publish_mock):
     legacy_publish_blocks([
         block_new,
         block_regex,
-        block_legacy_dropped,
-        block_legacy_dropped_regex,
-        block_never_legacy,
         block_update])
     assert publish_mock.call_args_list == [
         mock.call(dict(guid='new@guid', **data)),
         mock.call(dict(guid='update@guid', **data), 'update')]
-    assert delete_mock.call_args_list == [
-        mock.call('dropped_legacy')]
     assert block_new.legacy_id == 'a-legacy-id'
     assert block_regex.legacy_id == '*regex'  # not changed
-    assert block_legacy_dropped_regex.legacy_id == ''  # cleared anyway
-    assert block_legacy_dropped.legacy_id == ''
     assert block_update.legacy_id == 'update'  # not changed
 
 
@@ -69,19 +50,13 @@ def test_legacy_publish_blocks(delete_mock, publish_mock):
 @mock.patch.object(RemoteSettings, 'delete_record')
 def test_legacy_delete_blocks(delete_record_mock):
     block = Block.objects.create(
-        guid='legacy@guid', include_in_legacy=True, updated_by=user_factory(),
-        legacy_id='legacy')
+        guid='legacy@guid', updated_by=user_factory(), legacy_id='legacy')
     block_regex = Block.objects.create(
-        guid='regex@guid', include_in_legacy=True, updated_by=user_factory(),
-        legacy_id='*regex')
+        guid='regex@guid', updated_by=user_factory(), legacy_id='*regex')
     block_not_legacy = Block.objects.create(
-        guid='not@guid', include_in_legacy=False, updated_by=user_factory(),
-        legacy_id='not_legacy')
-    block_not_imported = Block.objects.create(
-        guid='new@guid', include_in_legacy=True, updated_by=user_factory())
+        guid='not@guid', updated_by=user_factory())
 
-    legacy_delete_blocks(
-        [block, block_regex, block_not_legacy, block_not_imported])
+    legacy_delete_blocks([block, block_regex, block_not_legacy])
     assert delete_record_mock.call_args_list == [mock.call('legacy')]
     assert block.legacy_id == ''
     assert block_regex.legacy_id == ''  # cleared anyway

src/olympia/blocklist/utils.py

@@ -27,13 +27,16 @@ def block_activity_log_save(obj, change, submission_obj=None):
     action = (
         amo.LOG.BLOCKLIST_BLOCK_EDITED if change else
         amo.LOG.BLOCKLIST_BLOCK_ADDED)
+    legacy_inclusion = getattr(
+        submission_obj if submission_obj else obj,
+        'in_legacy_blocklist')
     details = {
         'guid': obj.guid,
         'min_version': obj.min_version,
         'max_version': obj.max_version,
         'url': obj.url,
         'reason': obj.reason,
-        'include_in_legacy': obj.include_in_legacy,
+        'include_in_legacy': legacy_inclusion,
         'comments': f'Versions {obj.min_version} - {obj.max_version} blocked.',
     }
     if submission_obj:
@@ -69,7 +72,7 @@ def block_activity_log_delete(obj, *, submission_obj=None, delete_user=None):
         'max_version': obj.max_version,
         'url': obj.url,
         'reason': obj.reason,
-        'include_in_legacy': obj.include_in_legacy,
+        'include_in_legacy': obj.in_legacy_blocklist,
         'comments': f'Versions {obj.min_version} - {obj.max_version} blocked.',
     }
     if submission_obj:
@@ -104,44 +107,33 @@ def legacy_publish_blocks(blocks):
     bucket = settings.REMOTE_SETTINGS_WRITER_BUCKET
     server = RemoteSettings(bucket, REMOTE_SETTINGS_COLLECTION_LEGACY)
     for block in blocks:
-        needs_updating = block.include_in_legacy and block.legacy_id
-        needs_creating = block.include_in_legacy and not block.legacy_id
-        needs_deleting = block.legacy_id and not block.include_in_legacy
+        needs_creating = not block.legacy_id
+
+        if block.is_imported_from_legacy_regex:
+            log.info(
+                f'Block [{block.guid}] was imported from a regex guid so '
+                'can\'t be safely updated.  Skipping.')
+            continue
+        data = {
+            'guid': block.guid,
+            'details': {
+                'bug': block.url,
+                'why': block.reason,
+                'name': str(block.reason).partition('.')[0],  # required
+            },
+            'enabled': True,
+            'versionRange': [{
+                'severity': 3,  # Always high severity now.
+                'minVersion': block.min_version,
+                'maxVersion': block.max_version,
+            }],
+        }
+        if needs_creating:
+            record = server.publish_record(data)
+            block.update(legacy_id=record.get('id', ''))
+        else:
+            server.publish_record(data, block.legacy_id)
 
-        if needs_updating or needs_creating:
-            if block.is_imported_from_legacy_regex:
-                log.info(
-                    f'Block [{block.guid}] was imported from a regex guid so '
-                    'can\'t be safely updated.  Skipping.')
-                continue
-            data = {
-                'guid': block.guid,
-                'details': {
-                    'bug': block.url,
-                    'why': block.reason,
-                    'name': str(block.reason).partition('.')[0],  # required
-                },
-                'enabled': True,
-                'versionRange': [{
-                    'severity': 3,  # Always high severity now.
-                    'minVersion': block.min_version,
-                    'maxVersion': block.max_version,
-                }],
-            }
-            if needs_creating:
-                record = server.publish_record(data)
-                block.update(legacy_id=record.get('id', ''))
-            else:
-                server.publish_record(data, block.legacy_id)
-        elif needs_deleting:
-            if block.is_imported_from_legacy_regex:
-                log.info(
-                    f'Block [{block.guid}] was imported from a regex guid so '
-                    'can\'t be safely deleted.  Skipping.')
-            else:
-                server.delete_record(block.legacy_id)
-            block.update(legacy_id='')
-        # else no existing legacy record and it shouldn't be in legacy so skip
     server.complete_session()
 
 
@@ -149,7 +141,7 @@ def legacy_delete_blocks(blocks):
     bucket = settings.REMOTE_SETTINGS_WRITER_BUCKET
     server = RemoteSettings(bucket, REMOTE_SETTINGS_COLLECTION_LEGACY)
     for block in blocks:
-        if block.legacy_id and block.include_in_legacy:
+        if block.legacy_id:
             if block.is_imported_from_legacy_regex:
                 log.info(
                     f'Block [{block.guid}] was imported from a regex guid so '