mozilla
/
addons-server
зеркало из https://github.com/mozilla/addons-server.git
1
0
Форкнуть 0
Код Задачи Пакеты Проекты Релизы Вики Активность

define if a blocklistsubmission updaates metadata (#20907)

This commit is contained in:
Andrew Williamson 2023-06-30 14:56:03 +01:00 коммит произвёл GitHub
Родитель 61f7fdb1e5
Коммит 8fce06e757
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
10 изменённых файлов: 306 добавлений и 41 удалений
Показать статистику Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

48
src/olympia/blocklist/forms.py
Просмотреть файл

@ -2,7 +2,6 @@ from datetime import datetime, timedelta
from django import forms
from django.core.exceptions import ValidationError
from django.utils.translation import gettext_lazy as _
from olympia.amo.admin import HTML5DateTimeInput
from olympia.amo.forms import AMOModelForm
@ -32,9 +31,7 @@ class MultiDeleteForm(MultiGUIDInputForm):
missing_guids = (guid for guid in guids if guid not in matching)
errors = [
ValidationError(
_('Block with GUID %(guid)s not found'), params={'guid': guid}
)
ValidationError(f'Block with GUID {guid} not found')
for guid in missing_guids
]
@ -46,20 +43,11 @@ class MultiAddForm(MultiGUIDInputForm):
def clean(self):
guids = splitlines(self.cleaned_data.get('guids'))
errors = []
if len(guids) == 1:
guid = guids[0]
blk = self.existing_block = Block.objects.filter(guid=guid).first()
if not blk and not Block.get_addons_for_guids_qs((guid,)).exists():
errors.append(
ValidationError(
_('Add-on with GUID %(guid)s does not exist'),
params={'guid': guid},
)
)
if errors:
raise ValidationError(errors)
raise ValidationError(f'Add-on with GUID {guid} does not exist')
def _get_version_choices(blocks, ver_filter=lambda v: True):
@ -89,7 +77,11 @@ class BlocklistSubmissionForm(AMOModelForm):
)
# Note we don't render the widget - we manually create the checkboxes in
# enhanced_blocks.html
changed_version_ids = forms.fields.TypedMultipleChoiceField(choices=(), coerce=int)
changed_version_ids = forms.fields.TypedMultipleChoiceField(
choices=(), coerce=int, required=False
)
update_reason = forms.fields.BooleanField(required=False, initial=True)
update_url = forms.fields.BooleanField(required=False, initial=True)
def __init__(self, data=None, *args, **kw):
instance = kw.get('instance')
@ -122,6 +114,7 @@ class BlocklistSubmissionForm(AMOModelForm):
filter_existing=is_add_change,
)
objects['total_adu'] = sum(block.current_adu for block in objects['blocks'])
self.initial = self.initial or {}
if changed_version_ids_field := self.fields.get('changed_version_ids'):
changed_version_ids_field.choices = _get_version_choices(
@ -139,10 +132,23 @@ class BlocklistSubmissionForm(AMOModelForm):
]
if not data and 'changed_version_ids' not in (self.initial or {}):
# preselect all the options
self.initial = {
**(self.initial or {}),
'changed_version_ids': self.changed_version_ids_choices,
}
self.initial[
'changed_version_ids'
] = self.changed_version_ids_choices
for field_name in ('reason', 'url'):
values = {
getattr(block, field_name, '')
for block in objects['blocks']
if block.id
}
update_field_name = f'update_{field_name}'
if len(values) == 1 and (value := tuple(values)[0]):
# if there's just one existing value, prefill the field
self.initial[field_name] = value
elif len(values) > 1:
# If the field has multiple existing values, default to not changing
self.initial[update_field_name] = False
for key, value in objects.items():
setattr(self, key, value)
elif instance:
@ -160,3 +166,7 @@ class BlocklistSubmissionForm(AMOModelForm):
data = self.cleaned_data
if delay_days := data.get('delay_days', 0):
data['delayed_until'] = datetime.now() + timedelta(days=delay_days)
if not data.get('update_reason'):
data['reason'] = None
if not data.get('update_url'):
data['url'] = None

