From 6b992149a9659207d992c2c225768cdff5b0fbbc Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 18 Dec 2019 20:29:21 -0800 Subject: Add regression test for the UniqueTogetherValidator fix --- pydis_site/apps/api/tests/test_infractions.py | 41 +++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index 7a54640e..e79bc2dc 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -4,9 +4,11 @@ from urllib.parse import quote from django.db.utils import IntegrityError from django_hosts.resolvers import reverse +from rest_framework.exceptions import ValidationError from .base import APISubdomainTestCase from ..models import Infraction, User +from ..serializers import InfractionSerializer class UnauthenticatedTests(APISubdomainTestCase): @@ -569,3 +571,42 @@ class ExpandedTests(APISubdomainTestCase): infraction = Infraction.objects.get(id=self.kick.id) self.assertEqual(infraction.active, data['active']) self.check_expanded_fields(response.json()) + + +class SerializerTests(APISubdomainTestCase): + @classmethod + def setUpTestData(cls): # noqa + cls.user = User.objects.create( + id=5, + name='james', + discriminator=1, + avatar_hash=None + ) + cls.ban_active = Infraction.objects.create( + user_id=cls.user.id, + actor_id=cls.user.id, + type='ban', + reason='He terk my jerb!', + expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc) + ) + cls.ban_inactive = Infraction.objects.create( + user_id=cls.user.id, + actor_id=cls.user.id, + type='ban', + reason='James is an ass, and we won\'t be working with him again.', + active=False + ) + + def test_is_valid_if_active_infraction_with_same_fields_exists(self): + data = {'reason': 'hello'} + serializer = InfractionSerializer(self.ban_inactive, data=data, partial=True) + + self.assertTrue(serializer.is_valid()) + + def test_validation_error_if_active_duplicate(self): + data = {'active': True} + serializer = InfractionSerializer(self.ban_inactive, data=data, partial=True) + + msg = 'This user already has an active infraction of this type' + with self.assertRaisesRegex(ValidationError, msg): + serializer.is_valid(raise_exception=True) -- cgit v1.2.3 From a0ea44f5eba9c6d3e03b48c4c28e9704da5b0788 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 18 Dec 2019 20:59:16 -0800 Subject: Add fixtures to create infractions in serializer tests The fixtures are invoked manually because not all cases may need the same infractions. --- pydis_site/apps/api/tests/test_infractions.py | 34 ++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index e79bc2dc..cacd4d06 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -575,37 +575,39 @@ class ExpandedTests(APISubdomainTestCase): class SerializerTests(APISubdomainTestCase): @classmethod - def setUpTestData(cls): # noqa + def setUpTestData(cls): cls.user = User.objects.create( id=5, name='james', discriminator=1, avatar_hash=None ) - cls.ban_active = Infraction.objects.create( - user_id=cls.user.id, - actor_id=cls.user.id, - type='ban', - reason='He terk my jerb!', - expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc) - ) - cls.ban_inactive = Infraction.objects.create( - user_id=cls.user.id, - actor_id=cls.user.id, - type='ban', - reason='James is an ass, and we won\'t be working with him again.', - active=False + + def create_infraction(self, _type: str, active: bool): + return Infraction.objects.create( + user_id=self.user.id, + actor_id=self.user.id, + type=_type, + reason='A reason.', + expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc), + active=active ) def test_is_valid_if_active_infraction_with_same_fields_exists(self): + self.create_infraction('ban', active=True) + instance = self.create_infraction('ban', active=False) + data = {'reason': 'hello'} - serializer = InfractionSerializer(self.ban_inactive, data=data, partial=True) + serializer = InfractionSerializer(instance, data=data, partial=True) self.assertTrue(serializer.is_valid()) def test_validation_error_if_active_duplicate(self): + self.create_infraction('ban', active=True) + instance = self.create_infraction('ban', active=False) + data = {'active': True} - serializer = InfractionSerializer(self.ban_inactive, data=data, partial=True) + serializer = InfractionSerializer(instance, data=data, partial=True) msg = 'This user already has an active infraction of this type' with self.assertRaisesRegex(ValidationError, msg): -- cgit v1.2.3 From 8d493899dba790286690126c121fc92658052dcd Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 18 Dec 2019 20:59:55 -0800 Subject: Test that new infractions pass validation --- pydis_site/apps/api/tests/test_infractions.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index cacd4d06..da2171d5 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -612,3 +612,17 @@ class SerializerTests(APISubdomainTestCase): msg = 'This user already has an active infraction of this type' with self.assertRaisesRegex(ValidationError, msg): serializer.is_valid(raise_exception=True) + + def test_is_valid_for_new_active_infraction(self): + self.create_infraction('ban', active=False) + + data = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': 'ban', + 'reason': 'A reason.', + 'active': True + } + serializer = InfractionSerializer(data=data) + + self.assertTrue(serializer.is_valid()) -- cgit v1.2.3 From 309acd3982fa0c28c9cb65dd38618067e3c6f4d4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 18 Dec 2019 21:03:28 -0800 Subject: Output errors more clearly for infraction serializer test failures --- pydis_site/apps/api/tests/test_infractions.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index da2171d5..4ed5ac25 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -4,7 +4,6 @@ from urllib.parse import quote from django.db.utils import IntegrityError from django_hosts.resolvers import reverse -from rest_framework.exceptions import ValidationError from .base import APISubdomainTestCase from ..models import Infraction, User @@ -600,7 +599,7 @@ class SerializerTests(APISubdomainTestCase): data = {'reason': 'hello'} serializer = InfractionSerializer(instance, data=data, partial=True) - self.assertTrue(serializer.is_valid()) + self.assertTrue(serializer.is_valid(), msg=serializer.errors) def test_validation_error_if_active_duplicate(self): self.create_infraction('ban', active=True) @@ -609,9 +608,14 @@ class SerializerTests(APISubdomainTestCase): data = {'active': True} serializer = InfractionSerializer(instance, data=data, partial=True) - msg = 'This user already has an active infraction of this type' - with self.assertRaisesRegex(ValidationError, msg): - serializer.is_valid(raise_exception=True) + if not serializer.is_valid(): + self.assertIn('non_field_errors', serializer.errors) + + code = serializer.errors['non_field_errors'][0].code + msg = f'Expected failure on unique validator but got {serializer.errors}' + self.assertEqual(code, 'unique', msg=msg) + else: + self.fail('Validation unexpectedly succeeded.') def test_is_valid_for_new_active_infraction(self): self.create_infraction('ban', active=False) @@ -625,4 +629,4 @@ class SerializerTests(APISubdomainTestCase): } serializer = InfractionSerializer(data=data) - self.assertTrue(serializer.is_valid()) + self.assertTrue(serializer.is_valid(), msg=serializer.errors) -- cgit v1.2.3 From 438e947d7edaf1f9dfa10bea0a2adf20e0ebbc9f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 19 Dec 2019 13:36:08 -0800 Subject: Remove default value from the active field of infractions Due to the active field being specified in the UniqueTogetherValidator, the field is implicitly required. Typically default values are excluded from this restriction but in this case some infraction types must always be False. It's easier and makes more sense to require the active field explicitly rather than to write logic in the serializer which is conditional on the type of infractions. --- ...0050_remove_infractions_active_default_value.py | 18 ++++++++++++++ pydis_site/apps/api/models/bot/infraction.py | 1 - pydis_site/apps/api/tests/test_infractions.py | 28 +++++++++++++++------- 3 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 pydis_site/apps/api/migrations/0050_remove_infractions_active_default_value.py (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/migrations/0050_remove_infractions_active_default_value.py b/pydis_site/apps/api/migrations/0050_remove_infractions_active_default_value.py new file mode 100644 index 00000000..a0f03ad4 --- /dev/null +++ b/pydis_site/apps/api/migrations/0050_remove_infractions_active_default_value.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.6 on 2019-12-19 21:17 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0049_offensivemessage'), + ] + + operations = [ + migrations.AlterField( + model_name='infraction', + name='active', + field=models.BooleanField(help_text='Whether the infraction is still active.'), + ), + ] diff --git a/pydis_site/apps/api/models/bot/infraction.py b/pydis_site/apps/api/models/bot/infraction.py index 108fd3a2..f58e89a3 100644 --- a/pydis_site/apps/api/models/bot/infraction.py +++ b/pydis_site/apps/api/models/bot/infraction.py @@ -29,7 +29,6 @@ class Infraction(ModelReprMixin, models.Model): ) ) active = models.BooleanField( - default=True, help_text="Whether the infraction is still active." ) user = models.ForeignKey( diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index 4ed5ac25..c59ddaed 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -55,7 +55,8 @@ class InfractionTests(APISubdomainTestCase): type='ban', reason='He terk my jerb!', hidden=True, - expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc) + expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc), + active=True ) cls.ban_inactive = Infraction.objects.create( user_id=cls.user.id, @@ -185,7 +186,8 @@ class CreationTests(APISubdomainTestCase): 'type': 'ban', 'reason': 'He terk my jerb!', 'hidden': True, - 'expires_at': '5018-11-20T15:52:00+00:00' + 'expires_at': '5018-11-20T15:52:00+00:00', + 'active': True, } response = self.client.post(url, data=data) @@ -209,7 +211,8 @@ class CreationTests(APISubdomainTestCase): url = reverse('bot:infraction-list', host='api') data = { 'actor': self.user.id, - 'type': 'kick' + 'type': 'kick', + 'active': False, } response = self.client.post(url, data=data) @@ -223,7 +226,8 @@ class CreationTests(APISubdomainTestCase): data = { 'user': 1337, 'actor': self.user.id, - 'type': 'kick' + 'type': 'kick', + 'active': True, } response = self.client.post(url, data=data) @@ -237,7 +241,8 @@ class CreationTests(APISubdomainTestCase): data = { 'user': self.user.id, 'actor': self.user.id, - 'type': 'hug' + 'type': 'hug', + 'active': True, } response = self.client.post(url, data=data) @@ -252,7 +257,8 @@ class CreationTests(APISubdomainTestCase): 'user': self.user.id, 'actor': self.user.id, 'type': 'ban', - 'expires_at': '20/11/5018 15:52:00' + 'expires_at': '20/11/5018 15:52:00', + 'active': True, } response = self.client.post(url, data=data) @@ -272,7 +278,8 @@ class CreationTests(APISubdomainTestCase): 'user': self.user.id, 'actor': self.user.id, 'type': infraction_type, - 'expires_at': '5018-11-20T15:52:00+00:00' + 'expires_at': '5018-11-20T15:52:00+00:00', + 'active': False, } response = self.client.post(url, data=data) @@ -289,7 +296,8 @@ class CreationTests(APISubdomainTestCase): 'user': self.user.id, 'actor': self.user.id, 'type': infraction_type, - 'hidden': True + 'hidden': True, + 'active': False, } response = self.client.post(url, data=data) @@ -306,6 +314,7 @@ class CreationTests(APISubdomainTestCase): 'actor': self.user.id, 'type': 'note', 'hidden': False, + 'active': False, } response = self.client.post(url, data=data) @@ -541,7 +550,8 @@ class ExpandedTests(APISubdomainTestCase): data = { 'user': self.user.id, 'actor': self.user.id, - 'type': 'warning' + 'type': 'warning', + 'active': False } response = self.client.post(url, data=data) -- cgit v1.2.3 From 7c991b292b81f8381b1b71eaa6374c23dc8d3521 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 19 Dec 2019 13:52:19 -0800 Subject: Add tests for missing active field of infractions --- pydis_site/apps/api/tests/test_infractions.py | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index c59ddaed..63d1b144 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -504,6 +504,16 @@ class CreationTests(APISubdomainTestCase): reason="An active ban for the second user" ) + def test_integrity_error_if_missing_active_field(self): + pattern = 'null value in column "active" violates not-null constraint' + with self.assertRaisesRegex(IntegrityError, pattern): + Infraction.objects.create( + user=self.user, + actor=self.user, + type='ban', + reason='A reason.', + ) + class ExpandedTests(APISubdomainTestCase): @classmethod @@ -640,3 +650,21 @@ class SerializerTests(APISubdomainTestCase): serializer = InfractionSerializer(data=data) self.assertTrue(serializer.is_valid(), msg=serializer.errors) + + def test_validation_error_if_missing_active_field(self): + data = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': 'ban', + 'reason': 'A reason.', + } + serializer = InfractionSerializer(data=data) + + if not serializer.is_valid(): + self.assertIn('active', serializer.errors) + + code = serializer.errors['active'][0].code + msg = f'Expected failure on required active field but got {serializer.errors}' + self.assertEqual(code, 'required', msg=msg) + else: + self.fail('Validation unexpectedly succeeded.') -- cgit v1.2.3 From f9223a543ddeba5e2ecf9bad3b7bbd4709319e08 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 19 Dec 2019 14:01:00 -0800 Subject: Ignore coverage for infraction serializer unexpectedly succeeding --- pydis_site/apps/api/tests/test_infractions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index 63d1b144..ca87026c 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -634,7 +634,7 @@ class SerializerTests(APISubdomainTestCase): code = serializer.errors['non_field_errors'][0].code msg = f'Expected failure on unique validator but got {serializer.errors}' self.assertEqual(code, 'unique', msg=msg) - else: + else: # pragma: no cover self.fail('Validation unexpectedly succeeded.') def test_is_valid_for_new_active_infraction(self): @@ -666,5 +666,5 @@ class SerializerTests(APISubdomainTestCase): code = serializer.errors['active'][0].code msg = f'Expected failure on required active field but got {serializer.errors}' self.assertEqual(code, 'required', msg=msg) - else: + else: # pragma: no cover self.fail('Validation unexpectedly succeeded.') -- cgit v1.2.3