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 +-
 pydis_site/apps/api/serializers.py            | 13 ++++
 pydis_site/apps/api/tests/test_infractions.py | 85 +++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)

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:
diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py
index 326e20e1..784bb614 100644
--- a/pydis_site/apps/api/serializers.py
+++ b/pydis_site/apps/api/serializers.py
@@ -1,4 +1,5 @@
 """Converters from Django models to data interchange formats and back."""
+import logging
 
 from rest_framework.serializers import ModelSerializer, PrimaryKeyRelatedField, ValidationError
 from rest_framework_bulk import BulkSerializerMixin
@@ -12,6 +13,8 @@ from .models import (
     Tag, User
 )
 
+log = logging.getLogger(__name__)
+
 
 class BotSettingSerializer(ModelSerializer):
     """A class providing (de-)serialization of `BotSetting` instances."""
@@ -110,6 +113,16 @@ class InfractionSerializer(ModelSerializer):
         """Validate data constraints for the given data and abort if it is invalid."""
         infr_type = attrs.get('type')
 
+        active = attrs.get('active')
+        if active and infr_type in ('note', 'warning', 'kick'):
+            raise ValidationError({'active': [f'{infr_type} infractions cannot be active.']})
+
+        user = attrs.get('user')
+        if active and Infraction.objects.filter(user=user, type=infr_type, active=True).exists():
+            raise ValidationError(
+                {'active': [f'This user already has an active {infr_type} infraction']}
+            )
+
         expires_at = attrs.get('expires_at')
         if expires_at and infr_type in ('kick', 'warning'):
             raise ValidationError({'expires_at': [f'{infr_type} infractions cannot expire.']})
diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py
index c58c32e2..8279b6a6 100644
--- a/pydis_site/apps/api/tests/test_infractions.py
+++ b/pydis_site/apps/api/tests/test_infractions.py
@@ -305,6 +305,91 @@ class CreationTests(APISubdomainTestCase):
             'hidden': [f'{data["type"]} infractions must be hidden.']
         })
 
+    def test_returns_400_for_active_infraction_of_type_that_cannot_be_active(self):
+        """Test if the API rejects active infractions for types that cannot be active."""
+        url = reverse('bot:infraction-list', host='api')
+        restricted_types = ('note', 'warning', 'kick')
+
+        for infraction_type in restricted_types:
+            with self.subTest(infraction_type=infraction_type):
+                invalid_infraction = {
+                    'user': self.user.id,
+                    'actor': self.user.id,
+                    'type': infraction_type,
+                    'reason': 'Take me on!',
+                    'hidden': True,
+                    'active': True,
+                    'expires_at': '2019-10-04T12:52:00+00:00'
+                }
+                response = self.client.post(url, data=invalid_infraction)
+                self.assertEqual(response.status_code, 400)
+                self.assertEqual(response.json(), {
+                    'active': [f'{infraction_type} infractions cannot be active.']
+                })
+
+    def test_returns_400_for_second_active_infraction_of_the_same_type(self):
+        """Test if the API rejects a second active infraction of the same type for a given user."""
+        url = reverse('bot:infraction-list', host='api')
+        active_infraction_types = ('mute', 'ban', 'superstar')
+
+        for infraction_type in active_infraction_types:
+            with self.subTest(infraction_type=infraction_type):
+                first_active_infraction = {
+                    'user': self.user.id,
+                    'actor': self.user.id,
+                    'type': infraction_type,
+                    'reason': 'Take me on!',
+                    'active': True,
+                    'expires_at': '2019-10-04T12:52:00+00:00'
+                }
+
+                # Post the first active infraction of a type and confirm it's accepted.
+                first_response = self.client.post(url, data=first_active_infraction)
+                self.assertEqual(first_response.status_code, 201)
+
+                second_active_infraction = {
+                    'user': self.user.id,
+                    'actor': self.user.id,
+                    'type': infraction_type,
+                    'reason': 'Take on me!',
+                    'active': True,
+                    'expires_at': '2019-10-04T12:52:00+00:00'
+                }
+                second_response = self.client.post(url, data=second_active_infraction)
+                self.assertEqual(second_response.status_code, 400)
+                self.assertEqual(second_response.json(), {
+                    'active': [f'This user already has an active {infraction_type} infraction']
+                })
+
+    def test_returns_201_for_second_active_infraction_of_different_type(self):
+        """Test if the API accepts a second active infraction of a different type than the first."""
+        url = reverse('bot:infraction-list', host='api')
+        first_active_infraction = {
+            'user': self.user.id,
+            'actor': self.user.id,
+            'type': 'mute',
+            'reason': 'Be silent!',
+            'hidden': True,
+            'active': True,
+            'expires_at': '2019-10-04T12:52:00+00:00'
+        }
+        second_active_infraction = {
+            'user': self.user.id,
+            'actor': self.user.id,
+            'type': 'ban',
+            'reason': 'Be gone!',
+            'hidden': True,
+            'active': True,
+            'expires_at': '2019-10-05T12:52:00+00:00'
+        }
+        # Post the first active infraction of a type and confirm it's accepted.
+        first_response = self.client.post(url, data=first_active_infraction)
+        self.assertEqual(first_response.status_code, 201)
+
+        # Post the first active infraction of a type and confirm it's accepted.
+        second_response = self.client.post(url, data=second_active_infraction)
+        self.assertEqual(second_response.status_code, 201)
+
 
 class ExpandedTests(APISubdomainTestCase):
     @classmethod
-- 
cgit v1.2.3


From cf1a0755580b109d5f35331b6d5cbfef0d92fa97 Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Mon, 7 Oct 2019 08:59:23 +0200
Subject: Migrate undesirable active infraction to inactive

https://github.com/python-discord/site/issues/273

This commit adds a data migration to migrate active infractions that
should not be active to inactive. There are two types of infractions
that this migration will migrate to inactive:

- Infractions of types that should never be active (e.g. notes)

- Secondary active infractions if a given user already has an active
  infraction of the same type.

Since this makes the migration file fairly complex, I have written
tests to make sure the migration works as expected. In order to do
this, I've subclassed `django.test.TestCase` to create a
`MigrationsTestCase` that takes care of reverting the database back
to a state prior to the migrations we want to test and injects test
data before applying the migrations we want to test.

For more information, see `pydis_site.apps.api.tests.migrations.base`

This implements the last part of and closes #273
---
 .../0044_active_infractions_migration.py           | 106 ++++
 pydis_site/apps/api/tests/migrations/__init__.py   |   1 +
 pydis_site/apps/api/tests/migrations/base.py       | 102 ++++
 .../migrations/test_active_infraction_migration.py | 545 +++++++++++++++++++++
 4 files changed, 754 insertions(+)
 create mode 100644 pydis_site/apps/api/migrations/0044_active_infractions_migration.py
 create mode 100644 pydis_site/apps/api/tests/migrations/__init__.py
 create mode 100644 pydis_site/apps/api/tests/migrations/base.py
 create mode 100644 pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py

