infrastructure/extensions-website
4
8
Fork 10
Code Issues 20 Pull Requests 2 Projects Releases Wiki Activity

Validation Messages: Add <strong> for missing_wheel and forbidden_filepaths #200

Open
Dalai Felinto wants to merge 1 commits from pr-error-messages-safe into main
pull from: pr-error-messages-safe
merge into: infrastructure:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 0 Commits 1 Files Changed 2 +29 -14
2 changed files with 29 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 328c6ecbc1 - Show all commits

22
extensions/tests/test_submit.py
View File

@ -118,12 +118,14 @@ EXPECTED_VALIDATION_ERRORS = {
'source': ['The manifest file is missing.'],
},
'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']},
'invalid-theme-multiple-xmls.zip': {'source': ['Themes can only contain <strong>one XML file</strong>.']},
'invalid-theme-multiple-xmls-macosx.zip': {'source': ['Archive contains forbidden files or directories: __MACOSX/']},
'invalid-theme-multiple-xmls.zip': {
'source': ['Themes can only contain <strong>one XML file</strong>.']
},
'invalid-theme-multiple-xmls-macosx.zip': {
'source': ['Archive contains forbidden files or directories: <strong>__MACOSX/</strong>']
},
'invalid-missing-wheels.zip': {
'source': [
'Python wheel missing: addon/wheels/test-wheel-whatever.whl'
]
'source': ['Python wheel missing: <strong>addon/wheels/test-wheel-whatever.whl</strong>']
},
}
POST_DATA = {
@ -224,6 +226,16 @@ class SubmitFileTest(TestCase):
self.assertEqual(response.status_code, 200)
self.assertDictEqual(response.context['form'].errors, expected_errors)
def test_validation_errors_with_html(self):
self.assertEqual(Extension.objects.count(), 0)
user = UserFactory()
self.client.force_login(user)
with open(TEST_FILES_DIR / 'invalid-theme-multiple-xmls-macosx.zip', 'rb') as fp:
response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True})
self.assertNotContains(response, "&lt;", html=False)
def test_addon_without_top_level_directory(self):
self.assertEqual(Extension.objects.count(), 0)
user = UserFactory()

21
files/forms.py
View File

@ -4,6 +4,7 @@ import zipfile
import tempfile
from django import forms
from django.utils.html import escape
from django.utils.safestring import mark_safe
from django.utils.translation import gettext_lazy as _
@ -36,11 +37,15 @@ class FileForm(forms.ModelForm):
# TODO: surface TOML parsing errors?
'invalid_manifest_toml': _('Manifest file contains invalid code.'),
'invalid_missing_init': mark_safe(_('Add-on file missing: <strong>__init__.py</strong>.')),
'missing_or_multiple_theme_xml': mark_safe(_('Themes can only contain <strong>one XML file</strong>.')),
'missing_or_multiple_theme_xml': mark_safe(
_('Themes can only contain <strong>one XML file</strong>.')
),
'invalid_zip_archive': msg_only_zip_files,
'missing_manifest_toml': _('The manifest file is missing.'),
'missing_wheel': _('Python wheel missing: %(path)s'), # TODO <strong>%(path)s</strong>
'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), #TODO <strong>%(paths)s</strong>
'missing_wheel': _('Python wheel missing: <strong>%(path)s</strong>'),
'forbidden_filepaths': mark_safe(
Oleg-Komarov commented 2024-07-04 12:53:46 +02:00
Review
Copy Link

this line and the line above look inconsistent: both of them have parameters, but only one is wrapped into mark_safe inside the error_messages declaration
this is confusing and doesn't establish a consistent pattern for further use: if it's possible it's better to make them the same

it seems that the code below (in clean) already takes care of mark_safe for cases when params are passed, so I would expect that mark_safe in this line is not needed. if this is not true, then the code structure is surprising - we should think how to make it more transparent.

this line and the line above look inconsistent: both of them have parameters, but only one is wrapped into `mark_safe` inside the `error_messages` declaration this is confusing and doesn't establish a consistent pattern for further use: if it's possible it's better to make them the same it seems that the code below (in `clean`) already takes care of `mark_safe` for cases when params are passed, so I would expect that `mark_safe` in this line is not needed. if this is not true, then the code structure is surprising - we should think how to make it more transparent.
_('Archive contains forbidden files or directories: <strong>%(paths)s</strong>')
),
}
class Meta:
@ -145,12 +150,10 @@ class FileForm(forms.ModelForm):
manifest, error_codes = utils.read_manifest_from_zip(file_path)
for code in error_codes:
if isinstance(code, dict):
errors.append(
forms.ValidationError(
self.error_messages[code['code']],
params=code['params'],
)
)
error_message = str(self.error_messages[code['code']])
Oleg-Komarov commented 2024-07-04 13:02:04 +02:00
Review
Copy Link

why is str cast needed here?

why is `str` cast needed here?
sanitized_params = {k: escape(v) for k, v in code['params'].items()}
error_message = mark_safe(error_message % sanitized_params)
errors.append(forms.ValidationError(error_message))
else:
errors.append(forms.ValidationError(self.error_messages[code]))
if errors: