From ec2ae9d1ef38d44cfeeb4abbe3bda035afe93e2b Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Fri, 4 Oct 2019 08:13:18 +0200 Subject: Add validation rules to Infraction serializer https://github.com/python-discord/site/issues/273 This commit adds validation rules to the Infraction serializer that validate if a given infraction should be accepted based on its status of being considered `active`. If the validation fails, the API will reject the request and return a 400 status. Specifically, this validator checks that: - infractions that can never be active do not have `active=True` set; - a user can never receive a second active infraction of the same type. Tests have been added to `test_infractions.py` to ensure that the validators work as expected. This commit implements the first part of #273 --- pydis_site/apps/api/models/bot/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'pydis_site/apps/api/models/bot') diff --git a/pydis_site/apps/api/models/bot/user.py b/pydis_site/apps/api/models/bot/user.py index 21617dc4..5140d2bf 100644 --- a/pydis_site/apps/api/models/bot/user.py +++ b/pydis_site/apps/api/models/bot/user.py @@ -50,7 +50,7 @@ class User(ModelReprMixin, models.Model): def __str__(self): """Returns the name and discriminator for the current user, for display purposes.""" - return f"{self.name}#{self.discriminator}" + return f"{self.name}#{self.discriminator:0>4}" @property def top_role(self) -> Role: -- cgit v1.2.3 From 0d383cbe925fdec97cb678c0168ecf7e90d3d729 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Tue, 8 Oct 2019 01:22:15 +0200 Subject: Prevent double active infractions with constraint https://github.com/python-discord/site/issues/273 This commits adds a UniqueConstraint for active infractions on a combination of the `user` and `type` field. This means that a user can only have one active infraction of a given type in the database at any time. I've also added tests to make sure that this behaves as expected. --- ...45_add_infractions_unique_constraints_active.py | 17 +++++ pydis_site/apps/api/models/bot/infraction.py | 7 ++ pydis_site/apps/api/tests/test_infractions.py | 89 +++++++++++++++++++++- 3 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py (limited to 'pydis_site/apps/api/models/bot') diff --git a/pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py b/pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py new file mode 100644 index 00000000..bacb56b8 --- /dev/null +++ b/pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py @@ -0,0 +1,17 @@ +# Generated by Django 2.2.6 on 2019-10-07 18:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0044_active_infractions_migration'), + ] + + operations = [ + migrations.AddConstraint( + model_name='infraction', + constraint=models.UniqueConstraint(condition=models.Q(active=True), fields=('user', 'type'), name='unique_active_infraction_per_type_per_user'), + ), + ] diff --git a/pydis_site/apps/api/models/bot/infraction.py b/pydis_site/apps/api/models/bot/infraction.py index dfb32a97..108fd3a2 100644 --- a/pydis_site/apps/api/models/bot/infraction.py +++ b/pydis_site/apps/api/models/bot/infraction.py @@ -71,3 +71,10 @@ class Infraction(ModelReprMixin, models.Model): """Defines the meta options for the infraction model.""" ordering = ['-inserted_at'] + constraints = ( + models.UniqueConstraint( + fields=["user", "type"], + condition=models.Q(active=True), + name="unique_active_infraction_per_type_per_user" + ), + ) diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index 8279b6a6..69d9ebea 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -1,6 +1,7 @@ from datetime import datetime as dt, timedelta, timezone from urllib.parse import quote +from django.db.utils import IntegrityError from django_hosts.resolvers import reverse from .base import APISubdomainTestCase @@ -167,6 +168,12 @@ class CreationTests(APISubdomainTestCase): discriminator=1, avatar_hash=None ) + cls.second_user = User.objects.create( + id=6, + name='carl', + discriminator=2, + avatar_hash=None + ) def test_accepts_valid_data(self): url = reverse('bot:infraction-list', host='api') @@ -390,6 +397,82 @@ class CreationTests(APISubdomainTestCase): second_response = self.client.post(url, data=second_active_infraction) self.assertEqual(second_response.status_code, 201) + def test_unique_constraint_raises_integrity_error_on_second_active_of_same_type(self): + """Do we raise `IntegrityError` for the second active infraction of a type for a user?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + with self.assertRaises(IntegrityError): + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The second active ban" + ) + + def test_unique_constraint_accepts_active_infraction_after_inactive_infraction(self): + """Do we accept an active infraction if the others of the same type are inactive?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=False, + reason="The first inactive ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=False, + reason="The second inactive ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + + def test_unique_constraint_accepts_second_active_of_different_type(self): + """Do we accept a second active infraction of a different type for a given user?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="mute", + active=True, + reason="The first active mute" + ) + + def test_unique_constraint_accepts_active_infractions_for_different_users(self): + """Do we accept two active infractions of the same type for two different users?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="An active ban for the first user" + ) + Infraction.objects.create( + user=self.second_user, + actor=self.second_user, + type="ban", + active=False, + reason="An active ban for the second user" + ) + class ExpandedTests(APISubdomainTestCase): @classmethod @@ -403,12 +486,14 @@ class ExpandedTests(APISubdomainTestCase): cls.kick = Infraction.objects.create( user_id=cls.user.id, actor_id=cls.user.id, - type='kick' + type='kick', + active=False ) cls.warning = Infraction.objects.create( user_id=cls.user.id, actor_id=cls.user.id, - type='warning' + type='warning', + active=False, ) def check_expanded_fields(self, infraction): -- cgit v1.2.3