change version checkboxes to make it clearer between blocking and already blocked (#20887)

* change version checkboxes to make it clearer between blocking and already blocked

* wrap blocked and not blocked icon & version string in a span, with title

src/olympia/blocklist/admin.py

@@ -259,13 +259,14 @@ class BlocklistSubmissionAdmin(AMOModelAdmin):
         else:
             edit_title = 'Proposed New Blocks'
 
+        changed_version_ids = ('changed_version_ids',) if not obj else ()
         add_change = (
             edit_title,
             {
                 'fields': (
                     'blocks',
                     'disable_addon',
-                    'changed_version_ids',
+                    *changed_version_ids,
                     'url',
                     'reason',
                     'updated_by',
@@ -403,6 +404,7 @@ class BlocklistSubmissionAdmin(AMOModelAdmin):
             'site_title': None,
             'is_popup': False,
             'form_url': '',
+            'is_delete': is_delete,
         }
         return TemplateResponse(
             request, 'admin/blocklist/blocklistsubmission_add_form.html', context
@@ -432,9 +434,8 @@ class BlocklistSubmissionAdmin(AMOModelAdmin):
     def render_change_form(
         self, request, context, add=False, change=False, form_url='', obj=None
     ):
-        if change:
-            # add this to the instance so blocks() below can reference it.
-            obj._blocks = context['adminform'].form.blocks
+        # add this to the instance so blocks() below can reference it.
+        obj._blocks = context['adminform'].form.blocks
         return super().render_change_form(
             request, context, add=add, change=change, form_url=form_url, obj=obj
         )
@@ -507,6 +508,8 @@ class BlocklistSubmissionAdmin(AMOModelAdmin):
             {
                 'form': SimpleNamespace(blocks=obj._blocks),
                 'submission_published': is_published,
+                'instance': obj,
+                'is_delete': obj.action == BlocklistSubmission.ACTION_DELETE,
             },
         )
 

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

@@ -25,20 +25,24 @@
       <ul>
         {% for version in block_obj.addon_versions %}
           <li data-version-id="{{ version.id }}">
+          {% if version.id in form.changed_version_ids_choices or version.id in instance.changed_version_ids %}
             <label><input
               type="checkbox"
               name="changed_version_ids"
               value="{{ version.id }}"
-              {% if version.id in form.changed_version_ids_choices %}
-                {% if version.id in form.changed_version_ids.value %}checked{% endif %}
-              {% else %}
-                disabled
-                {% if version.is_blocked %}checked{% endif %}
+              {% if version.id in form.changed_version_ids.value %}checked{% endif %}
+              {% if version.id in instance.changed_version_ids %}checked disabled{% endif %}
+            > {% if is_delete %}Unblock{% else %}Block{% endif %} {{ version.version }}</label>
+          {% else %}
+            <span title="{% if version.is_blocked %}Blocked{% else %}Not blocked{% endif %}">
+              <!-- Red Hexagonal stop sign for Blocked; Green cirle for not blocked -->
+              {% if version.is_blocked %}&#x1F6D1;{% else %}&#x1F7E2;{% endif %}{{ version.version }}
+              {% if version.blocklist_submission_id %}
+                [<a href="{% url 'admin:blocklist_blocklistsubmission_change' version.blocklist_submission_id %}">Edit Submission</a>]
               {% endif %}
-            >{{ version.version }}</label>
-            {% if version.blocklist_submission_id %}
-              [<a href="{% url 'admin:blocklist_blocklistsubmission_change' version.blocklist_submission_id %}">Edit Submission</a>]
-            {% endif %}
+            </span>
+
+          {% endif %}
           </li>
         {% endfor %}
       </ul>

src/olympia/blocklist/tests/test_admin.py

@@ -263,18 +263,18 @@ class TestBlocklistSubmissionAdmin(TestCase):
         doc = pq(response.content)
         checkboxes = doc('input[name=changed_version_ids]')
 
-        assert len(checkboxes) == 5
-        # pre-checked because available to block
+        assert len(checkboxes) == 3
         check_checkbox(checkboxes[0], ver, True, False)
-        # pre-checked because available to block
         check_checkbox(checkboxes[1], ver_deleted, True, False)
-        # not checked because not blocked currently, but disabled because in submission
-        check_checkbox(checkboxes[2], ver_add_subm, False, True)
-        # pre-checked because available to block
-        check_checkbox(checkboxes[3], ver_other, True, False)
-        # checked and disabled because blocked already, and this is an add action
-        check_checkbox(checkboxes[4], ver_block, True, True)
+        check_checkbox(checkboxes[2], ver_other, True, False)
 
+        # not a checkbox because in a submission, green circle because not blocked
+        assert doc(f'li[data-version-id="{ver_add_subm.id}"]').text() == (
+            f'\U0001F7E2{ver_add_subm.version} [Edit Submission]'
+        )
+        assert doc(f'li[data-version-id="{ver_add_subm.id}"] span').attr('title') == (
+            'Not blocked'
+        )
         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(
@@ -282,6 +282,31 @@ class TestBlocklistSubmissionAdmin(TestCase):
             args=(add_submission.id,),
         )
 
+        # not a checkbox because blocked already and this is an add action
+        assert doc(f'li[data-version-id="{ver_block.id}"]').text() == (
+            f'\U0001F6D1{ver_block.version}'
+        )
+        assert doc(f'li[data-version-id="{ver_block.id}"] span').attr('title') == (
+            'Blocked'
+        )
+
+        # Now with an existing submission
+        submission = BlocklistSubmission.objects.create(
+            input_guids=f'{addon.guid}\n {ver_block.addon.guid}\n',
+            changed_version_ids=[ver_deleted.id, ver_other.id],
+        )
+        response = self.client.get(
+            reverse(
+                'admin:blocklist_blocklistsubmission_change', args=(submission.id,)
+            ),
+            {'guids': f'{addon.guid}\n {ver_block.addon.guid}\n'},
+        )
+        doc = pq(response.content)
+        checkboxes = doc('input[name=changed_version_ids]')
+        assert len(checkboxes) == 2
+        check_checkbox(checkboxes[0], ver_deleted, True, True)
+        check_checkbox(checkboxes[1], ver_other, True, True)
+
     def test_add_single(self):
         user = user_factory(email='someone@mozilla.com')
         self.grant_permission(user, 'Blocklist:Create')
@@ -2031,17 +2056,32 @@ class TestBlockAdminDelete(TestCase):
         doc = pq(response.content)
         checkboxes = doc('input[name=changed_version_ids]')
 
-        assert len(checkboxes) == 5
-        # not checked, and disabled, because not blocked currently
-        check_checkbox(checkboxes[0], ver, False, True)
-        # not checked because not blocked currently, but disabled because in submission
-        check_checkbox(checkboxes[1], ver_add_subm, False, True)
-        # checked because blocked, but disabled because in a submission
-        check_checkbox(checkboxes[2], ver_del_subm, True, True)
-        # not checked, and disabled, because not blocked currently
-        check_checkbox(checkboxes[3], ver_deleted, False, True)
-        # pre-checked because blocked, and this is a delete action
-        check_checkbox(checkboxes[4], ver_block, True, False)
+        assert len(checkboxes) == 1
+
+        check_checkbox(checkboxes[0], ver_block, True, False)
+
+        # not a checkbox because in a submission, green circle because not blocked
+        assert doc(f'li[data-version-id="{ver_add_subm.id}"]').text() == (
+            f'\U0001F7E2{ver_add_subm.version} [Edit Submission]'
+        )
+        assert doc(f'li[data-version-id="{ver_add_subm.id}"] span').attr('title') == (
+            'Not blocked'
+        )
+        # not a checkbox because in a submission, red hexagon because not blocked
+        assert doc(f'li[data-version-id="{ver_del_subm.id}"]').text() == (
+            f'\U0001F6D1{ver_del_subm.version} [Edit Submission]'
+        )
+        assert doc(f'li[data-version-id="{ver_del_subm.id}"] span').attr('title') == (
+            'Blocked'
+        )
+        # not a checkbox because not blocked, and this is a delete action
+        assert doc(f'li[data-version-id="{ver.id}"]').text() == (
+            f'\U0001F7E2{ver.version}'
+        )
+        # not a checkbox because not blocked, and this is a delete action
+        assert doc(f'li[data-version-id="{ver_deleted.id}"]').text() == (
+            f'\U0001F7E2{ver_deleted.version}'
+        )
 
         submission_link = doc(f'li[data-version-id="{ver_add_subm.id}"] a')
         assert submission_link.text() == 'Edit Submission'

static/css/admin/blocklist_blocklistsubmission.css

@@ -17,7 +17,7 @@ form .guid_list li ul {
     margin: 0;
 }
 
-form ul.guid_list li {
+form ul.guid_list >li {
     list-style-type: disc;
 }