31
src/olympia/blocklist/migrations/0032_alter_blocklistsubmission_reason_url_null_true.py Normal file
Просмотреть файл

@ -0,0 +1,31 @@
# Generated by Django 4.2.2 on 2023-06-29 12:45
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('blocklist', '0031_alter_blocklistsubmission_block_add_changed_versions'),
]
operations = [
migrations.AlterField(
model_name='blocklistsubmission',
name='reason',
field=models.TextField(
blank=True,
help_text='Note this reason will be displayed publicly on the block-addon pages.',
null=True,
),
),
migrations.AlterField(
model_name='blocklistsubmission',
name='url',
field=models.CharField(
blank=True,
help_text='The URL related to this block, i.e. the bug filed.',
max_length=255,
null=True,
),
),
]

16
src/olympia/blocklist/models.py
Просмотреть файл

@ -234,10 +234,12 @@ class BlocklistSubmission(ModelBase):
url = models.CharField(
max_length=255,
blank=True,
null=True,
help_text='The URL related to this block, i.e. the bug filed.',
)
reason = models.TextField(
blank=True,
null=True,
help_text='Note this reason will be displayed publicly on the block-addon '
'pages.',
)
@ -495,15 +497,10 @@ class BlocklistSubmission(ModelBase):
assert self.is_submission_ready
assert self.action == self.ACTION_ADDCHANGE
fields_to_set = [
'url',
'reason',
'updated_by',
]
all_guids_to_block = [block['guid'] for block in self.to_block]
for guids_chunk in chunked(all_guids_to_block, 100):
save_versions_to_blocks(guids_chunk, self, fields_to_set=fields_to_set)
save_versions_to_blocks(guids_chunk, self)
self.save()
self.update(signoff_state=self.SIGNOFF_PUBLISHED)
@ -512,16 +509,11 @@ class BlocklistSubmission(ModelBase):
assert self.is_submission_ready
assert self.action == self.ACTION_DELETE
fields_to_set = [
'url',
'reason',
'updated_by',
]
all_guids_to_block = [block['guid'] for block in self.to_block]
for guids_chunk in chunked(all_guids_to_block, 100):
# This function will remove BlockVersions and delete the Block if empty
delete_versions_from_blocks(guids_chunk, self, fields_to_set=fields_to_set)
delete_versions_from_blocks(guids_chunk, self)
self.save()
self.update(signoff_state=self.SIGNOFF_PUBLISHED)

5
src/olympia/blocklist/templates/admin/blocklist/blocklistsubmission_add_form.html
Просмотреть файл

@ -3,6 +3,7 @@
{% block extrahead %}{{ block.super }}
<script type="text/javascript" src="{% static 'js/i18n/en-US.js' %}"></script>
<script type="text/javascript" src="{% static 'js/admin/blocklist_blocklistsubmission.js' %}"></script>
{{ media }}
{% endblock %}
@ -77,12 +78,16 @@
<p class="help">{{ form.disable_addon.help_text }}</p>
</div>
<div class="form-row">
{{ form.update_url }}
{{ form.update_url.errors }}
{{ form.url.errors }}
{{ form.url.label_tag }}
{{ form.url }}
<p class="help">{{ form.url.help_text }}</p>
</div>
<div class="form-row">
{{ form.update_reason }}
{{ form.update_reason.errors }}
{{ form.reason.errors }}
{{ form.reason.label_tag }}
{{ form.reason }}

102
src/olympia/blocklist/tests/test_admin.py
Просмотреть файл