diff --git a/pydis_site/apps/api/migrations/0044_active_infractions_migration.py b/pydis_site/apps/api/migrations/0044_active_infractions_migration.py
new file mode 100644
index 00000000..14746712
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0044_active_infractions_migration.py
@@ -0,0 +1,106 @@
+# Generated by Django 2.2.6 on 2019-10-07 15:59
+
+from django.db import migrations
+from django.db.models import Count, Prefetch, Q, QuerySet
+
+
+class ExpirationWrapper:
+    """Wraps an expiration date to properly compare permanent and temporary infractions."""
+
+    def __init__(self, infraction):
+        self.expiration_date = infraction.expires_at
+
+    def __lt__(self, other):
+        """An `expiration_date` is considered smaller when it comes earlier than the `other`."""
+        if self.expiration_date is None:
+            # A permanent infraction can never end sooner than another infraction
+            return False
+        elif other.expiration_date is None:
+            # If `self` is temporary, but `other` is permanent, `self` is smaller
+            return True
+        else:
+            return self.expiration_date < other.expiration_date
+
+    def __eq__(self, other):
+        """If both expiration dates are permanent they're equal, otherwise compare dates."""
+        if self.expiration_date is None and other.expiration_date is None:
+            return True
+        elif self.expiration_date is None or other.expiration_date is None:
+            return False
+        else:
+            return self.expiration_date == other.expiration_date
+
+
+def migrate_inactive_types_to_inactive(apps, schema_editor):
+    """Migrates infractions of non-active types to inactive."""
+    inactive_infraction_types = Q(type="note") | Q(type="warning") | Q(type="kick")
+    infraction_model = apps.get_model('api', 'Infraction')
+    infraction_model.objects.filter(inactive_infraction_types).update(active=False)
+
+
+def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
+    """
+    Creates QuerySet to fetch users with multiple active infractions of the given `type`.
+
+    The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the
+    `User` instances.
+    """
+    active_infractions = infraction_model.objects.filter(type=infr_type, active=True)
+
+    # Build an SQL query by chaining methods together
+
+    # Get users with active infraction(s) of the provided `infr_type`
+    query = user_model.objects.filter(
+        Q(infractions_received__type=infr_type, infractions_received__active=True)
+    )
+
+    # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
+    query = query.prefetch_related(
+        Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions')
+    )
+
+    # Count and only include them if they have at least 2 active infractions of the `type`
+    query = query.annotate(num_infractions=Count('infractions_received'))
+    query = query.filter(num_infractions__gte=2)
+
+    # Make sure we return each individual only once
+    query = query.distinct()
+
+    return query
+
+
+def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor):
+    """
+    Make sure a user only has one active infraction of a given "active" infraction type.
+
+    If a user has multiple active infraction, we keep the one with longest expiration date active
+    and migrate the others to inactive.
+    """
+    infraction_model = apps.get_model('api', 'Infraction')
+    user_model = apps.get_model('api', 'User')
+
+    for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
+        query = get_query(user_model, infraction_model, infraction_type)
+        for user in query:
+            infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True)
+            for infraction in infractions[1:]:
+                infraction.active = False
+                infraction.save()
+
+
+def reverse_migration(apps, schema_editor):
+    """There's no need to do anything special to reverse these migrations."""
+    return
+
+
+class Migration(migrations.Migration):
+    """Data migration to get the database consistent with the new infraction validation rules."""
+
+    dependencies = [
+        ('api', '0043_infraction_hidden_warnings_to_notes'),
+    ]
+
+    operations = [
+        migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration),
+        migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration)
+    ]
diff --git a/pydis_site/apps/api/tests/migrations/__init__.py b/pydis_site/apps/api/tests/migrations/__init__.py
new file mode 100644
index 00000000..38e42ffc
--- /dev/null
+++ b/pydis_site/apps/api/tests/migrations/__init__.py
@@ -0,0 +1 @@
+"""This submodule contains tests for functions used in data migrations."""
diff --git a/pydis_site/apps/api/tests/migrations/base.py b/pydis_site/apps/api/tests/migrations/base.py
new file mode 100644
index 00000000..0c0a5bd0
--- /dev/null
+++ b/pydis_site/apps/api/tests/migrations/base.py
@@ -0,0 +1,102 @@
+"""Includes utilities for testing migrations."""
+from django.db import connection
+from django.db.migrations.executor import MigrationExecutor
+from django.test import TestCase
+
+
+class MigrationsTestCase(TestCase):
+    """
+    A `TestCase` subclass to test migration files.
+
+    To be able to properly test a migration, we will need to inject data into the test database
+    before the migrations we want to test are applied, but after the older migrations have been
+    applied. This makes sure that we are testing "as if" we were actually applying this migration
+    to a database in the state it was in before introducing the new migration.
+
+    To set up a MigrationsTestCase, create a subclass of this class and set the following
+    class-level attributes:
+
+    - app: The name of the app that contains the migrations (e.g., `'api'`)
+    - migration_prior: The name* of the last migration file before the migrations you want to test
+    - migration_target: The name* of the last migration file we want to test
+
+    *) Specify the file names without a path or the `.py` file extension.
+
+    Additionally, overwrite the `setUpMigrationData` in the subclass to inject data into the
+    database before the migrations we want to test are applied. Please read the docstring of the
+    method for more information. An optional hook, `setUpPostMigrationData` is also provided.
+    """
+
+    # These class-level attributes should be set in classes that inherit from this base class.
+    app = None
+    migration_prior = None
+    migration_target = None
+
+    @classmethod
+    def setUpTestData(cls):
+        """
+        Injects data into the test database prior to the migration we're trying to test.
+
+        This class methods reverts the test database back to the state of the last migration file
+        prior to the migrations we want to test. It will then allow the user to inject data into the
+        test database by calling the `setUpMigrationData` hook. After the data has been injected, it
+        will apply the migrations we want to test and call the `setUpPostMigrationData` hook. The
+        user can now test if the migration correctly migrated the injected test data.
+        """
+        if not cls.app:
+            raise ValueError("The `app` attribute was not set.")
+
+        if not cls.migration_prior or not cls.migration_target:
+            raise ValueError("Both ` migration_prior` and `migration_target` need to be set.")
+
+        cls.migrate_from = [(cls.app, cls.migration_prior)]
+        cls.migrate_to = [(cls.app, cls.migration_target)]
+
+        # Reverse to database state prior to the migrations we want to test
+        executor = MigrationExecutor(connection)
+        executor.migrate(cls.migrate_from)
+
+        # Call the data injection hook with the current state of the project
+        old_apps = executor.loader.project_state(cls.migrate_from).apps
+        cls.setUpMigrationData(old_apps)
+
+        # Run the migrations we want to test
+        executor = MigrationExecutor(connection)
+        executor.loader.build_graph()
+        executor.migrate(cls.migrate_to)
+
+        # Save the project state so we're able to work with the correct model states
+        cls.apps = executor.loader.project_state(cls.migrate_to).apps
+
+        # Call `setUpPostMigrationData` to potentially set up post migration data used in testing
+        cls.setUpPostMigrationData(cls.apps)
+
+    @classmethod
+    def setUpMigrationData(cls, apps):
+        """
+        Override this method to inject data into the test database before the migration is applied.
+
+        This method will be called after setting up the database according to the migrations that
+        come before the migration(s) we are trying to test, but before the to-be-tested migration(s)
+        are applied. This allows us to simulate a database state just prior to the migrations we are
+        trying to test.
+
+        To make sure we're creating objects according to the state the models were in at this point
+        in the migration history, use `apps.get_model(app_name: str, model_name: str)` to get the
+        appropriate model, e.g.:
+
+        >>> Infraction = apps.get_model('api', 'Infraction')
+        """
+        pass
+
+    @classmethod
+    def setUpPostMigrationData(cls, apps):
+        """
+        Set up additional test data after the target migration has been applied.
+
+        Use `apps.get_model(app_name: str, model_name: str)` to get the correct instances of the
+        model classes:
+
+        >>> Infraction = apps.get_model('api', 'Infraction')
+        """
+        pass
diff --git a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
new file mode 100644
index 00000000..cfae4219
--- /dev/null
+++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
@@ -0,0 +1,545 @@
+"""Tests for the data migration in `filename`."""
+import logging
+from collections import ChainMap, namedtuple
+from datetime import timedelta
+from itertools import count
+from typing import Dict, Iterable, Type, Union
+
+from django.db.models import Q
+from django.forms.models import model_to_dict
+from django.utils import timezone
+
+from pydis_site.apps.api.models import Infraction, User
+from .base import MigrationsTestCase
+
+log = logging.getLogger(__name__)
+log.setLevel(logging.DEBUG)
+
+
+InfractionHistory = namedtuple('InfractionHistory', ("user_id", "infraction_history"))
+
+
+class InfractionFactory:
+    """Factory that creates infractions for a User instance."""
+
+    infraction_id = count(1)
+    user_id = count(1)
+    default_values = {
+        'active': True,
+        'expires_at': None,
+        'hidden': False,
+    }
+
+    @classmethod
+    def create(
+        cls,
+        actor: User,
+        infractions: Iterable[Dict[str, Union[str, int, bool]]],
+        infraction_model: Type[Infraction] = Infraction,
+        user_model: Type[User] = User,
+    ) -> InfractionHistory:
+        """
+        Creates `infractions` for the `user` with the given `actor`.
+
+        The `infractions` dictionary can contain the following fields:
+         - `type` (required)
+         - `active` (default: True)
+         - `expires_at` (default: None; i.e, permanent)
+         - `hidden` (default: False).
+
+         The parameters `infraction_model` and `user_model` can be used to pass in an instance of
+         both model classes from a different migration/project state.
+        """
+        user_id = next(cls.user_id)
+        user = user_model.objects.create(
+            id=user_id,
+            name=f"Infracted user {user_id}",
+            discriminator=user_id,
+            avatar_hash=None,
+        )
+        infraction_history = []
+
+        for infraction in infractions:
+            infraction = dict(infraction)
+            infraction["id"] = next(cls.infraction_id)
+            infraction = ChainMap(infraction, cls.default_values)
+            new_infraction = infraction_model.objects.create(
+                user=user,
+                actor=actor,
+                type=infraction["type"],
+                reason=f"`{infraction['type']}` infraction (ID: {infraction['id']} of {user}",
+                active=infraction['active'],
+                hidden=infraction['hidden'],
+                expires_at=infraction['expires_at'],
+            )
+            infraction_history.append(new_infraction)
+
+        return InfractionHistory(user_id=user_id, infraction_history=infraction_history)
+
+
+class InfractionFactoryTests(MigrationsTestCase):
+    """Tests for the InfractionFactory."""
+
+    app = "api"
+    migration_prior = "0043_infraction_hidden_warnings_to_notes"
+    migration_target = "0043_infraction_hidden_warnings_to_notes"
+
+    @classmethod
+    def setUpPostMigrationData(cls, apps):
+        """Create a default actor for all infractions."""
+        cls.infraction_model = apps.get_model('api', 'Infraction')
+        cls.user_model = apps.get_model('api', 'User')
+
+        cls.actor = cls.user_model.objects.create(
+            id=9999,
+            name="Unknown Moderator",
+            discriminator=1040,
+            avatar_hash=None,
+        )
+
+    def test_infraction_factory_total_count(self):
+        """Does the test database hold as many infractions as we tried to create?"""
+        InfractionFactory.create(
+            actor=self.actor,
+            infractions=(
+                {'type': 'kick', 'active': False, 'hidden': False},
+                {'type': 'ban', 'active': True, 'hidden': False},
+                {'type': 'note', 'active': False, 'hidden': True},
+            ),
+            infraction_model=self.infraction_model,
+            user_model=self.user_model,
+        )
+        database_count = Infraction.objects.all().count()
+        self.assertEqual(3, database_count)
+
+    def test_infraction_factory_multiple_users(self):
+        """Does the test database hold as many infractions as we tried to create?"""
+        created_infractions = {}
+        for user in range(5):
+            created_infractions.update(
+                {
+                    user: InfractionFactory.create(
+                        actor=self.actor,
+                        infractions=(
+                            {'type': 'kick', 'active': False, 'hidden': True},
+                            {'type': 'ban', 'active': True, 'hidden': False},
+                        ),
+                        infraction_model=self.infraction_model,
+                        user_model=self.user_model,
+                    )
+                }
+            )
+
+        # Check if infractions and users are recorded properly in the database
+        database_count = Infraction.objects.all().count()
+        self.assertEqual(database_count, 10)
+
+        user_count = User.objects.all().count()
+        self.assertEqual(user_count, 5 + 1)
+
+    def test_infraction_factory_sets_correct_fields(self):
+        """Does the InfractionFactory set the correct attributes?"""
+        infractions = (
+            {
+                'type': 'note',
+                'active': False,
+                'hidden': True,
+                'expires_at': timezone.now()
+            },
+            {'type': 'warning', 'active': False, 'hidden': False, 'expires_at': None},
+            {'type': 'watch', 'active': False, 'hidden': True, 'expires_at': None},
+            {'type': 'mute', 'active': True, 'hidden': False, 'expires_at': None},
+            {'type': 'kick', 'active': True, 'hidden': True, 'expires_at': None},
+            {'type': 'ban', 'active': True, 'hidden': False, 'expires_at': None},
+            {
+                'type': 'superstar',
+                'active': True,
+                'hidden': True,
+                'expires_at': timezone.now()
+            },
+        )
+
+        InfractionFactory.create(
+            actor=self.actor,
+            infractions=infractions,
+            infraction_model=self.infraction_model,
+            user_model=self.user_model,
+        )
+
+        for infraction in infractions:
+            with self.subTest(**infraction):
+                self.assertTrue(Infraction.objects.filter(**infraction).exists())
+
+
+class ActiveInfractionMigrationTests(MigrationsTestCase):
+    """
+    Tests the active infraction data migration.
+
+    The active infraction data migration should do the following things:
+
+    1.  migrates all active notes, warnings, and kicks to an inactive status;
+    2.  migrates all users with multiple active infractions of a single type to have only one active
+        infraction of that type. The infraction with the longest duration stays active.
+    """
+
+    app = "api"
+    migration_prior = "0043_infraction_hidden_warnings_to_notes"
+    migration_target = "0044_active_infractions_migration"
+
+    @classmethod
+    def setUpMigrationData(cls, apps):
+        """Sets up an initial database state that contains the relevant test cases."""
+        # Fetch the Infraction and User model in the current migration state
+        cls.infraction_model = apps.get_model('api', 'Infraction')
+        cls.user_model = apps.get_model('api', 'User')
+
+        cls.created_infractions = {}
+
+        # Moderator that serves as actor for all infractions
+        cls.user_moderator = cls.user_model.objects.create(
+            id=9999,
+            name="Olivier de Vienne",
+            discriminator=1040,
+            avatar_hash=None,
+        )
+
+        # User #1: clean user with no infractions
+        cls.created_infractions.update(
+            {
+                1: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=[],
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #2: One inactive note infraction
+        cls.created_infractions.update(
+            {
+                2: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'note', 'active': False, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #3: One active note infraction
+        cls.created_infractions.update(
+            {
+                3: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'note', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #4: One active and one inactive note infraction
+        cls.created_infractions.update(
+            {
+                4: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'note', 'active': False, 'hidden': True},
+                        {'type': 'note', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #5: Once active note, one active kick, once active warning
+        cls.created_infractions.update(
+            {
+                5: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'note', 'active': True, 'hidden': True},
+                        {'type': 'kick', 'active': True, 'hidden': True},
+                        {'type': 'warning', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #6: One inactive ban and one active ban
+        cls.created_infractions.update(
+            {
+                6: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'ban', 'active': False, 'hidden': True},
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #7: Two active permanent bans
+        cls.created_infractions.update(
+            {
+                7: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #8: Multiple active temporary bans
+        cls.created_infractions.update(
+            {
+                8: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=1)
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=10)
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=20)
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=5)
+                        },
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #9: One active permanent ban, two active temporary bans
+        cls.created_infractions.update(
+            {
+                9: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=10)
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': None,
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=7)
+                        },
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #10: One inactive permanent ban, two active temporary bans
+        cls.created_infractions.update(
+            {
+                10: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=10)
+                        },
+                        {
+                            'type': 'ban',
+                            'active': False,
+                            'hidden': True,
+                            'expires_at': None,
+                        },
+                        {
+                            'type': 'ban',
+                            'active': True,
+                            'hidden': True,
+                            'expires_at': timezone.now() + timedelta(days=7)
+                        },
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #11: Active ban, active mute, active superstar
+        cls.created_infractions.update(
+            {
+                11: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                        {'type': 'mute', 'active': True, 'hidden': True},
+                        {'type': 'superstar', 'active': True, 'hidden': True},
+                        {'type': 'watch', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+        # User #12: Multiple active bans, active mutes, active superstars
+        cls.created_infractions.update(
+            {
+                12: InfractionFactory.create(
+                    actor=cls.user_moderator,
+                    infractions=(
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                        {'type': 'ban', 'active': True, 'hidden': True},
+                        {'type': 'mute', 'active': True, 'hidden': True},
+                        {'type': 'mute', 'active': True, 'hidden': True},
+                        {'type': 'mute', 'active': True, 'hidden': True},
+                        {'type': 'superstar', 'active': True, 'hidden': True},
+                        {'type': 'superstar', 'active': True, 'hidden': True},
+                        {'type': 'superstar', 'active': True, 'hidden': True},
+                        {'type': 'watch', 'active': True, 'hidden': True},
+                        {'type': 'watch', 'active': True, 'hidden': True},
+                        {'type': 'watch', 'active': True, 'hidden': True},
+                    ),
+                    infraction_model=cls.infraction_model,
+                    user_model=cls.user_model,
+                )
+            }
+        )
+
+    def test_all_never_active_types_became_inactive(self):
+        """Are all infractions of a non-active type inactive after the migration?"""
+        inactive_type_query = Q(type="note") | Q(type="warning") | Q(type="kick")
+        self.assertFalse(
+            self.infraction_model.objects.filter(inactive_type_query, active=True).exists()
+        )
+
+    def test_migration_left_clean_user_without_infractions(self):
+        """Do users without infractions have no infractions after the migration?"""
+        user_id, infraction_history = self.created_infractions[1]
+        self.assertFalse(
+            self.infraction_model.objects.filter(user__id=user_id).exists()
+        )
+
+    def test_migration_left_user_with_inactive_note_untouched(self):
+        """Did the migration leave users with only an inactive note untouched?"""
+        user_id, infraction_history = self.created_infractions[2]
+        inactive_note = infraction_history[0]
+        self.assertTrue(
+            self.infraction_model.objects.filter(**model_to_dict(inactive_note)).exists()
+        )
+
+    def test_migration_only_touched_active_field_of_active_note(self):
+        """Does the migration only change the `active` field?"""
+        user_id, infraction_history = self.created_infractions[3]
+        note = model_to_dict(infraction_history[0])
+        note['active'] = False
+        self.assertTrue(
+            self.infraction_model.objects.filter(**note).exists()
+        )
+
+    def test_migration_only_touched_active_field_of_active_note_left_inactive_untouched(self):
+        """Does the migration only change the `active` field of active notes?"""
+        user_id, infraction_history = self.created_infractions[4]
+        for note in infraction_history:
+            with self.subTest(active=note.active):
+                note = model_to_dict(note)
+                note['active'] = False
+                self.assertTrue(
+                    self.infraction_model.objects.filter(**note).exists()
+                )
+
+    def test_migration_migrates_all_nonactive_types_to_inactive(self):
+        """Do we set the `active` field of all non-active infractions to `False`?"""
+        user_id, infraction_history = self.created_infractions[5]
+        self.assertFalse(
+            self.infraction_model.objects.filter(user__id=user_id, active=True).exists()
+        )
+
+    def test_migration_leaves_user_with_one_active_ban_untouched(self):
+        """Do we leave a user with one active and one inactive ban untouched?"""
+        user_id, infraction_history = self.created_infractions[6]
+        for infraction in infraction_history:
+            with self.subTest(active=infraction.active):
+                self.assertTrue(
+                    self.infraction_model.objects.filter(**model_to_dict(infraction)).exists()
+                )
+
+    def test_migration_turns_double_active_perm_ban_into_single_active_perm_ban(self):
+        """Does the migration turn two active permanent bans into one active permanent ban?"""
+        user_id, infraction_history = self.created_infractions[7]
+        active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count()
+        self.assertEqual(active_count, 1)
+
+    def test_migration_leaves_temporary_ban_with_longest_duration_active(self):
+        """Does the migration turn two active permanent bans into one active permanent ban?"""
+        user_id, infraction_history = self.created_infractions[8]
+        active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
+        self.assertEqual(active_ban.expires_at, infraction_history[2].expires_at)
+
+    def test_migration_leaves_permanent_ban_active(self):
+        """Does the migration leave the permanent ban active?"""
+        user_id, infraction_history = self.created_infractions[9]
+        active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
+        self.assertIsNone(active_ban.expires_at)
+
+    def test_migration_leaves_longest_temp_ban_active_with_inactive_permanent_ban(self):
+        """Does the longest temp ban stay active, even with an inactive perm ban present?"""
+        user_id, infraction_history = self.created_infractions[10]
+        active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
+        self.assertEqual(active_ban.expires_at, infraction_history[0].expires_at)
+
+    def test_migration_leaves_all_active_types_active_if_one_of_each_exists(self):
+        """Do all active infractions stay active if only one of each is present?"""
+        user_id, infraction_history = self.created_infractions[11]
+        active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count()
+        self.assertEqual(active_count, 4)
+
+    def test_migration_reduces_all_active_types_to_a_single_active_infraction(self):
+        """Do we reduce all of the infraction types to one active infraction?"""
+        user_id, infraction_history = self.created_infractions[12]
+        active_infractions = self.infraction_model.objects.filter(user__id=user_id, active=True)
+        self.assertEqual(len(active_infractions), 4)
+        types_observed = [infraction.type for infraction in active_infractions]
+
+        for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
+            with self.subTest(type=infraction_type):
+                self.assertIn(infraction_type, types_observed)
-- 
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

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


From e415428386caae19aa0398aea66ee5053569abcf Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Fri, 18 Oct 2019 12:52:11 +0200
Subject: Solve migration conflict by renaming migrations

The migration files were generated and named before the migrations
added by other pull requests. This caused the migration path to
diverge. Since the migrations did not touch the same models, the
solution was to rename the migration files to place them at the end
of the migration history.
---
 .../0044_active_infractions_migration.py           | 106 ---------------------
 ...45_add_infractions_unique_constraints_active.py |  17 ----
 .../0046_active_infractions_migration.py           | 106 +++++++++++++++++++++
 ...47_add_infractions_unique_constraints_active.py |  17 ++++
 .../migrations/test_active_infraction_migration.py |   8 +-
 5 files changed, 127 insertions(+), 127 deletions(-)
 delete mode 100644 pydis_site/apps/api/migrations/0044_active_infractions_migration.py
 delete mode 100644 pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py
 create mode 100644 pydis_site/apps/api/migrations/0046_active_infractions_migration.py
 create mode 100644 pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py

diff --git a/pydis_site/apps/api/migrations/0044_active_infractions_migration.py b/pydis_site/apps/api/migrations/0044_active_infractions_migration.py
deleted file mode 100644
index 14746712..00000000
--- a/pydis_site/apps/api/migrations/0044_active_infractions_migration.py
+++ /dev/null
@@ -1,106 +0,0 @@
-# Generated by Django 2.2.6 on 2019-10-07 15:59
-
-from django.db import migrations
-from django.db.models import Count, Prefetch, Q, QuerySet
-
-
-class ExpirationWrapper:
-    """Wraps an expiration date to properly compare permanent and temporary infractions."""
-
-    def __init__(self, infraction):
-        self.expiration_date = infraction.expires_at
-
-    def __lt__(self, other):
-        """An `expiration_date` is considered smaller when it comes earlier than the `other`."""
-        if self.expiration_date is None:
-            # A permanent infraction can never end sooner than another infraction
-            return False
-        elif other.expiration_date is None:
-            # If `self` is temporary, but `other` is permanent, `self` is smaller
-            return True
-        else:
-            return self.expiration_date < other.expiration_date
-
-    def __eq__(self, other):
-        """If both expiration dates are permanent they're equal, otherwise compare dates."""
-        if self.expiration_date is None and other.expiration_date is None:
-            return True
-        elif self.expiration_date is None or other.expiration_date is None:
-            return False
-        else:
-            return self.expiration_date == other.expiration_date
-
-
-def migrate_inactive_types_to_inactive(apps, schema_editor):
-    """Migrates infractions of non-active types to inactive."""
-    inactive_infraction_types = Q(type="note") | Q(type="warning") | Q(type="kick")
-    infraction_model = apps.get_model('api', 'Infraction')
-    infraction_model.objects.filter(inactive_infraction_types).update(active=False)
-
-
-def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
-    """
-    Creates QuerySet to fetch users with multiple active infractions of the given `type`.
-
-    The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the
-    `User` instances.
-    """
-    active_infractions = infraction_model.objects.filter(type=infr_type, active=True)
-
-    # Build an SQL query by chaining methods together
-
-    # Get users with active infraction(s) of the provided `infr_type`
-    query = user_model.objects.filter(
-        Q(infractions_received__type=infr_type, infractions_received__active=True)
-    )
-
-    # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
-    query = query.prefetch_related(
-        Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions')
-    )
-
-    # Count and only include them if they have at least 2 active infractions of the `type`
-    query = query.annotate(num_infractions=Count('infractions_received'))
-    query = query.filter(num_infractions__gte=2)
-
-    # Make sure we return each individual only once
-    query = query.distinct()
-
-    return query
-
-
-def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor):
-    """
-    Make sure a user only has one active infraction of a given "active" infraction type.
-
-    If a user has multiple active infraction, we keep the one with longest expiration date active
-    and migrate the others to inactive.
-    """
-    infraction_model = apps.get_model('api', 'Infraction')
-    user_model = apps.get_model('api', 'User')
-
-    for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
-        query = get_query(user_model, infraction_model, infraction_type)
-        for user in query:
-            infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True)
-            for infraction in infractions[1:]:
-                infraction.active = False
-                infraction.save()
-
-
-def reverse_migration(apps, schema_editor):
-    """There's no need to do anything special to reverse these migrations."""
-    return
-
-
-class Migration(migrations.Migration):
-    """Data migration to get the database consistent with the new infraction validation rules."""
-
-    dependencies = [
-        ('api', '0043_infraction_hidden_warnings_to_notes'),
-    ]
-
-    operations = [
-        migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration),
-        migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration)
-    ]
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
deleted file mode 100644
index bacb56b8..00000000
--- a/pydis_site/apps/api/migrations/0045_add_infractions_unique_constraints_active.py
+++ /dev/null
@@ -1,17 +0,0 @@
-# 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/migrations/0046_active_infractions_migration.py b/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
new file mode 100644
index 00000000..7e5a631c
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
@@ -0,0 +1,106 @@
+# Generated by Django 2.2.6 on 2019-10-07 15:59
+
+from django.db import migrations
+from django.db.models import Count, Prefetch, Q, QuerySet
+
+
+class ExpirationWrapper:
+    """Wraps an expiration date to properly compare permanent and temporary infractions."""
+
+    def __init__(self, infraction):
+        self.expiration_date = infraction.expires_at
+
+    def __lt__(self, other):
+        """An `expiration_date` is considered smaller when it comes earlier than the `other`."""
+        if self.expiration_date is None:
+            # A permanent infraction can never end sooner than another infraction
+            return False
+        elif other.expiration_date is None:
+            # If `self` is temporary, but `other` is permanent, `self` is smaller
+            return True
+        else:
+            return self.expiration_date < other.expiration_date
+
+    def __eq__(self, other):
+        """If both expiration dates are permanent they're equal, otherwise compare dates."""
+        if self.expiration_date is None and other.expiration_date is None:
+            return True
+        elif self.expiration_date is None or other.expiration_date is None:
+            return False
+        else:
+            return self.expiration_date == other.expiration_date
+
+
+def migrate_inactive_types_to_inactive(apps, schema_editor):
+    """Migrates infractions of non-active types to inactive."""
+    inactive_infraction_types = Q(type="note") | Q(type="warning") | Q(type="kick")
+    infraction_model = apps.get_model('api', 'Infraction')
+    infraction_model.objects.filter(inactive_infraction_types).update(active=False)
+
+
+def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
+    """
+    Creates QuerySet to fetch users with multiple active infractions of the given `type`.
+
+    The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the
+    `User` instances.
+    """
+    active_infractions = infraction_model.objects.filter(type=infr_type, active=True)
+
+    # Build an SQL query by chaining methods together
+
+    # Get users with active infraction(s) of the provided `infr_type`
+    query = user_model.objects.filter(
+        Q(infractions_received__type=infr_type, infractions_received__active=True)
+    )
+
+    # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
+    query = query.prefetch_related(
+        Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions')
+    )
+
+    # Count and only include them if they have at least 2 active infractions of the `type`
+    query = query.annotate(num_infractions=Count('infractions_received'))
+    query = query.filter(num_infractions__gte=2)
+
+    # Make sure we return each individual only once
+    query = query.distinct()
+
+    return query
+
+
+def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor):
+    """
+    Make sure a user only has one active infraction of a given "active" infraction type.
+
+    If a user has multiple active infraction, we keep the one with longest expiration date active
+    and migrate the others to inactive.
+    """
+    infraction_model = apps.get_model('api', 'Infraction')
+    user_model = apps.get_model('api', 'User')
+
+    for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
+        query = get_query(user_model, infraction_model, infraction_type)
+        for user in query:
+            infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True)
+            for infraction in infractions[1:]:
+                infraction.active = False
+                infraction.save()
+
+
+def reverse_migration(apps, schema_editor):
+    """There's no need to do anything special to reverse these migrations."""
+    return
+
+
+class Migration(migrations.Migration):
+    """Data migration to get the database consistent with the new infraction validation rules."""
+
+    dependencies = [
+        ('api', '0045_add_plural_name_for_log_entry'),
+    ]
+
+    operations = [
+        migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration),
+        migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration)
+    ]
diff --git a/pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py b/pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py
new file mode 100644
index 00000000..dc266474
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0047_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', '0046_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/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
index cfae4219..61574b7e 100644
--- a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
+++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
@@ -81,8 +81,8 @@ class InfractionFactoryTests(MigrationsTestCase):
     """Tests for the InfractionFactory."""
 
     app = "api"
-    migration_prior = "0043_infraction_hidden_warnings_to_notes"
-    migration_target = "0043_infraction_hidden_warnings_to_notes"
+    migration_prior = "0045_add_plural_name_for_log_entry"
+    migration_target = "0045_add_plural_name_for_log_entry"
 
     @classmethod
     def setUpPostMigrationData(cls, apps):
@@ -183,8 +183,8 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
     """
 
     app = "api"
-    migration_prior = "0043_infraction_hidden_warnings_to_notes"
-    migration_target = "0044_active_infractions_migration"
+    migration_prior = "0045_add_plural_name_for_log_entry"
+    migration_target = "0046_active_infractions_migration"
 
     @classmethod
     def setUpMigrationData(cls, apps):
-- 
cgit v1.2.3


From 1ce639c6e560ff7aff405ad055e80b78efee8dfc Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Sat, 19 Oct 2019 17:17:39 +0200
Subject: Apply feedback for pull request #278

https://github.com/python-discord/site/pull/278

This commit applies the feedback given to PR #278. I have made a
number of significant changes:

- I have added tests for the `MigrationsTestCase` class, including a
  test to see if the test case travels to the specified points in the
  migration history.

- The dictionary of infracted users in the active migration tests now
  uses informative strings as keys. This makes it easier to relate a
  specific test to the data it's testing.

- I have simplified the migration query logic by replacing unneeded Q
  objects by simpler keyword arguments to `filter` method calls.

- I have removed the custom validation logic for allowing only one
  active infraction of a given type per user and replaced it by a
  `UniqueTogetherValidator` with a custom validation error message.

- `test_unique_constraint_accepts_active_infraction_after_inactive_
   infraction` relied on an Error results instead of a Fail result
   to indicate an issue with the database constraint. Since a fail
   should lead to a Fail status, I've replaced it by a try-except
   block that signals a test failure if an `IntegrityError` was
   caught by it.
---
 .../0046_active_infractions_migration.py           |   7 +-
 pydis_site/apps/api/serializers.py                 |  18 +-
 .../migrations/test_active_infraction_migration.py | 415 +++++++++------------
 pydis_site/apps/api/tests/migrations/test_base.py  | 135 +++++++
 pydis_site/apps/api/tests/test_infractions.py      |  71 ++--
 5 files changed, 373 insertions(+), 273 deletions(-)
 create mode 100644 pydis_site/apps/api/tests/migrations/test_base.py

diff --git a/pydis_site/apps/api/migrations/0046_active_infractions_migration.py b/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
index 7e5a631c..cc53f58f 100644
--- a/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
+++ b/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
@@ -1,7 +1,7 @@
 # Generated by Django 2.2.6 on 2019-10-07 15:59
 
 from django.db import migrations
-from django.db.models import Count, Prefetch, Q, QuerySet
+from django.db.models import Count, Prefetch, QuerySet
 
 
 class ExpirationWrapper:
@@ -33,9 +33,8 @@ class ExpirationWrapper:
 
 def migrate_inactive_types_to_inactive(apps, schema_editor):
     """Migrates infractions of non-active types to inactive."""
-    inactive_infraction_types = Q(type="note") | Q(type="warning") | Q(type="kick")
     infraction_model = apps.get_model('api', 'Infraction')
-    infraction_model.objects.filter(inactive_infraction_types).update(active=False)
+    infraction_model.objects.filter(type__in=('note', 'warning', 'kick')).update(active=False)
 
 
 def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
@@ -51,7 +50,7 @@ def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
 
     # Get users with active infraction(s) of the provided `infr_type`
     query = user_model.objects.filter(
-        Q(infractions_received__type=infr_type, infractions_received__active=True)
+        infractions_received__type=infr_type, infractions_received__active=True
     )
 
     # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py
index 784bb614..c3d5f696 100644
--- a/pydis_site/apps/api/serializers.py
+++ b/pydis_site/apps/api/serializers.py
@@ -1,7 +1,6 @@
 """Converters from Django models to data interchange formats and back."""
-import logging
-
 from rest_framework.serializers import ModelSerializer, PrimaryKeyRelatedField, ValidationError
+from rest_framework.validators import UniqueTogetherValidator
 from rest_framework_bulk import BulkSerializerMixin
 
 from .models import (
@@ -13,8 +12,6 @@ from .models import (
     Tag, User
 )
 
-log = logging.getLogger(__name__)
-
 
 class BotSettingSerializer(ModelSerializer):
     """A class providing (de-)serialization of `BotSetting` instances."""
@@ -108,6 +105,13 @@ class InfractionSerializer(ModelSerializer):
         fields = (
             'id', 'inserted_at', 'expires_at', 'active', 'user', 'actor', 'type', 'reason', 'hidden'
         )
+        validators = [
+            UniqueTogetherValidator(
+                queryset=Infraction.objects.filter(active=True),
+                fields=['user', 'type'],
+                message='This user already has an active infraction of this type.',
+            )
+        ]
 
     def validate(self, attrs: dict) -> dict:
         """Validate data constraints for the given data and abort if it is invalid."""
@@ -117,12 +121,6 @@ class InfractionSerializer(ModelSerializer):
         if active and infr_type in ('note', 'warning', 'kick'):
             raise ValidationError({'active': [f'{infr_type} infractions cannot be active.']})
 
-        user = attrs.get('user')
-        if active and Infraction.objects.filter(user=user, type=infr_type, active=True).exists():
-            raise ValidationError(
-                {'active': [f'This user already has an active {infr_type} infraction']}
-            )
-
         expires_at = attrs.get('expires_at')
         if expires_at and infr_type in ('kick', 'warning'):
             raise ValidationError({'expires_at': [f'{infr_type} infractions cannot expire.']})
diff --git a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
index 61574b7e..c17a2083 100644
--- a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
+++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
@@ -114,20 +114,15 @@ class InfractionFactoryTests(MigrationsTestCase):
 
     def test_infraction_factory_multiple_users(self):
         """Does the test database hold as many infractions as we tried to create?"""
-        created_infractions = {}
-        for user in range(5):
-            created_infractions.update(
-                {
-                    user: InfractionFactory.create(
-                        actor=self.actor,
-                        infractions=(
-                            {'type': 'kick', 'active': False, 'hidden': True},
-                            {'type': 'ban', 'active': True, 'hidden': False},
-                        ),
-                        infraction_model=self.infraction_model,
-                        user_model=self.user_model,
-                    )
-                }
+        for _user in range(5):
+            InfractionFactory.create(
+                actor=self.actor,
+                infractions=(
+                    {'type': 'kick', 'active': False, 'hidden': True},
+                    {'type': 'ban', 'active': True, 'hidden': False},
+                ),
+                infraction_model=self.infraction_model,
+                user_model=self.user_model,
             )
 
         # Check if infractions and users are recorded properly in the database
@@ -204,245 +199,199 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
         )
 
         # User #1: clean user with no infractions
-        cls.created_infractions.update(
-            {
-                1: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=[],
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["no infractions"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=[],
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #2: One inactive note infraction
-        cls.created_infractions.update(
-            {
-                2: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'note', 'active': False, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["one inactive note"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'note', 'active': False, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #3: One active note infraction
-        cls.created_infractions.update(
-            {
-                3: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'note', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["one active note"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'note', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #4: One active and one inactive note infraction
-        cls.created_infractions.update(
-            {
-                4: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'note', 'active': False, 'hidden': True},
-                        {'type': 'note', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["one active and one inactive note"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'note', 'active': False, 'hidden': True},
+                {'type': 'note', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #5: Once active note, one active kick, once active warning
-        cls.created_infractions.update(
-            {
-                5: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'note', 'active': True, 'hidden': True},
-                        {'type': 'kick', 'active': True, 'hidden': True},
-                        {'type': 'warning', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["active note, kick, warning"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'note', 'active': True, 'hidden': True},
+                {'type': 'kick', 'active': True, 'hidden': True},
+                {'type': 'warning', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #6: One inactive ban and one active ban
-        cls.created_infractions.update(
-            {
-                6: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'ban', 'active': False, 'hidden': True},
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["one inactive and one active ban"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'ban', 'active': False, 'hidden': True},
+                {'type': 'ban', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #7: Two active permanent bans
-        cls.created_infractions.update(
-            {
-                7: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["two active perm bans"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'ban', 'active': True, 'hidden': True},
+                {'type': 'ban', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #8: Multiple active temporary bans
-        cls.created_infractions.update(
-            {
-                8: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=1)
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=10)
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=20)
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=5)
-                        },
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["multiple active temp bans"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=1)
+                },
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=10)
+                },
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=20)
+                },
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=5)
+                },
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #9: One active permanent ban, two active temporary bans
-        cls.created_infractions.update(
-            {
-                9: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=10)
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': None,
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=7)
-                        },
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["active perm, two active temp bans"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=10)
+                },
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': None,
+                },
+                {
+                    'type': 'ban',
+                    'active': True,
+                    'hidden': True,
+                    'expires_at': timezone.now() + timedelta(days=7)
+                },
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #10: One inactive permanent ban, two active temporary bans
-        cls.created_infractions.update(
-            {
-                10: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=10)
-                        },
-                        {
-                            'type': 'ban',
-                            'active': False,
-                            'hidden': True,
-                            'expires_at': None,
-                        },
-                        {
-                            'type': 'ban',
-                            'active': True,
-                            'hidden': True,
-                            'expires_at': timezone.now() + timedelta(days=7)
-                        },
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["one inactive perm ban, two active temp bans"] = (
+            InfractionFactory.create(
+                actor=cls.user_moderator,
+                infractions=(
+                    {
+                        'type': 'ban',
+                        'active': True,
+                        'hidden': True,
+                        'expires_at': timezone.now() + timedelta(days=10)
+                    },
+                    {
+                        'type': 'ban',
+                        'active': False,
+                        'hidden': True,
+                        'expires_at': None,
+                    },
+                    {
+                        'type': 'ban',
+                        'active': True,
+                        'hidden': True,
+                        'expires_at': timezone.now() + timedelta(days=7)
+                    },
+                ),
+                infraction_model=cls.infraction_model,
+                user_model=cls.user_model,
+            )
         )
 
         # User #11: Active ban, active mute, active superstar
-        cls.created_infractions.update(
-            {
-                11: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                        {'type': 'mute', 'active': True, 'hidden': True},
-                        {'type': 'superstar', 'active': True, 'hidden': True},
-                        {'type': 'watch', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["active ban, mute, and superstar"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'ban', 'active': True, 'hidden': True},
+                {'type': 'mute', 'active': True, 'hidden': True},
+                {'type': 'superstar', 'active': True, 'hidden': True},
+                {'type': 'watch', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
         # User #12: Multiple active bans, active mutes, active superstars
-        cls.created_infractions.update(
-            {
-                12: InfractionFactory.create(
-                    actor=cls.user_moderator,
-                    infractions=(
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                        {'type': 'ban', 'active': True, 'hidden': True},
-                        {'type': 'mute', 'active': True, 'hidden': True},
-                        {'type': 'mute', 'active': True, 'hidden': True},
-                        {'type': 'mute', 'active': True, 'hidden': True},
-                        {'type': 'superstar', 'active': True, 'hidden': True},
-                        {'type': 'superstar', 'active': True, 'hidden': True},
-                        {'type': 'superstar', 'active': True, 'hidden': True},
-                        {'type': 'watch', 'active': True, 'hidden': True},
-                        {'type': 'watch', 'active': True, 'hidden': True},
-                        {'type': 'watch', 'active': True, 'hidden': True},
-                    ),
-                    infraction_model=cls.infraction_model,
-                    user_model=cls.user_model,
-                )
-            }
+        cls.created_infractions["multiple active bans, mutes, stars"] = InfractionFactory.create(
+            actor=cls.user_moderator,
+            infractions=(
+                {'type': 'ban', 'active': True, 'hidden': True},
+                {'type': 'ban', 'active': True, 'hidden': True},
+                {'type': 'ban', 'active': True, 'hidden': True},
+                {'type': 'mute', 'active': True, 'hidden': True},
+                {'type': 'mute', 'active': True, 'hidden': True},
+                {'type': 'mute', 'active': True, 'hidden': True},
+                {'type': 'superstar', 'active': True, 'hidden': True},
+                {'type': 'superstar', 'active': True, 'hidden': True},
+                {'type': 'superstar', 'active': True, 'hidden': True},
+                {'type': 'watch', 'active': True, 'hidden': True},
+                {'type': 'watch', 'active': True, 'hidden': True},
+                {'type': 'watch', 'active': True, 'hidden': True},
+            ),
+            infraction_model=cls.infraction_model,
+            user_model=cls.user_model,
         )
 
     def test_all_never_active_types_became_inactive(self):
@@ -454,14 +403,14 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
 
     def test_migration_left_clean_user_without_infractions(self):
         """Do users without infractions have no infractions after the migration?"""
-        user_id, infraction_history = self.created_infractions[1]
+        user_id, infraction_history = self.created_infractions["no infractions"]
         self.assertFalse(
             self.infraction_model.objects.filter(user__id=user_id).exists()
         )
 
     def test_migration_left_user_with_inactive_note_untouched(self):
         """Did the migration leave users with only an inactive note untouched?"""
-        user_id, infraction_history = self.created_infractions[2]
+        user_id, infraction_history = self.created_infractions["one inactive note"]
         inactive_note = infraction_history[0]
         self.assertTrue(
             self.infraction_model.objects.filter(**model_to_dict(inactive_note)).exists()
@@ -469,7 +418,7 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
 
     def test_migration_only_touched_active_field_of_active_note(self):
         """Does the migration only change the `active` field?"""
-        user_id, infraction_history = self.created_infractions[3]
+        user_id, infraction_history = self.created_infractions["one active note"]
         note = model_to_dict(infraction_history[0])
         note['active'] = False
         self.assertTrue(
@@ -478,7 +427,7 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
 
     def test_migration_only_touched_active_field_of_active_note_left_inactive_untouched(self):
         """Does the migration only change the `active` field of active notes?"""
-        user_id, infraction_history = self.created_infractions[4]
+        user_id, infraction_history = self.created_infractions["one active and one inactive note"]
         for note in infraction_history:
             with self.subTest(active=note.active):
                 note = model_to_dict(note)
@@ -489,14 +438,14 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
 
     def test_migration_migrates_all_nonactive_types_to_inactive(self):
         """Do we set the `active` field of all non-active infractions to `False`?"""
-        user_id, infraction_history = self.created_infractions[5]
+        user_id, infraction_history = self.created_infractions["active note, kick, warning"]
         self.assertFalse(
             self.infraction_model.objects.filter(user__id=user_id, active=True).exists()
         )
 
     def test_migration_leaves_user_with_one_active_ban_untouched(self):
         """Do we leave a user with one active and one inactive ban untouched?"""
-        user_id, infraction_history = self.created_infractions[6]
+        user_id, infraction_history = self.created_infractions["one inactive and one active ban"]
         for infraction in infraction_history:
             with self.subTest(active=infraction.active):
                 self.assertTrue(
@@ -505,37 +454,39 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
 
     def test_migration_turns_double_active_perm_ban_into_single_active_perm_ban(self):
         """Does the migration turn two active permanent bans into one active permanent ban?"""
-        user_id, infraction_history = self.created_infractions[7]
+        user_id, infraction_history = self.created_infractions["two active perm bans"]
         active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count()
         self.assertEqual(active_count, 1)
 
     def test_migration_leaves_temporary_ban_with_longest_duration_active(self):
         """Does the migration turn two active permanent bans into one active permanent ban?"""
-        user_id, infraction_history = self.created_infractions[8]
+        user_id, infraction_history = self.created_infractions["multiple active temp bans"]
         active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
         self.assertEqual(active_ban.expires_at, infraction_history[2].expires_at)
 
     def test_migration_leaves_permanent_ban_active(self):
         """Does the migration leave the permanent ban active?"""
-        user_id, infraction_history = self.created_infractions[9]
+        user_id, infraction_history = self.created_infractions["active perm, two active temp bans"]
         active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
         self.assertIsNone(active_ban.expires_at)
 
     def test_migration_leaves_longest_temp_ban_active_with_inactive_permanent_ban(self):
         """Does the longest temp ban stay active, even with an inactive perm ban present?"""
-        user_id, infraction_history = self.created_infractions[10]
+        user_id, infraction_history = self.created_infractions[
+            "one inactive perm ban, two active temp bans"
+        ]
         active_ban = self.infraction_model.objects.get(user__id=user_id, active=True)
         self.assertEqual(active_ban.expires_at, infraction_history[0].expires_at)
 
     def test_migration_leaves_all_active_types_active_if_one_of_each_exists(self):
         """Do all active infractions stay active if only one of each is present?"""
-        user_id, infraction_history = self.created_infractions[11]
+        user_id, infraction_history = self.created_infractions["active ban, mute, and superstar"]
         active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count()
         self.assertEqual(active_count, 4)
 
     def test_migration_reduces_all_active_types_to_a_single_active_infraction(self):
         """Do we reduce all of the infraction types to one active infraction?"""
-        user_id, infraction_history = self.created_infractions[12]
+        user_id, infraction_history = self.created_infractions["multiple active bans, mutes, stars"]
         active_infractions = self.infraction_model.objects.filter(user__id=user_id, active=True)
         self.assertEqual(len(active_infractions), 4)
         types_observed = [infraction.type for infraction in active_infractions]
diff --git a/pydis_site/apps/api/tests/migrations/test_base.py b/pydis_site/apps/api/tests/migrations/test_base.py
new file mode 100644
index 00000000..12677720
--- /dev/null
+++ b/pydis_site/apps/api/tests/migrations/test_base.py
@@ -0,0 +1,135 @@
+import logging
+from unittest.mock import call, patch
+
+from django.db.migrations.loader import MigrationLoader
+from django.test import TestCase
+
+from .base import MigrationsTestCase, connection
+
+log = logging.getLogger(__name__)
+
+
+class SpanishInquisition(MigrationsTestCase):
+    app = "api"
+    migration_prior = "scragly"
+    migration_target = "kosa"
+
+
+@patch("pydis_site.apps.api.tests.migrations.base.MigrationExecutor")
+class MigrationsTestCaseNoSideEffectsTests(TestCase):
+    """Tests the MigrationTestCase class with actual migration side effects disabled."""
+
+    def setUp(self):
+        """Set up an instance of MigrationsTestCase for use in tests."""
+        self.test_case = SpanishInquisition()
+
+    def test_missing_app_class_raises_value_error(self, _migration_executor):
+        """A MigrationsTestCase subclass should set the class-attribute `app`."""
+        class Spam(MigrationsTestCase):
+            pass
+
+        spam = Spam()
+        with self.assertRaises(ValueError, msg="The `app` attribute was not set."):
+            spam.setUpTestData()
+
+    def test_missing_migration_class_attributes_raise_value_error(self, _migration_executor):
+        """A MigrationsTestCase subclass should set both `migration_prior` and `migration_target`"""
+        class Eggs(MigrationsTestCase):
+            app = "api"
+            migration_target = "lemon"
+
+        class Bacon(MigrationsTestCase):
+            app = "api"
+            migration_prior = "mark"
+
+        instances = (Eggs(), Bacon())
+
+        exception_message = "Both ` migration_prior` and `migration_target` need to be set."
+        for instance in instances:
+            with self.subTest(
+                    migration_prior=instance.migration_prior,
+                    migration_target=instance.migration_target,
+            ):
+                with self.assertRaises(ValueError, msg=exception_message):
+                    instance.setUpTestData()
+
+    @patch(f"{__name__}.SpanishInquisition.setUpMigrationData")
+    @patch(f"{__name__}.SpanishInquisition.setUpPostMigrationData")
+    def test_migration_data_hooks_are_called_once(self, pre_hook, post_hook, _migration_executor):
+        """The `setUpMigrationData` and `setUpPostMigrationData` hooks should be called once."""
+        self.test_case.setUpTestData()
+        for hook in (pre_hook, post_hook):
+            with self.subTest(hook=repr(hook)):
+                hook.assert_called_once()
+
+    def test_migration_executor_is_instantiated_twice(self, migration_executor):
+        """The `MigrationExecutor` should be instantiated with the database connection twice."""
+        self.test_case.setUpTestData()
+
+        expected_args = [call(connection), call(connection)]
+        self.assertEqual(migration_executor.call_args_list, expected_args)
+
+    def test_project_state_is_loaded_for_correct_migration_files_twice(self, migration_executor):
+        """The `project_state` should first be loaded with `migrate_from`, then `migrate_to`."""
+        self.test_case.setUpTestData()
+
+        expected_args = [call(self.test_case.migrate_from), call(self.test_case.migrate_to)]
+        self.assertEqual(migration_executor().loader.project_state.call_args_list, expected_args)
+
+    def test_loader_build_graph_gets_called_once(self, migration_executor):
+        """We should rebuild the before applying the second set of migrations."""
+        self.test_case.setUpTestData()
+
+        migration_executor().loader.build_graph.assert_called_once()
+
+    def test_migration_executor_migrate_method_is_called_correctly_twice(self, migration_executor):
+        """The migrate method of the executor should be called twice with the correct arguments."""
+        self.test_case.setUpTestData()
+
+        self.assertEqual(migration_executor().migrate.call_count, 2)
+        calls = [call([('api', 'scragly')]), call([('api', 'kosa')])]
+        migration_executor().migrate.assert_has_calls(calls)
+
+
+class LifeOfBrian(MigrationsTestCase):
+    app = "api"
+    migration_prior = "0045_add_plural_name_for_log_entry"
+    migration_target = "0047_add_infractions_unique_constraints_active"
+
+    @classmethod
+    def log_last_migration(cls):
+        """Parses the applied migrations dictionary to log the last applied migration."""
+        loader = MigrationLoader(connection)
+        api_migrations = [
+            migration for app, migration in loader.applied_migrations if app == cls.app
+        ]
+        last_migration = max(api_migrations, key=lambda name: int(name[:4]))
+        log.info(f"The last applied migration: {last_migration}")
+
+    @classmethod
+    def setUpMigrationData(cls, apps):
+        """Method that logs the last applied migration at this point."""
+        cls.log_last_migration()
+
+    @classmethod
+    def setUpPostMigrationData(cls, apps):
+        """Method that logs the last applied migration at this point."""
+        cls.log_last_migration()
+
+
+class MigrationsTestCaseMigrationTest(TestCase):
+    """Tests if `MigrationsTestCase` travels to the right points in the migration history."""
+
+    def test_migrations_test_case_travels_to_correct_migrations_in_history(self):
+        """The test case should first revert to `migration_prior`, then go to `migration_target`."""
+        brian = LifeOfBrian()
+
+        with self.assertLogs(log, level=logging.INFO) as logs:
+            brian.setUpTestData()
+
+            self.assertEqual(len(logs.records), 2)
+
+            for time_point, record in zip(("migration_prior", "migration_target"), logs.records):
+                with self.subTest(time_point=time_point):
+                    message = f"The last applied migration: {getattr(brian, time_point)}"
+                    self.assertEqual(record.getMessage(), message)
diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py
index 69d9ebea..f1591b4b 100644
--- a/pydis_site/apps/api/tests/test_infractions.py
+++ b/pydis_site/apps/api/tests/test_infractions.py
@@ -1,4 +1,5 @@
 from datetime import datetime as dt, timedelta, timezone
+from unittest.mock import patch
 from urllib.parse import quote
 
 from django.db.utils import IntegrityError
@@ -330,9 +331,10 @@ class CreationTests(APISubdomainTestCase):
                 }
                 response = self.client.post(url, data=invalid_infraction)
                 self.assertEqual(response.status_code, 400)
-                self.assertEqual(response.json(), {
-                    'active': [f'{infraction_type} infractions cannot be active.']
-                })
+                self.assertEqual(
+                    response.json(),
+                    {'active': [f'{infraction_type} infractions cannot be active.']}
+                )
 
     def test_returns_400_for_second_active_infraction_of_the_same_type(self):
         """Test if the API rejects a second active infraction of the same type for a given user."""
@@ -364,9 +366,14 @@ class CreationTests(APISubdomainTestCase):
                 }
                 second_response = self.client.post(url, data=second_active_infraction)
                 self.assertEqual(second_response.status_code, 400)
-                self.assertEqual(second_response.json(), {
-                    'active': [f'This user already has an active {infraction_type} infraction']
-                })
+                self.assertEqual(
+                    second_response.json(),
+                    {
+                        'non_field_errors': [
+                            'This user already has an active infraction of this type.'
+                        ]
+                    }
+                )
 
     def test_returns_201_for_second_active_infraction_of_different_type(self):
         """Test if the API accepts a second active infraction of a different type than the first."""
@@ -417,27 +424,37 @@ class CreationTests(APISubdomainTestCase):
 
     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"
-        )
+        try:
+            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"
+            )
+        except IntegrityError:
+            self.fail("An unexpected IntegrityError was raised.")
+
+    @patch(f"{__name__}.Infraction")
+    def test_the_accepts_active_infraction_after_inactive_infractions_test(self, infraction_patch):
+        """Does the test properly catch the IntegrityError and raise an AssertionError?"""
+        infraction_patch.objects.create.side_effect = IntegrityError
+        with self.assertRaises(AssertionError, msg="An unexpected IntegrityError was raised."):
+            self.test_unique_constraint_accepts_active_infraction_after_inactive_infraction()
 
     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?"""
-- 
cgit v1.2.3


From 13083b3b9961ed986d8f614baf4a55f71426b5c9 Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Sat, 19 Oct 2019 19:32:55 +0200
Subject: Fix indentation and missing word in docstring

The last two lines in the docstring of `InfractionFactory` in the
`test_active_infraction_migration.py` file were overindented by one
space. I've removed the space.

The docstring of the `test_loader_build_graph_gets_called_once`
method in the `MigrationsTestCaseNoSideEffectsTests` calss of the
`test_base.py` file was missing two words. Corrected.
---
 .../apps/api/tests/migrations/test_active_infraction_migration.py     | 4 ++--
 pydis_site/apps/api/tests/migrations/test_base.py                     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
index c17a2083..2d5fd94c 100644
--- a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
+++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
@@ -47,8 +47,8 @@ class InfractionFactory:
          - `expires_at` (default: None; i.e, permanent)
          - `hidden` (default: False).
 
-         The parameters `infraction_model` and `user_model` can be used to pass in an instance of
-         both model classes from a different migration/project state.
+        The parameters `infraction_model` and `user_model` can be used to pass in an instance of
+        both model classes from a different migration/project state.
         """
         user_id = next(cls.user_id)
         user = user_model.objects.create(
diff --git a/pydis_site/apps/api/tests/migrations/test_base.py b/pydis_site/apps/api/tests/migrations/test_base.py
index 12677720..5d38e2dd 100644
--- a/pydis_site/apps/api/tests/migrations/test_base.py
+++ b/pydis_site/apps/api/tests/migrations/test_base.py
@@ -77,7 +77,7 @@ class MigrationsTestCaseNoSideEffectsTests(TestCase):
         self.assertEqual(migration_executor().loader.project_state.call_args_list, expected_args)
 
     def test_loader_build_graph_gets_called_once(self, migration_executor):
-        """We should rebuild the before applying the second set of migrations."""
+        """We should rebuild the migration graph before applying the second set of migrations."""
         self.test_case.setUpTestData()
 
         migration_executor().loader.build_graph.assert_called_once()
-- 
cgit v1.2.3


From 9989170efe2f4afced40defc346179d046ebb9aa Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Sat, 19 Oct 2019 20:18:40 +0200
Subject: Make test less fragile and improve test name

The `test_returns_400_for_active_infractions_of_type_that_cannot_be_
active` test relied on the order in which the validation was done
since it contained incompatible combinations of arguments. The test
has been changed to make sure the data is valid except for the thing
we actually want to test.

I have also tried to improve the name of the test that tests the
`test_unique_constraint_accepts_active_infraction_after_inactive_
infraction` test. It now includes the logic of what it does, but
not the entire name of the test it's testing.
---
 pydis_site/apps/api/tests/test_infractions.py | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py
index f1591b4b..7a54640e 100644
--- a/pydis_site/apps/api/tests/test_infractions.py
+++ b/pydis_site/apps/api/tests/test_infractions.py
@@ -316,18 +316,22 @@ class CreationTests(APISubdomainTestCase):
     def test_returns_400_for_active_infraction_of_type_that_cannot_be_active(self):
         """Test if the API rejects active infractions for types that cannot be active."""
         url = reverse('bot:infraction-list', host='api')
-        restricted_types = ('note', 'warning', 'kick')
+        restricted_types = (
+            ('note', True),
+            ('warning', False),
+            ('kick', False),
+        )
 
-        for infraction_type in restricted_types:
+        for infraction_type, hidden in restricted_types:
             with self.subTest(infraction_type=infraction_type):
                 invalid_infraction = {
                     'user': self.user.id,
                     'actor': self.user.id,
                     'type': infraction_type,
                     'reason': 'Take me on!',
-                    'hidden': True,
+                    'hidden': hidden,
                     'active': True,
-                    'expires_at': '2019-10-04T12:52:00+00:00'
+                    'expires_at': None,
                 }
                 response = self.client.post(url, data=invalid_infraction)
                 self.assertEqual(response.status_code, 400)
@@ -450,7 +454,7 @@ class CreationTests(APISubdomainTestCase):
             self.fail("An unexpected IntegrityError was raised.")
 
     @patch(f"{__name__}.Infraction")
-    def test_the_accepts_active_infraction_after_inactive_infractions_test(self, infraction_patch):
+    def test_if_accepts_active_infraction_test_catches_integrity_error(self, infraction_patch):
         """Does the test properly catch the IntegrityError and raise an AssertionError?"""
         infraction_patch.objects.create.side_effect = IntegrityError
         with self.assertRaises(AssertionError, msg="An unexpected IntegrityError was raised."):
-- 
cgit v1.2.3


From 48617b0425bb6b4c7f253f9b44363432658c2f1c Mon Sep 17 00:00:00 2001
From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Date: Sun, 3 Nov 2019 21:08:33 +0100
Subject: Resolve migration merge conflicts

Since other pull requests were merged that also included migrations
for the API app, this PR needed to be updated to avoid conflicts in
the migration history. In addition, the test files contained names of
specific migration files that needed to be updated to the correct
names after the merge resolution.
---
 .../0046_active_infractions_migration.py           | 105 ---------------------
 .../0047_active_infractions_migration.py           | 105 +++++++++++++++++++++
 ...47_add_infractions_unique_constraints_active.py |  17 ----
 ...48_add_infractions_unique_constraints_active.py |  17 ++++
 .../migrations/test_active_infraction_migration.py |   8 +-
 pydis_site/apps/api/tests/migrations/test_base.py  |   4 +-
 6 files changed, 128 insertions(+), 128 deletions(-)
 delete mode 100644 pydis_site/apps/api/migrations/0046_active_infractions_migration.py
 create mode 100644 pydis_site/apps/api/migrations/0047_active_infractions_migration.py
 delete mode 100644 pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py
 create mode 100644 pydis_site/apps/api/migrations/0048_add_infractions_unique_constraints_active.py

diff --git a/pydis_site/apps/api/migrations/0046_active_infractions_migration.py b/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
deleted file mode 100644
index cc53f58f..00000000
--- a/pydis_site/apps/api/migrations/0046_active_infractions_migration.py
+++ /dev/null
@@ -1,105 +0,0 @@
-# Generated by Django 2.2.6 on 2019-10-07 15:59
-
-from django.db import migrations
-from django.db.models import Count, Prefetch, QuerySet
-
-
-class ExpirationWrapper:
-    """Wraps an expiration date to properly compare permanent and temporary infractions."""
-
-    def __init__(self, infraction):
-        self.expiration_date = infraction.expires_at
-
-    def __lt__(self, other):
-        """An `expiration_date` is considered smaller when it comes earlier than the `other`."""
-        if self.expiration_date is None:
-            # A permanent infraction can never end sooner than another infraction
-            return False
-        elif other.expiration_date is None:
-            # If `self` is temporary, but `other` is permanent, `self` is smaller
-            return True
-        else:
-            return self.expiration_date < other.expiration_date
-
-    def __eq__(self, other):
-        """If both expiration dates are permanent they're equal, otherwise compare dates."""
-        if self.expiration_date is None and other.expiration_date is None:
-            return True
-        elif self.expiration_date is None or other.expiration_date is None:
-            return False
-        else:
-            return self.expiration_date == other.expiration_date
-
-
-def migrate_inactive_types_to_inactive(apps, schema_editor):
-    """Migrates infractions of non-active types to inactive."""
-    infraction_model = apps.get_model('api', 'Infraction')
-    infraction_model.objects.filter(type__in=('note', 'warning', 'kick')).update(active=False)
-
-
-def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
-    """
-    Creates QuerySet to fetch users with multiple active infractions of the given `type`.
-
-    The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the
-    `User` instances.
-    """
-    active_infractions = infraction_model.objects.filter(type=infr_type, active=True)
-
-    # Build an SQL query by chaining methods together
-
-    # Get users with active infraction(s) of the provided `infr_type`
-    query = user_model.objects.filter(
-        infractions_received__type=infr_type, infractions_received__active=True
-    )
-
-    # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
-    query = query.prefetch_related(
-        Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions')
-    )
-
-    # Count and only include them if they have at least 2 active infractions of the `type`
-    query = query.annotate(num_infractions=Count('infractions_received'))
-    query = query.filter(num_infractions__gte=2)
-
-    # Make sure we return each individual only once
-    query = query.distinct()
-
-    return query
-
-
-def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor):
-    """
-    Make sure a user only has one active infraction of a given "active" infraction type.
-
-    If a user has multiple active infraction, we keep the one with longest expiration date active
-    and migrate the others to inactive.
-    """
-    infraction_model = apps.get_model('api', 'Infraction')
-    user_model = apps.get_model('api', 'User')
-
-    for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
-        query = get_query(user_model, infraction_model, infraction_type)
-        for user in query:
-            infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True)
-            for infraction in infractions[1:]:
-                infraction.active = False
-                infraction.save()
-
-
-def reverse_migration(apps, schema_editor):
-    """There's no need to do anything special to reverse these migrations."""
-    return
-
-
-class Migration(migrations.Migration):
-    """Data migration to get the database consistent with the new infraction validation rules."""
-
-    dependencies = [
-        ('api', '0045_add_plural_name_for_log_entry'),
-    ]
-
-    operations = [
-        migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration),
-        migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration)
-    ]
diff --git a/pydis_site/apps/api/migrations/0047_active_infractions_migration.py b/pydis_site/apps/api/migrations/0047_active_infractions_migration.py
new file mode 100644
index 00000000..9ac791dc
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0047_active_infractions_migration.py
@@ -0,0 +1,105 @@
+# Generated by Django 2.2.6 on 2019-10-07 15:59
+
+from django.db import migrations
+from django.db.models import Count, Prefetch, QuerySet
+
+
+class ExpirationWrapper:
+    """Wraps an expiration date to properly compare permanent and temporary infractions."""
+
+    def __init__(self, infraction):
+        self.expiration_date = infraction.expires_at
+
+    def __lt__(self, other):
+        """An `expiration_date` is considered smaller when it comes earlier than the `other`."""
+        if self.expiration_date is None:
+            # A permanent infraction can never end sooner than another infraction
+            return False
+        elif other.expiration_date is None:
+            # If `self` is temporary, but `other` is permanent, `self` is smaller
+            return True
+        else:
+            return self.expiration_date < other.expiration_date
+
+    def __eq__(self, other):
+        """If both expiration dates are permanent they're equal, otherwise compare dates."""
+        if self.expiration_date is None and other.expiration_date is None:
+            return True
+        elif self.expiration_date is None or other.expiration_date is None:
+            return False
+        else:
+            return self.expiration_date == other.expiration_date
+
+
+def migrate_inactive_types_to_inactive(apps, schema_editor):
+    """Migrates infractions of non-active types to inactive."""
+    infraction_model = apps.get_model('api', 'Infraction')
+    infraction_model.objects.filter(type__in=('note', 'warning', 'kick')).update(active=False)
+
+
+def get_query(user_model, infraction_model, infr_type: str) -> QuerySet:
+    """
+    Creates QuerySet to fetch users with multiple active infractions of the given `type`.
+
+    The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the
+    `User` instances.
+    """
+    active_infractions = infraction_model.objects.filter(type=infr_type, active=True)
+
+    # Build an SQL query by chaining methods together
+
+    # Get users with active infraction(s) of the provided `infr_type`
+    query = user_model.objects.filter(
+        infractions_received__type=infr_type, infractions_received__active=True
+    )
+
+    # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute
+    query = query.prefetch_related(
+        Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions')
+    )
+
+    # Count and only include them if they have at least 2 active infractions of the `type`
+    query = query.annotate(num_infractions=Count('infractions_received'))
+    query = query.filter(num_infractions__gte=2)
+
+    # Make sure we return each individual only once
+    query = query.distinct()
+
+    return query
+
+
+def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor):
+    """
+    Make sure a user only has one active infraction of a given "active" infraction type.
+
+    If a user has multiple active infraction, we keep the one with longest expiration date active
+    and migrate the others to inactive.
+    """
+    infraction_model = apps.get_model('api', 'Infraction')
+    user_model = apps.get_model('api', 'User')
+
+    for infraction_type in ('ban', 'mute', 'superstar', 'watch'):
+        query = get_query(user_model, infraction_model, infraction_type)
+        for user in query:
+            infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True)
+            for infraction in infractions[1:]:
+                infraction.active = False
+                infraction.save()
+
+
+def reverse_migration(apps, schema_editor):
+    """There's no need to do anything special to reverse these migrations."""
+    return
+
+
+class Migration(migrations.Migration):
+    """Data migration to get the database consistent with the new infraction validation rules."""
+
+    dependencies = [
+        ('api', '0046_reminder_jump_url'),
+    ]
+
+    operations = [
+        migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration),
+        migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration)
+    ]
diff --git a/pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py b/pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py
deleted file mode 100644
index dc266474..00000000
--- a/pydis_site/apps/api/migrations/0047_add_infractions_unique_constraints_active.py
+++ /dev/null
@@ -1,17 +0,0 @@
-# Generated by Django 2.2.6 on 2019-10-07 18:27
-
-from django.db import migrations, models
-
-
-class Migration(migrations.Migration):
-
-    dependencies = [
-        ('api', '0046_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/migrations/0048_add_infractions_unique_constraints_active.py b/pydis_site/apps/api/migrations/0048_add_infractions_unique_constraints_active.py
new file mode 100644
index 00000000..4ea1fb90
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0048_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', '0047_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/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
index 2d5fd94c..8dc29b34 100644
--- a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
+++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py
@@ -81,8 +81,8 @@ class InfractionFactoryTests(MigrationsTestCase):
     """Tests for the InfractionFactory."""
 
     app = "api"
-    migration_prior = "0045_add_plural_name_for_log_entry"
-    migration_target = "0045_add_plural_name_for_log_entry"
+    migration_prior = "0046_reminder_jump_url"
+    migration_target = "0046_reminder_jump_url"
 
     @classmethod
     def setUpPostMigrationData(cls, apps):
@@ -178,8 +178,8 @@ class ActiveInfractionMigrationTests(MigrationsTestCase):
     """
 
     app = "api"
-    migration_prior = "0045_add_plural_name_for_log_entry"
-    migration_target = "0046_active_infractions_migration"
+    migration_prior = "0046_reminder_jump_url"
+    migration_target = "0047_active_infractions_migration"
 
     @classmethod
     def setUpMigrationData(cls, apps):
diff --git a/pydis_site/apps/api/tests/migrations/test_base.py b/pydis_site/apps/api/tests/migrations/test_base.py
index 5d38e2dd..f69bc92c 100644
--- a/pydis_site/apps/api/tests/migrations/test_base.py
+++ b/pydis_site/apps/api/tests/migrations/test_base.py
@@ -93,8 +93,8 @@ class MigrationsTestCaseNoSideEffectsTests(TestCase):
 
 class LifeOfBrian(MigrationsTestCase):
     app = "api"
-    migration_prior = "0045_add_plural_name_for_log_entry"
-    migration_target = "0047_add_infractions_unique_constraints_active"
+    migration_prior = "0046_reminder_jump_url"
+    migration_target = "0048_add_infractions_unique_constraints_active"
 
     @classmethod
     def log_last_migration(cls):
-- 
cgit v1.2.3