From 2f895832eaba73993204d74b213d6fda27bf72e2 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Wed, 28 Jun 2023 15:45:39 +0100 Subject: [PATCH] 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 | 11 ++- .../blocklist/includes/enhanced_blocks.html | 22 ++--- src/olympia/blocklist/tests/test_admin.py | 80 ++++++++++++++----- .../admin/blocklist_blocklistsubmission.css | 2 +- 4 files changed, 81 insertions(+), 34 deletions(-) diff --git a/src/olympia/blocklist/admin.py b/src/olympia/blocklist/admin.py index a0d83a7fc4..82d72d382a 100644 --- a/src/olympia/blocklist/admin.py +++ b/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, }, ) diff --git a/src/olympia/blocklist/templates/admin/blocklist/includes/enhanced_blocks.html b/src/olympia/blocklist/templates/admin/blocklist/includes/enhanced_blocks.html index 1ff389c510..e634fca274 100644 --- a/src/olympia/blocklist/templates/admin/blocklist/includes/enhanced_blocks.html +++ b/src/olympia/blocklist/templates/admin/blocklist/includes/enhanced_blocks.html @@ -25,20 +25,24 @@ diff --git a/src/olympia/blocklist/tests/test_admin.py b/src/olympia/blocklist/tests/test_admin.py index bf8192a316..a74f92c577 100644 --- a/src/olympia/blocklist/tests/test_admin.py +++ b/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' diff --git a/static/css/admin/blocklist_blocklistsubmission.css b/static/css/admin/blocklist_blocklistsubmission.css index c016a2dd95..2044834be6 100644 --- a/static/css/admin/blocklist_blocklistsubmission.css +++ b/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; }