@ -367,6 +367,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': changed_version_ids,
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'_save': 'Save',
},
follow=True,
@ -529,6 +531,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'disable_addon': True,
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'delay_days': delay,
'_save': 'Save',
},
@ -733,6 +737,78 @@ class TestBlocklistSubmissionAdmin(TestCase):
new_addon, existing_and_full, partial_addon, existing_and_partial
)
def test_submit_no_metadata_updates(self):
user = user_factory(email='someone@mozilla.com')
self.grant_permission(user, 'Blocklist:Create')
self.client.force_login(user)
addon_adu = settings.DUAL_SIGNOFF_AVERAGE_DAILY_USERS_THRESHOLD
new_addon = addon_factory(
guid='any@new', name='New Danger', average_daily_users=addon_adu
)
partial_addon_adu = addon_adu - 1
partial_addon = addon_factory(
guid='partial@existing',
name='Partial Danger',
average_daily_users=(partial_addon_adu),
)
existing_and_partial = block_factory(
guid=partial_addon.guid,
# should be updated to addon's adu
average_daily_users_snapshot=146722437,
updated_by=user_factory(),
reason='partial reason',
url='partial url',
)
version_factory(addon=partial_addon)
existing_and_complete = block_factory(
addon=addon_factory(guid='full@existing', name='Full Danger'),
# addon will have a different adu
average_daily_users_snapshot=346733434,
updated_by=user_factory(),
reason='full reason',
url='full url',
)
# Create the block submission
response = self.client.post(
self.submission_url,
{
'input_guids': ('any@new\npartial@existing\nfull@existing\ninvalid@'),
'action': str(BlocklistSubmission.ACTION_ADDCHANGE),
'changed_version_ids': [
new_addon.current_version.id,
partial_addon.current_version.id,
],
'disable_addon': True,
'url': 'new url that will be ignored because no update_url=True',
'reason': 'new reason',
# no 'update_url'
'update_reason': True,
'_save': 'Save',
},
follow=True,
)
assert response.status_code == 200
assert BlocklistSubmission.objects.count() == 1
submission = BlocklistSubmission.objects.get()
assert submission.reason == 'new reason'
assert submission.url is None
assert Block.objects.count() == 3
new_block = Block.objects.exclude(
id__in=[existing_and_complete.id, existing_and_partial.id]
).get()
existing_and_complete.reload()
existing_and_partial.reload()
assert existing_and_complete.reason == 'full reason' # not affected at all
assert existing_and_partial.reason == 'new reason'
assert new_block.reason == 'new reason'
assert existing_and_complete.url == 'full url' # not affected at all
assert existing_and_partial.url == 'partial url' # .url is None
assert new_block.url == ''
@mock.patch('olympia.blocklist.forms.GUID_FULL_LOAD_LIMIT', 1)
def test_add_multiple_bulk_so_fake_block_objects(self):
user = user_factory(email='someone@mozilla.com')
@ -868,6 +944,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'action': str(BlocklistSubmission.ACTION_ADDCHANGE),
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'_save': 'Save',
},
follow=True,
@ -990,6 +1068,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'url': 'new.url',
# disable_addon defaults to True, so omitting it is changing to False
'reason': 'a new reason thats longer than 40 charactors',
'update_url': True,
'update_reason': True,
'_save': 'Update',
},
follow=True,
@ -1068,6 +1148,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [],
'url': 'new.url',
'reason': 'a reason',
'update_url': True,
'update_reason': True,
'_save': 'Update',
},
follow=True,
@ -1116,6 +1198,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [], # should be ignored
'url': 'new.url', # should be ignored
'reason': 'a reason', # should be ignored
'update_url': True,
'update_reason': True,
'_approve': 'Approve Submission',
},
follow=True,
@ -1234,6 +1318,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'versions': [], # should be ignored
'url': 'new.url', # should be ignored
'reason': 'a reason', # should be ignored
'update_url': True,
'update_reason': True,
'_reject': 'Reject Submission',
},
follow=True,
@ -1305,6 +1391,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [], # should be ignored
'url': 'new.url', # could be updated with this permission
'reason': 'a reason', # could be updated with this permission
'update_url': True,
'update_reason': True,
'_approve': 'Approve Submission',
},
follow=True,
@ -1344,6 +1432,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [], # should be ignored
'url': 'new.url', # could be updated with this permission
'reason': 'a reason', # could be updated with this permission
'update_url': True,
'update_reason': True,
'_reject': 'Reject Submission',
},
follow=True,
@ -1375,6 +1465,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [], # should be ignored
'url': 'new.url', # could be updated with this permission
'reason': 'a reason', # could be updated with this permission
'update_url': True,
'update_reason': True,
'_reject': 'Reject Submission',
},
follow=True,
@ -1536,6 +1628,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'disable_addon': True,
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'_save': 'Save',
},
follow=True,
@ -1580,6 +1674,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [version.id],
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'_save': 'Save',
},
follow=True,
@ -1690,6 +1786,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
'changed_version_ids': [], # should be ignored
'url': 'new.url', # should be ignored
'reason': 'a reason', # should be ignored
'update_url': True,
'update_reason': True,
'_reject': 'Reject Submission',
},
follow=True,
@ -1825,6 +1923,8 @@ class TestBlocklistSubmissionAdmin(TestCase):
# 'disable_addon' it's a checkbox so leaving it out is False
'url': 'dfd',
'reason': 'some reason',
'update_url': True,
'update_reason': True,
'delay_days': 0,
'_save': 'Save',
},
@ -1922,7 +2022,7 @@ class TestBlockAdminDelete(TestCase):
assert f'{block_one_ver.addon.average_daily_users} users' in content
assert '{12345-6789}' in content
# The fields only used for Add/Change submissions shouldn't be shown
assert 'reason' not in content
assert 'id_reason' not in content
# Check we didn't delete the blocks already
assert Block.objects.count() == 3
assert BlocklistSubmission.objects.count() == 0

