Merge pull request #317 from python-discord/fix-infraction-validator

Fix Infraction UniqueTogetherValidator

6 files changed, 135 insertions, 18 deletions

diff --git a/README.md b/README.md
index 86509beb..ec2f0af3 100644
--- a/README.md
+++ b/README.md
@@ -4,14 +4,15 @@
 [![Tests](https://img.shields.io/azure-devops/tests/python-discord/Python%20Discord/2?compact_message)](https://dev.azure.com/python-discord/Python%20Discord/_apis/build/status/Site?branchName=master)
 [![Coverage](https://img.shields.io/azure-devops/coverage/python-discord/Python%20Discord/2/master)](https://dev.azure.com/python-discord/Python%20Discord/_apis/build/status/Site?branchName=master)
 [![License](https://img.shields.io/github/license/python-discord/site)](LICENSE)
-[![Status](https://img.shields.io/website?url=https%3A%2F%2Fpythondiscord.com)](https://pythondiscord.com)
+[![Status](https://img.shields.io/website?url=https%3A%2F%2Fpythondiscord.com)][1]
 
-This is all of the code that is responsible for maintaining
-[our website](https://pythondiscord.com), and all of its subdomains.
+This is all of the code that is responsible for maintaining [our website][1] and all of its subdomains.
 
 The website is built on Django and should be simple to set up and get started with.
 If you happen to run into issues with setup, please don't hesitate to open an issue!
 
-If you're looking to contribute or play around with the code,
-take a look at the [`docs` directory](docs). If you're looking for things
-to do, check out [our issues](https://gitlab.com/python-discord/projects/site/issues).
+If you're looking to contribute or play around with the code, take a look at [the wiki][2] or the [`docs` directory](docs). If you're looking for things to do, check out [our issues][3].
+
+[1]: https://pythondiscord.com
+[2]: https://pythondiscord.com/pages/contributing/site/
+[3]: https://github.com/python-discord/site/issues
diff --git a/manage.py b/manage.py
index d2996488..ee071376 100755
--- a/manage.py
+++ b/manage.py
@@ -87,7 +87,7 @@ class SiteManager:
 
         # Get database URL based on environmental variable passed in compose
         database_url = os.environ["DATABASE_URL"]
-        match = re.search(r"@(\w+):(\d+)/", database_url)
+        match = re.search(r"@([\w.]+):(\d+)/", database_url)
         if not match:
             raise OSError("Valid DATABASE_URL environmental variable not found.")
         domain = match.group(1)
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..90c91d63
--- /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 2020-02-08 19:00
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api', '0049_deletedmessage_attachments'),
+    ]
+
+    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/serializers.py b/pydis_site/apps/api/serializers.py
index 0d1a4684..e11c4af2 100644
--- a/pydis_site/apps/api/serializers.py
+++ b/pydis_site/apps/api/serializers.py
@@ -110,7 +110,7 @@ class InfractionSerializer(ModelSerializer):
         validators = [
             UniqueTogetherValidator(
                 queryset=Infraction.objects.filter(active=True),
-                fields=['user', 'type'],
+                fields=['user', 'type', 'active'],
                 message='This user already has an active infraction of this type.',
             )
         ]
diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py
index 7a54640e..ca87026c 100644
--- a/pydis_site/apps/api/tests/test_infractions.py
+++ b/pydis_site/apps/api/tests/test_infractions.py
@@ -7,6 +7,7 @@ from django_hosts.resolvers import reverse
 
 from .base import APISubdomainTestCase
 from ..models import Infraction, User
+from ..serializers import InfractionSerializer
 
 
 class UnauthenticatedTests(APISubdomainTestCase):
@@ -54,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,
@@ -184,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)
@@ -208,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)
@@ -222,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)
@@ -236,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)
@@ -251,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)
@@ -271,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)
@@ -288,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)
@@ -305,6 +314,7 @@ class CreationTests(APISubdomainTestCase):
             'actor': self.user.id,
             'type': 'note',
             'hidden': False,
+            'active': False,
         }
 
         response = self.client.post(url, data=data)
@@ -494,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
@@ -540,7 +560,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)
@@ -569,3 +590,81 @@ 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):
+        cls.user = User.objects.create(
+            id=5,
+            name='james',
+            discriminator=1,
+            avatar_hash=None
+        )
+
+    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(instance, data=data, partial=True)
+
+        self.assertTrue(serializer.is_valid(), msg=serializer.errors)
+
+    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(instance, data=data, partial=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:  # pragma: no cover
+            self.fail('Validation unexpectedly succeeded.')
+
+    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(), 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:  # pragma: no cover
+            self.fail('Validation unexpectedly succeeded.')