Display block type in reviewer tools & Use specific activity log for …

…soft-blocking versions (#22891)

* Display block type in reviewer tools & Use specific activity log for soft-blocking versions

src/olympia/blocklist/admin.py

@@ -17,7 +17,7 @@
     MultiAddForm,
     MultiDeleteForm,
 )
-from .models import Block, BlocklistSubmission, BlockVersion
+from .models import Block, BlocklistSubmission, BlockType, BlockVersion
 from .tasks import process_blocklistsubmission
 from .utils import splitlines
 
@@ -540,7 +540,9 @@ def block_history(self, obj):
             .filter(action__in=Block.ACTIVITY_IDS)
             .order_by('created')
         )
-        return render_to_string('admin/blocklist/includes/logs.html', {'logs': logs})
+        return render_to_string(
+            'admin/blocklist/includes/logs.html', {'BlockType': BlockType, 'logs': logs}
+        )
 
 
 @admin.register(Block)
@@ -597,6 +599,7 @@ def block_history(self, obj):
         return render_to_string(
             'admin/blocklist/includes/logs.html',
             {
+                'BlockType': BlockType,
                 'logs': logs,
                 'blocklistsubmission': submission,
                 'blocklistsubmission_changes': submission.get_changes_from_block(obj)

src/olympia/blocklist/templates/admin/blocklist/includes/logs.html

@@ -10,7 +10,8 @@
         {% if log.details %}{{ log.details.guid }}{% else %}{{ log.arguments.1 }}{% endif %}{% if 'min_version' in log.details %}
             , versions {{ log.details.min_version }} - {{ log.details.max_version }}.
         {% elif 'added_versions' in log.details %}
-        , versions {% if log.details.soft %}soft-{% else %}hard-{% endif %}blocked [{{ log.details.added_versions|join:', ' }}].
+          {# log.details.block_type was added for soft-blocks, if absent the block is a hard one #}
+        , versions {% if log.details.block_type == BlockType.SOFT_BLOCKED %}soft-{% else %}hard-{% endif %}blocked [{{ log.details.added_versions|join:', ' }}].
         {% elif 'removed_versions' in log.details %}
         , versions unblocked [{{ log.details.removed_versions|join:', ' }}].
         {% else %}.

src/olympia/blocklist/tests/test_admin.py

@@ -635,7 +635,7 @@ def test_add_single(self):
         assert block_log.details['blocked_versions'] == changed_version_strs
         assert block_log.details['added_versions'] == changed_version_strs
         assert block_log.details['reason'] == 'some reason'
-        assert not block_log.details['soft']
+        assert block_log.details['block_type'] == BlockType.BLOCKED
         assert block_log == (
             ActivityLog.objects.for_block(block).filter(action=block_log.action).get()
         )

src/olympia/blocklist/tests/test_utils.py

@@ -57,13 +57,14 @@ def test_metadata_updates(self):
     def test_log_entries_new_block(self):
         user_new = user_factory()
         addon = addon_factory()
+        version = addon.current_version
         submission = BlocklistSubmission.objects.create(
             input_guids=addon.guid,
             reason='some reason',
             url=None,
             updated_by=user_new,
             disable_addon=True,
-            changed_version_ids=[addon.current_version.pk],
+            changed_version_ids=[version.pk],
             signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED,
         )
         ActivityLog.objects.all().delete()
@@ -86,6 +87,7 @@ def test_log_entries_new_block(self):
         assert activity.details['comments'] == 'some reason'
         assert activity.details['is_addon_being_blocked']
         assert activity.details['is_addon_being_disabled']
+        assert activity.versionlog_set.all()[0].version == version
 
         activity = ActivityLog.objects.get(action=amo.LOG.BLOCKLIST_BLOCK_ADDED.id)
         assert activity.user == user_new
@@ -100,25 +102,26 @@ def test_log_entries_new_block(self):
             'guid': f'{addon.guid}',
             'reason': 'some reason',
             'signoff_state': 'Published',
-            'soft': False,
+            'block_type': BlockType.BLOCKED,
             'url': '',
         }
 
         activity = ActivityLog.objects.get(action=amo.LOG.BLOCKLIST_VERSION_BLOCKED.id)
         assert activity.user == user_new
-        assert not activity.details['soft']
+        assert activity.versionlog_set.all()[0].version == version
 
     def test_log_soft_block(self):
         user_new = user_factory()
         addon = addon_factory()
+        version = addon.current_version
         submission = BlocklistSubmission.objects.create(
             input_guids=addon.guid,
             reason='some reason',
             url=None,
             updated_by=user_new,
             disable_addon=True,
             block_type=BlockType.SOFT_BLOCKED,
-            changed_version_ids=[addon.current_version.pk],
+            changed_version_ids=[version.pk],
             signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED,
         )
         ActivityLog.objects.all().delete()
@@ -130,7 +133,7 @@ def test_log_soft_block(self):
             ActivityLog.objects.order_by('pk').values_list('action', flat=True)
         ) == [
             amo.LOG.BLOCKLIST_BLOCK_ADDED.id,
-            amo.LOG.BLOCKLIST_VERSION_BLOCKED.id,
+            amo.LOG.BLOCKLIST_VERSION_SOFT_BLOCKED.id,
             amo.LOG.CHANGE_STATUS.id,
             amo.LOG.REJECT_VERSION.id,
         ]
@@ -141,6 +144,7 @@ def test_log_soft_block(self):
         assert activity.details['comments'] == 'some reason'
         assert activity.details['is_addon_being_blocked']
         assert activity.details['is_addon_being_disabled']
+        assert activity.versionlog_set.all()[0].version == version
 
         activity = ActivityLog.objects.get(action=amo.LOG.BLOCKLIST_BLOCK_ADDED.id)
         assert activity.user == user_new
@@ -155,13 +159,15 @@ def test_log_soft_block(self):
             'guid': f'{addon.guid}',
             'reason': 'some reason',
             'signoff_state': 'Published',
-            'soft': True,
+            'block_type': BlockType.SOFT_BLOCKED,
             'url': '',
         }
 