56
src/olympia/blocklist/tests/test_forms.py
Просмотреть файл

@ -163,6 +163,62 @@ class TestBlocklistSubmissionForm(TestCase):
]
}
def test_initial_reason_and_url_values(self):
block_admin = BlocklistSubmissionAdmin(
model=BlocklistSubmission, admin_site=admin_site
)
request = RequestFactory().get('/')
Form = block_admin.get_form(request=request)
data = {
'input_guids': f'{self.new_addon.guid}\n'
f'{self.existing_block_full.guid}\n'
f'{self.existing_block_partial.guid}\n'
'invalid@guid',
}
self.existing_block_partial.update(reason='partial reason')
self.existing_block_full.update(reason='full reason')
self.existing_block_partial.update(url='url')
self.existing_block_full.update(url='url')
form = Form(initial=data)
# when we just have a single existing block we default to the existing values
# (existing_block_full is ignored entirely because won't be updated)
assert form.initial['reason'] == 'partial reason'
assert form.initial['url'] == 'url'
assert 'update_url' not in form.initial
assert 'update_reason' not in form.initial
# lets make existing_block_full not fully blocked
self.full_existing_addon_v2.blockversion.delete()
form = Form(initial=data)
assert 'reason' not in form.initial # two values so not default
assert form.initial['url'] == 'url' # both the same, so we can default
assert 'update_url' not in form.initial
assert form.initial['update_reason'] is False # so the checkbox defaults false
def test_update_url_reason_sets_null(self):
block_admin = BlocklistSubmissionAdmin(
model=BlocklistSubmission, admin_site=admin_site
)
request = RequestFactory().get('/')
Form = block_admin.get_form(request=request)
data = {
'input_guids': f'{self.new_addon.guid}\n'
f'{self.existing_block_full.guid}\n'
f'{self.existing_block_partial.guid}\n'
'invalid@guid',
'url': 'new url',
'update_url': True,
'reason': 'new reason',
# no update_url
}
form = Form(data=data)
form.is_valid()
assert form.cleaned_data['url'] == 'new url'
assert form.cleaned_data['reason'] is None
class TestMultiDeleteForm(TestCase):
def test_guids_must_exist_for_block_deletion(self):

30
src/olympia/blocklist/tests/test_utils.py
Просмотреть файл

