diff options
author | 2019-10-19 17:17:39 +0200 | |
---|---|---|
committer | 2019-10-19 17:17:39 +0200 | |
commit | 1ce639c6e560ff7aff405ad055e80b78efee8dfc (patch) | |
tree | 9f1f5cce9e0e47c372daed8764053d22f714a920 /pydis_site | |
parent | Solve migration conflict by renaming migrations (diff) |
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.
Diffstat (limited to 'pydis_site')
5 files changed, 373 insertions, 273 deletions
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?""" |