From 8aa39660cd8f7bca60876aa2585c75f175b6ef22 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 16 Feb 2021 15:57:18 +0000 Subject: [PATCH] don't 500 on empty/bad addon_count value in admin form; show form error (#16552) --- src/olympia/discovery/tests/test_admin.py | 46 ++++++++++++++++ .../migrations/0011_auto_20210215_1216.py | 18 ++++++ src/olympia/shelves/models.py | 1 - src/olympia/shelves/tests/test_forms.py | 55 ++++++++++++++++++- 4 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 src/olympia/shelves/migrations/0011_auto_20210215_1216.py diff --git a/src/olympia/discovery/tests/test_admin.py b/src/olympia/discovery/tests/test_admin.py index cddda6b368..ea7207db9a 100644 --- a/src/olympia/discovery/tests/test_admin.py +++ b/src/olympia/discovery/tests/test_admin.py @@ -913,6 +913,49 @@ class TestShelfAdmin(TestCase): assert item.criteria == ('?promoted=recommended&sort=random&type=extension') assert item.addon_count == item.get_count() == 2 + def test_blank_or_nonnumber_addon_count_errors(self): + item = Shelf.objects.create( + title='Popular extensions', + endpoint='search', + criteria='?sort=users&type=extension', + footer_text='See more', + footer_pathname='/this/is/the/pathname', + ) + detail_url = reverse(self.detail_url_name, args=(item.pk,)) + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Discovery:Edit') + self.client.login(email=user.email) + data = { + 'title': 'Recommended extensions', + 'endpoint': 'search', + 'criteria': ('?promoted=recommended&sort=random&type=extension'), + 'footer_text': 'See more', + 'footer_pathname': '/this/is/the/pathname', + # addon_count is missing + } + response = self.client.post(detail_url, data, follow=False) + self.assertFormError( + response, 'adminform', 'addon_count', 'This field is required.' + ) + # as an empty string + data['addon_count'] = '' + response = self.client.post(detail_url, data, follow=False) + self.assertFormError( + response, 'adminform', 'addon_count', 'This field is required.' + ) + # without a valid number + data['addon_count'] = 'aa' + response = self.client.post(detail_url, data, follow=False) + self.assertFormError( + response, 'adminform', 'addon_count', 'Enter a whole number.' + ) + # and finally with a valid number + data['addon_count'] = '1' + response = self.client.post(detail_url, data, follow=False) + assert response.status_code == 302, response.content + item.reload() + assert item.addon_count == 1 + def test_can_delete_with_discovery_edit_permission(self): item = Shelf.objects.create( title='Recommended extensions', @@ -951,6 +994,7 @@ class TestShelfAdmin(TestCase): 'criteria': ('?promoted=recommended&sort=random&type=extension'), 'footer_text': 'See more', 'footer_pathname': '/this/is/the/pathname', + 'addon_count': '0', } response = self.client.post(add_url, mock_clean.return_value, follow=True) @@ -976,6 +1020,7 @@ class TestShelfAdmin(TestCase): 'criteria': '?promoted=recommended&sort=random&type=extension', 'footer_text': 'See more', 'footer_pathname': '/this/is/the/pathname', + 'addon_count': '0', }, follow=True, ) @@ -1004,6 +1049,7 @@ class TestShelfAdmin(TestCase): 'criteria': '?promoted=recommended&sort=users&type=extension', 'footer_text': 'See more', 'footer_pathname': '/this/is/the/pathname', + 'addon_count': '0', }, follow=True, ) diff --git a/src/olympia/shelves/migrations/0011_auto_20210215_1216.py b/src/olympia/shelves/migrations/0011_auto_20210215_1216.py new file mode 100644 index 0000000000..6fe5894382 --- /dev/null +++ b/src/olympia/shelves/migrations/0011_auto_20210215_1216.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.18 on 2021-02-15 12:16 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('shelves', '0010_shelf_addon_count'), + ] + + operations = [ + migrations.AlterField( + model_name='shelf', + name='addon_count', + field=models.PositiveSmallIntegerField(default=0, help_text='0 means the default number (4, or 3 for search-themes) of add-ons will included be in shelf responses. Set to override.'), + ), + ] diff --git a/src/olympia/shelves/models.py b/src/olympia/shelves/models.py index 9c814617c7..c5095ccf18 100644 --- a/src/olympia/shelves/models.py +++ b/src/olympia/shelves/models.py @@ -27,7 +27,6 @@ class Shelf(ModelBase): ) addon_count = models.PositiveSmallIntegerField( default=0, - blank=True, help_text='0 means the default number (4, or 3 for search-themes) of add-ons ' 'will included be in shelf responses. Set to override.', ) diff --git a/src/olympia/shelves/tests/test_forms.py b/src/olympia/shelves/tests/test_forms.py index 79b5f3ace0..5c0c827c31 100644 --- a/src/olympia/shelves/tests/test_forms.py +++ b/src/olympia/shelves/tests/test_forms.py @@ -64,6 +64,7 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': 'search', 'criteria': self.criteria_sea, + 'addon_count': '0', }, ) assert form.is_valid(), form.errors @@ -77,6 +78,7 @@ class TestShelfForm(TestCase): 'title': 'Password managers (Collections)', 'endpoint': 'collections', 'criteria': self.criteria_col, + 'addon_count': '0', }, ) assert form.is_valid(), form.errors @@ -84,7 +86,12 @@ class TestShelfForm(TestCase): def test_clean_form_is_missing_title_field(self): form = ShelfForm( - {'title': '', 'endpoint': 'search', 'criteria': self.criteria_sea}, + { + 'title': '', + 'endpoint': 'search', + 'criteria': self.criteria_sea, + 'addon_count': '0', + }, ) assert not form.is_valid() assert form.errors == {'title': ['This field is required.']} @@ -95,14 +102,44 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': '', 'criteria': self.criteria_sea, + 'addon_count': '0', }, ) assert not form.is_valid() assert form.errors == {'endpoint': ['This field is required.']} + def test_clean_form_is_missing_addon_count_field(self): + data = { + 'title': 'Recommended extensions', + 'endpoint': 'search', + 'criteria': self.criteria_sea, + } + form = ShelfForm(data) + assert not form.is_valid() + assert form.errors == {'addon_count': ['This field is required.']} + + data['addon_count'] = '' + form = ShelfForm(data) + assert not form.is_valid() + assert form.errors == {'addon_count': ['This field is required.']} + + data['addon_count'] = 'aa' + form = ShelfForm(data) + assert not form.is_valid() + assert form.errors == {'addon_count': ['Enter a whole number.']} + + data['addon_count'] = '0' + form = ShelfForm(data) + assert form.is_valid(), form.errors + def test_clean_form_is_missing_criteria_field(self): form = ShelfForm( - {'title': 'Recommended extensions', 'endpoint': 'search', 'criteria': ''}, + { + 'title': 'Recommended extensions', + 'endpoint': 'search', + 'criteria': '', + 'addon_count': '0', + }, ) assert not form.is_valid() assert form.errors == {'criteria': ['This field is required.']} @@ -113,6 +150,7 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': 'search', 'criteria': '..?recommended-true', + 'addon_count': '0', }, ) assert not form.is_valid() @@ -126,6 +164,7 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': 'search', 'criteria': '??recommended-true', + 'addon_count': '0', }, ) assert not form.is_valid() @@ -139,6 +178,7 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': 'search', 'criteria': '?recommended=true&type=statictheme', + 'addon_count': '0', } ) assert not form.is_valid() @@ -154,6 +194,7 @@ class TestShelfForm(TestCase): 'title': 'Recommended extensions', 'endpoint': 'search-themes', 'criteria': '?recommended=true&type=extension', + 'addon_count': '0', } ) assert not form.is_valid() @@ -165,7 +206,12 @@ class TestShelfForm(TestCase): def test_clean_form_throws_error_for_NoReverseMatch(self): form = ShelfForm( - {'title': 'New collection', 'endpoint': 'collections', 'criteria': '/'}, + { + 'title': 'New collection', + 'endpoint': 'collections', + 'criteria': '/', + 'addon_count': '0', + }, ) assert not form.is_valid() with self.assertRaises(ValidationError) as exc: @@ -179,6 +225,7 @@ class TestShelfForm(TestCase): 'title': 'Password manager (Collections)', 'endpoint': 'collections', 'criteria': self.criteria_col_404, + 'addon_count': '0', } form = ShelfForm(data) assert not form.is_valid() @@ -191,6 +238,7 @@ class TestShelfForm(TestCase): 'title': 'Popular themes', 'endpoint': 'search', 'criteria': self.criteria_not_200, + 'addon_count': '0', } form = ShelfForm(data) assert not form.is_valid() @@ -203,6 +251,7 @@ class TestShelfForm(TestCase): 'title': 'Popular themes', 'endpoint': 'search', 'criteria': self.criteria_empty, + 'addon_count': '0', } form = ShelfForm(data) assert not form.is_valid()