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

Fix Infraction UniqueTogetherValidator #317

Merged
merged 13 commits into from Feb 8, 2020
Merged

Fix Infraction UniqueTogetherValidator #317

merged 13 commits into from Feb 8, 2020

Conversation

@MarkKoz
Copy link
Member

MarkKoz commented Dec 19, 2019

Must merge python-discord/bot/pull/705 first

The active infractions queryset only gets filtered by the fields specified. This meant that if the same user and type had another infraction instance which was active, the validator would fail. The validator assumes failure if it sees any items still in the queryset after filtering.

By including the active field in the validator, the queryset will be filtered by the active field too. In the case described above, the queryset would end up empty because a no infractions which are active will ever match an active=False filter.

A consequence of doing this is that the active field is now explicitly required - it no longer has a default value.

MarkKoz added 5 commits Dec 19, 2019
The active infractions queryset only gets filtered by the fields
specified. This meant that if the same user and type had another
infraction instance which was active, the validator would fail. The
validator assumes failure if it sees any items still in the queryset
after filtering.

By including the active field in the validator, the queryset will be
filtered by the active field too. In the case described above, the
queryset would end up empty because a no infractions which are active
will ever match an active=False filter.
The fixtures are invoked manually because not all cases may need the
same infractions.
@MarkKoz

This comment has been minimized.

Copy link
Member Author

MarkKoz commented Dec 19, 2019

@SebastiaanZ The side affect of adding active to the validator's fields is that it is now a required field. ModelSerializer does not "inherit" the default value specified in the model, despite active having a default. Setting it in InfractionSerializer like so

class InfractionSerializer(ModelSerializer):
    """A class providing (de-)serialization of `Infraction` instances."""
    active = BooleanField(default=True)

does not work because not all infraction types are allowed to be active. In fact, it's another bug (!!!!) that the validator will incorrectly allow such types to be created as active if the active field is simply not specified in the payload.

The simplest fix is to explicitly require active. This may make sense from a design point of view anyway because it is otherwise confusing that some infraction types need to default to True while others to False. Alternatively, can look into advanced customisation of the serializer to set a default based on the type. The model also currently doesn't have any validation let alone setting the default value correctly based on type, so that's a larger issue.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Dec 19, 2019

Okay, that makes sense. I think I'm in favor of:

The simplest fix is to explicitly require active. This may make sense from a design point of view anyway because it is otherwise confusing that some infraction types need to default to True while others to False.

Using a default value of True when we do not even want to allow that value for some entries in the first place doesn't sound like it passes the principle of least surprise to me. We could, in theory, use a default value of False, since that's a valid value for all infraction types, and have active infractions specify that explicitly (turning the current situation upside down) or just require an explicit value for this field.

Since the active field is fundamental to an infraction's nature, I think that requiring an explicit value makes the most sense: If you accidentally forget to set it, the API complains instead of assuming a value for you.

MarkKoz added 3 commits Dec 19, 2019
Due to the active field being specified in the UniqueTogetherValidator,
the field is implicitly required. Typically default values are excluded
from this restriction but in this case some infraction types must always
be False.

It's easier and makes more sense to require the active field explicitly
rather than to write logic in the serializer which is conditional on the
type of infractions.
@MarkKoz MarkKoz marked this pull request as ready for review Dec 19, 2019
MarkKoz added a commit to python-discord/bot that referenced this pull request Dec 19, 2019
Required due to changes in the API making the active field required.
See python-discord/site/pull/317
MarkKoz added a commit to python-discord/bot that referenced this pull request Dec 19, 2019
Required due to changes in the API making the active field required.
See python-discord/site/pull/317
MarkKoz added 3 commits Dec 20, 2019
@jos-b jos-b requested review from python-discord/core-developers and eivl and removed request for python-discord/core-developers and eivl Feb 2, 2020
@jos-b jos-b requested a review from lemonsaurus Feb 2, 2020
Copy link
Member

SebastiaanZ left a comment

Nice work.

@MarkKoz MarkKoz requested a review from python-discord/core-developers as a code owner Feb 8, 2020
@MarkKoz MarkKoz merged commit b2f5a7a into master Feb 8, 2020
2 checks passed
2 checks passed
Site Build #20200208.2 succeeded
Details
Site (Test & Lint) Test & Lint succeeded
Details
@MarkKoz MarkKoz deleted the fix-infraction-validator branch Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.