-        activity = ActivityLog.objects.get(action=amo.LOG.BLOCKLIST_VERSION_BLOCKED.id)
+        activity = ActivityLog.objects.get(
+            action=amo.LOG.BLOCKLIST_VERSION_SOFT_BLOCKED.id
+        )
         assert activity.user == user_new
-        assert activity.details['soft']
+        assert activity.versionlog_set.all()[0].version == version
 
     def test_no_empty_new_blocks(self):
         user_new = user_factory()

src/olympia/blocklist/utils.py

@@ -18,6 +18,7 @@ def block_activity_log_save(
     from .models import BlockType
 
     action = amo.LOG.BLOCKLIST_BLOCK_EDITED if change else amo.LOG.BLOCKLIST_BLOCK_ADDED
+    action_version = amo.LOG.BLOCKLIST_VERSION_BLOCKED
     addon_versions = {ver.id: ver.version for ver in obj.addon_versions}
     blocked_versions = sorted(
         ver.version for ver in obj.addon_versions if ver.is_blocked
@@ -38,23 +39,22 @@ def block_activity_log_save(
         'comments': f'{len(changed_versions)} versions added to block; '
         f'{len(blocked_versions)} total versions now blocked.',
     }
-    version_details = {}
     if submission_obj:
         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'] = (
-            submission_obj.block_type == BlockType.SOFT_BLOCKED
-        )
+        details['block_type'] = submission_obj.block_type
+        if submission_obj.block_type == BlockType.SOFT_BLOCKED:
+            action_version = amo.LOG.BLOCKLIST_VERSION_SOFT_BLOCKED
 
     log_create(action, obj.addon, obj.guid, obj, details=details, user=obj.updated_by)
     log_create(
-        amo.LOG.BLOCKLIST_VERSION_BLOCKED,
+        action_version,
         *((Version, version_id) for version_id in changed_version_ids),
         obj,
-        **{'details': version_details} if version_details else {},
+        details={},
         user=obj.updated_by,
     )
 

src/olympia/constants/activity.py

@@ -1101,6 +1101,15 @@ class HELD_ACTION_FORCE_DISABLE(_LOG):
     admin_event = True
 
 
+class BLOCKLIST_VERSION_SOFT_BLOCKED(_LOG):
+    id = 197
+    keep = True
+    action_class = 'add'
+    hide_developer = True
+    format = '{version} added to Soft Blocklist.'
+    short = 'Version Soft Blocked'
+
+
 LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG]
 # Make sure there's no duplicate IDs.
 assert len(LOGS) == len({log.id for log in LOGS})

src/olympia/reviewers/templates/reviewers/includes/version.html

@@ -21,7 +21,7 @@
     {% endif %}
 
     {% if version.is_blocked %}
-    <span class="blocked-version">Blocked</span>
+    <span class="blocked-version">{{ version.blockversion.get_block_type_display() }}</span>
     {% endif %}
   </th>
   <td class="due_date">

src/olympia/reviewers/tests/test_views.py

@@ -57,7 +57,7 @@
     version_factory,
     version_review_flags_factory,
 )
-from olympia.blocklist.models import Block, BlocklistSubmission, BlockVersion
+from olympia.blocklist.models import Block, BlocklistSubmission, BlockType, BlockVersion
 from olympia.blocklist.utils import block_activity_log_save
 from olympia.constants.abuse import DECISION_ACTIONS
 from olympia.constants.promoted import LINE, NOTABLE, RECOMMENDED, SPOTLIGHT, STRATEGIC
@@ -5561,15 +5561,17 @@ def test_blocked_versions(self):
         response = self.client.get(self.url)
         assert b'Blocked' in response.content
         span = pq(response.content)('#versions-history .blocked-version')
-        assert span.text() == 'Blocked'
+        assert span.text() == '🛑 Hard-Blocked'
         assert span.length == 1  # addon only has 1 version
 
         blockversion = BlockVersion.objects.create(
-            block=block, version=version_factory(addon=self.addon, version='99')
+            block=block,
+            version=version_factory(addon=self.addon, version='99'),
+            block_type=BlockType.SOFT_BLOCKED,
         )
         response = self.client.get(self.url)
         span = pq(response.content)('#versions-history .blocked-version')
-        assert span.text() == 'Blocked Blocked'
+        assert span.text() == '🛑 Hard-Blocked ⚠️ Soft-Blocked'
         assert span.length == 2  # a new version is blocked too
 
         block_reason = 'Very bad addon!'
@@ -5578,7 +5580,7 @@ def test_blocked_versions(self):
         block_activity_log_save(obj=block, change=False)
         response = self.client.get(self.url)
         span = pq(response.content)('#versions-history .blocked-version')
-        assert span.text() == 'Blocked'
+        assert span.text() == '🛑 Hard-Blocked'
         assert span.length == 1
         assert 'Version Blocked' in (
             pq(response.content)('#versions-history .activity').text()