Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation for active infractions #278

Open
wants to merge 3 commits into
base: master
from

Conversation

@SebastiaanZ
Copy link
Member

commented Oct 7, 2019

This pull request needs to be manually merged after #269 has been merged to avoid conflicting migration files.

This pull request adds validation rules to the API to validate infractions based on the active field. This means that the API will now reject active infractions of types that should never be active (notes, warnings, and kicks) and only accept one active infraction per user for the other types of infractions (mute, ban, watch, superstar).

In addition, this pull requests adds a data migration to make sure the database itself is consistent with the new validation rules. Since this is a fairly complex data migration, I have added tests to make sure the migration works as expected. (Read more about that below.)

Finally, I've introduced a database-level UniqueConstraint to prevent a user from having multiple active infractions of the same type. I have also added tests to make sure this constraint works as expected.

Testing migrations

To test the effect of the migration file, I've created a subclass of django.test.TestCase, MigrationsTestCase, that reverts the test database to the migration just prior to the one of interest. We can then inject test data, apply the migration, and test if the migration had the desired effect.

Since a migration is really snapshot in time of the migration history, this also allows us to work with the database models as they were at those two time points in the migration history (directly before and directly after applying this migration), since otherwise these tests will fail in the future if we decide to change the models.

SebastiaanZ added 3 commits Oct 4, 2019
#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
#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
#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.
@@ -1,4 +1,5 @@
"""Converters from Django models to data interchange formats and back."""
import logging

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 7, 2019

Author Member

I've accidentally left this logging import here; I'll remove it when pushing the commit that straightens out the migration conflict that #269 will cause.

@@ -12,6 +13,8 @@
Tag, User
)

log = logging.getLogger(__name__)

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 7, 2019

Author Member

I'll remove this then as well, obviously.

'reason': 'Take me on!',
'hidden': True,
'active': True,
'expires_at': '2019-10-04T12:52:00+00:00'

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

This test is a bit fragile in the sense that it relies on the active validation logic preceding the validation for allowing expiration or being hidden. But I think I can live with that.

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():

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

How come a UniqueValidator wasn't used instead? Also, supposedly a ModelSerializer does this:

It will automatically generate validators for the serializer, such as unique_together validators.

I wonder, does it support generating validators for unique constraints on the model?

reason="The second active ban"
)

def test_unique_constraint_accepts_active_infraction_after_inactive_infraction(self):

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

Since this and the following test cases have no assertions, it means you're relying on IntegrityError not being raised? If it was raised (like any other exception), it'd fail the test, right? Sorry if that has an obvious answer, just making sure here.

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 9, 2019

Author Member

I will change this to a more explicit variant. I've looked around for how to properly test a assertNotRaises situation, since there's no built-in way to do that with unittest, but I'm finding conflicting answers.

Some advise to just run it, like I've done here, and rely on the test resulting in Error if an exception occurs, but other argue, and I think I agree with that, that observing an exception where non should occur should result in a test Failure instead.

One solution would be to do something like:

try:
    # the model creation
except IntegrityError:
    self.fail("Raised IntegrityError unexpectedly!")
}
response = self.client.post(url, data=invalid_infraction)
self.assertEqual(response.status_code, 400)
self.assertEqual(response.json(), {

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

I don't believe this is consistent with our code style but honestly it currently looks better than what it's "supposed" to be:

self.assertEqual(
    response.json(), 
    {
        'active': [f'{infraction_type} infractions cannot be active.']
    }
)

It wouldn't be as bad if the dict was on one line:

self.assertEqual(
    response.json(), 
    {'active': [f'{infraction_type} infractions cannot be active.']}
)

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

Oh, so I'm not sure if this should be changed or not. Thoughts?

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 9, 2019

Author Member

I'll change that.

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']

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

Same as line 333.

- `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.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

These two lines seem to be indented a bit too far.


def test_infraction_factory_multiple_users(self):
"""Does the test database hold as many infractions as we tried to create?"""
created_infractions = {}

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

This dictionary doesn't seem to be used anywhere else.

)

# User #1: clean user with no infractions
cls.created_infractions.update(

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

Why use .update() over cls.created_infractions[1] = InfractionFactory.create(...)?

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 9, 2019

Author Member

Good point. Adaptation from an earlier version and it was late.

# User #1: clean user with no infractions
cls.created_infractions.update(
{
1: InfractionFactory.create(

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

Do you think using strings as keys instead of IDs would improve readability, or is it already pretty clear from the test case names which infractions are being used?

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 9, 2019

Author Member

I think strings like "user 1" would be clearer, so I'm going to change that.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

I was thinking something along the lines of "no_infractions", "one_active_note", etc.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 9, 2019

Member

I disliked scrolling back and forth in the module to see what the infractions are supposed to be but I understand the objects need to be created in the hook rather than in the test cases (which would be more ideal for readability).

Copy link
Member

left a comment

  • Migration testing is quite clever. I'm impressed. Nice work!
  • The infractions tests don't strike me as very DRY, though many other tests for the site are probably also guilty of that. Would it be feasible to come up with some functions to make it more DRY, at least for the new infraction tests you've written?
  • While your tests are already thorough, I noticed that MigrationsTestCase does not itself have any tests. Would it be feasible to come up with tests for that? Is it even worth it?
"""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)

This comment has been minimized.

Copy link
@jchristgit

jchristgit Oct 11, 2019

Member

can't we use type__in here instead?

.filter(type__in=('note', 'warning', 'kick'))

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 11, 2019

Author Member

Yes, we can. I'll change that as well.


# 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)

This comment has been minimized.

Copy link
@jchristgit

jchristgit Oct 11, 2019

Member

I am probably missing something, but what's the difference between Q and no Q?

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 11, 2019

Author Member

Nothing. I changed my approach, copy-pasted it, and did not think about it properly. I'm working on an update of this PR, so I'll this here as well.

@jchristgit jchristgit self-assigned this Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.