@ -1,8 +1,36 @@
from datetime import datetime
from olympia.blocklist.utils import datetime_to_ts
from django.conf import settings
import pytest
from olympia.amo.tests import addon_factory, user_factory
from ..models import Block, BlocklistSubmission
from ..utils import datetime_to_ts, save_versions_to_blocks
def test_datetime_to_ts():
now = datetime.now()
assert datetime_to_ts(now) == int(now.timestamp() * 1000)
@pytest.mark.django_db
def test_save_versions_to_blocks_metadata_updates():
user_old = user_factory(pk=settings.TASK_USER_ID)
user_new = user_factory()
addon = addon_factory()
existing_block = Block.objects.create(
guid=addon.guid,
updated_by=user_old,
reason='old reason',
url='old url',
)
submission = BlocklistSubmission.objects.create(
input_guids=addon.guid, reason='new reason', url=None, updated_by=user_new
)
save_versions_to_blocks([addon.guid], submission)
existing_block.reload()
assert existing_block.reason == 'new reason'
assert existing_block.url == 'old url'

20
src/olympia/blocklist/utils.py
Просмотреть файл

@ -138,12 +138,11 @@ def disable_versions_for_block(block, submission):
review.clear_specific_needs_human_review_flags(version)
def save_versions_to_blocks(guids, submission, *, fields_to_set):
def save_versions_to_blocks(guids, submission):
from olympia.addons.models import GuidAlreadyDeniedError
from .models import Block, BlockVersion
common_args = {field: getattr(submission, field) for field in fields_to_set}
modified_datetime = datetime.now()
blocks = Block.get_blocks_from_guids(guids)
@ -151,8 +150,11 @@ def save_versions_to_blocks(guids, submission, *, fields_to_set):
change = bool(block.id)
if change:
setattr(block, 'modified', modified_datetime)
for field, val in common_args.items():
setattr(block, field, val)
block.updated_by = submission.updated_by
if submission.reason is not None:
block.reason = submission.reason
if submission.url is not None:
block.url = submission.url
block.average_daily_users_snapshot = block.current_adu
block.save()
# And now update the BlockVersion instances - instances to add first
@ -186,10 +188,9 @@ def save_versions_to_blocks(guids, submission, *, fields_to_set):
return blocks
def delete_versions_from_blocks(guids, submission, *, fields_to_set):
def delete_versions_from_blocks(guids, submission):
from .models import Block, BlockVersion
common_args = {field: getattr(submission, field) for field in fields_to_set}
modified_datetime = datetime.now()
blocks = Block.get_blocks_from_guids(guids)
@ -204,8 +205,11 @@ def delete_versions_from_blocks(guids, submission, *, fields_to_set):
if BlockVersion.objects.filter(block=block).exists():
# if there are still other versions blocked update the metadata
for field, val in common_args.items():
setattr(block, field, val)
block.updated_by = submission.updated_by
if submission.reason is not None:
block.reason = submission.reason
if submission.url is not None:
block.url = submission.url
block.average_daily_users_snapshot = block.current_adu
block.save()
should_delete = False

9
static/css/admin/blocklist_blocklistsubmission.css
Просмотреть файл

@ -46,3 +46,12 @@ form ul.guid_list .addon-name {
margin-left: 0 !important;
margin-right: 0;
}
#id_reason:disabled,
#id_url:disabled {
background-color:#eeeeee;
}
label[for="id_reason"],
label[for="id_url"] {
display: inline-block;
}

30
static/js/admin/blocklist_blocklistsubmission.js Normal file
Просмотреть файл

@ -0,0 +1,30 @@
document.addEventListener('DOMContentLoaded', () => {
'use strict';
const enableIfChecked = (isChecked, fieldId) => {
const field = document.querySelector(`#${fieldId}`);
if (isChecked) {
field.removeAttribute('disabled');
} else {
field.setAttribute('disabled', true);
}
};
document
.querySelector('#id_update_reason')
.addEventListener('click', (e) =>
enableIfChecked(e.target.checked, 'id_reason'),
);
document
.querySelector('#id_update_url')
.addEventListener('click', (e) =>
enableIfChecked(e.target.checked, 'id_url'),
);
enableIfChecked(
document.querySelector('#id_update_reason').checked,
'id_reason',
);
enableIfChecked(document.querySelector('#id_update_url').checked, 'id_url');
});